Return a more helpful error when recipient space developers try to update a shared service instance

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

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
 

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 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

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

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'