App Dev Sharers Cannot Delete Shared Service Instances

app/controllers/services/service_instances_controller.rb

@@ -34,14 +34,13 @@ class ServiceInstancesController < RestController::ModelController
     define_routes
 
     def self.translate_validation_exception(e, attributes)
-      space_and_name_errors = e.errors.on([:space_id, :name]).to_a
-      quota_errors = e.errors.on(:quota).to_a
-      service_plan_errors = e.errors.on(:service_plan).to_a
-      service_instance_errors = e.errors.on(:service_instance).to_a
+      quota_errors                 = e.errors.on(:quota).to_a
+      service_plan_errors          = e.errors.on(:service_plan).to_a
+      service_instance_errors      = e.errors.on(:service_instance).to_a
       service_instance_name_errors = e.errors.on(:name).to_a
       service_instance_tags_errors = e.errors.on(:tags).to_a
 
-      if space_and_name_errors.include?(:unique)
+      if service_instance_name_errors.include?(:unique)
         return CloudController::Errors::ApiError.new_from_details('ServiceInstanceNameTaken', attributes['name'])
       elsif quota_errors.include?(:service_instance_space_quota_exceeded)
         return CloudController::Errors::ApiError.new_from_details('ServiceInstanceSpaceQuotaExceeded')
@@ -160,11 +159,16 @@ def delete(guid)
       end
 
       validate_access(:delete, service_instance)
-      has_assocations = has_routes?(service_instance) ||
-        has_bindings?(service_instance) ||
-        has_keys?(service_instance)
 
-      association_not_empty! if has_assocations && !recursive_delete?
+      unless recursive_delete?
+        service_is_shared!(service_instance.name) if has_shares?(service_instance)
+
+        has_associations = has_routes?(service_instance) ||
+          has_bindings?(service_instance) ||
+          has_keys?(service_instance)
+
+        association_not_empty! if has_associations
+      end
 
       deprovisioner = ServiceInstanceDeprovisioner.new(@services_event_repository, self, logger)
       delete_job = deprovisioner.deprovision_service_instance(service_instance, accepts_incomplete, async)
@@ -459,6 +463,10 @@ def association_not_empty!
       raise CloudController::Errors::ApiError.new_from_details('AssociationNotEmpty', associations, :service_instances)
     end
 
+    def service_is_shared!(name)
+      raise CloudController::Errors::ApiError.new_from_details('ServiceInstanceDeletionSharesExists', name)
+    end
+
     def space_change_not_allowed!
       raise CloudController::Errors::ApiError.new_from_details('ServiceInstanceSpaceChangeNotAllowed')
     end
@@ -491,6 +499,10 @@ def has_keys?(service_instance)
       !service_instance.service_keys.empty?
     end
 
+    def has_shares?(service_instance)
+      !service_instance.shared_spaces.empty?
+    end
+
     def space_change_requested?(requested_space_guid, current_space)
       requested_space_guid && requested_space_guid != current_space.guid
     end

app/controllers/services/user_provided_service_instances_controller.rb

@@ -29,11 +29,11 @@ def inject_dependencies(dependencies)
     end
 
     def self.translate_validation_exception(e, attributes)
-      space_and_name_errors = e.errors.on([:space_id, :name])
+      name_errors = e.errors.on(:name)
       service_instance_errors = e.errors.on(:service_instance)
       service_instance_name_errors = e.errors.on(:name).to_a
 
-      if space_and_name_errors&.include?(:unique)
+      if name_errors&.include?(:unique)
         CloudController::Errors::ApiError.new_from_details('ServiceInstanceNameTaken', attributes['name'])
       elsif service_instance_errors&.include?(:space_mismatch)
         CloudController::Errors::ApiError.new_from_details('ServiceInstanceRouteBindingSpaceMismatch')

app/models/services/service_instance.rb

@@ -87,14 +87,27 @@ def managed_instance?
       !user_provided_instance?
     end
 
+    def name_clashes
+      proc do |_, instance|
+        next if instance.space_id.nil? || instance.name.nil?
+
+        clashes_with_shared_instance_names =
+          ServiceInstance.select_all(ServiceInstance.table_name).
+          join(:service_instance_shares, service_instance_guid: :guid, target_space_guid: instance.space_guid).
+          where(name: instance.name)
+
+        clashes_with_instance_names =
+          ServiceInstance.select_all(ServiceInstance.table_name).
+          where(space_id: instance.space_id, name: instance.name)
+
+        clashes_with_shared_instance_names.union(clashes_with_instance_names)
+      end
+    end
+
     def validate
       validates_presence :name
       validates_presence :space
-      validates_unique [:space_id, :name], where: (proc do |_, obj, arr|
-                                                     vals = arr.map { |x| obj.send(x) }
-                                                     next if vals.any?(&:nil?)
-                                                     ServiceInstance.where(arr.zip(vals))
-                                                   end)
+      validates_unique :name, where: name_clashes
       validates_max_length 50, :name
       validates_max_length 10_000, :syslog_drain_url, allow_nil: true
     end

