Remove joins to space and org tables for permissions

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

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
 

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
 

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

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

lib/cloud_controller/membership.rb

@@ -17,40 +17,48 @@ def initialize(user)
     end
 
     def has_any_roles?(roles, space_guid=nil, org_guid=nil)
-      space_ids = Space.where(guid: space_guid).select(:id)
-      return true if SpaceRole.where(type: space_roles(roles), user_id: @user.id, space_id: space_ids).any?
+      if space_guid && space_role?(roles)
+        space_id = Space.where(guid: space_guid).select(:id)
+        return true unless SpaceRole.where(type: space_roles(roles), user_id: @user.id, space_id: space_id).empty?
+      end
 
-      org_ids = Organization.where(guid: org_guid).select(:id)
-      OrganizationRole.where(type: org_roles(roles), user_id: @user.id, organization_id: org_ids).any?
+      if org_guid && org_role?(roles)
+        org_id = Organization.where(guid: org_guid).select(:id)
+        return true unless OrganizationRole.where(type: org_roles(roles), user_id: @user.id, organization_id: org_id).empty?
+      end
+
+      false
     end
 
     def org_guids_for_roles(roles)
       org_guids_for_roles_subquery(roles).all.map(&:guid)
     end
 
     def org_guids_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)
-
-      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)
+      orgs_for_roles_subquery(roles).select(:guid)
+    end
 
-      org_guids_for_space_roles_dataset.union(org_guid_dataset)
+    def orgs_for_roles_subquery(roles)
+      org_ids = org_ids_for_org_roles(roles)
+      space_ids = space_ids_for_space_roles(roles)
+      org_ids_from_space_roles = if space_ids
+                                   Space.where(id: space_ids).select(:organization_id)
+                                 end
+      Organization.where(id: org_ids).or(id: org_ids_from_space_roles).select(:id, :guid)
     end
 
     def space_guids_for_roles(roles)
       space_guids_for_roles_subquery(roles).all.map(&:guid)
     end
 
     def space_guids_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)
-
-      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)
+      spaces_for_roles_subquery(roles).select(:guid)
+    end
 
-      space_guids_for_org_roles_dataset.union(space_guid_dataset)
+    def spaces_for_roles_subquery(roles)
+      space_ids = space_ids_for_space_roles(roles)
+      org_ids = org_ids_for_org_roles(roles)
+      Space.where(id: space_ids).or(organization_id: org_ids).select(:id, :guid)
     end
 
     private
@@ -62,5 +70,25 @@ def space_roles(roles)
     def org_roles(roles)
       Array(roles) & ORG_ROLES
     end
+
+    def space_role?(roles)
+      space_roles(roles).any?
+    end
+
+    def org_role?(roles)
+      org_roles(roles).any?
+    end
+
+    def space_ids_for_space_roles(roles)
+      if space_role?(roles)
+        SpaceRole.where(type: space_roles(roles), user_id: @user.id).select(:space_id)
+      end
+    end
+
+    def org_ids_for_org_roles(roles)
+      if org_role?(roles)
+        OrganizationRole.where(type: org_roles(roles), user_id: @user.id).select(:organization_id)
+      end
+    end
   end
 end

lib/cloud_controller/permissions.rb

@@ -122,6 +122,14 @@ def readable_org_guids_query
     end
   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_for_domains_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?
 

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