Fix typos in v3 service instance controller spec

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

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
 

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,21 +2689,51 @@ 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
 
             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'
+              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

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,16 +357,16 @@
     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
 
       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: [

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'