Remove joins to space and org tables for permissions

There were 2 joins performed to the space and org tables just so that the permissions check could filter by readable space guids and readable org guids. As we fetch the entire space/org object in the permissions check, it is simple to just grab the IDs instead. As the tables reference the space_id and organization_id, we can avoid joining to space and org and just do a WHERE IN check on those columns instead. For filtering, the API still accepts GUIDs so the joins are still performed there. Co-authored-by: Philipp Thun <[email protected]>
cloudfoundry · Oct 11, 2021 · a068b49 · a068b49
1 parent 79da053
commit a068b49
Show file tree

Hide file tree

Showing 9 changed files with 92 additions and 64 deletions.
diff --git a/app/controllers/v3/service_offerings_controller.rb b/app/controllers/v3/service_offerings_controller.rb
@@ -34,8 +34,8 @@ def index
               else
                 ServiceOfferingListFetcher.fetch(
                   message,
-                  readable_org_guids: permission_queryer.readable_org_guids,
-                  readable_space_guids: permission_queryer.readable_space_scoped_space_guids,
+                  readable_orgs: permission_queryer.readable_orgs,
+                  readable_spaces: permission_queryer.readable_space_scoped_spaces,
                   eager_loaded_associations: Presenters::V3::ServiceOfferingPresenter.associated_resources,
                 )
               end

diff --git a/app/controllers/v3/service_plans_controller.rb b/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_org_guids: permission_queryer.readable_org_guids,
-                  readable_space_guids: permission_queryer.readable_space_scoped_space_guids,
+                  readable_orgs: permission_queryer.readable_orgs,
+                  readable_spaces: permission_queryer.readable_space_scoped_spaces,
                 )
               end
 

diff --git a/app/fetchers/base_service_list_fetcher.rb b/app/fetchers/base_service_list_fetcher.rb
@@ -7,16 +7,16 @@ class BaseServiceListFetcher < BaseListFetcher
     class << self
       private
 
-      def select_readable(dataset, message, omniscient: false, readable_space_guids: [], readable_org_guids: [])
-        if readable_org_guids.any?
+      def select_readable(dataset, message, omniscient: false, readable_spaces: [], readable_orgs: [])
+        if readable_orgs.any?
           dataset = join_service_plans(dataset)
-          dataset = join_plan_orgs(dataset)
-          dataset = join_broker_spaces(dataset)
+          dataset = join_plan_org_visibilities(dataset)
+          dataset = join_service_brokers(dataset)
 
           dataset = dataset.where do
             (Sequel[:service_plans][:public] =~ true) |
-              (Sequel[:plan_orgs][:guid] =~ readable_org_guids) |
-              (Sequel[:broker_spaces][:guid] =~ readable_space_guids)
+              (Sequel[:service_plan_visibilities][:organization_id] =~ readable_orgs.map(&:id)) |
+              (Sequel[:service_brokers][:space_id] =~ readable_spaces.map(&:id))
           end
         elsif !omniscient
           dataset = join_service_plans(dataset)
@@ -27,7 +27,7 @@ def select_readable(dataset, message, omniscient: false, readable_space_guids: [
           dataset = filter_spaces(
             dataset,
             filtered_space_guids: message.space_guids,
-            readable_space_guids: readable_space_guids,
+            readable_space_guids: readable_spaces.map(&:guid),
             omniscient: omniscient,
           )
         end
@@ -119,9 +119,13 @@ def join_broker_orgs(dataset)
         join(dataset, :left, Sequel[:organizations].as(:broker_orgs), id: Sequel[:broker_spaces][:organization_id])
       end
 
-      def join_plan_orgs(dataset)
+      def join_plan_org_visibilities(dataset)
         dataset = join_service_plans(dataset)
-        dataset = join(dataset, :left, :service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id])
+        join(dataset, :left, :service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id])
+      end
+
+      def join_plan_orgs(dataset)
+        dataset = join_plan_org_visibilities(dataset)
         join(dataset, :left, Sequel[:organizations].as(:plan_orgs), id: Sequel[:service_plan_visibilities][:organization_id])
       end
 

