From 408d53b5e6f017cc2764a10f8e242a27c99db7c2 Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Tue, 9 Jun 2026 12:28:37 +0200 Subject: [PATCH] Performance fixes for permission queries PR #5157 introduced authorized_space_ids_subquery and authorized_org_ids_subquery. These methods avoid unnecessary subplans on the spaces / organizations tables on every permission lookup and are now also used by: - Permissions#readable_security_group_guids_query - Permissions#readable_space_quota_guids - Permissions#is_org_manager? - Membership#authorized_space_guids_subquery - Membership#authorized_org_guids_subquery - Service-credential-binding fetchers - Service-offering / service-plan list fetchers The service-offering / service-plan list fetchers now take both *_ids_query and *_query; the id-query feeds the always-executed permission filter, the full query is consulted only on guid-filtered requests. Also removes the now-dead 'membership' helper from v3/application_controller.rb (no V3 controller calls it after #5094). --- app/controllers/v3/application_controller.rb | 4 -- .../service_credential_bindings_controller.rb | 8 ++-- .../v3/service_offerings_controller.rb | 2 + .../v3/service_plans_controller.rb | 2 + app/fetchers/base_service_list_fetcher.rb | 43 +++++++++++-------- .../service_credential_binding_fetcher.rb | 4 +- ...service_credential_binding_list_fetcher.rb | 6 +-- app/fetchers/service_offering_list_fetcher.rb | 7 ++- app/fetchers/service_plan_list_fetcher.rb | 7 ++- lib/cloud_controller/membership.rb | 4 +- lib/cloud_controller/permissions.rb | 16 +++++-- ...service_credential_binding_fetcher_spec.rb | 30 ++++++------- ...ce_credential_binding_list_fetcher_spec.rb | 42 +++++++++--------- .../service_offering_list_fetcher_spec.rb | 22 +++++++++- .../service_plan_list_fetcher_spec.rb | 20 +++++++++ .../lib/cloud_controller/permissions_spec.rb | 10 +++++ 16 files changed, 152 insertions(+), 75 deletions(-) diff --git a/app/controllers/v3/application_controller.rb b/app/controllers/v3/application_controller.rb index 30a8f2ca69d..34fc4b91753 100644 --- a/app/controllers/v3/application_controller.rb +++ b/app/controllers/v3/application_controller.rb @@ -239,8 +239,4 @@ def handle_exception(error) def null_coalesce_body hashed_params[:body] ||= {} end - - def membership - @membership ||= Membership.new(current_user) - end end diff --git a/app/controllers/v3/service_credential_bindings_controller.rb b/app/controllers/v3/service_credential_bindings_controller.rb index 8549251f7c0..eb1490073c1 100644 --- a/app/controllers/v3/service_credential_bindings_controller.rb +++ b/app/controllers/v3/service_credential_bindings_controller.rb @@ -22,7 +22,7 @@ def index invalid_param!(message.errors.full_messages) unless message.valid? results = list_fetcher.fetch( - readable_spaces_query: spaces_query, + readable_space_ids_query: space_ids_query, message: message, eager_loaded_associations: Presenters::V3::ServiceCredentialBindingPresenter.associated_resources ) @@ -332,14 +332,14 @@ def fetch_credentials_value(name) end def service_credential_binding - @service_credential_binding ||= fetcher.fetch(hashed_params[:guid], readable_spaces_query: spaces_query) + @service_credential_binding ||= fetcher.fetch(hashed_params[:guid], readable_space_ids_query: space_ids_query) end - def spaces_query + def space_ids_query if permission_queryer.can_read_globally? nil else - permission_queryer.readable_spaces_query + permission_queryer.readable_space_ids_query end end diff --git a/app/controllers/v3/service_offerings_controller.rb b/app/controllers/v3/service_offerings_controller.rb index 270f1a4dcf1..73284ab154b 100644 --- a/app/controllers/v3/service_offerings_controller.rb +++ b/app/controllers/v3/service_offerings_controller.rb @@ -35,7 +35,9 @@ def index ServiceOfferingListFetcher.fetch( message, eager_loaded_associations: Presenters::V3::ServiceOfferingPresenter.associated_resources, + readable_org_ids_query: permission_queryer.readable_org_ids_query, readable_orgs_query: permission_queryer.readable_orgs_query, + readable_space_ids_query: permission_queryer.readable_space_scoped_space_ids_query, readable_spaces_query: permission_queryer.readable_space_scoped_spaces_query ) end diff --git a/app/controllers/v3/service_plans_controller.rb b/app/controllers/v3/service_plans_controller.rb index fe08c2c2eb3..521f1833030 100644 --- a/app/controllers/v3/service_plans_controller.rb +++ b/app/controllers/v3/service_plans_controller.rb @@ -35,7 +35,9 @@ def index ServicePlanListFetcher.fetch( message, eager_loaded_associations: Presenters::V3::ServicePlanPresenter.associated_resources, + readable_org_ids_query: permission_queryer.readable_org_ids_query, readable_orgs_query: permission_queryer.readable_orgs_query, + readable_space_ids_query: permission_queryer.readable_space_scoped_space_ids_query, readable_spaces_query: permission_queryer.readable_space_scoped_spaces_query ) end diff --git a/app/fetchers/base_service_list_fetcher.rb b/app/fetchers/base_service_list_fetcher.rb index c38e6dd3ce9..1245a2f9671 100644 --- a/app/fetchers/base_service_list_fetcher.rb +++ b/app/fetchers/base_service_list_fetcher.rb @@ -6,7 +6,10 @@ class BaseServiceListFetcher < BaseListFetcher class << self private - def fetch(klass, message, omniscient: false, readable_orgs_query: nil, readable_spaces_query: nil, eager_loaded_associations: []) + def fetch(klass, message, omniscient: false, + readable_org_ids_query: nil, readable_orgs_query: nil, + readable_space_ids_query: nil, readable_spaces_query: nil, + eager_loaded_associations: []) # The base dataset for the given model; other tables might be joined later on for filtering, # but we are only interested in the columns from the base table. dataset = klass.dataset.select_all(klass.table_name) @@ -15,10 +18,10 @@ def fetch(klass, message, omniscient: false, readable_orgs_query: nil, readable_ dataset = filter(message, dataset, klass) # Filter by permissions granted on org level for plans. - plan_dataset = readable_by_plan_org(dataset, readable_orgs_query) + plan_dataset = readable_by_plan_org(dataset, readable_org_ids_query) # Filter by permissions granted on space level for brokers. - broker_dataset = readable_by_broker_space(dataset, readable_spaces_query) + broker_dataset = readable_by_broker_space(dataset, readable_space_ids_query) # Apply additional filtering by org / space guids (if requested). if message.requested?(:organization_guids) @@ -27,8 +30,9 @@ def fetch(klass, message, omniscient: false, readable_orgs_query: nil, readable_ plan_dataset, broker_dataset, omniscient, + readable_org_ids_query, readable_orgs_query, - readable_spaces_query, + readable_space_ids_query, dataset ) end @@ -39,7 +43,8 @@ def fetch(klass, message, omniscient: false, readable_orgs_query: nil, readable_ plan_dataset, broker_dataset, omniscient, - readable_orgs_query, + readable_org_ids_query, + readable_space_ids_query, readable_spaces_query, dataset ) @@ -66,36 +71,38 @@ def fetch(klass, message, omniscient: false, readable_orgs_query: nil, readable_ dataset.eager(eager_loaded_associations) end - def readable_by_plan_org(dataset, readable_orgs_query) - return if readable_orgs_query.nil? + def readable_by_plan_org(dataset, readable_org_ids_query) + return if readable_org_ids_query.nil? plan_dataset = dataset.clone plan_dataset = join_plan_org_visibilities(plan_dataset) - plan_dataset.where { Sequel[:service_plan_visibilities][:organization_id] =~ readable_orgs_query.select(:id) } + plan_dataset.where { Sequel[:service_plan_visibilities][:organization_id] =~ readable_org_ids_query } end - def readable_by_broker_space(dataset, readable_spaces_query) - return if readable_spaces_query.nil? + def readable_by_broker_space(dataset, readable_space_ids_query) + return if readable_space_ids_query.nil? broker_dataset = dataset.clone broker_dataset = join_service_brokers(broker_dataset) - broker_dataset.where { Sequel[:service_brokers][:space_id] =~ readable_spaces_query.select(:id) } + broker_dataset.where { Sequel[:service_brokers][:space_id] =~ readable_space_ids_query } end - def filter_by_org_guid(org_guids, plan_dataset, broker_dataset, omniscient, readable_orgs_query, readable_spaces_query, dataset) + # The readable_orgs_query projects the readable guid set against the user-supplied filter. + # The readable_org_ids_query / readable_space_ids_query gate which sub-datasets are included. + def filter_by_org_guid(org_guids, plan_dataset, broker_dataset, omniscient, readable_org_ids_query, readable_orgs_query, readable_space_ids_query, dataset) authorized_org_guids = if !omniscient && !readable_orgs_query.nil? readable_orgs_query.where(guid: org_guids).select_map(:guid) else org_guids end - if omniscient || !readable_orgs_query.nil? + if omniscient || !readable_org_ids_query.nil? plan_dataset = dataset.clone if plan_dataset.nil? plan_dataset = join_plan_orgs(plan_dataset) plan_dataset = plan_dataset.where { Sequel[:plan_orgs][:guid] =~ authorized_org_guids } end - if omniscient || !readable_spaces_query.nil? + if omniscient || !readable_space_ids_query.nil? broker_dataset = dataset.clone if broker_dataset.nil? broker_dataset = join_broker_orgs(broker_dataset) broker_dataset = broker_dataset.where { Sequel[:broker_orgs][:guid] =~ authorized_org_guids } @@ -104,20 +111,22 @@ def filter_by_org_guid(org_guids, plan_dataset, broker_dataset, omniscient, read [plan_dataset, broker_dataset] end - def filter_by_space_guid(space_guids, plan_dataset, broker_dataset, omniscient, readable_orgs_query, readable_spaces_query, dataset) + # The readable_spaces_query projects the readable guid set against the user-supplied filter. + # The readable_org_ids_query / readable_space_ids_query gate which sub-datasets are included. + def filter_by_space_guid(space_guids, plan_dataset, broker_dataset, omniscient, readable_org_ids_query, readable_space_ids_query, readable_spaces_query, dataset) authorized_space_guids = if !omniscient && !readable_spaces_query.nil? readable_spaces_query.where(guid: space_guids).select_map(:guid) else space_guids end - if omniscient || !readable_orgs_query.nil? + if omniscient || !readable_org_ids_query.nil? plan_dataset = dataset.clone if plan_dataset.nil? plan_dataset = join_plan_spaces(plan_dataset) plan_dataset = plan_dataset.where { Sequel[:plan_spaces][:guid] =~ authorized_space_guids } end - if omniscient || !readable_spaces_query.nil? + if omniscient || !readable_space_ids_query.nil? broker_dataset = dataset.clone if broker_dataset.nil? broker_dataset = join_broker_spaces(broker_dataset) broker_dataset = broker_dataset.where { Sequel[:broker_spaces][:guid] =~ authorized_space_guids } diff --git a/app/fetchers/service_credential_binding_fetcher.rb b/app/fetchers/service_credential_binding_fetcher.rb index be6a77dc58a..cb821ea662e 100644 --- a/app/fetchers/service_credential_binding_fetcher.rb +++ b/app/fetchers/service_credential_binding_fetcher.rb @@ -3,8 +3,8 @@ module VCAP module CloudController class ServiceCredentialBindingFetcher - def fetch(guid, readable_spaces_query: nil) - list_fetcher.fetch(readable_spaces_query:).first(guid:) + def fetch(guid, readable_space_ids_query: nil) + list_fetcher.fetch(readable_space_ids_query:).first(guid:) end private diff --git a/app/fetchers/service_credential_binding_list_fetcher.rb b/app/fetchers/service_credential_binding_list_fetcher.rb index bf1c27e988a..daf4108ff29 100644 --- a/app/fetchers/service_credential_binding_list_fetcher.rb +++ b/app/fetchers/service_credential_binding_list_fetcher.rb @@ -18,12 +18,12 @@ class << self service_offering_guid ].freeze - def fetch(readable_spaces_query: nil, message: nil, eager_loaded_associations: []) - dataset = case readable_spaces_query + def fetch(readable_space_ids_query: nil, message: nil, eager_loaded_associations: []) + dataset = case readable_space_ids_query when nil ServiceCredentialBinding::View.dataset else - ServiceCredentialBinding::View.where { Sequel[:space_id] =~ readable_spaces_query.select(:id) } + ServiceCredentialBinding::View.where { Sequel[:space_id] =~ readable_space_ids_query } end dataset = dataset.eager(eager_loaded_associations) diff --git a/app/fetchers/service_offering_list_fetcher.rb b/app/fetchers/service_offering_list_fetcher.rb index 8342ef8d3b0..64a40e520df 100644 --- a/app/fetchers/service_offering_list_fetcher.rb +++ b/app/fetchers/service_offering_list_fetcher.rb @@ -3,11 +3,16 @@ module VCAP::CloudController class ServiceOfferingListFetcher < BaseServiceListFetcher class << self - def fetch(message, omniscient: false, readable_orgs_query: nil, readable_spaces_query: nil, eager_loaded_associations: []) + def fetch(message, omniscient: false, + readable_org_ids_query: nil, readable_orgs_query: nil, + readable_space_ids_query: nil, readable_spaces_query: nil, + eager_loaded_associations: []) super(Service, message, omniscient:, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query:, eager_loaded_associations:) end diff --git a/app/fetchers/service_plan_list_fetcher.rb b/app/fetchers/service_plan_list_fetcher.rb index 2ae1ca04431..bc2adc08e94 100644 --- a/app/fetchers/service_plan_list_fetcher.rb +++ b/app/fetchers/service_plan_list_fetcher.rb @@ -3,11 +3,16 @@ module VCAP::CloudController class ServicePlanListFetcher < BaseServiceListFetcher class << self - def fetch(message, omniscient: false, readable_orgs_query: nil, readable_spaces_query: nil, eager_loaded_associations: []) + def fetch(message, omniscient: false, + readable_org_ids_query: nil, readable_orgs_query: nil, + readable_space_ids_query: nil, readable_spaces_query: nil, + eager_loaded_associations: []) super(ServicePlan, message, omniscient: omniscient, + readable_org_ids_query: readable_org_ids_query, readable_orgs_query: readable_orgs_query, + readable_space_ids_query: readable_space_ids_query, readable_spaces_query: readable_spaces_query, eager_loaded_associations: eager_loaded_associations.append(:orgs_visibility)) end diff --git a/lib/cloud_controller/membership.rb b/lib/cloud_controller/membership.rb index 178d41a1c85..b75c353ccba 100644 --- a/lib/cloud_controller/membership.rb +++ b/lib/cloud_controller/membership.rb @@ -25,7 +25,7 @@ def authorized_org_guids(roles) end def authorized_org_guids_subquery(roles) - authorized_orgs_subquery(roles).select(:guid) + Organization.where(id: authorized_org_ids_subquery(roles)).select(:guid) end def authorized_orgs_subquery(roles) @@ -50,7 +50,7 @@ def authorized_space_guids(roles) end def authorized_space_guids_subquery(roles) - authorized_spaces_subquery(roles).select(:guid) + Space.where(id: authorized_space_ids_subquery(roles)).select(:guid) end def authorized_spaces_subquery(roles) diff --git a/lib/cloud_controller/permissions.rb b/lib/cloud_controller/permissions.rb index eb4ec13116e..421c2a389eb 100644 --- a/lib/cloud_controller/permissions.rb +++ b/lib/cloud_controller/permissions.rb @@ -98,7 +98,7 @@ def can_write_globally? end def is_org_manager? - membership.authorized_orgs_subquery(VCAP::CloudController::Membership::ORG_MANAGER).any? + membership.authorized_org_ids_subquery(VCAP::CloudController::Membership::ORG_MANAGER).any? end def readable_org_guids @@ -251,6 +251,14 @@ def readable_space_scoped_spaces_query end end + def readable_space_scoped_space_ids_query + if can_read_globally? + VCAP::CloudController::Space.select(:id) + else + membership.authorized_space_ids_subquery(SPACE_ROLES) + end + end + def can_read_route?(space_id) return true if can_read_globally? @@ -289,8 +297,8 @@ def readable_space_quota_guids if can_read_globally? VCAP::CloudController::SpaceQuotaDefinition.select_map(:guid) elsif @user - visible_space_ids = membership.authorized_spaces_subquery(SPACE_ROLES).select(:id) - org_manager_org_ids = membership.authorized_orgs_subquery(VCAP::CloudController::Membership::ORG_MANAGER).select(:id) + visible_space_ids = membership.authorized_space_ids_subquery(SPACE_ROLES) + org_manager_org_ids = membership.authorized_org_ids_subquery(VCAP::CloudController::Membership::ORG_MANAGER) VCAP::CloudController::SpaceQuotaDefinition.where( Sequel.or([ @@ -313,7 +321,7 @@ def readable_security_group_guids_query if can_read_globally? VCAP::CloudController::SecurityGroup.dataset.select(:guid) elsif @user - visible_space_ids = membership.authorized_spaces_subquery(ROLES_FOR_SPACE_READING).select(:id) + visible_space_ids = membership.authorized_space_ids_subquery(ROLES_FOR_SPACE_READING) VCAP::CloudController::SecurityGroup.where( Sequel.or([ diff --git a/spec/unit/fetchers/service_credential_binding_fetcher_spec.rb b/spec/unit/fetchers/service_credential_binding_fetcher_spec.rb index 4cf0cb6984d..3a071de250f 100644 --- a/spec/unit/fetchers/service_credential_binding_fetcher_spec.rb +++ b/spec/unit/fetchers/service_credential_binding_fetcher_spec.rb @@ -10,20 +10,20 @@ module CloudController let!(:existing_credential_binding) { ServiceBinding.make } it 'returns nothing' do - credential_binding = fetcher.fetch('does-not-exist', readable_spaces_query: nil) + credential_binding = fetcher.fetch('does-not-exist', readable_space_ids_query: nil) expect(credential_binding).to be_nil end end describe 'service keys' do let(:space) { Space.make } - let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) } + let(:readable_space_ids_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)).select(:id) } let(:service_instance) { ManagedServiceInstance.make(space:) } let!(:service_key) { ServiceKey.make(service_instance:) } describe 'when in the space' do it 'can be found' do - credential_binding = fetcher.fetch(service_key.guid, readable_spaces_query:) + credential_binding = fetcher.fetch(service_key.guid, readable_space_ids_query:) expect(credential_binding).not_to be_nil expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceKey) @@ -36,10 +36,10 @@ module CloudController describe 'when not in the space' do let!(:other_space) { Space.make } - let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) } + let!(:readable_space_ids_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)).select(:id) } it 'can not be found' do - credential_binding = fetcher.fetch(service_key.guid, readable_spaces_query:) + credential_binding = fetcher.fetch(service_key.guid, readable_space_ids_query:) expect(credential_binding).to be_nil end @@ -49,12 +49,12 @@ module CloudController describe 'app bindings' do describe 'managed services' do let(:space) { Space.make } - let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) } + let(:readable_space_ids_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)).select(:id) } let(:service_instance) { ManagedServiceInstance.make(space:) } let!(:app_binding) { ServiceBinding.make(service_instance: service_instance, name: 'some-name') } it 'can be found' do - credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query:) + credential_binding = fetcher.fetch(app_binding.guid, readable_space_ids_query:) expect(credential_binding).not_to be_nil expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceBinding) @@ -68,10 +68,10 @@ module CloudController describe 'when not in the space' do let!(:other_space) { Space.make } - let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) } + let!(:readable_space_ids_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)).select(:id) } it 'can not be found' do - credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query:) + credential_binding = fetcher.fetch(app_binding.guid, readable_space_ids_query:) expect(credential_binding).to be_nil end @@ -80,12 +80,12 @@ module CloudController describe 'user provided services' do let(:space) { Space.make } - let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) } + let(:readable_space_ids_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)).select(:id) } let(:service_instance) { UserProvidedServiceInstance.make(space:) } let!(:app_binding) { ServiceBinding.make(service_instance: service_instance, name: 'some-name') } it 'can be found' do - credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query:) + credential_binding = fetcher.fetch(app_binding.guid, readable_space_ids_query:) expect(credential_binding).not_to be_nil expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceBinding) @@ -99,10 +99,10 @@ module CloudController describe 'when not in the space' do let!(:other_space) { Space.make } - let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) } + let!(:readable_space_ids_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)).select(:id) } it 'can not be found' do - credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query:) + credential_binding = fetcher.fetch(app_binding.guid, readable_space_ids_query:) expect(credential_binding).to be_nil end @@ -111,7 +111,7 @@ module CloudController describe 'with last operation' do let(:service_instance) { ManagedServiceInstance.make } - let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [service_instance.space].map(&:id)) } + let(:readable_space_ids_query) { VCAP::CloudController::Space.where(id: [service_instance.space].map(&:id)).select(:id) } let!(:app_binding) do binding = ServiceBinding.make(service_instance:) binding.save_with_new_operation({ state: 'succeeded', type: 'create', description: 'radical avocado' }) @@ -119,7 +119,7 @@ module CloudController end it 'fetches the last operation' do - credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query:) + credential_binding = fetcher.fetch(app_binding.guid, readable_space_ids_query:) expect(credential_binding.last_operation).not_to be_nil diff --git a/spec/unit/fetchers/service_credential_binding_list_fetcher_spec.rb b/spec/unit/fetchers/service_credential_binding_list_fetcher_spec.rb index 2cef4c48032..6e55ba510c8 100644 --- a/spec/unit/fetchers/service_credential_binding_list_fetcher_spec.rb +++ b/spec/unit/fetchers/service_credential_binding_list_fetcher_spec.rb @@ -12,7 +12,7 @@ module CloudController describe 'no bindings' do it 'returns an empty result' do - expect(fetcher.fetch(readable_spaces_query: nil, message: message).all).to eql([]) + expect(fetcher.fetch(readable_space_ids_query: nil, message: message).all).to eql([]) end end @@ -37,7 +37,7 @@ module CloudController let!(:app_binding) { VCAP::CloudController::ServiceBinding.make(service_instance: instance, name: Sham.name, **app_binding_details) } it 'eager loads the specified resources' do - dataset = fetcher.fetch(readable_spaces_query: nil, message: message, eager_loaded_associations: [:labels_sti_eager_load]) + dataset = fetcher.fetch(readable_space_ids_query: nil, message: message, eager_loaded_associations: [:labels_sti_eager_load]) expect(dataset.all.first.associations.key?(:labels)).to be true expect(dataset.all.first.associations.key?(:annotations)).to be false @@ -45,7 +45,7 @@ module CloudController context 'when getting everything' do it 'returns both key and app bindings' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all to_hash = lambda { |b| { guid: b.guid, @@ -63,12 +63,12 @@ module CloudController context 'when limiting to a space' do let(:other_space) { VCAP::CloudController::Space.make } let(:other_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: other_space) } - let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) } + let(:readable_space_ids_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)).select(:id) } let!(:key_other_binding) { VCAP::CloudController::ServiceKey.make(service_instance: other_instance) } let!(:app_other_binding) { VCAP::CloudController::ServiceBinding.make(service_instance: other_instance) } it 'returns only the bindings within that space' do - bindings = fetcher.fetch(readable_spaces_query:, message:).all + bindings = fetcher.fetch(readable_space_ids_query:, message:).all binding_guids = bindings.map(&:guid) expect(binding_guids).to contain_exactly(key_binding.guid, app_binding.guid) @@ -84,7 +84,7 @@ module CloudController let(:params) { { 'service_instance_names' => instance.name } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid) end end @@ -93,7 +93,7 @@ module CloudController let(:params) { { 'service_instance_guids' => instance.guid } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid) end end @@ -102,7 +102,7 @@ module CloudController let(:params) { { 'app_names' => "#{app_binding.app.name},'some-other-name'" } } it 'can filter by app name' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid) end end @@ -111,7 +111,7 @@ module CloudController let(:params) { { 'app_guids' => "#{app_binding.app.guid}, 'some-other-guid'" } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid) end end @@ -120,7 +120,7 @@ module CloudController let(:params) { { 'names' => "#{key_binding.name},#{app_binding.name}" } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid) end end @@ -129,7 +129,7 @@ module CloudController let(:params) { { 'service_plan_names' => instance.service_plan.name } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid) end end @@ -138,7 +138,7 @@ module CloudController let(:params) { { 'service_plan_guids' => instance.service_plan.guid } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid) end end @@ -147,7 +147,7 @@ module CloudController let(:params) { { 'service_offering_names' => app_binding.service.name.to_s } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid) end end @@ -156,7 +156,7 @@ module CloudController let(:params) { { 'service_offering_guids' => app_binding.service.guid.to_s } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid) end end @@ -177,7 +177,7 @@ module CloudController let(:params) { { 'label_selector' => 'fruit=strawberry,tier in (backend,worker)' } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, another_binding.guid) end end @@ -187,7 +187,7 @@ module CloudController let(:params) { { 'type' => 'app' } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, another_binding.guid) end end @@ -196,14 +196,14 @@ module CloudController let(:params) { { type: 'key' } } it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, another_key.guid) end end end it 'returns all if no filter is passed' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.count).to eq(4) end @@ -213,7 +213,7 @@ module CloudController end it 'returns empty' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings).to be_empty end end @@ -224,7 +224,7 @@ module CloudController end it 'returns the right result' do - bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all + bindings = fetcher.fetch(readable_space_ids_query: nil, message: message).all expect(bindings.map(&:guid)).to contain_exactly(another_binding.guid) end end @@ -243,7 +243,7 @@ module CloudController } ) - credential_binding = fetcher.fetch(readable_spaces_query: nil, message: message).first + credential_binding = fetcher.fetch(readable_space_ids_query: nil, message: message).first last_operation = credential_binding.last_operation expect(last_operation).to be_present diff --git a/spec/unit/fetchers/service_offering_list_fetcher_spec.rb b/spec/unit/fetchers/service_offering_list_fetcher_spec.rb index 6870940f3f7..1dce3e2632c 100644 --- a/spec/unit/fetchers/service_offering_list_fetcher_spec.rb +++ b/spec/unit/fetchers/service_offering_list_fetcher_spec.rb @@ -9,7 +9,9 @@ module VCAP::CloudController describe '#fetch' do let(:readable_orgs_query) { VCAP::CloudController::Organization.where(id: readable_orgs&.map(&:id)) } + let(:readable_org_ids_query) { readable_orgs_query.select(:id) } let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: readable_spaces&.map(&:id)) } + let(:readable_space_ids_query) { readable_spaces_query.select(:id) } context 'when there are no offerings' do it 'is empty' do @@ -28,7 +30,7 @@ module VCAP::CloudController service_plan = ServicePlan.make(public: false, service: service_offering) ServicePlanVisibility.make(organization: org, service_plan: service_plan) - service_offerings = fetcher.fetch(message, readable_orgs_query:).all + service_offerings = fetcher.fetch(message, readable_org_ids_query:, readable_orgs_query:).all expect(service_offerings).to contain_exactly(service_offering) end end @@ -100,7 +102,9 @@ module VCAP::CloudController it 'includes public plans and ones for those orgs' do service_offerings = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -122,7 +126,9 @@ module VCAP::CloudController it 'includes public plans, ones for those spaces and ones for those orgs' do service_offerings = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -206,7 +212,9 @@ module VCAP::CloudController service_offerings = ServiceOfferingListFetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -225,7 +233,9 @@ module VCAP::CloudController service_offerings = ServiceOfferingListFetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -245,7 +255,9 @@ module VCAP::CloudController service_offerings = ServiceOfferingListFetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -284,7 +296,9 @@ module VCAP::CloudController }.with_indifferent_access) service_offerings = ServiceOfferingListFetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all expect(service_offerings).to contain_exactly(space_scoped_offering_1_1, org_restricted_offering_1, org_restricted_offering_4, public_offering) @@ -301,7 +315,9 @@ module VCAP::CloudController }.with_indifferent_access) service_offerings = ServiceOfferingListFetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all expect(service_offerings).to contain_exactly(space_scoped_offering_1_1, org_restricted_offering_1, org_restricted_offering_4, public_offering) @@ -319,7 +335,9 @@ module VCAP::CloudController service_offerings = ServiceOfferingListFetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -360,7 +378,9 @@ module VCAP::CloudController }.with_indifferent_access) service_offerings = ServiceOfferingListFetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all expect(service_offerings).to contain_exactly(public_offering) diff --git a/spec/unit/fetchers/service_plan_list_fetcher_spec.rb b/spec/unit/fetchers/service_plan_list_fetcher_spec.rb index 4e45898df31..5633c01de6b 100644 --- a/spec/unit/fetchers/service_plan_list_fetcher_spec.rb +++ b/spec/unit/fetchers/service_plan_list_fetcher_spec.rb @@ -9,7 +9,9 @@ module VCAP::CloudController describe '#fetch' do let(:readable_orgs_query) { VCAP::CloudController::Organization.where(id: readable_orgs&.map(&:id)) } + let(:readable_org_ids_query) { readable_orgs_query.select(:id) } let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: readable_spaces&.map(&:id)) } + let(:readable_space_ids_query) { readable_spaces_query.select(:id) } context 'when there are no plans' do it 'is empty' do @@ -94,7 +96,9 @@ module VCAP::CloudController it 'includes public plans and ones for those orgs' do service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -116,7 +120,9 @@ module VCAP::CloudController it 'includes public plans, ones for those spaces and ones for those orgs' do service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -195,7 +201,9 @@ module VCAP::CloudController service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -214,7 +222,9 @@ module VCAP::CloudController service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -234,7 +244,9 @@ module VCAP::CloudController service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -272,7 +284,9 @@ module VCAP::CloudController }.with_indifferent_access) service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all expect(service_plans).to contain_exactly(space_scoped_plan_1_1, org_restricted_plan_1, org_restricted_plan_4, public_plan) @@ -289,7 +303,9 @@ module VCAP::CloudController }.with_indifferent_access) service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all expect(service_plans).to contain_exactly(space_scoped_plan_1_1, org_restricted_plan_1, org_restricted_plan_4, public_plan) @@ -307,7 +323,9 @@ module VCAP::CloudController service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all @@ -348,7 +366,9 @@ module VCAP::CloudController }.with_indifferent_access) service_plans = fetcher.fetch( message, + readable_org_ids_query:, readable_orgs_query:, + readable_space_ids_query:, readable_spaces_query: ).all expect(service_plans).to contain_exactly(public_plan) diff --git a/spec/unit/lib/cloud_controller/permissions_spec.rb b/spec/unit/lib/cloud_controller/permissions_spec.rb index a2efd751a58..85e1c5714e9 100644 --- a/spec/unit/lib/cloud_controller/permissions_spec.rb +++ b/spec/unit/lib/cloud_controller/permissions_spec.rb @@ -615,6 +615,16 @@ module VCAP::CloudController end end + describe '#readable_space_scoped_space_ids_query' do + it 'returns subquery from membership' do + membership = instance_double(Membership) + subquery = instance_double(Sequel::Dataset) + expect(Membership).to receive(:new).with(user).and_return(membership) + expect(membership).to receive(:authorized_space_ids_subquery).with(Permissions::SPACE_ROLES).and_return(subquery) + expect(permissions.readable_space_scoped_space_ids_query).to be(subquery) + end + end + describe '#can_write_to_active_space?' do context 'user has no membership' do context 'and user is an admin' do