Prevent service instance sharers from attempting to share with themselves

app/access/service_instance_access.rb

@@ -4,13 +4,13 @@ def create?(service_instance, params=nil)
       return true if admin_user?
       FeatureFlag.raise_unless_enabled!(:service_instance_creation)
       return false if service_instance.in_suspended_org?
-      service_instance.space.has_developer?(context.user) && allowed?(service_instance)
+      service_instance.space&.has_developer?(context.user) && allowed?(service_instance)
     end
 
     def read_for_update?(service_instance, params=nil)
       return true if admin_user?
       return false if service_instance.in_suspended_org?
-      service_instance.space.has_developer?(context.user)
+      service_instance.space&.has_developer?(context.user)
     end
 
     def update?(service_instance, params=nil)
@@ -20,21 +20,20 @@ def update?(service_instance, params=nil)
     def delete?(service_instance)
       return true if admin_user?
       return false if service_instance.in_suspended_org?
-      service_instance.space.has_developer?(context.user)
+      service_instance.space&.has_developer?(context.user)
     end
 
     def manage_permissions?(service_instance)
       return true if admin_user?
-      service_instance.space.has_developer?(context.user)
+      service_instance.space&.has_developer?(context.user)
     end
 
     def manage_permissions_with_token?(service_instance)
       read_with_token?(service_instance) || has_read_permissions_scope?
     end
 
     def read_permissions?(service_instance)
-      return true if admin_user? || admin_read_only_user?
-      service_instance.space.has_member?(context.user) || service_instance.space.organization.managers.include?(context.user)
+      admin_user? || admin_read_only_user? || object_is_visible_to_user?(service_instance, context.user)
     end
 
     def read_permissions_with_token?(service_instance)
@@ -43,7 +42,7 @@ def read_permissions_with_token?(service_instance)
 
     def read_env?(service_instance)
       return true if admin_user? || admin_read_only_user?
-      service_instance.space.has_developer?(context.user)
+      service_instance.space&.has_developer?(context.user)
     end
 
     def read_env_with_token?(service_instance)
@@ -64,7 +63,7 @@ def allowed?(service_instance)
     end
 
     def purge?(service_instance)
-      admin_user? || (service_instance.space.has_developer?(context.user) && service_instance.service_broker.private?)
+      admin_user? || (service_instance.space&.has_developer?(context.user) && service_instance.service_broker.private?)
     end
 
     def purge_with_token?(instance)

app/actions/service_instance_share.rb

@@ -3,14 +3,21 @@
 module VCAP::CloudController
   class ServiceInstanceShare
     def create(service_instance, target_spaces, user_audit_info)
+      if service_instance.managed_instance?
+        unless service_instance.shareable?
+          raise CloudController::Errors::ApiError.new_from_details('ServiceShareIsDisabled', service_instance.service.label)
+        end
+      end
+
       ServiceInstance.db.transaction do
         target_spaces.each do |space|
           service_instance.add_shared_space(space)
         end
       end
 
       Repositories::ServiceInstanceShareEventRepository.record_share_event(
-        service_instance, target_spaces.map(&:guid), user_audit_info)
+        service_instance, target_spaces.map(&:guid), user_audit_info
+      )
       service_instance
     end
   end

app/controllers/runtime/spaces_controller.rb

@@ -138,20 +138,18 @@ def enumerate_services(guid)
     def enumerate_service_instances(guid)
       space = find_guid_and_validate_access(:read, guid)
 
-      if params['return_user_provided_service_instances'] == 'true'
-        model_class = ServiceInstance
-        relation_name = :service_instances
-      else
-        model_class = ManagedServiceInstance
-        relation_name = :managed_service_instances
-      end
+      model_class = params['return_user_provided_service_instances'] == 'true' ? ServiceInstance : ManagedServiceInstance
 
       service_instances = Query.filtered_dataset_from_query_params(
         model_class,
-        space.user_visible_relationship_dataset(relation_name, @access_context.user, @access_context.admin_override),
+        model_class.user_visible(@access_context.user, @access_context.admin_override),
         ServiceInstancesController.query_parameters,
         @opts)
