Rewrite permission queries in domain_fetcher and route_fetcher to use UNION ALL

app/controllers/v3/routes_controller.rb

@@ -37,6 +37,7 @@ def index
               else
                 RouteFetcher.fetch(
                   message,
+                  readable_space_ids_dataset: permission_queryer.space_ids_with_readable_routes_query,
                   readable_space_guids_dataset: permission_queryer.space_guids_with_readable_routes_query,
                   eager_loaded_associations: Presenters::V3::RoutePresenter.associated_resources
                 )
@@ -311,6 +312,7 @@ def index_by_app
               else
                 RouteFetcher.fetch(
                   message,
+                  readable_space_ids_dataset: permission_queryer.readable_space_ids_query,
                   readable_space_guids_dataset: permission_queryer.readable_space_guids_query,
                   eager_loaded_associations: Presenters::V3::RoutePresenter.associated_resources
                 )

app/fetchers/domain_fetcher.rb

@@ -4,22 +4,17 @@ module VCAP::CloudController
   class DomainFetcher < BaseListFetcher
     class << self
       def fetch_all_for_orgs(readable_org_ids)
-        # Q: The "Domain" in Domain.dataset is arbitrary -- just a way to get access to any database table.
-        # If there's a way to use a more generic way to access the database, maybe this can be revised.
-        #
-        readable_orgs_filter = Domain.dataset.db[:organizations].where(id: readable_org_ids).select(:id)
-
-        readable_shared_private_domains_filter = Domain.dataset.db[:organizations_private_domains].where(
-          organization_id: readable_orgs_filter
-        ).select(:private_domain_id)
-
-        user_visible_domains = Sequel.or([
-          Domain::SHARED_DOMAIN_CONDITION.flatten,
-          [:owning_organization_id, readable_orgs_filter],
-          [:id, readable_shared_private_domains_filter]
-        ]).sql_boolean
-
-        Domain.where(user_visible_domains).qualify
+        shared_domain_ids = Domain.where(owning_organization_id: nil).select(:id)
+        owned_domain_ids  = Domain.where(owning_organization_id: readable_org_ids).select(:id)
+        shared_private_domain_ids = Domain.dataset.db[:organizations_private_domains].where(
+          organization_id: readable_org_ids
+        ).select(Sequel[:private_domain_id].as(:id))
+
+        all_domain_ids = shared_domain_ids.
+                         union(owned_domain_ids, all: true, from_self: false).
+                         union(shared_private_domain_ids, all: true, from_self: false)
+
+        Domain.where(id: all_domain_ids).qualify
       end
 
       def fetch(message, readable_org_ids)

app/fetchers/route_fetcher.rb

@@ -3,23 +3,57 @@
 module VCAP::CloudController
   class RouteFetcher < BaseListFetcher
     class << self
-      def fetch(message, readable_space_guids_dataset: nil, eager_loaded_associations: [], omniscient: false)
+      def fetch(message, readable_space_guids_dataset: nil, readable_space_ids_dataset: nil, eager_loaded_associations: [], omniscient: false)
         dataset = Route.dataset.eager(eager_loaded_associations).
-                  join(:spaces, id: Sequel[:routes][:space_id]).
-                  left_join(:route_shares, route_guid: Sequel[:routes][:guid]).qualify
+                  join(:spaces, id: Sequel[:routes][:space_id]).qualify
 
         unless omniscient
-          dataset = dataset.where do
-            (Sequel[:spaces][:guid] =~ readable_space_guids_dataset) |
-              (Sequel[:route_shares][:target_space_guid] =~ readable_space_guids_dataset)
-          end
+          dataset = dataset.where(Sequel[:routes][:guid] => accessible_route_guids_dataset(
+            readable_space_ids_dataset: readable_space_ids_dataset,
+            readable_space_guids_dataset: readable_space_guids_dataset
+          ))
         end
-        dataset = dataset.distinct(Sequel[:routes][:guid])
         filter(message, dataset)
       end
 
       private
 
