From 5640df7f6379673631563e79748a73cda663f940 Mon Sep 17 00:00:00 2001 From: rkoster Date: Thu, 5 Mar 2026 08:25:37 +0000 Subject: [PATCH 01/11] Add allowed_sources support for mTLS app-to-app routing - Add app_to_app_mtls_routing feature flag (default: false) - Add allowed_sources to RouteOptionsMessage with validation - Validate allowed_sources structure (apps/spaces/orgs arrays, any boolean) - Validate that app/space/org GUIDs exist in database - Enforce mutual exclusivity of 'any' with apps/spaces/orgs lists --- app/messages/route_options_message.rb | 94 ++++++++++++++++++++++++++- app/models/runtime/feature_flag.rb | 3 +- 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index f79a2bb6a0c..b45d0462c95 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -3,11 +3,12 @@ module VCAP::CloudController class RouteOptionsMessage < BaseMessage # Register all possible keys upfront so attr_accessors are created - register_allowed_keys %i[loadbalancing hash_header hash_balance] + register_allowed_keys %i[loadbalancing hash_header hash_balance allowed_sources] def self.valid_route_options options = %i[loadbalancing] options += %i[hash_header hash_balance] if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + options += %i[allowed_sources] if VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) options.freeze end @@ -21,6 +22,7 @@ def self.valid_loadbalancing_algorithms validate :loadbalancing_algorithm_is_valid validate :route_options_are_valid validate :hash_options_are_valid + validate :allowed_sources_options_are_valid def loadbalancing_algorithm_is_valid return if loadbalancing.blank? @@ -82,5 +84,95 @@ def validate_hash_options_with_loadbalancing errors.add(:base, 'Hash header can only be set when loadbalancing is hash') if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' end + + def allowed_sources_options_are_valid + # Only validate allowed_sources when the feature flag is enabled + # If disabled, route_options_are_valid will already report it as unknown field + return unless VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) + return if allowed_sources.blank? + + validate_allowed_sources_structure + validate_allowed_sources_any_exclusivity + validate_allowed_sources_guids_exist + end + + private + + def validate_allowed_sources_structure + unless allowed_sources.is_a?(Hash) + errors.add(:allowed_sources, 'must be an object') + return + end + + valid_keys = %w[apps spaces orgs any] + invalid_keys = allowed_sources.keys - valid_keys + errors.add(:allowed_sources, "contains invalid keys: #{invalid_keys.join(', ')}") if invalid_keys.any? + + # Validate types + %w[apps spaces orgs].each do |key| + next unless allowed_sources[key].present? + + unless allowed_sources[key].is_a?(Array) && allowed_sources[key].all? { |v| v.is_a?(String) } + errors.add(:allowed_sources, "#{key} must be an array of strings") + end + end + + return unless allowed_sources['any'].present? && ![true, false].include?(allowed_sources['any']) + + errors.add(:allowed_sources, 'any must be a boolean') + end + + def validate_allowed_sources_any_exclusivity + return unless allowed_sources.is_a?(Hash) + + has_any = allowed_sources['any'] == true + has_lists = %w[apps spaces orgs].any? { |key| allowed_sources[key].present? && allowed_sources[key].any? } + + return unless has_any && has_lists + + errors.add(:allowed_sources, 'any is mutually exclusive with apps, spaces, and orgs') + end + + def validate_allowed_sources_guids_exist + return unless allowed_sources.is_a?(Hash) + return if errors[:allowed_sources].any? # Skip if already invalid + + validate_app_guids_exist + validate_space_guids_exist + validate_org_guids_exist + end + + def validate_app_guids_exist + app_guids = allowed_sources['apps'] + return if app_guids.blank? + + existing_guids = AppModel.where(guid: app_guids).select_map(:guid) + missing_guids = app_guids - existing_guids + return if missing_guids.empty? + + errors.add(:allowed_sources, "apps contains non-existent app GUIDs: #{missing_guids.join(', ')}") + end + + def validate_space_guids_exist + space_guids = allowed_sources['spaces'] + return if space_guids.blank? + + existing_guids = Space.where(guid: space_guids).select_map(:guid) + missing_guids = space_guids - existing_guids + return if missing_guids.empty? + + errors.add(:allowed_sources, "spaces contains non-existent space GUIDs: #{missing_guids.join(', ')}") + end + + def validate_org_guids_exist + org_guids = allowed_sources['orgs'] + return if org_guids.blank? + + existing_guids = Organization.where(guid: org_guids).select_map(:guid) + missing_guids = org_guids - existing_guids + return if missing_guids.empty? + + errors.add(:allowed_sources, "orgs contains non-existent organization GUIDs: #{missing_guids.join(', ')}") + end end end diff --git a/app/models/runtime/feature_flag.rb b/app/models/runtime/feature_flag.rb index df991d89f7e..da4bc026f2c 100644 --- a/app/models/runtime/feature_flag.rb +++ b/app/models/runtime/feature_flag.rb @@ -24,7 +24,8 @@ class UndefinedFeatureFlagError < StandardError hide_marketplace_from_unauthenticated_users: false, resource_matching: true, route_sharing: false, - hash_based_routing: false + hash_based_routing: false, + app_to_app_mtls_routing: false }.freeze ADMIN_SKIPPABLE = %i[ From 223902c0c3ec5f71e90e8b7310d53696e637edef Mon Sep 17 00:00:00 2001 From: rkoster Date: Thu, 5 Mar 2026 08:26:35 +0000 Subject: [PATCH 02/11] Add unit tests for allowed_sources validation Tests cover: - Feature flag disabled: allowed_sources rejected as unknown field - Structure validation: object type, valid keys, array types, boolean any - any exclusivity: cannot combine any:true with apps/spaces/orgs lists - GUID existence validation: apps, spaces, orgs must exist in database - Combined options: allowed_sources works with loadbalancing --- .../messages/route_options_message_spec.rb | 198 ++++++++++++++++++ 1 file changed, 198 insertions(+) diff --git a/spec/unit/messages/route_options_message_spec.rb b/spec/unit/messages/route_options_message_spec.rb index 57646d21950..aa60e654deb 100644 --- a/spec/unit/messages/route_options_message_spec.rb +++ b/spec/unit/messages/route_options_message_spec.rb @@ -37,6 +37,204 @@ module VCAP::CloudController end end + describe 'allowed_sources validations' do + context 'when app_to_app_mtls_routing feature flag is disabled' do + it 'does not allow allowed_sources option' do + message = RouteOptionsMessage.new({ allowed_sources: { apps: ['app-guid-1'] } }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'allowed_sources'") + end + end + + context 'when app_to_app_mtls_routing feature flag is enabled' do + before do + VCAP::CloudController::FeatureFlag.make(name: 'app_to_app_mtls_routing', enabled: true) + end + + describe 'structure validation' do + it 'allows valid allowed_sources with apps' do + app = AppModel.make + message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => [app.guid] } }) + expect(message).to be_valid + end + + it 'allows valid allowed_sources with spaces' do + space = Space.make + message = RouteOptionsMessage.new({ allowed_sources: { 'spaces' => [space.guid] } }) + expect(message).to be_valid + end + + it 'allows valid allowed_sources with orgs' do + org = Organization.make + message = RouteOptionsMessage.new({ allowed_sources: { 'orgs' => [org.guid] } }) + expect(message).to be_valid + end + + it 'allows valid allowed_sources with any: true' do + message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true } }) + expect(message).to be_valid + end + + it 'allows valid allowed_sources with any: false' do + message = RouteOptionsMessage.new({ allowed_sources: { 'any' => false } }) + expect(message).to be_valid + end + + it 'allows empty allowed_sources object' do + message = RouteOptionsMessage.new({ allowed_sources: {} }) + expect(message).to be_valid + end + + it 'does not allow non-object allowed_sources' do + message = RouteOptionsMessage.new({ allowed_sources: 'invalid' }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('must be an object') + end + + it 'does not allow array allowed_sources' do + message = RouteOptionsMessage.new({ allowed_sources: ['app-guid-1'] }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('must be an object') + end + + it 'does not allow invalid keys in allowed_sources' do + message = RouteOptionsMessage.new({ allowed_sources: { 'invalid_key' => 'value' } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('contains invalid keys: invalid_key') + end + + it 'does not allow non-array apps' do + message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => 'not-an-array' } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('apps must be an array of strings') + end + + it 'does not allow non-string elements in apps array' do + message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => [123, 456] } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('apps must be an array of strings') + end + + it 'does not allow non-array spaces' do + message = RouteOptionsMessage.new({ allowed_sources: { 'spaces' => 'not-an-array' } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('spaces must be an array of strings') + end + + it 'does not allow non-array orgs' do + message = RouteOptionsMessage.new({ allowed_sources: { 'orgs' => 'not-an-array' } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('orgs must be an array of strings') + end + + it 'does not allow non-boolean any' do + message = RouteOptionsMessage.new({ allowed_sources: { 'any' => 'true' } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('any must be a boolean') + end + end + + describe 'any exclusivity validation' do + it 'does not allow any: true with apps list' do + app = AppModel.make + message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true, 'apps' => [app.guid] } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + end + + it 'does not allow any: true with spaces list' do + space = Space.make + message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true, 'spaces' => [space.guid] } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + end + + it 'does not allow any: true with orgs list' do + org = Organization.make + message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true, 'orgs' => [org.guid] } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + end + + it 'allows any: false with apps list' do + app = AppModel.make + message = RouteOptionsMessage.new({ allowed_sources: { 'any' => false, 'apps' => [app.guid] } }) + expect(message).to be_valid + end + + it 'allows any: true with empty apps list' do + message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true, 'apps' => [] } }) + expect(message).to be_valid + end + end + + describe 'GUID existence validation' do + it 'validates that app GUIDs exist' do + message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => ['non-existent-app-guid'] } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('apps contains non-existent app GUIDs: non-existent-app-guid') + end + + it 'validates that space GUIDs exist' do + message = RouteOptionsMessage.new({ allowed_sources: { 'spaces' => ['non-existent-space-guid'] } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('spaces contains non-existent space GUIDs: non-existent-space-guid') + end + + it 'validates that org GUIDs exist' do + message = RouteOptionsMessage.new({ allowed_sources: { 'orgs' => ['non-existent-org-guid'] } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('orgs contains non-existent organization GUIDs: non-existent-org-guid') + end + + it 'reports multiple non-existent app GUIDs' do + message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => ['guid-1', 'guid-2'] } }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('apps contains non-existent app GUIDs: guid-1, guid-2') + end + + it 'allows mix of existing apps, spaces, and orgs' do + app = AppModel.make + space = Space.make + org = Organization.make + message = RouteOptionsMessage.new({ + allowed_sources: { + 'apps' => [app.guid], + 'spaces' => [space.guid], + 'orgs' => [org.guid] + } + }) + expect(message).to be_valid + end + + it 'validates all types of GUIDs when multiple are provided' do + app = AppModel.make + message = RouteOptionsMessage.new({ + allowed_sources: { + 'apps' => [app.guid], + 'spaces' => ['non-existent-space'], + 'orgs' => ['non-existent-org'] + } + }) + expect(message).not_to be_valid + expect(message.errors_on(:allowed_sources)).to include('spaces contains non-existent space GUIDs: non-existent-space') + expect(message.errors_on(:allowed_sources)).to include('orgs contains non-existent organization GUIDs: non-existent-org') + end + end + + describe 'combined with other options' do + it 'allows allowed_sources with loadbalancing' do + app = AppModel.make + message = RouteOptionsMessage.new({ + loadbalancing: 'round-robin', + allowed_sources: { 'apps' => [app.guid] } + }) + expect(message).to be_valid + end + end + end + end + describe 'hash-based routing validations' do context 'when hash_based_routing feature flag is disabled' do it 'does not allow hash_header option' do From f560db569090ecbdacced340e6cb4a2692d7e305 Mon Sep 17 00:00:00 2001 From: rkoster Date: Thu, 5 Mar 2026 08:39:30 +0000 Subject: [PATCH 03/11] Fix allowed_sources validation to handle symbol keys Rails parses JSON with symbol keys, but validation was comparing against string keys. Add normalized_allowed_sources helper to transform keys to strings for consistent comparison. --- app/messages/route_options_message.rb | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index b45d0462c95..6983d7e7012 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -98,6 +98,11 @@ def allowed_sources_options_are_valid private + # Normalize allowed_sources to use string keys (Rails may parse JSON with symbol keys) + def normalized_allowed_sources + @normalized_allowed_sources ||= allowed_sources.is_a?(Hash) ? allowed_sources.transform_keys(&:to_s) : allowed_sources + end + def validate_allowed_sources_structure unless allowed_sources.is_a?(Hash) errors.add(:allowed_sources, 'must be an object') @@ -105,19 +110,19 @@ def validate_allowed_sources_structure end valid_keys = %w[apps spaces orgs any] - invalid_keys = allowed_sources.keys - valid_keys + invalid_keys = normalized_allowed_sources.keys - valid_keys errors.add(:allowed_sources, "contains invalid keys: #{invalid_keys.join(', ')}") if invalid_keys.any? # Validate types %w[apps spaces orgs].each do |key| - next unless allowed_sources[key].present? + next unless normalized_allowed_sources[key].present? - unless allowed_sources[key].is_a?(Array) && allowed_sources[key].all? { |v| v.is_a?(String) } + unless normalized_allowed_sources[key].is_a?(Array) && normalized_allowed_sources[key].all? { |v| v.is_a?(String) } errors.add(:allowed_sources, "#{key} must be an array of strings") end end - return unless allowed_sources['any'].present? && ![true, false].include?(allowed_sources['any']) + return unless normalized_allowed_sources['any'].present? && ![true, false].include?(normalized_allowed_sources['any']) errors.add(:allowed_sources, 'any must be a boolean') end @@ -125,8 +130,8 @@ def validate_allowed_sources_structure def validate_allowed_sources_any_exclusivity return unless allowed_sources.is_a?(Hash) - has_any = allowed_sources['any'] == true - has_lists = %w[apps spaces orgs].any? { |key| allowed_sources[key].present? && allowed_sources[key].any? } + has_any = normalized_allowed_sources['any'] == true + has_lists = %w[apps spaces orgs].any? { |key| normalized_allowed_sources[key].present? && normalized_allowed_sources[key].any? } return unless has_any && has_lists @@ -143,7 +148,7 @@ def validate_allowed_sources_guids_exist end def validate_app_guids_exist - app_guids = allowed_sources['apps'] + app_guids = normalized_allowed_sources['apps'] return if app_guids.blank? existing_guids = AppModel.where(guid: app_guids).select_map(:guid) @@ -154,7 +159,7 @@ def validate_app_guids_exist end def validate_space_guids_exist - space_guids = allowed_sources['spaces'] + space_guids = normalized_allowed_sources['spaces'] return if space_guids.blank? existing_guids = Space.where(guid: space_guids).select_map(:guid) @@ -165,7 +170,7 @@ def validate_space_guids_exist end def validate_org_guids_exist - org_guids = allowed_sources['orgs'] + org_guids = normalized_allowed_sources['orgs'] return if org_guids.blank? existing_guids = Organization.where(guid: org_guids).select_map(:guid) From 936d4dd8bd0520bfc4a4b4c08d0983f76a103bc3 Mon Sep 17 00:00:00 2001 From: rkoster Date: Thu, 5 Mar 2026 10:01:09 +0000 Subject: [PATCH 04/11] Rename allowed_sources to mtls_allowed_sources for clarity Rename the route options field from allowed_sources to mtls_allowed_sources for better clarity about its purpose in mTLS app-to-app routing. Updates RouteOptionsMessage to use the new field name in: - Allowed keys registration - Feature flag gating - Validation methods - All related tests --- app/messages/route_options_message.rb | 72 +++++------ .../messages/route_options_message_spec.rb | 114 +++++++++--------- 2 files changed, 93 insertions(+), 93 deletions(-) diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 6983d7e7012..ab688c6bbb6 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -3,12 +3,12 @@ module VCAP::CloudController class RouteOptionsMessage < BaseMessage # Register all possible keys upfront so attr_accessors are created - register_allowed_keys %i[loadbalancing hash_header hash_balance allowed_sources] + register_allowed_keys %i[loadbalancing hash_header hash_balance mtls_allowed_sources] def self.valid_route_options options = %i[loadbalancing] options += %i[hash_header hash_balance] if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) - options += %i[allowed_sources] if VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) + options += %i[mtls_allowed_sources] if VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) options.freeze end @@ -22,7 +22,7 @@ def self.valid_loadbalancing_algorithms validate :loadbalancing_algorithm_is_valid validate :route_options_are_valid validate :hash_options_are_valid - validate :allowed_sources_options_are_valid + validate :mtls_allowed_sources_options_are_valid def loadbalancing_algorithm_is_valid return if loadbalancing.blank? @@ -85,62 +85,62 @@ def validate_hash_options_with_loadbalancing errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' end - def allowed_sources_options_are_valid - # Only validate allowed_sources when the feature flag is enabled + def mtls_allowed_sources_options_are_valid + # Only validate mtls_allowed_sources when the feature flag is enabled # If disabled, route_options_are_valid will already report it as unknown field return unless VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) - return if allowed_sources.blank? + return if mtls_allowed_sources.blank? - validate_allowed_sources_structure - validate_allowed_sources_any_exclusivity - validate_allowed_sources_guids_exist + validate_mtls_allowed_sources_structure + validate_mtls_allowed_sources_any_exclusivity + validate_mtls_allowed_sources_guids_exist end private - # Normalize allowed_sources to use string keys (Rails may parse JSON with symbol keys) - def normalized_allowed_sources - @normalized_allowed_sources ||= allowed_sources.is_a?(Hash) ? allowed_sources.transform_keys(&:to_s) : allowed_sources + # Normalize mtls_allowed_sources to use string keys (Rails may parse JSON with symbol keys) + def normalized_mtls_allowed_sources + @normalized_mtls_allowed_sources ||= mtls_allowed_sources.is_a?(Hash) ? mtls_allowed_sources.transform_keys(&:to_s) : mtls_allowed_sources end - def validate_allowed_sources_structure - unless allowed_sources.is_a?(Hash) - errors.add(:allowed_sources, 'must be an object') + def validate_mtls_allowed_sources_structure + unless mtls_allowed_sources.is_a?(Hash) + errors.add(:mtls_allowed_sources, 'must be an object') return end valid_keys = %w[apps spaces orgs any] - invalid_keys = normalized_allowed_sources.keys - valid_keys - errors.add(:allowed_sources, "contains invalid keys: #{invalid_keys.join(', ')}") if invalid_keys.any? + invalid_keys = normalized_mtls_allowed_sources.keys - valid_keys + errors.add(:mtls_allowed_sources, "contains invalid keys: #{invalid_keys.join(', ')}") if invalid_keys.any? # Validate types %w[apps spaces orgs].each do |key| - next unless normalized_allowed_sources[key].present? + next unless normalized_mtls_allowed_sources[key].present? - unless normalized_allowed_sources[key].is_a?(Array) && normalized_allowed_sources[key].all? { |v| v.is_a?(String) } - errors.add(:allowed_sources, "#{key} must be an array of strings") + unless normalized_mtls_allowed_sources[key].is_a?(Array) && normalized_mtls_allowed_sources[key].all? { |v| v.is_a?(String) } + errors.add(:mtls_allowed_sources, "#{key} must be an array of strings") end end - return unless normalized_allowed_sources['any'].present? && ![true, false].include?(normalized_allowed_sources['any']) + return unless normalized_mtls_allowed_sources['any'].present? && ![true, false].include?(normalized_mtls_allowed_sources['any']) - errors.add(:allowed_sources, 'any must be a boolean') + errors.add(:mtls_allowed_sources, 'any must be a boolean') end - def validate_allowed_sources_any_exclusivity - return unless allowed_sources.is_a?(Hash) + def validate_mtls_allowed_sources_any_exclusivity + return unless mtls_allowed_sources.is_a?(Hash) - has_any = normalized_allowed_sources['any'] == true - has_lists = %w[apps spaces orgs].any? { |key| normalized_allowed_sources[key].present? && normalized_allowed_sources[key].any? } + has_any = normalized_mtls_allowed_sources['any'] == true + has_lists = %w[apps spaces orgs].any? { |key| normalized_mtls_allowed_sources[key].present? && normalized_mtls_allowed_sources[key].any? } return unless has_any && has_lists - errors.add(:allowed_sources, 'any is mutually exclusive with apps, spaces, and orgs') + errors.add(:mtls_allowed_sources, 'any is mutually exclusive with apps, spaces, and orgs') end - def validate_allowed_sources_guids_exist - return unless allowed_sources.is_a?(Hash) - return if errors[:allowed_sources].any? # Skip if already invalid + def validate_mtls_allowed_sources_guids_exist + return unless mtls_allowed_sources.is_a?(Hash) + return if errors[:mtls_allowed_sources].any? # Skip if already invalid validate_app_guids_exist validate_space_guids_exist @@ -148,36 +148,36 @@ def validate_allowed_sources_guids_exist end def validate_app_guids_exist - app_guids = normalized_allowed_sources['apps'] + app_guids = normalized_mtls_allowed_sources['apps'] return if app_guids.blank? existing_guids = AppModel.where(guid: app_guids).select_map(:guid) missing_guids = app_guids - existing_guids return if missing_guids.empty? - errors.add(:allowed_sources, "apps contains non-existent app GUIDs: #{missing_guids.join(', ')}") + errors.add(:mtls_allowed_sources, "apps contains non-existent app GUIDs: #{missing_guids.join(', ')}") end def validate_space_guids_exist - space_guids = normalized_allowed_sources['spaces'] + space_guids = normalized_mtls_allowed_sources['spaces'] return if space_guids.blank? existing_guids = Space.where(guid: space_guids).select_map(:guid) missing_guids = space_guids - existing_guids return if missing_guids.empty? - errors.add(:allowed_sources, "spaces contains non-existent space GUIDs: #{missing_guids.join(', ')}") + errors.add(:mtls_allowed_sources, "spaces contains non-existent space GUIDs: #{missing_guids.join(', ')}") end def validate_org_guids_exist - org_guids = normalized_allowed_sources['orgs'] + org_guids = normalized_mtls_allowed_sources['orgs'] return if org_guids.blank? existing_guids = Organization.where(guid: org_guids).select_map(:guid) missing_guids = org_guids - existing_guids return if missing_guids.empty? - errors.add(:allowed_sources, "orgs contains non-existent organization GUIDs: #{missing_guids.join(', ')}") + errors.add(:mtls_allowed_sources, "orgs contains non-existent organization GUIDs: #{missing_guids.join(', ')}") end end end diff --git a/spec/unit/messages/route_options_message_spec.rb b/spec/unit/messages/route_options_message_spec.rb index aa60e654deb..c9d86df339c 100644 --- a/spec/unit/messages/route_options_message_spec.rb +++ b/spec/unit/messages/route_options_message_spec.rb @@ -37,12 +37,12 @@ module VCAP::CloudController end end - describe 'allowed_sources validations' do + describe 'mtls_allowed_sources validations' do context 'when app_to_app_mtls_routing feature flag is disabled' do - it 'does not allow allowed_sources option' do - message = RouteOptionsMessage.new({ allowed_sources: { apps: ['app-guid-1'] } }) + it 'does not allow mtls_allowed_sources option' do + message = RouteOptionsMessage.new({ mtls_allowed_sources: { apps: ['app-guid-1'] } }) expect(message).not_to be_valid - expect(message.errors_on(:base)).to include("Unknown field(s): 'allowed_sources'") + expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allowed_sources'") end end @@ -52,145 +52,145 @@ module VCAP::CloudController end describe 'structure validation' do - it 'allows valid allowed_sources with apps' do + it 'allows valid mtls_allowed_sources with apps' do app = AppModel.make - message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => [app.guid] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => [app.guid] } }) expect(message).to be_valid end - it 'allows valid allowed_sources with spaces' do + it 'allows valid mtls_allowed_sources with spaces' do space = Space.make - message = RouteOptionsMessage.new({ allowed_sources: { 'spaces' => [space.guid] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'spaces' => [space.guid] } }) expect(message).to be_valid end - it 'allows valid allowed_sources with orgs' do + it 'allows valid mtls_allowed_sources with orgs' do org = Organization.make - message = RouteOptionsMessage.new({ allowed_sources: { 'orgs' => [org.guid] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'orgs' => [org.guid] } }) expect(message).to be_valid end - it 'allows valid allowed_sources with any: true' do - message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true } }) + it 'allows valid mtls_allowed_sources with any: true' do + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true } }) expect(message).to be_valid end - it 'allows valid allowed_sources with any: false' do - message = RouteOptionsMessage.new({ allowed_sources: { 'any' => false } }) + it 'allows valid mtls_allowed_sources with any: false' do + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => false } }) expect(message).to be_valid end - it 'allows empty allowed_sources object' do - message = RouteOptionsMessage.new({ allowed_sources: {} }) + it 'allows empty mtls_allowed_sources object' do + message = RouteOptionsMessage.new({ mtls_allowed_sources: {} }) expect(message).to be_valid end - it 'does not allow non-object allowed_sources' do - message = RouteOptionsMessage.new({ allowed_sources: 'invalid' }) + it 'does not allow non-object mtls_allowed_sources' do + message = RouteOptionsMessage.new({ mtls_allowed_sources: 'invalid' }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('must be an object') + expect(message.errors_on(:mtls_allowed_sources)).to include('must be an object') end - it 'does not allow array allowed_sources' do - message = RouteOptionsMessage.new({ allowed_sources: ['app-guid-1'] }) + it 'does not allow array mtls_allowed_sources' do + message = RouteOptionsMessage.new({ mtls_allowed_sources: ['app-guid-1'] }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('must be an object') + expect(message.errors_on(:mtls_allowed_sources)).to include('must be an object') end - it 'does not allow invalid keys in allowed_sources' do - message = RouteOptionsMessage.new({ allowed_sources: { 'invalid_key' => 'value' } }) + it 'does not allow invalid keys in mtls_allowed_sources' do + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'invalid_key' => 'value' } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('contains invalid keys: invalid_key') + expect(message.errors_on(:mtls_allowed_sources)).to include('contains invalid keys: invalid_key') end it 'does not allow non-array apps' do - message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => 'not-an-array' } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => 'not-an-array' } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('apps must be an array of strings') + expect(message.errors_on(:mtls_allowed_sources)).to include('apps must be an array of strings') end it 'does not allow non-string elements in apps array' do - message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => [123, 456] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => [123, 456] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('apps must be an array of strings') + expect(message.errors_on(:mtls_allowed_sources)).to include('apps must be an array of strings') end it 'does not allow non-array spaces' do - message = RouteOptionsMessage.new({ allowed_sources: { 'spaces' => 'not-an-array' } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'spaces' => 'not-an-array' } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('spaces must be an array of strings') + expect(message.errors_on(:mtls_allowed_sources)).to include('spaces must be an array of strings') end it 'does not allow non-array orgs' do - message = RouteOptionsMessage.new({ allowed_sources: { 'orgs' => 'not-an-array' } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'orgs' => 'not-an-array' } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('orgs must be an array of strings') + expect(message.errors_on(:mtls_allowed_sources)).to include('orgs must be an array of strings') end it 'does not allow non-boolean any' do - message = RouteOptionsMessage.new({ allowed_sources: { 'any' => 'true' } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => 'true' } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('any must be a boolean') + expect(message.errors_on(:mtls_allowed_sources)).to include('any must be a boolean') end end describe 'any exclusivity validation' do it 'does not allow any: true with apps list' do app = AppModel.make - message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true, 'apps' => [app.guid] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true, 'apps' => [app.guid] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + expect(message.errors_on(:mtls_allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') end it 'does not allow any: true with spaces list' do space = Space.make - message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true, 'spaces' => [space.guid] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true, 'spaces' => [space.guid] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + expect(message.errors_on(:mtls_allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') end it 'does not allow any: true with orgs list' do org = Organization.make - message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true, 'orgs' => [org.guid] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true, 'orgs' => [org.guid] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + expect(message.errors_on(:mtls_allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') end it 'allows any: false with apps list' do app = AppModel.make - message = RouteOptionsMessage.new({ allowed_sources: { 'any' => false, 'apps' => [app.guid] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => false, 'apps' => [app.guid] } }) expect(message).to be_valid end it 'allows any: true with empty apps list' do - message = RouteOptionsMessage.new({ allowed_sources: { 'any' => true, 'apps' => [] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true, 'apps' => [] } }) expect(message).to be_valid end end describe 'GUID existence validation' do it 'validates that app GUIDs exist' do - message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => ['non-existent-app-guid'] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => ['non-existent-app-guid'] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('apps contains non-existent app GUIDs: non-existent-app-guid') + expect(message.errors_on(:mtls_allowed_sources)).to include('apps contains non-existent app GUIDs: non-existent-app-guid') end it 'validates that space GUIDs exist' do - message = RouteOptionsMessage.new({ allowed_sources: { 'spaces' => ['non-existent-space-guid'] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'spaces' => ['non-existent-space-guid'] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('spaces contains non-existent space GUIDs: non-existent-space-guid') + expect(message.errors_on(:mtls_allowed_sources)).to include('spaces contains non-existent space GUIDs: non-existent-space-guid') end it 'validates that org GUIDs exist' do - message = RouteOptionsMessage.new({ allowed_sources: { 'orgs' => ['non-existent-org-guid'] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'orgs' => ['non-existent-org-guid'] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('orgs contains non-existent organization GUIDs: non-existent-org-guid') + expect(message.errors_on(:mtls_allowed_sources)).to include('orgs contains non-existent organization GUIDs: non-existent-org-guid') end it 'reports multiple non-existent app GUIDs' do - message = RouteOptionsMessage.new({ allowed_sources: { 'apps' => ['guid-1', 'guid-2'] } }) + message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => ['guid-1', 'guid-2'] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('apps contains non-existent app GUIDs: guid-1, guid-2') + expect(message.errors_on(:mtls_allowed_sources)).to include('apps contains non-existent app GUIDs: guid-1, guid-2') end it 'allows mix of existing apps, spaces, and orgs' do @@ -198,7 +198,7 @@ module VCAP::CloudController space = Space.make org = Organization.make message = RouteOptionsMessage.new({ - allowed_sources: { + mtls_allowed_sources: { 'apps' => [app.guid], 'spaces' => [space.guid], 'orgs' => [org.guid] @@ -210,24 +210,24 @@ module VCAP::CloudController it 'validates all types of GUIDs when multiple are provided' do app = AppModel.make message = RouteOptionsMessage.new({ - allowed_sources: { + mtls_allowed_sources: { 'apps' => [app.guid], 'spaces' => ['non-existent-space'], 'orgs' => ['non-existent-org'] } }) expect(message).not_to be_valid - expect(message.errors_on(:allowed_sources)).to include('spaces contains non-existent space GUIDs: non-existent-space') - expect(message.errors_on(:allowed_sources)).to include('orgs contains non-existent organization GUIDs: non-existent-org') + expect(message.errors_on(:mtls_allowed_sources)).to include('spaces contains non-existent space GUIDs: non-existent-space') + expect(message.errors_on(:mtls_allowed_sources)).to include('orgs contains non-existent organization GUIDs: non-existent-org') end end describe 'combined with other options' do - it 'allows allowed_sources with loadbalancing' do + it 'allows mtls_allowed_sources with loadbalancing' do app = AppModel.make message = RouteOptionsMessage.new({ loadbalancing: 'round-robin', - allowed_sources: { 'apps' => [app.guid] } + mtls_allowed_sources: { 'apps' => [app.guid] } }) expect(message).to be_valid end From 97470469ca3a223e8616e8a588b17b5fe8d41f80 Mon Sep 17 00:00:00 2001 From: rkoster Date: Thu, 5 Mar 2026 15:06:16 +0000 Subject: [PATCH 05/11] Refactor mTLS route options to RFC-0027 compliant flat format Change from nested mtls_allowed_sources object to flat options: - mtls_allowed_apps: comma-separated app GUIDs (string) - mtls_allowed_spaces: comma-separated space GUIDs (string) - mtls_allowed_orgs: comma-separated org GUIDs (string) - mtls_allow_any: boolean (true/false) This complies with RFC-0027 which requires route options to only use numbers, strings, and boolean values (no nested objects or arrays). --- app/messages/route_options_message.rb | 92 ++++--- .../messages/route_options_message_spec.rb | 235 +++++++++--------- 2 files changed, 168 insertions(+), 159 deletions(-) diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index ab688c6bbb6..c8b6d82a115 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -3,12 +3,15 @@ module VCAP::CloudController class RouteOptionsMessage < BaseMessage # Register all possible keys upfront so attr_accessors are created - register_allowed_keys %i[loadbalancing hash_header hash_balance mtls_allowed_sources] + # RFC-0027 compliant: only string/number/boolean values (no nested objects/arrays) + # mtls_allowed_apps, mtls_allowed_spaces, mtls_allowed_orgs are comma-separated GUIDs + # mtls_allow_any is a boolean + register_allowed_keys %i[loadbalancing hash_header hash_balance mtls_allowed_apps mtls_allowed_spaces mtls_allowed_orgs mtls_allow_any] def self.valid_route_options options = %i[loadbalancing] options += %i[hash_header hash_balance] if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) - options += %i[mtls_allowed_sources] if VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) + options += %i[mtls_allowed_apps mtls_allowed_spaces mtls_allowed_orgs mtls_allow_any] if VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) options.freeze end @@ -86,61 +89,56 @@ def validate_hash_options_with_loadbalancing end def mtls_allowed_sources_options_are_valid - # Only validate mtls_allowed_sources when the feature flag is enabled - # If disabled, route_options_are_valid will already report it as unknown field + # Only validate mtls options when the feature flag is enabled + # If disabled, route_options_are_valid will already report them as unknown fields return unless VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) - return if mtls_allowed_sources.blank? - validate_mtls_allowed_sources_structure - validate_mtls_allowed_sources_any_exclusivity - validate_mtls_allowed_sources_guids_exist + validate_mtls_string_types + validate_mtls_allow_any_type + validate_mtls_allow_any_exclusivity + validate_mtls_guids_exist end private - # Normalize mtls_allowed_sources to use string keys (Rails may parse JSON with symbol keys) - def normalized_mtls_allowed_sources - @normalized_mtls_allowed_sources ||= mtls_allowed_sources.is_a?(Hash) ? mtls_allowed_sources.transform_keys(&:to_s) : mtls_allowed_sources - end - - def validate_mtls_allowed_sources_structure - unless mtls_allowed_sources.is_a?(Hash) - errors.add(:mtls_allowed_sources, 'must be an object') - return - end + # Parse comma-separated GUIDs into an array + def parse_guid_list(value) + return [] if value.blank? - valid_keys = %w[apps spaces orgs any] - invalid_keys = normalized_mtls_allowed_sources.keys - valid_keys - errors.add(:mtls_allowed_sources, "contains invalid keys: #{invalid_keys.join(', ')}") if invalid_keys.any? + value.to_s.split(',').map(&:strip).reject(&:empty?) + end - # Validate types - %w[apps spaces orgs].each do |key| - next unless normalized_mtls_allowed_sources[key].present? + def validate_mtls_string_types + # These should be strings (comma-separated GUIDs) per RFC-0027 + %i[mtls_allowed_apps mtls_allowed_spaces mtls_allowed_orgs].each do |key| + value = public_send(key) + next if value.blank? - unless normalized_mtls_allowed_sources[key].is_a?(Array) && normalized_mtls_allowed_sources[key].all? { |v| v.is_a?(String) } - errors.add(:mtls_allowed_sources, "#{key} must be an array of strings") + unless value.is_a?(String) + errors.add(key, 'must be a string of comma-separated GUIDs') end end + end - return unless normalized_mtls_allowed_sources['any'].present? && ![true, false].include?(normalized_mtls_allowed_sources['any']) + def validate_mtls_allow_any_type + return if mtls_allow_any.nil? - errors.add(:mtls_allowed_sources, 'any must be a boolean') + unless [true, false, 'true', 'false'].include?(mtls_allow_any) + errors.add(:mtls_allow_any, 'must be a boolean (true or false)') + end end - def validate_mtls_allowed_sources_any_exclusivity - return unless mtls_allowed_sources.is_a?(Hash) - - has_any = normalized_mtls_allowed_sources['any'] == true - has_lists = %w[apps spaces orgs].any? { |key| normalized_mtls_allowed_sources[key].present? && normalized_mtls_allowed_sources[key].any? } + def validate_mtls_allow_any_exclusivity + allow_any = mtls_allow_any == true || mtls_allow_any == 'true' + has_specific = [mtls_allowed_apps, mtls_allowed_spaces, mtls_allowed_orgs].any?(&:present?) - return unless has_any && has_lists + return unless allow_any && has_specific - errors.add(:mtls_allowed_sources, 'any is mutually exclusive with apps, spaces, and orgs') + errors.add(:mtls_allow_any, 'is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') end - def validate_mtls_allowed_sources_guids_exist - return unless mtls_allowed_sources.is_a?(Hash) - return if errors[:mtls_allowed_sources].any? # Skip if already invalid + def validate_mtls_guids_exist + return if errors.any? # Skip if already invalid validate_app_guids_exist validate_space_guids_exist @@ -148,36 +146,36 @@ def validate_mtls_allowed_sources_guids_exist end def validate_app_guids_exist - app_guids = normalized_mtls_allowed_sources['apps'] - return if app_guids.blank? + app_guids = parse_guid_list(mtls_allowed_apps) + return if app_guids.empty? existing_guids = AppModel.where(guid: app_guids).select_map(:guid) missing_guids = app_guids - existing_guids return if missing_guids.empty? - errors.add(:mtls_allowed_sources, "apps contains non-existent app GUIDs: #{missing_guids.join(', ')}") + errors.add(:mtls_allowed_apps, "contains non-existent app GUIDs: #{missing_guids.join(', ')}") end def validate_space_guids_exist - space_guids = normalized_mtls_allowed_sources['spaces'] - return if space_guids.blank? + space_guids = parse_guid_list(mtls_allowed_spaces) + return if space_guids.empty? existing_guids = Space.where(guid: space_guids).select_map(:guid) missing_guids = space_guids - existing_guids return if missing_guids.empty? - errors.add(:mtls_allowed_sources, "spaces contains non-existent space GUIDs: #{missing_guids.join(', ')}") + errors.add(:mtls_allowed_spaces, "contains non-existent space GUIDs: #{missing_guids.join(', ')}") end def validate_org_guids_exist - org_guids = normalized_mtls_allowed_sources['orgs'] - return if org_guids.blank? + org_guids = parse_guid_list(mtls_allowed_orgs) + return if org_guids.empty? existing_guids = Organization.where(guid: org_guids).select_map(:guid) missing_guids = org_guids - existing_guids return if missing_guids.empty? - errors.add(:mtls_allowed_sources, "orgs contains non-existent organization GUIDs: #{missing_guids.join(', ')}") + errors.add(:mtls_allowed_orgs, "contains non-existent organization GUIDs: #{missing_guids.join(', ')}") end end end diff --git a/spec/unit/messages/route_options_message_spec.rb b/spec/unit/messages/route_options_message_spec.rb index c9d86df339c..f081ecc942b 100644 --- a/spec/unit/messages/route_options_message_spec.rb +++ b/spec/unit/messages/route_options_message_spec.rb @@ -37,12 +37,30 @@ module VCAP::CloudController end end - describe 'mtls_allowed_sources validations' do + describe 'mTLS allowed sources validations (RFC-0027 compliant flat options)' do context 'when app_to_app_mtls_routing feature flag is disabled' do - it 'does not allow mtls_allowed_sources option' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { apps: ['app-guid-1'] } }) + it 'does not allow mtls_allowed_apps option' do + message = RouteOptionsMessage.new({ mtls_allowed_apps: 'app-guid-1' }) expect(message).not_to be_valid - expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allowed_sources'") + expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allowed_apps'") + end + + it 'does not allow mtls_allowed_spaces option' do + message = RouteOptionsMessage.new({ mtls_allowed_spaces: 'space-guid-1' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allowed_spaces'") + end + + it 'does not allow mtls_allowed_orgs option' do + message = RouteOptionsMessage.new({ mtls_allowed_orgs: 'org-guid-1' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allowed_orgs'") + end + + it 'does not allow mtls_allow_any option' do + message = RouteOptionsMessage.new({ mtls_allow_any: true }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allow_any'") end end @@ -51,183 +69,176 @@ module VCAP::CloudController VCAP::CloudController::FeatureFlag.make(name: 'app_to_app_mtls_routing', enabled: true) end - describe 'structure validation' do - it 'allows valid mtls_allowed_sources with apps' do - app = AppModel.make - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => [app.guid] } }) - expect(message).to be_valid - end - - it 'allows valid mtls_allowed_sources with spaces' do - space = Space.make - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'spaces' => [space.guid] } }) + describe 'mtls_allowed_apps validation' do + it 'allows valid comma-separated app GUIDs' do + app1 = AppModel.make + app2 = AppModel.make + message = RouteOptionsMessage.new({ mtls_allowed_apps: "#{app1.guid},#{app2.guid}" }) expect(message).to be_valid end - it 'allows valid mtls_allowed_sources with orgs' do - org = Organization.make - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'orgs' => [org.guid] } }) + it 'allows single app GUID' do + app = AppModel.make + message = RouteOptionsMessage.new({ mtls_allowed_apps: app.guid }) expect(message).to be_valid end - it 'allows valid mtls_allowed_sources with any: true' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true } }) + it 'allows app GUIDs with whitespace around commas' do + app1 = AppModel.make + app2 = AppModel.make + message = RouteOptionsMessage.new({ mtls_allowed_apps: "#{app1.guid} , #{app2.guid}" }) expect(message).to be_valid end - it 'allows valid mtls_allowed_sources with any: false' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => false } }) - expect(message).to be_valid + it 'rejects non-existent app GUIDs' do + message = RouteOptionsMessage.new({ mtls_allowed_apps: 'non-existent-guid' }) + expect(message).not_to be_valid + expect(message.errors_on(:mtls_allowed_apps)).to include('contains non-existent app GUIDs: non-existent-guid') end - it 'allows empty mtls_allowed_sources object' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: {} }) - expect(message).to be_valid + it 'reports multiple non-existent app GUIDs' do + message = RouteOptionsMessage.new({ mtls_allowed_apps: 'guid-1,guid-2' }) + expect(message).not_to be_valid + expect(message.errors_on(:mtls_allowed_apps)).to include('contains non-existent app GUIDs: guid-1, guid-2') end - it 'does not allow non-object mtls_allowed_sources' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: 'invalid' }) + it 'rejects non-string values' do + message = RouteOptionsMessage.new({ mtls_allowed_apps: ['array-not-string'] }) expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('must be an object') + expect(message.errors_on(:mtls_allowed_apps)).to include('must be a string of comma-separated GUIDs') end + end - it 'does not allow array mtls_allowed_sources' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: ['app-guid-1'] }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('must be an object') + describe 'mtls_allowed_spaces validation' do + it 'allows valid comma-separated space GUIDs' do + space1 = Space.make + space2 = Space.make + message = RouteOptionsMessage.new({ mtls_allowed_spaces: "#{space1.guid},#{space2.guid}" }) + expect(message).to be_valid end - it 'does not allow invalid keys in mtls_allowed_sources' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'invalid_key' => 'value' } }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('contains invalid keys: invalid_key') + it 'allows single space GUID' do + space = Space.make + message = RouteOptionsMessage.new({ mtls_allowed_spaces: space.guid }) + expect(message).to be_valid end - it 'does not allow non-array apps' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => 'not-an-array' } }) + it 'rejects non-existent space GUIDs' do + message = RouteOptionsMessage.new({ mtls_allowed_spaces: 'non-existent-space' }) expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('apps must be an array of strings') + expect(message.errors_on(:mtls_allowed_spaces)).to include('contains non-existent space GUIDs: non-existent-space') end - it 'does not allow non-string elements in apps array' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => [123, 456] } }) + it 'rejects non-string values' do + message = RouteOptionsMessage.new({ mtls_allowed_spaces: { 'nested' => 'object' } }) expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('apps must be an array of strings') + expect(message.errors_on(:mtls_allowed_spaces)).to include('must be a string of comma-separated GUIDs') end + end - it 'does not allow non-array spaces' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'spaces' => 'not-an-array' } }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('spaces must be an array of strings') + describe 'mtls_allowed_orgs validation' do + it 'allows valid comma-separated org GUIDs' do + org1 = Organization.make + org2 = Organization.make + message = RouteOptionsMessage.new({ mtls_allowed_orgs: "#{org1.guid},#{org2.guid}" }) + expect(message).to be_valid end - it 'does not allow non-array orgs' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'orgs' => 'not-an-array' } }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('orgs must be an array of strings') + it 'allows single org GUID' do + org = Organization.make + message = RouteOptionsMessage.new({ mtls_allowed_orgs: org.guid }) + expect(message).to be_valid end - it 'does not allow non-boolean any' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => 'true' } }) + it 'rejects non-existent org GUIDs' do + message = RouteOptionsMessage.new({ mtls_allowed_orgs: 'non-existent-org' }) expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('any must be a boolean') + expect(message.errors_on(:mtls_allowed_orgs)).to include('contains non-existent organization GUIDs: non-existent-org') end end - describe 'any exclusivity validation' do - it 'does not allow any: true with apps list' do - app = AppModel.make - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true, 'apps' => [app.guid] } }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + describe 'mtls_allow_any validation' do + it 'allows true value' do + message = RouteOptionsMessage.new({ mtls_allow_any: true }) + expect(message).to be_valid end - it 'does not allow any: true with spaces list' do - space = Space.make - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true, 'spaces' => [space.guid] } }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + it 'allows false value' do + message = RouteOptionsMessage.new({ mtls_allow_any: false }) + expect(message).to be_valid end - it 'does not allow any: true with orgs list' do - org = Organization.make - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true, 'orgs' => [org.guid] } }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('any is mutually exclusive with apps, spaces, and orgs') + it 'allows string "true"' do + message = RouteOptionsMessage.new({ mtls_allow_any: 'true' }) + expect(message).to be_valid end - it 'allows any: false with apps list' do - app = AppModel.make - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => false, 'apps' => [app.guid] } }) + it 'allows string "false"' do + message = RouteOptionsMessage.new({ mtls_allow_any: 'false' }) expect(message).to be_valid end - it 'allows any: true with empty apps list' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'any' => true, 'apps' => [] } }) - expect(message).to be_valid + it 'rejects non-boolean values' do + message = RouteOptionsMessage.new({ mtls_allow_any: 'yes' }) + expect(message).not_to be_valid + expect(message.errors_on(:mtls_allow_any)).to include('must be a boolean (true or false)') end end - describe 'GUID existence validation' do - it 'validates that app GUIDs exist' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => ['non-existent-app-guid'] } }) + describe 'mtls_allow_any exclusivity validation' do + it 'does not allow mtls_allow_any with mtls_allowed_apps' do + app = AppModel.make + message = RouteOptionsMessage.new({ mtls_allow_any: true, mtls_allowed_apps: app.guid }) expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('apps contains non-existent app GUIDs: non-existent-app-guid') + expect(message.errors_on(:mtls_allow_any)).to include('is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') end - it 'validates that space GUIDs exist' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'spaces' => ['non-existent-space-guid'] } }) + it 'does not allow mtls_allow_any with mtls_allowed_spaces' do + space = Space.make + message = RouteOptionsMessage.new({ mtls_allow_any: true, mtls_allowed_spaces: space.guid }) expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('spaces contains non-existent space GUIDs: non-existent-space-guid') + expect(message.errors_on(:mtls_allow_any)).to include('is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') end - it 'validates that org GUIDs exist' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'orgs' => ['non-existent-org-guid'] } }) + it 'does not allow mtls_allow_any with mtls_allowed_orgs' do + org = Organization.make + message = RouteOptionsMessage.new({ mtls_allow_any: true, mtls_allowed_orgs: org.guid }) expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('orgs contains non-existent organization GUIDs: non-existent-org-guid') + expect(message.errors_on(:mtls_allow_any)).to include('is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') end - it 'reports multiple non-existent app GUIDs' do - message = RouteOptionsMessage.new({ mtls_allowed_sources: { 'apps' => ['guid-1', 'guid-2'] } }) + it 'allows mtls_allow_any: false with specific GUIDs' do + app = AppModel.make + message = RouteOptionsMessage.new({ mtls_allow_any: false, mtls_allowed_apps: app.guid }) + expect(message).to be_valid + end + + it 'allows string "true" exclusivity check' do + app = AppModel.make + message = RouteOptionsMessage.new({ mtls_allow_any: 'true', mtls_allowed_apps: app.guid }) expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('apps contains non-existent app GUIDs: guid-1, guid-2') + expect(message.errors_on(:mtls_allow_any)).to include('is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') end + end - it 'allows mix of existing apps, spaces, and orgs' do + describe 'combined options' do + it 'allows all mTLS options together (without mtls_allow_any)' do app = AppModel.make space = Space.make org = Organization.make message = RouteOptionsMessage.new({ - mtls_allowed_sources: { - 'apps' => [app.guid], - 'spaces' => [space.guid], - 'orgs' => [org.guid] - } + mtls_allowed_apps: app.guid, + mtls_allowed_spaces: space.guid, + mtls_allowed_orgs: org.guid }) expect(message).to be_valid end - it 'validates all types of GUIDs when multiple are provided' do - app = AppModel.make - message = RouteOptionsMessage.new({ - mtls_allowed_sources: { - 'apps' => [app.guid], - 'spaces' => ['non-existent-space'], - 'orgs' => ['non-existent-org'] - } - }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_sources)).to include('spaces contains non-existent space GUIDs: non-existent-space') - expect(message.errors_on(:mtls_allowed_sources)).to include('orgs contains non-existent organization GUIDs: non-existent-org') - end - end - - describe 'combined with other options' do - it 'allows mtls_allowed_sources with loadbalancing' do + it 'allows mTLS options with loadbalancing' do app = AppModel.make message = RouteOptionsMessage.new({ loadbalancing: 'round-robin', - mtls_allowed_sources: { 'apps' => [app.guid] } + mtls_allowed_apps: app.guid }) expect(message).to be_valid end From df1ac2ffd5b84369b3b13f43a52012f46b66d9de Mon Sep 17 00:00:00 2001 From: rkoster Date: Thu, 9 Apr 2026 07:52:15 +0000 Subject: [PATCH 06/11] Implement RFC domain-scoped mTLS routing with /v3/access_rules API Replace POC route-options-based mTLS implementation with RFC-compliant architecture: Domain model changes: - Add enforce_access_rules (boolean) and access_rules_scope (any/org/space) to domains - Fields are immutable after domain creation - Update DomainCreateMessage, DomainPresenter, and DomainCreate action Access Rules resource: - New /v3/access_rules API with full CRUD operations - RouteAccessRule model with guid, name, selector, route_id - Selector format: cf:app:, cf:space:, cf:org:, or cf:any - Enforce cf:any exclusivity and per-route name/selector uniqueness - Space Developer can manage rules for routes in their space Diego sync path: - Inject access_scope and access_rules into route options for GoRouter - Filter internal mTLS keys (access_scope, access_rules) from public /v3/routes API - Add access_rules to eager load to avoid N+1 queries Tests: - Unit tests for AccessRuleCreateMessage (selector validation, cf:any rules) - Request specs for /v3/access_rules CRUD (create, show, list, delete, metadata update) - Updated domain_create_message_spec for enforce_access_rules validation - Updated routing_info_spec to verify mTLS options injection - Updated route_presenter_spec to verify internal keys are filtered Remove POC artifacts: - Remove app_to_app_mtls_routing feature flag - Remove mtls_allowed_* keys from route_options_message --- app/access/access_rule_access.rb | 66 ++++ app/actions/domain_create.rb | 2 + app/controllers/v3/access_rules_controller.rb | 129 +++++++ ...access_rule_selector_resource_decorator.rb | 40 ++ app/messages/access_rule_create_message.rb | 52 +++ app/messages/access_rule_update_message.rb | 9 + app/messages/access_rules_list_message.rb | 17 + app/messages/domain_create_message.rb | 22 ++ app/messages/route_options_message.rb | 98 +---- app/models/runtime/domain.rb | 4 +- app/models/runtime/feature_flag.rb | 3 +- app/models/runtime/route.rb | 3 + app/models/runtime/route_access_rule.rb | 15 + app/presenters/v3/access_rule_presenter.rb | 47 +++ app/presenters/v3/domain_presenter.rb | 2 + app/presenters/v3/route_presenter.rb | 7 +- config/routes.rb | 7 + ...000_add_enforce_access_rules_to_domains.rb | 15 + ...0260407100001_create_route_access_rules.rb | 24 ++ .../diego/protocol/routing_info.rb | 13 +- spec/request/access_rules_spec.rb | 357 ++++++++++++++++++ .../diego/protocol/routing_info_spec.rb | 58 +++ .../access_rule_create_message_spec.rb | 248 ++++++++++++ .../messages/domain_create_message_spec.rb | 87 +++++ .../messages/route_options_message_spec.rb | 209 ---------- .../presenters/v3/route_presenter_spec.rb | 38 ++ 26 files changed, 1260 insertions(+), 312 deletions(-) create mode 100644 app/access/access_rule_access.rb create mode 100644 app/controllers/v3/access_rules_controller.rb create mode 100644 app/decorators/include_access_rule_selector_resource_decorator.rb create mode 100644 app/messages/access_rule_create_message.rb create mode 100644 app/messages/access_rule_update_message.rb create mode 100644 app/messages/access_rules_list_message.rb create mode 100644 app/models/runtime/route_access_rule.rb create mode 100644 app/presenters/v3/access_rule_presenter.rb create mode 100644 db/migrations/20260407100000_add_enforce_access_rules_to_domains.rb create mode 100644 db/migrations/20260407100001_create_route_access_rules.rb create mode 100644 spec/request/access_rules_spec.rb create mode 100644 spec/unit/messages/access_rule_create_message_spec.rb diff --git a/app/access/access_rule_access.rb b/app/access/access_rule_access.rb new file mode 100644 index 00000000000..72fff7ebf30 --- /dev/null +++ b/app/access/access_rule_access.rb @@ -0,0 +1,66 @@ +module VCAP::CloudController + class AccessRuleAccess < BaseAccess + # Space Developer of the route's space can manage access rules. + # No bilateral requirement — destination-controlled auth only. + + def create?(access_rule, _params=nil) + return true if admin_user? + + route = access_rule.route + return false unless route + + space = route.space + context.user_email && context.user.is_a?(User) && + space.developers.include?(context.user) + end + + def read?(access_rule) + return true if admin_user? || admin_read_only_user? || global_auditor? + + route = access_rule.route + return false unless route + + object_is_visible_to_user?(access_rule, context.user) + end + + def update?(access_rule, _params=nil) + create?(access_rule) + end + + def delete?(access_rule) + create?(access_rule) + end + + def index?(_object_class, _params=nil) + admin_user? || admin_read_only_user? || has_read_scope? || global_auditor? + end + + def read_with_token?(_) + admin_user? || admin_read_only_user? || has_read_scope? || global_auditor? + end + + def create_with_token?(_) + admin_user? || has_write_scope? + end + + def read_for_update_with_token?(_) + admin_user? || has_write_scope? + end + + def can_remove_related_object_with_token?(*args) + read_for_update_with_token?(*args) + end + + def read_related_object_for_update_with_token?(*args) + read_for_update_with_token?(*args) + end + + def update_with_token?(_) + admin_user? || has_write_scope? + end + + def delete_with_token?(_) + admin_user? || has_write_scope? + end + end +end diff --git a/app/actions/domain_create.rb b/app/actions/domain_create.rb index f69d05cd7a5..2ebbe778c14 100644 --- a/app/actions/domain_create.rb +++ b/app/actions/domain_create.rb @@ -21,6 +21,8 @@ def create(message:, shared_organizations: []) end domain.router_group_guid = message.router_group_guid + domain.enforce_access_rules = message.enforce_access_rules || false + domain.access_rules_scope = message.access_rules_scope Domain.db.transaction do domain.save diff --git a/app/controllers/v3/access_rules_controller.rb b/app/controllers/v3/access_rules_controller.rb new file mode 100644 index 00000000000..a45b982bb64 --- /dev/null +++ b/app/controllers/v3/access_rules_controller.rb @@ -0,0 +1,129 @@ +require 'messages/access_rule_create_message' +require 'messages/access_rule_update_message' +require 'messages/access_rules_list_message' +require 'presenters/v3/access_rule_presenter' + +class AccessRulesController < ApplicationController + def index + message = AccessRulesListMessage.from_params(query_params) + invalid_param!(message.errors.full_messages) unless message.valid? + + dataset = build_dataset(message) + + render status: :ok, json: Presenters::V3::PaginatedListPresenter.new( + presenter: Presenters::V3::AccessRulePresenter, + paginated_result: SequelPaginator.new.get_page(dataset, message.try(:pagination_options)), + path: '/v3/access_rules', + message: message + ) + end + + def show + access_rule = VCAP::CloudController::RouteAccessRule.find(guid: hashed_params[:guid]) + resource_not_found!(:access_rule) unless access_rule + + route = access_rule.route + resource_not_found!(:access_rule) unless route && permission_queryer.can_read_from_space?(route.space.id, route.space.organization_id) + + render status: :ok, json: Presenters::V3::AccessRulePresenter.new(access_rule) + end + + def create + message = AccessRuleCreateMessage.new(hashed_params[:body]) + unprocessable!(message.errors.full_messages) unless message.valid? + + route = VCAP::CloudController::Route.find(guid: message.route_guid) + resource_not_found!(:route) unless route && permission_queryer.can_read_from_space?(route.space.id, route.space.organization_id) + unauthorized! unless permission_queryer.can_write_to_active_space?(route.space.id) + suspended! unless permission_queryer.is_space_active?(route.space.id) + + unless route.domain.enforce_access_rules + unprocessable!("Cannot create access rules for route '#{route.guid}': the route's domain does not have enforce_access_rules enabled.") + end + + # Enforce cf:any exclusivity: if route already has a cf:any rule, reject new rules; + # if new rule is cf:any, reject if route already has any rules. + existing_selectors = route.access_rules.map(&:selector) + if message.selector == 'cf:any' && existing_selectors.any? + unprocessable!("Cannot add 'cf:any' selector when other access rules already exist for this route.") + end + if existing_selectors.include?('cf:any') && message.selector != 'cf:any' + unprocessable!("Cannot add selector '#{message.selector}': route already has a 'cf:any' rule.") + end + + # Uniqueness: name and selector must be unique per route + if route.access_rules.any? { |r| r.name == message.name } + unprocessable!("An access rule with name '#{message.name}' already exists for this route.") + end + if existing_selectors.include?(message.selector) + unprocessable!("An access rule with selector '#{message.selector}' already exists for this route.") + end + + access_rule = VCAP::CloudController::RouteAccessRule.new( + guid: SecureRandom.uuid, + name: message.name, + selector: message.selector, + route_id: route.id, + created_at: Time.now.utc, + updated_at: Time.now.utc + ) + access_rule.save + + render status: :created, json: Presenters::V3::AccessRulePresenter.new(access_rule) + end + + def update + access_rule = VCAP::CloudController::RouteAccessRule.find(guid: hashed_params[:guid]) + resource_not_found!(:access_rule) unless access_rule + + route = access_rule.route + resource_not_found!(:access_rule) unless route && permission_queryer.can_read_from_space?(route.space.id, route.space.organization_id) + unauthorized! unless permission_queryer.can_write_to_active_space?(route.space.id) + suspended! unless permission_queryer.is_space_active?(route.space.id) + + message = AccessRuleUpdateMessage.new(hashed_params[:body]) + unprocessable!(message.errors.full_messages) unless message.valid? + + VCAP::CloudController::MetadataUpdate.update(access_rule, message) + + render status: :ok, json: Presenters::V3::AccessRulePresenter.new(access_rule.reload) + end + + def destroy + access_rule = VCAP::CloudController::RouteAccessRule.find(guid: hashed_params[:guid]) + resource_not_found!(:access_rule) unless access_rule + + route = access_rule.route + resource_not_found!(:access_rule) unless route && permission_queryer.can_read_from_space?(route.space.id, route.space.organization_id) + unauthorized! unless permission_queryer.can_write_to_active_space?(route.space.id) + suspended! unless permission_queryer.is_space_active?(route.space.id) + + access_rule.destroy + head :no_content + end + + private + + def build_dataset(message) + dataset = VCAP::CloudController::RouteAccessRule.dataset + + readable_route_ids = VCAP::CloudController::Route. + join(:spaces, id: :space_id). + where(Sequel.lit(permission_queryer.readable_space_scoped_space_guids_query)). + select(:routes__id) + + dataset = dataset.where(route_id: readable_route_ids) + + if message.requested?(:route_guids) + dataset = dataset. + join(:routes, id: :route_id). + where(routes__guid: message.route_guids). + select_all(:route_access_rules) + end + + dataset = dataset.where(name: message.names) if message.requested?(:names) + dataset = dataset.where(selector: message.selectors) if message.requested?(:selectors) + + dataset + end +end diff --git a/app/decorators/include_access_rule_selector_resource_decorator.rb b/app/decorators/include_access_rule_selector_resource_decorator.rb new file mode 100644 index 00000000000..c5ac7552860 --- /dev/null +++ b/app/decorators/include_access_rule_selector_resource_decorator.rb @@ -0,0 +1,40 @@ +module VCAP::CloudController + class IncludeAccessRuleSelectorResourceDecorator + # Handles `?include=selector_resource` for GET /v3/access_rules + # Stale/missing resources (selector GUIDs that no longer exist) are silently absent. + + SELECTOR_REGEX = /\Acf:(app|space|org):([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})\z/ + + def self.match?(include_params) + include_params&.include?('selector_resource') + end + + def self.decorate(hash, access_rules) + included = [] + + access_rules.each do |rule| + match = SELECTOR_REGEX.match(rule.selector) + next unless match + + resource_type = match[1] + resource_guid = match[2] + + resource = case resource_type + when 'app' + VCAP::CloudController::AppModel.find(guid: resource_guid) + when 'space' + VCAP::CloudController::Space.find(guid: resource_guid) + when 'org' + VCAP::CloudController::Organization.find(guid: resource_guid) + end + + next if resource.nil? + + included << { type: resource_type, guid: resource.guid } + end + + hash[:included] = { selector_resources: included } + hash + end + end +end diff --git a/app/messages/access_rule_create_message.rb b/app/messages/access_rule_create_message.rb new file mode 100644 index 00000000000..f3086bf95ee --- /dev/null +++ b/app/messages/access_rule_create_message.rb @@ -0,0 +1,52 @@ +require 'messages/metadata_base_message' + +module VCAP::CloudController + class AccessRuleCreateMessage < MetadataBaseMessage + SELECTOR_REGEX = /\A(cf:(app|space|org):[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|cf:any)\z/ + + register_allowed_keys %i[ + name + selector + relationships + ] + + validates_with NoAdditionalKeysValidator + validates_with RelationshipValidator + + validates :name, presence: true, string: true + validates :selector, presence: true, string: true + + validate :selector_format_valid + validate :selector_not_cf_any_with_others + + delegate :route_guid, to: :relationships_message + + def relationships_message + @relationships_message ||= Relationships.new(relationships&.deep_symbolize_keys) + end + + private + + def selector_format_valid + return unless selector.is_a?(String) + return if SELECTOR_REGEX.match?(selector) + + errors.add(:selector, "must be in format 'cf:app:', 'cf:space:', 'cf:org:', or 'cf:any'") + end + + def selector_not_cf_any_with_others + # enforced at the controller level when checking existing rules on the route + end + + class Relationships < BaseMessage + register_allowed_keys [:route] + + validates_with NoAdditionalKeysValidator + validates :route, presence: true, to_one_relationship: true + + def route_guid + HashUtils.dig(route, :data, :guid) + end + end + end +end diff --git a/app/messages/access_rule_update_message.rb b/app/messages/access_rule_update_message.rb new file mode 100644 index 00000000000..b9adcf62a4a --- /dev/null +++ b/app/messages/access_rule_update_message.rb @@ -0,0 +1,9 @@ +require 'messages/metadata_base_message' + +module VCAP::CloudController + class AccessRuleUpdateMessage < MetadataBaseMessage + register_allowed_keys [] + + validates_with NoAdditionalKeysValidator + end +end diff --git a/app/messages/access_rules_list_message.rb b/app/messages/access_rules_list_message.rb new file mode 100644 index 00000000000..7c7973fda97 --- /dev/null +++ b/app/messages/access_rules_list_message.rb @@ -0,0 +1,17 @@ +require 'messages/list_message' + +module VCAP::CloudController + class AccessRulesListMessage < ListMessage + register_allowed_keys %i[ + route_guids + names + selectors + ] + + validates_with NoAdditionalParamsValidator + + def self.from_params(params) + super(params, %w[route_guids names selectors]) + end + end +end diff --git a/app/messages/domain_create_message.rb b/app/messages/domain_create_message.rb index 110bc0d499b..b10d065b553 100644 --- a/app/messages/domain_create_message.rb +++ b/app/messages/domain_create_message.rb @@ -16,6 +16,8 @@ class DomainCreateMessage < MetadataBaseMessage internal relationships router_group + enforce_access_rules + access_rules_scope ] def self.relationships_requested? @@ -59,6 +61,12 @@ def self.relationships_requested? allow_nil: true, boolean: true + validates :enforce_access_rules, + allow_nil: true, + boolean: true + + validate :access_rules_scope_validation + delegate :organization_guid, to: :relationships_message delegate :shared_organizations_guids, to: :relationships_message @@ -97,6 +105,20 @@ def router_group_validation errors.add(:router_group, 'guid must be a string') unless router_group_guid.is_a?(String) end + def access_rules_scope_validation + if requested?(:access_rules_scope) + unless access_rules_scope.nil? || %w[any org space].include?(access_rules_scope) + errors.add(:access_rules_scope, "must be one of 'any', 'org', 'space'") + end + end + + if requested?(:enforce_access_rules) && enforce_access_rules == true + if !requested?(:access_rules_scope) || access_rules_scope.nil? + errors.add(:access_rules_scope, 'is required when enforce_access_rules is true') + end + end + end + class Relationships < BaseMessage def self.shared_organizations_requested? @shared_organizations_requested ||= proc { |a| a.requested?(:shared_organizations) } diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index c8b6d82a115..7371b391558 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -2,16 +2,11 @@ module VCAP::CloudController class RouteOptionsMessage < BaseMessage - # Register all possible keys upfront so attr_accessors are created - # RFC-0027 compliant: only string/number/boolean values (no nested objects/arrays) - # mtls_allowed_apps, mtls_allowed_spaces, mtls_allowed_orgs are comma-separated GUIDs - # mtls_allow_any is a boolean - register_allowed_keys %i[loadbalancing hash_header hash_balance mtls_allowed_apps mtls_allowed_spaces mtls_allowed_orgs mtls_allow_any] + register_allowed_keys %i[loadbalancing hash_header hash_balance] def self.valid_route_options options = %i[loadbalancing] options += %i[hash_header hash_balance] if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) - options += %i[mtls_allowed_apps mtls_allowed_spaces mtls_allowed_orgs mtls_allow_any] if VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) options.freeze end @@ -25,7 +20,6 @@ def self.valid_loadbalancing_algorithms validate :loadbalancing_algorithm_is_valid validate :route_options_are_valid validate :hash_options_are_valid - validate :mtls_allowed_sources_options_are_valid def loadbalancing_algorithm_is_valid return if loadbalancing.blank? @@ -87,95 +81,5 @@ def validate_hash_options_with_loadbalancing errors.add(:base, 'Hash header can only be set when loadbalancing is hash') if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' end - - def mtls_allowed_sources_options_are_valid - # Only validate mtls options when the feature flag is enabled - # If disabled, route_options_are_valid will already report them as unknown fields - return unless VCAP::CloudController::FeatureFlag.enabled?(:app_to_app_mtls_routing) - - validate_mtls_string_types - validate_mtls_allow_any_type - validate_mtls_allow_any_exclusivity - validate_mtls_guids_exist - end - - private - - # Parse comma-separated GUIDs into an array - def parse_guid_list(value) - return [] if value.blank? - - value.to_s.split(',').map(&:strip).reject(&:empty?) - end - - def validate_mtls_string_types - # These should be strings (comma-separated GUIDs) per RFC-0027 - %i[mtls_allowed_apps mtls_allowed_spaces mtls_allowed_orgs].each do |key| - value = public_send(key) - next if value.blank? - - unless value.is_a?(String) - errors.add(key, 'must be a string of comma-separated GUIDs') - end - end - end - - def validate_mtls_allow_any_type - return if mtls_allow_any.nil? - - unless [true, false, 'true', 'false'].include?(mtls_allow_any) - errors.add(:mtls_allow_any, 'must be a boolean (true or false)') - end - end - - def validate_mtls_allow_any_exclusivity - allow_any = mtls_allow_any == true || mtls_allow_any == 'true' - has_specific = [mtls_allowed_apps, mtls_allowed_spaces, mtls_allowed_orgs].any?(&:present?) - - return unless allow_any && has_specific - - errors.add(:mtls_allow_any, 'is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') - end - - def validate_mtls_guids_exist - return if errors.any? # Skip if already invalid - - validate_app_guids_exist - validate_space_guids_exist - validate_org_guids_exist - end - - def validate_app_guids_exist - app_guids = parse_guid_list(mtls_allowed_apps) - return if app_guids.empty? - - existing_guids = AppModel.where(guid: app_guids).select_map(:guid) - missing_guids = app_guids - existing_guids - return if missing_guids.empty? - - errors.add(:mtls_allowed_apps, "contains non-existent app GUIDs: #{missing_guids.join(', ')}") - end - - def validate_space_guids_exist - space_guids = parse_guid_list(mtls_allowed_spaces) - return if space_guids.empty? - - existing_guids = Space.where(guid: space_guids).select_map(:guid) - missing_guids = space_guids - existing_guids - return if missing_guids.empty? - - errors.add(:mtls_allowed_spaces, "contains non-existent space GUIDs: #{missing_guids.join(', ')}") - end - - def validate_org_guids_exist - org_guids = parse_guid_list(mtls_allowed_orgs) - return if org_guids.empty? - - existing_guids = Organization.where(guid: org_guids).select_map(:guid) - missing_guids = org_guids - existing_guids - return if missing_guids.empty? - - errors.add(:mtls_allowed_orgs, "contains non-existent organization GUIDs: #{missing_guids.join(', ')}") - end end end diff --git a/app/models/runtime/domain.rb b/app/models/runtime/domain.rb index 56839444059..a26ff7b05bd 100644 --- a/app/models/runtime/domain.rb +++ b/app/models/runtime/domain.rb @@ -79,8 +79,8 @@ def shared_or_owned_by(organization_ids) one_to_many :labels, class: 'VCAP::CloudController::DomainLabelModel', key: :resource_guid, primary_key: :guid one_to_many :annotations, class: 'VCAP::CloudController::DomainAnnotationModel', key: :resource_guid, primary_key: :guid - export_attributes :name, :owning_organization_guid, :shared_organizations - import_attributes :name, :owning_organization_guid + export_attributes :name, :owning_organization_guid, :shared_organizations, :enforce_access_rules, :access_rules_scope + import_attributes :name, :owning_organization_guid, :enforce_access_rules, :access_rules_scope strip_attributes :name add_association_dependencies labels: :destroy diff --git a/app/models/runtime/feature_flag.rb b/app/models/runtime/feature_flag.rb index da4bc026f2c..df991d89f7e 100644 --- a/app/models/runtime/feature_flag.rb +++ b/app/models/runtime/feature_flag.rb @@ -24,8 +24,7 @@ class UndefinedFeatureFlagError < StandardError hide_marketplace_from_unauthenticated_users: false, resource_matching: true, route_sharing: false, - hash_based_routing: false, - app_to_app_mtls_routing: false + hash_based_routing: false }.freeze ADMIN_SKIPPABLE = %i[ diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index bdefff78c41..84032473a23 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -39,6 +39,9 @@ class InvalidOrganizationRelation < CloudController::Errors::InvalidRelation; en add_association_dependencies route_mappings: :destroy + one_to_many :access_rules, class: 'VCAP::CloudController::RouteAccessRule', key: :route_id, primary_key: :id + add_association_dependencies access_rules: :destroy + export_attributes :host, :path, :domain_guid, :space_guid, :service_instance_guid, :port, :options import_attributes :host, :path, :domain_guid, :space_guid, :app_guids, :port, :options diff --git a/app/models/runtime/route_access_rule.rb b/app/models/runtime/route_access_rule.rb new file mode 100644 index 00000000000..08ed6c6e3e2 --- /dev/null +++ b/app/models/runtime/route_access_rule.rb @@ -0,0 +1,15 @@ +module VCAP::CloudController + class RouteAccessRule < Sequel::Model(:route_access_rules) + many_to_one :route, + class: 'VCAP::CloudController::Route', + key: :route_id, + primary_key: :id, + without_guid_generation: true + + def validate + validates_presence :name + validates_presence :selector + validates_presence :route_id + end + end +end diff --git a/app/presenters/v3/access_rule_presenter.rb b/app/presenters/v3/access_rule_presenter.rb new file mode 100644 index 00000000000..cd5f18d2c47 --- /dev/null +++ b/app/presenters/v3/access_rule_presenter.rb @@ -0,0 +1,47 @@ +require 'presenters/v3/base_presenter' +require 'presenters/mixins/metadata_presentation_helpers' + +module VCAP::CloudController + module Presenters + module V3 + class AccessRulePresenter < BasePresenter + include VCAP::CloudController::Presenters::Mixins::MetadataPresentationHelpers + + def to_hash + { + guid: access_rule.guid, + created_at: access_rule.created_at, + updated_at: access_rule.updated_at, + name: access_rule.name, + selector: access_rule.selector, + relationships: { + route: { + data: { + guid: access_rule.route.guid + } + } + }, + links: build_links + } + end + + private + + def access_rule + @resource + end + + def build_links + { + self: { + href: url_builder.build_url(path: "/v3/access_rules/#{access_rule.guid}") + }, + route: { + href: url_builder.build_url(path: "/v3/routes/#{access_rule.route.guid}") + } + } + end + end + end + end +end diff --git a/app/presenters/v3/domain_presenter.rb b/app/presenters/v3/domain_presenter.rb index 9ffa51fa951..8f655fa9927 100644 --- a/app/presenters/v3/domain_presenter.rb +++ b/app/presenters/v3/domain_presenter.rb @@ -28,6 +28,8 @@ def to_hash internal: domain.internal, router_group: hashified_router_group(domain.router_group_guid), supported_protocols: domain.protocols, + enforce_access_rules: domain.enforce_access_rules || false, + access_rules_scope: domain.access_rules_scope, relationships: { organization: { data: owning_org_guid diff --git a/app/presenters/v3/route_presenter.rb b/app/presenters/v3/route_presenter.rb index c090fafae5b..8eab8b790c3 100644 --- a/app/presenters/v3/route_presenter.rb +++ b/app/presenters/v3/route_presenter.rb @@ -48,13 +48,18 @@ def to_hash }, links: build_links } - hash.merge!(options: route.options) unless route.options.nil? + unless route.options.nil? + public_options = route.options.reject { |k, _| INTERNAL_ROUTE_OPTIONS.include?(k.to_s) } + hash.merge!(options: public_options) unless public_options.empty? + end @decorators.reduce(hash) { |memo, d| d.decorate(memo, [route]) } end private + INTERNAL_ROUTE_OPTIONS = %w[access_scope access_rules].freeze + def route @resource end diff --git a/config/routes.rb b/config/routes.rb index d1723e474e0..9f62790b9af 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -344,6 +344,13 @@ post '/roles', to: 'roles#create' delete '/roles/:guid', to: 'roles#destroy' + # access_rules + get '/access_rules', to: 'access_rules#index' + get '/access_rules/:guid', to: 'access_rules#show' + post '/access_rules', to: 'access_rules#create' + patch '/access_rules/:guid', to: 'access_rules#update' + delete '/access_rules/:guid', to: 'access_rules#destroy' + # info get '/info', to: 'info#v3_info' get '/info/usage_summary', to: 'info#show_usage_summary' diff --git a/db/migrations/20260407100000_add_enforce_access_rules_to_domains.rb b/db/migrations/20260407100000_add_enforce_access_rules_to_domains.rb new file mode 100644 index 00000000000..5f2df5e415b --- /dev/null +++ b/db/migrations/20260407100000_add_enforce_access_rules_to_domains.rb @@ -0,0 +1,15 @@ +Sequel.migration do + up do + alter_table :domains do + add_column :enforce_access_rules, :boolean, default: false, null: false unless @db.schema(:domains).map(&:first).include?(:enforce_access_rules) + add_column :access_rules_scope, String, null: true, size: 255 unless @db.schema(:domains).map(&:first).include?(:access_rules_scope) + end + end + + down do + alter_table :domains do + drop_column :enforce_access_rules if @db.schema(:domains).map(&:first).include?(:enforce_access_rules) + drop_column :access_rules_scope if @db.schema(:domains).map(&:first).include?(:access_rules_scope) + end + end +end diff --git a/db/migrations/20260407100001_create_route_access_rules.rb b/db/migrations/20260407100001_create_route_access_rules.rb new file mode 100644 index 00000000000..4c8c78f4216 --- /dev/null +++ b/db/migrations/20260407100001_create_route_access_rules.rb @@ -0,0 +1,24 @@ +Sequel.migration do + up do + unless table_exists?(:route_access_rules) + create_table :route_access_rules do + String :guid, size: 255, null: false + primary_key :id + String :name, size: 255, null: false + String :selector, size: 255, null: false + Integer :route_id, null: false + DateTime :created_at, null: false + DateTime :updated_at, null: false + + index :guid, unique: true, name: :route_access_rules_guid_index + index %i[route_id name], unique: true, name: :route_access_rules_route_id_name_index + index %i[route_id selector], unique: true, name: :route_access_rules_route_id_selector_index + foreign_key [:route_id], :routes, on_delete: :cascade, name: :fk_route_access_rules_route_id + end + end + end + + down do + drop_table(:route_access_rules) if table_exists?(:route_access_rules) + end +end diff --git a/lib/cloud_controller/diego/protocol/routing_info.rb b/lib/cloud_controller/diego/protocol/routing_info.rb index e85c061a4fd..27908728008 100644 --- a/lib/cloud_controller/diego/protocol/routing_info.rb +++ b/lib/cloud_controller/diego/protocol/routing_info.rb @@ -9,7 +9,7 @@ def initialize(process) end def routing_info - process_eager = ProcessModel.eager(route_mappings: { route: %i[domain route_binding] }).where(id: process.id).all + process_eager = ProcessModel.eager(route_mappings: { route: %i[domain route_binding access_rules] }).where(id: process.id).all return {} if process_eager.empty? @@ -44,6 +44,17 @@ def http_info(process_eager) info['port'] = get_port_to_use(route_mapping) info['protocol'] = route_mapping.protocol info['options'] = r.options if r.options + + # Inject mTLS access control options for enforce_access_rules domains. + # These are GoRouter-internal keys and are filtered from the /v3/routes API. + if r.domain.enforce_access_rules + mtls_options = info['options']&.dup || {} + mtls_options['access_scope'] = r.domain.access_rules_scope if r.domain.access_rules_scope + selectors = r.access_rules.map(&:selector) + mtls_options['access_rules'] = selectors.join(',') unless selectors.empty? + info['options'] = mtls_options + end + info end end diff --git a/spec/request/access_rules_spec.rb b/spec/request/access_rules_spec.rb new file mode 100644 index 00000000000..3962cc59a66 --- /dev/null +++ b/spec/request/access_rules_spec.rb @@ -0,0 +1,357 @@ +require 'spec_helper' + +RSpec.describe 'Access Rules' do + let(:user) { VCAP::CloudController::User.make } + let(:admin_header) { admin_headers_for(user) } + let(:org) { VCAP::CloudController::Organization.make } + let(:space) { VCAP::CloudController::Space.make(organization: org) } + + let(:mtls_domain) do + VCAP::CloudController::PrivateDomain.make( + owning_organization: org, + enforce_access_rules: true, + access_rules_scope: 'space' + ) + end + let(:regular_domain) do + VCAP::CloudController::PrivateDomain.make(owning_organization: org) + end + + let(:mtls_route) { VCAP::CloudController::Route.make(space: space, domain: mtls_domain) } + let(:regular_route) { VCAP::CloudController::Route.make(space: space, domain: regular_domain) } + + let(:valid_uuid) { '11111111-2222-3333-4444-555555555555' } + + def expected_rule_json(rule) + { + guid: rule.guid, + created_at: iso8601, + updated_at: iso8601, + name: rule.name, + selector: rule.selector, + relationships: { + route: { data: { guid: rule.route.guid } } + }, + links: { + self: { href: %r{/v3/access_rules/#{rule.guid}} }, + route: { href: %r{/v3/routes/#{rule.route.guid}} } + } + } + end + + before do + TestConfig.override(kubernetes: {}) + space.organization.add_user(user) + space.add_developer(user) + end + + describe 'POST /v3/access_rules' do + let(:request_body) do + { + name: 'allow-frontend', + selector: "cf:app:#{valid_uuid}", + relationships: { + route: { data: { guid: mtls_route.guid } } + } + } + end + + context 'as admin' do + it 'creates an access rule and returns 201' do + post '/v3/access_rules', request_body.to_json, admin_header + + expect(last_response.status).to eq(201) + parsed = Oj.load(last_response.body) + expect(parsed['name']).to eq('allow-frontend') + expect(parsed['selector']).to eq("cf:app:#{valid_uuid}") + expect(parsed['relationships']['route']['data']['guid']).to eq(mtls_route.guid) + end + end + + context 'as space developer' do + let(:user_headers) { headers_for(user) } + + it 'creates an access rule' do + post '/v3/access_rules', request_body.to_json, user_headers + + expect(last_response.status).to eq(201) + end + end + + context 'when the domain does not have enforce_access_rules enabled' do + let(:request_body) do + { + name: 'disallowed-rule', + selector: "cf:app:#{valid_uuid}", + relationships: { + route: { data: { guid: regular_route.guid } } + } + } + end + + it 'returns 422' do + post '/v3/access_rules', request_body.to_json, admin_header + + expect(last_response.status).to eq(422) + expect(last_response.body).to include('enforce_access_rules') + end + end + + context 'when the route does not exist' do + let(:request_body) do + { + name: 'bad-rule', + selector: "cf:app:#{valid_uuid}", + relationships: { + route: { data: { guid: 'nonexistent-guid' } } + } + } + end + + it 'returns 404' do + post '/v3/access_rules', request_body.to_json, admin_header + + expect(last_response.status).to eq(404) + end + end + + context 'cf:any exclusivity' do + before do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'existing-rule', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + end + + it 'rejects cf:any when other rules exist' do + post '/v3/access_rules', { + name: 'any-rule', + selector: 'cf:any', + relationships: { route: { data: { guid: mtls_route.guid } } } + }.to_json, admin_header + + expect(last_response.status).to eq(422) + expect(last_response.body).to include("cf:any") + end + end + + context 'when a cf:any rule already exists' do + before do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'any-rule', + selector: 'cf:any', + route_id: mtls_route.id + ) + end + + it 'rejects adding a specific selector' do + post '/v3/access_rules', { + name: 'specific-rule', + selector: "cf:space:#{valid_uuid}", + relationships: { route: { data: { guid: mtls_route.guid } } } + }.to_json, admin_header + + expect(last_response.status).to eq(422) + expect(last_response.body).to include("cf:any") + end + end + + context 'duplicate name per route' do + before do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'allow-frontend', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + end + + it 'returns 422' do + other_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' + post '/v3/access_rules', { + name: 'allow-frontend', + selector: "cf:space:#{other_uuid}", + relationships: { route: { data: { guid: mtls_route.guid } } } + }.to_json, admin_header + + expect(last_response.status).to eq(422) + expect(last_response.body).to include('allow-frontend') + end + end + + context 'duplicate selector per route' do + before do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'first-rule', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + end + + it 'returns 422' do + post '/v3/access_rules', { + name: 'second-rule', + selector: "cf:app:#{valid_uuid}", + relationships: { route: { data: { guid: mtls_route.guid } } } + }.to_json, admin_header + + expect(last_response.status).to eq(422) + end + end + + context 'invalid selector format' do + it 'returns 422' do + post '/v3/access_rules', { + name: 'bad-rule', + selector: 'not-valid', + relationships: { route: { data: { guid: mtls_route.guid } } } + }.to_json, admin_header + + expect(last_response.status).to eq(422) + expect(last_response.body).to include('selector') + end + end + end + + describe 'GET /v3/access_rules/:guid' do + let!(:access_rule) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'allow-frontend', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + end + + it 'returns the access rule' do + get "/v3/access_rules/#{access_rule.guid}", nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + expect(parsed['guid']).to eq(access_rule.guid) + expect(parsed['name']).to eq('allow-frontend') + expect(parsed['selector']).to eq("cf:app:#{valid_uuid}") + end + + context 'when the access rule does not exist' do + it 'returns 404' do + get '/v3/access_rules/nonexistent-guid', nil, admin_header + + expect(last_response.status).to eq(404) + end + end + end + + describe 'GET /v3/access_rules' do + let!(:rule1) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'rule-one', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + end + let!(:rule2) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'rule-two', + selector: 'cf:any', + route_id: VCAP::CloudController::Route.make(space: space, domain: mtls_domain).id + ) + end + + it 'lists all accessible access rules' do + get '/v3/access_rules', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + guids = parsed['resources'].map { |r| r['guid'] } + expect(guids).to include(rule1.guid, rule2.guid) + end + + it 'filters by route_guids' do + get "/v3/access_rules?route_guids=#{mtls_route.guid}", nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + guids = parsed['resources'].map { |r| r['guid'] } + expect(guids).to include(rule1.guid) + expect(guids).not_to include(rule2.guid) + end + + it 'filters by names' do + get '/v3/access_rules?names=rule-one', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + expect(parsed['resources'].length).to eq(1) + expect(parsed['resources'][0]['name']).to eq('rule-one') + end + + it 'filters by selectors' do + get '/v3/access_rules?selectors=cf:any', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + expect(parsed['resources'].length).to eq(1) + expect(parsed['resources'][0]['selector']).to eq('cf:any') + end + end + + describe 'DELETE /v3/access_rules/:guid' do + let!(:access_rule) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'to-delete', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + end + + it 'deletes the access rule and returns 204' do + delete "/v3/access_rules/#{access_rule.guid}", nil, admin_header + + expect(last_response.status).to eq(204) + expect(VCAP::CloudController::RouteAccessRule.find(guid: access_rule.guid)).to be_nil + end + + context 'when the access rule does not exist' do + it 'returns 404' do + delete '/v3/access_rules/nonexistent-guid', nil, admin_header + + expect(last_response.status).to eq(404) + end + end + end + + describe 'PATCH /v3/access_rules/:guid (metadata update)' do + let!(:access_rule) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'patchable', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + end + + it 'returns 200' do + patch "/v3/access_rules/#{access_rule.guid}", { + metadata: { labels: { env: 'production' } } + }.to_json, admin_header + + expect(last_response.status).to eq(200) + end + + context 'when the access rule does not exist' do + it 'returns 404' do + patch '/v3/access_rules/nonexistent-guid', {}.to_json, admin_header + + expect(last_response.status).to eq(404) + end + end + end +end diff --git a/spec/unit/lib/cloud_controller/diego/protocol/routing_info_spec.rb b/spec/unit/lib/cloud_controller/diego/protocol/routing_info_spec.rb index d74502cf615..95c39e1356f 100644 --- a/spec/unit/lib/cloud_controller/diego/protocol/routing_info_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/protocol/routing_info_spec.rb @@ -250,6 +250,64 @@ class Protocol it 'does not include the internal routes' do end end + + context 'when the route domain has enforce_access_rules enabled' do + let(:valid_uuid) { '11111111-2222-3333-4444-555555555555' } + let(:enforce_domain) do + PrivateDomain.make( + name: 'mtls.example.com', + owning_organization: org, + enforce_access_rules: true, + access_rules_scope: 'space' + ) + end + let(:mtls_route) { Route.make(host: 'myapp', domain: enforce_domain, space: space) } + let!(:access_rule1) do + RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'allow-app', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + end + let!(:access_rule2) do + RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'allow-space', + selector: "cf:space:#{valid_uuid}", + route_id: mtls_route.id + ) + end + + before do + RouteMappingModel.make(app: process.app, route: mtls_route, process_type: process.type) + end + + it 'injects access_scope and access_rules into route options' do + http_routes = ri['http_routes'] + mtls_entry = http_routes.find { |r| r['hostname'] == "myapp.mtls.example.com" } + + expect(mtls_entry).not_to be_nil + expect(mtls_entry['options']['access_scope']).to eq('space') + expect(mtls_entry['options']['access_rules']).to include("cf:app:#{valid_uuid}") + expect(mtls_entry['options']['access_rules']).to include("cf:space:#{valid_uuid}") + end + + context 'when the route has no access rules' do + before do + access_rule1.destroy + access_rule2.destroy + end + + it 'injects access_scope but omits access_rules key' do + http_routes = ri['http_routes'] + mtls_entry = http_routes.find { |r| r['hostname'] == "myapp.mtls.example.com" } + + expect(mtls_entry['options']['access_scope']).to eq('space') + expect(mtls_entry['options']).not_to have_key('access_rules') + end + end + end end context 'tcp routes' do diff --git a/spec/unit/messages/access_rule_create_message_spec.rb b/spec/unit/messages/access_rule_create_message_spec.rb new file mode 100644 index 00000000000..4d7adc60757 --- /dev/null +++ b/spec/unit/messages/access_rule_create_message_spec.rb @@ -0,0 +1,248 @@ +require 'spec_helper' +require 'messages/access_rule_create_message' + +module VCAP::CloudController + RSpec.describe AccessRuleCreateMessage do + let(:valid_uuid) { '11111111-2222-3333-4444-555555555555' } + let(:valid_route_relationship) do + { relationships: { route: { data: { guid: valid_uuid } } } } + end + + subject { AccessRuleCreateMessage.new(params) } + + describe 'validations' do + context 'when all valid params are given' do + let(:params) do + { + name: 'allow-frontend', + selector: "cf:app:#{valid_uuid}", + }.merge(valid_route_relationship) + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when unexpected keys are provided' do + let(:params) do + { + name: 'allow-frontend', + selector: "cf:app:#{valid_uuid}", + unexpected: 'field', + }.merge(valid_route_relationship) + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages[0]).to include("Unknown field(s): 'unexpected'") + end + end + + describe 'name' do + context 'when name is missing' do + let(:params) do + { + selector: "cf:app:#{valid_uuid}", + }.merge(valid_route_relationship) + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:name]).to include("can't be blank") + end + end + + context 'when name is not a string' do + let(:params) do + { + name: 42, + selector: "cf:app:#{valid_uuid}", + }.merge(valid_route_relationship) + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:name]).to include('must be a string') + end + end + end + + describe 'selector' do + context 'when selector is missing' do + let(:params) do + { + name: 'allow-frontend', + }.merge(valid_route_relationship) + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:selector]).to include("can't be blank") + end + end + + context 'when selector is not a string' do + let(:params) do + { + name: 'allow-frontend', + selector: 123, + }.merge(valid_route_relationship) + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:selector]).to include('must be a string') + end + end + + context 'selector format' do + context 'cf:app:' do + let(:params) do + { + name: 'allow-app', + selector: "cf:app:#{valid_uuid}", + }.merge(valid_route_relationship) + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'cf:space:' do + let(:params) do + { + name: 'allow-space', + selector: "cf:space:#{valid_uuid}", + }.merge(valid_route_relationship) + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'cf:org:' do + let(:params) do + { + name: 'allow-org', + selector: "cf:org:#{valid_uuid}", + }.merge(valid_route_relationship) + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'cf:any' do + let(:params) do + { + name: 'allow-any', + selector: 'cf:any', + }.merge(valid_route_relationship) + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'invalid format' do + let(:params) do + { + name: 'bad-rule', + selector: 'not-valid', + }.merge(valid_route_relationship) + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:selector]).to include( + "must be in format 'cf:app:', 'cf:space:', 'cf:org:', or 'cf:any'" + ) + end + end + + context 'cf:app: with invalid uuid' do + let(:params) do + { + name: 'bad-rule', + selector: 'cf:app:not-a-uuid', + }.merge(valid_route_relationship) + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:selector]).to include( + "must be in format 'cf:app:', 'cf:space:', 'cf:org:', or 'cf:any'" + ) + end + end + + context 'cf:unknown type' do + let(:params) do + { + name: 'bad-rule', + selector: "cf:team:#{valid_uuid}", + }.merge(valid_route_relationship) + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:selector]).to include( + "must be in format 'cf:app:', 'cf:space:', 'cf:org:', or 'cf:any'" + ) + end + end + end + end + + describe 'relationships' do + context 'when relationships is missing' do + let(:params) do + { + name: 'allow-frontend', + selector: "cf:app:#{valid_uuid}", + } + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:relationships]).to be_present + end + end + + context 'when route relationship is missing' do + let(:params) do + { + name: 'allow-frontend', + selector: "cf:app:#{valid_uuid}", + relationships: {}, + } + end + + it 'is not valid' do + expect(subject).not_to be_valid + end + end + + context 'when route guid is provided' do + let(:params) do + { + name: 'allow-frontend', + selector: "cf:app:#{valid_uuid}", + relationships: { route: { data: { guid: 'some-route-guid' } } }, + } + end + + it 'exposes the route_guid' do + expect(subject).to be_valid + expect(subject.route_guid).to eq('some-route-guid') + end + end + end + end + end +end diff --git a/spec/unit/messages/domain_create_message_spec.rb b/spec/unit/messages/domain_create_message_spec.rb index f7dae8db280..8caab439a11 100644 --- a/spec/unit/messages/domain_create_message_spec.rb +++ b/spec/unit/messages/domain_create_message_spec.rb @@ -403,6 +403,93 @@ module VCAP::CloudController expect(subject).to be_valid end end + + context 'enforce_access_rules' do + context 'when not a boolean' do + let(:params) { { name: 'name.com', enforce_access_rules: 'yes' } } + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:enforce_access_rules]).to include('must be a boolean') + end + end + + context 'when true without access_rules_scope' do + let(:params) { { name: 'name.com', enforce_access_rules: true } } + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:access_rules_scope]).to include('is required when enforce_access_rules is true') + end + end + + context 'when true with a valid access_rules_scope' do + let(:params) { { name: 'name.com', enforce_access_rules: true, access_rules_scope: 'space' } } + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when false without access_rules_scope' do + let(:params) { { name: 'name.com', enforce_access_rules: false } } + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when omitted' do + let(:params) { { name: 'name.com' } } + + it 'is valid' do + expect(subject).to be_valid + end + end + end + + context 'access_rules_scope' do + context 'when set to an invalid value' do + let(:params) { { name: 'name.com', enforce_access_rules: true, access_rules_scope: 'invalid' } } + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:access_rules_scope]).to include("must be one of 'any', 'org', 'space'") + end + end + + context "when set to 'any'" do + let(:params) { { name: 'name.com', enforce_access_rules: true, access_rules_scope: 'any' } } + + it 'is valid' do + expect(subject).to be_valid + end + end + + context "when set to 'org'" do + let(:params) { { name: 'name.com', enforce_access_rules: true, access_rules_scope: 'org' } } + + it 'is valid' do + expect(subject).to be_valid + end + end + + context "when set to 'space'" do + let(:params) { { name: 'name.com', enforce_access_rules: true, access_rules_scope: 'space' } } + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when provided without enforce_access_rules' do + let(:params) { { name: 'name.com', access_rules_scope: 'space' } } + + it 'is valid (scope alone is permissible)' do + expect(subject).to be_valid + end + end + end end describe 'accessor methods' do diff --git a/spec/unit/messages/route_options_message_spec.rb b/spec/unit/messages/route_options_message_spec.rb index f081ecc942b..57646d21950 100644 --- a/spec/unit/messages/route_options_message_spec.rb +++ b/spec/unit/messages/route_options_message_spec.rb @@ -37,215 +37,6 @@ module VCAP::CloudController end end - describe 'mTLS allowed sources validations (RFC-0027 compliant flat options)' do - context 'when app_to_app_mtls_routing feature flag is disabled' do - it 'does not allow mtls_allowed_apps option' do - message = RouteOptionsMessage.new({ mtls_allowed_apps: 'app-guid-1' }) - expect(message).not_to be_valid - expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allowed_apps'") - end - - it 'does not allow mtls_allowed_spaces option' do - message = RouteOptionsMessage.new({ mtls_allowed_spaces: 'space-guid-1' }) - expect(message).not_to be_valid - expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allowed_spaces'") - end - - it 'does not allow mtls_allowed_orgs option' do - message = RouteOptionsMessage.new({ mtls_allowed_orgs: 'org-guid-1' }) - expect(message).not_to be_valid - expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allowed_orgs'") - end - - it 'does not allow mtls_allow_any option' do - message = RouteOptionsMessage.new({ mtls_allow_any: true }) - expect(message).not_to be_valid - expect(message.errors_on(:base)).to include("Unknown field(s): 'mtls_allow_any'") - end - end - - context 'when app_to_app_mtls_routing feature flag is enabled' do - before do - VCAP::CloudController::FeatureFlag.make(name: 'app_to_app_mtls_routing', enabled: true) - end - - describe 'mtls_allowed_apps validation' do - it 'allows valid comma-separated app GUIDs' do - app1 = AppModel.make - app2 = AppModel.make - message = RouteOptionsMessage.new({ mtls_allowed_apps: "#{app1.guid},#{app2.guid}" }) - expect(message).to be_valid - end - - it 'allows single app GUID' do - app = AppModel.make - message = RouteOptionsMessage.new({ mtls_allowed_apps: app.guid }) - expect(message).to be_valid - end - - it 'allows app GUIDs with whitespace around commas' do - app1 = AppModel.make - app2 = AppModel.make - message = RouteOptionsMessage.new({ mtls_allowed_apps: "#{app1.guid} , #{app2.guid}" }) - expect(message).to be_valid - end - - it 'rejects non-existent app GUIDs' do - message = RouteOptionsMessage.new({ mtls_allowed_apps: 'non-existent-guid' }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_apps)).to include('contains non-existent app GUIDs: non-existent-guid') - end - - it 'reports multiple non-existent app GUIDs' do - message = RouteOptionsMessage.new({ mtls_allowed_apps: 'guid-1,guid-2' }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_apps)).to include('contains non-existent app GUIDs: guid-1, guid-2') - end - - it 'rejects non-string values' do - message = RouteOptionsMessage.new({ mtls_allowed_apps: ['array-not-string'] }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_apps)).to include('must be a string of comma-separated GUIDs') - end - end - - describe 'mtls_allowed_spaces validation' do - it 'allows valid comma-separated space GUIDs' do - space1 = Space.make - space2 = Space.make - message = RouteOptionsMessage.new({ mtls_allowed_spaces: "#{space1.guid},#{space2.guid}" }) - expect(message).to be_valid - end - - it 'allows single space GUID' do - space = Space.make - message = RouteOptionsMessage.new({ mtls_allowed_spaces: space.guid }) - expect(message).to be_valid - end - - it 'rejects non-existent space GUIDs' do - message = RouteOptionsMessage.new({ mtls_allowed_spaces: 'non-existent-space' }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_spaces)).to include('contains non-existent space GUIDs: non-existent-space') - end - - it 'rejects non-string values' do - message = RouteOptionsMessage.new({ mtls_allowed_spaces: { 'nested' => 'object' } }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_spaces)).to include('must be a string of comma-separated GUIDs') - end - end - - describe 'mtls_allowed_orgs validation' do - it 'allows valid comma-separated org GUIDs' do - org1 = Organization.make - org2 = Organization.make - message = RouteOptionsMessage.new({ mtls_allowed_orgs: "#{org1.guid},#{org2.guid}" }) - expect(message).to be_valid - end - - it 'allows single org GUID' do - org = Organization.make - message = RouteOptionsMessage.new({ mtls_allowed_orgs: org.guid }) - expect(message).to be_valid - end - - it 'rejects non-existent org GUIDs' do - message = RouteOptionsMessage.new({ mtls_allowed_orgs: 'non-existent-org' }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allowed_orgs)).to include('contains non-existent organization GUIDs: non-existent-org') - end - end - - describe 'mtls_allow_any validation' do - it 'allows true value' do - message = RouteOptionsMessage.new({ mtls_allow_any: true }) - expect(message).to be_valid - end - - it 'allows false value' do - message = RouteOptionsMessage.new({ mtls_allow_any: false }) - expect(message).to be_valid - end - - it 'allows string "true"' do - message = RouteOptionsMessage.new({ mtls_allow_any: 'true' }) - expect(message).to be_valid - end - - it 'allows string "false"' do - message = RouteOptionsMessage.new({ mtls_allow_any: 'false' }) - expect(message).to be_valid - end - - it 'rejects non-boolean values' do - message = RouteOptionsMessage.new({ mtls_allow_any: 'yes' }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allow_any)).to include('must be a boolean (true or false)') - end - end - - describe 'mtls_allow_any exclusivity validation' do - it 'does not allow mtls_allow_any with mtls_allowed_apps' do - app = AppModel.make - message = RouteOptionsMessage.new({ mtls_allow_any: true, mtls_allowed_apps: app.guid }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allow_any)).to include('is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') - end - - it 'does not allow mtls_allow_any with mtls_allowed_spaces' do - space = Space.make - message = RouteOptionsMessage.new({ mtls_allow_any: true, mtls_allowed_spaces: space.guid }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allow_any)).to include('is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') - end - - it 'does not allow mtls_allow_any with mtls_allowed_orgs' do - org = Organization.make - message = RouteOptionsMessage.new({ mtls_allow_any: true, mtls_allowed_orgs: org.guid }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allow_any)).to include('is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') - end - - it 'allows mtls_allow_any: false with specific GUIDs' do - app = AppModel.make - message = RouteOptionsMessage.new({ mtls_allow_any: false, mtls_allowed_apps: app.guid }) - expect(message).to be_valid - end - - it 'allows string "true" exclusivity check' do - app = AppModel.make - message = RouteOptionsMessage.new({ mtls_allow_any: 'true', mtls_allowed_apps: app.guid }) - expect(message).not_to be_valid - expect(message.errors_on(:mtls_allow_any)).to include('is mutually exclusive with mtls_allowed_apps, mtls_allowed_spaces, and mtls_allowed_orgs') - end - end - - describe 'combined options' do - it 'allows all mTLS options together (without mtls_allow_any)' do - app = AppModel.make - space = Space.make - org = Organization.make - message = RouteOptionsMessage.new({ - mtls_allowed_apps: app.guid, - mtls_allowed_spaces: space.guid, - mtls_allowed_orgs: org.guid - }) - expect(message).to be_valid - end - - it 'allows mTLS options with loadbalancing' do - app = AppModel.make - message = RouteOptionsMessage.new({ - loadbalancing: 'round-robin', - mtls_allowed_apps: app.guid - }) - expect(message).to be_valid - end - end - end - end - describe 'hash-based routing validations' do context 'when hash_based_routing feature flag is disabled' do it 'does not allow hash_header option' do diff --git a/spec/unit/presenters/v3/route_presenter_spec.rb b/spec/unit/presenters/v3/route_presenter_spec.rb index 684b132e407..3c78892c26e 100644 --- a/spec/unit/presenters/v3/route_presenter_spec.rb +++ b/spec/unit/presenters/v3/route_presenter_spec.rb @@ -147,6 +147,44 @@ module VCAP::CloudController::Presenters::V3 end end + context 'when options contains only internal mTLS keys' do + let(:route) do + VCAP::CloudController::Route.make( + host: 'foobar', + path: path, + space: space, + domain: domain, + options: { 'access_scope' => 'space', 'access_rules' => 'cf:app:some-guid' } + ) + end + + it 'omits the options key entirely from the response' do + expect(subject).not_to have_key(:options) + end + end + + context 'when options contains a mix of public and internal keys' do + let(:route) do + VCAP::CloudController::Route.make( + host: 'foobar', + path: path, + space: space, + domain: domain, + options: { + 'loadbalancing' => 'round-robin', + 'access_scope' => 'space', + 'access_rules' => 'cf:app:some-guid' + } + ) + end + + it 'exposes only the public options' do + expect(subject[:options]).to eq('loadbalancing' => 'round-robin') + expect(subject[:options]).not_to have_key('access_scope') + expect(subject[:options]).not_to have_key('access_rules') + end + end + context 'when there are decorators' do let(:banana_decorator) do Class.new do From 67f786224ed85b889e17caba5350f53a3105ec80 Mon Sep 17 00:00:00 2001 From: rkoster Date: Thu, 9 Apr 2026 12:35:18 +0000 Subject: [PATCH 07/11] Fix access_rules_controller permissions query - Replace non-existent readable_space_scoped_space_guids_query with proper subquery - Use readable_space_scoped_spaces_query for non-global readers - Handle global readers separately with all routes --- app/controllers/v3/access_rules_controller.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/v3/access_rules_controller.rb b/app/controllers/v3/access_rules_controller.rb index a45b982bb64..ac84d80f449 100644 --- a/app/controllers/v3/access_rules_controller.rb +++ b/app/controllers/v3/access_rules_controller.rb @@ -107,10 +107,12 @@ def destroy def build_dataset(message) dataset = VCAP::CloudController::RouteAccessRule.dataset - readable_route_ids = VCAP::CloudController::Route. - join(:spaces, id: :space_id). - where(Sequel.lit(permission_queryer.readable_space_scoped_space_guids_query)). - select(:routes__id) + if permission_queryer.can_read_globally? + readable_route_ids = VCAP::CloudController::Route.select(:id) + else + readable_space_ids = permission_queryer.readable_space_scoped_spaces_query.select(:id) + readable_route_ids = VCAP::CloudController::Route.where(space_id: readable_space_ids).select(:id) + end dataset = dataset.where(route_id: readable_route_ids) From 568bb0bb66f1e82d3e8e9978ba0bbf945dec41d2 Mon Sep 17 00:00:00 2001 From: rkoster Date: Thu, 9 Apr 2026 13:44:57 +0000 Subject: [PATCH 08/11] Add automatic Diego sync callbacks to RouteAccessRule - Add after_create and after_destroy callbacks to touch associated processes - Updates process.updated_at to trigger Diego ProcessesSync immediately - Eliminates 30-second wait for access rule changes to propagate to GoRouter - Add comprehensive unit tests for callbacks and validations - Ensure RouteAccessRule model is loaded in app/models.rb This enables automatic synchronization of access rules to Diego/GoRouter within seconds instead of requiring manual app restarts or waiting for the next sync cycle. --- app/models.rb | 1 + app/models/runtime/route_access_rule.rb | 22 ++++ .../models/runtime/route_access_rule_spec.rb | 112 ++++++++++++++++++ 3 files changed, 135 insertions(+) create mode 100644 spec/unit/models/runtime/route_access_rule_spec.rb diff --git a/app/models.rb b/app/models.rb index 93e1594b38d..84ddeb4813e 100644 --- a/app/models.rb +++ b/app/models.rb @@ -69,6 +69,7 @@ require 'models/runtime/revision_sidecar_model' require 'models/runtime/revision_sidecar_process_type_model' require 'models/runtime/route' +require 'models/runtime/route_access_rule' require 'models/runtime/space_routes' require 'models/runtime/space_quota_definition' require 'models/runtime/stack' diff --git a/app/models/runtime/route_access_rule.rb b/app/models/runtime/route_access_rule.rb index 08ed6c6e3e2..cf554de3fd8 100644 --- a/app/models/runtime/route_access_rule.rb +++ b/app/models/runtime/route_access_rule.rb @@ -11,5 +11,27 @@ def validate validates_presence :selector validates_presence :route_id end + + def after_create + super + touch_associated_processes + end + + def after_destroy + super + touch_associated_processes + end + + private + + def touch_associated_processes + # Update the timestamp on all processes associated with this route + # This triggers Diego's ProcessesSync to pick up the route changes + return unless route + + route.apps.each do |process| + process.update(updated_at: Time.now) + end + end end end diff --git a/spec/unit/models/runtime/route_access_rule_spec.rb b/spec/unit/models/runtime/route_access_rule_spec.rb new file mode 100644 index 00000000000..89e1a536f47 --- /dev/null +++ b/spec/unit/models/runtime/route_access_rule_spec.rb @@ -0,0 +1,112 @@ +require 'spec_helper' + +module VCAP::CloudController + RSpec.describe RouteAccessRule, type: :model do + let(:space) { Space.make } + let(:domain) { SharedDomain.make(name: 'apps.identity') } + let(:route) { Route.make(space: space, domain: domain) } + let(:process) { ProcessModelFactory.make(space: space) } + let(:app_guid) { SecureRandom.uuid } + + before do + RouteMappingModel.make(app: process, route: route, process_type: 'web') + end + + describe 'validations' do + it 'requires a name' do + rule = RouteAccessRule.new(selector: 'cf:app:123', route: route) + expect(rule.valid?).to be false + expect(rule.errors[:name]).to include("can't be blank") + end + + it 'requires a selector' do + rule = RouteAccessRule.new(name: 'test-rule', route: route) + expect(rule.valid?).to be false + expect(rule.errors[:selector]).to include("can't be blank") + end + + it 'requires a route_id' do + rule = RouteAccessRule.new(name: 'test-rule', selector: 'cf:app:123') + expect(rule.valid?).to be false + expect(rule.errors[:route_id]).to include("can't be blank") + end + end + + describe 'associations' do + it 'belongs to a route' do + rule = RouteAccessRule.create( + name: 'test-rule', + selector: 'cf:app:123', + route: route + ) + expect(rule.route).to eq(route) + end + end + + describe 'callbacks' do + describe 'after_create' do + it 'touches associated processes to trigger Diego sync' do + initial_updated_at = process.updated_at + + # Sleep to ensure timestamp difference + sleep 0.1 + + RouteAccessRule.create( + name: 'test-rule', + selector: "cf:app:#{app_guid}", + route: route + ) + + process.reload + expect(process.updated_at).to be > initial_updated_at + end + + it 'does not fail if route has no associated processes' do + route_without_processes = Route.make(space: space, domain: domain) + + expect { + RouteAccessRule.create( + name: 'test-rule', + selector: "cf:app:#{app_guid}", + route: route_without_processes + ) + }.not_to raise_error + end + end + + describe 'after_destroy' do + it 'touches associated processes to trigger Diego sync' do + rule = RouteAccessRule.create( + name: 'test-rule', + selector: "cf:app:#{app_guid}", + route: route + ) + + process.reload + initial_updated_at = process.updated_at + + # Sleep to ensure timestamp difference + sleep 0.1 + + rule.destroy + + process.reload + expect(process.updated_at).to be > initial_updated_at + end + + it 'does not fail if route has no associated processes' do + route_without_processes = Route.make(space: space, domain: domain) + rule = RouteAccessRule.create( + name: 'test-rule', + selector: "cf:app:#{app_guid}", + route: route_without_processes + ) + + expect { + rule.destroy + }.not_to raise_error + end + end + end + end +end From ea99dbb7daef31354b8fceb6c7ad78b9e269fb07 Mon Sep 17 00:00:00 2001 From: rkoster Date: Fri, 10 Apr 2026 06:38:05 +0000 Subject: [PATCH 09/11] Implement include=selector_resource for /v3/access_rules endpoint - Add include parameter support to AccessRulesListMessage - Refactor IncludeAccessRuleSelectorResourceDecorator to match RFC format: - Return separate arrays for apps, spaces, organizations instead of selector_resources - Include full resource details using appropriate presenters - Batch resource fetching by type with eager loading - Auto-deduplicate resources - Gracefully handle stale/missing resources - Wire up decorator to AccessRulesController - Add comprehensive request specs for include=selector_resource Fixes: uninitialized constant error by adding proper require statement --- app/controllers/v3/access_rules_controller.rb | 7 +- ...access_rule_selector_resource_decorator.rb | 60 ++++++--- app/messages/access_rules_list_message.rb | 4 +- spec/request/access_rules_spec.rb | 114 ++++++++++++++++++ 4 files changed, 169 insertions(+), 16 deletions(-) diff --git a/app/controllers/v3/access_rules_controller.rb b/app/controllers/v3/access_rules_controller.rb index ac84d80f449..a64fb16e66f 100644 --- a/app/controllers/v3/access_rules_controller.rb +++ b/app/controllers/v3/access_rules_controller.rb @@ -2,6 +2,7 @@ require 'messages/access_rule_update_message' require 'messages/access_rules_list_message' require 'presenters/v3/access_rule_presenter' +require 'decorators/include_access_rule_selector_resource_decorator' class AccessRulesController < ApplicationController def index @@ -10,11 +11,15 @@ def index dataset = build_dataset(message) + decorators = [] + decorators << IncludeAccessRuleSelectorResourceDecorator if IncludeAccessRuleSelectorResourceDecorator.match?(message.include) + render status: :ok, json: Presenters::V3::PaginatedListPresenter.new( presenter: Presenters::V3::AccessRulePresenter, paginated_result: SequelPaginator.new.get_page(dataset, message.try(:pagination_options)), path: '/v3/access_rules', - message: message + message: message, + decorators: decorators ) end diff --git a/app/decorators/include_access_rule_selector_resource_decorator.rb b/app/decorators/include_access_rule_selector_resource_decorator.rb index c5ac7552860..cd85dd0ef1c 100644 --- a/app/decorators/include_access_rule_selector_resource_decorator.rb +++ b/app/decorators/include_access_rule_selector_resource_decorator.rb @@ -10,7 +10,12 @@ def self.match?(include_params) end def self.decorate(hash, access_rules) - included = [] + hash[:included] ||= {} + + # Collect all GUIDs by type + app_guids = [] + space_guids = [] + org_guids = [] access_rules.each do |rule| match = SELECTOR_REGEX.match(rule.selector) @@ -19,22 +24,49 @@ def self.decorate(hash, access_rules) resource_type = match[1] resource_guid = match[2] - resource = case resource_type - when 'app' - VCAP::CloudController::AppModel.find(guid: resource_guid) - when 'space' - VCAP::CloudController::Space.find(guid: resource_guid) - when 'org' - VCAP::CloudController::Organization.find(guid: resource_guid) - end - - next if resource.nil? - - included << { type: resource_type, guid: resource.guid } + case resource_type + when 'app' + app_guids << resource_guid + when 'space' + space_guids << resource_guid + when 'org' + org_guids << resource_guid + end end - hash[:included] = { selector_resources: included } + # Fetch and present resources + hash[:included][:apps] = fetch_and_present_apps(app_guids.uniq) + hash[:included][:spaces] = fetch_and_present_spaces(space_guids.uniq) + hash[:included][:organizations] = fetch_and_present_organizations(org_guids.uniq) + hash end + + private_class_method def self.fetch_and_present_apps(guids) + return [] if guids.empty? + + apps = VCAP::CloudController::AppModel.where(guid: guids). + order(:created_at, :guid). + eager(VCAP::CloudController::Presenters::V3::AppPresenter.associated_resources).all + apps.map { |app| VCAP::CloudController::Presenters::V3::AppPresenter.new(app).to_hash } + end + + private_class_method def self.fetch_and_present_spaces(guids) + return [] if guids.empty? + + spaces = VCAP::CloudController::Space.where(guid: guids). + order(:created_at, :guid). + eager(VCAP::CloudController::Presenters::V3::SpacePresenter.associated_resources).all + spaces.map { |space| VCAP::CloudController::Presenters::V3::SpacePresenter.new(space).to_hash } + end + + private_class_method def self.fetch_and_present_organizations(guids) + return [] if guids.empty? + + orgs = VCAP::CloudController::Organization.where(guid: guids). + order(:created_at, :guid). + eager(VCAP::CloudController::Presenters::V3::OrganizationPresenter.associated_resources).all + orgs.map { |org| VCAP::CloudController::Presenters::V3::OrganizationPresenter.new(org).to_hash } + end end end diff --git a/app/messages/access_rules_list_message.rb b/app/messages/access_rules_list_message.rb index 7c7973fda97..b2eb08002bf 100644 --- a/app/messages/access_rules_list_message.rb +++ b/app/messages/access_rules_list_message.rb @@ -6,12 +6,14 @@ class AccessRulesListMessage < ListMessage route_guids names selectors + include ] validates_with NoAdditionalParamsValidator + validates_with IncludeParamValidator, valid_values: ['selector_resource', 'route'] def self.from_params(params) - super(params, %w[route_guids names selectors]) + super(params, %w[route_guids names selectors include]) end end end diff --git a/spec/request/access_rules_spec.rb b/spec/request/access_rules_spec.rb index 3962cc59a66..4828cac2660 100644 --- a/spec/request/access_rules_spec.rb +++ b/spec/request/access_rules_spec.rb @@ -300,6 +300,120 @@ def expected_rule_json(rule) expect(parsed['resources'].length).to eq(1) expect(parsed['resources'][0]['selector']).to eq('cf:any') end + + context 'with include=selector_resource' do + let!(:app) { VCAP::CloudController::AppModel.make(space: space, name: 'frontend-app') } + let!(:other_space) { VCAP::CloudController::Space.make(organization: org, name: 'other-space') } + let!(:other_org) { VCAP::CloudController::Organization.make(name: 'other-org') } + + let!(:app_rule) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'app-rule', + selector: "cf:app:#{app.guid}", + route_id: mtls_route.id + ) + end + + let!(:space_rule) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'space-rule', + selector: "cf:space:#{other_space.guid}", + route_id: mtls_route.id + ) + end + + let!(:org_rule) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'org-rule', + selector: "cf:org:#{other_org.guid}", + route_id: mtls_route.id + ) + end + + it 'includes resolved selector resources' do + get '/v3/access_rules?include=selector_resource', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + + # Check included structure + expect(parsed['included']).to be_a(Hash) + expect(parsed['included']['apps']).to be_an(Array) + expect(parsed['included']['spaces']).to be_an(Array) + expect(parsed['included']['organizations']).to be_an(Array) + + # Check app is included with full details + app_included = parsed['included']['apps'].find { |a| a['guid'] == app.guid } + expect(app_included).to be_present + expect(app_included['name']).to eq('frontend-app') + expect(app_included['guid']).to eq(app.guid) + + # Check space is included + space_included = parsed['included']['spaces'].find { |s| s['guid'] == other_space.guid } + expect(space_included).to be_present + expect(space_included['name']).to eq('other-space') + + # Check org is included + org_included = parsed['included']['organizations'].find { |o| o['guid'] == other_org.guid } + expect(org_included).to be_present + expect(org_included['name']).to eq('other-org') + end + + it 'handles stale resources (missing GUIDs) gracefully' do + stale_guid = '99999999-9999-9999-9999-999999999999' + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'stale-rule', + selector: "cf:app:#{stale_guid}", + route_id: mtls_route.id + ) + + get '/v3/access_rules?include=selector_resource', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + + # Stale resource should not appear in included + stale_app = parsed['included']['apps'].find { |a| a['guid'] == stale_guid } + expect(stale_app).to be_nil + end + + it 'includes only unique resources when multiple rules reference the same resource' do + # Create another rule referencing the same app + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'another-app-rule', + selector: "cf:app:#{app.guid}", + route_id: VCAP::CloudController::Route.make(space: space, domain: mtls_domain).id + ) + + get '/v3/access_rules?include=selector_resource', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + + # App should appear only once + app_count = parsed['included']['apps'].count { |a| a['guid'] == app.guid } + expect(app_count).to eq(1) + end + + it 'does not include resources for cf:any selectors' do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'any-rule', + selector: 'cf:any', + route_id: VCAP::CloudController::Route.make(space: space, domain: mtls_domain).id + ) + + get '/v3/access_rules?include=selector_resource', nil, admin_header + + expect(last_response.status).to eq(200) + # Should succeed without error even with cf:any selector + end + end end describe 'DELETE /v3/access_rules/:guid' do From 8903be969c9290c2f20a11f4ab577887189aad35 Mon Sep 17 00:00:00 2001 From: rkoster Date: Fri, 10 Apr 2026 08:18:47 +0000 Subject: [PATCH 10/11] Add space_guids filtering to /v3/access_rules endpoint Implement space-based filtering for access rules endpoint to enable querying all access rules within a given space using ?space_guids= query parameter. Changes: - Add space_guids to AccessRulesListMessage with array validation - Implement space filtering in AccessRulesController#build_dataset - Add comprehensive unit tests for AccessRulesListMessage - Add request specs for single/multiple space filtering and combinations - Follow existing CAPI patterns for space_guids filtering The filter joins through the routes table to filter access rules by the space_id of their associated routes. --- app/controllers/v3/access_rules_controller.rb | 7 + app/messages/access_rules_list_message.rb | 5 +- spec/request/access_rules_spec.rb | 67 +++++++++ .../access_rules_list_message_spec.rb | 135 ++++++++++++++++++ 4 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 spec/unit/messages/access_rules_list_message_spec.rb diff --git a/app/controllers/v3/access_rules_controller.rb b/app/controllers/v3/access_rules_controller.rb index a64fb16e66f..eb6ff20aa0e 100644 --- a/app/controllers/v3/access_rules_controller.rb +++ b/app/controllers/v3/access_rules_controller.rb @@ -128,6 +128,13 @@ def build_dataset(message) select_all(:route_access_rules) end + if message.requested?(:space_guids) + dataset = dataset. + join(:routes, id: :route_id). + where(routes__space_id: VCAP::CloudController::Space.where(guid: message.space_guids).select(:id)). + select_all(:route_access_rules) + end + dataset = dataset.where(name: message.names) if message.requested?(:names) dataset = dataset.where(selector: message.selectors) if message.requested?(:selectors) diff --git a/app/messages/access_rules_list_message.rb b/app/messages/access_rules_list_message.rb index b2eb08002bf..ddf22935f51 100644 --- a/app/messages/access_rules_list_message.rb +++ b/app/messages/access_rules_list_message.rb @@ -4,6 +4,7 @@ module VCAP::CloudController class AccessRulesListMessage < ListMessage register_allowed_keys %i[ route_guids + space_guids names selectors include @@ -12,8 +13,10 @@ class AccessRulesListMessage < ListMessage validates_with NoAdditionalParamsValidator validates_with IncludeParamValidator, valid_values: ['selector_resource', 'route'] + validates :space_guids, array: true, allow_nil: true + def self.from_params(params) - super(params, %w[route_guids names selectors include]) + super(params, %w[route_guids space_guids names selectors include]) end end end diff --git a/spec/request/access_rules_spec.rb b/spec/request/access_rules_spec.rb index 4828cac2660..95e90b51cca 100644 --- a/spec/request/access_rules_spec.rb +++ b/spec/request/access_rules_spec.rb @@ -301,6 +301,73 @@ def expected_rule_json(rule) expect(parsed['resources'][0]['selector']).to eq('cf:any') end + describe 'filtering by space_guids' do + let(:other_org) { VCAP::CloudController::Organization.make } + let(:other_space) { VCAP::CloudController::Space.make(organization: other_org) } + let(:other_mtls_domain) do + VCAP::CloudController::PrivateDomain.make( + owning_organization: other_org, + enforce_access_rules: true, + access_rules_scope: 'space' + ) + end + let(:other_route) { VCAP::CloudController::Route.make(space: other_space, domain: other_mtls_domain) } + let!(:rule_in_other_space) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'rule-in-other-space', + selector: 'cf:any', + route_id: other_route.id + ) + end + + before do + other_org.add_user(user) + other_space.add_developer(user) + end + + it 'filters by single space_guid' do + get "/v3/access_rules?space_guids=#{space.guid}", nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + guids = parsed['resources'].map { |r| r['guid'] } + expect(guids).to include(rule1.guid, rule2.guid) + expect(guids).not_to include(rule_in_other_space.guid) + end + + it 'filters by multiple space_guids' do + get "/v3/access_rules?space_guids=#{space.guid},#{other_space.guid}", nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + guids = parsed['resources'].map { |r| r['guid'] } + expect(guids).to include(rule1.guid, rule2.guid, rule_in_other_space.guid) + end + + it 'combines space_guids with other filters' do + get "/v3/access_rules?space_guids=#{space.guid}&names=rule-one", nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + expect(parsed['resources'].length).to eq(1) + expect(parsed['resources'][0]['guid']).to eq(rule1.guid) + expect(parsed['resources'][0]['name']).to eq('rule-one') + end + + it 'returns empty when space has no access rules' do + empty_space = VCAP::CloudController::Space.make(organization: org) + org.add_user(user) + empty_space.add_developer(user) + + get "/v3/access_rules?space_guids=#{empty_space.guid}", nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + expect(parsed['resources'].length).to eq(0) + end + end + context 'with include=selector_resource' do let!(:app) { VCAP::CloudController::AppModel.make(space: space, name: 'frontend-app') } let!(:other_space) { VCAP::CloudController::Space.make(organization: org, name: 'other-space') } diff --git a/spec/unit/messages/access_rules_list_message_spec.rb b/spec/unit/messages/access_rules_list_message_spec.rb new file mode 100644 index 00000000000..443fdf70bfd --- /dev/null +++ b/spec/unit/messages/access_rules_list_message_spec.rb @@ -0,0 +1,135 @@ +require 'spec_helper' +require 'messages/access_rules_list_message' + +module VCAP::CloudController + RSpec.describe AccessRulesListMessage do + describe '.from_params' do + let(:params) do + { + 'route_guids' => 'route1,route2', + 'space_guids' => 'space1,space2', + 'names' => 'name1,name2', + 'selectors' => 'selector1,selector2', + 'page' => 1, + 'per_page' => 5, + 'order_by' => 'created_at', + 'include' => 'selector_resource,route' + } + end + + it 'returns the correct AccessRulesListMessage' do + message = AccessRulesListMessage.from_params(params) + + expect(message).to be_a(AccessRulesListMessage) + expect(message.route_guids).to eq(%w[route1 route2]) + expect(message.space_guids).to eq(%w[space1 space2]) + expect(message.names).to eq(%w[name1 name2]) + expect(message.selectors).to eq(%w[selector1 selector2]) + expect(message.page).to eq(1) + expect(message.per_page).to eq(5) + expect(message.order_by).to eq('created_at') + expect(message.include).to eq(%w[selector_resource route]) + end + + it 'converts requested keys to symbols' do + message = AccessRulesListMessage.from_params(params) + + expect(message).to be_requested(:route_guids) + expect(message).to be_requested(:space_guids) + expect(message).to be_requested(:names) + expect(message).to be_requested(:selectors) + expect(message).to be_requested(:page) + expect(message).to be_requested(:per_page) + expect(message).to be_requested(:order_by) + expect(message).to be_requested(:include) + end + end + + describe '#to_param_hash' do + let(:opts) do + { + route_guids: %w[route1 route2], + space_guids: %w[space1 space2], + names: %w[name1 name2], + selectors: %w[selector1 selector2], + page: 1, + per_page: 5, + order_by: 'created_at', + include: %w[selector_resource route] + } + end + + it 'excludes the pagination keys' do + expected_params = %i[route_guids space_guids names selectors include] + expect(AccessRulesListMessage.from_params(opts).to_param_hash.keys).to match_array(expected_params) + end + end + + describe 'fields' do + it 'accepts a set of fields' do + expect do + AccessRulesListMessage.from_params({ + route_guids: [], + space_guids: [], + names: [], + selectors: [], + page: 1, + per_page: 5, + order_by: 'created_at', + include: ['selector_resource', 'route'] + }) + end.not_to raise_error + end + + it 'accepts an empty set' do + message = AccessRulesListMessage.from_params({}) + expect(message).to be_valid + end + + it 'does not accept a field not in this set' do + message = AccessRulesListMessage.from_params({ foobar: 'pants' }) + + expect(message).not_to be_valid + expect(message.errors[:base][0]).to include("Unknown query parameter(s): 'foobar'") + end + + describe 'include validations' do + it 'accepts valid include values' do + message = AccessRulesListMessage.from_params({ 'include' => 'selector_resource' }) + expect(message).to be_valid + + message = AccessRulesListMessage.from_params({ 'include' => 'route' }) + expect(message).to be_valid + + message = AccessRulesListMessage.from_params({ 'include' => 'selector_resource,route' }) + expect(message).to be_valid + end + + it 'rejects invalid include values' do + message = AccessRulesListMessage.from_params({ 'include' => 'invalid' }) + expect(message).not_to be_valid + end + end + + describe 'validations' do + it 'validates space_guids is an array' do + message = AccessRulesListMessage.from_params space_guids: 'not array' + expect(message).not_to be_valid + expect(message.errors[:space_guids].length).to eq 1 + end + + it 'allows space_guids to be nil' do + message = AccessRulesListMessage.from_params({}) + expect(message).to be_valid + expect(message.space_guids).to be_nil + end + + it 'allows space_guids to be an array' do + message = AccessRulesListMessage.from_params space_guids: %w[space1 space2] + expect(message).to be_valid + expect(message.space_guids).to eq(%w[space1 space2]) + end + end + end + end +end From e6415864b9a96414f9538800d218d3b777e4e5fd Mon Sep 17 00:00:00 2001 From: rkoster Date: Fri, 10 Apr 2026 08:35:29 +0000 Subject: [PATCH 11/11] Implement include=route for /v3/access_rules endpoint Add support for including route resources when listing access rules via the ?include=route query parameter. Changes: - Create IncludeAccessRuleRouteDecorator to handle route inclusion - Wire up decorator in AccessRulesController - Add comprehensive request specs for include=route - Test single/multiple routes, uniqueness, and combining with selector_resource - Follow existing CAPI decorator patterns for resource inclusion The decorator fetches and presents Route resources referenced by the access rules, adding them to the 'included' section of the response. --- app/controllers/v3/access_rules_controller.rb | 34 +++---- .../include_access_rule_route_decorator.rb | 27 ++++++ spec/request/access_rules_spec.rb | 94 ++++++++++++++++++- 3 files changed, 132 insertions(+), 23 deletions(-) create mode 100644 app/decorators/include_access_rule_route_decorator.rb diff --git a/app/controllers/v3/access_rules_controller.rb b/app/controllers/v3/access_rules_controller.rb index eb6ff20aa0e..73876128299 100644 --- a/app/controllers/v3/access_rules_controller.rb +++ b/app/controllers/v3/access_rules_controller.rb @@ -3,6 +3,7 @@ require 'messages/access_rules_list_message' require 'presenters/v3/access_rule_presenter' require 'decorators/include_access_rule_selector_resource_decorator' +require 'decorators/include_access_rule_route_decorator' class AccessRulesController < ApplicationController def index @@ -13,6 +14,7 @@ def index decorators = [] decorators << IncludeAccessRuleSelectorResourceDecorator if IncludeAccessRuleSelectorResourceDecorator.match?(message.include) + decorators << IncludeAccessRuleRouteDecorator if IncludeAccessRuleRouteDecorator.match?(message.include) render status: :ok, json: Presenters::V3::PaginatedListPresenter.new( presenter: Presenters::V3::AccessRulePresenter, @@ -42,27 +44,17 @@ def create unauthorized! unless permission_queryer.can_write_to_active_space?(route.space.id) suspended! unless permission_queryer.is_space_active?(route.space.id) - unless route.domain.enforce_access_rules - unprocessable!("Cannot create access rules for route '#{route.guid}': the route's domain does not have enforce_access_rules enabled.") - end + unprocessable!("Cannot create access rules for route '#{route.guid}': the route's domain does not have enforce_access_rules enabled.") unless route.domain.enforce_access_rules # Enforce cf:any exclusivity: if route already has a cf:any rule, reject new rules; # if new rule is cf:any, reject if route already has any rules. existing_selectors = route.access_rules.map(&:selector) - if message.selector == 'cf:any' && existing_selectors.any? - unprocessable!("Cannot add 'cf:any' selector when other access rules already exist for this route.") - end - if existing_selectors.include?('cf:any') && message.selector != 'cf:any' - unprocessable!("Cannot add selector '#{message.selector}': route already has a 'cf:any' rule.") - end + unprocessable!("Cannot add 'cf:any' selector when other access rules already exist for this route.") if message.selector == 'cf:any' && existing_selectors.any? + unprocessable!("Cannot add selector '#{message.selector}': route already has a 'cf:any' rule.") if existing_selectors.include?('cf:any') && message.selector != 'cf:any' # Uniqueness: name and selector must be unique per route - if route.access_rules.any? { |r| r.name == message.name } - unprocessable!("An access rule with name '#{message.name}' already exists for this route.") - end - if existing_selectors.include?(message.selector) - unprocessable!("An access rule with selector '#{message.selector}' already exists for this route.") - end + unprocessable!("An access rule with name '#{message.name}' already exists for this route.") if route.access_rules.any? { |r| r.name == message.name } + unprocessable!("An access rule with selector '#{message.selector}' already exists for this route.") if existing_selectors.include?(message.selector) access_rule = VCAP::CloudController::RouteAccessRule.new( guid: SecureRandom.uuid, @@ -123,16 +115,16 @@ def build_dataset(message) if message.requested?(:route_guids) dataset = dataset. - join(:routes, id: :route_id). - where(routes__guid: message.route_guids). - select_all(:route_access_rules) + join(:routes, id: :route_id). + where(routes__guid: message.route_guids). + select_all(:route_access_rules) end if message.requested?(:space_guids) dataset = dataset. - join(:routes, id: :route_id). - where(routes__space_id: VCAP::CloudController::Space.where(guid: message.space_guids).select(:id)). - select_all(:route_access_rules) + join(:routes, id: :route_id). + where(routes__space_id: VCAP::CloudController::Space.where(guid: message.space_guids).select(:id)). + select_all(:route_access_rules) end dataset = dataset.where(name: message.names) if message.requested?(:names) diff --git a/app/decorators/include_access_rule_route_decorator.rb b/app/decorators/include_access_rule_route_decorator.rb new file mode 100644 index 00000000000..178da8be3db --- /dev/null +++ b/app/decorators/include_access_rule_route_decorator.rb @@ -0,0 +1,27 @@ +module VCAP::CloudController + class IncludeAccessRuleRouteDecorator + # Handles `?include=route` for GET /v3/access_rules + # Includes the route resources associated with the access rules + + def self.match?(include_params) + include_params&.include?('route') + end + + def self.decorate(hash, access_rules) + hash[:included] ||= {} + + # Collect all unique route IDs from access rules + route_ids = access_rules.map(&:route_id).uniq + + # Fetch routes with their associations + routes = VCAP::CloudController::Route.where(id: route_ids). + order(:created_at, :guid). + eager(VCAP::CloudController::Presenters::V3::RoutePresenter.associated_resources).all + + # Present routes + hash[:included][:routes] = routes.map { |route| VCAP::CloudController::Presenters::V3::RoutePresenter.new(route).to_hash } + + hash + end + end +end diff --git a/spec/request/access_rules_spec.rb b/spec/request/access_rules_spec.rb index 95e90b51cca..4fdd65f5736 100644 --- a/spec/request/access_rules_spec.rb +++ b/spec/request/access_rules_spec.rb @@ -133,7 +133,7 @@ def expected_rule_json(rule) }.to_json, admin_header expect(last_response.status).to eq(422) - expect(last_response.body).to include("cf:any") + expect(last_response.body).to include('cf:any') end end @@ -155,7 +155,7 @@ def expected_rule_json(rule) }.to_json, admin_header expect(last_response.status).to eq(422) - expect(last_response.body).to include("cf:any") + expect(last_response.body).to include('cf:any') end end @@ -481,6 +481,96 @@ def expected_rule_json(rule) # Should succeed without error even with cf:any selector end end + + context 'with include=route' do + let(:route2) { VCAP::CloudController::Route.make(space: space, domain: mtls_domain) } + + let!(:rule_on_route1) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'rule-on-route1', + selector: 'cf:any', + route_id: mtls_route.id + ) + end + + let!(:rule_on_route2) do + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'rule-on-route2', + selector: "cf:app:#{valid_uuid}", + route_id: route2.id + ) + end + + it 'includes route resources' do + get '/v3/access_rules?include=route', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + + # Check included structure + expect(parsed['included']).to be_a(Hash) + expect(parsed['included']['routes']).to be_an(Array) + expect(parsed['included']['routes'].length).to be >= 2 + + # Check routes are included with full details + route1_included = parsed['included']['routes'].find { |r| r['guid'] == mtls_route.guid } + expect(route1_included).to be_present + expect(route1_included['guid']).to eq(mtls_route.guid) + expect(route1_included['url']).to be_present + + route2_included = parsed['included']['routes'].find { |r| r['guid'] == route2.guid } + expect(route2_included).to be_present + expect(route2_included['guid']).to eq(route2.guid) + end + + it 'includes only unique routes when multiple rules reference the same route' do + # Create another rule on the same route + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'another-rule-on-route1', + selector: "cf:app:#{valid_uuid}", + route_id: mtls_route.id + ) + + get '/v3/access_rules?include=route', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + + # Route should appear only once + route_count = parsed['included']['routes'].count { |r| r['guid'] == mtls_route.guid } + expect(route_count).to eq(1) + end + + it 'combines include=route with include=selector_resource' do + app = VCAP::CloudController::AppModel.make(space: space, name: 'test-app') + VCAP::CloudController::RouteAccessRule.create( + guid: SecureRandom.uuid, + name: 'combined-rule', + selector: "cf:app:#{app.guid}", + route_id: mtls_route.id + ) + + get '/v3/access_rules?include=route,selector_resource', nil, admin_header + + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + + # Both routes and selector resources should be included + expect(parsed['included']['routes']).to be_an(Array) + expect(parsed['included']['apps']).to be_an(Array) + + # Verify route is present + route_included = parsed['included']['routes'].find { |r| r['guid'] == mtls_route.guid } + expect(route_included).to be_present + + # Verify app is present + app_included = parsed['included']['apps'].find { |a| a['guid'] == app.guid } + expect(app_included).to be_present + end + end end describe 'DELETE /v3/access_rules/:guid' do