diff --git a/app/controllers/v3/routes_controller.rb b/app/controllers/v3/routes_controller.rb index 8783fe8d049..1de986f35bc 100644 --- a/app/controllers/v3/routes_controller.rb +++ b/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 ) diff --git a/app/fetchers/domain_fetcher.rb b/app/fetchers/domain_fetcher.rb index a0cb6ddf5ba..faac79f9443 100644 --- a/app/fetchers/domain_fetcher.rb +++ b/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) diff --git a/app/fetchers/route_fetcher.rb b/app/fetchers/route_fetcher.rb index 1e572808668..95aa9b0b4e1 100644 --- a/app/fetchers/route_fetcher.rb +++ b/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 diff --git a/lib/cloud_controller/permissions.rb b/lib/cloud_controller/permissions.rb index ac8cf2d3237..56024f72c8e 100644 --- a/lib/cloud_controller/permissions.rb +++ b/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) diff --git a/spec/unit/fetchers/route_fetcher_spec.rb b/spec/unit/fetchers/route_fetcher_spec.rb index d28a0c2dde2..6553e6d1e7c 100644 --- a/spec/unit/fetchers/route_fetcher_spec.rb +++ b/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: '???' } } diff --git a/spec/unit/lib/cloud_controller/permissions_spec.rb b/spec/unit/lib/cloud_controller/permissions_spec.rb index fcde36f9ae9..fa71a56ddc9 100644 --- a/spec/unit/lib/cloud_controller/permissions_spec.rb +++ b/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