From 0974196a0490f71442ba2a4335b32161c4483fef Mon Sep 17 00:00:00 2001 From: Alex Blease Date: Wed, 18 Oct 2017 10:45:12 +0100 Subject: [PATCH 1/7] Add tests for ServiceInstance.user_visibility_filter [#152035378] Signed-off-by: Jen Spinney --- .../models/services/service_instance_spec.rb | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 671bcc5b92f..36d37512c4f 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/spec/unit/models/services/service_instance_spec.rb @@ -326,5 +326,48 @@ module VCAP::CloudController expect(service_instance.to_hash(opts)['credentials']).to eq({ redacted_message: '[PRIVATE DATA HIDDEN]' }) end end + + describe '#user_visibility_filter' do + let(:developer) { make_developer_for_space(service_instance.space) } + let(:auditor) { make_auditor_for_space(service_instance.space) } + let(:user) { make_user_for_space(service_instance.space) } + let(:org_manager) { make_manager_for_org(service_instance.space.organization) } + let(:space_manager) { make_manager_for_space(service_instance.space) } + + context 'when a user is an org manager where the instance was created' do + it 'the service instance is visible' do + filter = ServiceInstance.user_visibility_filter(org_manager) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + + context 'when a user is a space developer in the space the instance was created' do + it 'the service instance is visible' do + filter = ServiceInstance.user_visibility_filter(developer) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + + context 'when a user is a space auditor in the space the instance was created' do + it 'the service instance is visible' do + filter = ServiceInstance.user_visibility_filter(auditor) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + + context 'when a user is a space manager in the space the instance was created' do + it 'the service instance is visible' do + filter = ServiceInstance.user_visibility_filter(space_manager) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + + context 'when a user does not have access to the originating space' do + it 'the service instance is not visible' do + filter = ServiceInstance.user_visibility_filter(user) + expect(ServiceInstance.filter(filter).all.length).to eq(0) + end + end + end end end From d92ffaeca6c9ad8b19399d9991c9c68440b7c678 Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Wed, 18 Oct 2017 10:48:15 +0100 Subject: [PATCH 2/7] Look at shared spaces to determine user visibility of service instances [#152035378] Signed-off-by: Alex Blease --- app/models/services/service_instance.rb | 1 + .../models/services/service_instance_spec.rb | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index b2aea99e2af..62fdf11e168 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -68,6 +68,7 @@ def self.user_visibility_filter(user) [:space, user.spaces_dataset], [:space, user.audited_spaces_dataset], [:space, user.managed_spaces_dataset], + [:shared_spaces, user.spaces_dataset], ]) end diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 36d37512c4f..abd3347eb92 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/spec/unit/models/services/service_instance_spec.rb @@ -368,6 +368,30 @@ module VCAP::CloudController expect(ServiceInstance.filter(filter).all.length).to eq(0) end end + + context 'when the service instance is shared' do + let(:target_space) { VCAP::CloudController::Space.make } + let(:target_space_dev) { make_developer_for_space(target_space) } + let(:target_space_auditor) { make_auditor_for_space(target_space) } + + before do + service_instance.add_shared_space(target_space) + end + + context 'when a user is a space developer in the target space' do + it 'the service instance is visible' do + filter = ServiceInstance.user_visibility_filter(target_space_dev) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + + context 'when a user is a space auditor in the target space' do + it 'the service instance is not visible' do + filter = ServiceInstance.user_visibility_filter(target_space_auditor) + expect(ServiceInstance.filter(filter).all.length).to eq(0) + end + end + end end end end From ec69e2758f19bcb2d65451c94e4dcfe2b868820d Mon Sep 17 00:00:00 2001 From: Alex Blease Date: Wed, 18 Oct 2017 14:53:59 +0100 Subject: [PATCH 3/7] Allow target space devs to view shared service instances /v2/spaces/:guid/services endpoint now includes service instances that have been shared into the given space. [#152035378] Signed-off-by: Jen Spinney --- app/access/service_instance_access.rb | 7 ++++ app/controllers/runtime/spaces_controller.rb | 16 ++++----- app/models/services/service_instance.rb | 2 +- .../runtime/spaces_controller_spec.rb | 33 +++++++++++++++++++ 4 files changed, 48 insertions(+), 10 deletions(-) diff --git a/app/access/service_instance_access.rb b/app/access/service_instance_access.rb index cd8ab240003..07d65219bed 100644 --- a/app/access/service_instance_access.rb +++ b/app/access/service_instance_access.rb @@ -1,6 +1,7 @@ module VCAP::CloudController class ServiceInstanceAccess < BaseAccess def create?(service_instance, params=nil) + return false unless service_instance.space return true if admin_user? FeatureFlag.raise_unless_enabled!(:service_instance_creation) return false if service_instance.in_suspended_org? @@ -8,6 +9,7 @@ def create?(service_instance, params=nil) end def read_for_update?(service_instance, params=nil) + return false unless service_instance.space return true if admin_user? return false if service_instance.in_suspended_org? service_instance.space.has_developer?(context.user) @@ -18,12 +20,14 @@ def update?(service_instance, params=nil) end def delete?(service_instance) + return false unless service_instance.space return true if admin_user? return false if service_instance.in_suspended_org? service_instance.space.has_developer?(context.user) end def manage_permissions?(service_instance) + return false unless service_instance.space return true if admin_user? service_instance.space.has_developer?(context.user) end @@ -33,6 +37,7 @@ def manage_permissions_with_token?(service_instance) end def read_permissions?(service_instance) + return false unless service_instance.space return true if admin_user? || admin_read_only_user? service_instance.space.has_member?(context.user) || service_instance.space.organization.managers.include?(context.user) end @@ -42,6 +47,7 @@ def read_permissions_with_token?(service_instance) end def read_env?(service_instance) + return false unless service_instance.space return true if admin_user? || admin_read_only_user? service_instance.space.has_developer?(context.user) end @@ -64,6 +70,7 @@ def allowed?(service_instance) end def purge?(service_instance) + return false unless service_instance.space admin_user? || (service_instance.space.has_developer?(context.user) && service_instance.service_broker.private?) end diff --git a/app/controllers/runtime/spaces_controller.rb b/app/controllers/runtime/spaces_controller.rb index 7132a96abb6..d7e8780036c 100644 --- a/app/controllers/runtime/spaces_controller.rb +++ b/app/controllers/runtime/spaces_controller.rb @@ -138,20 +138,18 @@ def enumerate_services(guid) def enumerate_service_instances(guid) space = find_guid_and_validate_access(:read, guid) - if params['return_user_provided_service_instances'] == 'true' - model_class = ServiceInstance - relation_name = :service_instances - else - model_class = ManagedServiceInstance - relation_name = :managed_service_instances - end + model_class = params['return_user_provided_service_instances'] == 'true' ? ServiceInstance : ManagedServiceInstance service_instances = Query.filtered_dataset_from_query_params( model_class, - space.user_visible_relationship_dataset(relation_name, @access_context.user, @access_context.admin_override), + model_class.user_visible(@access_context.user, @access_context.admin_override), ServiceInstancesController.query_parameters, @opts) - service_instances.filter(space: space) + + service_instances = service_instances.filter(Sequel.or([ + [:space, space], + [:shared_spaces, space] + ])) collection_renderer.render_json( ServiceInstancesController, diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index 62fdf11e168..6f8c65613f0 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -135,7 +135,7 @@ def credentials_with_serialization alias_method_chain :credentials, 'serialization' def in_suspended_org? - space.in_suspended_org? + space && space.in_suspended_org? end def after_create diff --git a/spec/unit/controllers/runtime/spaces_controller_spec.rb b/spec/unit/controllers/runtime/spaces_controller_spec.rb index 7477158ad49..e407cf5af60 100644 --- a/spec/unit/controllers/runtime/spaces_controller_spec.rb +++ b/spec/unit/controllers/runtime/spaces_controller_spec.rb @@ -240,6 +240,39 @@ def decoded_guids end end + describe 'shared service instances' do + context 'when a service instance has been shared from another space' do + let(:shared_service_instance) { ManagedServiceInstance.make(space: Space.make) } + + before do + shared_service_instance.add_shared_space(space) + end + + it 'returns the shared service instance' do + get "v2/spaces/#{space.guid}/service_instances" + + guids = decoded_response.fetch('resources').map { |service| service.fetch('metadata').fetch('guid') } + expect(guids).to include(shared_service_instance.guid) + end + end + + context 'when a service instance has been shared between two spaces that are not the queried space' do + let(:other_space) { make_space_for_user(developer) } + let(:irrelevant_shared_service_instance) { ManagedServiceInstance.make(space: Space.make) } + + before do + irrelevant_shared_service_instance.add_shared_space(other_space) + end + + it 'does not return the irrelevant shared service instance' do + get "v2/spaces/#{space.guid}/service_instances" + + guids = decoded_response.fetch('resources').map { |service| service.fetch('metadata').fetch('guid') } + expect(guids).not_to include(irrelevant_shared_service_instance.guid) + end + end + end + context 'when there are provided service instances' do let!(:user_provided_service_instance) { UserProvidedServiceInstance.make(space: space) } let!(:managed_service_instance) { ManagedServiceInstance.make(space: space) } From a1449c43cfb6962b38fcad7fcfd0fcb8849d13ea Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Thu, 19 Oct 2017 16:24:24 +0100 Subject: [PATCH 4/7] Read permissions on service instance should be the same as read * The only visible change is that the global auditor role now receives "true" for read when calling /v2/service_instances/:guid/permissions. This tells the service broker that the user is allowed to view the service instance dashboard. They already have read access to the service instance, so we don't anticipate that this is a problem. [#152035378] Signed-off-by: Derik Evangelista --- app/access/service_instance_access.rb | 4 +--- .../controllers/services/service_instances_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/access/service_instance_access.rb b/app/access/service_instance_access.rb index 07d65219bed..562919072cc 100644 --- a/app/access/service_instance_access.rb +++ b/app/access/service_instance_access.rb @@ -37,9 +37,7 @@ def manage_permissions_with_token?(service_instance) end def read_permissions?(service_instance) - return false unless service_instance.space - return true if admin_user? || admin_read_only_user? - service_instance.space.has_member?(context.user) || service_instance.space.organization.managers.include?(context.user) + read?(service_instance) end def read_permissions_with_token?(service_instance) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 6f6607fa9e2..157eb768ec9 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -3404,7 +3404,7 @@ def verify_forbidden(user) 'org_manager' => { manage: false, read: true }, 'admin' => { manage: true, read: true }, 'admin_read_only' => { manage: false, read: true }, - 'global_auditor' => { manage: false, read: false }, + 'global_auditor' => { manage: false, read: true }, }.each do |role, expected_return_values| context "as an #{role}" do before do From 03e1f83d78a1179181116aa37b11d61d88665733 Mon Sep 17 00:00:00 2001 From: Derik Evangelista Date: Fri, 20 Oct 2017 10:41:36 +0100 Subject: [PATCH 5/7] Allow target space auditors, managers and org managers to view shared spaces * Also cleaned up some of the null checking in service_instance_acess and added tests for access permissions on shared service instances. [#152035378] Signed-off-by: Jen Spinney --- app/access/service_instance_access.rb | 18 ++---- app/models/services/service_instance.rb | 3 + .../access/service_instance_access_spec.rb | 57 +++++++++++++++++++ .../models/services/service_instance_spec.rb | 41 ++++++++++++- 4 files changed, 106 insertions(+), 13 deletions(-) diff --git a/app/access/service_instance_access.rb b/app/access/service_instance_access.rb index 562919072cc..085de94693a 100644 --- a/app/access/service_instance_access.rb +++ b/app/access/service_instance_access.rb @@ -1,18 +1,16 @@ module VCAP::CloudController class ServiceInstanceAccess < BaseAccess def create?(service_instance, params=nil) - return false unless service_instance.space return true if admin_user? FeatureFlag.raise_unless_enabled!(:service_instance_creation) return false if service_instance.in_suspended_org? - service_instance.space.has_developer?(context.user) && allowed?(service_instance) + service_instance.space && service_instance.space.has_developer?(context.user) && allowed?(service_instance) end def read_for_update?(service_instance, params=nil) - return false unless service_instance.space return true if admin_user? return false if service_instance.in_suspended_org? - service_instance.space.has_developer?(context.user) + service_instance.space && service_instance.space.has_developer?(context.user) end def update?(service_instance, params=nil) @@ -20,16 +18,14 @@ def update?(service_instance, params=nil) end def delete?(service_instance) - return false unless service_instance.space return true if admin_user? return false if service_instance.in_suspended_org? - service_instance.space.has_developer?(context.user) + service_instance.space && service_instance.space.has_developer?(context.user) end def manage_permissions?(service_instance) - return false unless service_instance.space return true if admin_user? - service_instance.space.has_developer?(context.user) + service_instance.space && service_instance.space.has_developer?(context.user) end def manage_permissions_with_token?(service_instance) @@ -45,9 +41,8 @@ def read_permissions_with_token?(service_instance) end def read_env?(service_instance) - return false unless service_instance.space return true if admin_user? || admin_read_only_user? - service_instance.space.has_developer?(context.user) + service_instance.space && service_instance.space.has_developer?(context.user) end def read_env_with_token?(service_instance) @@ -68,8 +63,7 @@ def allowed?(service_instance) end def purge?(service_instance) - return false unless service_instance.space - admin_user? || (service_instance.space.has_developer?(context.user) && service_instance.service_broker.private?) + admin_user? || (service_instance.space && service_instance.space.has_developer?(context.user) && service_instance.service_broker.private?) end def purge_with_token?(instance) diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index 6f8c65613f0..cb3d9d4ec56 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -69,6 +69,9 @@ def self.user_visibility_filter(user) [:space, user.audited_spaces_dataset], [:space, user.managed_spaces_dataset], [:shared_spaces, user.spaces_dataset], + [:shared_spaces, user.managed_spaces_dataset], + [:shared_spaces, user.audited_spaces_dataset], + [:shared_spaces, managed_organizations_spaces_dataset(user.managed_organizations_dataset)], ]) end diff --git a/spec/unit/access/service_instance_access_spec.rb b/spec/unit/access/service_instance_access_spec.rb index 1361e7236d4..7ed67887254 100644 --- a/spec/unit/access/service_instance_access_spec.rb +++ b/spec/unit/access/service_instance_access_spec.rb @@ -149,6 +149,63 @@ module VCAP::CloudController end end + context 'space developer in a space that the service instance has been shared into' do + before do + org.add_user(user) + target_space = VCAP::CloudController::Space.make(organization: org) + target_space.add_developer(user) + service_instance.add_shared_space(target_space) + end + + context 'when the space of the service instance is visible' do + it_behaves_like :read_only_access do + let(:object) { service_instance } + end + + it 'does NOT allow the user to have manage permissions of the service instance' do + expect(subject).to_not allow_op_on_object(:manage_permissions, service_instance) + end + + it 'allows the user to have read permissions of the service instance' do + expect(subject).to allow_op_on_object(:read_permissions, service_instance) + end + + it 'does NOT allow the user to read default credentials of the service instance' do + expect(subject).not_to allow_op_on_object(:read_env, service_instance) + end + + it 'returns false for purge' do + expect(subject).not_to allow_op_on_object(:purge, service_instance) + end + end + + context 'when the space of the service instance is not visible' do + before do + service_instance.space = nil + end + + it_behaves_like :read_only_access do + let(:object) { service_instance } + end + + it 'does NOT allow the user to have manage permissions of the service instance' do + expect(subject).to_not allow_op_on_object(:manage_permissions, service_instance) + end + + it 'allows the user to have read permissions of the service instance' do + expect(subject).to allow_op_on_object(:read_permissions, service_instance) + end + + it 'does NOT allow the user to read default credentials of the service instance' do + expect(subject).not_to allow_op_on_object(:read_env, service_instance) + end + + it 'returns false for purge' do + expect(subject).not_to allow_op_on_object(:purge, service_instance) + end + end + end + context 'organization manager (defensive)' do before { org.add_manager(user) } diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index abd3347eb92..29b5cf92b7e 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/spec/unit/models/services/service_instance_spec.rb @@ -298,6 +298,14 @@ module VCAP::CloudController expect(service_instance).not_to be_in_suspended_org end end + + context 'when the service instance space is not visible' do + let(:space) { nil } + + it 'is false' do + expect(service_instance).not_to be_in_suspended_org + end + end end describe '#to_hash' do @@ -372,7 +380,10 @@ module VCAP::CloudController context 'when the service instance is shared' do let(:target_space) { VCAP::CloudController::Space.make } let(:target_space_dev) { make_developer_for_space(target_space) } + let(:target_org_user) { make_user_for_org(target_space.organization) } let(:target_space_auditor) { make_auditor_for_space(target_space) } + let(:target_space_manager) { make_manager_for_space(target_space) } + let(:target_space_org_manager) { make_manager_for_org(target_space.organization) } before do service_instance.add_shared_space(target_space) @@ -385,9 +396,37 @@ module VCAP::CloudController end end + context 'when a user is a space developer in the source space' do + it 'the service instance is visible' do + filter = ServiceInstance.user_visibility_filter(developer) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + context 'when a user is a space auditor in the target space' do - it 'the service instance is not visible' do + it 'the service instance is visible' do filter = ServiceInstance.user_visibility_filter(target_space_auditor) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + + context 'when a user is a space manager in the target space' do + it 'the service instance is visible' do + filter = ServiceInstance.user_visibility_filter(target_space_manager) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + + context 'when a user is a org manager in the target space' do + it 'the service instance is visible' do + filter = ServiceInstance.user_visibility_filter(target_space_org_manager) + expect(ServiceInstance.filter(filter).all.length).to eq(1) + end + end + + context 'when a user does not have access to the target space' do + it 'the service instance is not visible' do + filter = ServiceInstance.user_visibility_filter(target_org_user) expect(ServiceInstance.filter(filter).all.length).to eq(0) end end From 8e0cfde32a9bfb5c1ded9ec7551e90f5a34285e4 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Wed, 1 Nov 2017 11:18:07 +0000 Subject: [PATCH 6/7] Use &. operator instead of service_instance && service_instance.space [#152035378] Signed-off-by: Jen Spinney --- app/access/service_instance_access.rb | 12 ++++++------ app/models/services/service_instance.rb | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/access/service_instance_access.rb b/app/access/service_instance_access.rb index 085de94693a..65179d795b8 100644 --- a/app/access/service_instance_access.rb +++ b/app/access/service_instance_access.rb @@ -4,13 +4,13 @@ def create?(service_instance, params=nil) return true if admin_user? FeatureFlag.raise_unless_enabled!(:service_instance_creation) return false if service_instance.in_suspended_org? - service_instance.space && service_instance.space.has_developer?(context.user) && allowed?(service_instance) + service_instance.space&.has_developer?(context.user) && allowed?(service_instance) end def read_for_update?(service_instance, params=nil) return true if admin_user? return false if service_instance.in_suspended_org? - service_instance.space && service_instance.space.has_developer?(context.user) + service_instance.space&.has_developer?(context.user) end def update?(service_instance, params=nil) @@ -20,12 +20,12 @@ def update?(service_instance, params=nil) def delete?(service_instance) return true if admin_user? return false if service_instance.in_suspended_org? - service_instance.space && service_instance.space.has_developer?(context.user) + service_instance.space&.has_developer?(context.user) end def manage_permissions?(service_instance) return true if admin_user? - service_instance.space && service_instance.space.has_developer?(context.user) + service_instance.space&.has_developer?(context.user) end def manage_permissions_with_token?(service_instance) @@ -42,7 +42,7 @@ def read_permissions_with_token?(service_instance) def read_env?(service_instance) return true if admin_user? || admin_read_only_user? - service_instance.space && service_instance.space.has_developer?(context.user) + service_instance.space&.has_developer?(context.user) end def read_env_with_token?(service_instance) @@ -63,7 +63,7 @@ def allowed?(service_instance) end def purge?(service_instance) - admin_user? || (service_instance.space && service_instance.space.has_developer?(context.user) && service_instance.service_broker.private?) + admin_user? || (service_instance.space&.has_developer?(context.user) && service_instance.service_broker.private?) end def purge_with_token?(instance) diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index cb3d9d4ec56..b17fbf5078a 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -138,7 +138,7 @@ def credentials_with_serialization alias_method_chain :credentials, 'serialization' def in_suspended_org? - space && space.in_suspended_org? + space&.in_suspended_org? end def after_create From a72880e90d583d56c5eac897a11f6610e932fd30 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Tue, 7 Nov 2017 11:21:55 +0000 Subject: [PATCH 7/7] Remove global auditors from read_permissions access [#152631507] Signed-off-by: Denise Yu --- app/access/service_instance_access.rb | 2 +- .../controllers/services/service_instances_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/access/service_instance_access.rb b/app/access/service_instance_access.rb index 65179d795b8..dce57e8faa5 100644 --- a/app/access/service_instance_access.rb +++ b/app/access/service_instance_access.rb @@ -33,7 +33,7 @@ def manage_permissions_with_token?(service_instance) end def read_permissions?(service_instance) - read?(service_instance) + admin_user? || admin_read_only_user? || object_is_visible_to_user?(service_instance, context.user) end def read_permissions_with_token?(service_instance) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 157eb768ec9..6f6607fa9e2 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -3404,7 +3404,7 @@ def verify_forbidden(user) 'org_manager' => { manage: false, read: true }, 'admin' => { manage: true, read: true }, 'admin_read_only' => { manage: false, read: true }, - 'global_auditor' => { manage: false, read: true }, + 'global_auditor' => { manage: false, read: false }, }.each do |role, expected_return_values| context "as an #{role}" do before do