Merge branch 'dm-deploy-keys-default-user' into 'master'

Ensure hooks run when a deploy key without a user pushes

Closes #44317

See merge request gitlab-org/gitlab-ce!18057

8 files changed, 52 insertions, 22 deletions

diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index c2e0a5fa126..89a74b7dcb1 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -27,6 +27,10 @@ class DeployKey < Key
     self.private?
   end
 
+  def user
+    super || User.ghost
+  end
+
   def has_access_to?(project)
     deploy_keys_project_for(project).present?
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 187878f4fb5..f934b654225 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -82,11 +82,8 @@ class User < ActiveRecord::Base
   has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, inverse_of: :owner, autosave: true # rubocop:disable Cop/ActiveRecordDependent
 
   # Profile
-  has_many :keys, -> do
-    type = Key.arel_table[:type]
-    where(type.not_eq('DeployKey').or(type.eq(nil)))
-  end, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+  has_many :keys, -> { where(type: ['Key', nil]) }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+  has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent
   has_many :gpg_keys
 
   has_many :emails, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
diff --git a/changelogs/unreleased/dm-deploy-keys-default-user.yml b/changelogs/unreleased/dm-deploy-keys-default-user.yml
new file mode 100644
index 00000000000..b82d67d028c
--- /dev/null
+++ b/changelogs/unreleased/dm-deploy-keys-default-user.yml
@@ -0,0 +1,5 @@
+---
+title: Ensure hooks run when a deploy key without a user pushes
+merge_request:
+author:
+type: fixed
diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb
index b0b7b50998f..70d43ac1d79 100644
--- a/lib/api/deploy_keys.rb
+++ b/lib/api/deploy_keys.rb
@@ -54,7 +54,7 @@ module API
         present key, with: Entities::DeployKeysProject
       end
 
-      desc 'Add new deploy key to currently authenticated user' do
+      desc 'Add new deploy key to a project' do
         success Entities::DeployKeysProject
       end
       params do
@@ -66,33 +66,32 @@ module API
         params[:key].strip!
 
         # Check for an existing key joined to this project
-        key = user_project.deploy_keys_projects
+        deploy_key_project = user_project.deploy_keys_projects
                           .joins(:deploy_key)
                           .find_by(keys: { key: params[:key] })
 
-        if key
-          present key, with: Entities::DeployKeysProject
+        if deploy_key_project
+          present deploy_key_project, with: Entities::DeployKeysProject
           break
         end
 
         # Check for available deploy keys in other projects
         key = current_user.accessible_deploy_keys.find_by(key: params[:key])
         if key
-          added_key = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push])
+          deploy_key_project = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push])
 
-          present added_key, with: Entities::DeployKeysProject
+          present deploy_key_project, with: Entities::DeployKeysProject
           break
         end
 
         # Create a new deploy key
-        key_attributes = { can_push: !!params[:can_push],
-                           deploy_key_attributes: declared_params.except(:can_push) }
-        key = add_deploy_keys_project(user_project, key_attributes)
+        deploy_key_attributes = declared_params.except(:can_push).merge(user: current_user)
+        deploy_key_project = add_deploy_keys_project(user_project, deploy_key_attributes: deploy_key_attributes, can_push: !!params[:can_push])
 
-        if key.valid?
-          present key, with: Entities::DeployKeysProject
+        if deploy_key_project.valid?
+          present deploy_key_project, with: Entities::DeployKeysProject
         else
-          render_validation_error!(key)
+          render_validation_error!(deploy_key_project)
         end
       end
 
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 0a2a23e835b..ed0644f6cf1 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -99,8 +99,6 @@ module Gitlab
     end
 
     def check_active_user!
-      return if deploy_key?
-
       if user && !user_access.allowed?
         raise UnauthorizedError, ERROR_MESSAGES[:account_blocked]
       end
@@ -215,7 +213,7 @@ module Gitlab
         raise UnauthorizedError, ERROR_MESSAGES[:read_only]
       end
 
-      if deploy_key
+      if deploy_key?
         unless deploy_key.can_push_to?(project)
           raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload]
         end
@@ -305,8 +303,10 @@ module Gitlab
         case actor
         when User
           actor
+        when DeployKey
+          nil
         when Key
-          actor.user unless actor.is_a?(DeployKey)
+          actor.user
         when :ci
           nil
         end
diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb
index 3d7283e2164..41440c6d288 100644
--- a/spec/models/deploy_key_spec.rb
+++ b/spec/models/deploy_key_spec.rb
@@ -17,4 +17,25 @@ describe DeployKey, :mailer do
       should_not_email(user)
     end
   end
+
+  describe '#user' do
+    let(:deploy_key) { create(:deploy_key) }
+    let(:user) { create(:user) }
+
+    context 'when user is set' do
+      before do
+        deploy_key.user = user
+      end
+
+      it 'returns the user' do
+        expect(deploy_key.user).to be(user)
+      end
+    end
+
+    context 'when user is not set' do
+      it 'returns the ghost user' do
+        expect(deploy_key.user).to eq(User.ghost)
+      end
+    end
+  end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bbfdda23a31..100418da804 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -25,7 +25,7 @@ describe User do
     it { is_expected.to have_many(:group_members) }
     it { is_expected.to have_many(:groups) }
     it { is_expected.to have_many(:keys).dependent(:destroy) }
-    it { is_expected.to have_many(:deploy_keys).dependent(:destroy) }
+    it { is_expected.to have_many(:deploy_keys).dependent(:nullify) }
     it { is_expected.to have_many(:events).dependent(:destroy) }
     it { is_expected.to have_many(:issues).dependent(:destroy) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb
index 0772b3f2e64..ae9c0e9c304 100644
--- a/spec/requests/api/deploy_keys_spec.rb
+++ b/spec/requests/api/deploy_keys_spec.rb
@@ -91,6 +91,10 @@ describe API::DeployKeys do
       expect do
         post api("/projects/#{project.id}/deploy_keys", admin), key_attrs
       end.to change { project.deploy_keys.count }.by(1)
+
+      new_key = project.deploy_keys.last
+      expect(new_key.key).to eq(key_attrs[:key])
+      expect(new_key.user).to eq(admin)
     end
 
     it 'returns an existing ssh key when attempting to add a duplicate' do