From d9bd5f2e546d4fe6e6c6830d4944f6dd22dda125 Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Thu, 14 Dec 2017 10:43:05 +0000 Subject: [PATCH 1/7] Give better error message when share command fails for multiple reasons * If a user tries to share a service instance to multiple spaces at the same time and there are different problems with different potential target spaces, we now return a more detailed error message explainig which target spaces failed for which reason. * If a user tries to share a service instance into a target space for which they have write access, we now return a 422 instaed of a 403 for simplicity and API consistency. [#153629300] Signed-off-by: Alex Blease --- .../v3/service_instances_controller.rb | 41 +++++---- .../v3/service_instances_controller_spec.rb | 85 ++++++++++++++++--- 2 files changed, 99 insertions(+), 27 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index ca347792f8b..d15fa051ede 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -37,8 +37,7 @@ def share_service_instance unprocessable!(message.errors.full_messages) unless message.valid? spaces = Space.where(guid: message.guids) - check_spaces_exist_and_are_readable!(message.guids, spaces) - check_spaces_are_writeable!(spaces) + check_spaces_exist_and_are_writeable!(service_instance, message.guids, spaces) share = ServiceInstanceShare.new share.create(service_instance, spaces, user_audit_info) @@ -76,26 +75,38 @@ def relationships_shared_spaces private - def check_spaces_are_writeable!(spaces) - unwriteable_spaces = spaces.reject do |space| + def check_spaces_exist_and_are_writeable!(service_instance, request_guids, found_spaces) + unreadable_space_guids = request_guids - found_spaces.map(&:guid) + + unreadable_spaces = found_spaces.reject do |space| + can_read_space?(space) + end + + unreadable_space_guids += unreadable_spaces.map(&:guid) + + unwriteable_spaces = found_spaces.reject do |space| can_write?(space.guid) end - unauthorized! unless unwriteable_spaces.empty? - end + unwriteable_space_guids = unwriteable_spaces.map(&:guid) - def check_spaces_exist_and_are_readable!(request_guids, found_spaces) - missing_guids = request_guids - found_spaces.map(&:guid) + unless unreadable_space_guids.empty? && unwriteable_space_guids.empty? + unreadable_guid_list = unreadable_space_guids.map { |g| "'#{g}'" }.join(', ') + unwriteable_guid_list = unwriteable_space_guids.map { |s| "'#{s}'" }.join(', ') - unreadable_spaces = found_spaces.reject do |space| - can_read_space?(space) - end + error_msg = '' + + unless unreadable_guid_list.empty? + error_msg += "Unable to share service instance #{service_instance.name} with spaces [#{unreadable_guid_list}]. Ensure the spaces exist and that you have access to them." + end - missing_guids += unreadable_spaces.map(&:guid) + unless unwriteable_guid_list.empty? + error_msg += "\n" unless unreadable_guid_list.empty? + error_msg += "Unable to share service instance #{service_instance.name} with spaces [#{unwriteable_guid_list}]. " + error_msg += 'Write permission is required in order to share a service instance with a space.' + end - unless missing_guids.empty? - guid_list = missing_guids.map { |g| "'#{g}'" }.join(', ') - unprocessable!("Unable to share to spaces [#{guid_list}] for the service instance. Ensure the spaces exist and you have access to them.") + unprocessable!(error_msg) end end diff --git a/spec/unit/controllers/v3/service_instances_controller_spec.rb b/spec/unit/controllers/v3/service_instances_controller_spec.rb index 8cc57246540..d9cb1468e38 100644 --- a/spec/unit/controllers/v3/service_instances_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instances_controller_spec.rb @@ -334,11 +334,11 @@ it 'returns a 422' 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('Unable to share to spaces') - expect(response.body).to include('nonexistant-space-guid') + expect(response.body).to include("Unable to share service instance #{service_instance.name} with spaces ['nonexistant-space-guid']. ") + expect(response.body).to include('Ensure the spaces exist and that you have access to them.') + expect(response.body).not_to include('Write permission is required in order to share a service instance with a space.') end end - context 'when multiple target spaces do not exist' do before do req_body[:data] = [ @@ -351,10 +351,71 @@ it 'does not share to any of the valid target spaces and returns a 422' 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('Unable to share to spaces') - expect(response.body).to include('nonexistant-space-guid') - expect(response.body).to include('nonexistant-space-guid2') - expect(service_instance.shared_spaces).to_not include(target_space) + expect(response.body).to include("Unable to share service instance #{service_instance.name} with spaces ['nonexistant-space-guid', 'nonexistant-space-guid2']. ") + expect(response.body).to include('Ensure the spaces exist and that you have access to them.') + expect(response.body).not_to include('Write permission is required in order to share a service instance with a space.') + end + end + + context 'when the user is a SpaceAuditor in the target space' do + before do + set_current_user_as_role(role: 'space_developer', org: source_space.organization, space: source_space, user: user) + set_current_user_as_role(role: 'space_auditor', org: target_space.organization, space: target_space, user: user) + end + + it 'returns a 422' 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("Unable to share service instance #{service_instance.name} with spaces ['#{target_space.guid}']. ") + expect(response.body).to include('Write permission is required in order to share a service instance with a space.') + expect(response.body).not_to include('Ensure the spaces exist and that you have access to them.') + end + end + + context 'when some target spaces are unreadable and some are unwriteable' do + before do + set_current_user_as_role(role: 'space_developer', org: source_space.organization, space: source_space, user: user) + set_current_user_as_role(role: 'space_auditor', org: target_space.organization, space: target_space, user: user) + + req_body[:data] = [ + { guid: 'nonexistant-space-guid' }, + { guid: target_space.guid } + ] + end + + it 'returns a 422' 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( + "Unable to share service instance #{service_instance.name} with spaces ['nonexistant-space-guid']. Ensure the spaces exist and that you have access to them.\\n" \ + "Unable to share service instance #{service_instance.name} with spaces ['#{target_space.guid}']. "\ + 'Write permission is required in order to share a service instance with a space.' + ) + end + end + + context 'when the user is a SpaceAuditor in mulitple target spaces' do + let(:req_body) do + { + data: [ + { guid: target_space.guid }, + { guid: target_space2.guid } + ] + } + end + + before do + set_current_user_as_role(role: 'space_developer', org: source_space.organization, space: source_space, user: user) + set_current_user_as_role(role: 'space_auditor', org: target_space.organization, space: target_space, user: user) + set_current_user_as_role(role: 'space_auditor', org: target_space2.organization, space: target_space2, user: user) + end + + it 'returns a 422' 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( + "Unable to share service instance #{service_instance.name} with spaces ['#{target_space.guid}', '#{target_space2.guid}']. "\ + 'Write permission is required in order to share a service instance with a space.') end end @@ -399,11 +460,11 @@ role_to_expected_http_response = { 'admin' => 200, 'space_developer' => 200, - 'admin_read_only' => 403, - 'global_auditor' => 403, - 'space_manager' => 403, - 'space_auditor' => 403, - 'org_manager' => 403, + 'admin_read_only' => 422, + 'global_auditor' => 422, + 'space_manager' => 422, + 'space_auditor' => 422, + 'org_manager' => 422, 'org_auditor' => 422, 'org_billing_manager' => 422, }.freeze From 067a1ff3ff43e8c3650423f424f052555f6daaaf Mon Sep 17 00:00:00 2001 From: Alex Blease Date: Thu, 14 Dec 2017 15:55:05 +0000 Subject: [PATCH 2/7] Don't include unreadable_space_guids in unwriteable_space_guids list [#153629300] Signed-off-by: Jen Spinney --- .../v3/service_instances_controller.rb | 8 +++----- .../v3/service_instances_controller_spec.rb | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index d15fa051ede..90c1342090d 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -76,18 +76,16 @@ def relationships_shared_spaces private def check_spaces_exist_and_are_writeable!(service_instance, request_guids, found_spaces) - unreadable_space_guids = request_guids - found_spaces.map(&:guid) - unreadable_spaces = found_spaces.reject do |space| can_read_space?(space) end - unreadable_space_guids += unreadable_spaces.map(&:guid) - unwriteable_spaces = found_spaces.reject do |space| - can_write?(space.guid) + can_write_space?(space) || unreadable_spaces.include?(space) end + unreadable_space_guids = request_guids - found_spaces.map(&:guid) + unreadable_space_guids += unreadable_spaces.map(&:guid) unwriteable_space_guids = unwriteable_spaces.map(&:guid) unless unreadable_space_guids.empty? && unwriteable_space_guids.empty? diff --git a/spec/unit/controllers/v3/service_instances_controller_spec.rb b/spec/unit/controllers/v3/service_instances_controller_spec.rb index d9cb1468e38..049fc15b7ca 100644 --- a/spec/unit/controllers/v3/service_instances_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instances_controller_spec.rb @@ -339,6 +339,21 @@ expect(response.body).not_to include('Write permission is required in order to share a service instance with a space.') end end + + context 'when the user does not have read access to the target space' do + before do + set_current_user_as_role(role: 'space_developer', org: source_space.organization, space: source_space, user: user) + end + + it 'returns a 422' 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("Unable to share service instance #{service_instance.name} with spaces ['#{target_space.guid}']. ") + expect(response.body).to include('Ensure the spaces exist and that you have access to them.') + expect(response.body).not_to include('Write permission is required in order to share a service instance with a space.') + end + end + context 'when multiple target spaces do not exist' do before do req_body[:data] = [ From a39eb2d42a1db63737afa74d76619d114c719cc6 Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Thu, 14 Dec 2017 17:00:43 +0000 Subject: [PATCH 3/7] Space guids in error message can be in any order [#153629300] Signed-off-by: Alex Blease --- .../controllers/v3/service_instances_controller_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/unit/controllers/v3/service_instances_controller_spec.rb b/spec/unit/controllers/v3/service_instances_controller_spec.rb index 049fc15b7ca..94e10d0e742 100644 --- a/spec/unit/controllers/v3/service_instances_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instances_controller_spec.rb @@ -428,9 +428,10 @@ it 'returns a 422' 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( - "Unable to share service instance #{service_instance.name} with spaces ['#{target_space.guid}', '#{target_space2.guid}']. "\ - 'Write permission is required in order to share a service instance with a space.') + expect(response.body).to include(target_space.guid) + expect(response.body).to include(target_space2.guid) + expect(response.body).to include("Unable to share service instance #{service_instance.name} with spaces ") + expect(response.body).to include('Write permission is required in order to share a service instance with a space.') end end From 22ae5694572e6a2d002f0230c9d1bfb8d24b9763 Mon Sep 17 00:00:00 2001 From: Jen Spinney Date: Wed, 27 Dec 2017 12:23:12 +0100 Subject: [PATCH 4/7] Refactor validation method to make error msg logic clearer [#153629300] --- .../v3/service_instances_controller.rb | 46 ++++++++++--------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 90c1342090d..a1a4211f649 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -76,35 +76,37 @@ def relationships_shared_spaces private def check_spaces_exist_and_are_writeable!(service_instance, request_guids, found_spaces) - unreadable_spaces = found_spaces.reject do |space| - can_read_space?(space) - end - - unwriteable_spaces = found_spaces.reject do |space| - can_write_space?(space) || unreadable_spaces.include?(space) - end + unreadable_spaces = found_spaces.reject { |s| can_read_space?(s) } + unwriteable_spaces = found_spaces.reject { |s| can_write_space?(s) || unreadable_spaces.include?(s) } - unreadable_space_guids = request_guids - found_spaces.map(&:guid) - unreadable_space_guids += unreadable_spaces.map(&:guid) + not_found_space_guids = request_guids - found_spaces.map(&:guid) + unreadable_space_guids = not_found_space_guids + unreadable_spaces.map(&:guid) unwriteable_space_guids = unwriteable_spaces.map(&:guid) - unless unreadable_space_guids.empty? && unwriteable_space_guids.empty? - unreadable_guid_list = unreadable_space_guids.map { |g| "'#{g}'" }.join(', ') - unwriteable_guid_list = unwriteable_space_guids.map { |s| "'#{s}'" }.join(', ') + if unreadable_space_guids.any? || unwriteable_space_guids.any? + unreadable_error = unreadable_error_message(service_instance.name, unreadable_space_guids) + unwriteable_error = unwriteable_error_message(service_instance.name, unwriteable_space_guids) - error_msg = '' + error_msg = [unreadable_error, unwriteable_error].map(&:presence).compact.join("\n") - unless unreadable_guid_list.empty? - error_msg += "Unable to share service instance #{service_instance.name} with spaces [#{unreadable_guid_list}]. Ensure the spaces exist and that you have access to them." - end + unprocessable!(error_msg) + end + end - unless unwriteable_guid_list.empty? - error_msg += "\n" unless unreadable_guid_list.empty? - error_msg += "Unable to share service instance #{service_instance.name} with spaces [#{unwriteable_guid_list}]. " - error_msg += 'Write permission is required in order to share a service instance with a space.' - end + def unreadable_error_message(service_instance_name, unreadable_space_guids) + if unreadable_space_guids.any? + unreadable_guid_list = unreadable_space_guids.map { |g| "'#{g}'" }.join(', ') - unprocessable!(error_msg) + "Unable to share service instance #{service_instance_name} with spaces [#{unreadable_guid_list}]. Ensure the spaces exist and that you have access to them." + end + end + + def unwriteable_error_message(service_instance_name, unwriteable_space_guids) + if unwriteable_space_guids.any? + unwriteable_guid_list = unwriteable_space_guids.map { |s| "'#{s}'" }.join(', ') + + "Unable to share service instance #{service_instance_name} with spaces [#{unwriteable_guid_list}]. "\ + 'Write permission is required in order to share a service instance with a space.' end end From 870270c2b73118b0ab3f608e09113f05914bb779 Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Thu, 14 Dec 2017 17:07:45 +0000 Subject: [PATCH 5/7] More specific error message when updating shared service instances [#152552023] Signed-off-by: Alex Blease --- .../services/service_instances_controller.rb | 7 +++ .../access/service_instance_access_spec.rb | 8 +++ .../service_instances_controller_spec.rb | 62 +++++++++++++++++-- vendor/errors/v2.yml | 5 ++ 4 files changed, 76 insertions(+), 6 deletions(-) diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index b53080e272f..95752d2ee2b 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -119,6 +119,7 @@ def update(guid) raise CloudController::Errors::ApiError.new_from_details('UserProvidedServiceInstanceHandlerNeeded') end + validate_shared_space_updateable(service_instance) validate_access(:read_for_update, service_instance) validate_access(:update, projected_service_instance(service_instance)) @@ -449,6 +450,12 @@ def validate_name_update(service_instance) end end + def validate_shared_space_updateable(service_instance) + if @access_context.can?(:read, service_instance) && @access_context.cannot?(:read, service_instance.space) + raise CloudController::Errors::ApiError.new_from_details('SharedServiceInstanceNotUpdateableInTargetSpace') + end + end + def invalid_service_instance!(service_instance) raise Sequel::ValidationFailed.new(service_instance) end diff --git a/spec/unit/access/service_instance_access_spec.rb b/spec/unit/access/service_instance_access_spec.rb index 7ed67887254..82c041004a2 100644 --- a/spec/unit/access/service_instance_access_spec.rb +++ b/spec/unit/access/service_instance_access_spec.rb @@ -177,6 +177,10 @@ module VCAP::CloudController it 'returns false for purge' do expect(subject).not_to allow_op_on_object(:purge, service_instance) end + + it 'does not allow the user to update the service' do + expect(subject).not_to allow_op_on_object(:update, service_instance) + end end context 'when the space of the service instance is not visible' do @@ -203,6 +207,10 @@ module VCAP::CloudController it 'returns false for purge' do expect(subject).not_to allow_op_on_object(:purge, service_instance) end + + it 'does not allow the user to update the service' do + expect(subject).not_to allow_op_on_object(:update, service_instance) + 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 8d91820442a..de1f0f8d9ca 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -1771,12 +1771,44 @@ def stub_delete_and_return(status, body) set_current_user(target_space_developer) 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 'SharedServiceInstanceNotUpdateableInTargetSpace' + expect(last_response.body).to include 'You cannot update service instances that have been shared with you' + end + end + + context 'and an auditor in the target space tries to update the instance' do + let(:target_space_auditor) { make_auditor_for_space(target_space) } + + before do + set_current_user(target_space_auditor) + 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 'SharedServiceInstanceNotUpdateableInTargetSpace' + expect(last_response.body).to include 'You cannot update service instances that have been shared with you' + end + end + + context 'and an developer in the target space and a auditor in the source space tries to update the instance' do + let(:target_developer_source_auditor) { make_developer_for_space(target_space) } + + before do + set_current_user(target_developer_source_auditor) + set_current_user_as_role(user: target_developer_source_auditor, role: 'space_auditor', org: space.organization, space: space) + 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 @@ -1901,17 +1933,35 @@ def stub_delete_and_return(status, body) end end - context 'when the user has read but not write permissions' do - let(:auditor) { User.make } + context 'when the user has no read permissions to the space' do + let(:org_auditor) { User.make } before do - service_instance.space.organization.add_auditor(auditor) - set_current_user(auditor) + service_instance.space.organization.add_auditor(org_auditor) + set_current_user(org_auditor) end - it 'does not call out to the service broker' do + it 'does not call out to the service broker and returns an authorization error' do + put "/v2/service_instances/#{service_instance.guid}", body + expect(last_response).to have_status_code 403 + expect(decoded_response['error_code']).to eq 'CF-NotAuthorized' + expect(a_request(:patch, service_broker_url)).to have_been_made.times(0) + end + end + + context 'when the user has read but not write permissions to the space' do + let(:space_auditor) { User.make } + + before do + service_instance.space.organization.add_user(space_auditor) + service_instance.space.add_auditor(space_auditor) + set_current_user(space_auditor) + end + + it 'does not call out to the service broker and returns an authorization error' do put "/v2/service_instances/#{service_instance.guid}", body expect(last_response).to have_status_code 403 + expect(decoded_response['error_code']).to eq 'CF-NotAuthorized' expect(a_request(:patch, service_broker_url)).to have_been_made.times(0) end end diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index 47f0498e0e0..5cd07885488 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -1143,3 +1143,8 @@ name: SharedServiceInstanceCannotBeRenamed http_code: 422 message: 'Service instances that have been shared cannot be renamed' + +390009: + name: SharedServiceInstanceNotUpdateableInTargetSpace + http_code: 403 + message: 'You cannot update service instances that have been shared with you' From 2a5eb8dcd3f1625ac407ad741171e227cc6b048e Mon Sep 17 00:00:00 2001 From: Alex Blease Date: Mon, 18 Dec 2017 14:04:30 +0000 Subject: [PATCH 6/7] Improve error message when deleting shared service instance [#152552021] Signed-off-by: Denise Yu --- .../services/service_instances_controller.rb | 7 +++ .../service_instances_controller_spec.rb | 47 +++++++++++++++---- vendor/errors/v2.yml | 5 ++ 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 95752d2ee2b..d784c2d8d38 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -160,6 +160,7 @@ def delete(guid) return [HTTP::NO_CONTENT, nil] end + validate_shared_space_deleteable(service_instance) validate_access(:delete, service_instance) unless recursive_delete? @@ -456,6 +457,12 @@ def validate_shared_space_updateable(service_instance) end end + def validate_shared_space_deleteable(service_instance) + if @access_context.can?(:read, service_instance) && @access_context.cannot?(:read, service_instance.space) + raise CloudController::Errors::ApiError.new_from_details('SharedServiceInstanceNotDeleteableInTargetSpace') + end + end + def invalid_service_instance!(service_instance) raise Sequel::ValidationFailed.new(service_instance) end diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index de1f0f8d9ca..83d90c14c0c 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -1796,7 +1796,7 @@ def stub_delete_and_return(status, body) end end - context 'and an developer in the target space and a auditor in the source space tries to update the instance' do + context 'and a developer in the target space and an auditor in the source space tries to update the instance' do let(:target_developer_source_auditor) { make_developer_for_space(target_space) } before do @@ -2631,10 +2631,11 @@ def stub_delete_and_return(status, body) context 'when the service instance has been shared' do let(:originating_space) { Space.make } + let(:shared_to_space) { Space.make } let!(:service_instance) { ManagedServiceInstance.make(space: originating_space) } before do - service_instance.add_shared_space(space) + service_instance.add_shared_space(shared_to_space) end context 'as a SpaceDeveloper in source and target space' do @@ -2659,7 +2660,7 @@ def stub_delete_and_return(status, body) context 'and there are bindings to the shared instance' do before do ServiceBinding.make( - app: AppModel.make(space: space), + app: AppModel.make(space: shared_to_space), service_instance: service_instance ) end @@ -2688,12 +2689,26 @@ def stub_delete_and_return(status, body) end end + context 'as a SpaceAuditor in the source space' do + let(:source_space_auditor) { make_auditor_for_space(originating_space) } + + before do + service_instance.add_shared_space(originating_space) + set_current_user(source_space_auditor) + 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' + end + end + context 'as a SpaceDeveloper in target space' do - let(:target_space) { Space.make } - let(:target_space_dev) { make_developer_for_space(target_space) } + let(:target_space_dev) { make_developer_for_space(shared_to_space) } before do - service_instance.add_shared_space(target_space) set_current_user(target_space_dev) end @@ -2701,8 +2716,24 @@ def stub_delete_and_return(status, body) 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' + expect(last_response.body).to include 'SharedServiceInstanceNotDeleteableInTargetSpace' + expect(last_response.body).to include 'You cannot delete service instances that have been shared with you' + end + end + + context 'as a SpaceAuditor in the target space' do + let(:target_space_auditor) { make_auditor_for_space(shared_to_space) } + + before do + set_current_user(target_space_auditor) + 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 'SharedServiceInstanceNotDeleteableInTargetSpace' + expect(last_response.body).to include 'You cannot delete service instances that have been shared with you' end end end diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index 5cd07885488..df6a87cc311 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -1148,3 +1148,8 @@ name: SharedServiceInstanceNotUpdateableInTargetSpace http_code: 403 message: 'You cannot update service instances that have been shared with you' + +390010: + name: SharedServiceInstanceNotDeleteableInTargetSpace + http_code: 403 + message: 'You cannot delete service instances that have been shared with you' From 4f7b84b76ee078293108cde923c05008176edd81 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Mon, 18 Dec 2017 16:32:03 +0000 Subject: [PATCH 7/7] Fix test variable names and remove debugging output [#153739127] Signed-off-by: Alex Blease --- .../services/service_instances_controller_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 83d90c14c0c..bb70e3113c4 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -4103,11 +4103,11 @@ def verify_forbidden(user) describe 'permissions' do let(:user) { User.make } - let(:other_org) { Organization.make } - let(:other_space) { Space.make(organization: other_org) } + let(:target_org) { Organization.make } + let(:target_space) { Space.make(organization: target_org) } before do - instance.add_shared_space(other_space) + instance.add_shared_space(target_space) end context 'when the user is a member of the org/space this instance exists in' do @@ -4134,7 +4134,7 @@ def verify_forbidden(user) 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}, response: #{last_response.body}" + expect(last_response.status).to eq(expected_status), "Expected #{expected_status}, got: #{last_response.status}, role: #{role}" end end end @@ -4153,8 +4153,8 @@ def verify_forbidden(user) before do set_current_user_as_role( role: role, - org: other_org, - space: other_space, + org: target_org, + space: target_space, user: user, ) end