Merge branch '30769-refactoring-for-deploy-key-on-protected-branches' into 'master'

[RUN AS-IF-FOSS] Refactoring to ease review on big backend MR

See merge request gitlab-org/gitlab!47088

app/controllers/autocomplete_controller.rb

@@ -53,7 +53,7 @@ class AutocompleteController < ApplicationController
   end
 
   def deploy_keys_with_owners
-    deploy_keys = DeployKeys::CollectKeysService.new(project, current_user).execute
+    deploy_keys = DeployKey.with_write_access_for_project(project)
 
     render json: DeployKeySerializer.new.represent(deploy_keys, { with_owner: true, user: current_user })
   end

app/models/deploy_key.rb

@@ -7,8 +7,9 @@ class DeployKey < Key
   has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :projects, through: :deploy_keys_projects
 
-  scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) }
-  scope :are_public,  -> { where(public: true) }
+  scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where(deploy_keys_projects: { project_id: projects }) }
+  scope :with_write_access, -> { joins(:deploy_keys_projects).merge(DeployKeysProject.with_write_access) }
+  scope :are_public, -> { where(public: true) }
   scope :with_projects, -> { includes(deploy_keys_projects: { project: [:route, namespace: :route] }) }
 
   ignore_column :can_push, remove_after: '2019-12-15', remove_with: '12.6'
@@ -54,4 +55,11 @@ class DeployKey < Key
   def projects_with_write_access
     Project.with_route.where(id: deploy_keys_projects.with_write_access.select(:project_id))
   end
+
+  def self.with_write_access_for_project(project, deploy_key: nil)
+    query = in_projects(project).with_write_access
+    query = query.where(id: deploy_key) if deploy_key
+
+    query
+  end
 end

app/models/deploy_keys_project.rb

@@ -6,7 +6,6 @@ class DeployKeysProject < ApplicationRecord
   scope :without_project_deleted,  -> { joins(:project).where(projects: { pending_delete: false }) }
   scope :in_project, ->(project) { where(project: project) }
   scope :with_write_access, -> { where(can_push: true) }
-  scope :with_deploy_keys, -> { includes(:deploy_key) }
 
   accepts_nested_attributes_for :deploy_key
 

app/services/deploy_keys/collect_keys_service.rb

-# frozen_string_literal: true
-
-module DeployKeys
-  class CollectKeysService
-    def initialize(project, current_user)
-      @project = project
-      @current_user = current_user
-    end
-
-    def execute
-      return [] unless current_user && project && user_can_read_project
-
-      project.deploy_keys_projects
-        .with_deploy_keys
-        .with_write_access
-        .map(&:deploy_key)
-    end
-
-    private
-
-    def user_can_read_project
-      Ability.allowed?(current_user, :read_project, project)
-    end
-
-    attr_reader :project, :current_user
-  end
-end

lib/gitlab/user_access.rb

@@ -90,9 +90,13 @@ module Gitlab
     def can_collaborate?(ref)
       assert_project!
 
+      can_push? || branch_allows_collaboration_for?(ref)
+    end
+
+    def branch_allows_collaboration_for?(ref)
       # Checking for an internal project or group to prevent an infinite loop:
       # https://gitlab.com/gitlab-org/gitlab/issues/36805
-      can_push? || (!project.internal? && project.branch_allows_collaboration?(user, ref))
+      (!project.internal? && project.branch_allows_collaboration?(user, ref))
     end
 
     def permission_cache

spec/controllers/autocomplete_controller_spec.rb

@@ -382,6 +382,17 @@ RSpec.describe AutocompleteController do
         sign_in(user)
       end
 
+      context 'and they cannot read the project' do
+        it 'returns a not found response' do
+          allow(Ability).to receive(:allowed?).and_call_original
+          allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(false)
+
+          get(:deploy_keys_with_owners, params: { project_id: project.id })
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+
       it 'renders the deploy key in a json payload, with its owner' do
         get(:deploy_keys_with_owners, params: { project_id: project.id })
 

spec/models/deploy_key_spec.rb

@@ -40,4 +40,56 @@ RSpec.describe DeployKey, :mailer do
       end
     end
   end
