Merge pull request #3106 from sap-contributions/space-subquery-for-se…

…rvice-binding

Use space subqueries for fetchting service credential bindings

app/controllers/v3/service_credential_bindings_controller.rb

@@ -22,7 +22,7 @@ def index
     invalid_param!(message.errors.full_messages) unless message.valid?
 
     results = list_fetcher.fetch(
-      space_guids: space_guids,
+      readable_spaces_query: spaces_query,
       message: message,
       eager_loaded_associations: Presenters::V3::ServiceCredentialBindingPresenter.associated_resources,
     )
@@ -328,14 +328,14 @@ def fetch_credentials_value(name)
   end
 
   def service_credential_binding
-    @service_credential_binding ||= fetcher.fetch(hashed_params[:guid], space_guids: space_guids)
+    @service_credential_binding ||= fetcher.fetch(hashed_params[:guid], readable_spaces_query: spaces_query)
   end
 
-  def space_guids
+  def spaces_query
     if permission_queryer.can_read_globally?
-      :all
+      nil
     else
-      permission_queryer.readable_space_guids
+      permission_queryer.readable_spaces_query
     end
   end
 

app/fetchers/service_credential_binding_fetcher.rb

@@ -3,8 +3,8 @@
 module VCAP
   module CloudController
     class ServiceCredentialBindingFetcher
-      def fetch(guid, space_guids:)
-        list_fetcher.fetch(space_guids: space_guids).first(guid: guid)
+      def fetch(guid, readable_spaces_query: nil)
+        list_fetcher.fetch(readable_spaces_query: readable_spaces_query).first(guid: guid)
       end
 
       private

app/fetchers/service_credential_binding_list_fetcher.rb

@@ -18,12 +18,12 @@ class << self
           service_offering_guid
         }.freeze
 
-        def fetch(space_guids:, message: nil, eager_loaded_associations: [])
-          dataset = case space_guids
-                    when :all
+        def fetch(readable_spaces_query: nil, message: nil, eager_loaded_associations: [])
+          dataset = case readable_spaces_query
+                    when nil
                       ServiceCredentialBinding::View.dataset
                     else
-                      ServiceCredentialBinding::View.where { Sequel[:space_guid] =~ space_guids }
+                      ServiceCredentialBinding::View.where { Sequel[:space_id] =~ readable_spaces_query.select(:id) }
                     end
 
           dataset = dataset.eager(eager_loaded_associations)

app/models/services/service_credential_binding_view.rb

@@ -10,7 +10,7 @@ module Types
         Sequel.as(:service_keys__id, :id),
         Sequel.as(:service_keys__guid, :guid),
         Sequel.as(Types::SERVICE_KEY, :type),
-        Sequel.as(:spaces__guid, :space_guid),
+        Sequel.as(:service_instances__space_id, :space_id),
         Sequel.as(:service_keys__created_at, :created_at),
         Sequel.as(:service_keys__updated_at, :updated_at),
         Sequel.as(:service_keys__name, :name),
@@ -36,15 +36,13 @@ module Types
         :service_plans, id: Sequel[:service_instances][:service_plan_id]
       ).left_join(
         :services, id: Sequel[:service_plans][:service_id]
-      ).join(
-        :spaces, id: Sequel[:service_instances][:space_id]
       ).freeze
 
       SERVICE_BINDING_VIEW = Sequel::Model(:service_bindings).select(
         Sequel.as(:service_bindings__id, :id),
         Sequel.as(:service_bindings__guid, :guid),
         Sequel.as(Types::SERVICE_BINDING, :type),
-        Sequel.as(:spaces__guid, :space_guid),
+        Sequel.as(:spaces__id, :space_id),
         Sequel.as(:service_bindings__created_at, :created_at),
         Sequel.as(:service_bindings__updated_at, :updated_at),
         Sequel.as(:service_bindings__name, :name),

lib/cloud_controller/permissions.rb

@@ -156,6 +156,14 @@ def readable_space_guids
     readable_space_guids_query.select_map(:guid)
   end
 
+  def readable_spaces_query
+    if can_read_globally?
+      raise 'must not be called for users that can read globally'
+    else
+      membership.authorized_spaces_subquery(ROLES_FOR_SPACE_READING)
+    end
+  end
+
   def readable_space_guids_query
     if can_read_globally?
       raise 'must not be called for users that can read globally'

spec/unit/fetchers/service_credential_binding_fetcher_spec.rb

@@ -10,19 +10,20 @@ module CloudController
         let!(:existing_credential_binding) { ServiceBinding.make }
 
         it 'should return nothing' do
-          credential_binding = fetcher.fetch('does-not-exist', space_guids: :all)
+          credential_binding = fetcher.fetch('does-not-exist', readable_spaces_query: nil)
           expect(credential_binding).to be_nil
         end
       end
 
       describe 'service keys' do
         let(:space) { Space.make }
+        let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) }
         let(:service_instance) { ManagedServiceInstance.make(space: space) }
         let!(:service_key) { ServiceKey.make(service_instance: service_instance) }
 
         describe 'when in the space' do
           it 'can be found' do
