From 0974196a0490f71442ba2a4335b32161c4483fef Mon Sep 17 00:00:00 2001 From: Alex Blease Date: Wed, 18 Oct 2017 10:45:12 +0100 Subject: [PATCH 01/32] 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 02/32] 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 03/32] 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 04/32] 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 05/32] 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 06/32] 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 07/32] 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 From 26d1e13b34f5a934aa53d4f91d63870ec8d52e59 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Tue, 24 Oct 2017 17:02:23 +0100 Subject: [PATCH 08/32] Implement /v3/service_instances endpoint * List all service instances that a user has permission to see [#152073155] Signed-off-by: Jen Spinney --- .../v3/service_instances_controller.rb | 19 +++ app/fetchers/service_instance_list_fetcher.rb | 25 ++++ .../service_instances_list_message.rb | 32 +++++ app/presenters/v3/paginated_list_presenter.rb | 4 +- .../v3/service_instance_presenter.rb | 24 ++++ config/routes.rb | 1 + spec/request/service_instances_spec.rb | 112 +++++++++++++++--- .../v3/service_instance_controller_spec.rb | 43 +++++++ .../service_instances_list_message_spec.rb | 60 ++++++++++ .../v3/service_instance_presenter_spec.rb | 20 ++++ .../service_instance_list_fetcher_spec.rb | 74 ++++++++++++ 11 files changed, 396 insertions(+), 18 deletions(-) create mode 100644 app/fetchers/service_instance_list_fetcher.rb create mode 100644 app/messages/service_instances/service_instances_list_message.rb create mode 100644 app/presenters/v3/service_instance_presenter.rb create mode 100644 spec/unit/messages/service_instances_list_message_spec.rb create mode 100644 spec/unit/presenters/v3/service_instance_presenter_spec.rb create mode 100644 spec/unit/queries/service_instance_list_fetcher_spec.rb diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 3f1141b7ae4..1aef6c27de5 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -1,11 +1,30 @@ require 'messages/to_many_relationship_message' +require 'messages/service_instances/service_instances_list_message' require 'presenters/v3/relationship_presenter' require 'presenters/v3/to_many_relationship_presenter' +require 'presenters/v3/paginated_list_presenter' require 'actions/service_instance_share' require 'actions/service_instance_unshare' +require 'fetchers/service_instance_list_fetcher' class ServiceInstancesV3Controller < ApplicationController + def index + message = ServiceInstancesListMessage.from_params(query_params) + invalid_param!(message.errors.full_messages) unless message.valid? + + dataset = if can_read_globally? + ServiceInstanceListFetcher.new.fetch_all(message: message) + else + ServiceInstanceListFetcher.new.fetch(message: message, space_guids: readable_space_guids) + end + + render status: :ok, json: Presenters::V3::PaginatedListPresenter.new( + dataset: dataset, + path: '/v3/service_instances', + message: message) + end + def share_service_instance FeatureFlag.raise_unless_enabled!(:service_instance_sharing) diff --git a/app/fetchers/service_instance_list_fetcher.rb b/app/fetchers/service_instance_list_fetcher.rb new file mode 100644 index 00000000000..84fc0e5e022 --- /dev/null +++ b/app/fetchers/service_instance_list_fetcher.rb @@ -0,0 +1,25 @@ +module VCAP::CloudController + class ServiceInstanceListFetcher + def fetch(message:, space_guids:) + dataset = ServiceInstance.select_all(ServiceInstance.table_name). + join(Space.table_name, id: :space_id, guid: space_guids) + + filter(dataset, message) + end + + def fetch_all(message:) + dataset = ServiceInstance.dataset + filter(dataset, message) + end + + private + + def filter(dataset, message) + if message.requested?(:names) + dataset = dataset.where(service_instances__name: message.names) + end + + dataset + end + end +end diff --git a/app/messages/service_instances/service_instances_list_message.rb b/app/messages/service_instances/service_instances_list_message.rb new file mode 100644 index 00000000000..f368e921fe4 --- /dev/null +++ b/app/messages/service_instances/service_instances_list_message.rb @@ -0,0 +1,32 @@ +require 'messages/list_message' + +module VCAP::CloudController + class ServiceInstancesListMessage < ListMessage + ALLOWED_KEYS = [:page, :per_page, :order_by, :names].freeze + + attr_accessor(*ALLOWED_KEYS) + + validates_with NoAdditionalParamsValidator + validates :names, array: true, allow_nil: true + + def initialize(params={}) + super(params.symbolize_keys) + end + + def self.from_params(params) + opts = params.dup + to_array! opts, 'names' + new(opts.symbolize_keys) + end + + def valid_order_by_values + super << :name + end + + private + + def allowed_keys + ALLOWED_KEYS + end + end +end diff --git a/app/presenters/v3/paginated_list_presenter.rb b/app/presenters/v3/paginated_list_presenter.rb index 48ad1f07a3f..238e18a7729 100644 --- a/app/presenters/v3/paginated_list_presenter.rb +++ b/app/presenters/v3/paginated_list_presenter.rb @@ -6,6 +6,7 @@ require 'presenters/v3/process_presenter' require 'presenters/v3/route_mapping_presenter' require 'presenters/v3/service_binding_presenter' +require 'presenters/v3/service_instance_presenter' require 'presenters/v3/task_presenter' require 'presenters/v3/organization_presenter' require 'presenters/v3/space_presenter' @@ -24,7 +25,8 @@ class PaginatedListPresenter 'PackageModel' => VCAP::CloudController::Presenters::V3::PackagePresenter, 'RouteMappingModel' => VCAP::CloudController::Presenters::V3::RouteMappingPresenter, 'ServiceBinding' => VCAP::CloudController::Presenters::V3::ServiceBindingPresenter, - 'TaskModel' => VCAP::CloudController::Presenters::V3::TaskPresenter, + 'ManagedServiceInstance' => VCAP::CloudController::Presenters::V3::ServiceInstancePresenter, + 'TaskModel' => VCAP::CloudController::Presenters::V3::TaskPresenter, }.freeze def initialize(dataset:, path:, message: nil, show_secrets: false) diff --git a/app/presenters/v3/service_instance_presenter.rb b/app/presenters/v3/service_instance_presenter.rb new file mode 100644 index 00000000000..537d1969e8a --- /dev/null +++ b/app/presenters/v3/service_instance_presenter.rb @@ -0,0 +1,24 @@ +require 'presenters/v3/base_presenter' + +module VCAP::CloudController + module Presenters + module V3 + class ServiceInstancePresenter < BasePresenter + def to_hash + { + guid: service_instance.guid, + created_at: service_instance.created_at, + updated_at: service_instance.updated_at, + name: service_instance.name + } + end + + private + + def service_instance + @resource + end + end + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 729606e0a8a..34661d0e55b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -119,6 +119,7 @@ get '/apps/:app_guid/tasks', to: 'tasks#index' # service_instances + get '/service_instances', to: 'service_instances_v3#index' post '/service_instances/:service_instance_guid/relationships/shared_spaces', to: 'service_instances_v3#share_service_instance' delete '/service_instances/:service_instance_guid/relationships/shared_spaces/:space_guid', to: 'service_instances_v3#unshare_service_instance' end diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index c9e0a68bb80..9279b5d22c5 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -2,11 +2,89 @@ RSpec.describe 'Service Instances' do let(:user_email) { 'user@email.example.com' } - let(:user_name) { 'sharer_username' } + let(:user_name) { 'username' } let(:user) { VCAP::CloudController::User.make } + let(:user_header) { headers_for(user) } let(:admin_header) { admin_headers_for(user, email: user_email, user_name: user_name) } + let(:space) { VCAP::CloudController::Space.make } let(:target_space) { VCAP::CloudController::Space.make } - let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make } + let!(:service_instance1) { VCAP::CloudController::ManagedServiceInstance.make(space: space, name: 'rabbitmq') } + let!(:service_instance2) { VCAP::CloudController::ManagedServiceInstance.make(space: space, name: 'redis') } + let!(:service_instance3) { VCAP::CloudController::ManagedServiceInstance.make(space: space, name: 'mysql') } + + describe 'GET /v3/service_instances' do + it 'returns a paginated list of service instances the user has access to' do + set_current_user_as_role(role: 'space_developer', org: space.organization, space: space, user: user) + get '/v3/service_instances?per_page=2&order_by=name', nil, user_header + expect(last_response.status).to eq(200) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response).to be_a_response_like( + { + 'pagination' => { + 'total_results' => 3, + 'total_pages' => 2, + 'first' => { + 'href' => "#{link_prefix}/v3/service_instances?order_by=name&page=1&per_page=2" + }, + 'last' => { + 'href' => "#{link_prefix}/v3/service_instances?order_by=name&page=2&per_page=2" + }, + 'next' => { + 'href' => "#{link_prefix}/v3/service_instances?order_by=name&page=2&per_page=2" + }, + 'previous' => nil + }, + 'resources' => [ + { + 'guid' => service_instance3.guid, + 'name' => 'mysql', + 'created_at' => iso8601, + 'updated_at' => iso8601, + }, + { + 'guid' => service_instance1.guid, + 'name' => 'rabbitmq', + 'created_at' => iso8601, + 'updated_at' => iso8601, + } + ] + } + ) + end + + it 'returns a paginated list of service instances filtered by name' do + set_current_user_as_role(role: 'space_developer', org: space.organization, space: space, user: user) + get '/v3/service_instances?per_page=2&names=redis', nil, user_header + expect(last_response.status).to eq(200) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response).to be_a_response_like( + { + 'pagination' => { + 'total_results' => 1, + 'total_pages' => 1, + 'first' => { + 'href' => "#{link_prefix}/v3/service_instances?names=redis&page=1&per_page=2" + }, + 'last' => { + 'href' => "#{link_prefix}/v3/service_instances?names=redis&page=1&per_page=2" + }, + 'next' => nil, + 'previous' => nil + }, + 'resources' => [ + { + 'guid' => service_instance2.guid, + 'name' => 'redis', + 'created_at' => iso8601, + 'updated_at' => iso8601, + } + ] + } + ) + end + end describe 'POST /v3/service_instances/:guid/relationships/shared_spaces' do before do @@ -20,7 +98,7 @@ ] } - post "/v3/service_instances/#{service_instance.guid}/relationships/shared_spaces", share_request.to_json, admin_header + post "/v3/service_instances/#{service_instance1.guid}/relationships/shared_spaces", share_request.to_json, admin_header parsed_response = MultiJson.load(last_response.body) expect(last_response.status).to eq(200) @@ -30,8 +108,8 @@ { 'guid' => target_space.guid } ], 'links' => { - 'self' => { 'href' => "#{link_prefix}/v3/service_instances/#{service_instance.guid}/relationships/shared_spaces" }, - 'related' => { 'href' => "#{link_prefix}/v3/service_instances/#{service_instance.guid}/shared_spaces" }, + 'self' => { 'href' => "#{link_prefix}/v3/service_instances/#{service_instance1.guid}/relationships/shared_spaces" }, + 'related' => { 'href' => "#{link_prefix}/v3/service_instances/#{service_instance1.guid}/shared_spaces" }, } } @@ -44,11 +122,11 @@ actor_type: 'user', actor_name: user_email, actor_username: user_name, - actee: service_instance.guid, + actee: service_instance1.guid, actee_type: 'service_instance', - actee_name: service_instance.name, - space_guid: service_instance.space.guid, - organization_guid: service_instance.space.organization.guid + actee_name: service_instance1.name, + space_guid: space.guid, + organization_guid: space.organization.guid }) expect(event.metadata['target_space_guids']).to eq([target_space.guid]) end @@ -68,12 +146,12 @@ ] } - post "/v3/service_instances/#{service_instance.guid}/relationships/shared_spaces", share_request.to_json, admin_header + post "/v3/service_instances/#{service_instance1.guid}/relationships/shared_spaces", share_request.to_json, admin_header expect(last_response.status).to eq(200) end it 'unshares the service instance from the target space' do - delete "/v3/service_instances/#{service_instance.guid}/relationships/shared_spaces/#{target_space.guid}", nil, admin_header + delete "/v3/service_instances/#{service_instance1.guid}/relationships/shared_spaces/#{target_space.guid}", nil, admin_header expect(last_response.status).to eq(204) event = VCAP::CloudController::Event.last @@ -83,23 +161,23 @@ actor_type: 'user', actor_name: user_email, actor_username: user_name, - actee: service_instance.guid, + actee: service_instance1.guid, actee_type: 'service_instance', - actee_name: service_instance.name, - space_guid: service_instance.space.guid, - organization_guid: service_instance.space.organization.guid + actee_name: service_instance1.name, + space_guid: space.guid, + organization_guid: space.organization.guid }) expect(event.metadata['target_space_guid']).to eq(target_space.guid) end it 'deletes associated bindings in target space when service instance is unshared' do process = VCAP::CloudController::ProcessModelFactory.make(diego: false, space: target_space) - service_binding = VCAP::CloudController::ServiceBinding.make(service_instance: service_instance, app: process.app, credentials: { secret: 'key' }) + service_binding = VCAP::CloudController::ServiceBinding.make(service_instance: service_instance1, app: process.app, credentials: { secret: 'key' }) get "/v2/service_bindings/#{service_binding.guid}", nil, admin_header expect(last_response.status).to eq(200) - delete "/v3/service_instances/#{service_instance.guid}/relationships/shared_spaces/#{target_space.guid}", nil, admin_header + delete "/v3/service_instances/#{service_instance1.guid}/relationships/shared_spaces/#{target_space.guid}", nil, admin_header expect(last_response.status).to eq(204) get "/v2/service_bindings/#{service_binding.guid}", nil, admin_header diff --git a/spec/unit/controllers/v3/service_instance_controller_spec.rb b/spec/unit/controllers/v3/service_instance_controller_spec.rb index 16dad33ef32..e1a1b7d7e62 100644 --- a/spec/unit/controllers/v3/service_instance_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instance_controller_spec.rb @@ -2,6 +2,49 @@ RSpec.describe ServiceInstancesV3Controller, type: :controller do let(:user) { set_current_user(VCAP::CloudController::User.make) } + let(:space) { VCAP::CloudController::Space.make } + let!(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space, name: 'awesome service') } + + describe '#index' do + describe 'permissions by role' do + role_to_expected_http_response = { + 'admin' => true, + 'admin_read_only' => true, + 'global_auditor' => true, + 'org_manager' => true, + 'org_auditor' => false, + 'org_billing_manager' => false, + 'space_manager' => true, + 'space_auditor' => true, + 'space_developer' => true, + }.freeze + + role_to_expected_http_response.each do |role, can_see_service_instance| + context "as an #{role}" do + it "#{can_see_service_instance ? 'can' : 'cannot'} see the service instance" do + set_current_user_as_role(role: role, org: space.organization, space: space, user: user) + + expected_service_instance_names = can_see_service_instance ? ['awesome service'] : [] + + get :index + expect(response.status).to eq(200), response.body + expect(parsed_body['resources'].map { |h| h['name'] }).to match_array(expected_service_instance_names) + end + end + end + end + + context 'when a non-supported value is specified' do + it 'a bad query parameter error is returned' do + set_current_user_as_admin + get :index, { order_by: 'banana' } + + expect(response.status).to eq(400) + expect(response.body).to include 'BadQueryParameter' + expect(response.body).to include("Order by can only be: 'created_at', 'updated_at', 'name'") + end + end + end describe '#share_service_instance' do let(:service_instance) { VCAP::CloudController::ServiceInstance.make } diff --git a/spec/unit/messages/service_instances_list_message_spec.rb b/spec/unit/messages/service_instances_list_message_spec.rb new file mode 100644 index 00000000000..1d935b1d602 --- /dev/null +++ b/spec/unit/messages/service_instances_list_message_spec.rb @@ -0,0 +1,60 @@ +require 'spec_helper' +require 'messages/service_instances/service_instances_list_message' + +module VCAP::CloudController + RSpec.describe ServiceInstancesListMessage do + describe '.from_params' do + let(:params) do + { + 'page' => 1, + 'per_page' => 5, + 'order_by' => 'name', + 'names' => 'rabbitmq, redis,mysql' + } + end + + it 'returns the correct ServiceInstancesListMessage' do + message = ServiceInstancesListMessage.from_params(params) + + expect(message).to be_a(ServiceInstancesListMessage) + expect(message.page).to eq(1) + expect(message.per_page).to eq(5) + expect(message.order_by).to eq('name') + expect(message.names).to match_array(['mysql', 'rabbitmq', 'redis']) + end + + it 'converts requested keys to symbols' do + message = ServiceInstancesListMessage.from_params(params) + + expect(message.requested?(:page)).to be_truthy + expect(message.requested?(:per_page)).to be_truthy + expect(message.requested?(:order_by)).to be_truthy + expect(message.requested?(:names)).to be_truthy + end + end + + describe 'fields' do + it 'accepts a set of fields' do + message = ServiceInstancesListMessage.new({ + page: 1, + per_page: 5, + order_by: 'created_at', + names: ['rabbitmq', 'redis'] + }) + expect(message).to be_valid + end + + it 'accepts an empty set' do + message = ServiceInstancesListMessage.new + expect(message).to be_valid + end + + it 'does not accept a field not in this set' do + message = ServiceInstancesListMessage.new({ foobar: 'pants' }) + + expect(message).not_to be_valid + expect(message.errors[:base]).to include("Unknown query parameter(s): 'foobar'") + end + end + end +end diff --git a/spec/unit/presenters/v3/service_instance_presenter_spec.rb b/spec/unit/presenters/v3/service_instance_presenter_spec.rb new file mode 100644 index 00000000000..58c595f90a4 --- /dev/null +++ b/spec/unit/presenters/v3/service_instance_presenter_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' +require 'presenters/v3/service_instance_presenter' + +module VCAP::CloudController::Presenters::V3 + RSpec.describe ServiceInstancePresenter do + let(:presenter) { ServiceInstancePresenter.new(service_instance) } + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(name: 'denise-db') } + + describe '#to_hash' do + let(:result) { presenter.to_hash } + + it 'presents the model as a hash' do + expect(result[:guid]).to eq(service_instance.guid) + expect(result[:created_at]).to eq(service_instance.created_at) + expect(result[:updated_at]).to eq(service_instance.updated_at) + expect(result[:name]).to eq('denise-db') + end + end + end +end diff --git a/spec/unit/queries/service_instance_list_fetcher_spec.rb b/spec/unit/queries/service_instance_list_fetcher_spec.rb new file mode 100644 index 00000000000..92accad7245 --- /dev/null +++ b/spec/unit/queries/service_instance_list_fetcher_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' +require 'fetchers/service_instance_list_fetcher' +require 'messages/service_instances/service_instances_list_message' + +module VCAP::CloudController + RSpec.describe ServiceInstanceListFetcher do + let(:filters) { {} } + let(:message) { ServiceInstancesListMessage.new(filters) } + let(:fetcher) { ServiceInstanceListFetcher.new } + + describe '#fetch_all' do + let!(:service_instance_1) { ManagedServiceInstance.make(name: 'rabbitmq') } + let!(:service_instance_2) { ManagedServiceInstance.make(name: 'redis') } + + it 'returns a Sequel::Dataset' do + results = fetcher.fetch_all(message: message) + expect(results).to be_a(Sequel::Dataset) + end + + it 'includes all the V3 Service Instances' do + results = fetcher.fetch_all(message: message).all + expect(results.length).to eq 2 + expect(results).to include(service_instance_1, service_instance_2) + end + + context 'filter' do + context 'by service instance name' do + let(:filters) { { names: ['rabbitmq'] } } + + it 'only returns matching service instances' do + results = fetcher.fetch_all(message: message).all + expect(results).to match_array([service_instance_1]) + expect(results).not_to include(service_instance_2) + end + end + end + end + + describe '#fetch' do + let!(:service_instance_1) { ManagedServiceInstance.make(name: 'rabbitmq', space: space_1) } + let!(:service_instance_2) { ManagedServiceInstance.make(name: 'redis', space: space_1) } + let!(:service_instance_3) { ManagedServiceInstance.make(name: 'mysql', space: space_2) } + + let(:space_1) { Space.make } + let(:space_2) { Space.make } + + it 'returns all of the service instances in the specified space' do + results = fetcher.fetch(message: message, space_guids: [space_1.guid]).all + + expect(results).to match_array([service_instance_1, service_instance_2]) + end + + context 'filter' do + context 'by service instance name' do + let(:filters) { { names: ['rabbitmq', 'redis'] } } + + it 'only returns matching service instances' do + results = fetcher.fetch(message: message, space_guids: [space_1.guid]).all + expect(results).to match_array([service_instance_1, service_instance_2]) + end + end + + context 'by non-existent service instance name' do + let(:filters) { { names: ['made-up-name'] } } + + it 'returns no matching service instances' do + results = fetcher.fetch(message: message, space_guids: [space_1.guid]).all + expect(results).to be_empty + end + end + end + end + end +end From 021bf4e7e6c19bda419a2524c8ee3a6d9668f24d Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Fri, 27 Oct 2017 10:37:11 +0100 Subject: [PATCH 09/32] Add docs for list V3 service instances [#152073155] Signed-off-by: Alex Blease --- .../api_resources/_service_instances.erb | 25 ++++++++++++++ .../service_instances/_list.md.erb | 34 +++++++++++++++++++ docs/v3/source/index.md | 1 + 3 files changed, 60 insertions(+) create mode 100644 docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb diff --git a/docs/v3/source/includes/api_resources/_service_instances.erb b/docs/v3/source/includes/api_resources/_service_instances.erb index 3868b3df80f..a9a15f49183 100644 --- a/docs/v3/source/includes/api_resources/_service_instances.erb +++ b/docs/v3/source/includes/api_resources/_service_instances.erb @@ -18,3 +18,28 @@ } } <% end %> + +<% content_for :paginated_list_of_service_instances do %> +{ + "pagination": { + "total_results": 1, + "total_pages": 1, + "first": { + "href": "https://api.example.org/v3/service_instances?page=1&per_page=50" + }, + "last": { + "href": "https://api.example.org/v3/service_instances?page=1&per_page=50" + }, + "next": null, + "previous": null + }, + "resources": [ + { + "guid": "d4c91047-7b29-4fda-b7f9-04033e5c9c9f", + "created_at": "2017-02-02T00:14:30Z", + "updated_at": "2017-02-02T00:14:30Z", + "name": "my_service_instance" + } + ] +} +<% end %> diff --git a/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb b/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb new file mode 100644 index 00000000000..6a10dd62241 --- /dev/null +++ b/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb @@ -0,0 +1,34 @@ +### List service instances + +``` +Example Request +``` + +```shell +curl "https://api.example.org/v3/service_instances" \ + -X GET \ + -H "Authorization: bearer [token]" +``` + +``` +Example Response +``` + +```http +HTTP/1.1 200 OK +Content-Type: application/json + +<%= yield_content :paginated_list_of_service_instances, '/v3/service_instances' %> +``` +This endpoint retrieves the service instances the user has access to. + +#### Definition +`GET /v3/service_instances` + +#### Query Parameters + +Name | Type | Description +---- | ---- | ------------ +**name** | _list of strings_ | Comma-delimited list of service instance names to filter by. +**page** | _integer_ | Page to display. Valid values are integers >= 1. +**per_page** | _integer_ | Number of results per page.
Valid values are 1 through 5000. diff --git a/docs/v3/source/index.md b/docs/v3/source/index.md index 75e2112c34b..b035577261e 100644 --- a/docs/v3/source/index.md +++ b/docs/v3/source/index.md @@ -137,6 +137,7 @@ includes: - experimental_resources/service_bindings/delete - experimental_resources/service_bindings/list - experimental_resources/service_instances/header + - experimental_resources/service_instances/list - experimental_resources/service_instances/share_to_space - experimental_resources/service_instances/unshare_from_space search: true From 74bbcc12f810b259a7a6304cf10de1abcd334709 Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Mon, 30 Oct 2017 16:46:45 +0000 Subject: [PATCH 10/32] GET /v3/service_instances includes shared instances This change allows developers who have been granted access to a service instance through service instance sharing, to query that service instance via /v3/service_instances. [#152344116] Signed-off-by: Alex Blease --- app/fetchers/service_instance_list_fetcher.rb | 9 ++- .../service_instances/_list.md.erb | 2 +- spec/request/service_instances_spec.rb | 44 +++++++++++- .../v3/service_instance_controller_spec.rb | 68 ++++++++++++++++++- .../service_instance_list_fetcher_spec.rb | 33 ++++++++- 5 files changed, 146 insertions(+), 10 deletions(-) diff --git a/app/fetchers/service_instance_list_fetcher.rb b/app/fetchers/service_instance_list_fetcher.rb index 84fc0e5e022..01212a3f3b1 100644 --- a/app/fetchers/service_instance_list_fetcher.rb +++ b/app/fetchers/service_instance_list_fetcher.rb @@ -1,8 +1,13 @@ module VCAP::CloudController class ServiceInstanceListFetcher def fetch(message:, space_guids:) - dataset = ServiceInstance.select_all(ServiceInstance.table_name). - join(Space.table_name, id: :space_id, guid: space_guids) + source_space_instance_dataset = ServiceInstance.select_all(ServiceInstance.table_name). + join(Space.table_name, id: :space_id, guid: space_guids) + + shared_instance_dataset = ServiceInstance.select_all(ServiceInstance.table_name). + join(:service_instance_shares, service_instance_guid: :guid, target_space_guid: space_guids) + + dataset = source_space_instance_dataset.union(shared_instance_dataset, alias: :service_instances) filter(dataset, message) end diff --git a/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb b/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb index 6a10dd62241..585f9aa095c 100644 --- a/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb +++ b/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb @@ -20,7 +20,7 @@ Content-Type: application/json <%= yield_content :paginated_list_of_service_instances, '/v3/service_instances' %> ``` -This endpoint retrieves the service instances the user has access to. +This endpoint retrieves the service instances the user has access to. This includes access granted by service instance sharing. #### Definition `GET /v3/service_instances` diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index 9279b5d22c5..b0376067818 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -38,13 +38,13 @@ 'resources' => [ { 'guid' => service_instance3.guid, - 'name' => 'mysql', + 'name' => service_instance3.name, 'created_at' => iso8601, 'updated_at' => iso8601, }, { 'guid' => service_instance1.guid, - 'name' => 'rabbitmq', + 'name' => service_instance1.name, 'created_at' => iso8601, 'updated_at' => iso8601, } @@ -76,7 +76,7 @@ 'resources' => [ { 'guid' => service_instance2.guid, - 'name' => 'redis', + 'name' => service_instance2.name, 'created_at' => iso8601, 'updated_at' => iso8601, } @@ -84,6 +84,44 @@ } ) end + + context 'when a user has access to a shared service instance' do + before do + service_instance1.add_shared_space(target_space) + end + + it 'returns a paginated list of service instances the user has access to' do + set_current_user_as_role(role: 'space_developer', org: target_space.organization, space: target_space, user: user) + get '/v3/service_instances?per_page=2&order_by=name', nil, user_header + expect(last_response.status).to eq(200) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response).to be_a_response_like( + { + 'pagination' => { + 'total_results' => 1, + 'total_pages' => 1, + 'first' => { + 'href' => "#{link_prefix}/v3/service_instances?order_by=name&page=1&per_page=2" + }, + 'last' => { + 'href' => "#{link_prefix}/v3/service_instances?order_by=name&page=1&per_page=2" + }, + 'next' => nil, + 'previous' => nil + }, + 'resources' => [ + { + 'guid' => service_instance1.guid, + 'name' => service_instance1.name, + 'created_at' => iso8601, + 'updated_at' => iso8601, + } + ] + } + ) + end + end end describe 'POST /v3/service_instances/:guid/relationships/shared_spaces' do diff --git a/spec/unit/controllers/v3/service_instance_controller_spec.rb b/spec/unit/controllers/v3/service_instance_controller_spec.rb index e1a1b7d7e62..b2025bc5e7f 100644 --- a/spec/unit/controllers/v3/service_instance_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instance_controller_spec.rb @@ -3,9 +3,44 @@ RSpec.describe ServiceInstancesV3Controller, type: :controller do let(:user) { set_current_user(VCAP::CloudController::User.make) } let(:space) { VCAP::CloudController::Space.make } - let!(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space, name: 'awesome service') } + let!(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space) } describe '#index' do + context 'when there are multiple service instances' do + let!(:service_instance2) { VCAP::CloudController::ManagedServiceInstance.make } + let!(:service_instance3) { VCAP::CloudController::ManagedServiceInstance.make } + + context 'as an admin' do + before do + set_current_user_as_admin + end + + it 'returns all service instances' do + get :index + expect(response.status).to eq(200), response.body + expect(parsed_body['resources'].length).to eq 3 + + response_names = parsed_body['resources'].map { |resource| resource['name'] } + expect(response_names).to include(service_instance.name, service_instance2.name, service_instance3.name) + end + end + + context 'as a user who only has limited access' do + before do + set_current_user_as_role(role: 'space_developer', org: space.organization, space: space, user: user) + end + + it 'returns a subset of service instances' do + get :index + expect(response.status).to eq(200), response.body + expect(parsed_body['resources'].length).to eq 1 + + response_names = parsed_body['resources'].map { |resource| resource['name'] } + expect(response_names).to include(service_instance.name) + end + end + end + describe 'permissions by role' do role_to_expected_http_response = { 'admin' => true, @@ -24,7 +59,36 @@ it "#{can_see_service_instance ? 'can' : 'cannot'} see the service instance" do set_current_user_as_role(role: role, org: space.organization, space: space, user: user) - expected_service_instance_names = can_see_service_instance ? ['awesome service'] : [] + expected_service_instance_names = can_see_service_instance ? [service_instance.name] : [] + + get :index + expect(response.status).to eq(200), response.body + expect(parsed_body['resources'].map { |h| h['name'] }).to match_array(expected_service_instance_names) + end + end + end + end + + describe 'permissions by role for shared services' do + let(:target_space) { VCAP::CloudController::Space.make } + before do + service_instance.add_shared_space(target_space) + end + role_to_expected_http_response = { + 'org_manager' => true, + 'org_auditor' => false, + 'org_billing_manager' => false, + 'space_manager' => true, + 'space_auditor' => true, + 'space_developer' => true, + }.freeze + + role_to_expected_http_response.each do |role, can_see_service_instance| + context "as an #{role}" do + it "#{can_see_service_instance ? 'can' : 'cannot'} see the service instance" do + set_current_user_as_role(role: role, org: target_space.organization, space: target_space, user: user) + + expected_service_instance_names = can_see_service_instance ? [service_instance.name] : [] get :index expect(response.status).to eq(200), response.body diff --git a/spec/unit/queries/service_instance_list_fetcher_spec.rb b/spec/unit/queries/service_instance_list_fetcher_spec.rb index 92accad7245..fa07743ce8d 100644 --- a/spec/unit/queries/service_instance_list_fetcher_spec.rb +++ b/spec/unit/queries/service_instance_list_fetcher_spec.rb @@ -52,11 +52,11 @@ module VCAP::CloudController context 'filter' do context 'by service instance name' do - let(:filters) { { names: ['rabbitmq', 'redis'] } } + let(:filters) { { names: ['rabbitmq'] } } it 'only returns matching service instances' do results = fetcher.fetch(message: message, space_guids: [space_1.guid]).all - expect(results).to match_array([service_instance_1, service_instance_2]) + expect(results).to match_array([service_instance_1]) end end @@ -69,6 +69,35 @@ module VCAP::CloudController end end end + + context 'when service instances are shared' do + let(:shared_to_space) { Space.make } + + before do + service_instance_2.add_shared_space(shared_to_space) + service_instance_1.add_shared_space(shared_to_space) + end + + it 'returns all of the service instances shared into the specified space' do + results = fetcher.fetch(message: message, space_guids: [shared_to_space.guid]).all + expect(results).to match_array([service_instance_1, service_instance_2]) + end + end + + context 'when a space contains both shared and non-shared service instances' do + let(:shared_to_space) { Space.make } + let!(:service_instance_4) { ManagedServiceInstance.make(space: shared_to_space) } + + before do + service_instance_2.add_shared_space(shared_to_space) + service_instance_1.add_shared_space(shared_to_space) + end + + it 'returns all of the service instances shared into the specified space' do + results = fetcher.fetch(message: message, space_guids: [shared_to_space.guid]).all + expect(results).to match_array([service_instance_1, service_instance_2, service_instance_4]) + end + end end end end From d4711428fa77e22b79f4abb08a29e557a97a809d Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Wed, 1 Nov 2017 16:43:44 +0000 Subject: [PATCH 11/32] Document that /v3/service_instances only returns managed instances [#152344116] Signed-off-by: Jen Spinney --- .../experimental_resources/service_instances/_list.md.erb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb b/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb index 585f9aa095c..de938427740 100644 --- a/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb +++ b/docs/v3/source/includes/experimental_resources/service_instances/_list.md.erb @@ -20,7 +20,9 @@ Content-Type: application/json <%= yield_content :paginated_list_of_service_instances, '/v3/service_instances' %> ``` -This endpoint retrieves the service instances the user has access to. This includes access granted by service instance sharing. +This endpoint retrieves the service instances the user has access to. At the moment, this endpoint only returns managed service instances. This may change in the future. + +This includes access granted by service instance sharing. #### Definition `GET /v3/service_instances` From 4dab92bce965faae121c7b06c026f49839443212 Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Wed, 8 Nov 2017 11:07:57 +0000 Subject: [PATCH 12/32] Service instances have /v2/service_instances/:guid/shared_from endpoint * All service_instances have shared_from_url * shared_from endpoint returns space name and org name of service instance, if that service instance has been shared * If not shared, this endpoint returns 204 No Content [#150973038] Signed-off-by: Denise Yu --- .../services/service_instances_controller.rb | 18 +++ app/models/services/service_instance.rb | 4 + .../v2/service_instance_presenter.rb | 1 + .../service_instance_shared_from_presenter.rb | 14 ++ docs/v2/index.html | 3 + ...binding_a_service_instance_to_a_route.html | 3 +- .../creating_a_service_instance.html | 19 ++- .../delete_a_service_instance.html | 19 ++- .../list_all_service_instances.html | 19 ++- ...etrieve_a_particular_service_instance.html | 19 ++- ...where_service_instance_is_shared_from.html | 152 ++++++++++++++++++ .../update_a_service_instance.html | 19 ++- ...ervice_instances_for_the_service_plan.html | 3 +- ...t_all_service_instances_for_the_space.html | 3 +- spec/request/v2/service_bindings_spec.rb | 3 +- spec/request/v2/service_instances_spec.rb | 63 +++++++- spec/request/v2/spaces_spec.rb | 58 ++++++- .../runtime/spaces_controller_spec.rb | 8 + .../service_instances_controller_spec.rb | 107 ++++++++++++ .../models/services/service_instance_spec.rb | 18 +++ .../v2/service_instance_presenter_spec.rb | 61 ++++--- ...ice_instance_shared_from_presenter_spec.rb | 18 +++ 22 files changed, 595 insertions(+), 37 deletions(-) create mode 100644 app/presenters/v2/service_instance_shared_from_presenter.rb create mode 100644 docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_from.html create mode 100644 spec/unit/presenters/v2/service_instance_shared_from_presenter_spec.rb diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 44cc9ebf2ea..bda6c82755b 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -5,6 +5,7 @@ require 'controllers/services/lifecycle/service_instance_deprovisioner' require 'controllers/services/lifecycle/service_instance_purger' require 'fetchers/service_instance_fetcher' +require 'presenters/v2/service_instance_shared_from_presenter' module VCAP::CloudController class ServiceInstancesController < RestController::ModelController @@ -205,6 +206,23 @@ def permissions(guid) end end + get '/v2/service_instances/:guid/shared_from', :shared_from_information + def shared_from_information(guid) + service_instance = find_guid_and_validate_access(:read, guid, ManagedServiceInstance) + + if service_instance.shared? + [HTTP::OK, {}, JSON.generate(CloudController::Presenters::V2::ServiceInstanceSharedFromPresenter.new.to_hash(service_instance.space))] + else + [HTTP::NO_CONTENT, {}, ''] + end + rescue CloudController::Errors::ApiError => e + if e.name == 'NotAuthorized' + HTTP::NOT_FOUND + else + raise e + end + end + def self.url_for_guid(guid) object = ServiceInstance.where(guid: guid).first diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index b17fbf5078a..02159e1d975 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -175,6 +175,10 @@ def volume_service? false end + def shared? + shared_spaces.any? + end + def self.managed_organizations_spaces_dataset(managed_organizations_dataset) VCAP::CloudController::Space.dataset.filter({ organization_id: managed_organizations_dataset.select(:organization_id) }) end diff --git a/app/presenters/v2/service_instance_presenter.rb b/app/presenters/v2/service_instance_presenter.rb index a020fdae8f2..82a678adc70 100644 --- a/app/presenters/v2/service_instance_presenter.rb +++ b/app/presenters/v2/service_instance_presenter.rb @@ -28,6 +28,7 @@ def entity_hash(controller, obj, opts, depth, parents, orphans=nil) obj_hash['service_plan_guid'] = service_plan.guid obj_hash['service_guid'] = service_plan.service.guid rel_hash['service_url'] = "/v2/services/#{service_plan.service.guid}" + rel_hash['shared_from_url'] = "/v2/service_instances/#{obj.guid}/shared_from" end obj_hash.merge!(rel_hash) diff --git a/app/presenters/v2/service_instance_shared_from_presenter.rb b/app/presenters/v2/service_instance_shared_from_presenter.rb new file mode 100644 index 00000000000..405ac03c3bd --- /dev/null +++ b/app/presenters/v2/service_instance_shared_from_presenter.rb @@ -0,0 +1,14 @@ +module CloudController + module Presenters + module V2 + class ServiceInstanceSharedFromPresenter + def to_hash(space) + { + 'space_name' => space.name, + 'organization_name' => space.organization.name + } + end + end + end + end +end diff --git a/docs/v2/index.html b/docs/v2/index.html index b2e77eb7adc..cc8cb8c269f 100644 --- a/docs/v2/index.html +++ b/docs/v2/index.html @@ -875,6 +875,9 @@

  • Retrieving permissions on a Service Instance
  • +
  • + Retrieve Information Regarding Where a Service Instance is Shared From (Experimental) +
  • Unbinding a service instance from a route
  • diff --git a/docs/v2/service_instances/binding_a_service_instance_to_a_route.html b/docs/v2/service_instances/binding_a_service_instance_to_a_route.html index 9ef1f2357c0..09718949180 100644 --- a/docs/v2/service_instances/binding_a_service_instance_to_a_route.html +++ b/docs/v2/service_instances/binding_a_service_instance_to_a_route.html @@ -164,7 +164,8 @@

    Body

    "service_plan_url": "/v2/service_plans/4cfefd27-3eb7-4f55-806e-1db649038ad2", "service_bindings_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/service_bindings", "service_keys_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/service_keys", - "routes_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/routes" + "routes_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/routes", + "shared_from_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/shared_from" } } diff --git a/docs/v2/service_instances/creating_a_service_instance.html b/docs/v2/service_instances/creating_a_service_instance.html index ba70c81d34a..0fba375a0cc 100644 --- a/docs/v2/service_instances/creating_a_service_instance.html +++ b/docs/v2/service_instances/creating_a_service_instance.html @@ -557,6 +557,22 @@

    Body

    + + + shared_from_url + + + Source information for a shared service instance. Users of a shared service instance can find out its space and org information. + + +
      +
    + + +
      +
    + + tags @@ -608,7 +624,8 @@

    Body

    "service_plan_url": "/v2/service_plans/fe173a83-df28-4891-8d91-46334e04600d", "service_bindings_url": "/v2/service_instances/cc3b67fa-cda6-4df7-ba47-eb5f2a123992/service_bindings", "service_keys_url": "/v2/service_instances/cc3b67fa-cda6-4df7-ba47-eb5f2a123992/service_keys", - "routes_url": "/v2/service_instances/cc3b67fa-cda6-4df7-ba47-eb5f2a123992/routes" + "routes_url": "/v2/service_instances/cc3b67fa-cda6-4df7-ba47-eb5f2a123992/routes", + "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" } } diff --git a/docs/v2/service_instances/delete_a_service_instance.html b/docs/v2/service_instances/delete_a_service_instance.html index d97550d3529..69f9b183e8c 100644 --- a/docs/v2/service_instances/delete_a_service_instance.html +++ b/docs/v2/service_instances/delete_a_service_instance.html @@ -461,6 +461,22 @@

    Body

    + + + shared_from_url + + + Source information for a shared service instance. Users of a shared service instance can find out its space and org information. + + +
      +
    + + +
      +
    + + tags @@ -512,7 +528,8 @@

    Body

    "service_plan_url": "/v2/service_plans/8ea19d29-2e20-469e-8b91-917a6410e2f2", "service_bindings_url": "/v2/service_instances/1aaeb02d-16c3-4405-bc41-80e83d196dff/service_bindings", "service_keys_url": "/v2/service_instances/1aaeb02d-16c3-4405-bc41-80e83d196dff/service_keys", - "routes_url": "/v2/service_instances/1aaeb02d-16c3-4405-bc41-80e83d196dff/routes" + "routes_url": "/v2/service_instances/1aaeb02d-16c3-4405-bc41-80e83d196dff/routes", + "shared_from_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/shared_from" } } diff --git a/docs/v2/service_instances/list_all_service_instances.html b/docs/v2/service_instances/list_all_service_instances.html index 9b24866534e..8947de4b9c3 100644 --- a/docs/v2/service_instances/list_all_service_instances.html +++ b/docs/v2/service_instances/list_all_service_instances.html @@ -512,6 +512,22 @@

    Body

    + + + shared_from_url + + + Source information for a shared service instance. Users of a shared service instance can find out its space and org information. + + +
      +
    + + +
      +
    + + tags @@ -571,7 +587,8 @@

    Body

    "service_plan_url": "/v2/service_plans/05a372c6-6dc2-4f7f-8f65-a90ebe5fa6e2", "service_bindings_url": "/v2/service_instances/215b97be-ec77-4224-9c38-c4f2d86b56c1/service_bindings", "service_keys_url": "/v2/service_instances/215b97be-ec77-4224-9c38-c4f2d86b56c1/service_keys", - "routes_url": "/v2/service_instances/215b97be-ec77-4224-9c38-c4f2d86b56c1/routes" + "routes_url": "/v2/service_instances/215b97be-ec77-4224-9c38-c4f2d86b56c1/routes", + "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" } } ] diff --git a/docs/v2/service_instances/retrieve_a_particular_service_instance.html b/docs/v2/service_instances/retrieve_a_particular_service_instance.html index 3b81aee4e6a..b197d20c32c 100644 --- a/docs/v2/service_instances/retrieve_a_particular_service_instance.html +++ b/docs/v2/service_instances/retrieve_a_particular_service_instance.html @@ -368,6 +368,22 @@

    Body

    + + + shared_from_url + + + Source information for a shared service instance. Users of a shared service instance can find out its space and org information. + + +
      +
    + + +
      +
    + + tags @@ -421,7 +437,8 @@

    Body

    "service_plan_url": "/v2/service_plans/779d2df0-9cdd-48e8-9781-ea05301cedb1", "service_bindings_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/service_bindings", "service_keys_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/service_keys", - "routes_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/routes" + "routes_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/routes", + "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" } } diff --git a/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_from.html b/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_from.html new file mode 100644 index 00000000000..eadbbdcf0e2 --- /dev/null +++ b/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_from.html @@ -0,0 +1,152 @@ + + + + Service Instances API + + + + + + +
    +

    Service Instances API

    + +
    +

    Retrieve Information Regarding Where a Service Instance is Shared From (Experimental)

    +

    GET /v2/service_instances/:guid/shared_from

    + +

    Request

    +

    Route

    +
    GET /v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from
    + +

    Headers

    +
    Authorization: bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VyX2lkIjoidWFhLWlkLTE1MCIsImVtYWlsIjoiZW1haWwtMTA1QHNvbWVkb21haW4uY29tIiwic2NvcGUiOlsiY2xvdWRfY29udHJvbGxlci5hZG1pbiJdLCJhdWQiOlsiY2xvdWRfY29udHJvbGxlciJdLCJleHAiOjE0NjYwMDg4ODl9.eyvSz10WWSQt0pFotoQI2Lm5XdH22-50O_R8rxy7n4k
    +Host: example.org
    +Cookie: 
    + +

    cURL

    +
    curl "https://api.[your-domain.com]/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" -X GET \
    +	-H "Authorization: bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VyX2lkIjoidWFhLWlkLTE1MCIsImVtYWlsIjoiZW1haWwtMTA1QHNvbWVkb21haW4uY29tIiwic2NvcGUiOlsiY2xvdWRfY29udHJvbGxlci5hZG1pbiJdLCJhdWQiOlsiY2xvdWRfY29udHJvbGxlciJdLCJleHAiOjE0NjYwMDg4ODl9.eyvSz10WWSQt0pFotoQI2Lm5XdH22-50O_R8rxy7n4k" \
    +	-H "Host: example.org" \
    +	-H "Cookie: "
    + +

    Response

    + +

    Status

    +
    200 OK
    + +

    Body

    + + + + + + + + + + + + + + + + + + + + + + + +
    NameDescriptionValid ValuesExample Values
    + space_name + + The space name that this service instance belongs to. + +
      +
    +
    +
      +
    +
    + organization_name + + The organization name that this service instance belongs to. + +
      +
    +
    +
      +
    +
    + +
    {
    +    "space_name": "space-name",
    +    "organization_name": "org-name"
    +}
    +
    + +

    Headers

    +
    Content-Type: application/json;charset=utf-8
    +X-VCAP-Request-ID: febb14ab-a7bc-492d-b017-3db59d83967d
    +Content-Length: 253
    +X-Content-Type-Options: nosniff
    +
    +
    + + diff --git a/docs/v2/service_instances/update_a_service_instance.html b/docs/v2/service_instances/update_a_service_instance.html index 8c5bee3b126..9392093c80c 100644 --- a/docs/v2/service_instances/update_a_service_instance.html +++ b/docs/v2/service_instances/update_a_service_instance.html @@ -499,6 +499,22 @@

    Body

    + + + shared_from_url + + + Source information for a shared service instance. Users of a shared service instance can find out its space and org information. + + +
      +
    + + +
      +
    + + tags @@ -549,7 +565,8 @@

    Body

    "service_plan_url": "/v2/service_plans/4ec73bf4-9f3a-44c7-bbac-61ee9cb5a511", "service_bindings_url": "/v2/service_instances/a34f1423-4b84-4727-ab49-3f1522c4cb16/service_bindings", "service_keys_url": "/v2/service_instances/a34f1423-4b84-4727-ab49-3f1522c4cb16/service_keys", - "routes_url": "/v2/service_instances/a34f1423-4b84-4727-ab49-3f1522c4cb16/routes" + "routes_url": "/v2/service_instances/a34f1423-4b84-4727-ab49-3f1522c4cb16/routes", + "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" } } diff --git a/docs/v2/service_plans/list_all_service_instances_for_the_service_plan.html b/docs/v2/service_plans/list_all_service_instances_for_the_service_plan.html index 5a7aab99fdb..30c860e00bf 100644 --- a/docs/v2/service_plans/list_all_service_instances_for_the_service_plan.html +++ b/docs/v2/service_plans/list_all_service_instances_for_the_service_plan.html @@ -303,7 +303,8 @@

    Body

    "service_plan_url": "/v2/service_plans/85615ea0-9d23-4de8-aabd-89bffcce39d5", "service_bindings_url": "/v2/service_instances/0fac6687-69fd-4567-afb0-dd39503523ff/service_bindings", "service_keys_url": "/v2/service_instances/0fac6687-69fd-4567-afb0-dd39503523ff/service_keys", - "routes_url": "/v2/service_instances/0fac6687-69fd-4567-afb0-dd39503523ff/routes" + "routes_url": "/v2/service_instances/0fac6687-69fd-4567-afb0-dd39503523ff/routes", + "shared_from_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/shared_from" } } ] diff --git a/docs/v2/spaces/list_all_service_instances_for_the_space.html b/docs/v2/spaces/list_all_service_instances_for_the_space.html index 19145598b74..5c4fda1066f 100644 --- a/docs/v2/spaces/list_all_service_instances_for_the_space.html +++ b/docs/v2/spaces/list_all_service_instances_for_the_space.html @@ -302,7 +302,8 @@

    Body

    "service_plan_url": "/v2/service_plans/fcf57f7f-3c51-49b2-b252-dc24e0f7dcab", "service_bindings_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/service_bindings", "service_keys_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/service_keys", - "routes_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/routes" + "routes_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/routes", + "shared_from_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/shared_from" } } ] diff --git a/spec/request/v2/service_bindings_spec.rb b/spec/request/v2/service_bindings_spec.rb index 11aea28425a..0f45371e09a 100644 --- a/spec/request/v2/service_bindings_spec.rb +++ b/spec/request/v2/service_bindings_spec.rb @@ -197,7 +197,8 @@ 'service_plan_url' => "/v2/service_plans/#{service_instance.service_plan.guid}", 'service_bindings_url' => "/v2/service_instances/#{service_instance.guid}/service_bindings", 'service_keys_url' => "/v2/service_instances/#{service_instance.guid}/service_keys", - 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes" + 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes", + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from" } } } diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb index 8708ea9e454..97cffdf5878 100644 --- a/spec/request/v2/service_instances_spec.rb +++ b/spec/request/v2/service_instances_spec.rb @@ -54,7 +54,8 @@ 'service_plan_url' => "/v2/service_plans/#{service_plan.guid}", 'service_bindings_url' => "/v2/service_instances/#{service_instance.guid}/service_bindings", 'service_keys_url' => "/v2/service_instances/#{service_instance.guid}/service_keys", - 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes" + 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes", + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from" } } ) @@ -96,7 +97,8 @@ 'service_url' => "/v2/services/#{service_instance.service.guid}", 'service_bindings_url' => "/v2/service_instances/#{service_instance.guid}/service_bindings", 'service_keys_url' => "/v2/service_instances/#{service_instance.guid}/service_keys", - 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes" + 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes", + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from" } } ) @@ -137,7 +139,8 @@ 'service_url' => "/v2/services/#{service_instance.service.guid}", 'service_bindings_url' => "/v2/service_instances/#{service_instance.guid}/service_bindings", 'service_keys_url' => "/v2/service_instances/#{service_instance.guid}/service_keys", - 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes" + 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes", + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from" } } ) @@ -145,4 +148,58 @@ end end end + + describe 'GET /v2/service_instances/:service_instance_guid/shared_from' do + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space) } + + before do + service_instance.add_shared_space(VCAP::CloudController::Space.make) + end + + it 'returns data about the source space and org' do + get "v2/service_instances/#{service_instance.guid}/shared_from", nil, admin_headers + + expect(last_response.status).to eq(200), last_response.body + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response).to be_a_response_like({ + 'space_name' => space.name, + 'organization_name' => space.organization.name + }) + end + + context 'when the user is a member of the space where a service instance has been shared to' do + let(:other_space) { VCAP::CloudController::Space.make } + let(:other_user) { make_developer_for_space(other_space) } + let(:req_body) do + { + data: [ + { guid: other_space.guid } + ] + }.to_json + end + + before do + VCAP::CloudController::FeatureFlag.make(name: 'service_instance_sharing', enabled: true, error_message: nil) + + other_space.organization.add_user(user) + other_space.add_developer(user) + + post "v3/service_instances/#{service_instance.guid}/relationships/shared_spaces", req_body, headers_for(user) + expect(last_response.status).to eq(200) + end + + it 'returns data about the source space and org' do + get "v2/service_instances/#{service_instance.guid}/shared_from", nil, headers_for(other_user) + + expect(last_response.status).to eq(200) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response).to be_a_response_like({ + 'space_name' => space.name, + 'organization_name' => space.organization.name + }) + end + end + end end diff --git a/spec/request/v2/spaces_spec.rb b/spec/request/v2/spaces_spec.rb index bdd08d3d8ed..204896abd6e 100644 --- a/spec/request/v2/spaces_spec.rb +++ b/spec/request/v2/spaces_spec.rb @@ -132,7 +132,7 @@ space.add_developer(user) end - it 'lists the isolation segment for SpaceDvelopers' do + it 'lists the isolation segment for SpaceDevelopers' do get "/v2/spaces/#{space.guid}", {}, headers_for(user) expect(last_response.status).to eq(200) @@ -170,6 +170,62 @@ end end + describe 'GET /v2/spaces/:guid/service_instances' do + let(:originating_space) { VCAP::CloudController::Space.make } + let(:shared_service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: originating_space) } + let(:space) { VCAP::CloudController::Space.make } + + before do + originating_space.organization.add_user(user) + originating_space.add_developer(user) + space.organization.add_user(user) + space.add_developer(user) + + shared_service_instance.add_shared_space(space) + end + + it 'shows the shared service instances associated with the space' do + get "/v2/spaces/#{space.guid}/service_instances", {}, headers_for(user) + + expect(last_response.status).to eq(200) + parsed_response = MultiJson.load(last_response.body) + + expect(parsed_response).to be_a_response_like({ + 'total_results' => 1, + 'total_pages' => 1, + 'prev_url' => nil, + 'next_url' => nil, + 'resources' => [{ + 'metadata' => { + 'guid' => shared_service_instance.guid, + 'url' => "/v2/service_instances/#{shared_service_instance.guid}", + 'created_at' => iso8601, + 'updated_at' => iso8601, + }, + 'entity' => { + 'name' => shared_service_instance.name, + 'credentials' => shared_service_instance.credentials, + 'service_plan_guid' => shared_service_instance.service_plan_guid, + 'space_guid' => originating_space.guid, + 'gateway_data' => nil, + 'dashboard_url' => nil, + 'type' => 'managed_service_instance', + 'last_operation' => nil, + 'tags' => [], + 'service_guid' => shared_service_instance.service_plan.service_guid, + 'space_url' => "/v2/spaces/#{originating_space.guid}", + 'service_plan_url' => "/v2/service_plans/#{shared_service_instance.service_plan_guid}", + 'service_bindings_url' => "/v2/service_instances/#{shared_service_instance.guid}/service_bindings", + 'service_keys_url' => "/v2/service_instances/#{shared_service_instance.guid}/service_keys", + 'routes_url' => "/v2/service_instances/#{shared_service_instance.guid}/routes", + 'service_url' => "/v2/services/#{shared_service_instance.service_plan.service_guid}", + 'shared_from_url' => "/v2/service_instances/#{shared_service_instance.guid}/shared_from" + } + }] + }) + end + end + describe 'DELETE /v2/spaces/:guid/unmapped_routes' do let(:space) { VCAP::CloudController::Space.make(organization: org) } let(:process) { VCAP::CloudController::ProcessModelFactory.make(state: 'STARTED') } diff --git a/spec/unit/controllers/runtime/spaces_controller_spec.rb b/spec/unit/controllers/runtime/spaces_controller_spec.rb index e407cf5af60..de2de367020 100644 --- a/spec/unit/controllers/runtime/spaces_controller_spec.rb +++ b/spec/unit/controllers/runtime/spaces_controller_spec.rb @@ -223,6 +223,14 @@ def decoded_guids before { set_current_user(developer) } + it 'returns the shared from url' do + space_instance = ManagedServiceInstance.make(space: space) + + get "/v2/spaces/#{space.guid}/service_instances" + service_instance_response = decoded_response.fetch('resources').first + expect(service_instance_response.fetch('entity').fetch('shared_from_url')).to eq("/v2/service_instances/#{space_instance.guid}/shared_from") + end + context 'when filtering results' do it 'returns only matching results' do user_provided_service_instance_1 = UserProvidedServiceInstance.make(space: space, name: 'provided service 1') diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 6f6607fa9e2..c69b134972b 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -3496,6 +3496,113 @@ def verify_forbidden(user) end end + describe 'GET /v2/service_instances/:service_instance_guid/shared_from' do + let(:org) { Organization.make } + let(:space) { Space.make(organization: org) } + let(:instance) { ManagedServiceInstance.make(space: space) } + + context 'when the service instance is not shared' do + it 'returns no content' do + set_current_user_as_admin + get "/v2/service_instances/#{instance.guid}/shared_from" + expect(last_response.status).to eql(204) + expect(JSON.parse(last_response.body)).to be nil + end + end + + context 'when the service instance is shared' do + let(:other_org) { Organization.make } + let(:other_space) { Space.make(organization: other_org) } + + before do + instance.add_shared_space(other_space) + end + + it 'returns the correct body' do + set_current_user_as_admin + get "/v2/service_instances/#{instance.guid}/shared_from" + expect(last_response.status).to eql(200), last_response.body + parsed_response = JSON.parse(last_response.body) + expect(parsed_response['space_name']).to eq(space.name) + expect(parsed_response['organization_name']).to eq(space.organization.name) + expect(parsed_response.keys).to match_array(['space_name', 'organization_name']) + end + + describe 'permissions' do + let(:user) { User.make } + + context 'when the user is a member of the org/space this instance exists in' do + { + 'admin' => 200, + 'space_developer' => 200, + 'admin_read_only' => 200, + 'global_auditor' => 200, + 'space_manager' => 200, + 'space_auditor' => 200, + 'org_manager' => 200, + 'org_auditor' => 404, + 'org_billing_manager' => 404, + }.each do |role, expected_status| + context "as an #{role}" do + before do + set_current_user_as_role( + role: role, + org: org, + space: space, + user: user, + scopes: ['cloud_controller.read'] + ) + end + + it "has a #{expected_status} http status code" do + get "/v2/service_instances/#{instance.guid}/shared_from" + expect(last_response.status).to eq(expected_status), "Expected #{expected_status}, got: #{last_response.status}, role: #{role}" + end + end + end + end + + context 'when the user is a member of the org/space where the service instance was shared to' do + { + 'space_developer' => 200, + 'space_manager' => 200, + 'space_auditor' => 200, + 'org_manager' => 200, + 'org_auditor' => 404, + 'org_billing_manager' => 404, + }.each do |role, expected_status| + context "as an #{role}" do + before do + set_current_user_as_role( + role: role, + org: other_org, + space: other_space, + user: user, + scopes: ['cloud_controller.read'] + ) + end + + it "has a #{expected_status} http status code" do + get "/v2/service_instances/#{instance.guid}/shared_from" + expect(last_response.status).to eq(expected_status), "Expected #{expected_status}, got: #{last_response.status}, role: #{role}" + end + end + end + end + + context 'when the user is NOT a member of the space this instance exists in' do + let(:instance) { ManagedServiceInstance.make } + + it 'returns a JSON payload indicating the user does not have permission to manage this instance' do + set_current_user(user) + get "/v2/service_instances/#{instance.guid}/shared_from" + expect(last_response.status).to eql(404) + end + end + end + end + end + describe 'GET /v2/service_instances/:service_instance_guid/service_keys' do let(:space) { Space.make } let(:manager) { make_manager_for_space(space) } diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 29b5cf92b7e..52bb0ff5662 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/spec/unit/models/services/service_instance_spec.rb @@ -432,5 +432,23 @@ module VCAP::CloudController end end end + + describe '#shared?' do + context 'when the service instance has shared spaces' do + before do + service_instance.add_shared_space(Space.make) + end + + it 'returns true' do + expect(service_instance.shared?).to be true + end + end + + context 'when the service instance does not have shared spaces' do + it 'returns false' do + expect(service_instance.shared?).to be false + end + end + end end end diff --git a/spec/unit/presenters/v2/service_instance_presenter_spec.rb b/spec/unit/presenters/v2/service_instance_presenter_spec.rb index 804e909e196..638abbd6065 100644 --- a/spec/unit/presenters/v2/service_instance_presenter_spec.rb +++ b/spec/unit/presenters/v2/service_instance_presenter_spec.rb @@ -11,35 +11,48 @@ module CloudController::Presenters::V2 let(:relations_hash) { { 'relationship_url' => 'http://relationship.example.com' } } subject { ServiceInstancePresenter.new } - describe '#entity_hash' do - before do - set_current_user_as_admin - end + before do + set_current_user_as_admin + allow(RelationsPresenter).to receive(:new).and_return(relations_presenter) + end - let(:service_instance) do - VCAP::CloudController::ServiceInstance.make( - name: 'things', - ) - end - let(:service_plan) { VCAP::CloudController::ServicePlan.make } + describe 'ManagedServiceInstance' do + describe '#entity_hash' do + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make } + let(:service_plan) { VCAP::CloudController::ServicePlan.make } - before do - service_instance.service_plan_id = service_plan.id - service_instance.save + before do + service_instance.service_plan_id = service_plan.id + service_instance.save + end - allow(RelationsPresenter).to receive(:new).and_return(relations_presenter) + it 'returns the service instance entity' do + expect(subject.entity_hash(controller, service_instance, opts, depth, parents, orphans)).to eq( + { + 'name' => service_instance.name, + 'service_plan_guid' => service_plan.guid, + 'service_guid' => service_plan.service.guid, + 'relationship_url' => 'http://relationship.example.com', + 'service_url' => "/v2/services/#{service_plan.service.guid}", + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from", + } + ) + end end + end + + describe 'UserProvidedServiceInstance' do + describe '#entity_hash' do + let(:service_instance) { VCAP::CloudController::UserProvidedServiceInstance.make } - it 'returns the service instance entity' do - expect(subject.entity_hash(controller, service_instance, opts, depth, parents, orphans)).to eq( - { - 'name' => service_instance.name, - 'service_plan_guid' => service_plan.guid, - 'service_guid' => service_plan.service.guid, - 'relationship_url' => 'http://relationship.example.com', - 'service_url' => "/v2/services/#{service_plan.service.guid}" - } - ) + it 'returns the service instance entity' do + expect(subject.entity_hash(controller, service_instance, opts, depth, parents, orphans)).to eq( + { + 'name' => service_instance.name, + 'relationship_url' => 'http://relationship.example.com', + } + ) + end end end end diff --git a/spec/unit/presenters/v2/service_instance_shared_from_presenter_spec.rb b/spec/unit/presenters/v2/service_instance_shared_from_presenter_spec.rb new file mode 100644 index 00000000000..8c18d57602e --- /dev/null +++ b/spec/unit/presenters/v2/service_instance_shared_from_presenter_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +module CloudController::Presenters::V2 + RSpec.describe ServiceInstanceSharedFromPresenter do + describe '#to_hash' do + it 'returns the space and org name' do + space = VCAP::CloudController::Space.make + presenter = ServiceInstanceSharedFromPresenter.new + expect(presenter.to_hash(space)).to eq( + { + 'space_name' => space.name, + 'organization_name' => space.organization.name, + } + ) + end + end + end +end From 59de5046b566561676a7e910eeee9a1eb1323260 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Wed, 8 Nov 2017 11:55:09 +0000 Subject: [PATCH 13/32] Service instances have /v2/service_instances/:guid/shared_to endpoint * All service_instance responses now return a shared_to_url * /v2/service_instances/:guid/shared_to endpoint returns a list of space and org name pairs indicating where the instance has been shared to [#151432798] Signed-off-by: Jen Spinney --- .../services/service_instances_controller.rb | 41 ++++ .../v2/service_instance_presenter.rb | 1 + .../service_instance_shared_to_presenter.rb | 13 + docs/v2/index.html | 3 + ...binding_a_service_instance_to_a_route.html | 3 +- .../creating_a_service_instance.html | 19 +- .../delete_a_service_instance.html | 19 +- .../list_all_service_instances.html | 19 +- ...etrieve_a_particular_service_instance.html | 19 +- ...o_where_service_instance_is_shared_to.html | 226 ++++++++++++++++++ .../update_a_service_instance.html | 19 +- ...ervice_instances_for_the_service_plan.html | 3 +- ...t_all_service_instances_for_the_space.html | 3 +- spec/request/v2/service_bindings_spec.rb | 3 +- spec/request/v2/service_instances_spec.rb | 46 +++- spec/request/v2/spaces_spec.rb | 3 +- .../service_instances_controller_spec.rb | 113 +++++++++ .../v2/service_instance_presenter_spec.rb | 1 + ...rvice_instance_shared_to_presenter_spec.rb | 18 ++ 19 files changed, 559 insertions(+), 13 deletions(-) create mode 100644 app/presenters/v2/service_instance_shared_to_presenter.rb create mode 100644 docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_to.html create mode 100644 spec/unit/presenters/v2/service_instance_shared_to_presenter_spec.rb diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index bda6c82755b..97e2c9294dd 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -5,6 +5,7 @@ require 'controllers/services/lifecycle/service_instance_deprovisioner' require 'controllers/services/lifecycle/service_instance_purger' require 'fetchers/service_instance_fetcher' +require 'presenters/v2/service_instance_shared_to_presenter' require 'presenters/v2/service_instance_shared_from_presenter' module VCAP::CloudController @@ -223,6 +224,29 @@ def shared_from_information(guid) end end + get '/v2/service_instances/:guid/shared_to', :enumerate_shared_to_information + def enumerate_shared_to_information(guid) + service_instance = find_guid_and_validate_access(:read, guid, ManagedServiceInstance) + validate_access(:read, service_instance.space) + + associated_controller = VCAP::CloudController::SpacesController + associated_path = "#{self.class.url_for_guid(guid)}/shared_to" + + create_paginated_collection_renderer.render_json( + associated_controller, + service_instance.shared_spaces_dataset, + associated_path, + @opts, + {}, + ) + rescue CloudController::Errors::ApiError => e + if e.name == 'NotAuthorized' + HTTP::NOT_FOUND + else + raise e + end + end + def self.url_for_guid(guid) object = ServiceInstance.where(guid: guid).first @@ -321,6 +345,23 @@ def unbind_route(route_guid, instance_guid) private + class ServiceInstanceSharedToSerializer + def serialize(controller, space, opts, orphans=nil) + CloudController::Presenters::V2::ServiceInstanceSharedToPresenter.new.to_hash(space) + end + end + + def create_paginated_collection_renderer + VCAP::CloudController::RestController::PaginatedCollectionRenderer.new( + VCAP::CloudController::RestController::SecureEagerLoader.new, + ServiceInstanceSharedToSerializer.new, + { + max_results_per_page: config.get(:renderer, :max_results_per_page), + default_results_per_page: config.get(:renderer, :default_results_per_page), + max_inline_relations_depth: config.get(:renderer, :max_inline_relations_depth), + }) + end + def route_services_enabled? @config.get(:route_services_enabled) end diff --git a/app/presenters/v2/service_instance_presenter.rb b/app/presenters/v2/service_instance_presenter.rb index 82a678adc70..8f688d6a781 100644 --- a/app/presenters/v2/service_instance_presenter.rb +++ b/app/presenters/v2/service_instance_presenter.rb @@ -29,6 +29,7 @@ def entity_hash(controller, obj, opts, depth, parents, orphans=nil) obj_hash['service_guid'] = service_plan.service.guid rel_hash['service_url'] = "/v2/services/#{service_plan.service.guid}" rel_hash['shared_from_url'] = "/v2/service_instances/#{obj.guid}/shared_from" + rel_hash['shared_to_url'] = "/v2/service_instances/#{obj.guid}/shared_to" end obj_hash.merge!(rel_hash) diff --git a/app/presenters/v2/service_instance_shared_to_presenter.rb b/app/presenters/v2/service_instance_shared_to_presenter.rb new file mode 100644 index 00000000000..70f927420c7 --- /dev/null +++ b/app/presenters/v2/service_instance_shared_to_presenter.rb @@ -0,0 +1,13 @@ +require 'presenters/v2/service_instance_shared_from_presenter' + +module CloudController + module Presenters + module V2 + class ServiceInstanceSharedToPresenter < ServiceInstanceSharedFromPresenter + def to_hash(space) + super(space) + end + end + end + end +end diff --git a/docs/v2/index.html b/docs/v2/index.html index cc8cb8c269f..6896378863e 100644 --- a/docs/v2/index.html +++ b/docs/v2/index.html @@ -878,6 +878,9 @@

  • Retrieve Information Regarding Where a Service Instance is Shared From (Experimental)
  • +
  • + Retrieve Information Regarding Where a Service Instance is Shared To (Experimental) +
  • Unbinding a service instance from a route
  • diff --git a/docs/v2/service_instances/binding_a_service_instance_to_a_route.html b/docs/v2/service_instances/binding_a_service_instance_to_a_route.html index 09718949180..1912be9e411 100644 --- a/docs/v2/service_instances/binding_a_service_instance_to_a_route.html +++ b/docs/v2/service_instances/binding_a_service_instance_to_a_route.html @@ -165,7 +165,8 @@

    Body

    "service_bindings_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/service_bindings", "service_keys_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/service_keys", "routes_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/routes", - "shared_from_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/shared_from" + "shared_from_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/shared_from", + "shared_to_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/shared_to" } } diff --git a/docs/v2/service_instances/creating_a_service_instance.html b/docs/v2/service_instances/creating_a_service_instance.html index 0fba375a0cc..2e6c57b78a7 100644 --- a/docs/v2/service_instances/creating_a_service_instance.html +++ b/docs/v2/service_instances/creating_a_service_instance.html @@ -573,6 +573,22 @@

    Body

    + + + shared_to_url + + + Information about where the service instance instance has been shared to. + + +
      +
    + + +
      +
    + + tags @@ -625,7 +641,8 @@

    Body

    "service_bindings_url": "/v2/service_instances/cc3b67fa-cda6-4df7-ba47-eb5f2a123992/service_bindings", "service_keys_url": "/v2/service_instances/cc3b67fa-cda6-4df7-ba47-eb5f2a123992/service_keys", "routes_url": "/v2/service_instances/cc3b67fa-cda6-4df7-ba47-eb5f2a123992/routes", - "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" + "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from", + "shared_to_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_to" } } diff --git a/docs/v2/service_instances/delete_a_service_instance.html b/docs/v2/service_instances/delete_a_service_instance.html index 69f9b183e8c..077ad3f0a72 100644 --- a/docs/v2/service_instances/delete_a_service_instance.html +++ b/docs/v2/service_instances/delete_a_service_instance.html @@ -477,6 +477,22 @@

    Body

    + + + shared_to_url + + + Information about where the service instance instance has been shared to. + + +
      +
    + + +
      +
    + + tags @@ -529,7 +545,8 @@

    Body

    "service_bindings_url": "/v2/service_instances/1aaeb02d-16c3-4405-bc41-80e83d196dff/service_bindings", "service_keys_url": "/v2/service_instances/1aaeb02d-16c3-4405-bc41-80e83d196dff/service_keys", "routes_url": "/v2/service_instances/1aaeb02d-16c3-4405-bc41-80e83d196dff/routes", - "shared_from_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/shared_from" + "shared_from_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/shared_from", + "shared_to_url": "/v2/service_instances/6da8d173-b409-4094-949f-3c1cc8a68503/shared_to" } } diff --git a/docs/v2/service_instances/list_all_service_instances.html b/docs/v2/service_instances/list_all_service_instances.html index 8947de4b9c3..16be8a03de0 100644 --- a/docs/v2/service_instances/list_all_service_instances.html +++ b/docs/v2/service_instances/list_all_service_instances.html @@ -528,6 +528,22 @@

    Body

    + + + shared_to_url + + + Information about where the service instance instance has been shared to. + + +
      +
    + + +
      +
    + + tags @@ -588,7 +604,8 @@

    Body

    "service_bindings_url": "/v2/service_instances/215b97be-ec77-4224-9c38-c4f2d86b56c1/service_bindings", "service_keys_url": "/v2/service_instances/215b97be-ec77-4224-9c38-c4f2d86b56c1/service_keys", "routes_url": "/v2/service_instances/215b97be-ec77-4224-9c38-c4f2d86b56c1/routes", - "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" + "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from", + "shared_to_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_to" } } ] diff --git a/docs/v2/service_instances/retrieve_a_particular_service_instance.html b/docs/v2/service_instances/retrieve_a_particular_service_instance.html index b197d20c32c..22431727eed 100644 --- a/docs/v2/service_instances/retrieve_a_particular_service_instance.html +++ b/docs/v2/service_instances/retrieve_a_particular_service_instance.html @@ -384,6 +384,22 @@

    Body

    + + + shared_to_url + + + Information about where the service instance instance has been shared to. + + +
      +
    + + +
      +
    + + tags @@ -438,7 +454,8 @@

    Body

    "service_bindings_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/service_bindings", "service_keys_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/service_keys", "routes_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/routes", - "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" + "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from", + "shared_to_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_to" } } diff --git a/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_to.html b/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_to.html new file mode 100644 index 00000000000..d3686b1ae0b --- /dev/null +++ b/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_to.html @@ -0,0 +1,226 @@ + + + + Service Instances API + + + + + + +
    +

    Service Instances API

    + +
    +

    Retrieve Information Regarding Where a Service Instance is Shared To (Experimental)

    +

    GET /v2/service_instances/:guid/shared_to

    + +

    Request

    +

    Route

    +
    GET /v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_to
    + +

    Parameters

    + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    NameDescriptionValid ValuesExample Values
    + page + + Page of results to fetch + +
      +
    +
    +
      +
    +
    + results-per-page + + Number of results per page + +
      +
    +
    +
      +
    +
    + order-direction + + Order of the results: asc (default) or desc + +
      +
    +
    +
      +
    +
    + +

    Headers

    +
    Authorization: bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VyX2lkIjoidWFhLWlkLTE1MCIsImVtYWlsIjoiZW1haWwtMTA1QHNvbWVkb21haW4uY29tIiwic2NvcGUiOlsiY2xvdWRfY29udHJvbGxlci5hZG1pbiJdLCJhdWQiOlsiY2xvdWRfY29udHJvbGxlciJdLCJleHAiOjE0NjYwMDg4ODl9.eyvSz10WWSQt0pFotoQI2Lm5XdH22-50O_R8rxy7n4k
    +Host: example.org
    +Cookie: 
    + +

    cURL

    +
    curl "https://api.[your-domain.com]/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_to" -X GET \
    +	-H "Authorization: bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VyX2lkIjoidWFhLWlkLTE1MCIsImVtYWlsIjoiZW1haWwtMTA1QHNvbWVkb21haW4uY29tIiwic2NvcGUiOlsiY2xvdWRfY29udHJvbGxlci5hZG1pbiJdLCJhdWQiOlsiY2xvdWRfY29udHJvbGxlciJdLCJleHAiOjE0NjYwMDg4ODl9.eyvSz10WWSQt0pFotoQI2Lm5XdH22-50O_R8rxy7n4k" \
    +	-H "Host: example.org" \
    +	-H "Cookie: "
    + +

    Response

    + +

    Status

    +
    200 OK
    + +

    Body

    + + + + + + + + + + + + + + + + + + + + + + + +
    NameDescriptionValid ValuesExample Values
    + space_name + + The name of the space being shared to. + +
      +
    +
    +
      +
    +
    + organization_name + + The name of the organization of the space the service instance has been shared to. + +
      +
    +
    +
      +
    +
    + +
    {
    +   "total_results": 2,
    +   "total_pages": 1,
    +   "prev_url": null,
    +   "next_url": null,
    +   "resources": [
    +      {
    +          "space_name": "target-space-1",
    +          "organization_name": "org-name"
    +      },
    +      {
    +          "space_name": "target-space-2",
    +          "organization_name": "org-name"
    +      }
    +   ]
    +}
    + +

    Headers

    +
    Content-Type: application/json;charset=utf-8
    +X-VCAP-Request-ID: febb14ab-a7bc-492d-b017-3db59d83967d
    +Content-Length: 378
    +X-Content-Type-Options: nosniff
    + +
    +
    + + diff --git a/docs/v2/service_instances/update_a_service_instance.html b/docs/v2/service_instances/update_a_service_instance.html index 9392093c80c..c1703cd1835 100644 --- a/docs/v2/service_instances/update_a_service_instance.html +++ b/docs/v2/service_instances/update_a_service_instance.html @@ -515,6 +515,22 @@

    Body

    + + + shared_to_url + + + Information about where the service instance instance has been shared to. + + +
      +
    + + +
      +
    + + tags @@ -566,7 +582,8 @@

    Body

    "service_bindings_url": "/v2/service_instances/a34f1423-4b84-4727-ab49-3f1522c4cb16/service_bindings", "service_keys_url": "/v2/service_instances/a34f1423-4b84-4727-ab49-3f1522c4cb16/service_keys", "routes_url": "/v2/service_instances/a34f1423-4b84-4727-ab49-3f1522c4cb16/routes", - "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from" + "shared_from_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_from", + "shared_to_url": "/v2/service_instances/0d632575-bb06-4ea5-bb19-a451a9644d92/shared_to" } } diff --git a/docs/v2/service_plans/list_all_service_instances_for_the_service_plan.html b/docs/v2/service_plans/list_all_service_instances_for_the_service_plan.html index 30c860e00bf..770bcaf779f 100644 --- a/docs/v2/service_plans/list_all_service_instances_for_the_service_plan.html +++ b/docs/v2/service_plans/list_all_service_instances_for_the_service_plan.html @@ -304,7 +304,8 @@

    Body

    "service_bindings_url": "/v2/service_instances/0fac6687-69fd-4567-afb0-dd39503523ff/service_bindings", "service_keys_url": "/v2/service_instances/0fac6687-69fd-4567-afb0-dd39503523ff/service_keys", "routes_url": "/v2/service_instances/0fac6687-69fd-4567-afb0-dd39503523ff/routes", - "shared_from_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/shared_from" + "shared_from_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/shared_from", + "shared_to_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/shared_to" } } ] diff --git a/docs/v2/spaces/list_all_service_instances_for_the_space.html b/docs/v2/spaces/list_all_service_instances_for_the_space.html index 5c4fda1066f..e3cd4cae7f9 100644 --- a/docs/v2/spaces/list_all_service_instances_for_the_space.html +++ b/docs/v2/spaces/list_all_service_instances_for_the_space.html @@ -303,7 +303,8 @@

    Body

    "service_bindings_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/service_bindings", "service_keys_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/service_keys", "routes_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/routes", - "shared_from_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/shared_from" + "shared_from_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/shared_from", + "shared_to_url": "/v2/service_instances/9547e9ed-e460-4abe-bda3-7070b9835917/shared_to" } } ] diff --git a/spec/request/v2/service_bindings_spec.rb b/spec/request/v2/service_bindings_spec.rb index 0f45371e09a..ce50ba51bb5 100644 --- a/spec/request/v2/service_bindings_spec.rb +++ b/spec/request/v2/service_bindings_spec.rb @@ -198,7 +198,8 @@ 'service_bindings_url' => "/v2/service_instances/#{service_instance.guid}/service_bindings", 'service_keys_url' => "/v2/service_instances/#{service_instance.guid}/service_keys", 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes", - 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from" + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from", + 'shared_to_url' => "/v2/service_instances/#{service_instance.guid}/shared_to", } } } diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb index 97cffdf5878..733d1e93582 100644 --- a/spec/request/v2/service_instances_spec.rb +++ b/spec/request/v2/service_instances_spec.rb @@ -55,7 +55,8 @@ 'service_bindings_url' => "/v2/service_instances/#{service_instance.guid}/service_bindings", 'service_keys_url' => "/v2/service_instances/#{service_instance.guid}/service_keys", 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes", - 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from" + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from", + 'shared_to_url' => "/v2/service_instances/#{service_instance.guid}/shared_to", } } ) @@ -98,7 +99,8 @@ 'service_bindings_url' => "/v2/service_instances/#{service_instance.guid}/service_bindings", 'service_keys_url' => "/v2/service_instances/#{service_instance.guid}/service_keys", 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes", - 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from" + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from", + 'shared_to_url' => "/v2/service_instances/#{service_instance.guid}/shared_to", } } ) @@ -140,7 +142,8 @@ 'service_bindings_url' => "/v2/service_instances/#{service_instance.guid}/service_bindings", 'service_keys_url' => "/v2/service_instances/#{service_instance.guid}/service_keys", 'routes_url' => "/v2/service_instances/#{service_instance.guid}/routes", - 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from" + 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from", + 'shared_to_url' => "/v2/service_instances/#{service_instance.guid}/shared_to", } } ) @@ -202,4 +205,41 @@ end end end + + describe 'GET /v2/service_instances/:service_instance_guid/shared_to' do + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space) } + let(:space1) { VCAP::CloudController::Space.make } + let(:space2) { VCAP::CloudController::Space.make } + + before do + service_instance.add_shared_space(space1) + service_instance.add_shared_space(space2) + end + + it 'returns data about the source space and org' do + get "v2/service_instances/#{service_instance.guid}/shared_to", nil, admin_headers + + expect(last_response.status).to eq(200) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response).to be_a_response_like( + { + 'total_results' => 2, + 'total_pages' => 1, + 'prev_url' => nil, + 'next_url' => nil, + 'resources' => [ + { + 'space_name' => space1.name, + 'organization_name' => space1.organization.name + }, + { + 'space_name' => space2.name, + 'organization_name' => space2.organization.name + } + ] + } + ) + end + end end diff --git a/spec/request/v2/spaces_spec.rb b/spec/request/v2/spaces_spec.rb index 204896abd6e..f163cf306fa 100644 --- a/spec/request/v2/spaces_spec.rb +++ b/spec/request/v2/spaces_spec.rb @@ -219,7 +219,8 @@ 'service_keys_url' => "/v2/service_instances/#{shared_service_instance.guid}/service_keys", 'routes_url' => "/v2/service_instances/#{shared_service_instance.guid}/routes", 'service_url' => "/v2/services/#{shared_service_instance.service_plan.service_guid}", - 'shared_from_url' => "/v2/service_instances/#{shared_service_instance.guid}/shared_from" + 'shared_from_url' => "/v2/service_instances/#{shared_service_instance.guid}/shared_from", + 'shared_to_url' => "/v2/service_instances/#{shared_service_instance.guid}/shared_to", } }] }) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index c69b134972b..2ae20afde85 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -3603,6 +3603,119 @@ def verify_forbidden(user) end end + describe 'GET /v2/service_instances/:service_instance_guid/shared_to' do + let(:org) { Organization.make } + let(:space) { Space.make(organization: org) } + let(:instance) { ManagedServiceInstance.make(space: space) } + + it 'returns the correct body' do + set_current_user_as_admin + get "/v2/service_instances/#{instance.guid}/shared_to" + expect(last_response.status).to eql(200) + expect(JSON.parse(last_response.body)['resources']).to eq([]) + end + + context 'when the service instance is shared into multiple spaces' do + let(:space1) { Space.make } + let(:space2) { Space.make } + + before do + FeatureFlag.make(name: 'service_instance_sharing', enabled: true, error_message: nil) + instance.add_shared_space(space1) + instance.add_shared_space(space2) + end + + it 'returns the correct body' do + set_current_user_as_admin + get "/v2/service_instances/#{instance.guid}/shared_to" + decoded_response = JSON.parse(last_response.body) + expect(last_response.status).to eql(200), last_response.body + expect(decoded_response.fetch('total_results')).to eq(2) + resources = decoded_response.fetch('resources') + + space1_resource = resources.find { |resource| resource['space_name'] == space1.name } + space2_resource = resources.find { |resource| resource['space_name'] == space2.name } + + expect(space1_resource.keys).to match_array(['space_name', 'organization_name']) + expect(space2_resource.keys).to match_array(['space_name', 'organization_name']) + + expect(space1_resource.fetch('space_name')).to eq(space1.name) + expect(space2_resource.fetch('space_name')).to eq(space2.name) + + expect(space1_resource.fetch('organization_name')).to eq(space1.organization.name) + expect(space2_resource.fetch('organization_name')).to eq(space2.organization.name) + end + end + + describe 'permissions' do + let(:user) { User.make } + + context 'when the user is a member of the org/space this instance exists in' do + { + 'admin' => 200, + 'space_developer' => 200, + 'admin_read_only' => 200, + 'global_auditor' => 200, + 'space_manager' => 200, + 'space_auditor' => 200, + 'org_manager' => 200, + 'org_auditor' => 404, + 'org_billing_manager' => 404, + }.each do |role, expected_status| + context "as an #{role}" do + before do + set_current_user_as_role( + role: role, + org: org, + space: space, + user: user, + ) + end + + it "has a #{expected_status} http status code" do + get "/v2/service_instances/#{instance.guid}/shared_to" + expect(last_response.status).to eq(expected_status), "Expected #{expected_status}, got: #{last_response.status}, role: #{role}" + end + end + end + end + + context 'when the user is a member of the org/space where the service instance was shared to' do + let(:other_org) { Organization.make } + let(:other_space) { Space.make(organization: other_org) } + + before do + instance.add_shared_space(other_space) + end + + { + 'space_developer' => 404, + 'space_manager' => 404, + 'space_auditor' => 404, + 'org_manager' => 404, + 'org_auditor' => 404, + 'org_billing_manager' => 404, + }.each do |role, expected_status| + context "as an #{role}" do + before do + set_current_user_as_role( + role: role, + org: other_org, + space: other_space, + user: user, + ) + end + + it "has a #{expected_status} http status code" do + get "/v2/service_instances/#{instance.guid}/shared_to" + expect(last_response.status).to eq(expected_status), "Expected #{expected_status}, got: #{last_response.status}, role: #{role}" + end + end + end + end + end + end + describe 'GET /v2/service_instances/:service_instance_guid/service_keys' do let(:space) { Space.make } let(:manager) { make_manager_for_space(space) } diff --git a/spec/unit/presenters/v2/service_instance_presenter_spec.rb b/spec/unit/presenters/v2/service_instance_presenter_spec.rb index 638abbd6065..811a2d87a33 100644 --- a/spec/unit/presenters/v2/service_instance_presenter_spec.rb +++ b/spec/unit/presenters/v2/service_instance_presenter_spec.rb @@ -35,6 +35,7 @@ module CloudController::Presenters::V2 'relationship_url' => 'http://relationship.example.com', 'service_url' => "/v2/services/#{service_plan.service.guid}", 'shared_from_url' => "/v2/service_instances/#{service_instance.guid}/shared_from", + 'shared_to_url' => "/v2/service_instances/#{service_instance.guid}/shared_to", } ) end diff --git a/spec/unit/presenters/v2/service_instance_shared_to_presenter_spec.rb b/spec/unit/presenters/v2/service_instance_shared_to_presenter_spec.rb new file mode 100644 index 00000000000..a765cc7b26f --- /dev/null +++ b/spec/unit/presenters/v2/service_instance_shared_to_presenter_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +module CloudController::Presenters::V2 + RSpec.describe ServiceInstanceSharedToPresenter do + describe '#to_hash' do + it 'returns the space and org name' do + space = VCAP::CloudController::Space.make + presenter = ServiceInstanceSharedToPresenter.new + expect(presenter.to_hash(space)).to eq( + { + 'space_name' => space.name, + 'organization_name' => space.organization.name, + } + ) + end + end + end +end From fe81c83404fa13954e4f13daf6a0c0884fa555f0 Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Wed, 8 Nov 2017 13:56:03 +0000 Subject: [PATCH 14/32] Add bound_app_count to /v2/service_instances/:guid/shared_to response * The /v2/service_instance/:guid/shared_to response body now includes the 'bound_app_count' key, which shows the number of apps bound to the service instance in each of the spaces that the instance has been shared to. [#151432730] Signed-off-by: Jen Spinney --- .../services/service_instances_controller.rb | 14 ++++-- app/fetchers/service_binding_list_fetcher.rb | 7 +++ .../service_instance_shared_to_presenter.rb | 4 +- ...o_where_service_instance_is_shared_to.html | 22 ++++++++- spec/request/v2/service_instances_spec.rb | 8 ++-- .../service_instances_controller_spec.rb | 31 +++++++++++- ...rvice_instance_shared_to_presenter_spec.rb | 5 +- .../service_binding_list_fetcher_spec.rb | 48 +++++++++++++++++++ 8 files changed, 124 insertions(+), 15 deletions(-) diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 97e2c9294dd..40786a6e6fe 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -5,6 +5,7 @@ require 'controllers/services/lifecycle/service_instance_deprovisioner' require 'controllers/services/lifecycle/service_instance_purger' require 'fetchers/service_instance_fetcher' +require 'fetchers/service_binding_list_fetcher' require 'presenters/v2/service_instance_shared_to_presenter' require 'presenters/v2/service_instance_shared_from_presenter' @@ -232,7 +233,7 @@ def enumerate_shared_to_information(guid) associated_controller = VCAP::CloudController::SpacesController associated_path = "#{self.class.url_for_guid(guid)}/shared_to" - create_paginated_collection_renderer.render_json( + create_paginated_collection_renderer(service_instance).render_json( associated_controller, service_instance.shared_spaces_dataset, associated_path, @@ -346,15 +347,20 @@ def unbind_route(route_guid, instance_guid) private class ServiceInstanceSharedToSerializer + def initialize(service_instance) + @service_instance = service_instance + end + def serialize(controller, space, opts, orphans=nil) - CloudController::Presenters::V2::ServiceInstanceSharedToPresenter.new.to_hash(space) + bound_app_count = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(@service_instance.guid, space.guid).count + CloudController::Presenters::V2::ServiceInstanceSharedToPresenter.new.to_hash(space, bound_app_count) end end - def create_paginated_collection_renderer + def create_paginated_collection_renderer(service_instance) VCAP::CloudController::RestController::PaginatedCollectionRenderer.new( VCAP::CloudController::RestController::SecureEagerLoader.new, - ServiceInstanceSharedToSerializer.new, + ServiceInstanceSharedToSerializer.new(service_instance), { max_results_per_page: config.get(:renderer, :max_results_per_page), default_results_per_page: config.get(:renderer, :default_results_per_page), diff --git a/app/fetchers/service_binding_list_fetcher.rb b/app/fetchers/service_binding_list_fetcher.rb index 2939ccd5eac..fd49728ba31 100644 --- a/app/fetchers/service_binding_list_fetcher.rb +++ b/app/fetchers/service_binding_list_fetcher.rb @@ -16,6 +16,13 @@ def fetch_all filter(dataset) end + def self.fetch_service_instance_bindings_in_space(service_instance_guid, space_guid) + ServiceBinding.select_all(ServiceBinding.table_name). + join(:apps, guid: :app_guid). + where(apps__space_guid: space_guid). + where(service_bindings__service_instance_guid: service_instance_guid) + end + private def filter(dataset) diff --git a/app/presenters/v2/service_instance_shared_to_presenter.rb b/app/presenters/v2/service_instance_shared_to_presenter.rb index 70f927420c7..0f3229579bf 100644 --- a/app/presenters/v2/service_instance_shared_to_presenter.rb +++ b/app/presenters/v2/service_instance_shared_to_presenter.rb @@ -4,8 +4,8 @@ module CloudController module Presenters module V2 class ServiceInstanceSharedToPresenter < ServiceInstanceSharedFromPresenter - def to_hash(space) - super(space) + def to_hash(space, bound_app_count) + super(space).merge({ 'bound_app_count' => bound_app_count }) end end end diff --git a/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_to.html b/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_to.html index d3686b1ae0b..4dfd7a1d103 100644 --- a/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_to.html +++ b/docs/v2/service_instances/retrieve_info_where_service_instance_is_shared_to.html @@ -194,6 +194,22 @@

    Body

    + + + bound_app_count + + + The number of applications bound to the service instance in the space. + + +
      +
    + + +
      +
    + + @@ -205,11 +221,13 @@

    Body

    "resources": [ { "space_name": "target-space-1", - "organization_name": "org-name" + "organization_name": "org-name", + "bound_app_count": 3 }, { "space_name": "target-space-2", - "organization_name": "org-name" + "organization_name": "org-name", + "bound_app_count": 0 } ] } diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb index 733d1e93582..2446c5b6414 100644 --- a/spec/request/v2/service_instances_spec.rb +++ b/spec/request/v2/service_instances_spec.rb @@ -216,7 +216,7 @@ service_instance.add_shared_space(space2) end - it 'returns data about the source space and org' do + it 'returns data about the source space, org, and bound_app_count' do get "v2/service_instances/#{service_instance.guid}/shared_to", nil, admin_headers expect(last_response.status).to eq(200) @@ -231,11 +231,13 @@ 'resources' => [ { 'space_name' => space1.name, - 'organization_name' => space1.organization.name + 'organization_name' => space1.organization.name, + 'bound_app_count' => 0 }, { 'space_name' => space2.name, - 'organization_name' => space2.organization.name + 'organization_name' => space2.organization.name, + 'bound_app_count' => 0 } ] } diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 2ae20afde85..93f3a78af3e 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -3636,14 +3636,41 @@ def verify_forbidden(user) space1_resource = resources.find { |resource| resource['space_name'] == space1.name } space2_resource = resources.find { |resource| resource['space_name'] == space2.name } - expect(space1_resource.keys).to match_array(['space_name', 'organization_name']) - expect(space2_resource.keys).to match_array(['space_name', 'organization_name']) + expect(space1_resource.keys).to match_array(['space_name', 'organization_name', 'bound_app_count']) + expect(space2_resource.keys).to match_array(['space_name', 'organization_name', 'bound_app_count']) expect(space1_resource.fetch('space_name')).to eq(space1.name) expect(space2_resource.fetch('space_name')).to eq(space2.name) expect(space1_resource.fetch('organization_name')).to eq(space1.organization.name) expect(space2_resource.fetch('organization_name')).to eq(space2.organization.name) + + expect(space1_resource.fetch('bound_app_count')).to eq(0) + expect(space2_resource.fetch('bound_app_count')).to eq(0) + end + + context 'when there are apps bound to the shared service instance' do + before do + ServiceBinding.make(service_instance: instance, app: AppModel.make(space: space1)) + ServiceBinding.make(service_instance: instance, app: AppModel.make(space: space1)) + ServiceBinding.make(service_instance: ServiceInstance.make(space: space1), app: AppModel.make(space: space1)) + + ServiceBinding.make(service_instance: instance, app: AppModel.make(space: space2)) + end + + it 'returns the correct bound_app_count' do + set_current_user_as_admin + get "/v2/service_instances/#{instance.guid}/shared_to" + decoded_response = JSON.parse(last_response.body) + expect(last_response.status).to eql(200), last_response.body + resources = decoded_response.fetch('resources') + + space1_resource = resources.find { |resource| resource['space_name'] == space1.name } + space2_resource = resources.find { |resource| resource['space_name'] == space2.name } + + expect(space1_resource.fetch('bound_app_count')).to eq(2) + expect(space2_resource.fetch('bound_app_count')).to eq(1) + end end end diff --git a/spec/unit/presenters/v2/service_instance_shared_to_presenter_spec.rb b/spec/unit/presenters/v2/service_instance_shared_to_presenter_spec.rb index a765cc7b26f..8119cfd3c69 100644 --- a/spec/unit/presenters/v2/service_instance_shared_to_presenter_spec.rb +++ b/spec/unit/presenters/v2/service_instance_shared_to_presenter_spec.rb @@ -3,13 +3,14 @@ module CloudController::Presenters::V2 RSpec.describe ServiceInstanceSharedToPresenter do describe '#to_hash' do - it 'returns the space and org name' do + it 'returns the space name, org name, and bound app count' do space = VCAP::CloudController::Space.make presenter = ServiceInstanceSharedToPresenter.new - expect(presenter.to_hash(space)).to eq( + expect(presenter.to_hash(space, 42)).to eq( { 'space_name' => space.name, 'organization_name' => space.organization.name, + 'bound_app_count' => 42 } ) end diff --git a/spec/unit/queries/service_binding_list_fetcher_spec.rb b/spec/unit/queries/service_binding_list_fetcher_spec.rb index 1e6e825dfaf..3e45c6eb02a 100644 --- a/spec/unit/queries/service_binding_list_fetcher_spec.rb +++ b/spec/unit/queries/service_binding_list_fetcher_spec.rb @@ -90,5 +90,53 @@ module VCAP::CloudController end end end + + describe '#fetch_service_instance_bindings_in_space' do + let(:space) { Space.make } + let(:service_instance) { ServiceInstance.make(space: space) } + + it 'returns a Sequel::Dataset' do + results = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(service_instance.guid, space.guid) + expect(results).to be_a(Sequel::Dataset) + end + + context 'when there are no bindings' do + it 'returns an empty dataset' do + results = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(service_instance.guid, space.guid) + expect(results.count).to eql(0) + end + end + + context 'when a binding exists in a space' do + let!(:service_binding) { ServiceBinding.make(app: AppModel.make(space: space), service_instance: service_instance) } + let!(:other_service_binding) { ServiceBinding.make } + + it 'returns the binding for the correct space' do + results = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(service_instance.guid, space.guid) + expect(results.count).to eql(1) + end + end + + context 'when multiple bindings exist in a space' do + let!(:service_binding1) { ServiceBinding.make(app: AppModel.make(space: space), service_instance: service_instance) } + let!(:service_binding2) { ServiceBinding.make(app: AppModel.make(space: space), service_instance: service_instance) } + let!(:other_service_binding) { ServiceBinding.make } + + it 'returns the bindings for the correct space' do + results = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(service_instance.guid, space.guid) + expect(results.count).to eql(2) + end + end + + context 'when multiple service instances exist' do + let!(:service_binding) { ServiceBinding.make(app: AppModel.make(space: space), service_instance: service_instance) } + let!(:other_service_binding) { ServiceBinding.make(service_instance: ServiceInstance.make(space: space)) } + + it 'returns the binding for the correct service instance' do + results = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(service_instance.guid, space.guid) + expect(results.count).to eql(1) + end + end + end end end From 005b33e0bc2151bfcb799ec737f903c64cbd2425 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Tue, 31 Oct 2017 11:24:21 +0000 Subject: [PATCH 15/32] Validate uniqueness of service instance name to include shared service instances [#152314627] Signed-off-by: Derik Evangelista --- .../services/service_instances_controller.rb | 3 +- ...r_provided_service_instances_controller.rb | 4 +-- app/models/services/service_instance.rb | 23 ++++++++++--- spec/support/matchers/sequel_validations.rb | 2 +- .../service_instances_controller_spec.rb | 34 ++++++++++++++++--- .../services/managed_service_instance_spec.rb | 2 +- .../models/services/service_instance_spec.rb | 34 ++++++++++++++++--- 7 files changed, 81 insertions(+), 21 deletions(-) diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 40786a6e6fe..70c291f26de 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -34,14 +34,13 @@ class ServiceInstancesController < RestController::ModelController define_routes def self.translate_validation_exception(e, attributes) - space_and_name_errors = e.errors.on([:space_id, :name]).to_a quota_errors = e.errors.on(:quota).to_a service_plan_errors = e.errors.on(:service_plan).to_a service_instance_errors = e.errors.on(:service_instance).to_a service_instance_name_errors = e.errors.on(:name).to_a service_instance_tags_errors = e.errors.on(:tags).to_a - if space_and_name_errors.include?(:unique) + if service_instance_name_errors.include?(:unique) return CloudController::Errors::ApiError.new_from_details('ServiceInstanceNameTaken', attributes['name']) elsif quota_errors.include?(:service_instance_space_quota_exceeded) return CloudController::Errors::ApiError.new_from_details('ServiceInstanceSpaceQuotaExceeded') diff --git a/app/controllers/services/user_provided_service_instances_controller.rb b/app/controllers/services/user_provided_service_instances_controller.rb index ad831bb98de..08b99d87fd6 100644 --- a/app/controllers/services/user_provided_service_instances_controller.rb +++ b/app/controllers/services/user_provided_service_instances_controller.rb @@ -29,11 +29,11 @@ def inject_dependencies(dependencies) end def self.translate_validation_exception(e, attributes) - space_and_name_errors = e.errors.on([:space_id, :name]) + name_errors = e.errors.on(:name) service_instance_errors = e.errors.on(:service_instance) service_instance_name_errors = e.errors.on(:name).to_a - if space_and_name_errors&.include?(:unique) + if name_errors&.include?(:unique) CloudController::Errors::ApiError.new_from_details('ServiceInstanceNameTaken', attributes['name']) elsif service_instance_errors&.include?(:space_mismatch) CloudController::Errors::ApiError.new_from_details('ServiceInstanceRouteBindingSpaceMismatch') diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index 02159e1d975..83c1d946ca7 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -87,14 +87,27 @@ def managed_instance? !user_provided_instance? end + def name_clashes + proc do |_, instance| + next if instance.space_id.nil? || instance.name.nil? + + clashes_with_shared_instance_names = + ServiceInstance.select_all(ServiceInstance.table_name). + join(:service_instance_shares, target_space_guid: instance.space_guid). + where(name: instance.name) + + clashes_with_instance_names = + ServiceInstance.select_all(ServiceInstance.table_name). + where(space_id: instance.space_id, name: instance.name) + + clashes_with_shared_instance_names.union(clashes_with_instance_names) + end + end + def validate validates_presence :name validates_presence :space - validates_unique [:space_id, :name], where: (proc do |_, obj, arr| - vals = arr.map { |x| obj.send(x) } - next if vals.any?(&:nil?) - ServiceInstance.where(arr.zip(vals)) - end) + validates_unique :name, where: name_clashes validates_max_length 50, :name validates_max_length 10_000, :syslog_drain_url, allow_nil: true end diff --git a/spec/support/matchers/sequel_validations.rb b/spec/support/matchers/sequel_validations.rb index 7e40682eb99..c4f397a2e07 100644 --- a/spec/support/matchers/sequel_validations.rb +++ b/spec/support/matchers/sequel_validations.rb @@ -48,7 +48,7 @@ duplicate_object[attr] = source_obj[attr] end unless duplicate_object.valid? - errors_key = attributes.length > 1 ? attributes : attributes.first + errors_key = options[:error_key] || (attributes.length > 1 ? attributes : attributes.first) errors = duplicate_object.errors.on(errors_key) expected_error = options[:message] || :unique errors && errors.include?(expected_error) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 93f3a78af3e..8dab830faf9 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -885,6 +885,32 @@ def stub_delete_and_return(status, body) expect(last_response.status).to eq(400) expect(decoded_response['code']).to eq(60002) end + + it 'does not allow a managed service instance with same name as a shared service instance' do + source_space = Space.make(organization: space.organization) + source_space.add_developer(developer) + service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space) + expect(last_response.status).to eq(201) + + service_instance.add_shared_space(space) + + create_managed_service_instance + expect(last_response.status).to eq(400) + expect(decoded_response['code']).to eq(60002) + end + + it 'does not allow a user provided service instance with same name as a shared service instance' do + source_space = Space.make(organization: space.organization) + source_space.add_developer(developer) + service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space) + expect(last_response.status).to eq(201) + + service_instance.add_shared_space(space) + + create_user_provided_service_instance + expect(last_response.status).to eq(400) + expect(decoded_response['code']).to eq(60002) + end end context 'when the service_plan does not exist' do @@ -4003,7 +4029,6 @@ def verify_forbidden(user) let(:errors) { instance_double(Sequel::Model::Errors) } let(:attributes) { {} } - let(:space_and_name_errors) { nil } let(:quota_errors) { nil } let(:service_plan_errors) { nil } let(:service_instance_name_errors) { nil } @@ -4013,7 +4038,6 @@ def verify_forbidden(user) before do allow(e).to receive(:errors).and_return(errors) - allow(errors).to receive(:on).with([:space_id, :name]).and_return(space_and_name_errors) allow(errors).to receive(:on).with(:quota).and_return(quota_errors) allow(errors).to receive(:on).with(:service_plan).and_return(service_plan_errors) allow(errors).to receive(:on).with(:name).and_return(service_instance_name_errors) @@ -4028,7 +4052,6 @@ def verify_forbidden(user) end context "when errors are included but aren't supported validation exceptions" do - let(:space_and_name_errors) { [:stuff] } let(:quota_errors) { [:stuff] } let(:service_plan_errors) { [:stuff] } let(:service_instance_name_errors) { [:stuff] } @@ -4042,7 +4065,7 @@ def verify_forbidden(user) context 'when there is a service instance name taken error' do let(:attributes) { { 'name' => 'test name' } } - let(:space_and_name_errors) { [:unique] } + let(:service_instance_name_errors) { [:unique] } it 'returns a ServiceInstanceNameTaken error' do expect(VCAP::CloudController::ServiceInstancesController.translate_validation_exception(e, attributes).name).to eq('ServiceInstanceNameTaken') @@ -4111,10 +4134,11 @@ def create_managed_service_instance(user_opts={}) arbitrary_params = user_opts.delete(:parameters) accepts_incomplete = user_opts.delete(:accepts_incomplete) { |_| 'true' } tags = user_opts.delete(:tags) + service_instance_space = user_opts.delete(:space) || space body = { name: 'foo', - space_guid: space.guid, + space_guid: service_instance_space.guid, service_plan_guid: plan.guid, } body[:parameters] = arbitrary_params if arbitrary_params diff --git a/spec/unit/models/services/managed_service_instance_spec.rb b/spec/unit/models/services/managed_service_instance_spec.rb index 56d9ea23ce9..8a2dbc54a51 100644 --- a/spec/unit/models/services/managed_service_instance_spec.rb +++ b/spec/unit/models/services/managed_service_instance_spec.rb @@ -32,7 +32,7 @@ module VCAP::CloudController it { is_expected.to validate_presence :name } it { is_expected.to validate_presence :service_plan } it { is_expected.to validate_presence :space } - it { is_expected.to validate_uniqueness [:space_id, :name] } + it { is_expected.to validate_uniqueness :space_id, :name, { error_key: :name } } it { is_expected.to strip_whitespace :name } let(:max_tags) { ['a' * 1024, 'b' * 1024] } diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 52bb0ff5662..5729832b48d 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/spec/unit/models/services/service_instance_spec.rb @@ -85,7 +85,7 @@ module VCAP::CloudController expect { service_instance_foo.set(name: 'bar') service_instance_foo.save_changes - }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/) + }.to raise_error(Sequel::ValidationFailed, /name unique/) end end end @@ -97,13 +97,13 @@ module VCAP::CloudController it 'raises an exception when creating another UserProvidedServiceInstance' do expect { UserProvidedServiceInstance.create(service_instance_attrs) - }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/) + }.to raise_error(Sequel::ValidationFailed, /name unique/) end it 'raises an exception when creating a ManagedServiceInstance' do expect { ManagedServiceInstance.create(service_instance_attrs) - }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/) + }.to raise_error(Sequel::ValidationFailed, /name unique/) end end @@ -116,13 +116,37 @@ module VCAP::CloudController it 'raises an exception when creating another ManagedServiceInstance' do expect { ManagedServiceInstance.create(service_instance_attrs) - }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/) + }.to raise_error(Sequel::ValidationFailed, /name unique/) end it 'raises an exception when creating a UserProvidedServiceInstance' do expect { UserProvidedServiceInstance.create(service_instance_attrs) - }.to raise_error(Sequel::ValidationFailed, /space_id and name unique/) + }.to raise_error(Sequel::ValidationFailed, /name unique/) + end + end + + describe 'when a ManagedServiceInstance has been shared' do + let(:space) { Space.make } + let(:originating_space) { Space.make } + let(:service_instance) { + ManagedServiceInstance.make(name: 'shared-service', space: originating_space) + } + + before do + service_instance.add_shared_space(space) + end + + it 'raises an exception when creating another ManagedServiceInstance' do + expect { + ManagedServiceInstance.make(name: 'shared-service', space: space) + }.to raise_error(Sequel::ValidationFailed, /name unique/) + end + + it 'raises an exception when creating another UserProvidedServiceInstance' do + expect { + UserProvidedServiceInstance.make(name: 'shared-service', space: space) + }.to raise_error(Sequel::ValidationFailed, /name unique/) end end end From f0b8aac25a9ad7d7664a773bdc4d608c1e29d437 Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Tue, 31 Oct 2017 14:51:40 +0000 Subject: [PATCH 16/32] Fix name collisions between shared spaces * This fixes an issue that any share between two spaces results in a shared name pool between spaces. Name collisions should only occur if the service instance has been shared into the space. [#152314627] --- app/models/services/service_instance.rb | 2 +- .../service_instances_controller_spec.rb | 62 +++++++++++++------ 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index 83c1d946ca7..ddf2420d28a 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -93,7 +93,7 @@ def name_clashes clashes_with_shared_instance_names = ServiceInstance.select_all(ServiceInstance.table_name). - join(:service_instance_shares, target_space_guid: instance.space_guid). + join(:service_instance_shares, service_instance_guid: :guid, target_space_guid: instance.space_guid). where(name: instance.name) clashes_with_instance_names = diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 8dab830faf9..f2cdee0e36c 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -886,30 +886,51 @@ def stub_delete_and_return(status, body) expect(decoded_response['code']).to eq(60002) end - it 'does not allow a managed service instance with same name as a shared service instance' do - source_space = Space.make(organization: space.organization) - source_space.add_developer(developer) - service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space) - expect(last_response.status).to eq(201) + context 'when a service instance share exists between spaces' do + let(:source_space) { Space.make(organization: space.organization) } + before do + source_space.add_developer(developer) - service_instance.add_shared_space(space) + service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space) + service_instance.add_shared_space(space) + expect(last_response.status).to eq(201) + end - create_managed_service_instance - expect(last_response.status).to eq(400) - expect(decoded_response['code']).to eq(60002) - end + it 'does not allow a managed service instance with same name as a shared service instance' do + create_managed_service_instance + expect(last_response.status).to eq(400) + expect(decoded_response['code']).to eq(60002) + end - it 'does not allow a user provided service instance with same name as a shared service instance' do - source_space = Space.make(organization: space.organization) - source_space.add_developer(developer) - service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space) - expect(last_response.status).to eq(201) + it 'does not allow a user provided service instance with same name as a shared service instance' do + create_user_provided_service_instance + expect(last_response.status).to eq(400) + expect(decoded_response['code']).to eq(60002) + end - service_instance.add_shared_space(space) + context 'when an unshared instance exists in the source space' do + before do + create_managed_service_instance(accepts_incomplete: 'false', space: source_space, name: 'bar') + expect(last_response.status).to eq(201) + end - create_user_provided_service_instance - expect(last_response.status).to eq(400) - expect(decoded_response['code']).to eq(60002) + it 'allows an instance of the same name to be created in the shared to space' do + create_managed_service_instance(accepts_incomplete: 'false', space: space, name: 'bar') + expect(last_response.status).to eq(201) + end + end + + context 'when an unshared instance exists in the shared to space' do + before do + create_managed_service_instance(accepts_incomplete: 'false', space: space, name: 'bar') + expect(last_response.status).to eq(201) + end + + it 'allows an instance of the same name to be created in the source space' do + create_managed_service_instance(accepts_incomplete: 'false', space: source_space, name: 'bar') + expect(last_response.status).to eq(201) + end + end end end @@ -4135,9 +4156,10 @@ def create_managed_service_instance(user_opts={}) accepts_incomplete = user_opts.delete(:accepts_incomplete) { |_| 'true' } tags = user_opts.delete(:tags) service_instance_space = user_opts.delete(:space) || space + service_instance_name = user_opts.delete(:name) || 'foo' body = { - name: 'foo', + name: service_instance_name, space_guid: service_instance_space.guid, service_plan_guid: plan.guid, } From e4d253fbef63ae236c98c7b0053d19a8f064e945 Mon Sep 17 00:00:00 2001 From: Alex Blease Date: Wed, 1 Nov 2017 15:05:53 +0000 Subject: [PATCH 17/32] Delete service instance should fail when service is shared [#152470931] Signed-off-by: Derik Evangelista --- .../services/service_instances_controller.rb | 20 ++++++++--- spec/request/v2/service_instances_spec.rb | 27 ++++++++++++++ .../fakes/fake_service_broker_v2_client.rb | 10 ++++++ .../service_instances_controller_spec.rb | 36 +++++++++++++++++++ vendor/errors/v2.yml | 5 +++ 5 files changed, 94 insertions(+), 4 deletions(-) diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 70c291f26de..b7050ddb9b7 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -159,11 +159,15 @@ def delete(guid) end validate_access(:delete, service_instance) - has_assocations = has_routes?(service_instance) || - has_bindings?(service_instance) || - has_keys?(service_instance) - association_not_empty! if has_assocations && !recursive_delete? + unless recursive_delete? + has_associations = has_routes?(service_instance) || + has_bindings?(service_instance) || + has_keys?(service_instance) + + association_not_empty! if has_associations + service_is_shared! if has_shares?(service_instance) + end deprovisioner = ServiceInstanceDeprovisioner.new(@services_event_repository, self, logger) delete_job = deprovisioner.deprovision_service_instance(service_instance, accepts_incomplete, async) @@ -461,6 +465,10 @@ def association_not_empty! raise CloudController::Errors::ApiError.new_from_details('AssociationNotEmpty', associations, :service_instances) end + def service_is_shared! + raise CloudController::Errors::ApiError.new_from_details('ServiceIsShared') + end + def space_change_not_allowed! raise CloudController::Errors::ApiError.new_from_details('ServiceInstanceSpaceChangeNotAllowed') end @@ -493,6 +501,10 @@ def has_keys?(service_instance) !service_instance.service_keys.empty? end + def has_shares?(service_instance) + !service_instance.shared_spaces.empty? + end + def space_change_requested?(requested_space_guid, current_space) requested_space_guid && requested_space_guid != current_space.guid end diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb index 2446c5b6414..a3c5f62b727 100644 --- a/spec/request/v2/service_instances_spec.rb +++ b/spec/request/v2/service_instances_spec.rb @@ -244,4 +244,31 @@ ) end end + + describe 'DELETE /v2/service_instance/:guid' do + let(:originating_space) { VCAP::CloudController::Space.make } + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: originating_space) } + + context 'when the service instance has been shared' do + before do + allow(VCAP::Services::ServiceBrokers::V2::Client).to receive(:new) do |*args, **kwargs, &block| + FakeServiceBrokerV2Client.new(*args, **kwargs, &block) + end + + set_current_user_as_admin + service_instance.add_shared_space(space) + end + + it 'fails with an appropriate response' do + delete "v2/service_instances/#{service_instance.guid}", nil, admin_headers + + expect(last_response.status).to eq(400) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted' + expect(parsed_response['error_code']).to eq 'CF-ServiceIsShared' + expect(parsed_response['code']).to eq 10014 + end + end + end end diff --git a/spec/support/fakes/fake_service_broker_v2_client.rb b/spec/support/fakes/fake_service_broker_v2_client.rb index 45ccacbdf04..2e23581aa54 100644 --- a/spec/support/fakes/fake_service_broker_v2_client.rb +++ b/spec/support/fakes/fake_service_broker_v2_client.rb @@ -46,6 +46,16 @@ def provision(_instance, arbitrary_parameters: {}, accepts_incomplete: false) } end + def deprovision(_instance, arbitrary_parameters: {}, accepts_incomplete: false) + { + last_operation: { + type: 'delete', + description: '', + state: 'succeeded' + } + } + end + def bind(_binding, _arbitrary_parameters) { credentials: credentials, diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index f2cdee0e36c..8b585992af1 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -2362,6 +2362,42 @@ def stub_delete_and_return(status, body) end end + context 'when the service instance has been shared' do + let(:originating_space) { Space.make } + let!(:service_instance) { ManagedServiceInstance.make(space: originating_space) } + + before do + service_instance.add_shared_space(space) + end + + it 'does not delete the associated shares' do + delete "/v2/service_instances/#{service_instance.guid}" + + expect(ServiceInstance.find(guid: service_instance.guid)).to be + expect(ServiceInstance.find(guid: service_instance.guid).shared_spaces.length).to eq(1) + end + + it 'should give the user an error' do + delete "/v2/service_instances/#{service_instance.guid}" + + expect(last_response).to have_status_code 400 + expect(last_response.body).to include 'ServiceIsShared' + expect(last_response.body).to include + 'Service instances must be unshared before they can be deleted' + end + + context 'and recursive=true' do + it 'deletes the associated shares' do + expect { + delete "/v2/service_instances/#{service_instance.guid}?recursive=true" + }.to change(ServiceInstance.join(:service_instance_shares, service_instance_guid: :service_instances__guid), :count).by(-1) + + expect(last_response.status).to eq(204) + expect(ServiceInstance.find(guid: service_instance.guid)).to be_nil + end + end + end + context 'with ?accepts_incomplete=true' do before do stub_deprovision(service_instance, body: body, status: status, accepts_incomplete: true) diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index 70d20c87ee8..5af32bb057a 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -88,6 +88,11 @@ http_code: 429 message: "Rate Limit Exceeded" +10014: + name: ServiceIsShared + http_code: 400 + message: "Service instances must be unshared before they can be deleted" + 20001: name: UserInvalid http_code: 400 From 7d0b34a6dda0611ee92dd8bacccf6f99e1f5b0b8 Mon Sep 17 00:00:00 2001 From: Alex Blease Date: Wed, 1 Nov 2017 17:05:47 +0000 Subject: [PATCH 18/32] Better warning when there are bindings on shared service instances [#151710830] Signed-off-by: Derik Evangelista --- .../services/service_instances_controller.rb | 7 ++--- spec/request/v2/service_instances_spec.rb | 4 ++- .../service_instances_controller_spec.rb | 26 +++++++++++++++++-- vendor/errors/v2.yml | 2 +- 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index b7050ddb9b7..33e655910b0 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -161,12 +161,13 @@ def delete(guid) validate_access(:delete, service_instance) unless recursive_delete? + service_is_shared!(service_instance.name) if has_shares?(service_instance) + has_associations = has_routes?(service_instance) || has_bindings?(service_instance) || has_keys?(service_instance) association_not_empty! if has_associations - service_is_shared! if has_shares?(service_instance) end deprovisioner = ServiceInstanceDeprovisioner.new(@services_event_repository, self, logger) @@ -465,8 +466,8 @@ def association_not_empty! raise CloudController::Errors::ApiError.new_from_details('AssociationNotEmpty', associations, :service_instances) end - def service_is_shared! - raise CloudController::Errors::ApiError.new_from_details('ServiceIsShared') + def service_is_shared!(name) + raise CloudController::Errors::ApiError.new_from_details('ServiceIsShared', name) end def space_change_not_allowed! diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb index a3c5f62b727..bf3a47b5745 100644 --- a/spec/request/v2/service_instances_spec.rb +++ b/spec/request/v2/service_instances_spec.rb @@ -265,7 +265,9 @@ expect(last_response.status).to eq(400) parsed_response = MultiJson.load(last_response.body) - expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted' + expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted. ' \ + "Unsharing #{service_instance.name} will automatically delete any bindings " \ + 'that have been made to applications in other spaces' expect(parsed_response['error_code']).to eq 'CF-ServiceIsShared' expect(parsed_response['code']).to eq 10014 end diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 8b585992af1..e70145d8171 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -2382,8 +2382,30 @@ def stub_delete_and_return(status, body) expect(last_response).to have_status_code 400 expect(last_response.body).to include 'ServiceIsShared' - expect(last_response.body).to include - 'Service instances must be unshared before they can be deleted' + expect(last_response.body).to include( + 'Service instances must be unshared before they can be deleted. ' \ + "Unsharing #{service_instance.name} will automatically delete any bindings " \ + 'that have been made to applications in other spaces') + end + + context 'and there are bindings to the shared instance' do + before do + ServiceBinding.make( + app: AppModel.make(space: space), + service_instance: service_instance + ) + end + + it 'should give the user an error' do + delete "/v2/service_instances/#{service_instance.guid}" + + expect(last_response).to have_status_code 400 + expect(last_response.body).to include 'ServiceIsShared' + expect(last_response.body).to include( + 'Service instances must be unshared before they can be deleted. ' \ + "Unsharing #{service_instance.name} will automatically delete any bindings " \ + 'that have been made to applications in other spaces') + end end context 'and recursive=true' do diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index 5af32bb057a..30c1ee40c26 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -91,7 +91,7 @@ 10014: name: ServiceIsShared http_code: 400 - message: "Service instances must be unshared before they can be deleted" + message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces" 20001: name: UserInvalid From 8d3eace2151ef8e0ad92e410157b6b5d04ee83c9 Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Thu, 2 Nov 2017 11:10:14 +0000 Subject: [PATCH 19/32] Improve delete instance when sharing error code * Change error code * Grammar [#151710830] Signed-off-by: Derik Evangelista --- spec/request/v2/service_instances_spec.rb | 4 ++-- .../services/service_instances_controller_spec.rb | 4 ++-- vendor/errors/v2.yml | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb index bf3a47b5745..9823829c38c 100644 --- a/spec/request/v2/service_instances_spec.rb +++ b/spec/request/v2/service_instances_spec.rb @@ -267,9 +267,9 @@ parsed_response = MultiJson.load(last_response.body) expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted. ' \ "Unsharing #{service_instance.name} will automatically delete any bindings " \ - 'that have been made to applications in other spaces' + 'that have been made to applications in other spaces.' expect(parsed_response['error_code']).to eq 'CF-ServiceIsShared' - expect(parsed_response['code']).to eq 10014 + expect(parsed_response['code']).to eq 390002 end end end diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index e70145d8171..cb98fcadc02 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -2385,7 +2385,7 @@ def stub_delete_and_return(status, body) expect(last_response.body).to include( 'Service instances must be unshared before they can be deleted. ' \ "Unsharing #{service_instance.name} will automatically delete any bindings " \ - 'that have been made to applications in other spaces') + 'that have been made to applications in other spaces.') end context 'and there are bindings to the shared instance' do @@ -2404,7 +2404,7 @@ def stub_delete_and_return(status, body) expect(last_response.body).to include( 'Service instances must be unshared before they can be deleted. ' \ "Unsharing #{service_instance.name} will automatically delete any bindings " \ - 'that have been made to applications in other spaces') + 'that have been made to applications in other spaces.') end end diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index 30c1ee40c26..b07dc388e50 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -88,11 +88,6 @@ http_code: 429 message: "Rate Limit Exceeded" -10014: - name: ServiceIsShared - http_code: 400 - message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces" - 20001: name: UserInvalid http_code: 400 @@ -1113,3 +1108,8 @@ name: ServiceInstanceUnshareFailed http_code: 502 message: "Unshare of service instance failed because one or more bindings could not be deleted.\n\n%s" + +390002: + name: ServiceIsShared + http_code: 400 + message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces." From 4109284e5d6f2af8f22ce1afc39a51d679588a6b Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Thu, 2 Nov 2017 13:52:55 +0000 Subject: [PATCH 20/32] Give a more meaningful name for the shared service deletion error code [Finishes #151710830] Signed-off-by: Derik Evangelista --- app/controllers/services/service_instances_controller.rb | 2 +- spec/request/v2/service_instances_spec.rb | 2 +- .../controllers/services/service_instances_controller_spec.rb | 4 ++-- vendor/errors/v2.yml | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 33e655910b0..c3dd190c79b 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -467,7 +467,7 @@ def association_not_empty! end def service_is_shared!(name) - raise CloudController::Errors::ApiError.new_from_details('ServiceIsShared', name) + raise CloudController::Errors::ApiError.new_from_details('ServiceInstanceDeletionSharesExists', name) end def space_change_not_allowed! diff --git a/spec/request/v2/service_instances_spec.rb b/spec/request/v2/service_instances_spec.rb index 9823829c38c..cb4ba70c566 100644 --- a/spec/request/v2/service_instances_spec.rb +++ b/spec/request/v2/service_instances_spec.rb @@ -268,7 +268,7 @@ expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted. ' \ "Unsharing #{service_instance.name} will automatically delete any bindings " \ 'that have been made to applications in other spaces.' - expect(parsed_response['error_code']).to eq 'CF-ServiceIsShared' + expect(parsed_response['error_code']).to eq 'CF-ServiceInstanceDeletionSharesExists' expect(parsed_response['code']).to eq 390002 end end diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index cb98fcadc02..770a26e1077 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -2381,7 +2381,7 @@ def stub_delete_and_return(status, body) delete "/v2/service_instances/#{service_instance.guid}" expect(last_response).to have_status_code 400 - expect(last_response.body).to include 'ServiceIsShared' + expect(last_response.body).to include 'ServiceInstanceDeletionSharesExists' expect(last_response.body).to include( 'Service instances must be unshared before they can be deleted. ' \ "Unsharing #{service_instance.name} will automatically delete any bindings " \ @@ -2400,7 +2400,7 @@ def stub_delete_and_return(status, body) delete "/v2/service_instances/#{service_instance.guid}" expect(last_response).to have_status_code 400 - expect(last_response.body).to include 'ServiceIsShared' + expect(last_response.body).to include 'ServiceInstanceDeletionSharesExists' expect(last_response.body).to include( 'Service instances must be unshared before they can be deleted. ' \ "Unsharing #{service_instance.name} will automatically delete any bindings " \ diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index b07dc388e50..3b4f5e9afcb 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -1110,6 +1110,6 @@ message: "Unshare of service instance failed because one or more bindings could not be deleted.\n\n%s" 390002: - name: ServiceIsShared + name: ServiceInstanceDeletionSharesExists http_code: 400 message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces." From 41ed7d8337c06ae1fbbccc77bd3ee7445622e7ac Mon Sep 17 00:00:00 2001 From: Derik Evangelista Date: Fri, 3 Nov 2017 14:56:27 +0000 Subject: [PATCH 21/32] Services control if their instances are shareable * The broker can return a shareable field as part of the service metadata response to /v2/catalog. [#152540454] Signed-off-by: Sam Gunaratne --- app/actions/service_instance_share.rb | 9 +++- .../services/managed_service_instance.rb | 4 ++ app/models/services/service.rb | 8 ++++ app/models/services/service_instance.rb | 4 ++ spec/request/service_instances_spec.rb | 22 +++++++++ spec/support/fakes/blueprints.rb | 1 + .../actions/service_instance_share_spec.rb | 48 +++++++++++++++++++ .../v3/service_instance_controller_spec.rb | 48 +++++++++---------- .../services/managed_service_instance_spec.rb | 26 ++++++++++ .../models/services/service_instance_spec.rb | 6 +++ spec/unit/models/services/service_spec.rb | 42 ++++++++++++++++ vendor/errors/v2.yml | 5 ++ 12 files changed, 197 insertions(+), 26 deletions(-) diff --git a/app/actions/service_instance_share.rb b/app/actions/service_instance_share.rb index 7defe5c2bb6..d3634f1afa6 100644 --- a/app/actions/service_instance_share.rb +++ b/app/actions/service_instance_share.rb @@ -3,6 +3,12 @@ module VCAP::CloudController class ServiceInstanceShare def create(service_instance, target_spaces, user_audit_info) + if service_instance.managed_instance? + unless service_instance.shareable? + raise CloudController::Errors::ApiError.new_from_details('ServiceShareIsDisabled', service_instance.service.label) + end + end + ServiceInstance.db.transaction do target_spaces.each do |space| service_instance.add_shared_space(space) @@ -10,7 +16,8 @@ def create(service_instance, target_spaces, user_audit_info) end Repositories::ServiceInstanceShareEventRepository.record_share_event( - service_instance, target_spaces.map(&:guid), user_audit_info) + service_instance, target_spaces.map(&:guid), user_audit_info + ) service_instance end end diff --git a/app/models/services/managed_service_instance.rb b/app/models/services/managed_service_instance.rb index 6c27b9626e6..f28c8a35938 100644 --- a/app/models/services/managed_service_instance.rb +++ b/app/models/services/managed_service_instance.rb @@ -103,6 +103,10 @@ def route_service? service.route_service? end + def shareable? + service.shareable? + end + def volume_service? service.volume_service? end diff --git a/app/models/services/service.rb b/app/models/services/service.rb index 5737aa973f0..f9e3fda9d52 100644 --- a/app/models/services/service.rb +++ b/app/models/services/service.rb @@ -129,6 +129,14 @@ def route_service? requires.include?('route_forwarding') end + def shareable? + return false if extra.nil? + metadata = JSON.parse(extra) + metadata && metadata['shareable'] + rescue JSON::ParserError + return false + end + def volume_service? requires.include?('volume_mount') end diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index ddf2420d28a..58c9449b507 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -184,6 +184,10 @@ def route_service? false end + def shareable? + false + end + def volume_service? false end diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index b0376067818..5d98657fb46 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -168,6 +168,28 @@ }) expect(event.metadata['target_space_guids']).to eq([target_space.guid]) end + + context 'when the service offering has shareable false' do + before do + service_instance1.service.extra = { shareable: false }.to_json + service_instance1.service.save + end + + it 'fails to share' do + share_request = { + 'data' => [ + { 'guid' => target_space.guid } + ] + } + + post "/v3/service_instances/#{service_instance1.guid}/relationships/shared_spaces", share_request.to_json, admin_header + + expect(last_response.status).to eq(400) + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response['errors'].first['code']).to eq(390003) + expect(parsed_response['errors'].first['title']).to eq('CF-ServiceShareIsDisabled') + end + end end describe 'DELETE /v3/service_instances/:guid/relationships/shared_spaces/:space-guid' do diff --git a/spec/support/fakes/blueprints.rb b/spec/support/fakes/blueprints.rb index 403c2ed6a90..78d2c49052d 100644 --- a/spec/support/fakes/blueprints.rb +++ b/spec/support/fakes/blueprints.rb @@ -205,6 +205,7 @@ module VCAP::CloudController active { true } service_broker { ServiceBroker.make } description { Sham.description } # remove hack + extra { '{"shareable": true}' } end Service.blueprint(:routing) do diff --git a/spec/unit/actions/service_instance_share_spec.rb b/spec/unit/actions/service_instance_share_spec.rb index 12f938d4c17..673ab809a41 100644 --- a/spec/unit/actions/service_instance_share_spec.rb +++ b/spec/unit/actions/service_instance_share_spec.rb @@ -29,6 +29,54 @@ module VCAP::CloudController expect(Repositories::ServiceInstanceShareEventRepository).to have_received(:record_share_event).with( service_instance, [target_space1.guid, target_space2.guid], user_audit_info) end + + context 'when a share already exists' do + before do + service_instance.add_shared_space(target_space1) + end + + it 'is idempotent' do + shared_instance = service_instance_share.create(service_instance, [target_space1], user_audit_info) + expect(shared_instance.shared_spaces.length).to eq 1 + end + end + + context 'when sharing one space from the list of spaces fails' do + before do + allow(service_instance).to receive(:add_shared_space).with(target_space1).and_call_original + allow(service_instance).to receive(:add_shared_space).with(target_space2).and_raise('db failure') + end + + it 'does not share with any spaces' do + expect { + service_instance_share.create(service_instance, [target_space1, target_space2], user_audit_info) + }.to raise_error('db failure') + + instance = ServiceInstance.find(guid: service_instance.guid) + + expect(instance.shared_spaces.length).to eq 0 + end + + it 'does not audit any share events' do + expect(Repositories::ServiceInstanceShareEventRepository).to_not receive(:record_share_event) + + expect { + service_instance_share.create(service_instance, [target_space1, target_space2], user_audit_info) + }.to raise_error('db failure') + end + end + + context 'when the service does is not shareable' do + before do + allow(service_instance).to receive(:shareable?).and_return(false) + end + + it 'raises an api error' do + expect { + service_instance_share.create(service_instance, [target_space1, target_space2], user_audit_info) + }.to raise_error(CloudController::Errors::ApiError, /Service #{service_instance.service.label} has not enabled service instance sharing/) + end + end end end end diff --git a/spec/unit/controllers/v3/service_instance_controller_spec.rb b/spec/unit/controllers/v3/service_instance_controller_spec.rb index b2025bc5e7f..8733f4ea8a9 100644 --- a/spec/unit/controllers/v3/service_instance_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instance_controller_spec.rb @@ -111,7 +111,7 @@ end describe '#share_service_instance' do - let(:service_instance) { VCAP::CloudController::ServiceInstance.make } + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make } let(:target_space) { VCAP::CloudController::Space.make } let(:target_space2) { VCAP::CloudController::Space.make } let(:service_instance_sharing_enabled) { true } @@ -130,28 +130,39 @@ VCAP::CloudController::FeatureFlag.make(name: 'service_instance_sharing', enabled: service_instance_sharing_enabled, error_message: nil) end - it 'shares the service instance to the target space' do - post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body + it 'calls the service instance share action' do + action = instance_double(VCAP::CloudController::ServiceInstanceShare) + allow(VCAP::CloudController::ServiceInstanceShare).to receive(:new).and_return(action) - expect(response.status).to eq 200 - expect(parsed_body['data'][0]['guid']).to eq(target_space.guid) - expect(service_instance.shared_spaces).to contain_exactly(target_space) + expect(action).to receive(:create).with(service_instance, [target_space], an_instance_of(VCAP::CloudController::UserAuditInfo)) + + post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body end it 'shares the service instance to multiple target spaces' do + action = instance_double(VCAP::CloudController::ServiceInstanceShare) + allow(VCAP::CloudController::ServiceInstanceShare).to receive(:new).and_return(action) + expect(action).to receive(:create).with(service_instance, [target_space, target_space2], an_instance_of(VCAP::CloudController::UserAuditInfo)) + req_body[:data] << { guid: target_space2.guid } post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body - expect(response.status).to eq 200 + end - target_space_guids = [] - parsed_body['data'].each do |item| - target_space_guids << item['guid'] + context 'when the service instance share action errors' do + before do + action = instance_double(VCAP::CloudController::ServiceInstanceShare) + allow(VCAP::CloudController::ServiceInstanceShare).to receive(:new).and_return(action) + + expect(action).to receive(:create).and_raise('boom') end - expect(target_space_guids).to contain_exactly(target_space.guid, target_space2.guid) - expect(service_instance.shared_spaces).to contain_exactly(target_space, target_space2) + it 'returns the error to the user' do + expect { + post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body + }.to raise_error('boom') + end end context 'when the service_instance_sharing feature flag is disabled' do @@ -206,19 +217,6 @@ end end - context 'when the service instance has already been shared with the specified space' do - before do - post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body - end - - it 'returns a 200 and leaves the existing share intact' do - post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body - - expect(response.status).to eq 200 - expect(service_instance.shared_spaces).to include(target_space) - end - end - context 'when the request is malformed' do let(:req_body) { { diff --git a/spec/unit/models/services/managed_service_instance_spec.rb b/spec/unit/models/services/managed_service_instance_spec.rb index 8a2dbc54a51..cffdec7fec9 100644 --- a/spec/unit/models/services/managed_service_instance_spec.rb +++ b/spec/unit/models/services/managed_service_instance_spec.rb @@ -258,6 +258,32 @@ module VCAP::CloudController end end + describe '#shareable?' do + let(:service) { Service.make } + let(:service_instance) { ManagedServiceInstance.make } + + before do + allow(service).to receive(:shareable?).and_return(is_shareable) + allow(service_instance).to receive(:service).and_return(service) + end + + context 'when the service instance is not a shareable' do + let(:is_shareable) { false } + + it 'returns false' do + expect(service_instance).to_not be_shareable + end + end + + context 'when the service instance is shareable' do + let(:is_shareable) { true } + + it 'returns true' do + expect(service_instance).to be_shareable + end + end + end + describe '#as_summary_json' do let(:service) { Service.make(label: 'YourSQL', guid: '9876XZ') } let(:service_plan) { ServicePlan.make(name: 'Gold Plan', guid: '12763abc', service: service) } diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 5729832b48d..5ba7dcd4ead 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/spec/unit/models/services/service_instance_spec.rb @@ -289,6 +289,12 @@ module VCAP::CloudController it { is_expected.to be_bindable } end + describe '#shareable?' do + it 'returns false' do + expect(service_instance.shareable?).to be_falsey + end + end + describe '#as_summary_json' do it 'contains name, guid, and binding count' do instance = VCAP::CloudController::ServiceInstance.make( diff --git a/spec/unit/models/services/service_spec.rb b/spec/unit/models/services/service_spec.rb index 6755b052670..c2a7f037cf3 100644 --- a/spec/unit/models/services/service_spec.rb +++ b/spec/unit/models/services/service_spec.rb @@ -397,6 +397,48 @@ def records(user) end end + describe '#shareable?' do + context 'when the service metadata include shareable true' do + let(:service) { Service.make(extra: '{"shareable":true}') } + + it 'returns true' do + expect(service).to be_shareable + end + end + + context 'when the service metadata include shareable false' do + let(:service) { Service.make(extra: '{"shareable":false}') } + + it 'returns false' do + expect(service).to_not be_shareable + end + end + + context 'when the service does not include the shareable field in metadata' do + let(:service) { Service.make(extra: '{"other-key": "value"}') } + + it 'returns false' do + expect(service).to_not be_shareable + end + end + + context 'when the service metadata is nil' do + let(:service) { Service.make(extra: nil) } + + it 'returns false' do + expect(service).to_not be_shareable + end + end + + context 'when extra contains malformed json' do + let(:service) { Service.make(extra: '{"not-json"}') } + + it 'returns false' do + expect(service).to_not be_shareable + end + end + end + describe '#client' do let(:service) { Service.make(service_broker: ServiceBroker.make) } diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index 3b4f5e9afcb..ee0aa7eca06 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -1113,3 +1113,8 @@ name: ServiceInstanceDeletionSharesExists http_code: 400 message: "Service instances must be unshared before they can be deleted. Unsharing %s will automatically delete any bindings that have been made to applications in other spaces." + +390003: + name: ServiceShareIsDisabled + http_code: 400 + message: "Service %s has not enabled service instance sharing." From 165d8b5a10680561cc9eb131ae09e2c4d7ba00f7 Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Mon, 6 Nov 2017 09:56:41 +0000 Subject: [PATCH 22/32] Updated service instance sharing disabled error msg [#151909779] --- vendor/errors/v2.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index ee0aa7eca06..818bf9bf196 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -1117,4 +1117,4 @@ 390003: name: ServiceShareIsDisabled http_code: 400 - message: "Service %s has not enabled service instance sharing." + message: "The %s service does not support service instance sharing." From 549f8db1d5fb1073ff2c03c11fb9632e06e5e1b2 Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Mon, 6 Nov 2017 13:47:02 +0000 Subject: [PATCH 23/32] Fix unit test for instance sharing msg [#151909779] --- spec/unit/actions/service_instance_share_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/actions/service_instance_share_spec.rb b/spec/unit/actions/service_instance_share_spec.rb index 673ab809a41..f39e2d20561 100644 --- a/spec/unit/actions/service_instance_share_spec.rb +++ b/spec/unit/actions/service_instance_share_spec.rb @@ -74,7 +74,7 @@ module VCAP::CloudController it 'raises an api error' do expect { service_instance_share.create(service_instance, [target_space1, target_space2], user_audit_info) - }.to raise_error(CloudController::Errors::ApiError, /Service #{service_instance.service.label} has not enabled service instance sharing/) + }.to raise_error(CloudController::Errors::ApiError, /The #{service_instance.service.label} service does not support service instance sharing./) end end end From 979a1fa31f269dbe9e512aab751d766f0b43ef80 Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Thu, 9 Nov 2017 13:11:08 +0100 Subject: [PATCH 24/32] Fix unit test flake in service instance sharing test [#152672105] --- spec/unit/controllers/v3/service_instance_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/controllers/v3/service_instance_controller_spec.rb b/spec/unit/controllers/v3/service_instance_controller_spec.rb index 8733f4ea8a9..9fbda3ba7b1 100644 --- a/spec/unit/controllers/v3/service_instance_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instance_controller_spec.rb @@ -142,7 +142,7 @@ it 'shares the service instance to multiple target spaces' do action = instance_double(VCAP::CloudController::ServiceInstanceShare) allow(VCAP::CloudController::ServiceInstanceShare).to receive(:new).and_return(action) - expect(action).to receive(:create).with(service_instance, [target_space, target_space2], an_instance_of(VCAP::CloudController::UserAuditInfo)) + expect(action).to receive(:create).with(service_instance, a_collection_containing_exactly(target_space, target_space2), an_instance_of(VCAP::CloudController::UserAuditInfo)) req_body[:data] << { guid: target_space2.guid } From bf355cf2c8989d690ec4509ff505a4cc0887e312 Mon Sep 17 00:00:00 2001 From: Alex Blease Date: Fri, 3 Nov 2017 10:04:06 +0000 Subject: [PATCH 25/32] Developers without source space access cannot delete a shared service instance * This behaviour already existed, this commit just adds additional tests [#150973376] Signed-off-by: Denise Yu --- .../service_instances_controller_spec.rb | 86 ++++++++++++------- 1 file changed, 53 insertions(+), 33 deletions(-) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 770a26e1077..5659f1786f3 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -2370,32 +2370,7 @@ def stub_delete_and_return(status, body) service_instance.add_shared_space(space) end - it 'does not delete the associated shares' do - delete "/v2/service_instances/#{service_instance.guid}" - - expect(ServiceInstance.find(guid: service_instance.guid)).to be - expect(ServiceInstance.find(guid: service_instance.guid).shared_spaces.length).to eq(1) - end - - it 'should give the user an error' do - delete "/v2/service_instances/#{service_instance.guid}" - - expect(last_response).to have_status_code 400 - expect(last_response.body).to include 'ServiceInstanceDeletionSharesExists' - expect(last_response.body).to include( - 'Service instances must be unshared before they can be deleted. ' \ - "Unsharing #{service_instance.name} will automatically delete any bindings " \ - 'that have been made to applications in other spaces.') - end - - context 'and there are bindings to the shared instance' do - before do - ServiceBinding.make( - app: AppModel.make(space: space), - service_instance: service_instance - ) - end - + context 'as a SpaceDeveloper in source and target space' do it 'should give the user an error' do delete "/v2/service_instances/#{service_instance.guid}" @@ -2406,16 +2381,61 @@ def stub_delete_and_return(status, body) "Unsharing #{service_instance.name} will automatically delete any bindings " \ 'that have been made to applications in other spaces.') end + + it 'associated shares are not deleted' do + delete "/v2/service_instances/#{service_instance.guid}" + + expect(ServiceInstance.find(guid: service_instance.guid)).to be + expect(ServiceInstance.find(guid: service_instance.guid).shared_spaces.length).to eq(1) + end + + context 'and there are bindings to the shared instance' do + before do + ServiceBinding.make( + app: AppModel.make(space: space), + service_instance: service_instance + ) + end + + it 'should give the user an error' do + delete "/v2/service_instances/#{service_instance.guid}" + + expect(last_response).to have_status_code 400 + expect(last_response.body).to include 'ServiceInstanceDeletionSharesExists' + expect(last_response.body).to include( + 'Service instances must be unshared before they can be deleted. ' \ + "Unsharing #{service_instance.name} will automatically delete any bindings " \ + 'that have been made to applications in other spaces.') + end + end + + context 'and recursive=true' do + it 'deletes the associated shares' do + expect { + delete "/v2/service_instances/#{service_instance.guid}?recursive=true" + }.to change(ServiceInstance.join(:service_instance_shares, service_instance_guid: :service_instances__guid), :count).by(-1) + + expect(last_response.status).to eq(204) + expect(ServiceInstance.find(guid: service_instance.guid)).to be_nil + end + end end - context 'and recursive=true' do - it 'deletes the associated shares' do - expect { - delete "/v2/service_instances/#{service_instance.guid}?recursive=true" - }.to change(ServiceInstance.join(:service_instance_shares, service_instance_guid: :service_instances__guid), :count).by(-1) + context 'as a SpaceDeveloper in target space' do + let(:target_space) { Space.make } + let(:tommy) { make_developer_for_space(target_space) } - expect(last_response.status).to eq(204) - expect(ServiceInstance.find(guid: service_instance.guid)).to be_nil + before do + service_instance.add_shared_space(target_space) + set_current_user(tommy, email: 'tommy@example.com') + end + + it 'should give the user an error' do + delete "/v2/service_instances/#{service_instance.guid}" + + expect(last_response).to have_status_code 403 + expect(last_response.body).to include 'CF-NotAuthorized' + expect(last_response.body).to include 'You are not authorized to perform the requested action' end end end From 75142a6567e11c20007b47959770fa40442988ce Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Fri, 3 Nov 2017 15:53:05 +0000 Subject: [PATCH 26/32] Add tests for updating shared service instances * Only developers who have write access to the service instance can perform an update. [#150973390] Signed-off-by: Derik Evangelista --- .../service_instances_controller_spec.rb | 41 ++++++++++++++++++- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 5659f1786f3..7fd3320d021 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -1589,6 +1589,43 @@ def stub_delete_and_return(status, body) end end + context 'when the service instance is shared' do + let(:service_instance) { ManagedServiceInstance.make } + let(:shared_to_space) { Space.make } + let(:body) do + { + tags: [] + }.to_json + end + + before do + service_instance.add_shared_space(shared_to_space) + end + + context 'and a developer in the originating space tries to update the instance' do + it 'updates successfully' do + put "/v2/service_instances/#{service_instance.guid}", body + expect(last_response).to have_status_code 201 + end + end + + context 'and a developer in the shared to space tries to update the instance' do + let(:shared_to_user) { make_developer_for_space(shared_to_space) } + + before do + set_current_user(shared_to_user) + end + + it 'should give the user an error' do + put "/v2/service_instances/#{service_instance.guid}", body + + expect(last_response).to have_status_code 403 + expect(last_response.body).to include 'CF-NotAuthorized' + expect(last_response.body).to include 'You are not authorized to perform the requested action' + end + end + end + describe 'error cases' do context 'when the service instance does not exist' do it 'returns a ServiceInstanceNotFound error' do @@ -2423,11 +2460,11 @@ def stub_delete_and_return(status, body) context 'as a SpaceDeveloper in target space' do let(:target_space) { Space.make } - let(:tommy) { make_developer_for_space(target_space) } + let(:target_space_dev) { make_developer_for_space(target_space) } before do service_instance.add_shared_space(target_space) - set_current_user(tommy, email: 'tommy@example.com') + set_current_user(target_space_dev) end it 'should give the user an error' do From dc05963833396b04c7ccc66c25bf299032cc87aa Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Thu, 9 Nov 2017 14:01:08 +0000 Subject: [PATCH 27/32] Return 403 when recipient of shared service instance attempts to share or unshare [#151441010] Signed-off-by: Denise Yu --- .../v3/service_instances_controller.rb | 14 +++++++-- .../v3/service_instance_controller_spec.rb | 31 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 1aef6c27de5..a3dd03dc0e9 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -30,7 +30,7 @@ def share_service_instance service_instance = ServiceInstance.first(guid: params[:service_instance_guid]) - resource_not_found!(:service_instance) unless service_instance && can_read_space?(service_instance.space) + resource_not_found!(:service_instance) unless service_instance && can_read_service_instance?(service_instance) unauthorized! unless can_write_space?(service_instance.space) message = VCAP::CloudController::ToManyRelationshipMessage.create_from_http_request(params[:body]) @@ -52,7 +52,7 @@ def unshare_service_instance service_instance = ServiceInstance.first(guid: params[:service_instance_guid]) - resource_not_found!(:service_instance) unless service_instance && can_read_space?(service_instance.space) + resource_not_found!(:service_instance) unless service_instance && can_read_service_instance?(service_instance) unauthorized! unless can_write_space?(service_instance.space) space_guid = params[:space_guid] @@ -93,8 +93,16 @@ def check_spaces_exist_and_are_readable!(request_guids, found_spaces) end end + def can_read_service_instance?(service_instance) + readable_spaces = service_instance.shared_spaces + [service_instance.space] + + readable_spaces.any? do |space| + can_read?(space.guid, space.organization_guid) + end + end + def can_read_space?(space) - can_read?(space.guid, space.organization_guid) + can_read?(space.guid, space.organization.guid) end def can_write_space?(space) diff --git a/spec/unit/controllers/v3/service_instance_controller_spec.rb b/spec/unit/controllers/v3/service_instance_controller_spec.rb index 9fbda3ba7b1..cb08319882d 100644 --- a/spec/unit/controllers/v3/service_instance_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instance_controller_spec.rb @@ -230,6 +230,25 @@ end end + context 'when the user has access to the service instance through a share' do + before do + service_instance.add_shared_space(target_space) + set_current_user_as_role(role: 'space_developer', org: target_space.organization, space: target_space, user: user) + + outer_space = VCAP::CloudController::Space.make + req_body[:data] = [{ guid: outer_space.guid }] + end + + after do + service_instance.remove_shared_space(target_space) + end + + it 'cannot share the service instance into another space' do + post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body + expect(response.status).to eq 403 + end + end + describe 'permissions by role' do context 'when the user is a space developer in the source space' do before do @@ -377,6 +396,18 @@ end end + context 'when the user has access to the service instance through a share' do + before do + service_instance.add_shared_space(target_space) + set_current_user_as_role(role: 'space_developer', org: target_space.organization, space: target_space, user: user) + end + + it 'cannot unshare the service instance from another space' do + delete :unshare_service_instance, service_instance_guid: service_instance.guid, space_guid: target_space.guid + expect(response.status).to eq 403 + end + end + describe 'permissions by role' do role_to_expected_http_response = { 'admin' => 204, From b197a7fb3c90e08fb07d5032817b835a48d189d2 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Thu, 9 Nov 2017 15:07:17 +0000 Subject: [PATCH 28/32] Tests that users cannot create service keys from shared instances * The intended behavior is that users who have access to a shared service instance, but not developer access to the originating space of the instance may not create service keys from the instance. * This behavior was already correct. This commit simply adds tests to lock down the behavior. [#152592943] Signed-off-by: Jen Spinney --- .../services/service_keys_controller_spec.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/unit/controllers/services/service_keys_controller_spec.rb b/spec/unit/controllers/services/service_keys_controller_spec.rb index d5ef8692e94..4378bd8267f 100644 --- a/spec/unit/controllers/services/service_keys_controller_spec.rb +++ b/spec/unit/controllers/services/service_keys_controller_spec.rb @@ -387,6 +387,30 @@ def bind_url_regex(opts={}) expect(a_request(:put, url_regex).with(body: hash_including(expected_body))).to have_been_made end end + + context 'when the service instance has been shared' do + let(:other_space) { Space.make } + + before do + instance.add_shared_space(other_space) + end + + context 'when the user is a space developer in the service instance space' do + it 'returns successfully' do + post '/v2/service_keys', req + expect(last_response).to have_status_code(201) + end + end + + context 'when the user does not have access to the service instance space' do + let(:developer) { make_developer_for_space(other_space) } + + it 'returns a 403' do + post '/v2/service_keys', req + expect(last_response).to have_status_code(403) + end + end + end end context 'for a user-provided service instance' do From ea6e67c600f628e8c4eec451b3e65c33e9e254dd Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Thu, 9 Nov 2017 15:37:02 +0000 Subject: [PATCH 29/32] Tests that users cannot list service keys from shared instances * The intended behavior is that users who have access to a shared service instance, but not developer access to the originating space of the instance may not list service keys for the instance. * This behavior was already correct. This commit simply adds tests to lock down the behavior. [#151950132] Signed-off-by: Denise Yu --- .../services/service_instances_controller_spec.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 7fd3320d021..020eee3c66a 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -3932,6 +3932,16 @@ def verify_forbidden(user) it 'returns the forbidden code for auditors' do verify_forbidden auditor end + + context 'when user is a developer in space to which the instance was shared' do + before do + instance.add_shared_space(space) + end + + it 'returns the forbidden code' do + verify_forbidden developer + end + end end context 'when the user is a member of the space this instance exists in' do From 64081e420989fb2c931f4ec20e3d31ba7cbc29f6 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Thu, 9 Nov 2017 16:15:41 +0000 Subject: [PATCH 30/32] Tests that users cannot delete service keys from shared instances * The intended behavior is that users who have access to a shared service instance, but not developer access to the originating space of the instance may not delete service key associated with the instance. * This behavior was already correct. This commit simply adds tests to lock down the behavior. [#152721273] Signed-off-by: Jen Spinney --- .../services/service_keys_controller_spec.rb | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/spec/unit/controllers/services/service_keys_controller_spec.rb b/spec/unit/controllers/services/service_keys_controller_spec.rb index 4378bd8267f..c85cf2ef528 100644 --- a/spec/unit/controllers/services/service_keys_controller_spec.rb +++ b/spec/unit/controllers/services/service_keys_controller_spec.rb @@ -553,10 +553,11 @@ def verify_not_found_response(service_key_guid) describe 'DELETE', '/v2/service_keys/:service_key_guid' do let(:service_key) { ServiceKey.make } - let(:developer) { make_developer_for_space(service_key.service_instance.space) } + let(:instance) { service_key.service_instance } + let(:developer) { make_developer_for_space(instance.space) } before do - stub_requests(service_key.service_instance.service.service_broker) + stub_requests(instance.service.service_broker) set_current_user(developer, email: 'example@example.com') end @@ -567,8 +568,8 @@ def verify_not_found_response(service_key_guid) end context 'Not authorized to perform delete operation' do - let(:manager) { make_manager_for_space(service_key.service_instance.space) } - let(:auditor) { make_auditor_for_space(service_key.service_instance.space) } + let(:manager) { make_manager_for_space(instance.space) } + let(:auditor) { make_auditor_for_space(instance.space) } it 'SpaceManager role can not delete a service key' do set_current_user(manager) @@ -581,6 +582,20 @@ def verify_not_found_response(service_key_guid) delete "/v2/service_keys/#{service_key.guid}" verify_not_found_response(service_key.guid) end + + context 'when the user is a developer in a space to which the service instance is shared' do + let(:other_space) { Space.make } + let(:developer) { make_developer_for_space(other_space) } + + before do + instance.add_shared_space(other_space) + end + + it 'is reports the key as not found' do + delete "/v2/service_keys/#{service_key.guid}" + verify_not_found_response(service_key.guid) + end + end end it 'returns ServiceKeyNotFound error if there is no such key' do From dd866d23a248c13755627a9e4f572c6f6e9aec94 Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Thu, 9 Nov 2017 16:24:33 +0000 Subject: [PATCH 31/32] Tests that users cannot GET specific service keys from shared instances * The intended behavior is that users who have access to a shared service instance, but not developer access to the originating space of the instance may not GET specific service keys for the instance. * This behavior was already correct. This commit simply adds tests to lock down the behavior. [#152721273] Signed-off-by: Denise Yu --- .../services/service_keys_controller_spec.rb | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/spec/unit/controllers/services/service_keys_controller_spec.rb b/spec/unit/controllers/services/service_keys_controller_spec.rb index c85cf2ef528..d14a0a0b8ee 100644 --- a/spec/unit/controllers/services/service_keys_controller_spec.rb +++ b/spec/unit/controllers/services/service_keys_controller_spec.rb @@ -480,8 +480,8 @@ def verify_not_found_response(service_key_guid) end context 'Not authorized to perform get operation' do - let(:manager) { make_manager_for_space(service_key.service_instance.space) } - let(:auditor) { make_auditor_for_space(service_key.service_instance.space) } + let(:manager) { make_manager_for_space(instance.space) } + let(:auditor) { make_auditor_for_space(instance.space) } it 'SpaceManager role can not get a service key' do set_current_user(manager) @@ -494,6 +494,20 @@ def verify_not_found_response(service_key_guid) get "/v2/service_keys/#{service_key.guid}" verify_not_found_response(service_key.guid) end + + context 'when the user is a developer in a space to which the service instance is shared' do + let(:other_space) { Space.make } + let(:developer) { make_developer_for_space(other_space) } + + before do + instance.add_shared_space(other_space) + end + + it 'is reports the key as not found' do + get "/v2/service_keys/#{service_key.guid}" + verify_not_found_response(service_key.guid) + end + end end context 'when the key is a CredHub reference' do From b0eccff1e782b25d45da21e79a40041f8ac1d88c Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Fri, 10 Nov 2017 11:22:56 +0000 Subject: [PATCH 32/32] Ensure service instances cannot be shared back to their own space * Raise 422 when source space is included in list of target spaces * Minor refactor to check_spaces_are_writable [#152109683] Signed-off-by: Denise Yu --- .../v3/service_instances_controller.rb | 7 +++++- .../actions/service_instance_share_spec.rb | 25 +++++++++++++++++++ .../v3/service_instance_controller_spec.rb | 17 +++++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index a3dd03dc0e9..525dc08a654 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -39,6 +39,7 @@ def share_service_instance spaces = Space.where(guid: message.guids) check_spaces_exist_and_are_readable!(message.guids, spaces) check_spaces_are_writeable!(spaces) + ensure_not_sharing_to_self!(service_instance.space, spaces) share = ServiceInstanceShare.new share.create(service_instance, spaces, user_audit_info) @@ -70,12 +71,16 @@ def unshare_service_instance private + def ensure_not_sharing_to_self!(service_instance_space, target_spaces) + unprocessable!('Service instances cannot be shared into the space where they were created') if target_spaces.include?(service_instance_space) + end + def check_spaces_are_writeable!(spaces) unwriteable_spaces = spaces.reject do |space| can_write?(space.guid) end - unauthorized! unless unwriteable_spaces.empty? + unauthorized! if unwriteable_spaces.any? end def check_spaces_exist_and_are_readable!(request_guids, found_spaces) diff --git a/spec/unit/actions/service_instance_share_spec.rb b/spec/unit/actions/service_instance_share_spec.rb index f39e2d20561..b63b54293b9 100644 --- a/spec/unit/actions/service_instance_share_spec.rb +++ b/spec/unit/actions/service_instance_share_spec.rb @@ -66,6 +66,31 @@ module VCAP::CloudController end end + context 'when source space is included in list of target spaces' do + before do + allow(service_instance).to receive(:add_shared_space).with(target_space1).and_call_original + allow(service_instance).to receive(:add_shared_space).with(service_instance.space).and_raise('db failure') + end + + it 'does not share with any spaces' do + expect { + service_instance_share.create(service_instance, [target_space1, service_instance.space], user_audit_info) + }.to raise_error('db failure') + + instance = ServiceInstance.find(guid: service_instance.guid) + + expect(instance.shared_spaces.length).to eq 0 + end + + it 'does not audit any share events' do + expect(Repositories::ServiceInstanceShareEventRepository).to_not receive(:record_share_event) + + expect { + service_instance_share.create(service_instance, [target_space1, service_instance.space], user_audit_info) + }.to raise_error('db failure') + end + end + context 'when the service does is not shareable' do before do allow(service_instance).to receive(:shareable?).and_return(false) diff --git a/spec/unit/controllers/v3/service_instance_controller_spec.rb b/spec/unit/controllers/v3/service_instance_controller_spec.rb index cb08319882d..8e170faf627 100644 --- a/spec/unit/controllers/v3/service_instance_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instance_controller_spec.rb @@ -217,6 +217,23 @@ end end + context 'when the source space is contained in the list of target spaces' do + before do + req_body[:data] = [ + { guid: service_instance.space.guid }, + { guid: target_space.guid } + ] + end + + it 'does not share into any spaces and returns an error message' do + post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body + + expect(response.status).to eq 422 + expect(response.body).to include('Service instances cannot be shared into the space where they were created') + expect(service_instance.shared_spaces).to be_empty + end + end + context 'when the request is malformed' do let(:req_body) { {