From ddc3801c493bbbcffc0a62a34e8336af6cd29677 Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Tue, 10 Sep 2024 12:32:55 +0200 Subject: [PATCH] Filter out space and organization names based on permissions When a user has access to a shared service instance (i.e. read permissions on any of the shared spaces), the guids of all shared spaces are visible, but only those space and organization names the user is allowed to read based on the given roles. Example: DEVELOPER who is space developer in SPACE_1, SPACE_2 and SPACE_3 shared SERVICE_INSTANCE from SPACE_1 to SPACE_2 and SPACE_3. For each space there is a dedicated space auditor (AUDITOR_1, AUDITOR_2 and AUDITOR_3). SPACE_1 ------- DEVELOPER (space developer) AUDITOR_1 (space auditor) SERVICE_INSTANCE SPACE_2 ------- DEVELOPER (space developer) AUDITOR_2 (space auditor) shared SERVICE_INSTANCE SPACE_3 ------- DEVELOPER (space developer) AUDITOR_3 (space auditor) shared SERVICE_INSTANCE Original behavior (before PR cloudfoundry/cloud_controller_ng#3931): - AUDITOR_1 can see SPACE_2.guid + name and SPACE_3.guid + name => SPACE_2.name and SPACE_3.name should not be readable - AUDITOR_2 cannot see shared spaces => shared spaces should be readable - AUDITOR_3 cannot see shared spaces => shared spaces should be readable Changed behavior (with PR cloudfoundry/cloud_controller_ng#3931): - AUDITOR_1 can see SPACE_2.guid + name and SPACE_3.guid + name => SPACE_2.name and SPACE_3.name should not be readable - AUDITOR_2 can see SPACE_2.guid + name and SPACE_3.guid + name => SPACE_3.name should not be readable - AUDITOR_3 can see SPACE_2.guid + name and SPACE_3.guid + name => SPACE_2.name should not be readable New behavior (this change): - AUDITOR_1 can see SPACE_2.guid and SPACE_3.guid - AUDITOR_2 can see SPACE_2.guid + name and SPACE_3.guid - AUDITOR_3 can see SPACE_2.guid and SPACE_3.guid + name --- ...service_instance_organization_decorator.rb | 14 ++++++++++- .../field_service_instance_space_decorator.rb | 13 ++++++++++- spec/request/service_instances_spec.rb | 23 +++++++++++++++++++ ...ce_instance_organization_decorator_spec.rb | 4 ++++ ...d_service_instance_space_decorator_spec.rb | 4 ++++ 5 files changed, 56 insertions(+), 2 deletions(-) diff --git a/app/decorators/field_service_instance_organization_decorator.rb b/app/decorators/field_service_instance_organization_decorator.rb index db1cde83847..d6532bb17ac 100644 --- a/app/decorators/field_service_instance_organization_decorator.rb +++ b/app/decorators/field_service_instance_organization_decorator.rb @@ -14,17 +14,29 @@ def initialize(fields) def decorate(hash, resources) hash[:included] ||= {} + spaces = resources.map { |r| r.try(:space) || r }.uniq orgs = spaces.map(&:organization).uniq + all_orgs_readable, readable_org_guids = permissions(orgs) hash[:included][:organizations] = orgs.sort_by { |o| [o.created_at, o.guid] }.map do |org| org_view = {} - org_view[:name] = org.name if @fields.include?('name') org_view[:guid] = org.guid if @fields.include?('guid') + org_view[:name] = org.name if @fields.include?('name') && (all_orgs_readable || readable_org_guids.include?(org.guid)) org_view end hash end + + private + + # This method is used to check if the user has permissions to read the organizations and display their names. + def permissions(orgs) + permission_queryer = Permissions.new(SecurityContext.current_user) + return [true, nil] if permission_queryer.can_read_globally? + + [false, Organization.where(guid: orgs.map(&:guid)).where(guid: permission_queryer.readable_org_guids_query).select_map(:guid)] + end end end diff --git a/app/decorators/field_service_instance_space_decorator.rb b/app/decorators/field_service_instance_space_decorator.rb index 1eb2f8709c4..4e5544bfbb2 100644 --- a/app/decorators/field_service_instance_space_decorator.rb +++ b/app/decorators/field_service_instance_space_decorator.rb @@ -16,11 +16,12 @@ def decorate(hash, resources) hash[:included] ||= {} spaces = resources.map { |r| r.try(:space) || r }.uniq + all_spaces_readable, readable_space_guids = permissions(spaces) hash[:included][:spaces] = spaces.sort_by { |s| [s.created_at, s.guid] }.map do |space| temp = {} temp[:guid] = space.guid if @fields.include?('guid') - temp[:name] = space.name if @fields.include?('name') + temp[:name] = space.name if @fields.include?('name') && (all_spaces_readable || readable_space_guids.include?(space.guid)) if @fields.include?('relationships.organization') temp[:relationships] = { @@ -36,5 +37,15 @@ def decorate(hash, resources) hash end + + private + + # This method is used to check if the user has permissions to read the spaces and display their names. + def permissions(spaces) + permission_queryer = Permissions.new(SecurityContext.current_user) + return [true, nil] if permission_queryer.can_read_globally? + + [false, Space.where(guid: spaces.map(&:guid)).where(guid: permission_queryer.readable_space_guids_query).select_map(:guid)] + end end end diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index bfba2f55ed5..65561bfe5a9 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -4095,6 +4095,29 @@ def check_filtered_instances(*instances) expect({ included: parsed_response['included'] }).to match_json_response({ included: }) end + context 'when the user can read from the originating space only' do + before do + set_current_user_as_role(role: 'space_developer', org: space.organization, space: space, user: user) + end + + it 'does not include space and organization names of the shared space' do + get "/v3/service_instances/#{instance.guid}/relationships/shared_spaces?fields[space]=name,guid,relationships.organization&fields[space.organization]=name,guid", nil, + user_header + expect(last_response).to have_status_code(200) + + included = { + spaces: [ + { guid: other_space.guid, relationships: { organization: { data: { guid: other_space.organization.guid } } } } + ], + organizations: [ + { guid: other_space.organization.guid } + ] + } + + expect({ included: parsed_response['included'] }).to match_json_response({ included: }) + end + end + it 'fails for invalid resources' do get "/v3/service_instances/#{instance.guid}/relationships/shared_spaces?fields[fruit]=name", nil, admin_headers expect(last_response).to have_status_code(400) diff --git a/spec/unit/decorators/field_service_instance_organization_decorator_spec.rb b/spec/unit/decorators/field_service_instance_organization_decorator_spec.rb index 1acb5659c05..49b6fdf02eb 100644 --- a/spec/unit/decorators/field_service_instance_organization_decorator_spec.rb +++ b/spec/unit/decorators/field_service_instance_organization_decorator_spec.rb @@ -14,6 +14,10 @@ module VCAP::CloudController let(:service_instance_1) { ManagedServiceInstance.make(space: space1) } let(:service_instance_2) { UserProvidedServiceInstance.make(space: space2) } + before do + allow(Permissions).to receive(:new).and_return(double(can_read_globally?: true)) + end + it 'decorated the given hash with orgs names from service instances' do undecorated_hash = { foo: 'bar', included: { monkeys: %w[zach greg] } } decorator = described_class.new({ 'space.organization': %w[name foo] }) diff --git a/spec/unit/decorators/field_service_instance_space_decorator_spec.rb b/spec/unit/decorators/field_service_instance_space_decorator_spec.rb index 5f4d25f84d4..c5995b2d93e 100644 --- a/spec/unit/decorators/field_service_instance_space_decorator_spec.rb +++ b/spec/unit/decorators/field_service_instance_space_decorator_spec.rb @@ -14,6 +14,10 @@ module VCAP::CloudController let(:service_instance_1) { ManagedServiceInstance.make(space: space1) } let(:service_instance_2) { UserProvidedServiceInstance.make(space: space2) } + before do + allow(Permissions).to receive(:new).and_return(double(can_read_globally?: true)) + end + context 'when space guid, name and relationship.organizations are requested' do let(:decorator) { described_class.new({ space: ['relationships.organization', 'guid', 'name'] }) }