+
+  describe '.with_write_access_for_project' do
+    let_it_be(:project) { create(:project, :private) }
+
+    subject { described_class.with_write_access_for_project(project) }
+
+    context 'when no project is passed in' do
+      let(:project) { nil }
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'when a project is passed in' do
+      let_it_be(:deploy_keys_project) { create(:deploy_keys_project, :write_access, project: project) }
+      let_it_be(:deploy_key) { deploy_keys_project.deploy_key }
+
+      it 'only returns deploy keys with write access' do
+        create(:deploy_keys_project, project: project)
+
+        is_expected.to contain_exactly(deploy_key)
+      end
+
+      it 'returns deploy keys only for this project' do
+        other_project = create(:project)
+        create(:deploy_keys_project, :write_access, project: other_project)
+
+        is_expected.to contain_exactly(deploy_key)
+      end
+
+      context 'and a specific deploy key is passed in' do
+        subject { described_class.with_write_access_for_project(project, deploy_key: specific_deploy_key) }
+
+        context 'and this deploy key is not linked to the project' do
+          let(:specific_deploy_key) { create(:deploy_key) }
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'and this deploy key has not write access to the project' do
+          let(:specific_deploy_key) { create(:deploy_key, deploy_keys_projects: [create(:deploy_keys_project, project: project)]) }
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'and this deploy key has write access to the project' do
+          let(:specific_deploy_key) { create(:deploy_key, deploy_keys_projects: [create(:deploy_keys_project, :write_access, project: project)]) }
+
+          it { is_expected.to contain_exactly(specific_deploy_key) }
+        end
+      end
+    end
+  end
 end

spec/models/deploy_keys_project_spec.rb

@@ -13,21 +13,6 @@ RSpec.describe DeployKeysProject do
     it { is_expected.to validate_presence_of(:deploy_key) }
   end
 
-  describe '.with_deploy_keys' do
-    subject(:scoped_query) { described_class.with_deploy_keys.last }
-
-    it 'includes deploy_keys in query' do
-      project = create(:project)
-      create(:deploy_keys_project, project: project, deploy_key: create(:deploy_key))
-
-      includes_query_count = ActiveRecord::QueryRecorder.new { scoped_query }.count
-      deploy_key_query_count = ActiveRecord::QueryRecorder.new { scoped_query.deploy_key }.count
-
-      expect(includes_query_count).to eq(2)
-      expect(deploy_key_query_count).to eq(0)
-    end
-  end
-
   describe "Destroying" do
     let(:project)     { create(:project) }
     subject           { create(:deploy_keys_project, project: project) }

spec/services/deploy_keys/collect_keys_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe DeployKeys::CollectKeysService do
-  let_it_be(:user) { create(:user) }
-  let_it_be(:project) { create(:project, :private) }
-
-  subject { DeployKeys::CollectKeysService.new(project, user) }
-
-  before do
-    project&.add_developer(user)
-  end
-
-  context 'when no project is passed in' do
-    let(:project) { nil }
-
-    it 'returns an empty Array' do
-      expect(subject.execute).to be_empty
-    end
-  end
-
-  context 'when no user is passed in' do
-    let(:user) { nil }
-
-    it 'returns an empty Array' do
-      expect(subject.execute).to be_empty
-    end
-  end
-
-  context 'when a project is passed in' do
-    let_it_be(:deploy_keys_project) { create(:deploy_keys_project, :write_access, project: project) }
-    let_it_be(:deploy_key) { deploy_keys_project.deploy_key }
-
-    it 'only returns deploy keys with write access' do
-      create(:deploy_keys_project, project: project)
-
-      expect(subject.execute).to contain_exactly(deploy_key)
-    end
-
-    it 'returns deploy keys only for this project' do
-      other_project = create(:project)
-      create(:deploy_keys_project, :write_access, project: other_project)
-
-      expect(subject.execute).to contain_exactly(deploy_key)
-    end
-  end
-
-  context 'when the user cannot read the project' do
-    before do
-      project.members.delete_all
-    end
-
-    it 'returns an empty Array' do
-      expect(subject.execute).to be_empty
-    end
-  end
-end