Use permissions subqueries in service list fetchers

app/controllers/v3/service_offerings_controller.rb

@@ -34,8 +34,8 @@ def index
               else
                 ServiceOfferingListFetcher.fetch(
                   message,
-                  readable_orgs: permission_queryer.readable_orgs,
-                  readable_spaces: permission_queryer.readable_space_scoped_spaces,
+                  readable_orgs_query: permission_queryer.readable_orgs_query,
+                  readable_spaces_query: permission_queryer.readable_space_scoped_spaces_query,
                   eager_loaded_associations: Presenters::V3::ServiceOfferingPresenter.associated_resources,
                 )
               end

app/controllers/v3/service_plans_controller.rb

@@ -54,8 +54,8 @@ def index
                 ServicePlanListFetcher.fetch(
                   message,
                   eager_loaded_associations: Presenters::V3::ServicePlanPresenter.associated_resources,
-                  readable_orgs: permission_queryer.readable_orgs,
-                  readable_spaces: permission_queryer.readable_space_scoped_spaces,
+                  readable_orgs_query: permission_queryer.readable_orgs_query,
+                  readable_spaces_query: permission_queryer.readable_space_scoped_spaces_query,
                 )
               end
 

app/fetchers/base_service_list_fetcher.rb

@@ -7,27 +7,29 @@ class BaseServiceListFetcher < BaseListFetcher
     class << self
       private
 
-      def select_readable(dataset, message, omniscient: false, readable_spaces: [], readable_orgs: [])
-        if readable_orgs.any?
+      def select_readable(dataset, message, omniscient: false, readable_spaces_query: nil, readable_orgs_query: nil)
+        if readable_orgs_query
           dataset = join_service_plans(dataset)
           dataset = join_plan_org_visibilities(dataset)
           dataset = join_service_brokers(dataset)
 
+          readable_space_ids_query = readable_spaces_query ? readable_spaces_query.select(:id) : nil
           dataset = dataset.where do
             (Sequel[:service_plans][:public] =~ true) |
-              (Sequel[:service_plan_visibilities][:organization_id] =~ readable_orgs.map(&:id)) |
-              (Sequel[:service_brokers][:space_id] =~ readable_spaces.map(&:id))
+              (Sequel[:service_plan_visibilities][:organization_id] =~ readable_orgs_query.select(:id)) |
+              (Sequel[:service_brokers][:space_id] =~ readable_space_ids_query)
           end
         elsif !omniscient
           dataset = join_service_plans(dataset)
           dataset = dataset.where { Sequel[:service_plans][:public] =~ true }
         end
 
         if message.requested?(:space_guids)
+          readable_space_guids = readable_spaces_query ? readable_spaces_query.select(:guid).all.map(&:guid) : []
           dataset = filter_spaces(
             dataset,
             filtered_space_guids: message.space_guids,
-            readable_space_guids: readable_spaces.map(&:guid),
+            readable_space_guids: readable_space_guids,
             omniscient: omniscient,
           )
         end

app/fetchers/service_offering_list_fetcher.rb

@@ -3,13 +3,13 @@
 module VCAP::CloudController
   class ServiceOfferingListFetcher < BaseServiceListFetcher
     class << self
-      def fetch(message, omniscient: false, readable_spaces: [], readable_orgs: [], eager_loaded_associations: [])
+      def fetch(message, omniscient: false, readable_spaces_query: nil, readable_orgs_query: nil, eager_loaded_associations: [])
         dataset = select_readable(
           Service.dataset.eager(eager_loaded_associations),
           message,
           omniscient: omniscient,
-          readable_orgs: readable_orgs,
-          readable_spaces: readable_spaces,
+          readable_orgs_query: readable_orgs_query,
+          readable_spaces_query: readable_spaces_query,
         )
 
         filter(message, dataset).select_all(:services).distinct

app/fetchers/service_plan_list_fetcher.rb

@@ -3,13 +3,13 @@
 module VCAP::CloudController
   class ServicePlanListFetcher < BaseServiceListFetcher
     class << self