spec/request/v2/service_instances_spec.rb

@@ -244,4 +244,33 @@
       )
     end
   end
+
+  describe 'DELETE /v2/service_instance/:guid' do
+    let(:originating_space) { VCAP::CloudController::Space.make }
+    let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: originating_space) }
+
+    context 'when the service instance has been shared' do
+      before do
+        allow(VCAP::Services::ServiceBrokers::V2::Client).to receive(:new) do |*args, **kwargs, &block|
+          FakeServiceBrokerV2Client.new(*args, **kwargs, &block)
+        end
+
+        set_current_user_as_admin
+        service_instance.add_shared_space(space)
+      end
+
+      it 'fails with an appropriate response' do
+        delete "v2/service_instances/#{service_instance.guid}", nil, admin_headers
+
+        expect(last_response.status).to eq(400)
+
+        parsed_response = MultiJson.load(last_response.body)
+        expect(parsed_response['description']).to eq 'Service instances must be unshared before they can be deleted. ' \
+          "Unsharing #{service_instance.name} will automatically delete any bindings " \
+          'that have been made to applications in other spaces.'
+        expect(parsed_response['error_code']).to eq 'CF-ServiceInstanceDeletionSharesExists'
+        expect(parsed_response['code']).to eq 390002
+      end
+    end
+  end
 end

spec/support/fakes/fake_service_broker_v2_client.rb

@@ -46,6 +46,16 @@ def provision(_instance, arbitrary_parameters: {}, accepts_incomplete: false)
     }
   end
 
