Merge branch 'refactor-services-for-audit-events' into 'master'

[EE] Refactor controller calls into services

Closes #3544

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

app/controllers/admin/applications_controller.rb

 class Admin::ApplicationsController < Admin::ApplicationController
   include OauthApplications
-  prepend EE::Admin::ApplicationsController
 
   before_action :set_application, only: [:show, :edit, :update, :destroy]
   before_action :load_scopes, only: [:new, :create, :edit, :update]
@@ -20,10 +19,12 @@ class Admin::ApplicationsController < Admin::ApplicationController
   end
 
   def create
-    @application = Doorkeeper::Application.new(application_params)
+    @application = Applications::CreateService.new(current_user, application_params).execute(request)
 
-    if @application.save
-      redirect_to_admin_page
+    if @application.persisted?
+      flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
+
+      redirect_to admin_application_url(@application)
     else
       render :new
     end
@@ -42,13 +43,6 @@ class Admin::ApplicationsController < Admin::ApplicationController
     redirect_to admin_applications_url, status: 302, notice: 'Application was successfully destroyed.'
   end
 
-  protected
-
-  def redirect_to_admin_page
-    flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
-    redirect_to admin_application_url(@application)
-  end
-
   private
 
   def set_application

app/controllers/concerns/oauth_applications.rb

 module OauthApplications
   extend ActiveSupport::Concern
-  prepend ::EE::Concerns::OauthApplications
 
   included do
     before_action :prepare_scopes, only: [:create, :update]

app/controllers/oauth/applications_controller.rb

@@ -3,7 +3,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
   include Gitlab::GonHelper
   include PageLayoutHelper
   include OauthApplications
-  prepend ::EE::Oauth::ApplicationsController
 
   before_action :verify_user_oauth_applications_enabled
   before_action :authenticate_user!
@@ -17,25 +16,18 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
   end
 
   def create
-    @application = Doorkeeper::Application.new(application_params)
+    @application = Applications::CreateService.new(current_user, create_application_params).execute(request)
 
-    @application.owner = current_user
+    if @application.persisted?
+      flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
 
-    if @application.save
-      redirect_to_oauth_application_page
+      redirect_to oauth_application_url(@application)
     else
       set_index_vars
       render :index
     end
   end
 
-  protected
-
-  def redirect_to_oauth_application_page
-    flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
-    redirect_to oauth_application_url(@application)
-  end
-
   private
 
   def verify_user_oauth_applications_enabled
@@ -62,4 +54,10 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
   rescue_from ActiveRecord::RecordNotFound do |exception|
     render "errors/not_found", layout: "errors", status: 404
   end
+
+  def create_application_params
+    application_params.tap do |params|
+      params[:owner] = current_user
+    end
+  end
 end

app/controllers/profiles/keys_controller.rb

 class Profiles::KeysController < Profiles::ApplicationController
-  prepend ::EE::Profiles::KeysController
-
   skip_before_action :authenticate_user!, only: [:get_keys]
 
   def index
@@ -13,10 +11,10 @@ class Profiles::KeysController < Profiles::ApplicationController
   end
 
   def create
-    @key = Keys::CreateService.new(current_user, key_params).execute
+    @key = Keys::CreateService.new(current_user, key_params.merge(ip_address: request.remote_ip)).execute
 
     if @key.persisted?
-      redirect_to_profile_key_path
+      redirect_to profile_key_path(@key)
     else
       @keys = current_user.keys.select(&:persisted?)
       render :index
@@ -52,12 +50,6 @@ class Profiles::KeysController < Profiles::ApplicationController
     end
   end
 
-  protected
-
-  def redirect_to_profile_key_path
-    redirect_to profile_key_path(@key)
-  end
-
   private
 
   def key_params

app/services/applications/create_service.rb

+module Applications
+  class CreateService
+    prepend ::EE::Applications::CreateService
+
+    def initialize(current_user, params)
+      @current_user = current_user
+      @params = params
+    end
+
+    def execute(request = nil)
+      Doorkeeper::Application.create(@params)
+    end
+  end
+end

app/services/keys/base_service.rb

@@ -4,6 +4,7 @@ module Keys
 
     def initialize(user, params)
       @user, @params = user, params
+      @ip_address = @params.delete(:ip_address)
     end
 
     def notification_service

app/services/keys/create_service.rb

 module Keys
   class CreateService < ::Keys::BaseService