-            credential_binding = fetcher.fetch(service_key.guid, space_guids: [space.guid])
+            credential_binding = fetcher.fetch(service_key.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding).not_to be_nil
             expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceKey)
@@ -35,8 +36,9 @@ module CloudController
 
         describe 'when not in the space' do
           let!(:other_space) { Space.make }
+          let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) }
           it 'can not be found' do
-            credential_binding = fetcher.fetch(service_key.guid, space_guids: [other_space.guid])
+            credential_binding = fetcher.fetch(service_key.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding).to be_nil
           end
@@ -46,11 +48,12 @@ module CloudController
       describe 'app bindings' do
         describe 'managed services' do
           let(:space) { Space.make }
+          let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) }
           let(:service_instance) { ManagedServiceInstance.make(space: space) }
           let!(:app_binding) { ServiceBinding.make(service_instance: service_instance, name: 'some-name') }
 
           it 'can be found' do
-            credential_binding = fetcher.fetch(app_binding.guid, space_guids: [space.guid])
+            credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding).not_to be_nil
             expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceBinding)
@@ -64,8 +67,9 @@ module CloudController
 
           describe 'when not in the space' do
             let!(:other_space) { Space.make }
+            let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) }
             it 'can not be found' do
-              credential_binding = fetcher.fetch(app_binding.guid, space_guids: [other_space.guid])
+              credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
               expect(credential_binding).to be_nil
             end
@@ -74,11 +78,12 @@ module CloudController
 
         describe 'user provided services' do
           let(:space) { Space.make }
+          let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) }
           let(:service_instance) { UserProvidedServiceInstance.make(space: space) }
           let!(:app_binding) { ServiceBinding.make(service_instance: service_instance, name: 'some-name') }
 
           it 'can be found' do
-            credential_binding = fetcher.fetch(app_binding.guid, space_guids: [space.guid])
+            credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding).not_to be_nil
             expect(credential_binding).to an_instance_of(VCAP::CloudController::ServiceBinding)
@@ -92,8 +97,9 @@ module CloudController
 
           describe 'when not in the space' do
             let!(:other_space) { Space.make }
+            let!(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [other_space].map(&:id)) }
             it 'can not be found' do
-              credential_binding = fetcher.fetch(app_binding.guid, space_guids: [other_space.guid])
+              credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
               expect(credential_binding).to be_nil
             end
@@ -102,14 +108,15 @@ module CloudController
 
         describe 'with last operation' do
           let(:service_instance) { ManagedServiceInstance.make }
+          let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [service_instance.space].map(&:id)) }
           let!(:app_binding) {
             binding = ServiceBinding.make(service_instance: service_instance)
             binding.save_with_new_operation({ state: 'succeeded', type: 'create', description: 'radical avocado' })
             binding
           }
 
           it 'fetches the last operation' do
-            credential_binding = fetcher.fetch(app_binding.guid, space_guids: [service_instance.space.guid])
+            credential_binding = fetcher.fetch(app_binding.guid, readable_spaces_query: readable_spaces_query)
 
             expect(credential_binding.last_operation).to_not be_nil
 

spec/unit/fetchers/service_credential_binding_list_fetcher_spec.rb

@@ -12,7 +12,7 @@ module CloudController
 
       describe 'no bindings' do
         it 'returns an empty result' do
-          expect(fetcher.fetch(space_guids: :all, message: message).all).to eql([])
+          expect(fetcher.fetch(readable_spaces_query: nil, message: message).all).to eql([])
         end
       end
 
@@ -37,15 +37,15 @@ module CloudController
         let!(:app_binding) { VCAP::CloudController::ServiceBinding.make(service_instance: instance, name: Sham.name, **app_binding_details) }
 
         it 'eager loads the specified resources' do
-          dataset = fetcher.fetch(space_guids: :all, message: message, eager_loaded_associations: [:labels_sti_eager_load])
+          dataset = fetcher.fetch(readable_spaces_query: nil, message: message, eager_loaded_associations: [:labels_sti_eager_load])
 
           expect(dataset.all.first.associations.key?(:labels)).to be true
           expect(dataset.all.first.associations.key?(:annotations)).to be false
         end
 
         context 'when getting everything' do
           it 'returns both key and app bindings' do