+  def deprovision(_instance, arbitrary_parameters: {}, accepts_incomplete: false)
+    {
+      last_operation: {
+        type:        'delete',
+        description: '',
+        state:       'succeeded'
+      }
+    }
+  end
+
   def bind(_binding, _arbitrary_parameters)
     {
       credentials: credentials,

spec/support/matchers/sequel_validations.rb

@@ -48,7 +48,7 @@
       duplicate_object[attr] = source_obj[attr]
     end
     unless duplicate_object.valid?
-      errors_key = attributes.length > 1 ? attributes : attributes.first
+      errors_key = options[:error_key] || (attributes.length > 1 ? attributes : attributes.first)
       errors = duplicate_object.errors.on(errors_key)
       expected_error = options[:message] || :unique
       errors && errors.include?(expected_error)

spec/unit/controllers/services/service_instances_controller_spec.rb

@@ -885,6 +885,53 @@ def stub_delete_and_return(status, body)
             expect(last_response.status).to eq(400)
             expect(decoded_response['code']).to eq(60002)
           end
+
+          context 'when a service instance share exists between spaces' do
+            let(:source_space) { Space.make(organization: space.organization) }
+            before do
+              source_space.add_developer(developer)
+
+              service_instance = create_managed_service_instance(accepts_incomplete: 'false', space: source_space)
+              service_instance.add_shared_space(space)
+              expect(last_response.status).to eq(201)
+            end
+
+            it 'does not allow a managed service instance with same name as a shared service instance' do
+              create_managed_service_instance
+              expect(last_response.status).to eq(400)
+              expect(decoded_response['code']).to eq(60002)
+            end
+
+            it 'does not allow a user provided service instance with same name as a shared service instance' do
+              create_user_provided_service_instance
+              expect(last_response.status).to eq(400)
+              expect(decoded_response['code']).to eq(60002)
+            end
+
+            context 'when an unshared instance exists in the source space' do
+              before do
+                create_managed_service_instance(accepts_incomplete: 'false', space: source_space, name: 'bar')
+                expect(last_response.status).to eq(201)
+              end
+
+              it 'allows an instance of the same name to be created in the shared to space' do
+                create_managed_service_instance(accepts_incomplete: 'false', space: space, name: 'bar')
+                expect(last_response.status).to eq(201)
+              end
+            end
+
+            context 'when an unshared instance exists in the shared to space' do
+              before do
+                create_managed_service_instance(accepts_incomplete: 'false', space: space, name: 'bar')
+                expect(last_response.status).to eq(201)
+              end
+
+              it 'allows an instance of the same name to be created in the source space' do
+                create_managed_service_instance(accepts_incomplete: 'false', space: source_space, name: 'bar')
+                expect(last_response.status).to eq(201)
+              end
+            end
+          end
         end
 
         context 'when the service_plan does not exist' do
@@ -2315,6 +2362,64 @@ def stub_delete_and_return(status, body)
           end
         end
 
+        context 'when the service instance has been shared' do
+          let(:originating_space) { Space.make }
+          let!(:service_instance) { ManagedServiceInstance.make(space: originating_space) }
+
+          before do
+            service_instance.add_shared_space(space)
+          end
+
+          it 'does not delete the associated shares' do
+            delete "/v2/service_instances/#{service_instance.guid}"
+
+            expect(ServiceInstance.find(guid: service_instance.guid)).to be
+            expect(ServiceInstance.find(guid: service_instance.guid).shared_spaces.length).to eq(1)
+          end
+
+          it 'should give the user an error' do
+            delete "/v2/service_instances/#{service_instance.guid}"
+
+            expect(last_response).to have_status_code 400
+            expect(last_response.body).to include 'ServiceInstanceDeletionSharesExists'
+            expect(last_response.body).to include(
+              'Service instances must be unshared before they can be deleted. ' \
+              "Unsharing #{service_instance.name} will automatically delete any bindings " \
+              'that have been made to applications in other spaces.')
+          end
+
+          context 'and there are bindings to the shared instance' do
+            before do
+              ServiceBinding.make(
+                app: AppModel.make(space: space),
+                service_instance: service_instance
+              )
+            end
+
+            it 'should give the user an error' do
+              delete "/v2/service_instances/#{service_instance.guid}"
+
+              expect(last_response).to have_status_code 400
+              expect(last_response.body).to include 'ServiceInstanceDeletionSharesExists'
+              expect(last_response.body).to include(
+                'Service instances must be unshared before they can be deleted. ' \
+                "Unsharing #{service_instance.name} will automatically delete any bindings " \
+                'that have been made to applications in other spaces.')
+            end
+          end
+
+          context 'and recursive=true' do
+            it 'deletes the associated shares' do
+              expect {
+                delete "/v2/service_instances/#{service_instance.guid}?recursive=true"
+              }.to change(ServiceInstance.join(:service_instance_shares, service_instance_guid: :service_instances__guid), :count).by(-1)
+
+              expect(last_response.status).to eq(204)
+              expect(ServiceInstance.find(guid: service_instance.guid)).to be_nil
+            end
+          end
+        end
+
         context 'with ?accepts_incomplete=true' do
           before do
             stub_deprovision(service_instance, body: body, status: status, accepts_incomplete: true)
@@ -4003,7 +4108,6 @@ def verify_forbidden(user)
       let(:errors) { instance_double(Sequel::Model::Errors) }
       let(:attributes) { {} }
 
-      let(:space_and_name_errors) { nil }
       let(:quota_errors) { nil }
       let(:service_plan_errors) { nil }
       let(:service_instance_name_errors) { nil }
@@ -4013,7 +4117,6 @@ def verify_forbidden(user)
 
       before do
         allow(e).to receive(:errors).and_return(errors)
-        allow(errors).to receive(:on).with([:space_id, :name]).and_return(space_and_name_errors)
         allow(errors).to receive(:on).with(:quota).and_return(quota_errors)
         allow(errors).to receive(:on).with(:service_plan).and_return(service_plan_errors)
         allow(errors).to receive(:on).with(:name).and_return(service_instance_name_errors)
@@ -4028,7 +4131,6 @@ def verify_forbidden(user)
       end
 
       context "when errors are included but aren't supported validation exceptions" do
-        let(:space_and_name_errors) { [:stuff] }
         let(:quota_errors) { [:stuff] }
         let(:service_plan_errors) { [:stuff] }
         let(:service_instance_name_errors) { [:stuff] }
@@ -4042,7 +4144,7 @@ def verify_forbidden(user)
 
       context 'when there is a service instance name taken error' do
         let(:attributes) { { 'name' => 'test name' } }
-        let(:space_and_name_errors) { [:unique] }
+        let(:service_instance_name_errors) { [:unique] }
 
         it 'returns a ServiceInstanceNameTaken error' do
           expect(VCAP::CloudController::ServiceInstancesController.translate_validation_exception(e, attributes).name).to eq('ServiceInstanceNameTaken')
@@ -4111,10 +4213,12 @@ def create_managed_service_instance(user_opts={})
       arbitrary_params = user_opts.delete(:parameters)
       accepts_incomplete = user_opts.delete(:accepts_incomplete) { |_| 'true' }
       tags = user_opts.delete(:tags)
+      service_instance_space = user_opts.delete(:space) || space
+      service_instance_name = user_opts.delete(:name) || 'foo'
 
       body = {
-        name: 'foo',
-        space_guid: space.guid,
+        name: service_instance_name,
+        space_guid: service_instance_space.guid,
         service_plan_guid: plan.guid,
       }
       body[:parameters] = arbitrary_params if arbitrary_params

spec/unit/models/services/managed_service_instance_spec.rb

@@ -32,7 +32,7 @@ module VCAP::CloudController
       it { is_expected.to validate_presence :name }
       it { is_expected.to validate_presence :service_plan }
       it { is_expected.to validate_presence :space }
-      it { is_expected.to validate_uniqueness [:space_id, :name] }
+      it { is_expected.to validate_uniqueness :space_id, :name, { error_key: :name } }
       it { is_expected.to strip_whitespace :name }
       let(:max_tags) { ['a' * 1024, 'b' * 1024] }