Merge branch 'sh-fix-issue-53783' into 'master'

Fix project deploy key creation and deletion as admin

Closes gitlab-ce#53783

See merge request gitlab-org/gitlab-ee!8432

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