-      def fetch(message, omniscient: false, readable_spaces: [], readable_orgs: [], eager_loaded_associations: [])
+      def fetch(message, omniscient: false, readable_spaces_query: nil, readable_orgs_query: nil, eager_loaded_associations: [])
         dataset = select_readable(
           ServicePlan.dataset.eager(eager_loaded_associations),
           message,
           omniscient: omniscient,
-          readable_orgs: readable_orgs,
-          readable_spaces: readable_spaces,
+          readable_orgs_query: readable_orgs_query,
+          readable_spaces_query: readable_spaces_query,
         )
 
         filter(message, dataset).select_all(:service_plans).distinct

lib/cloud_controller/permissions.rb

@@ -123,10 +123,14 @@ def readable_org_guids_query
   end
 
   def readable_orgs
+    readable_orgs_query.all
+  end
+
+  def readable_orgs_query
     if can_read_globally?
-      VCAP::CloudController::Organization.select(:id, :guid).all
+      VCAP::CloudController::Organization.select(:id, :guid)
     else
-      membership.orgs_for_roles_subquery(ROLES_FOR_ORG_READING).all
+      membership.orgs_for_roles_subquery(ROLES_FOR_ORG_READING)
     end
   end
 
@@ -250,10 +254,14 @@ def readable_space_scoped_space_guids
   end
 
   def readable_space_scoped_spaces
+    readable_space_scoped_spaces_query.all
+  end
+
+  def readable_space_scoped_spaces_query
     if can_read_globally?
-      VCAP::CloudController::Space.select(:id, :guid).all
+      VCAP::CloudController::Space.select(:id, :guid)
     else
-      membership.spaces_for_roles_subquery(SPACE_ROLES).all
+      membership.spaces_for_roles_subquery(SPACE_ROLES)
     end
   end
 

spec/unit/fetchers/service_offering_list_fetcher_spec.rb

@@ -8,6 +8,9 @@ module VCAP::CloudController
     let(:fetcher) { described_class }
 
     describe '#fetch' do
+      let(:readable_orgs_query) { VCAP::CloudController::Organization.where(id: readable_orgs&.map(&:id)) }
+      let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: readable_spaces&.map(&:id)) }
+
       context 'when there are no offerings' do
         it 'is empty' do
           service_offerings = ServiceOfferingListFetcher.fetch(message, omniscient: true).all
@@ -66,11 +69,14 @@ module VCAP::CloudController
         end
 
         context 'when `readable_orgs` are specified' do
+          let(:readable_orgs) { [org_1, org_3] }
+          let(:readable_spaces) { [] }
+
           it 'includes public plans and ones for those orgs' do
             service_offerings = fetcher.fetch(
               message,
-              readable_orgs: [org_1, org_3],
-              readable_spaces: [],
+              readable_orgs_query: readable_orgs_query,
+              readable_spaces_query: readable_spaces_query,
             ).all
 
             expect(service_offerings).to contain_exactly(
@@ -84,11 +90,14 @@ module VCAP::CloudController
         end
 
         context 'when both `readable_spaces` and `readable_orgs` are specified' do
+          let(:readable_orgs) { [org_3] }
+          let(:readable_spaces) { [space_3] }
+
           it 'includes public plans, ones for those spaces and ones for those orgs' do
             service_offerings = fetcher.fetch(
               message,
-              readable_spaces: [space_3],
-              readable_orgs: [org_3],
+              readable_orgs_query: readable_orgs_query,
+              readable_spaces_query: readable_spaces_query,
             ).all
 
             expect(service_offerings).to contain_exactly(
@@ -145,31 +154,37 @@ module VCAP::CloudController
           end
 
           context 'only some orgs are readable (ORG_AUDITOR etc)' do
+            let(:readable_orgs) { [org_1] }
+            let(:readable_spaces) { [] }
+
             it 'shows only readable plans' do
               message = ServiceOfferingsListMessage.from_params({
                 organization_guids: [org_1.guid, org_2.guid].join(',')
               }.with_indifferent_access)
 
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_orgs: [org_1],
-                readable_spaces: [],
+                readable_orgs_query: readable_orgs_query,
+                readable_spaces_query: readable_spaces_query,
               ).all
 
               expect(service_offerings).to contain_exactly(org_restricted_offering_1, public_offering)
             end
           end
 
           context 'no orgs are readable' do
+            let(:readable_orgs) { [] }
+            let(:readable_spaces) { [] }
+
             it 'shows only public plans' do
               message = ServiceOfferingsListMessage.from_params({
                 organization_guids: [org_1.guid, org_2.guid].join(',')
               }.with_indifferent_access)
 
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_orgs: [],
-                readable_spaces: [],
+                readable_orgs_query: readable_orgs_query,
+                readable_spaces_query: readable_spaces_query,
               ).all
 
               expect(service_offerings).to contain_exactly(public_offering)