+      def accessible_route_guids_dataset(readable_space_ids_dataset:, readable_space_guids_dataset:)
+        raise ArgumentError.new('readable space ids or guids dataset required') unless readable_space_ids_dataset || readable_space_guids_dataset
+
+        owned_space_column = readable_space_ids_dataset ? :id : :guid
+        owned_space_values = readable_space_ids_dataset || readable_space_guids_dataset
+        shared_space_guids = readable_space_guids_dataset || Space.where(id: readable_space_ids_dataset).select(:guid)
+
+        route_guids_dataset(
+          owned_space_column: owned_space_column,
+          owned_space_values: owned_space_values,
+          shared_space_guids: shared_space_guids
+        )
+      end
+
+      def route_guids_for_space_guids_dataset(space_guids)
+        route_guids_dataset(
+          owned_space_column: :guid,
+          owned_space_values: space_guids,
+          shared_space_guids: space_guids
+        )
+      end
+
+      def route_guids_dataset(owned_space_column:, owned_space_values:, shared_space_guids:)
+        owned_route_guids = Route.dataset.
+                            join(:spaces, id: Sequel[:routes][:space_id]).
+                            where(Sequel[:spaces][owned_space_column] =~ owned_space_values).
+                            select(Sequel[:routes][:guid])
+
+        shared_route_guids = Route.dataset.
+                             join(:route_shares, route_guid: Sequel[:routes][:guid]).
+                             where(Sequel[:route_shares][:target_space_guid] =~ shared_space_guids).
+                             select(Sequel[:routes][:guid])
+
+        owned_route_guids.union(shared_route_guids, all: true, from_self: false)
+      end
+
       def filter(message, dataset)
         dataset = dataset.where(host: message.hosts) if message.requested?(:hosts)
 
@@ -59,12 +93,7 @@ def filter(message, dataset)
           )
         end
 
-        if message.requested?(:space_guids)
-          dataset = dataset.where do
-            (Sequel[:spaces][:guid] =~ message.space_guids) |
-              (Sequel[:route_shares][:target_space_guid] =~ message.space_guids)
-          end
-        end
+        dataset = dataset.where(Sequel[:routes][:guid] => route_guids_for_space_guids_dataset(message.space_guids)) if message.requested?(:space_guids)
 
         super(message, dataset, Route)
       end

lib/cloud_controller/permissions.rb

@@ -317,6 +317,12 @@ def space_guids_with_readable_routes_query
     membership.authorized_space_guids_subquery(ROLES_FOR_ROUTE_READING)
   end
 
+  def space_ids_with_readable_routes_query
+    raise 'must not be called for users that can read globally' if can_read_globally?
+
+    membership.authorized_space_ids_subquery(ROLES_FOR_ROUTE_READING)
+  end
+
   def can_read_app_environment_variables?(space_id, org_id)
     can_read_secrets_globally? ||
       membership.role_applies?(ROLES_FOR_APP_ENVIRONMENT_VARIABLES_READING, space_id, org_id)

spec/unit/fetchers/route_fetcher_spec.rb

@@ -37,24 +37,79 @@ module VCAP::CloudController
         end
 
         it 'fetches the routes owned by readable spaces' do
-          dataset = RouteFetcher.fetch(message, readable_space_guids_dataset: Space.where(id: [space1.id]).select(:guid))
+          dataset = RouteFetcher.fetch(
+            message,
+            readable_space_ids_dataset: Space.where(id: [space1.id]).select(:id),
+            readable_space_guids_dataset: Space.where(id: [space1.id]).select(:guid)
+          )
           expect(dataset.all).to contain_exactly(route1, route2)
         end
 
-        it 'fetches the instances shared to readable spaces' do
+        it 'fetches routes shared into readable spaces' do
           space3 = create(:space)
           shared_route = create(:route, space: space3)
           shared_route.add_shared_space(space2)