diff --git a/app/fetchers/service_offering_list_fetcher.rb b/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_space_guids: [], readable_org_guids: [], eager_loaded_associations: [])
+      def fetch(message, omniscient: false, readable_spaces: [], readable_orgs: [], eager_loaded_associations: [])
         dataset = select_readable(
           Service.dataset.eager(eager_loaded_associations),
           message,
           omniscient: omniscient,
-          readable_org_guids: readable_org_guids,
-          readable_space_guids: readable_space_guids,
+          readable_orgs: readable_orgs,
+          readable_spaces: readable_spaces,
         )
 
         filter(message, dataset).select_all(:services).distinct

diff --git a/app/fetchers/service_plan_list_fetcher.rb b/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_space_guids: [], readable_org_guids: [], eager_loaded_associations: [])
+      def fetch(message, omniscient: false, readable_spaces: [], readable_orgs: [], eager_loaded_associations: [])
         dataset = select_readable(
           ServicePlan.dataset.eager(eager_loaded_associations),
           message,
           omniscient: omniscient,
-          readable_org_guids: readable_org_guids,
-          readable_space_guids: readable_space_guids,
+          readable_orgs: readable_orgs,
+          readable_spaces: readable_spaces,
         )
 
         filter(message, dataset).select_all(:service_plans).distinct

diff --git a/lib/cloud_controller/membership.rb b/lib/cloud_controller/membership.rb
@@ -29,12 +29,16 @@ def org_guids_for_roles(roles)
     end
 
     def org_guids_for_roles_subquery(roles)
+      return orgs_for_roles_subquery(roles).select(:guid)
+    end
+
+    def orgs_for_roles_subquery(roles)
       org_role_dataset = OrganizationRole.where(type: org_roles(roles), user_id: @user.id)
-      org_guid_dataset = Organization.where(id: org_role_dataset.select(:organization_id)).select(:guid)
+      org_guid_dataset = Organization.where(id: org_role_dataset.select(:organization_id)).select(:id, :guid)
 
       space_role_dataset = SpaceRole.where(type: space_roles(roles), user_id: @user.id)
       spaces_dataset = Space.where(id: space_role_dataset.select(:space_id))
-      org_guids_for_space_roles_dataset = Organization.where(id: spaces_dataset.select(:organization_id)).select(:guid)
+      org_guids_for_space_roles_dataset = Organization.where(id: spaces_dataset.select(:organization_id)).select(:id, :guid)
 
       org_guids_for_space_roles_dataset.union(org_guid_dataset)
     end
@@ -44,11 +48,15 @@ def space_guids_for_roles(roles)
     end
 
     def space_guids_for_roles_subquery(roles)
+      spaces_for_roles_subquery(roles).select(:guid)
+    end
+
+    def spaces_for_roles_subquery(roles)
       space_role_dataset = SpaceRole.where(type: space_roles(roles), user_id: @user.id)
-      space_guid_dataset = Space.where(id: space_role_dataset.select(:space_id)).select(:guid)
+      space_guid_dataset = Space.where(id: space_role_dataset.select(:space_id)).select(:id, :guid)
 
       org_role_dataset = OrganizationRole.where(type: org_roles(roles), user_id: @user.id)
-      space_guids_for_org_roles_dataset = Space.where(organization_id: org_role_dataset.select(:organization_id)).select(:guid)
+      space_guids_for_org_roles_dataset = Space.where(organization_id: org_role_dataset.select(:organization_id)).select(:id, :guid)
 
       space_guids_for_org_roles_dataset.union(space_guid_dataset)
     end

diff --git a/lib/cloud_controller/permissions.rb b/lib/cloud_controller/permissions.rb
@@ -114,6 +114,14 @@ def readable_org_guids
     readable_org_guids_query.all.map(&:guid)
   end
 
+  def readable_orgs
+    if can_read_globally?
+      VCAP::CloudController::Organization.select(:id, :guid).all
+    else
+      membership.orgs_for_roles_subquery(ROLES_FOR_ORG_READING).all
+    end
+  end
+
   def readable_org_guids_query
     if can_read_globally?
       VCAP::CloudController::Organization.select(:guid)
@@ -241,6 +249,14 @@ def readable_space_scoped_space_guids
     end
   end
 
