Fix project deploy key creation and deletion as admin

In EE, the DeployKeyController reloads the user's accesible key so that
it can audit changes. However, this has a side effect of preventing
admins from deleting keys in the interface.  Note that the deletion
problem does not happen in CE because auditing is not performed there.

We can clean up the code by reusing the key that was created or destroyed
and pushing the logic into the service.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/53783

app/controllers/projects/deploy_keys_controller.rb

@@ -48,9 +48,9 @@ class Projects::DeployKeysController < Projects::ApplicationController
   end
 
   def enable
-    load_key
-    Projects::EnableDeployKeyService.new(@project, current_user, params).execute
-    log_audit_event(@key.title, action: :create)
+    key = Projects::EnableDeployKeyService.new(@project, current_user, params).execute
+
+    return render_404 unless key
 
     respond_to do |format|
       format.html { redirect_to_repository_settings(@project, anchor: 'js-deploy-keys-settings') }
@@ -58,21 +58,16 @@ class Projects::DeployKeysController < Projects::ApplicationController
     end
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def disable
-    deploy_key_project = @project.deploy_keys_projects.find_by(deploy_key_id: params[:id])
-    return render_404 unless deploy_key_project
+    deploy_key_project = Projects::DisableDeployKeyService.new(@project, current_user, params).execute
 
-    load_key
-    deploy_key_project.destroy!
-    log_audit_event(@key.title, action: :destroy)
+    return render_404 unless deploy_key_project
 
     respond_to do |format|
       format.html { redirect_to_repository_settings(@project, anchor: 'js-deploy-keys-settings') }
       format.json { head :ok }
     end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   protected
 
@@ -99,8 +94,4 @@ class Projects::DeployKeysController < Projects::ApplicationController
     AuditEventService.new(current_user, @project, options)
       .for_deploy_key(key_title).security_event
   end
-
-  def load_key
-    @key ||= current_user.accessible_deploy_keys.find(params[:id])
-  end
 end

app/services/projects/disable_deploy_key_service.rb

+# frozen_string_literal: true
+
+module Projects
+  class DisableDeployKeyService < BaseService
+    def execute
+      # rubocop: disable CodeReuse/ActiveRecord
+      deploy_key_project = project.deploy_keys_projects.find_by(deploy_key_id: params[:id])
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      deploy_key_project&.destroy!
+    end
+  end
+end
+
+Projects::DisableDeployKeyService.prepend(EE::Projects::DisableDeployKeyService)

app/services/projects/enable_deploy_key_service.rb

@@ -2,9 +2,10 @@
 
 module Projects
   class EnableDeployKeyService < BaseService
-    # rubocop: disable CodeReuse/ActiveRecord
     def execute
-      key = accessible_keys.find_by(id: params[:key_id] || params[:id])
+      key_id = params[:key_id] || params[:id]
+      key = find_accessible_key(key_id)
+
       return unless key
 
       unless project.deploy_keys.include?(key)
@@ -13,12 +14,17 @@ module Projects
 
       key
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     private
 
-    def accessible_keys
-      current_user.accessible_deploy_keys
+    def find_accessible_key(key_id)
+      if current_user.admin?
+        DeployKey.find_by_id(key_id)
+      else
+        current_user.accessible_deploy_keys.find_by_id(key_id)
+      end
     end
   end
 end
+
+Projects::EnableDeployKeyService.prepend(EE::Projects::EnableDeployKeyService)

ee/app/services/ee/projects/disable_deploy_key_service.rb

+# frozen_string_literal: true
+
+module EE::Projects::DisableDeployKeyService
+  extend ActiveSupport::Concern
+  extend ::Gitlab::Utils::Override
+
+  override :execute
+  def execute
+    super.tap do |deploy_key_project|
+      break unless deploy_key_project
+
+      log_audit_event(deploy_key_project.deploy_key.title, action: :destroy)
+    end
+  end
+
+  private
+
+  def log_audit_event(key_title, options = {})
+    AuditEventService.new(current_user, project, options)
+      .for_deploy_key(key_title).security_event
+  end
+end

ee/app/services/ee/projects/enable_deploy_key_service.rb

+# frozen_string_literal: true
+
+module EE::Projects::EnableDeployKeyService
+  extend ActiveSupport::Concern
+  extend ::Gitlab::Utils::Override
+
+  override :execute
+  def execute
+    super.tap do |key|
+      break unless key
+
+      log_audit_event(key.title, action: :create)
+    end
+  end
+
+  private
+
+  def log_audit_event(key_title, options = {})
+    AuditEventService.new(current_user, project, options)
+      .for_deploy_key(key_title).security_event
+  end
+end

ee/changelogs/unreleased/sh-fix-issue-53783.yml

+---
+title: Fix project deploy key creation and deletion as admin
+merge_request: 8432
+author:
+type: fixed

spec/controllers/projects/deploy_keys_controller_spec.rb

@@ -27,12 +27,8 @@ describe Projects::DeployKeysController do
       let(:project2) { create(:project, :internal)}
       let(:project_private) { create(:project, :private)}
 