@@ -179,6 +194,8 @@ module VCAP::CloudController
           context 'when the user only has access to some spaces in an org' do
             let(:space_1a) { Space.make(organization: org_1) }
             let!(:space_scoped_offering_1a) { make_space_scoped_offering(space_1a) }
+            let(:readable_orgs) { [org_1] }
+            let(:readable_spaces) { [space_1] }
 
             it 'only shows space-scoped plans for the readable spaces' do
               message = ServiceOfferingsListMessage.from_params({
@@ -187,8 +204,8 @@ module VCAP::CloudController
 
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_orgs: [org_1],
-                readable_spaces: [space_1],
+                readable_orgs_query: readable_orgs_query,
+                readable_spaces_query: readable_spaces_query,
               ).all
 
               expect(service_offerings).to contain_exactly(public_offering, org_restricted_offering_1, space_scoped_offering_1)
@@ -216,43 +233,52 @@ module VCAP::CloudController
           end
 
           context 'only some spaces are readable (SPACE_DEVELOPER, SPACE_AUDITOR etc)' do
+            let(:readable_orgs) { [org_1] }
+            let(:readable_spaces) { [space_1] }
+
             it 'shows only plans for readable spaces and orgs' do
               message = ServiceOfferingsListMessage.from_params({
                 space_guids: [space_1.guid, space_2.guid].join(',')
               }.with_indifferent_access)
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_orgs: [org_1],
-                readable_spaces: [space_1],
+                readable_orgs_query: readable_orgs_query,
+                readable_spaces_query: readable_spaces_query,
               ).all
               expect(service_offerings).to contain_exactly(space_scoped_offering_1, org_restricted_offering_1, public_offering)
             end
           end
 
           context 'when a filter contains a space guid that the user cannot access' do
+            let(:readable_orgs) { [org_1, org_2] }
+            let(:readable_spaces) { [space_1] }
+
             it 'ignores the unauthorized space guid' do
               message = ServiceOfferingsListMessage.from_params({
                 space_guids: [space_1.guid, space_2.guid].join(',')
               }.with_indifferent_access)
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_orgs: [org_1, org_2],
-                readable_spaces: [space_1],
+                readable_orgs_query: readable_orgs_query,
+                readable_spaces_query: readable_spaces_query,
               ).all
               expect(service_offerings).to contain_exactly(space_scoped_offering_1, org_restricted_offering_1, public_offering)
             end
           end
 
           context 'no spaces are readable' do
+            let(:readable_orgs) { [] }
+            let(:readable_spaces) { [] }
+
             it 'shows only public plans' do
               message = ServiceOfferingsListMessage.from_params({
                 space_guids: [space_1.guid, space_2.guid].join(',')
               }.with_indifferent_access)
 
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_orgs: [],
-                readable_spaces: [],
+                readable_orgs_query: readable_orgs_query,
+                readable_spaces_query: readable_spaces_query,
               ).all
 
               expect(service_offerings).to contain_exactly(public_offering)
@@ -282,15 +308,18 @@ module VCAP::CloudController
           end
 
           context 'when only some spaces and orgs are visible' do
+            let(:readable_orgs) { [org_1, org_2] }
+            let(:readable_spaces) { [space_1] }
+
             it 'excludes plans that do not meet all the filter conditions' do
               message = ServiceOfferingsListMessage.from_params({
                 space_guids: [space_1.guid, space_2.guid].join(','),
                 organization_guids: [org_2.guid, org_3.guid].join(',')
               }.with_indifferent_access)
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_orgs: [org_1, org_2],
-                readable_spaces: [space_1],
+                readable_orgs_query: readable_orgs_query,
+                readable_spaces_query: readable_spaces_query,
               ).all
               expect(service_offerings).to contain_exactly(public_offering)
             end