+  def readable_space_scoped_spaces
+    if can_read_globally?
+      VCAP::CloudController::Space.select(:id, :guid).all
+    else
+      membership.spaces_for_roles_subquery(SPACE_ROLES).all
+    end
+  end
+
   def can_read_route?(space_guid, org_guid)
     return true if can_read_globally?
 

diff --git a/spec/unit/fetchers/service_offering_list_fetcher_spec.rb b/spec/unit/fetchers/service_offering_list_fetcher_spec.rb
@@ -65,12 +65,12 @@ module VCAP::CloudController
           end
         end
 
-        context 'when `readable_org_guids` are specified' do
+        context 'when `readable_orgs` are specified' do
           it 'includes public plans and ones for those orgs' do
             service_offerings = fetcher.fetch(
               message,
-              readable_org_guids: [org_1.guid, org_3.guid],
-              readable_space_guids: [],
+              readable_orgs: [org_1, org_3],
+              readable_spaces: [],
             ).all
 
             expect(service_offerings).to contain_exactly(
@@ -83,12 +83,12 @@ module VCAP::CloudController
           end
         end
 
-        context 'when both `readable_space_guids` and `readable_org_guids` are specified' do
+        context 'when both `readable_spaces` and `readable_orgs` are specified' do
           it 'includes public plans, ones for those spaces and ones for those orgs' do
             service_offerings = fetcher.fetch(
               message,
-              readable_space_guids: [space_3.guid],
-              readable_org_guids: [org_3.guid],
+              readable_spaces: [space_3],
+              readable_orgs: [org_3],
             ).all
 
             expect(service_offerings).to contain_exactly(
@@ -152,8 +152,8 @@ module VCAP::CloudController
 
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid],
-                readable_space_guids: [],
+                readable_orgs: [org_1],
+                readable_spaces: [],
               ).all
 
               expect(service_offerings).to contain_exactly(org_restricted_offering_1, public_offering)
@@ -168,8 +168,8 @@ module VCAP::CloudController
 
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_org_guids: [],
-                readable_space_guids: [],
+                readable_orgs: [],
+                readable_spaces: [],
               ).all
 
               expect(service_offerings).to contain_exactly(public_offering)
@@ -187,8 +187,8 @@ module VCAP::CloudController
 
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid],
-                readable_space_guids: [space_1.guid],
+                readable_orgs: [org_1],
+                readable_spaces: [space_1],
               ).all
 
               expect(service_offerings).to contain_exactly(public_offering, org_restricted_offering_1, space_scoped_offering_1)
@@ -222,8 +222,8 @@ module VCAP::CloudController
               }.with_indifferent_access)
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid],
-                readable_space_guids: [space_1.guid],
+                readable_orgs: [org_1],
+                readable_spaces: [space_1],
               ).all
               expect(service_offerings).to contain_exactly(space_scoped_offering_1, org_restricted_offering_1, public_offering)
             end
@@ -236,8 +236,8 @@ module VCAP::CloudController
               }.with_indifferent_access)
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid, org_2.guid],
-                readable_space_guids: [space_1.guid],
+                readable_orgs: [org_1, org_2],
+                readable_spaces: [space_1],
               ).all
               expect(service_offerings).to contain_exactly(space_scoped_offering_1, org_restricted_offering_1, public_offering)
             end
@@ -251,8 +251,8 @@ module VCAP::CloudController
 
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_org_guids: [],
-                readable_space_guids: [],
+                readable_orgs: [],
+                readable_spaces: [],
               ).all
 
               expect(service_offerings).to contain_exactly(public_offering)
@@ -289,8 +289,8 @@ module VCAP::CloudController
               }.with_indifferent_access)
               service_offerings = ServiceOfferingListFetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid, org_2.guid],
-                readable_space_guids: [space_1.guid],
+                readable_orgs: [org_1, org_2],
+                readable_spaces: [space_1],
               ).all
               expect(service_offerings).to contain_exactly(public_offering)
             end

diff --git a/spec/unit/fetchers/service_plan_list_fetcher_spec.rb b/spec/unit/fetchers/service_plan_list_fetcher_spec.rb
@@ -78,12 +78,12 @@ module VCAP::CloudController
           end
         end
 
