Merge branch 'reduce-deploy_keys_controller-diff-with-ce' into 'master'

Reduce diff with CE in DeployKeys::CreateService

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

app/controllers/projects/deploy_keys_controller.rb

@@ -24,11 +24,9 @@ class Projects::DeployKeysController < Projects::ApplicationController
   end
 
   def create
-    @key = DeployKeys::CreateService.new(current_user, create_params).execute
+    @key = DeployKeys::CreateService.new(current_user, create_params).execute(project: @project)
 
-    if @key.valid?
-      log_audit_event(@key.title, action: :create)
-    else
+    unless @key.valid?
       flash[:alert] = @key.errors.full_messages.join(', ').html_safe
     end
 
@@ -89,9 +87,4 @@ class Projects::DeployKeysController < Projects::ApplicationController
   def authorize_update_deploy_key!
     access_denied! unless can?(current_user, :update_deploy_key, deploy_key)
   end
-
-  def log_audit_event(key_title, options = {})
-    AuditEventService.new(current_user, @project, options)
-      .for_deploy_key(key_title).security_event
-  end
 end

app/services/deploy_keys/create_service.rb

@@ -2,8 +2,10 @@
 
 module DeployKeys
   class CreateService < Keys::BaseService
-    def execute
+    def execute(project: nil)
       DeployKey.create(params.merge(user: user))
     end
   end
 end
+
+DeployKeys::CreateService.prepend(::EE::DeployKeys::CreateService)

ee/app/services/ee/deploy_keys/create_service.rb

+# frozen_string_literal: true
+
+module EE
+  module DeployKeys
+    module CreateService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(project: nil)
+        super.tap do |key|
+          if project && key.persisted?
+            log_audit_event(key.title, project, action: :create)
+          end
+        end
+      end
+
+      private
+
+      def log_audit_event(key_title, project, options = {})
+        ::AuditEventService.new(user, project, options)
+          .for_deploy_key(key_title).security_event
+      end
+    end
+  end
+end

ee/spec/controllers/projects/deploy_keys_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::DeployKeysController do
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user) }
+
+  before do
+    project.add_maintainer(user)
+
+    sign_in(user)
+  end
+
+  describe 'POST create' do
+    let(:deploy_key_attrs) { attributes_for(:deploy_key) }
+    let(:title) { 'my-key' }
+    let(:params) do
+      {
+        namespace_id: project.namespace.path,
+        project_id: project.path,
+        deploy_key: {
+          title: title,
+          key: deploy_key_attrs[:key],
+          deploy_keys_projects_attributes: { '0' => { can_push: '1' } }
+        }
+      }
+    end
+
+    it 'records an audit event' do
+      expect { post :create, params: params }.to change { AuditEvent.count }.by(1)
+
+      expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-deploy-keys-settings'))
+    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 user with permission' do
+      before do
+        project2.add_maintainer(user)
+      end
+
+      it 'records an audit event' do
+        expect do
+          put :enable, params: { id: deploy_key.id, namespace_id: project.namespace, project_id: project }
+        end.to change { AuditEvent.count }.by(1)
+      end
+
+      it 'returns 404' do
+        put :enable, params: { id: 0, namespace_id: project.namespace, project_id: project }
+
+        expect(response).to have_http_status(404)
+      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 admin' do
+      before do
+        sign_in(create(:admin))
+      end
+
+      it 'records an audit event' do
+        expect do
+          put :disable, params: { id: deploy_key.id, namespace_id: project.namespace, project_id: project }
+        end.to change { AuditEvent.count }.by(1)
+      end
+    end
+  end
+end

spec/controllers/projects/deploy_keys_controller_spec.rb

@@ -16,7 +16,7 @@ describe Projects::DeployKeysController do
     end
 
     context 'when html requested' do
-      it 'redirects to blob' do
+      it 'redirects to project settings with the correct anchor' do
         get :index, params: params
 
         expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-deploy-keys-settings'))
@@ -60,6 +60,40 @@ describe Projects::DeployKeysController do
     end
   end
 
+  describe 'POST create' do
+    def create_params(title = 'my-key')
+      {
+        namespace_id: project.namespace.path,
+        project_id: project.path,
+        deploy_key: {
+          title: title,
+          key: attributes_for(:deploy_key)[:key],
+          deploy_keys_projects_attributes: { '0' => { can_push: '1' } }
+        }
+      }
+    end
+
+    it 'creates a new deploy key for the project' do
+      expect { post :create, params: create_params }.to change(project.deploy_keys, :count).by(1)
+
+      expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-deploy-keys-settings'))
+    end
+
+    it 'redirects to project settings with the correct anchor' do
+      post :create, params: create_params
+
+      expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-deploy-keys-settings'))
+    end
+
+    context 'when the deploy key is invalid' do
+      it 'shows an alert with the validations errors' do
+        post :create, params: create_params(nil)
+
+        expect(flash[:alert]).to eq("Title can't be blank, Deploy keys projects deploy key title can't be blank")
+      end
+    end
+  end
+
   describe '/enable/:id' do
     let(:deploy_key) { create(:deploy_key) }
     let(:project2) { create(:project) }
@@ -127,7 +161,6 @@ describe Projects::DeployKeysController do
         expect do
           put :enable, params: { 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)
@@ -193,7 +226,6 @@ describe Projects::DeployKeysController do
         expect do
           put :disable, params: { 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'))