-      service_instances.filter(space: space)
+
+      service_instances = service_instances.filter(Sequel.or([
+        [:space, space],
+        [:shared_spaces, space]
+      ]))
 
       collection_renderer.render_json(
         ServiceInstancesController,

app/controllers/services/service_instances_controller.rb

@@ -5,6 +5,9 @@
 require 'controllers/services/lifecycle/service_instance_deprovisioner'
 require 'controllers/services/lifecycle/service_instance_purger'
 require 'fetchers/service_instance_fetcher'
+require 'fetchers/service_binding_list_fetcher'
+require 'presenters/v2/service_instance_shared_to_presenter'
+require 'presenters/v2/service_instance_shared_from_presenter'
 
 module VCAP::CloudController
   class ServiceInstancesController < RestController::ModelController
@@ -31,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
       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')
@@ -157,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)
@@ -205,6 +212,46 @@ def permissions(guid)
       end
     end
 
+    get '/v2/service_instances/:guid/shared_from', :shared_from_information
+    def shared_from_information(guid)
+      service_instance = find_guid_and_validate_access(:read, guid, ManagedServiceInstance)
+
+      if service_instance.shared?
+        [HTTP::OK, {}, JSON.generate(CloudController::Presenters::V2::ServiceInstanceSharedFromPresenter.new.to_hash(service_instance.space))]
+      else
+        [HTTP::NO_CONTENT, {}, '']
+      end
+    rescue CloudController::Errors::ApiError => e
+      if e.name == 'NotAuthorized'
+        HTTP::NOT_FOUND
+      else
+        raise e
+      end
+    end
+
+    get '/v2/service_instances/:guid/shared_to', :enumerate_shared_to_information
+    def enumerate_shared_to_information(guid)
+      service_instance = find_guid_and_validate_access(:read, guid, ManagedServiceInstance)
+      validate_access(:read, service_instance.space)
+
+      associated_controller = VCAP::CloudController::SpacesController
+      associated_path = "#{self.class.url_for_guid(guid)}/shared_to"
+
+      create_paginated_collection_renderer(service_instance).render_json(
+        associated_controller,
+        service_instance.shared_spaces_dataset,
+        associated_path,
+        @opts,
+        {},
+      )
+    rescue CloudController::Errors::ApiError => e
+      if e.name == 'NotAuthorized'
+        HTTP::NOT_FOUND
+      else
+        raise e
+      end
+    end
+
     def self.url_for_guid(guid)
       object = ServiceInstance.where(guid: guid).first
 
@@ -303,6 +350,28 @@ def unbind_route(route_guid, instance_guid)
 
     private
 
+    class ServiceInstanceSharedToSerializer
+      def initialize(service_instance)
+        @service_instance = service_instance
+      end
+
+      def serialize(controller, space, opts, orphans=nil)
+        bound_app_count = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(@service_instance.guid, space.guid).count
+        CloudController::Presenters::V2::ServiceInstanceSharedToPresenter.new.to_hash(space, bound_app_count)
+      end
+    end
+
+    def create_paginated_collection_renderer(service_instance)
+      VCAP::CloudController::RestController::PaginatedCollectionRenderer.new(
+        VCAP::CloudController::RestController::SecureEagerLoader.new,
+        ServiceInstanceSharedToSerializer.new(service_instance),
+        {
+          max_results_per_page: config.get(:renderer, :max_results_per_page),
+          default_results_per_page: config.get(:renderer, :default_results_per_page),
+          max_inline_relations_depth: config.get(:renderer, :max_inline_relations_depth),
+        })
+    end
+
     def route_services_enabled?
       @config.get(:route_services_enabled)
     end
@@ -397,6 +466,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
@@ -429,6 +502,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/controllers/v3/service_instances_controller.rb

@@ -1,17 +1,36 @@
 require 'messages/to_many_relationship_message'
+require 'messages/service_instances/service_instances_list_message'
 
 require 'presenters/v3/relationship_presenter'
 require 'presenters/v3/to_many_relationship_presenter'