-          dataset = RouteFetcher.fetch(message, readable_space_guids_dataset: Space.where(id: [space2.id]).select(:guid))
+          dataset = RouteFetcher.fetch(
+            message,
+            readable_space_ids_dataset: Space.where(id: [space2.id]).select(:id),
+            readable_space_guids_dataset: Space.where(id: [space2.id]).select(:guid)
+          )
           expect(dataset.all).to contain_exactly(route3, shared_route)
         end
+
+        it 'fetches the routes owned by readable spaces using space ids dataset' do
+          dataset = RouteFetcher.fetch(
+            message,
+            readable_space_ids_dataset: Space.where(id: [space1.id]).select(:id),
+            readable_space_guids_dataset: Space.where(id: []).select(:guid)
+          )
+          expect(dataset.all).to contain_exactly(route1, route2)
+        end
+
+        it 'fetches the routes owned by readable spaces using only space guids dataset' do
+          dataset = RouteFetcher.fetch(
+            message,
+            readable_space_guids_dataset: Space.where(id: [space1.id]).select(:guid)
+          )
+          expect(dataset.all).to contain_exactly(route1, route2)
+        end
+
+        it 'fetches routes shared to readable spaces using only space ids dataset' do
+          space3 = create(:space)
+          shared_route = create(:route, space: space3)
+          shared_route.add_shared_space(space2)
+
+          dataset = RouteFetcher.fetch(
+            message,
+            readable_space_ids_dataset: Space.where(id: [space2.id]).select(:id)
+          )
+
+          expect(dataset.all).to contain_exactly(route3, shared_route)
+        end
+
+        it 'does not duplicate routes visible through both owned and shared access paths' do
+          route1.add_shared_space(space2)
+
+          dataset = RouteFetcher.fetch(
+            message,
+            readable_space_ids_dataset: Space.where(id: [space1.id, space2.id]).select(:id),
+            readable_space_guids_dataset: Space.where(id: [space1.id, space2.id]).select(:guid)
+          )
+
+          expect(dataset.all).to contain_exactly(route1, route2, route3)
+        end
       end
 
       describe 'eager loading associated resources' do
         let(:routes_filter) { {} }
 
         it 'eager loads the specified resources for the routes' do
-          results = RouteFetcher.fetch(message, readable_space_guids_dataset: Space.where(guid: [space1.guid]).select(:guid), eager_loaded_associations: %i[labels domain]).all
+          results = RouteFetcher.fetch(
+            message,
+            readable_space_ids_dataset: Space.where(guid: [space1.guid]).select(:id),
+            readable_space_guids_dataset: Space.where(guid: [space1.guid]).select(:guid),
+            eager_loaded_associations: %i[labels domain]
+          ).all
 
           expect(results.first.associations.key?(:labels)).to be true
           expect(results.first.associations.key?(:domain)).to be true
@@ -124,6 +179,16 @@ module VCAP::CloudController
             end
           end
 
+          context 'when a route matches through both owned and shared space paths' do
+            let(:routes_filter) { { space_guids: [space1.guid, space2.guid] } }
+
+            it 'returns the route only once' do
+              route1.add_shared_space(space2)
+
+              expect(results.map(&:guid)).to contain_exactly(route1.guid, route2.guid, route3.guid)
+            end
+          end
+
           context 'when there is no matching route' do
             let(:routes_filter) { { space_guids: '???' } }
 

spec/unit/lib/cloud_controller/permissions_spec.rb

@@ -534,6 +534,16 @@ module VCAP::CloudController
       end
     end
 
+    describe '#space_ids_with_readable_routes_query' do
+      it 'returns subquery from membership using ROLES_FOR_ROUTE_READING' 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::ROLES_FOR_ROUTE_READING).and_return(subquery)
+        expect(permissions.space_ids_with_readable_routes_query).to be(subquery)
+      end
+    end
+
     describe '#can_read_route_policy_from_space?' do
       context 'user has no membership' do
         context 'and user is an admin' do