-      let(:deploy_key_internal) do
-        create(:deploy_key, key: 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAgQCdMHEHyhRjbhEZVddFn6lTWdgEy5Q6Bz4nwGB76xWZI5YT/1WJOMEW+sL5zYd31kk7sd3FJ5L9ft8zWMWrr/iWXQikC2cqZK24H1xy+ZUmrRuJD4qGAaIVoyyzBL+avL+lF8J5lg6YSw8gwJY/lX64/vnJHUlWw2n5BF8IFOWhiw== dummy@gitlab.com')
-      end
-      let(:deploy_key_actual) do
-        create(:deploy_key, key: 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAgQDNd/UJWhPrpb+b/G5oL109y57yKuCxE+WUGJGYaj7WQKsYRJmLYh1mgjrl+KVyfsWpq4ylOxIfFSnN9xBBFN8mlb0Fma5DC7YsSsibJr3MZ19ZNBprwNcdogET7aW9I0In7Wu5f2KqI6e5W/spJHCy4JVxzVMUvk6Myab0LnJ2iQ== dummy@gitlab.com')
-      end
+      let(:deploy_key_internal) { create(:deploy_key) }
+      let(:deploy_key_actual) { create(:deploy_key) }
       let!(:deploy_key_public) { create(:deploy_key, public: true) }
 
       let!(:deploy_keys_project_internal) do
@@ -63,4 +59,147 @@ describe Projects::DeployKeysController do
       end
     end
   end
+
+  describe '/enable/:id' do
+    let(:deploy_key) { create(:deploy_key) }
+    let(:project2) { create(:project) }
+    let!(:deploy_keys_project_internal) do
+      create(:deploy_keys_project, project: project2, deploy_key: deploy_key)
+    end
+
+    context 'with anonymous user' do
+      before do
+        sign_out(:user)
+      end
+
+      it 'redirects to login' do
+        expect do
+          put :enable, id: deploy_key.id, namespace_id: project.namespace, project_id: project
+        end.not_to change { DeployKeysProject.count }
+
+        expect(response).to have_http_status(302)
+        expect(response).to redirect_to(new_user_session_path)
+      end
+    end
+
+    context 'with user with no permission' do
+      before do
+        sign_in(create(:user))
+      end
+
+      it 'returns 404' do
+        expect do
+          put :enable, id: deploy_key.id, namespace_id: project.namespace, project_id: project
+        end.not_to change { DeployKeysProject.count }
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'with user with permission' do
+      before do
+        project2.add_maintainer(user)
+      end
+
+      it 'returns 302' do
+        expect do
+          put :enable, id: deploy_key.id, namespace_id: project.namespace, project_id: project
+        end.to change { DeployKeysProject.count }.by(1)
+
+        expect(DeployKeysProject.where(project_id: project.id, deploy_key_id: deploy_key.id).count).to eq(1)
+        expect(response).to have_http_status(302)
+        expect(response).to redirect_to(namespace_project_settings_repository_path(anchor: 'js-deploy-keys-settings'))
+      end
+
+      it 'returns 404' do
+        put :enable, id: 0, namespace_id: project.namespace, project_id: project
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'with admin' do
+      before do
+        sign_in(create(:admin))
+      end
+
+      it 'returns 302' do
+        expect do
+          put :enable, id: deploy_key.id, namespace_id: project.namespace, project_id: project
+        end.to change { DeployKeysProject.count }.by(1)
+           .and change { AuditEvent.count }.by(1) ## EE only
+
+        expect(DeployKeysProject.where(project_id: project.id, deploy_key_id: deploy_key.id).count).to eq(1)
+        expect(response).to have_http_status(302)
+        expect(response).to redirect_to(namespace_project_settings_repository_path(anchor: 'js-deploy-keys-settings'))
+      end
+    end
+  end
+
+  describe '/disable/:id' do
+    let(:deploy_key) { create(:deploy_key) }
+    let!(:deploy_key_project) { create(:deploy_keys_project, project: project, deploy_key: deploy_key) }
+
+    context 'with anonymous user' do
+      before do
+        sign_out(:user)
+      end
+
+      it 'redirects to login' do
+        put :disable, id: deploy_key.id, namespace_id: project.namespace, project_id: project
+
+        expect(response).to have_http_status(302)
+        expect(response).to redirect_to(new_user_session_path)
+        expect(DeployKey.find(deploy_key.id)).to eq(deploy_key)
+      end
+    end
+
+    context 'with user with no permission' do
+      before do
+        sign_in(create(:user))
+      end
+
+      it 'returns 404' do
+        put :disable, id: deploy_key.id, namespace_id: project.namespace, project_id: project
+
+        expect(response).to have_http_status(404)
+        expect(DeployKey.find(deploy_key.id)).to eq(deploy_key)
+      end
+    end
+
+    context 'with user with permission' do
+      it 'returns 302' do
+        put :disable, id: deploy_key.id, namespace_id: project.namespace, project_id: project
+
+        expect(response).to have_http_status(302)
+        expect(response).to redirect_to(namespace_project_settings_repository_path(anchor: 'js-deploy-keys-settings'))
+
+        expect { DeployKey.find(deploy_key.id) }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'returns 404' do
+        put :disable, id: 0, namespace_id: project.namespace, project_id: project
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'with admin' do
+      before do
+        sign_in(create(:admin))
+      end
+
+      it 'returns 302' do
+        expect do
+          put :disable, id: deploy_key.id, namespace_id: project.namespace, project_id: project
+        end.to change { DeployKey.count }.by(-1)
+           .and change { AuditEvent.count }.by(1) ## EE only
+
+        expect(response).to have_http_status(302)
+        expect(response).to redirect_to(namespace_project_settings_repository_path(anchor: 'js-deploy-keys-settings'))
+
+        expect { DeployKey.find(deploy_key.id) }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+  end
 end