Performance fixes for permission queries (#5172)

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).

Author: philippthun

app/controllers/v3/application_controller.rb

@@ -268,8 +268,4 @@ def handle_exception(error)
   def null_coalesce_body
     hashed_params[:body] ||= {}
   end
-
-  def membership
-    @membership ||= Membership.new(current_user)
-  end
 end

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
 

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

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

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 }

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

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)

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

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

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)