diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index ca347792f8b..a1a4211f649 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| - can_write?(space.guid) - end + def check_spaces_exist_and_are_writeable!(service_instance, request_guids, found_spaces) + unreadable_spaces = found_spaces.reject { |s| can_read_space?(s) } + unwriteable_spaces = found_spaces.reject { |s| can_write_space?(s) || unreadable_spaces.include?(s) } + + 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) + + 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) - unauthorized! unless unwriteable_spaces.empty? + error_msg = [unreadable_error, unwriteable_error].map(&:presence).compact.join("\n") + + unprocessable!(error_msg) + end end - def check_spaces_exist_and_are_readable!(request_guids, found_spaces) - missing_guids = request_guids - found_spaces.map(&:guid) + 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(', ') - unreadable_spaces = found_spaces.reject do |space| - can_read_space?(space) + "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 - missing_guids += unreadable_spaces.map(&:guid) + 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(', ') - 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.") + "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 diff --git a/spec/unit/controllers/v3/service_instances_controller_spec.rb b/spec/unit/controllers/v3/service_instances_controller_spec.rb index 8cc57246540..94e10d0e742 100644 --- a/spec/unit/controllers/v3/service_instances_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instances_controller_spec.rb @@ -334,8 +334,23 @@ 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 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 @@ -351,10 +366,72 @@ 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(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 @@ -399,11 +476,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