-            bindings = fetcher.fetch(space_guids: :all, message: message).all
+            bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
             to_hash = ->(b) {
               {
                 guid: b.guid,
@@ -63,11 +63,12 @@ module CloudController
         context 'when limiting to a space' do
           let(:other_space) { VCAP::CloudController::Space.make }
           let(:other_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: other_space) }
+          let(:readable_spaces_query) { VCAP::CloudController::Space.where(id: [space].map(&:id)) }
           let!(:key_other_binding) { VCAP::CloudController::ServiceKey.make(service_instance: other_instance) }
           let!(:app_other_binding) { VCAP::CloudController::ServiceBinding.make(service_instance: other_instance) }
 
           it 'returns only the bindings within that space' do
-            bindings = fetcher.fetch(space_guids: [space.guid], message: message).all
+            bindings = fetcher.fetch(readable_spaces_query: readable_spaces_query, message: message).all
             binding_guids = bindings.map(&:guid)
 
             expect(binding_guids).to contain_exactly(key_binding.guid, app_binding.guid)
@@ -82,71 +83,71 @@ module CloudController
           context 'service instance name' do
             let(:params) { { 'service_instance_names' => instance.name } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid)
             end
           end
 
           context 'service instance guid' do
             let(:params) { { 'service_instance_guids' => instance.guid } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid)
             end
           end
 
           context 'app name' do
             let(:params) { { 'app_names' => "#{app_binding.app.name},'some-other-name'" } }
             it 'can filter by app name' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid)
             end
           end
 
           context 'app guid' do
             let(:params) { { 'app_guids' => "#{app_binding.app.guid}, 'some-other-guid'" } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid)
             end
           end
 
           context 'binding name' do
             let(:params) { { 'names' => "#{key_binding.name},#{app_binding.name}" } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, app_binding.guid)
             end
           end
 
           context 'service plan names' do
             let(:params) { { 'service_plan_names' => instance.service_plan.name } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid)
             end
           end
 
           context 'service plan guids' do
             let(:params) { { 'service_plan_guids' => instance.service_plan.guid } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid)
             end
           end
 
           context 'service offering names' do
             let(:params) { { 'service_offering_names' => app_binding.service.name.to_s } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid)
             end
           end
 
           context 'service offering guids' do
             let(:params) { { 'service_offering_guids' => app_binding.service.guid.to_s } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, key_binding.guid)
             end
           end
@@ -166,7 +167,7 @@ module CloudController
 
             let(:params) { { 'label_selector' => 'fruit=strawberry,tier in (backend,worker)' } }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, another_binding.guid)
             end
           end
@@ -176,7 +177,7 @@ module CloudController
               let(:params) { { 'type' => 'app' } }
 
               it 'returns the right result' do
-                bindings = fetcher.fetch(space_guids: :all, message: message).all
+                bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
                 expect(bindings.map(&:guid)).to contain_exactly(app_binding.guid, another_binding.guid)
               end
             end
@@ -185,14 +186,14 @@ module CloudController
               let(:params) { { type: 'key' } }
 
               it 'returns the right result' do
-                bindings = fetcher.fetch(space_guids: :all, message: message).all
+                bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
                 expect(bindings.map(&:guid)).to contain_exactly(key_binding.guid, another_key.guid)
               end
             end
           end
 
           it 'returns all if no filter is passed' do
-            bindings = fetcher.fetch(space_guids: :all, message: message).all
+            bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
             expect(bindings.count).to eq(4)
           end
 
@@ -201,7 +202,7 @@ module CloudController
               { service_instance_guids: ['fake-guid'], service_instance_names: ['fake-name'] }
             }
             it 'returns empty' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings).to be_empty
             end
           end
@@ -211,7 +212,7 @@ module CloudController
               { names: [key_binding.name, another_binding.name], service_instance_guids: [another_instance.guid] }
             }
             it 'returns the right result' do
-              bindings = fetcher.fetch(space_guids: :all, message: message).all
+              bindings = fetcher.fetch(readable_spaces_query: nil, message: message).all
               expect(bindings.map(&:guid)).to contain_exactly(another_binding.guid)
             end
           end
@@ -230,7 +231,7 @@ module CloudController
             }
           )
 
-          credential_binding = fetcher.fetch(space_guids: :all, message: message).first
+          credential_binding = fetcher.fetch(readable_spaces_query: nil, message: message).first
           last_operation = credential_binding.last_operation
 
           expect(last_operation).to be_present

spec/unit/lib/cloud_controller/permissions_spec.rb

@@ -378,6 +378,16 @@ module VCAP::CloudController
       end
     end
 
+    describe '#readables_space_query' do
+      it 'returns subquery from membership' do
+        membership = instance_double(Membership)
+        subquery = instance_double(Sequel::Dataset)
+        expect(Membership).to receive(:new).with(user).and_return(membership)
+        expect(membership).to receive(:authorized_spaces_subquery).with(Permissions::ROLES_FOR_SPACE_READING).and_return(subquery)
+        expect(permissions.readable_spaces_query).to be(subquery)
+      end
+    end
+
     describe '#readables_space_guids_query' do
       it 'returns subquery from membership' do
         membership = instance_double(Membership)