diff --git a/app/access/service_instance_access.rb b/app/access/service_instance_access.rb index cd8ab240003..dce57e8faa5 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.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.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.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.has_developer?(context.user) + service_instance.space&.has_developer?(context.user) end def manage_permissions_with_token?(service_instance) @@ -33,8 +33,7 @@ def manage_permissions_with_token?(service_instance) end def read_permissions?(service_instance) - return true if admin_user? || admin_read_only_user? - service_instance.space.has_member?(context.user) || service_instance.space.organization.managers.include?(context.user) + admin_user? || admin_read_only_user? || object_is_visible_to_user?(service_instance, context.user) end def read_permissions_with_token?(service_instance) @@ -43,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.has_developer?(context.user) + service_instance.space&.has_developer?(context.user) end def read_env_with_token?(service_instance) @@ -64,7 +63,7 @@ def allowed?(service_instance) end def purge?(service_instance) - admin_user? || (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/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 b2aea99e2af..b17fbf5078a 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -68,6 +68,10 @@ 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], + [:shared_spaces, user.managed_spaces_dataset], + [:shared_spaces, user.audited_spaces_dataset], + [:shared_spaces, managed_organizations_spaces_dataset(user.managed_organizations_dataset)], ]) end @@ -134,7 +138,7 @@ def credentials_with_serialization alias_method_chain :credentials, 'serialization' def in_suspended_org? - space.in_suspended_org? + space&.in_suspended_org? end def after_create 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/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) } diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 671bcc5b92f..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 @@ -326,5 +334,103 @@ 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 + + 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) + 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 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 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 + end + end end end