diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index b53080e272f..d784c2d8d38 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)) @@ -159,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? @@ -449,6 +451,18 @@ 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 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/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..bb70e3113c4 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 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 + 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 @@ -2581,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 @@ -2609,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 @@ -2638,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 @@ -2651,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 @@ -4022,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 @@ -4053,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 @@ -4072,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 diff --git a/spec/unit/controllers/v3/service_instances_controller_spec.rb b/spec/unit/controllers/v3/service_instances_controller_spec.rb index 94e10d0e742..12f4ba57de3 100644 --- a/spec/unit/controllers/v3/service_instances_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instances_controller_spec.rb @@ -259,7 +259,7 @@ context 'when the service instance does not exist' do it 'returns a 404' do - post :share_service_instance, service_instance_guid: 'nonexistant-service-instance-guid', body: req_body + post :share_service_instance, service_instance_guid: 'nonexistent-service-instance-guid', body: req_body expect(response.status).to eq 404 expect(response.body).to include('Service instance not found') end @@ -328,13 +328,13 @@ context 'when the target space does not exist' do before do - req_body[:data] = [{ guid: 'nonexistant-space-guid' }] + req_body[:data] = [{ guid: 'nonexistent-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']. ") + expect(response.body).to include("Unable to share service instance #{service_instance.name} with spaces ['nonexistent-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 @@ -357,8 +357,8 @@ context 'when multiple target spaces do not exist' do before do req_body[:data] = [ - { guid: 'nonexistant-space-guid' }, - { guid: 'nonexistant-space-guid2' }, + { guid: 'nonexistent-space-guid' }, + { guid: 'nonexistent-space-guid2' }, { guid: target_space.guid } ] end @@ -366,7 +366,7 @@ 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 service instance #{service_instance.name} with spaces ['nonexistant-space-guid', 'nonexistant-space-guid2']. ") + expect(response.body).to include("Unable to share service instance #{service_instance.name} with spaces ['nonexistent-space-guid', 'nonexistent-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 @@ -393,7 +393,7 @@ 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: 'nonexistent-space-guid' }, { guid: target_space.guid } ] end @@ -402,14 +402,14 @@ 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 ['nonexistent-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 + context 'when the user is a SpaceAuditor in multiple target spaces' do let(:req_body) do { data: [ diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index 47f0498e0e0..df6a87cc311 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -1143,3 +1143,13 @@ 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' + +390010: + name: SharedServiceInstanceNotDeleteableInTargetSpace + http_code: 403 + message: 'You cannot delete service instances that have been shared with you'