+require 'presenters/v3/paginated_list_presenter'
 require 'actions/service_instance_share'
 require 'actions/service_instance_unshare'
+require 'fetchers/service_instance_list_fetcher'
 
 class ServiceInstancesV3Controller < ApplicationController
+  def index
+    message = ServiceInstancesListMessage.from_params(query_params)
+    invalid_param!(message.errors.full_messages) unless message.valid?
+
+    dataset = if can_read_globally?
+                ServiceInstanceListFetcher.new.fetch_all(message: message)
+              else
+                ServiceInstanceListFetcher.new.fetch(message: message, space_guids: readable_space_guids)
+              end
+
+    render status: :ok, json: Presenters::V3::PaginatedListPresenter.new(
+      dataset: dataset,
+      path: '/v3/service_instances',
+      message: message)
+  end
+
   def share_service_instance
     FeatureFlag.raise_unless_enabled!(:service_instance_sharing)
 
     service_instance = ServiceInstance.first(guid: params[:service_instance_guid])
 
-    resource_not_found!(:service_instance) unless service_instance && can_read_space?(service_instance.space)
+    resource_not_found!(:service_instance) unless service_instance && can_read_service_instance?(service_instance)
     unauthorized! unless can_write_space?(service_instance.space)
 
     message = VCAP::CloudController::ToManyRelationshipMessage.create_from_http_request(params[:body])
@@ -20,6 +39,7 @@ def share_service_instance
     spaces = Space.where(guid: message.guids)
     check_spaces_exist_and_are_readable!(message.guids, spaces)
     check_spaces_are_writeable!(spaces)
+    ensure_not_sharing_to_self!(service_instance.space, spaces)
 
     share = ServiceInstanceShare.new
     share.create(service_instance, spaces, user_audit_info)
@@ -33,7 +53,7 @@ def unshare_service_instance
 
     service_instance = ServiceInstance.first(guid: params[:service_instance_guid])
 
-    resource_not_found!(:service_instance) unless service_instance && can_read_space?(service_instance.space)
+    resource_not_found!(:service_instance) unless service_instance && can_read_service_instance?(service_instance)
     unauthorized! unless can_write_space?(service_instance.space)
 
     space_guid = params[:space_guid]
@@ -51,12 +71,16 @@ def unshare_service_instance
 
   private
 
+  def ensure_not_sharing_to_self!(service_instance_space, target_spaces)
+    unprocessable!('Service instances cannot be shared into the space where they were created') if target_spaces.include?(service_instance_space)
+  end
+
   def check_spaces_are_writeable!(spaces)
     unwriteable_spaces = spaces.reject do |space|
       can_write?(space.guid)
     end
 
-    unauthorized! unless unwriteable_spaces.empty?
+    unauthorized! if unwriteable_spaces.any?
   end
 
   def check_spaces_exist_and_are_readable!(request_guids, found_spaces)
@@ -74,8 +98,16 @@ def check_spaces_exist_and_are_readable!(request_guids, found_spaces)
     end
   end
 
+  def can_read_service_instance?(service_instance)
+    readable_spaces = service_instance.shared_spaces + [service_instance.space]
+
+    readable_spaces.any? do |space|
+      can_read?(space.guid, space.organization_guid)
+    end
+  end
+
   def can_read_space?(space)
-    can_read?(space.guid, space.organization_guid)
+    can_read?(space.guid, space.organization.guid)
   end
 
   def can_write_space?(space)

app/fetchers/service_binding_list_fetcher.rb

@@ -16,6 +16,13 @@ def fetch_all
       filter(dataset)
     end
 
+    def self.fetch_service_instance_bindings_in_space(service_instance_guid, space_guid)
+      ServiceBinding.select_all(ServiceBinding.table_name).
+        join(:apps, guid: :app_guid).
+        where(apps__space_guid: space_guid).
+        where(service_bindings__service_instance_guid: service_instance_guid)
+    end
+
     private
 
     def filter(dataset)