+    prepend EE::Keys::CreateService
+
     def execute
       key = user.keys.create(params)
       notification_service.new_key(key) if key.persisted?

ee/app/controllers/ee/concerns/oauth_applications.rb

-module EE
-  module Concerns
-    module OauthApplications
-      extend ActiveSupport::Concern
-
-      def log_audit_event
-        ::AuditEventService.new(current_user,
-                                current_user,
-                                action: :custom,
-                                custom_message: 'OAuth access granted',
-                                ip_address: request.remote_ip)
-            .for_user(@application.name).security_event
-      end
-    end
-  end
-end

ee/app/controllers/ee/oauth/applications_controller.rb

-module EE
-  module Oauth
-    module ApplicationsController
-      protected
-
-      def redirect_to_oauth_application_page
-        raise NotImplementedError unless defined?(super)
-
-        log_audit_event
-
-        super
-      end
-    end
-  end
-end

ee/app/controllers/ee/profiles/keys_controller.rb

-module EE
-  module Profiles
-    module KeysController
-      protected
-
-      def redirect_to_profile_key_path
-        raise NotImplementedError unless defined?(super)
-
-        log_audit_event
-
-        super
-      end
-
-      private
-
-      def log_audit_event
-        ::AuditEventService.new(current_user,
-                              current_user,
-                              action: :custom,
-                              custom_message: 'Added SSH key',
-                              ip_address: request.remote_ip)
-            .for_user(@key.title).security_event
-      end
-    end
-  end
-end

ee/app/services/ee/applications/create_service.rb

+module EE
+  module Applications
+    module CreateService
+      def execute(request)
+        super.tap do |application|
+          audit_event_service(request.remote_ip).for_user(application.name).security_event
+        end
+      end
+
+      def audit_event_service(ip_address)
+        ::AuditEventService.new(@current_user,
+                                @current_user,
+                                action: :custom,
+                                custom_message: 'OAuth access granted',
+                                ip_address: ip_address)
+      end
+    end
+  end
+end

ee/app/services/ee/keys/create_service.rb

+module EE
+  module Keys
+    module CreateService
+      def execute
+        super.tap do |key|
+          log_audit_event(key)
+        end
+      end
+
+      def log_audit_event(key)
+        audit_event_service.for_user(key.title).security_event
+      end
+
+      def audit_event_service
+        ::AuditEventService.new(@user,
+                                @user,
+                                action: :custom,
+                                custom_message: 'Added SSH key',
+                                ip_address: @ip_address)
+      end
+    end
+  end
+end

spec/controllers/oauth/applications_controller_spec.rb

@@ -35,7 +35,7 @@ describe Oauth::ApplicationsController do
         application = build(:oauth_application)
         application_attributes = application.attributes.merge(scopes: [])
 
-        expect { post :create, doorkeeper_application:  application_attributes }.to change { SecurityEvent.count }.by(1)
+        expect { post :create, doorkeeper_application: application_attributes }.to change { SecurityEvent.count }.by(1)
       end
     end
   end

spec/ee/spec/services/ee/applications/create_service_spec.rb

+require 'spec_helper'
+
+describe ::Applications::CreateService do
+  let(:user) { create(:user) }
+  let(:params) { attributes_for(:application) }
+  let(:request) { ActionController::TestRequest.new(remote_ip: '127.0.0.1') }
+
+  subject { described_class.new(user, params) }
+
+  it 'creates an audit log' do
+    stub_licensed_features(extended_audit_events: true)
+
+    expect { subject.execute(request) }.to change { SecurityEvent.count }.by(1)
+  end
+end

spec/ee/spec/services/ee/keys/create_service_spec.rb

+require 'spec_helper'
+
+describe Keys::CreateService do
+  let(:user) { create(:user) }
+  let(:params) { attributes_for(:key) }
+
+  subject { described_class.new(user, params) }
+
+  it 'creates' do
+    stub_licensed_features(extended_audit_events: true)
+
+    expect { subject.execute }.to change { SecurityEvent.count }.by(1)
+  end
+end

spec/services/applications/create_service_spec.rb

+require 'spec_helper'
+
+describe ::Applications::CreateService do
+  let(:user) { create(:user) }
+  let(:params) { attributes_for(:application) }
+  let(:request) { ActionController::TestRequest.new(remote_ip: '127.0.0.1') }
+
+  subject { described_class.new(user, params) }
+
+  it 'creates an application' do
+    expect { subject.execute(request) }.to change { Doorkeeper::Application.count }.by(1)
+  end
+end