-        context 'when `readable_org_guids` are specified' do
+        context 'when `readable_org` are specified' do
           it 'includes public plans and ones for those orgs' do
             service_plans = fetcher.fetch(
               message,
-              readable_org_guids: [org_1.guid, org_3.guid],
-              readable_space_guids: [],
+              readable_orgs: [org_1, org_3],
+              readable_spaces: [],
             ).all
 
             expect(service_plans).to contain_exactly(
@@ -96,12 +96,12 @@ module VCAP::CloudController
           end
         end
 
-        context 'when both `readable_space_guids` and `readable_org_guids` are specified' do
+        context 'when both `readable_spaces` and `readable_orgs` are specified' do
           it 'includes public plans, ones for those spaces and ones for those orgs' do
             service_plans = fetcher.fetch(
               message,
-              readable_space_guids: [space_3.guid],
-              readable_org_guids: [org_3.guid],
+              readable_spaces: [space_3],
+              readable_orgs: [org_3],
             ).all
 
             expect(service_plans).to contain_exactly(
@@ -165,8 +165,8 @@ module VCAP::CloudController
 
               service_plans = fetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid],
-                readable_space_guids: [],
+                readable_orgs: [org_1],
+                readable_spaces: [],
               ).all
 
               expect(service_plans).to contain_exactly(org_restricted_plan_1, public_plan)
@@ -181,8 +181,8 @@ module VCAP::CloudController
 
               service_plans = fetcher.fetch(
                 message,
-                readable_org_guids: [],
-                readable_space_guids: [],
+                readable_orgs: [],
+                readable_spaces: [],
               ).all
 
               expect(service_plans).to contain_exactly(public_plan)
@@ -200,8 +200,8 @@ module VCAP::CloudController
 
               service_plans = fetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid],
-                readable_space_guids: [space_1.guid],
+                readable_orgs: [org_1],
+                readable_spaces: [space_1],
               ).all
 
               expect(service_plans).to contain_exactly(public_plan, org_restricted_plan_1, space_scoped_plan_1)
@@ -235,8 +235,8 @@ module VCAP::CloudController
               }.with_indifferent_access)
               service_plans = fetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid],
-                readable_space_guids: [space_1.guid],
+                readable_orgs: [org_1],
+                readable_spaces: [space_1],
               ).all
               expect(service_plans).to contain_exactly(space_scoped_plan_1, org_restricted_plan_1, public_plan)
             end
@@ -249,8 +249,8 @@ module VCAP::CloudController
               }.with_indifferent_access)
               service_plans = fetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid, org_2.guid],
-                readable_space_guids: [space_1.guid],
+                readable_orgs: [org_1, org_2],
+                readable_spaces: [space_1],
               ).all
               expect(service_plans).to contain_exactly(space_scoped_plan_1, org_restricted_plan_1, public_plan)
             end
@@ -264,8 +264,8 @@ module VCAP::CloudController
 
               service_plans = fetcher.fetch(
                 message,
-                readable_org_guids: [],
-                readable_space_guids: [],
+                readable_orgs: [],
+                readable_spaces: [],
               ).all
 
               expect(service_plans).to contain_exactly(public_plan)
@@ -302,8 +302,8 @@ module VCAP::CloudController
               }.with_indifferent_access)
               service_plans = fetcher.fetch(
                 message,
-                readable_org_guids: [org_1.guid, org_2.guid],
-                readable_space_guids: [space_1.guid],
+                readable_orgs: [org_1, org_2],
+                readable_spaces: [space_1],
               ).all
               expect(service_plans).to contain_exactly(public_plan)
             end
@@ -464,7 +464,7 @@ module VCAP::CloudController
         context 'when org user' do
           let(:org_1) { Organization.make }
           let(:space_1) { Space.make(organization: org_1) }
-          let(:service_plans) { fetcher.fetch(message, omniscient: false, readable_space_guids: [space_1.guid], readable_org_guids: [org_1.guid]).all }
+          let(:service_plans) { fetcher.fetch(message, omniscient: false, readable_spaces: [space_1], readable_orgs: [org_1]).all }
 
           it_behaves_like 'filtered service plans fetcher'
         end