diff --git a/app/actions/route_policy_create.rb b/app/actions/route_policy_create.rb index 8540df42d63..b2905c15b46 100644 --- a/app/actions/route_policy_create.rb +++ b/app/actions/route_policy_create.rb @@ -1,12 +1,18 @@ +require 'repositories/route_policy_event_repository' + module VCAP::CloudController class RoutePolicyCreate class Error < StandardError end + def initialize(user_audit_info) + @user_audit_info = user_audit_info + end + def create(route:, message:) validate_scope!(route.domain, message.source) - RoutePolicy.db.transaction do + route_policy = RoutePolicy.db.transaction do # Lock the parent route row to serialize concurrent creates. # SELECT ... FOR UPDATE on an empty policies table acquires no row locks, # so two concurrent transactions can both read [] and both pass cf:any @@ -14,16 +20,27 @@ def create(route:, message:) # ensures they serialize regardless of how many policies currently exist. Route.where(id: route.id).for_update.first or raise Error.new("Route '#{route.guid}' not found.") - existing_policies = RoutePolicy.where(route_id: route.id).all - validate_source_exclusivity(existing_policies, message.source) - - RoutePolicy.create( + policy = RoutePolicy.create( source: message.source, route_id: route.id ) + + Repositories::RoutePolicyEventRepository.new.record_route_policy_create( + policy, + @user_audit_info, + { 'source' => message.source, 'route_guid' => route.guid } + ) + + policy end + + route_policy.notify_diego + + route_policy rescue Sequel::UniqueConstraintViolation raise Error.new("A route policy with source '#{message.source}' already exists for this route.") + rescue Sequel::ValidationFailed => e + raise Error.new(e.errors.full_messages.join(', ')) end private @@ -34,14 +51,5 @@ def validate_scope!(domain, source) raise Error.new("Source '#{source}' is not allowed: domain's route_policies_scope is 'space'.") end - - def validate_source_exclusivity(locked_policies, source) - existing_sources = locked_policies.map(&:source) - - # Enforce cf:any exclusivity: if new policy is cf:any, reject if route already has any policies; - # if route already has a cf:any policy, reject new policies. - raise Error.new("Cannot add 'cf:any' source when other route policies already exist for this route.") if source == 'cf:any' && existing_sources.any? - raise Error.new("Cannot add source '#{source}': route already has a 'cf:any' policy.") if existing_sources.include?('cf:any') - end end end diff --git a/app/actions/route_policy_destroy.rb b/app/actions/route_policy_destroy.rb new file mode 100644 index 00000000000..a17f81624c1 --- /dev/null +++ b/app/actions/route_policy_destroy.rb @@ -0,0 +1,24 @@ +require 'repositories/route_policy_event_repository' + +module VCAP::CloudController + class RoutePolicyDestroy + def initialize(user_audit_info) + @user_audit_info = user_audit_info + end + + def delete(route_policy) + RoutePolicy.db.transaction do + route_policy.destroy + + Repositories::RoutePolicyEventRepository.new.record_route_policy_delete( + route_policy, + @user_audit_info + ) + end + + route_policy.notify_diego + + nil + end + end +end diff --git a/app/actions/route_policy_update.rb b/app/actions/route_policy_update.rb new file mode 100644 index 00000000000..d31430b23aa --- /dev/null +++ b/app/actions/route_policy_update.rb @@ -0,0 +1,23 @@ +require 'repositories/route_policy_event_repository' + +module VCAP::CloudController + class RoutePolicyUpdate + def initialize(user_audit_info) + @user_audit_info = user_audit_info + end + + def update(route_policy, message) + RoutePolicy.db.transaction do + MetadataUpdate.update(route_policy, message) + + Repositories::RoutePolicyEventRepository.new.record_route_policy_update( + route_policy, + @user_audit_info, + message.audit_hash + ) + end + + route_policy + end + end +end diff --git a/app/controllers/v3/route_policies_controller.rb b/app/controllers/v3/route_policies_controller.rb index cade4523880..0fe23c7b67f 100755 --- a/app/controllers/v3/route_policies_controller.rb +++ b/app/controllers/v3/route_policies_controller.rb @@ -6,7 +6,8 @@ require 'decorators/include_route_policy_source_decorator' require 'decorators/include_route_policy_route_decorator' require 'actions/route_policy_create' -require 'repositories/route_policy_event_repository' +require 'actions/route_policy_update' +require 'actions/route_policy_destroy' require 'fetchers/label_selector_query_generator' class RoutePoliciesController < ApplicationController @@ -33,7 +34,7 @@ def show message = RoutePolicyShowMessage.from_params(query_params) unprocessable!(message.errors.full_messages) unless message.valid? - route_policy = VCAP::CloudController::RoutePolicy.find(guid: hashed_params[:guid]) + route_policy = find_route_policy_with_route_and_space(hashed_params[:guid]) resource_not_found!(:route_policy) unless route_policy route = route_policy.route @@ -53,13 +54,7 @@ def create route = find_and_authorize_route(message.route_guid) validate_route_domain(route) - route_policy = VCAP::CloudController::RoutePolicyCreate.new.create(route: route, message: message) - - route_policy_event_repository.record_route_policy_create( - route_policy, - user_audit_info, - { 'source' => message.source, 'route_guid' => message.route_guid } - ) + route_policy = VCAP::CloudController::RoutePolicyCreate.new(user_audit_info).create(route: route, message: message) render status: :created, json: Presenters::V3::RoutePolicyPresenter.new(route_policy) rescue VCAP::CloudController::RoutePolicyCreate::Error => e @@ -67,7 +62,7 @@ def create end def update - route_policy = VCAP::CloudController::RoutePolicy.find(guid: hashed_params[:guid]) + route_policy = find_route_policy_with_route_and_space(hashed_params[:guid]) resource_not_found!(:route_policy) unless route_policy find_and_authorize_route_for_policy(route_policy) @@ -75,36 +70,26 @@ def update message = RoutePolicyUpdateMessage.new(hashed_params[:body]) unprocessable!(message.errors.full_messages) unless message.valid? - VCAP::CloudController::MetadataUpdate.update(route_policy, message) + route_policy = VCAP::CloudController::RoutePolicyUpdate.new(user_audit_info).update(route_policy, message) - route_policy_event_repository.record_route_policy_update( - route_policy.reload, - user_audit_info, - message.audit_hash - ) - - render status: :ok, json: Presenters::V3::RoutePolicyPresenter.new(route_policy.reload) + render status: :ok, json: Presenters::V3::RoutePolicyPresenter.new(route_policy) end def destroy - route_policy = VCAP::CloudController::RoutePolicy.find(guid: hashed_params[:guid]) + route_policy = find_route_policy_with_route_and_space(hashed_params[:guid]) resource_not_found!(:route_policy) unless route_policy find_and_authorize_route_for_policy(route_policy) - route_policy.destroy + VCAP::CloudController::RoutePolicyDestroy.new(user_audit_info).delete(route_policy) - route_policy_event_repository.record_route_policy_delete( - route_policy, - user_audit_info - ) head :no_content end private - def route_policy_event_repository - @route_policy_event_repository ||= Repositories::RoutePolicyEventRepository.new + def find_route_policy_with_route_and_space(guid) + VCAP::CloudController::RoutePolicy.eager_graph(route: :space).where(Sequel[:route_policies][:guid] => guid).all[0] end def find_and_authorize_route(route_guid) diff --git a/app/decorators/include_route_policies_decorator.rb b/app/decorators/include_route_policies_decorator.rb index dccb346434e..b3020df3c89 100644 --- a/app/decorators/include_route_policies_decorator.rb +++ b/app/decorators/include_route_policies_decorator.rb @@ -2,14 +2,15 @@ module VCAP::CloudController class IncludeRoutePoliciesDecorator class << self def match?(include) - include&.any? { |i| %w[route_policies].include?(i) } + include&.include?('route_policies') end def decorate(hash, routes) hash[:included] ||= {} route_ids = routes.map(&:id).uniq route_policies = RoutePolicy.where(route_id: route_ids). - eager(:route, :labels, :annotations).all + order(:created_at, :guid). + eager(Presenters::V3::RoutePolicyPresenter.associated_resources).all hash[:included][:route_policies] = route_policies.map { |rp| Presenters::V3::RoutePolicyPresenter.new(rp).to_hash } hash diff --git a/app/decorators/include_route_policy_route_decorator.rb b/app/decorators/include_route_policy_route_decorator.rb index dbc4e1ea04f..6fb2029f4cf 100644 --- a/app/decorators/include_route_policy_route_decorator.rb +++ b/app/decorators/include_route_policy_route_decorator.rb @@ -3,25 +3,24 @@ class IncludeRoutePolicyRouteDecorator # Handles `?include=route` for GET /v3/route_policies # Includes the route resources associated with the route policies - def self.match?(include_params) - include_params&.include?('route') - end + class << self + def match?(include_params) + include_params&.include?('route') + end - def self.decorate(hash, route_policies) - hash[:included] ||= {} + def decorate(hash, route_policies) + hash[:included] ||= {} - # Collect all unique route IDs from route policies - route_ids = route_policies.map(&:route_id).uniq + route_ids = route_policies.map(&:route_id).uniq - # Fetch routes with their associations - routes = Route.where(id: route_ids). - order(:created_at, :guid). - eager(Presenters::V3::RoutePresenter.associated_resources).all + routes = Route.where(id: route_ids). + order(:created_at, :guid). + eager(Presenters::V3::RoutePresenter.associated_resources).all - # Present routes - hash[:included][:routes] = routes.map { |route| Presenters::V3::RoutePresenter.new(route).to_hash } + hash[:included][:routes] = routes.map { |route| Presenters::V3::RoutePresenter.new(route).to_hash } - hash + hash + end end end end diff --git a/app/decorators/include_route_policy_source_decorator.rb b/app/decorators/include_route_policy_source_decorator.rb index 72461f9d2d3..c89d1e72be9 100644 --- a/app/decorators/include_route_policy_source_decorator.rb +++ b/app/decorators/include_route_policy_source_decorator.rb @@ -4,68 +4,68 @@ class IncludeRoutePolicySourceDecorator # Stale/missing resources (source GUIDs that no longer exist) are silently absent. # Resources the current user cannot read are also silently absent. - def self.match?(include_params) - return false unless include_params - - include_params.include?('source') - end + class << self + def match?(include_params) + include_params&.include?('source') + end - def self.decorate(hash, route_policies) - hash[:included] ||= {} + def decorate(hash, route_policies) + hash[:included] ||= {} - # Collect all GUIDs by type - app_guids = [] - space_guids = [] - org_guids = [] + app_guids = [] + space_guids = [] + org_guids = [] - route_policies.each do |policy| - next if policy.source_type == 'any' + route_policies.each do |policy| + next if policy.source_type == 'any' - case policy.source_type - when 'app' then app_guids << policy.source_guid - when 'space' then space_guids << policy.source_guid - when 'org' then org_guids << policy.source_guid + case policy.source_type + when 'app' then app_guids << policy.source_guid + when 'space' then space_guids << policy.source_guid + when 'org' then org_guids << policy.source_guid + end end - end - permission_queryer = Permissions.new(SecurityContext.current_user) + permission_queryer = Permissions.new(SecurityContext.current_user) - # Fetch and present resources, filtering by what the current user can read - hash[:included][:apps] = fetch_and_present_apps(app_guids.uniq, permission_queryer) - hash[:included][:spaces] = fetch_and_present_spaces(space_guids.uniq, permission_queryer) - hash[:included][:organizations] = fetch_and_present_organizations(org_guids.uniq, permission_queryer) + hash[:included][:apps] = fetch_and_present_apps(app_guids.uniq, permission_queryer) + hash[:included][:spaces] = fetch_and_present_spaces(space_guids.uniq, permission_queryer) + hash[:included][:organizations] = fetch_and_present_organizations(org_guids.uniq, permission_queryer) - hash - end + hash + end - private_class_method def self.fetch_and_present_apps(guids, permission_queryer) - return [] if guids.empty? + private - apps = AppModel.where(guid: guids) - apps = apps.where(space_guid: permission_queryer.readable_space_guids_query) unless permission_queryer.can_read_globally? - apps.order(:created_at, :guid). - eager(Presenters::V3::AppPresenter.associated_resources).all. - map { |app| Presenters::V3::AppPresenter.new(app).to_hash } - end + def fetch_and_present_apps(guids, permission_queryer) + return [] if guids.empty? - private_class_method def self.fetch_and_present_spaces(guids, permission_queryer) - return [] if guids.empty? + apps = AppModel.where(guid: guids) + apps = apps.where(space_guid: permission_queryer.readable_space_guids_query) unless permission_queryer.can_read_globally? + apps.order(:created_at, :guid). + eager(Presenters::V3::AppPresenter.associated_resources).all. + map { |app| Presenters::V3::AppPresenter.new(app).to_hash } + end - spaces = Space.where(guid: guids) - spaces = spaces.where(guid: permission_queryer.readable_space_guids_query) unless permission_queryer.can_read_globally? - spaces.order(:created_at, :guid). - eager(Presenters::V3::SpacePresenter.associated_resources).all. - map { |space| Presenters::V3::SpacePresenter.new(space).to_hash } - end + def fetch_and_present_spaces(guids, permission_queryer) + return [] if guids.empty? - private_class_method def self.fetch_and_present_organizations(guids, permission_queryer) - return [] if guids.empty? + spaces = Space.where(guid: guids) + spaces = spaces.where(guid: permission_queryer.readable_space_guids_query) unless permission_queryer.can_read_globally? + spaces.order(:created_at, :guid). + eager(Presenters::V3::SpacePresenter.associated_resources).all. + map { |space| Presenters::V3::SpacePresenter.new(space).to_hash } + end + + def fetch_and_present_organizations(guids, permission_queryer) + return [] if guids.empty? - orgs = Organization.where(guid: guids) - orgs = orgs.where(guid: permission_queryer.readable_org_guids_query) unless permission_queryer.can_read_globally? - orgs.order(:created_at, :guid). - eager(Presenters::V3::OrganizationPresenter.associated_resources).all. - map { |org| Presenters::V3::OrganizationPresenter.new(org).to_hash } + orgs = Organization.where(guid: guids) + orgs = orgs.where(guid: permission_queryer.readable_org_guids_query) unless permission_queryer.can_read_globally? + orgs.order(:created_at, :guid). + eager(Presenters::V3::OrganizationPresenter.associated_resources).all. + map { |org| Presenters::V3::OrganizationPresenter.new(org).to_hash } + end end end end diff --git a/app/messages/domain_create_message.rb b/app/messages/domain_create_message.rb index fad00cd32a3..bee7a352b89 100644 --- a/app/messages/domain_create_message.rb +++ b/app/messages/domain_create_message.rb @@ -127,7 +127,7 @@ def route_policies_scope_validation end return unless requested?(:enforce_route_policies) && enforce_route_policies == true - return unless !requested?(:route_policies_scope) || route_policies_scope.nil? + return if route_policies_scope.present? errors.add(:route_policies_scope, 'is required when enforce_route_policies is true') end diff --git a/app/messages/route_policies_list_message.rb b/app/messages/route_policies_list_message.rb index 40ba3ffc6f2..6e380806aaf 100644 --- a/app/messages/route_policies_list_message.rb +++ b/app/messages/route_policies_list_message.rb @@ -14,6 +14,7 @@ class RoutePoliciesListMessage < MetadataListMessage validates_with NoAdditionalParamsValidator validates_with IncludeParamValidator, valid_values: %w[source route] + validates :route_guids, array: true, allow_nil: true validates :space_guids, array: true, allow_nil: true validates :source_guids, array: true, allow_nil: true validates :sources, array: true, allow_nil: true diff --git a/app/models/runtime/route_policy.rb b/app/models/runtime/route_policy.rb index 21f22803e2e..87f63a70ebb 100644 --- a/app/models/runtime/route_policy.rb +++ b/app/models/runtime/route_policy.rb @@ -33,27 +33,29 @@ def source=(val) def validate validates_presence :source_type validates_presence :route_id + validate_cf_any_exclusivity end - def after_create - super - notify_processes_of_route_update - end + def notify_diego + return unless route - def after_destroy - super - notify_processes_of_route_update + route.apps.each do |process| + ProcessRouteHandler.new(process).notify_backend_of_route_update + end end private - def notify_processes_of_route_update - return unless route + def validate_cf_any_exclusivity + return unless route_id + + siblings = RoutePolicy.where(route_id: route_id).exclude(id: id) + return if siblings.empty? - db.after_commit do - route.apps.each do |process| - ProcessRouteHandler.new(process).notify_backend_of_route_update - end + if source_type == 'any' + errors.add(:source, "'cf:any' cannot coexist with other route policies on the same route") + elsif siblings.where(source_type: 'any').any? + errors.add(:source, "cannot coexist with the existing 'cf:any' policy on this route") end end end diff --git a/app/presenters/v3/domain_presenter.rb b/app/presenters/v3/domain_presenter.rb index 4b4900a6660..596113e0eb4 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_route_policies: true, # positioned here for field ordering; both keys are deleted below unless enforcement is on + route_policies_scope: domain.route_policies_scope, relationships: { organization: { data: owning_org_guid @@ -43,9 +45,9 @@ def to_hash links: build_links } - if domain.enforce_route_policies - hash[:enforce_route_policies] = true - hash[:route_policies_scope] = domain.route_policies_scope + unless domain.enforce_route_policies + hash.delete(:enforce_route_policies) + hash.delete(:route_policies_scope) end hash diff --git a/app/presenters/v3/route_policy_presenter.rb b/app/presenters/v3/route_policy_presenter.rb index 0a59a89d141..ddbe02283d8 100755 --- a/app/presenters/v3/route_policy_presenter.rb +++ b/app/presenters/v3/route_policy_presenter.rb @@ -7,6 +7,12 @@ module V3 class RoutePolicyPresenter < BasePresenter include VCAP::CloudController::Presenters::Mixins::MetadataPresentationHelpers + class << self + def associated_resources + super + [:route] + end + end + def to_hash hash = { guid: route_policy.guid, @@ -30,29 +36,16 @@ def route_policy end def build_relationships - relationships = { - route: { - data: { - guid: route_policy.route.guid - } - } + { + route: { data: { guid: route_policy.route.guid } }, + app: { data: route_policy.source_type == 'app' ? { guid: route_policy.source_guid } : nil }, + space: { data: route_policy.source_type == 'space' ? { guid: route_policy.source_guid } : nil }, + organization: { data: route_policy.source_type == 'org' ? { guid: route_policy.source_guid } : nil } } - - if route_policy.source_type == 'any' - relationships[:app] = { data: nil } - relationships[:space] = { data: nil } - relationships[:organization] = { data: nil } - else - relationships[:app] = { data: route_policy.source_type == 'app' ? { guid: route_policy.source_guid } : nil } - relationships[:space] = { data: route_policy.source_type == 'space' ? { guid: route_policy.source_guid } : nil } - relationships[:organization] = { data: route_policy.source_type == 'org' ? { guid: route_policy.source_guid } : nil } - end - - relationships end def build_links - { + links = { self: { href: url_builder.build_url(path: "/v3/route_policies/#{route_policy.guid}") }, @@ -60,6 +53,17 @@ def build_links href: url_builder.build_url(path: "/v3/routes/#{route_policy.route.guid}") } } + + case route_policy.source_type + when 'app' + links[:app] = { href: url_builder.build_url(path: "/v3/apps/#{route_policy.source_guid}") } + when 'space' + links[:space] = { href: url_builder.build_url(path: "/v3/spaces/#{route_policy.source_guid}") } + when 'org' + links[:organization] = { href: url_builder.build_url(path: "/v3/organizations/#{route_policy.source_guid}") } + end + + links end end end diff --git a/app/presenters/v3/route_presenter.rb b/app/presenters/v3/route_presenter.rb index f9bac40fdba..802d1b4c95b 100644 --- a/app/presenters/v3/route_presenter.rb +++ b/app/presenters/v3/route_presenter.rb @@ -34,6 +34,7 @@ def to_hash port: route.port, url: build_url, destinations: RouteDestinationsPresenter.new(route.route_mappings, route:).presented_destinations, + options: route.options, metadata: { labels: hashified_labels(route.labels), annotations: hashified_annotations(route.annotations) @@ -48,16 +49,11 @@ def to_hash }, links: build_links } - unless route.options.nil? - public_options = route.options.reject { |k, _| INTERNAL_ROUTE_OPTIONS.include?(k.to_s) } - hash.merge!(options: public_options) if route.options.empty? || public_options.present? - end + hash.delete(:options) if route.options.nil? @decorators.reduce(hash) { |memo, d| d.decorate(memo, [route]) } end - INTERNAL_ROUTE_OPTIONS = %w[route_policy_scope route_policy_sources].freeze - private def route diff --git a/docs/v3/source/includes/api_resources/_route_policies.erb b/docs/v3/source/includes/api_resources/_route_policies.erb index de53e9b9e63..f6cf96907e6 100644 --- a/docs/v3/source/includes/api_resources/_route_policies.erb +++ b/docs/v3/source/includes/api_resources/_route_policies.erb @@ -32,6 +32,9 @@ }, "route": { "href": "https://api.example.org/v3/routes/89b32bd6-688f-4424-b94f-2e2c86495a5f" + }, + "app": { + "href": "https://api.example.org/v3/apps/d76446a1-f429-4444-8797-be2f78b75b08" } } } @@ -87,6 +90,9 @@ }, "route": { "href": "https://api.example.org/v3/routes/89b32bd6-688f-4424-b94f-2e2c86495a5f" + }, + "app": { + "href": "https://api.example.org/v3/apps/d76446a1-f429-4444-8797-be2f78b75b08" } } }, @@ -123,6 +129,9 @@ }, "route": { "href": "https://api.example.org/v3/routes/89b32bd6-688f-4424-b94f-2e2c86495a5f" + }, + "space": { + "href": "https://api.example.org/v3/spaces/3fa85f64-5717-4562-b3fc-2c963f66afa6" } } } diff --git a/docs/v3/source/includes/resources/domains/_create.md.erb b/docs/v3/source/includes/resources/domains/_create.md.erb index 6598afc801a..8776407152c 100644 --- a/docs/v3/source/includes/resources/domains/_create.md.erb +++ b/docs/v3/source/includes/resources/domains/_create.md.erb @@ -58,8 +58,8 @@ Name | Type | Description | ----------- | -------- | ------------------------------------------------------------------------ | ------- | | **internal** | _boolean_ | Whether the domain is used for internal (container-to-container) traffic, or external (user-to-container) traffic | false | | **router_group.guid** | _uuid_ | The desired router group guid.
_note: creates a `tcp` domain; cannot be used when `internal` is set to `true` or domain is scoped to an org_ | null | -| **enforce_route_policies** | _boolean_ | When `true`, GoRouter enforces route policies for routes on this domain using mutual TLS (mTLS). **Immutable** after creation. Cannot be used with internal domains | false | -| **route_policies_scope** | _string_ | Operator-defined boundary for allowed callers: `any`, `org`, or `space`. Required when `enforce_route_policies` is `true`. **Immutable** after creation | | +| **enforce_route_policies** | _boolean_ | When `true`, GoRouter enforces route policies for routes on this domain using mutual TLS (mTLS). Set at creation only; cannot be changed on update. Cannot be used with internal domains | false | +| **route_policies_scope** | _string_ | Operator-defined boundary for allowed callers: `any`, `org`, or `space`. Required when `enforce_route_policies` is `true`. Set at creation only; cannot be changed on update | | | **organization** | [_to-one relationship_](#to-one-relationships) | A relationship to the organization the domain will be scoped to;
_note: cannot be used when `internal` is set to `true` or domain is associated with a router group_ | | | **shared_organizations** | [_to-many relationship_](#to-many-relationships) | A relationship to organizations the domain will be shared with
_Note: cannot be used without an organization relationship_ | | | **metadata.labels** | [_label object_](#labels) | Labels applied to the domain | | diff --git a/docs/v3/source/includes/resources/domains/_object.md.erb b/docs/v3/source/includes/resources/domains/_object.md.erb index e4cc5c5fad2..1e5ee878f5f 100644 --- a/docs/v3/source/includes/resources/domains/_object.md.erb +++ b/docs/v3/source/includes/resources/domains/_object.md.erb @@ -17,8 +17,8 @@ Example Domain object | **internal** | _boolean_ | Whether the domain is used for internal (container-to-container) traffic | **router_group.guid** | _uuid_ | The guid of the desired router group to route `tcp` traffic through; if set, the domain will only be available for `tcp` traffic | **supported_protocols** | _list of strings_ | Available protocols for routes using the domain, currently `http` and `tcp` -| **enforce_route_policies** | _boolean_ | When `true`, GoRouter enforces route policies for routes on this domain. This field only appears in the response when set to `true`. **Immutable** after domain creation -| **route_policies_scope** | _string_ | Operator-defined boundary for allowed callers: `any`, `org`, or `space`. Required when `enforce_route_policies` is `true`. This field only appears when `enforce_route_policies` is `true`. **Immutable** after domain creation +| **enforce_route_policies** | _boolean_ | When `true`, GoRouter enforces route policies for routes on this domain. This field only appears in the response when set to `true`. Set at creation only; cannot be changed on update +| **route_policies_scope** | _string_ | Operator-defined boundary for allowed callers: `any`, `org`, or `space`. Required when `enforce_route_policies` is `true`. This field only appears when `enforce_route_policies` is `true`. Set at creation only; cannot be changed on update | **relationships.organization** | [_to-one relationship_](#to-one-relationships) | The organization the domain is scoped to; if set, the domain will only be available in that organization; otherwise, the domain will be globally available | **relationships.shared_organizations** | [_to-many relationship_](#to-many-relationships) | Organizations the domain is shared with; if set, the domain will be available in these organizations in addition to the organization the domain is scoped to | **metadata.labels** | [_label object_](#labels) | Labels applied to the domain diff --git a/docs/v3/source/includes/resources/route_policies/_create.md.erb b/docs/v3/source/includes/resources/route_policies/_create.md.erb index 9a8e5465f66..ff02af2aaa4 100644 --- a/docs/v3/source/includes/resources/route_policies/_create.md.erb +++ b/docs/v3/source/includes/resources/route_policies/_create.md.erb @@ -105,7 +105,8 @@ curl "https://api.example.org/v3/route_policies" \ - The `source` must be unique per route (duplicate sources are rejected) - If the route already has a `cf:any` policy, no other sources can be added - If adding `cf:any`, the route must not have any existing policies -- The source GUID is not validated at creation time (allows cross-org sharing) +- The source GUID is not checked for existence at creation time; stale references are tolerated, and sources referencing resources the caller cannot see are accepted but do not appear under `?include=source` +- The `app`, `space`, and `organization` relationships are derived from `source` and cannot be set directly #### Permitted roles diff --git a/docs/v3/source/includes/resources/route_policies/_list.md.erb b/docs/v3/source/includes/resources/route_policies/_list.md.erb index e1c837b10c1..56bab951674 100644 --- a/docs/v3/source/includes/resources/route_policies/_list.md.erb +++ b/docs/v3/source/includes/resources/route_policies/_list.md.erb @@ -57,7 +57,7 @@ curl "https://api.example.org/v3/route_policies?include=route,source" \ | **per_page** | _integer_ | Number of results per page; valid values are 1 through 5000 | **order_by** | _string_ | Value to sort by. Defaults to ascending; prepend with `-` to sort descending. Valid values are `created_at`, `updated_at` | **label_selector** | _string_ | A query string containing a list of [label selector](#labels-and-selectors) requirements -| **include** | _string_ | Optionally include related resources in the response; valid values are `route` and `source` (source includes the app, space, or organization based on the source type) +| **include** | _list of strings_ | Optionally include related resources in the response; valid values are `route` and `source` (source includes the app, space, or organization based on the source type) | **created_ats** | _[timestamp](#timestamps)_ | Timestamp to filter by. When filtering on equality, several comma-delimited timestamps may be passed. Also supports filtering with [relational operators](#relational-operators) | **updated_ats** | _[timestamp](#timestamps)_ | Timestamp to filter by. When filtering on equality, several comma-delimited timestamps may be passed. Also supports filtering with [relational operators](#relational-operators) @@ -129,7 +129,7 @@ curl "https://api.example.org/v3/route_policies?space_guids=my-space-guid" \ **Find stale policies referencing a deleted app:** ```shell -curl "https://api.example.org/v3/route_policies?source_guids=deleted-app-guid&include=source" \ +curl "https://api.example.org/v3/route_policies?source_guids=d76446a1-f429-4444-8797-be2f78b75b08&include=source" \ -X GET \ -H "Authorization: bearer [token]" ``` diff --git a/docs/v3/source/includes/resources/route_policies/_object.md.erb b/docs/v3/source/includes/resources/route_policies/_object.md.erb index 9b58b0fd6c2..169eee778a1 100644 --- a/docs/v3/source/includes/resources/route_policies/_object.md.erb +++ b/docs/v3/source/includes/resources/route_policies/_object.md.erb @@ -14,10 +14,10 @@ Example Route Policy object | **created_at** | _[timestamp](#timestamps)_ | The time with zone when the object was created | **updated_at** | _[timestamp](#timestamps)_ | The time with zone when the object was last updated | **source** | _string_ | The policy selector specifying who can access the route. Must be one of:
- `cf:app:` (specific app)
- `cf:space:` (all apps in a space)
- `cf:org:` (all apps in an organization)
- `cf:any` (any caller - cannot be combined with other sources) -| **relationships.route** | [_to-one relationship_](#to-one-relationships) | The route this policy applies to -| **relationships.app** | [_to-one relationship_](#to-one-relationships) | Read-only. The app referenced in the source (populated only when source is `cf:app:`) -| **relationships.space** | [_to-one relationship_](#to-one-relationships) | Read-only. The space referenced in the source (populated only when source is `cf:space:`) -| **relationships.organization** | [_to-one relationship_](#to-one-relationships) | Read-only. The organization referenced in the source (populated only when source is `cf:org:`) | **metadata.labels** | [_label object_](#labels) | Labels applied to the route policy | **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the route policy -| **links** | [_links object_](#links) | Links to related resources +| **relationships.route** | [_to-one relationship_](#to-one-relationships) | The route this policy applies to +| **relationships.app** | [_to-one relationship_](#to-one-relationships) | Read-only. Always present; `data` is `null` unless the source is `cf:app:` +| **relationships.space** | [_to-one relationship_](#to-one-relationships) | Read-only. Always present; `data` is `null` unless the source is `cf:space:` +| **relationships.organization** | [_to-one relationship_](#to-one-relationships) | Read-only. Always present; `data` is `null` unless the source is `cf:org:` +| **links** | [_links object_](#links) | Links to related resources. Always includes `self` and `route`; includes `app`, `space`, or `organization` when the source references that resource diff --git a/spec/request/domains_spec.rb b/spec/request/domains_spec.rb index 0107b6a3c2c..6bcf286dfc5 100644 --- a/spec/request/domains_spec.rb +++ b/spec/request/domains_spec.rb @@ -2326,5 +2326,24 @@ it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS end + + context 'when the request includes a create-only route-policy field' do + let(:domain) { create(:shared_domain) } + let(:user_header) { admin_headers_for(user) } + + it 'rejects updates to enforce_route_policies' do + patch "/v3/domains/#{domain.guid}", { enforce_route_policies: false }.to_json, user_header + + expect(last_response.status).to eq(422) + expect(parsed_response['errors'][0]['detail']).to include("Unknown field(s): 'enforce_route_policies'") + end + + it 'rejects updates to route_policies_scope' do + patch "/v3/domains/#{domain.guid}", { route_policies_scope: 'org' }.to_json, user_header + + expect(last_response.status).to eq(422) + expect(parsed_response['errors'][0]['detail']).to include("Unknown field(s): 'route_policies_scope'") + end + end end end diff --git a/spec/request/route_policies_spec.rb b/spec/request/route_policies_spec.rb index 9fecad66a03..c909997fdbe 100755 --- a/spec/request/route_policies_spec.rb +++ b/spec/request/route_policies_spec.rb @@ -28,18 +28,26 @@ let(:valid_uuid) { '11111111-2222-3333-4444-555555555555' } - def expected_rule_json(rule) + let(:route_policy_json) do { - guid: rule.guid, + guid: UUID_REGEX, created_at: iso8601, updated_at: iso8601, - source: rule.source, + source: "cf:app:#{valid_uuid}", + metadata: { + labels: {}, + annotations: {} + }, relationships: { - route: { data: { guid: rule.route.guid } } + route: { data: { guid: mtls_route.guid } }, + app: { data: { guid: valid_uuid } }, + space: { data: nil }, + organization: { data: nil } }, links: { - self: { href: %r{/v3/route_policies/#{rule.guid}} }, - route: { href: %r{/v3/routes/#{rule.route.guid}} } + self: { href: %r{#{Regexp.escape(link_prefix)}/v3/route_policies/#{UUID_REGEX}} }, + route: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{mtls_route.guid}} }, + app: { href: %r{#{Regexp.escape(link_prefix)}/v3/apps/#{valid_uuid}} } } } end @@ -55,7 +63,7 @@ def expected_rule_json(rule) end context 'as admin' do - it 'creates an access rule and returns 201' do + it 'creates a route policy and returns 201' do post '/v3/route_policies', request_body.to_json, admin_header expect(last_response.status).to eq(201) @@ -76,13 +84,41 @@ def expected_rule_json(rule) end end - context 'as space developer' do - let(:user_headers) { set_user_with_header_as_role(role: 'space_developer', org: org, space: space, user: user) } + context 'permissions' do + let(:api_call) { ->(user_headers) { post '/v3/route_policies', request_body.to_json, user_headers } } + let(:expected_codes_and_responses) do + h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) + %w[no_role org_billing_manager org_auditor].each { |r| h[r] = { code: 404 } } + h['admin'] = { code: 201, response_object: route_policy_json } + h['space_developer'] = { code: 201, response_object: route_policy_json } + h + end - it 'creates an access rule' do - post '/v3/route_policies', request_body.to_json, user_headers + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + end - expect(last_response.status).to eq(201) + context 'when the organization is suspended' do + let(:api_call) { ->(user_headers) { post '/v3/route_policies', request_body.to_json, user_headers } } + let(:expected_codes_and_responses) do + h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) + %w[no_role org_billing_manager org_auditor].each { |r| h[r] = { code: 404 } } + h['admin'] = { code: 201, response_object: route_policy_json } + h['space_developer'] = { code: 403, errors: CF_ORG_SUSPENDED } + h + end + + before do + org.update(status: VCAP::CloudController::Organization::SUSPENDED) + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + end + + context 'when the user is not logged in' do + it 'returns 401 for Unauthenticated requests' do + post '/v3/route_policies', request_body.to_json, base_json_headers + + expect(last_response).to have_status_code(401) end end @@ -142,14 +178,10 @@ def expected_rule_json(rule) context 'cf:any exclusivity' do before do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{valid_uuid}", - route_id: mtls_route.id - ) + create(:route_policy, source: "cf:app:#{valid_uuid}", route_id: mtls_route.id) end - it 'rejects cf:any when other rules exist' do + it 'rejects cf:any when other policies exist' do post '/v3/route_policies', { source: 'cf:any', relationships: { route: { data: { guid: mtls_route.guid } } } @@ -160,13 +192,9 @@ def expected_rule_json(rule) end end - context 'when a cf:any rule already exists' do + context 'when a cf:any policy already exists' do before do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: 'cf:any', - route_id: mtls_route.id - ) + create(:route_policy, source: 'cf:any', route_id: mtls_route.id) end it 'rejects adding a specific selector' do @@ -182,11 +210,7 @@ def expected_rule_json(rule) context 'duplicate selector per route' do before do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{valid_uuid}", - route_id: mtls_route.id - ) + create(:route_policy, source: "cf:app:#{valid_uuid}", route_id: mtls_route.id) end it 'returns 422' do @@ -232,14 +256,10 @@ def expected_rule_json(rule) describe 'GET /v3/route_policies/:guid' do let!(:route_policy) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{valid_uuid}", - route_id: mtls_route.id - ) + create(:route_policy, source: "cf:app:#{valid_uuid}", route_id: mtls_route.id) end - it 'returns the access rule' do + it 'returns the route policy' do get "/v3/route_policies/#{route_policy.guid}", nil, admin_header expect(last_response.status).to eq(200) @@ -248,6 +268,18 @@ def expected_rule_json(rule) expect(parsed['source']).to eq("cf:app:#{valid_uuid}") end + context 'eager loading' do + let(:get_route_policy) { -> { get "/v3/route_policies/#{route_policy.guid}", nil, admin_header } } + + it 'eager loads the route and space in the initial fetch' do + expect { get_route_policy.call }.to have_queried_db_times(/SELECT .* FROM .route_policies. /i, 1) + expect(last_response.status).to eq(200) + + expect { get_route_policy.call }.to have_queried_db_times(/SELECT .* FROM .routes. /i, 0) # joined via eager_graph, not a separate query + expect { get_route_policy.call }.to have_queried_db_times(/SELECT .* FROM .spaces. /i, 0) # joined via eager_graph, not a separate query + end + end + context 'role-based visibility' do let(:api_call) { ->(headers) { get "/v3/route_policies/#{route_policy.guid}", nil, headers } } let(:expected_codes_and_responses) do @@ -261,7 +293,7 @@ def expected_rule_json(rule) it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS end - context 'when the access rule does not exist' do + context 'when the route policy does not exist' do it 'returns 404' do get '/v3/route_policies/nonexistent-guid', nil, admin_header @@ -272,11 +304,7 @@ def expected_rule_json(rule) context 'with include=source' do let!(:frontend_app) { create(:app_model, space: space, name: 'frontend-app') } let!(:app_policy) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{frontend_app.guid}", - route_id: mtls_route.id - ) + create(:route_policy, source: "cf:app:#{frontend_app.guid}", route_id: mtls_route.id) end it 'includes the resolved source app resource' do @@ -310,11 +338,7 @@ def expected_rule_json(rule) context 'with include=route,source' do let!(:frontend_app) { create(:app_model, space: space, name: 'frontend-app') } let!(:app_policy) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{frontend_app.guid}", - route_id: mtls_route.id - ) + create(:route_policy, source: "cf:app:#{frontend_app.guid}", route_id: mtls_route.id) end it 'includes both route and source resources' do @@ -340,28 +364,20 @@ def expected_rule_json(rule) end describe 'GET /v3/route_policies' do - let!(:rule1) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{valid_uuid}", - route_id: mtls_route.id - ) + let!(:policy1) do + create(:route_policy, source: "cf:app:#{valid_uuid}", route_id: mtls_route.id) end - let!(:rule2) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: 'cf:any', - route_id: create(:route, space: space, domain: mtls_domain).id - ) + let!(:policy2) do + create(:route_policy, source: 'cf:any', route_id: create(:route, space: space, domain: mtls_domain).id) end - it 'lists all accessible access rules' do + it 'lists all accessible route policies' do get '/v3/route_policies', nil, admin_header expect(last_response.status).to eq(200) parsed = Oj.load(last_response.body) guids = parsed['resources'].pluck('guid') - expect(guids).to include(rule1.guid, rule2.guid) + expect(guids).to include(policy1.guid, policy2.guid) end context 'role-based visibility' do @@ -370,7 +386,7 @@ def expected_rule_json(rule) h = Hash.new({ code: 200, response_guids: [] }.freeze) %w[admin admin_read_only global_auditor space_developer space_manager space_auditor space_supporter - org_manager org_auditor].each { |r| h[r] = { code: 200, response_guids: [rule1.guid, rule2.guid] } } + org_manager org_auditor].each { |r| h[r] = { code: 200, response_guids: [policy1.guid, policy2.guid] } } h end @@ -383,8 +399,8 @@ def expected_rule_json(rule) expect(last_response.status).to eq(200) parsed = Oj.load(last_response.body) guids = parsed['resources'].pluck('guid') - expect(guids).to include(rule1.guid) - expect(guids).not_to include(rule2.guid) + expect(guids).to include(policy1.guid) + expect(guids).not_to include(policy2.guid) end it 'filters by selectors' do @@ -396,7 +412,7 @@ def expected_rule_json(rule) expect(parsed['resources'][0]['source']).to eq('cf:any') end - describe 'filtering by space_guids' do + describe 'with a policy in another space' do let(:other_org) { create(:organization) } let(:other_space) { create(:space, organization: other_org) } let(:other_mtls_domain) do @@ -406,12 +422,8 @@ def expected_rule_json(rule) route_policies_scope: 'space') end let(:other_route) { create(:route, space: other_space, domain: other_mtls_domain) } - let!(:rule_in_other_space) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: 'cf:any', - route_id: other_route.id - ) + let!(:policy_in_other_space) do + create(:route_policy, source: 'cf:any', route_id: other_route.id) end before do @@ -419,96 +431,70 @@ def expected_rule_json(rule) other_space.add_developer(user) end - it 'filters by single space_guid' do - get "/v3/route_policies?space_guids=#{space.guid}", nil, admin_header + describe 'filtering by space_guids' do + it 'filters by single space_guid' do + get "/v3/route_policies?space_guids=#{space.guid}", nil, admin_header - expect(last_response.status).to eq(200) - parsed = Oj.load(last_response.body) - guids = parsed['resources'].pluck('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/route_policies?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'].pluck('guid') - expect(guids).to include(rule1.guid, rule2.guid, rule_in_other_space.guid) - end + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + guids = parsed['resources'].pluck('guid') + expect(guids).to include(policy1.guid, policy2.guid) + expect(guids).not_to include(policy_in_other_space.guid) + end - it 'combines space_guids with other filters' do - get "/v3/route_policies?space_guids=#{space.guid}&sources=cf:app:#{valid_uuid}", nil, admin_header + it 'filters by multiple space_guids' do + get "/v3/route_policies?space_guids=#{space.guid},#{other_space.guid}", 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]['source']).to eq("cf:app:#{valid_uuid}") - end + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + guids = parsed['resources'].pluck('guid') + expect(guids).to include(policy1.guid, policy2.guid, policy_in_other_space.guid) + end - it 'returns empty when space has no access rules' do - empty_space = create(:space, organization: org) - org.add_user(user) - empty_space.add_developer(user) + it 'combines space_guids with other filters' do + get "/v3/route_policies?space_guids=#{space.guid}&sources=cf:app:#{valid_uuid}", nil, admin_header - get "/v3/route_policies?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(1) + expect(parsed['resources'][0]['guid']).to eq(policy1.guid) + expect(parsed['resources'][0]['source']).to eq("cf:app:#{valid_uuid}") + end - expect(last_response.status).to eq(200) - parsed = Oj.load(last_response.body) - expect(parsed['resources'].length).to eq(0) - end - end + it 'returns empty when space has no route policies' do + empty_space = create(:space, organization: org) + org.add_user(user) + empty_space.add_developer(user) - describe 'filtering by both route_guids and space_guids' do - let(:other_org) { create(:organization) } - let(:other_space) { create(:space, organization: other_org) } - let(:other_mtls_domain) do - create(:private_domain, - owning_organization: other_org, - enforce_route_policies: true, - route_policies_scope: 'space') - end - let(:other_route) { create(:route, space: other_space, domain: other_mtls_domain) } - let!(:rule_in_other_space) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: 'cf:any', - route_id: other_route.id - ) - end + get "/v3/route_policies?space_guids=#{empty_space.guid}", nil, admin_header - before do - other_org.add_user(user) - other_space.add_developer(user) + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + expect(parsed['resources'].length).to eq(0) + end end - it 'returns results matching both route_guids and space_guids without ambiguous column errors' do - get "/v3/route_policies?route_guids=#{mtls_route.guid}&space_guids=#{space.guid}", nil, admin_header + describe 'filtering by both route_guids and space_guids' do + it 'returns results matching both route_guids and space_guids without ambiguous column errors' do + get "/v3/route_policies?route_guids=#{mtls_route.guid}&space_guids=#{space.guid}", nil, admin_header - expect(last_response.status).to eq(200) - parsed = Oj.load(last_response.body) - guids = parsed['resources'].pluck('guid') - expect(guids).to include(rule1.guid) - expect(guids).not_to include(rule_in_other_space.guid) + expect(last_response.status).to eq(200) + parsed = Oj.load(last_response.body) + guids = parsed['resources'].pluck('guid') + expect(guids).to include(policy1.guid) + expect(guids).not_to include(policy_in_other_space.guid) + end end end describe 'filtering by source_guids' do let(:app1) { create(:app_model, space: space) } let(:app2) { create(:app_model, space: space) } - let!(:rule_app1) do - VCAP::CloudController::RoutePolicy.create( - source: "cf:app:#{app1.guid}", - route_id: mtls_route.id - ) + let!(:app1_policy) do + create(:route_policy, source: "cf:app:#{app1.guid}", route_id: mtls_route.id) end - let!(:rule_app2) do - VCAP::CloudController::RoutePolicy.create( - source: "cf:app:#{app2.guid}", - route_id: mtls_route.id - ) + let!(:app2_policy) do + create(:route_policy, source: "cf:app:#{app2.guid}", route_id: mtls_route.id) end it 'returns only the policy whose source_guid matches the queried GUID' do @@ -517,8 +503,8 @@ def expected_rule_json(rule) expect(last_response.status).to eq(200) parsed = Oj.load(last_response.body) returned_guids = parsed['resources'].pluck('guid') - expect(returned_guids).to include(rule_app1.guid) - expect(returned_guids).not_to include(rule_app2.guid) + expect(returned_guids).to include(app1_policy.guid) + expect(returned_guids).not_to include(app2_policy.guid) end it 'returns an empty list when the GUID matches nothing' do @@ -531,7 +517,7 @@ def expected_rule_json(rule) it 'does not return cf:any policies for a GUID query' do any_route = create(:route, space: space, domain: mtls_domain) - VCAP::CloudController::RoutePolicy.create(source: 'cf:any', route_id: any_route.id) + create(:route_policy, source: 'cf:any', route_id: any_route.id) get "/v3/route_policies?source_guids=#{app1.guid}", nil, admin_header @@ -544,18 +530,12 @@ def expected_rule_json(rule) describe 'filtering by sources' do let(:app1) { create(:app_model, space: space) } - let(:another_route) { create(:route, space: space, domain: mtls_domain) } - let!(:rule_typed) do - VCAP::CloudController::RoutePolicy.create( - source: "cf:app:#{app1.guid}", - route_id: mtls_route.id - ) + let(:route2) { create(:route, space: space, domain: mtls_domain) } + let!(:typed_policy) do + create(:route_policy, source: "cf:app:#{app1.guid}", route_id: mtls_route.id) end - let!(:rule_any) do - VCAP::CloudController::RoutePolicy.create( - source: 'cf:any', - route_id: another_route.id - ) + let!(:any_policy) do + create(:route_policy, source: 'cf:any', route_id: route2.id) end it 'returns only the policy with the exact source value' do @@ -564,8 +544,8 @@ def expected_rule_json(rule) expect(last_response.status).to eq(200) parsed = Oj.load(last_response.body) guids = parsed['resources'].pluck('guid') - expect(guids).to include(rule_typed.guid) - expect(guids).not_to include(rule_any.guid) + expect(guids).to include(typed_policy.guid) + expect(guids).not_to include(any_policy.guid) end it 'returns cf:any policies when sources=cf:any' do @@ -574,20 +554,21 @@ def expected_rule_json(rule) expect(last_response.status).to eq(200) parsed = Oj.load(last_response.body) guids = parsed['resources'].pluck('guid') - expect(guids).to include(rule_any.guid) - expect(guids).not_to include(rule_typed.guid) + expect(guids).to include(any_policy.guid) + expect(guids).not_to include(typed_policy.guid) end end describe 'filtering by label_selector' do - let(:another_route) { create(:route, space: space, domain: mtls_domain) } + let(:route1) { create(:route, space: space, domain: mtls_domain) } + let(:route2) { create(:route, space: space, domain: mtls_domain) } let!(:labelled_policy) do - policy = VCAP::CloudController::RoutePolicy.create(source: 'cf:any', route_id: mtls_route.id) - VCAP::CloudController::RoutePolicyLabelModel.create(resource_guid: policy.guid, key_name: 'env', value: 'prod') + policy = create(:route_policy, source: 'cf:any', route_id: route1.id) + create(:route_policy_label_model, resource_guid: policy.guid, key_name: 'env', value: 'prod') policy end let!(:unlabelled_policy) do - VCAP::CloudController::RoutePolicy.create(source: 'cf:any', route_id: another_route.id) + create(:route_policy, source: 'cf:any', route_id: route2.id) end it 'returns only policies matching the label selector' do @@ -606,28 +587,16 @@ def expected_rule_json(rule) let!(:other_space) { create(:space, organization: org, name: 'other-space') } let!(:other_org) { create(:organization, name: 'other-org') } - let!(:app_rule) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{frontend_app.guid}", - route_id: mtls_route.id - ) + let!(:app_policy) do + create(:route_policy, source: "cf:app:#{frontend_app.guid}", route_id: mtls_route.id) end - let!(:space_rule) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:space:#{other_space.guid}", - route_id: mtls_route.id - ) + let!(:space_policy) do + create(:route_policy, source: "cf:space:#{other_space.guid}", route_id: mtls_route.id) end - let!(:org_rule) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:org:#{other_org.guid}", - route_id: mtls_route.id - ) + let!(:org_policy) do + create(:route_policy, source: "cf:org:#{other_org.guid}", route_id: mtls_route.id) end it 'includes resolved selector resources' do @@ -661,11 +630,7 @@ def expected_rule_json(rule) it 'handles stale resources (missing GUIDs) gracefully' do stale_guid = '99999999-9999-9999-9999-999999999999' - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{stale_guid}", - route_id: mtls_route.id - ) + create(:route_policy, source: "cf:app:#{stale_guid}", route_id: mtls_route.id) get '/v3/route_policies?include=source', nil, admin_header @@ -677,13 +642,9 @@ def expected_rule_json(rule) 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::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{frontend_app.guid}", - route_id: create(:route, space: space, domain: mtls_domain).id - ) + it 'includes only unique resources when multiple policies reference the same resource' do + # Create another policy referencing the same app + create(:route_policy, source: "cf:app:#{frontend_app.guid}", route_id: create(:route, space: space, domain: mtls_domain).id) get '/v3/route_policies?include=source', nil, admin_header @@ -696,11 +657,7 @@ def expected_rule_json(rule) end it 'does not include resources for cf:any selectors' do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: 'cf:any', - route_id: create(:route, space: space, domain: mtls_domain).id - ) + create(:route_policy, source: 'cf:any', route_id: create(:route, space: space, domain: mtls_domain).id) get '/v3/route_policies?include=source', nil, admin_header @@ -712,20 +669,12 @@ def expected_rule_json(rule) context 'with include=route' do let(:route2) { create(:route, space: space, domain: mtls_domain) } - let!(:rule_on_route1) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: 'cf:any', - route_id: mtls_route.id - ) + let!(:policy_on_route1) do + create(:route_policy, source: "cf:app:#{SecureRandom.uuid}", route_id: mtls_route.id) end - let!(:rule_on_route2) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{valid_uuid}", - route_id: route2.id - ) + let!(:policy_on_route2) do + create(:route_policy, source: "cf:app:#{valid_uuid}", route_id: route2.id) end it 'includes route resources' do @@ -750,13 +699,9 @@ def expected_rule_json(rule) 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 with a different selector - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{SecureRandom.uuid}", - route_id: mtls_route.id - ) + it 'includes only unique routes when multiple policies reference the same route' do + # Create another policy on the same route with a different selector + create(:route_policy, source: "cf:app:#{SecureRandom.uuid}", route_id: mtls_route.id) get '/v3/route_policies?include=route', nil, admin_header @@ -770,11 +715,7 @@ def expected_rule_json(rule) it 'combines include=route with include=source' do test_app = create(:app_model, space: space, name: 'test-app') - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{test_app.guid}", - route_id: mtls_route.id - ) + create(:route_policy, source: "cf:app:#{test_app.guid}", route_id: mtls_route.id) get '/v3/route_policies?include=route,source', nil, admin_header @@ -798,14 +739,10 @@ def expected_rule_json(rule) describe 'DELETE /v3/route_policies/:guid' do let!(:route_policy) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{valid_uuid}", - route_id: mtls_route.id - ) + create(:route_policy, source: "cf:app:#{valid_uuid}", route_id: mtls_route.id) end - it 'deletes the access rule and returns 204' do + it 'deletes the route policy and returns 204' do delete "/v3/route_policies/#{route_policy.guid}", nil, admin_header expect(last_response.status).to eq(204) @@ -836,36 +773,59 @@ def expected_rule_json(rule) end end - context 'when the access rule does not exist' do + context 'when the route policy does not exist' do it 'returns 404' do delete '/v3/route_policies/nonexistent-guid', nil, admin_header expect(last_response.status).to eq(404) end end + + context 'permissions' do + let(:api_call) { ->(user_headers) { delete "/v3/route_policies/#{route_policy.guid}", nil, user_headers } } + let(:expected_codes_and_responses) do + h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) + %w[no_role org_billing_manager].each { |r| h[r] = { code: 404 } } + h['admin'] = { code: 204 } + h['space_developer'] = { code: 204 } + h + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + end + + context 'when the user is not logged in' do + it 'returns 401 for Unauthenticated requests' do + delete "/v3/route_policies/#{route_policy.guid}", nil, base_json_headers + + expect(last_response).to have_status_code(401) + end + end end describe 'PATCH /v3/route_policies/:guid (metadata update)' do let!(:route_policy) do - VCAP::CloudController::RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{valid_uuid}", - route_id: mtls_route.id + create(:route_policy, source: "cf:app:#{valid_uuid}", route_id: mtls_route.id) + end + let(:update_request_body) do + { metadata: { labels: { env: 'production' } } } + end + let(:updated_route_policy_json) do + route_policy_json.merge( + guid: route_policy.guid, + metadata: { labels: { env: 'production' }, annotations: {} } ) end - it 'returns 200' do - patch "/v3/route_policies/#{route_policy.guid}", { - metadata: { labels: { env: 'production' } } - }.to_json, admin_header + it 'returns 200 and the updated route policy' do + patch "/v3/route_policies/#{route_policy.guid}", update_request_body.to_json, admin_header expect(last_response.status).to eq(200) + expect(parsed_response).to match_json_response(updated_route_policy_json) end it 'records an audit event' do - patch "/v3/route_policies/#{route_policy.guid}", { - metadata: { labels: { env: 'production' } } - }.to_json, admin_header + patch "/v3/route_policies/#{route_policy.guid}", update_request_body.to_json, admin_header expect(last_response.status).to eq(200) event = VCAP::CloudController::Event.last @@ -874,12 +834,33 @@ def expected_rule_json(rule) expect(event.actee_type).to eq('route_policy') end - context 'when the access rule does not exist' do + context 'permissions' do + let(:api_call) { ->(user_headers) { patch "/v3/route_policies/#{route_policy.guid}", update_request_body.to_json, user_headers } } + let(:expected_codes_and_responses) do + h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) + %w[no_role org_billing_manager].each { |r| h[r] = { code: 404 } } + h['admin'] = { code: 200, response_object: updated_route_policy_json } + h['space_developer'] = { code: 200, response_object: updated_route_policy_json } + h + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + end + + context 'when the route policy does not exist' do it 'returns 404' do patch '/v3/route_policies/nonexistent-guid', {}.to_json, admin_header expect(last_response.status).to eq(404) end end + + context 'when the user is not logged in' do + it 'returns 401 for Unauthenticated requests' do + patch "/v3/route_policies/#{route_policy.guid}", update_request_body.to_json, base_json_headers + + expect(last_response).to have_status_code(401) + end + end end end diff --git a/spec/request/routes_spec.rb b/spec/request/routes_spec.rb index cdd7d05dfd8..53f26f3ddc0 100644 --- a/spec/request/routes_spec.rb +++ b/spec/request/routes_spec.rb @@ -389,7 +389,7 @@ end context 'when including route_policies' do - let!(:route_policy) { VCAP::CloudController::RoutePolicy.create(route: route_in_org, source_type: 'app', source_guid: 'some-app-guid') } + let!(:route_policy) { create(:route_policy, route: route_in_org, source_type: 'app', source_guid: 'some-app-guid') } it 'includes the route_policies for the routes' do get '/v3/routes?include=route_policies', nil, admin_header @@ -1134,7 +1134,7 @@ end context 'when including route_policies' do - let!(:route_policy) { VCAP::CloudController::RoutePolicy.create(route: route, source_type: 'app', source_guid: 'some-app-guid') } + let!(:route_policy) { create(:route_policy, route: route, source_type: 'app', source_guid: 'some-app-guid') } it 'includes the route_policies for the route' do get "/v3/routes/#{route.guid}?include=route_policies", nil, admin_header diff --git a/spec/support/factory_definitions/route_policy.rb b/spec/support/factory_definitions/route_policy.rb new file mode 100644 index 00000000000..79897387370 --- /dev/null +++ b/spec/support/factory_definitions/route_policy.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :route_policy, class: 'VCAP::CloudController::RoutePolicy' do + route + source { "cf:app:#{SecureRandom.uuid}" } + end +end diff --git a/spec/support/factory_definitions/route_policy_annotation_model.rb b/spec/support/factory_definitions/route_policy_annotation_model.rb new file mode 100644 index 00000000000..84c5ff672a9 --- /dev/null +++ b/spec/support/factory_definitions/route_policy_annotation_model.rb @@ -0,0 +1,8 @@ +FactoryBot.define do + factory :route_policy_annotation_model, class: 'VCAP::CloudController::RoutePolicyAnnotationModel' do + guid { generate(:guid) } + resource_guid { create(:route_policy).guid } + key_name { 'key' } + value { 'value' } + end +end diff --git a/spec/support/factory_definitions/route_policy_label_model.rb b/spec/support/factory_definitions/route_policy_label_model.rb new file mode 100644 index 00000000000..99bb16db02c --- /dev/null +++ b/spec/support/factory_definitions/route_policy_label_model.rb @@ -0,0 +1,8 @@ +FactoryBot.define do + factory :route_policy_label_model, class: 'VCAP::CloudController::RoutePolicyLabelModel' do + guid { generate(:guid) } + resource_guid { create(:route_policy).guid } + key_name { 'key' } + value { 'value' } + end +end diff --git a/spec/unit/actions/route_policy_create_spec.rb b/spec/unit/actions/route_policy_create_spec.rb index 6d5777490a5..73680d958bc 100644 --- a/spec/unit/actions/route_policy_create_spec.rb +++ b/spec/unit/actions/route_policy_create_spec.rb @@ -3,8 +3,9 @@ module VCAP::CloudController RSpec.describe RoutePolicyCreate do - subject(:action) { RoutePolicyCreate.new } + subject(:action) { RoutePolicyCreate.new(user_audit_info) } + let(:user_audit_info) { UserAuditInfo.new(user_email: 'user@example.com', user_guid: 'some-user-guid') } let(:space) { create(:space) } let(:domain) { create(:shared_domain, name: 'apps.identity', enforce_route_policies: true) } let(:route) { create(:route, space:, domain:) } @@ -33,9 +34,44 @@ module VCAP::CloudController expect(policy.source_guid).to eq('') end + it 'notifies diego once, on the created policy' do + allow_any_instance_of(RoutePolicy).to receive(:notify_diego) + + policy = action.create(route:, message:) + + expect(policy).to have_received(:notify_diego).once + end + + it 'records a route_policy_create audit event with the source and route guid' do + expect_any_instance_of(Repositories::RoutePolicyEventRepository). + to receive(:record_route_policy_create).once. + with(instance_of(RoutePolicy), user_audit_info, { 'source' => "cf:app:#{app_guid}", 'route_guid' => route.guid }) + + action.create(route:, message:) + end + + it 'returns the route policy' do + expect(action.create(route:, message:)).to be_a(RoutePolicy) + end + + context 'when recording the audit event fails inside the transaction' do + before do + allow_any_instance_of(Repositories::RoutePolicyEventRepository). + to receive(:record_route_policy_create).and_raise('boom') + end + + it 'rolls back the policy and does not notify diego' do + expect_any_instance_of(RoutePolicy).not_to receive(:notify_diego) + + expect do + expect { action.create(route:, message:) }.to raise_error('boom') + end.not_to change(RoutePolicy, :count) + end + end + context 'when the same source already exists for the route' do before do - RoutePolicy.create(source: "cf:app:#{app_guid}", route_id: route.id) + create(:route_policy, source: "cf:app:#{app_guid}", route_id: route.id) end it 'raises an error' do @@ -49,26 +85,26 @@ module VCAP::CloudController let(:message) { instance_double(RoutePolicyCreateMessage, source: 'cf:any') } before do - RoutePolicy.create(source: "cf:app:#{app_guid}", route_id: route.id) + create(:route_policy, source: "cf:app:#{app_guid}", route_id: route.id) end it 'raises an error' do expect do action.create(route:, message:) - end.to raise_error(RoutePolicyCreate::Error, /cannot add 'cf:any'/i) + end.to raise_error(RoutePolicyCreate::Error, /'cf:any' cannot coexist with other route policies/) end end context 'when a cf:any policy already exists for the route' do before do - RoutePolicy.create(source: 'cf:any', route_id: route.id) + create(:route_policy, source: 'cf:any', route_id: route.id) end it 'raises an error when adding any other source' do other_message = instance_double(RoutePolicyCreateMessage, source: "cf:app:#{SecureRandom.uuid}") expect do action.create(route: route, message: other_message) - end.to raise_error(RoutePolicyCreate::Error, /already has a 'cf:any' policy/) + end.to raise_error(RoutePolicyCreate::Error, /cannot coexist with the existing 'cf:any' policy/) end end diff --git a/spec/unit/actions/route_policy_destroy_spec.rb b/spec/unit/actions/route_policy_destroy_spec.rb new file mode 100644 index 00000000000..943a1dede3b --- /dev/null +++ b/spec/unit/actions/route_policy_destroy_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' +require 'actions/route_policy_destroy' + +module VCAP::CloudController + RSpec.describe RoutePolicyDestroy do + subject(:action) { RoutePolicyDestroy.new(user_audit_info) } + + let(:user_audit_info) { UserAuditInfo.new(user_email: 'user@example.com', user_guid: 'some-user-guid') } + let(:space) { create(:space) } + let(:domain) { create(:shared_domain, name: 'apps.identity', enforce_route_policies: true) } + let(:route) { create(:route, space:, domain:) } + let(:app_guid) { SecureRandom.uuid } + let!(:route_policy) { create(:route_policy, source: "cf:app:#{app_guid}", route: route) } + + describe '#delete' do + it 'destroys the route policy' do + expect { action.delete(route_policy) }.to change(RoutePolicy, :count).by(-1) + end + + it 'notifies diego after destroying' do + expect(route_policy).to receive(:notify_diego).once + + action.delete(route_policy) + end + + it 'records a route_policy_delete audit event' do + expect_any_instance_of(Repositories::RoutePolicyEventRepository). + to receive(:record_route_policy_delete).once. + with(route_policy, user_audit_info) + + action.delete(route_policy) + end + + context 'when recording the audit event fails inside the transaction' do + before do + allow_any_instance_of(Repositories::RoutePolicyEventRepository). + to receive(:record_route_policy_delete).and_raise('boom') + end + + it 'rolls back the destroy and does not notify diego' do + expect(route_policy).not_to receive(:notify_diego) + + expect do + expect { action.delete(route_policy) }.to raise_error('boom') + end.not_to change(RoutePolicy, :count) + end + end + end + end +end diff --git a/spec/unit/actions/route_policy_update_spec.rb b/spec/unit/actions/route_policy_update_spec.rb new file mode 100644 index 00000000000..38510780dae --- /dev/null +++ b/spec/unit/actions/route_policy_update_spec.rb @@ -0,0 +1,58 @@ +require 'spec_helper' +require 'actions/route_policy_update' + +module VCAP::CloudController + RSpec.describe RoutePolicyUpdate do + subject(:action) { RoutePolicyUpdate.new(user_audit_info) } + + let(:user_audit_info) { UserAuditInfo.new(user_email: 'user@example.com', user_guid: 'some-user-guid') } + let(:space) { create(:space) } + let(:domain) { create(:shared_domain, name: 'apps.identity', enforce_route_policies: true) } + let(:route) { create(:route, space:, domain:) } + let(:app_guid) { SecureRandom.uuid } + let(:route_policy) { create(:route_policy, source: "cf:app:#{app_guid}", route: route) } + let(:message) do + RoutePolicyUpdateMessage.new(metadata: { labels: { 'key' => 'value' }, annotations: { 'a' => 'note' } }) + end + + describe '#update' do + it 'applies the label metadata update' do + action.update(route_policy, message) + + expect(route_policy.reload.labels.map { |l| [l.key_name, l.value] }).to include(%w[key value]) + end + + it 'applies the annotation metadata update' do + action.update(route_policy, message) + + expect(route_policy.reload.annotations.map { |a| [a.key_name, a.value] }).to include(%w[a note]) + end + + it 'records a route_policy_update audit event with the requested metadata' do + expect_any_instance_of(Repositories::RoutePolicyEventRepository). + to receive(:record_route_policy_update).once. + with(route_policy, user_audit_info, { 'metadata' => { 'labels' => { 'key' => 'value' }, 'annotations' => { 'a' => 'note' } } }) + + action.update(route_policy, message) + end + + it 'returns the route policy' do + expect(action.update(route_policy, message)).to eq(route_policy) + end + + context 'when recording the audit event fails inside the transaction' do + before do + allow_any_instance_of(Repositories::RoutePolicyEventRepository). + to receive(:record_route_policy_update).and_raise('boom') + end + + it 'rolls back the metadata update' do + expect { action.update(route_policy, message) }.to raise_error('boom') + + expect(route_policy.reload.labels).to be_empty + expect(route_policy.reload.annotations).to be_empty + end + end + end + end +end diff --git a/spec/unit/decorators/include_route_policies_decorator_spec.rb b/spec/unit/decorators/include_route_policies_decorator_spec.rb index 47468bcdb9c..ca75870bab4 100644 --- a/spec/unit/decorators/include_route_policies_decorator_spec.rb +++ b/spec/unit/decorators/include_route_policies_decorator_spec.rb @@ -11,8 +11,8 @@ module VCAP::CloudController let(:route2) { create(:route, space: space, domain: domain) } it 'decorates the given hash with route_policies from routes' do - route_policy1 = RoutePolicy.create(route: route1, source_type: 'app', source_guid: 'app-guid-1') - route_policy2 = RoutePolicy.create(route: route2, source_type: 'app', source_guid: 'app-guid-2') + route_policy1 = create(:route_policy, route: route1, source_type: 'app', source_guid: 'app-guid-1') + route_policy2 = create(:route_policy, route: route2, source_type: 'app', source_guid: 'app-guid-2') undecorated_hash = { i_am: 'tim' } hash = subject.decorate(undecorated_hash, [route1, route2]) expect(hash[:i_am]).to eq('tim') @@ -23,7 +23,7 @@ module VCAP::CloudController end it 'does not overwrite other included fields' do - route_policy1 = RoutePolicy.create(route: route1, source_type: 'app', source_guid: 'app-guid-1') + route_policy1 = create(:route_policy, route: route1, source_type: 'app', source_guid: 'app-guid-1') undecorated_hash = { foo: 'bar', included: { favorite_fruits: %w[tomato cucumber] } } hash = subject.decorate(undecorated_hash, [route1]) expect(hash[:foo]).to eq('bar') @@ -37,8 +37,8 @@ module VCAP::CloudController end it 'includes multiple policies for the same route' do - policy_a = RoutePolicy.create(route: route1, source_type: 'app', source_guid: 'app-guid-a') - policy_b = RoutePolicy.create(route: route1, source_type: 'app', source_guid: 'app-guid-b') + policy_a = create(:route_policy, route: route1, source_type: 'app', source_guid: 'app-guid-a') + policy_b = create(:route_policy, route: route1, source_type: 'app', source_guid: 'app-guid-b') hash = subject.decorate({}, [route1]) expect(hash[:included][:route_policies]).to contain_exactly( Presenters::V3::RoutePolicyPresenter.new(policy_a).to_hash, diff --git a/spec/unit/decorators/include_route_policy_source_decorator_spec.rb b/spec/unit/decorators/include_route_policy_source_decorator_spec.rb index e14d25bb626..0f34a91f7a8 100644 --- a/spec/unit/decorators/include_route_policy_source_decorator_spec.rb +++ b/spec/unit/decorators/include_route_policy_source_decorator_spec.rb @@ -23,7 +23,7 @@ module VCAP::CloudController end it 'does not match nil' do - expect(decorator.match?(nil)).to be false + expect(decorator.match?(nil)).not_to be(true) end end @@ -31,10 +31,10 @@ module VCAP::CloudController let(:app1) { create(:app_model, space:) } let(:space1) { create(:space) } let(:org1) { space1.organization } - let(:policy_app) { RoutePolicy.create(source: "cf:app:#{app1.guid}", route_id: route.id) } - let(:policy_space) { RoutePolicy.create(source: "cf:space:#{space1.guid}", route_id: route.id) } - let(:policy_org) { RoutePolicy.create(source: "cf:org:#{org1.guid}", route_id: route.id) } - let(:policy_any) { RoutePolicy.create(source: 'cf:any', route_id: route.id) } + let(:policy_app) { create(:route_policy, source: "cf:app:#{app1.guid}", route_id: route.id) } + let(:policy_space) { create(:route_policy, source: "cf:space:#{space1.guid}", route_id: route.id) } + let(:policy_org) { create(:route_policy, source: "cf:org:#{org1.guid}", route_id: route.id) } + let(:policy_any) { create(:route_policy, source: 'cf:any', route_id: route.id) } it 'includes apps, spaces, and orgs from policy sources' do hash = decorator.decorate({}, [policy_app, policy_space, policy_org]) @@ -60,12 +60,12 @@ module VCAP::CloudController let(:other_org) { other_space.organization } let(:other_app) { create(:app_model, space: other_space) } - let(:policy_readable_app) { RoutePolicy.create(source: "cf:app:#{app1.guid}", route_id: route.id) } - let(:policy_unreadable_app) { RoutePolicy.create(source: "cf:app:#{other_app.guid}", route_id: route.id) } - let(:policy_readable_space) { RoutePolicy.create(source: "cf:space:#{space1.guid}", route_id: route.id) } - let(:policy_unreadable_space) { RoutePolicy.create(source: "cf:space:#{other_space.guid}", route_id: route.id) } - let(:policy_readable_org) { RoutePolicy.create(source: "cf:org:#{org1.guid}", route_id: route.id) } - let(:policy_unreadable_org) { RoutePolicy.create(source: "cf:org:#{other_org.guid}", route_id: route.id) } + let(:policy_readable_app) { create(:route_policy, source: "cf:app:#{app1.guid}", route_id: route.id) } + let(:policy_unreadable_app) { create(:route_policy, source: "cf:app:#{other_app.guid}", route_id: route.id) } + let(:policy_readable_space) { create(:route_policy, source: "cf:space:#{space1.guid}", route_id: route.id) } + let(:policy_unreadable_space) { create(:route_policy, source: "cf:space:#{other_space.guid}", route_id: route.id) } + let(:policy_readable_org) { create(:route_policy, source: "cf:org:#{org1.guid}", route_id: route.id) } + let(:policy_unreadable_org) { create(:route_policy, source: "cf:org:#{other_org.guid}", route_id: route.id) } let(:permission_queryer) do instance_double( 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 edd208fdedc..fa921267332 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 @@ -260,26 +260,18 @@ class Protocol route_policies_scope: 'space') end let(:mtls_route) { create(:route, host: 'myapp', domain: enforce_domain, space: space) } - let!(:access_rule1) do - RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:app:#{valid_uuid}", - route_id: mtls_route.id - ) + let!(:app_policy) do + create(:route_policy, source: "cf:app:#{valid_uuid}", route_id: mtls_route.id) end - let!(:access_rule2) do - RoutePolicy.create( - guid: SecureRandom.uuid, - source: "cf:space:#{valid_uuid}", - route_id: mtls_route.id - ) + let!(:space_policy) do + create(:route_policy, source: "cf:space:#{valid_uuid}", route_id: mtls_route.id) end before do create(:route_mapping_model, app: process.app, route: mtls_route, process_type: process.type) end - it 'injects access_scope and access_rules into route options' do + it 'injects route_policy_scope and route_policy_sources into route options' do http_routes = ri['http_routes'] mtls_entry = http_routes.find { |r| r['hostname'] == 'myapp.mtls.example.com' } @@ -289,13 +281,13 @@ class Protocol expect(mtls_entry['options']['route_policy_sources']).to include("cf:space:#{valid_uuid}") end - context 'when the route has no access rules' do + context 'when the route has no route policies' do before do - access_rule1.destroy - access_rule2.destroy + app_policy.destroy + space_policy.destroy end - it 'injects access_scope but omits access_rules key' do + it 'injects route_policy_scope but omits route_policy_sources key' do http_routes = ri['http_routes'] mtls_entry = http_routes.find { |r| r['hostname'] == 'myapp.mtls.example.com' } diff --git a/spec/unit/messages/route_options_message_spec.rb b/spec/unit/messages/route_options_message_spec.rb index ccde930b18b..1be8aea5e24 100644 --- a/spec/unit/messages/route_options_message_spec.rb +++ b/spec/unit/messages/route_options_message_spec.rb @@ -35,6 +35,18 @@ module VCAP::CloudController expect(message).not_to be_valid expect(message.errors_on(:base)).to include("Unknown field(s): 'cookies'") end + + it 'rejects route_policy_scope, which is computed for routing_info and never persisted' do + message = RouteOptionsMessage.new({ route_policy_scope: 'org' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'route_policy_scope'") + end + + it 'rejects route_policy_sources, which is computed for routing_info and never persisted' do + message = RouteOptionsMessage.new({ route_policy_sources: 'cf:any' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'route_policy_sources'") + end end describe 'hash-based routing validations' do diff --git a/spec/unit/models/runtime/route_policy_spec.rb b/spec/unit/models/runtime/route_policy_spec.rb index 2821d775a24..835e05cdd47 100644 --- a/spec/unit/models/runtime/route_policy_spec.rb +++ b/spec/unit/models/runtime/route_policy_spec.rb @@ -93,101 +93,105 @@ module VCAP::CloudController describe 'validations' do it 'requires a selector' do - rule = RoutePolicy.new(route:) - expect(rule.valid?).to be false - expect(rule.errors[:source_type]).to include(:presence) + policy = RoutePolicy.new(route:) + expect(policy.valid?).to be false + expect(policy.errors[:source_type]).to include(:presence) end it 'requires a route_id' do - rule = RoutePolicy.new(source: 'cf:app:123') - expect(rule.valid?).to be false - expect(rule.errors[:route_id]).to include(:presence) + policy = RoutePolicy.new(source: 'cf:app:123') + expect(policy.valid?).to be false + expect(policy.errors[:route_id]).to include(:presence) + end + + describe 'cf:any exclusivity' do + it 'rejects cf:any when another policy already exists on the route' do + create(:route_policy, source: "cf:app:#{app_guid}", route: route) + policy = RoutePolicy.new(source: 'cf:any', route: route) + + expect(policy.valid?).to be false + expect(policy.errors[:source]).to include("'cf:any' cannot coexist with other route policies on the same route") + end + + it 'rejects a non-cf:any policy when a cf:any policy already exists on the route' do + create(:route_policy, source: 'cf:any', route: route) + policy = RoutePolicy.new(source: "cf:app:#{app_guid}", route: route) + + expect(policy.valid?).to be false + expect(policy.errors[:source]).to include("cannot coexist with the existing 'cf:any' policy on this route") + end + + it 'raises Sequel::ValidationFailed when saving a conflicting cf:any policy' do + create(:route_policy, source: "cf:app:#{app_guid}", route: route) + + expect do + create(:route_policy, source: 'cf:any', route: route) + end.to raise_error(Sequel::ValidationFailed) + end + + it 'allows multiple non-cf:any policies on the same route' do + create(:route_policy, source: "cf:app:#{app_guid}", route: route) + + expect do + create(:route_policy, source: "cf:app:#{SecureRandom.uuid}", route: route) + end.not_to raise_error + end + + it 'allows cf:any when the route has no other policies' do + expect do + create(:route_policy, source: 'cf:any', route: route) + end.not_to raise_error + end end end describe 'associations' do it 'belongs to a route' do - rule = RoutePolicy.create( - source: 'cf:app:123', - route: route - ) - expect(rule.route).to eq(route) + policy = create(:route_policy, source: 'cf:app:123', route: route) + expect(policy.route).to eq(route) end end describe 'columns' do it 'persists source_type for a typed source' do - policy = RoutePolicy.create(source: "cf:app:#{app_guid}", route: route) + policy = create(:route_policy, source: "cf:app:#{app_guid}", route: route) expect(policy.source_type).to eq('app') expect(policy.source_guid).to eq(app_guid) end it 'persists source_type and empty source_guid for cf:any' do - policy = RoutePolicy.create(source: 'cf:any', route: route) + policy = create(:route_policy, source: 'cf:any', route: route) expect(policy.source_type).to eq('any') expect(policy.source_guid).to eq('') end end - describe 'callbacks' do - describe 'after_create' do - it 'calls notify_processes_of_route_update' do - expect_any_instance_of(RoutePolicy).to receive(:notify_processes_of_route_update).and_call_original - - RoutePolicy.create( - source: "cf:app:#{app_guid}", - route: route - ) - end + describe '#notify_diego' do + let(:policy) { create(:route_policy, source: "cf:app:#{app_guid}", route: route) } - it 'updates associated processes' do - process # force creation + it 'notifies the backend for each associated process' do + process # force creation - # Record the SQL update queries to verify the process row is updated - RoutePolicy.create( - source: "cf:app:#{app_guid}", - route: route - ) + expect_any_instance_of(ProcessRouteHandler).to receive(:notify_backend_of_route_update).once - # Verify the route has linked processes - expect(route.apps).to include(process) - end - - it 'does not fail if route has no associated processes' do - route_without_processes = create(:route, space:, domain:) - - expect do - RoutePolicy.create( - source: "cf:app:#{app_guid}", - route: route_without_processes - ) - end.not_to raise_error - end + policy.notify_diego end - describe 'after_destroy' do - it 'calls notify_processes_of_route_update' do - rule = RoutePolicy.create( - source: "cf:app:#{app_guid}", - route: route - ) + it 'does nothing when the route has no associated processes' do + route_without_processes = create(:route, space:, domain:) + policy_without_processes = create(:route_policy, source: "cf:app:#{app_guid}", route: route_without_processes) - expect_any_instance_of(RoutePolicy).to receive(:notify_processes_of_route_update).and_call_original - - rule.destroy - end + expect do + policy_without_processes.notify_diego + end.not_to raise_error + end - it 'does not fail if route has no associated processes' do - route_without_processes = create(:route, space:, domain:) - rule = RoutePolicy.create( - source: "cf:app:#{app_guid}", - route: route_without_processes - ) + it 'does nothing when the route is nil' do + policy_without_route = RoutePolicy.new(source: "cf:app:#{app_guid}") - expect do - rule.destroy - end.not_to raise_error - end + expect do + policy_without_route.notify_diego + end.not_to raise_error end end end diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index 5971290ea0b..922b3d722e0 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -1789,6 +1789,31 @@ module VCAP::CloudController end end end + + context 'with route policies' do + let(:space) { create(:space) } + let(:domain) { create(:shared_domain, name: 'apps.identity', enforce_route_policies: true) } + let(:route) { create(:route, space:, domain:) } + let(:process) { ProcessModelFactory.make(space: space, state: 'STARTED', diego: false) } + let(:fake_route_handler) { instance_double(ProcessRouteHandler, notify_backend_of_route_update: nil) } + + before do + create(:route_mapping_model, app: process.app, route: route, process_type: process.type) + route.reload + allow(ProcessRouteHandler).to receive(:new).with(route.apps.first).and_return(fake_route_handler) + create_list(:route_policy, 3, route: route) + end + + it 'deletes the associated route_policies' do + expect { route.destroy }.to change { RoutePolicy.where(route_id: route.id).count }.from(3).to(0) + end + + it 'notifies diego exactly once per associated process, regardless of the policy count' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update).once + + route.destroy + end + end end def assert_valid_path(path) diff --git a/spec/unit/presenters/v3/route_policy_presenter_spec.rb b/spec/unit/presenters/v3/route_policy_presenter_spec.rb index da223cf8a5f..68120bc46d4 100644 --- a/spec/unit/presenters/v3/route_policy_presenter_spec.rb +++ b/spec/unit/presenters/v3/route_policy_presenter_spec.rb @@ -14,7 +14,7 @@ module V3 describe '#to_hash relationships' do context 'when source is cf:app:' do - let(:route_policy) { RoutePolicy.create(source: "cf:app:#{app_model.guid}", route: route) } + let(:route_policy) { create(:route_policy, source: "cf:app:#{app_model.guid}", route: route) } it 'populates relationships.app and nulls space and organization' do expect(result[:relationships][:app]).to eq(data: { guid: app_model.guid }) @@ -24,7 +24,7 @@ module V3 end context 'when source is cf:space:' do - let(:route_policy) { RoutePolicy.create(source: "cf:space:#{space.guid}", route: route) } + let(:route_policy) { create(:route_policy, source: "cf:space:#{space.guid}", route: route) } it 'populates relationships.space and nulls app and organization' do expect(result[:relationships][:app]).to eq(data: nil) @@ -34,7 +34,7 @@ module V3 end context 'when source is cf:org:' do - let(:route_policy) { RoutePolicy.create(source: "cf:org:#{space.organization.guid}", route: route) } + let(:route_policy) { create(:route_policy, source: "cf:org:#{space.organization.guid}", route: route) } it 'populates relationships.organization and nulls app and space' do expect(result[:relationships][:app]).to eq(data: nil) @@ -44,7 +44,7 @@ module V3 end context 'when source is cf:any' do - let(:route_policy) { RoutePolicy.create(source: 'cf:any', route: route) } + let(:route_policy) { create(:route_policy, source: 'cf:any', route: route) } it 'nulls all source relationships' do expect(result[:relationships][:app]).to eq(data: nil) @@ -56,7 +56,7 @@ module V3 describe '#to_hash source field' do context 'when source is cf:app' do - let(:route_policy) { RoutePolicy.create(source: "cf:app:#{app_model.guid}", route: route) } + let(:route_policy) { create(:route_policy, source: "cf:app:#{app_model.guid}", route: route) } it 'emits the composite source string' do expect(result[:source]).to eq("cf:app:#{app_model.guid}") @@ -64,13 +64,53 @@ module V3 end context 'when source is cf:any' do - let(:route_policy) { RoutePolicy.create(source: 'cf:any', route: route) } + let(:route_policy) { create(:route_policy, source: 'cf:any', route: route) } it 'emits cf:any' do expect(result[:source]).to eq('cf:any') end end end + + describe '#to_hash links' do + context 'when source is cf:app:' do + let(:route_policy) { create(:route_policy, source: "cf:app:#{app_model.guid}", route: route) } + + it 'includes an app link and omits space and organization links' do + expect(result[:links][:app][:href]).to end_with("/v3/apps/#{app_model.guid}") + expect(result[:links]).not_to have_key(:space) + expect(result[:links]).not_to have_key(:organization) + end + end + + context 'when source is cf:space:' do + let(:route_policy) { create(:route_policy, source: "cf:space:#{space.guid}", route: route) } + + it 'includes a space link and omits app and organization links' do + expect(result[:links][:space][:href]).to end_with("/v3/spaces/#{space.guid}") + expect(result[:links]).not_to have_key(:app) + expect(result[:links]).not_to have_key(:organization) + end + end + + context 'when source is cf:org:' do + let(:route_policy) { create(:route_policy, source: "cf:org:#{space.organization.guid}", route: route) } + + it 'includes an organization link and omits app and space links' do + expect(result[:links][:organization][:href]).to end_with("/v3/organizations/#{space.organization.guid}") + expect(result[:links]).not_to have_key(:app) + expect(result[:links]).not_to have_key(:space) + end + end + + context 'when source is cf:any' do + let(:route_policy) { create(:route_policy, source: 'cf:any', route: route) } + + it 'includes only self and route links' do + expect(result[:links].keys).to contain_exactly(:self, :route) + end + end + end end end end diff --git a/spec/unit/presenters/v3/route_presenter_spec.rb b/spec/unit/presenters/v3/route_presenter_spec.rb index 30870548628..b1627b1f53e 100644 --- a/spec/unit/presenters/v3/route_presenter_spec.rb +++ b/spec/unit/presenters/v3/route_presenter_spec.rb @@ -133,42 +133,6 @@ module VCAP::CloudController::Presenters::V3 end end - context 'when options contains only internal mTLS keys' do - let(:route) do - create(:route, - host: 'foobar', - path: path, - space: space, - domain: domain, - options: { 'route_policy_scope' => 'space', 'route_policy_sources' => '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 - create(:route, - host: 'foobar', - path: path, - space: space, - domain: domain, - options: { - 'loadbalancing' => 'round-robin', - 'route_policy_scope' => 'space', - 'route_policy_sources' => '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('route_policy_scope') - expect(subject[:options]).not_to have_key('route_policy_sources') - end - end - context 'when there are decorators' do let(:banana_decorator) do Class.new do diff --git a/spec/unit/repositories/route_policy_event_repository_spec.rb b/spec/unit/repositories/route_policy_event_repository_spec.rb index 4a148ee02ee..8a4f11b7937 100644 --- a/spec/unit/repositories/route_policy_event_repository_spec.rb +++ b/spec/unit/repositories/route_policy_event_repository_spec.rb @@ -8,7 +8,7 @@ module Repositories let(:space) { create(:space) } let(:domain) { create(:shared_domain, name: 'apps.identity', enforce_route_policies: true) } let(:route) { create(:route, space:, domain:) } - let(:route_policy) { RoutePolicy.create(source: 'cf:any', route: route) } + let(:route_policy) { create(:route_policy, source: 'cf:any', route: route) } let(:user_email) { 'user@example.com' } let(:user_name) { 'some-user' } let(:actor_audit_info) { UserAuditInfo.new(user_guid: user.guid, user_name: user_name, user_email: user_email) }