From 2d615c3072d3c8491a2be512722c4e3df4b52a87 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 3 Jul 2017 09:34:14 +0200
Subject: [PATCH 01/50] rebase and refactor user specs

---
 spec/requests/api/users_spec.rb | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 2e4d753007c85c..040458c9c7cdec 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -478,6 +478,7 @@
 
       expect(response).to have_http_status(200)
       expect(user.reload.password_expires_at).to be <= Time.now
+      expect(AuditEvent.count).to eq(1)
     end
 
     it "updates user with organization" do
@@ -488,6 +489,14 @@
       expect(user.reload.organization).to eq('GitLab')
     end
 
+    it 'updates user with a new email' do
+      put api("/users/#{user.id}", admin), email: 'new@email.com'
+
+      expect(response).to have_http_status(200)
+      expect(user.reload.notification_email).to eq('new@email.com')
+      expect(AuditEvent.count).to eq(1)
+    end
+
     it 'updates user with avatar' do
       put api("/users/#{user.id}", admin), { avatar: fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
 
-- 
GitLab


From 76bcb1d996d666adf5ce5337eb7fe2c09eb184d1 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 3 Jul 2017 11:13:38 +0200
Subject: [PATCH 02/50] add new changes tests

---
 spec/lib/ee/audit/changes_spec.rb   | 30 +++++++++++++++++++++++++++++
 spec/lib/gitlab/ldap/access_spec.rb |  1 +
 2 files changed, 31 insertions(+)
 create mode 100644 spec/lib/ee/audit/changes_spec.rb

diff --git a/spec/lib/ee/audit/changes_spec.rb b/spec/lib/ee/audit/changes_spec.rb
new file mode 100644
index 00000000000000..f96b2ddfd553ea
--- /dev/null
+++ b/spec/lib/ee/audit/changes_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe EE::Audit::Changes do
+  describe '.audit_changes' do
+    let(:user) { create(:user) }
+    let(:foo_class) { Class.new { include EE::Audit::Changes } }
+
+    before do
+      allow_any_instance_of(foo_class).to receive(:model).and_return(user)
+    end
+
+    describe 'non audit changes' do
+      it 'does not call the audit event service' do
+        expect_any_instance_of(AuditEventService).not_to receive(:security_event)
+
+        user.name = 'new name'
+        foo_class.new.audit_changes(:email)
+      end
+    end
+
+    describe 'audit changes' do
+      it 'calls the audit event service' do
+        expect_any_instance_of(AuditEventService).to receive(:security_event)
+
+        user.name = 'new name'
+        foo_class.new.audit_changes(:name)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb
index b592307dc12f50..be856d142df1cb 100644
--- a/spec/lib/gitlab/ldap/access_spec.rb
+++ b/spec/lib/gitlab/ldap/access_spec.rb
@@ -203,6 +203,7 @@
 
     it 'updates email address' do
       expect(access).to receive(:update_email).once
+      expect(AuditEvents.count).to eq(1)
 
       subject
     end
-- 
GitLab


From a9743cb27452a27a744d731554af3ae641145f57 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 3 Jul 2017 12:00:35 +0200
Subject: [PATCH 03/50] add EE audit changes class

---
 lib/ee/audit/changes.rb | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 lib/ee/audit/changes.rb

diff --git a/lib/ee/audit/changes.rb b/lib/ee/audit/changes.rb
new file mode 100644
index 00000000000000..278ae5a2d3360b
--- /dev/null
+++ b/lib/ee/audit/changes.rb
@@ -0,0 +1,39 @@
+module EE
+  module Audit
+    module Changes
+      def audit_changes(column, options = {})
+        return unless model.send("#{column}_changed?")
+
+        @column = column
+        @options = generate_options(options)
+
+        audit_event
+      end
+
+      protected
+
+      def model
+        raise NotImplementedError, "#{self} does not implement #{__method__}"
+      end
+
+      private
+
+      def generate_options(options)
+        options.tap do |options_hash|
+          options_hash[:column] = @column
+          options_hash[:action] = :update
+
+          unless options[:skip_changes]
+            options_hash[:from] = model.public_send("#{@column}_was")
+            options_hash[:to] = model.public_send("#{@column}")
+          end
+        end
+      end
+
+      def audit_event
+        AuditEventService.new(@current_user, model, @options).
+          for_changes.security_event
+      end
+    end
+  end
+end
-- 
GitLab


From 3a1ff1e32e7e6b902106abef600cb52ab9af7cc6 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 3 Jul 2017 12:17:54 +0200
Subject: [PATCH 04/50] update services to include the current user

---
 app/controllers/admin/users_controller.rb              |  6 +++---
 app/controllers/profiles/avatars_controller.rb         |  2 +-
 app/controllers/profiles/emails_controller.rb          |  4 ++--
 app/controllers/profiles/notifications_controller.rb   |  2 +-
 app/controllers/profiles/passwords_controller.rb       |  6 +++---
 app/controllers/profiles/preferences_controller.rb     |  2 +-
 .../profiles/two_factor_auths_controller.rb            |  6 +++---
 app/controllers/profiles_controller.rb                 | 10 +++++-----
 app/controllers/sessions_controller.rb                 |  2 +-
 app/models/user.rb                                     | 10 +++++-----
 lib/api/internal.rb                                    |  2 +-
 lib/api/notification_settings.rb                       |  2 +-
 lib/api/users.rb                                       | 10 +++++-----
 lib/gitlab/ldap/access.rb                              |  7 ++++---
 lib/gitlab/o_auth/user.rb                              |  2 +-
 15 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 8e914c29a9f759..9d2f7a6e9aef87 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -128,7 +128,7 @@ def update
     end
 
     respond_to do |format|
-      result = Users::UpdateService.new(user, user_params_with_pass).execute do |user|
+      result = Users::UpdateService.new(current_user, user, user_params_with_pass).execute do |user|
         user.skip_reconfirmation!
       end
 
@@ -155,7 +155,7 @@ def destroy
 
   def remove_email
     email = user.emails.find(params[:email_id])
-    success = Emails::DestroyService.new(user, email: email.email).execute
+    success = Emails::DestroyService.new(current_user, user, email: email.email).execute
 
     respond_to do |format|
       if success
@@ -226,7 +226,7 @@ def user_params_ee
   end
 
   def update_user(&block)
-    result = Users::UpdateService.new(user).execute(&block)
+    result = Users::UpdateService.new(current_user, user).execute(&block)
 
     result[:status] == :success
   end
diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb
index 408650aac548c0..5a94dc88455b00 100644
--- a/app/controllers/profiles/avatars_controller.rb
+++ b/app/controllers/profiles/avatars_controller.rb
@@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController
   def destroy
     @user = current_user
 
-    Users::UpdateService.new(@user).execute { |user| user.remove_avatar! }
+    Users::UpdateService.new(current_user, @user).execute { |user| user.remove_avatar! }
 
     redirect_to profile_path, status: 302
   end
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index ddb67d1c4d11f7..8328d23027607d 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -5,7 +5,7 @@ def index
   end
 
   def create
-    @email = Emails::CreateService.new(current_user, email_params).execute
+    @email = Emails::CreateService.new(current_user, current_user, email_params).execute
 
     if @email.errors.blank?
       NotificationService.new.new_email(@email)
@@ -19,7 +19,7 @@ def create
   def destroy
     @email = current_user.emails.find(params[:id])
 
-    Emails::DestroyService.new(current_user, email: @email.email).execute
+    Emails::DestroyService.new(current_user, current_user, email: @email.email).execute
 
     respond_to do |format|
       format.html { redirect_to profile_emails_url, status: 302 }
diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb
index 960b75126023c4..45d7bca27bbe31 100644
--- a/app/controllers/profiles/notifications_controller.rb
+++ b/app/controllers/profiles/notifications_controller.rb
@@ -7,7 +7,7 @@ def show
   end
 
   def update
-    result = Users::UpdateService.new(current_user, user_params).execute
+    result = Users::UpdateService.new(current_user, current_user, user_params).execute
 
     if result[:status] == :success
       flash[:notice] = "Notification settings saved"
diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb
index 7beb52dd8e86b8..08b438de9e25d6 100644
--- a/app/controllers/profiles/passwords_controller.rb
+++ b/app/controllers/profiles/passwords_controller.rb
@@ -21,10 +21,10 @@ def create
       password_automatically_set: false
     }
 
-    result = Users::UpdateService.new(@user, password_attributes).execute
+    result = Users::UpdateService.new(current_user, @user, password_attributes).execute
 
     if result[:status] == :success
-      Users::UpdateService.new(@user, password_expires_at: nil).execute
+      Users::UpdateService.new(current_user, @user, password_expires_at: nil).execute
 
       redirect_to root_path, notice: 'Password successfully changed'
     else
@@ -46,7 +46,7 @@ def update
       return
     end
 
-    result = Users::UpdateService.new(@user, password_attributes).execute
+    result = Users::UpdateService.new(current_user, @user, password_attributes).execute
 
     if result[:status] == :success
       flash[:notice] = "Password was successfully updated. Please login with it"
diff --git a/app/controllers/profiles/preferences_controller.rb b/app/controllers/profiles/preferences_controller.rb
index cce2a847b53890..a13b9a616cbb85 100644
--- a/app/controllers/profiles/preferences_controller.rb
+++ b/app/controllers/profiles/preferences_controller.rb
@@ -6,7 +6,7 @@ def show
 
   def update
     begin
-      result = Users::UpdateService.new(user, preferences_params).execute
+      result = Users::UpdateService.new(current_user, user, preferences_params).execute
 
       if result[:status] == :success
         flash[:notice] = 'Preferences saved.'
diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb
index 1a4f77639e7293..b590846257b2fc 100644
--- a/app/controllers/profiles/two_factor_auths_controller.rb
+++ b/app/controllers/profiles/two_factor_auths_controller.rb
@@ -10,7 +10,7 @@ def show
       current_user.otp_grace_period_started_at = Time.current
     end
 
-    Users::UpdateService.new(current_user).execute!
+    Users::UpdateService.new(current_user, current_user).execute!
 
     if two_factor_authentication_required? && !current_user.two_factor_enabled?
       two_factor_authentication_reason(
@@ -41,7 +41,7 @@ def show
 
   def create
     if current_user.validate_and_consume_otp!(params[:pin_code])
-      Users::UpdateService.new(current_user, otp_required_for_login: true).execute! do |user|
+      Users::UpdateService.new(current_user, current_user, otp_required_for_login: true).execute! do |user|
         @codes = user.generate_otp_backup_codes!
       end
 
@@ -70,7 +70,7 @@ def create_u2f
   end
 
   def codes
-    Users::UpdateService.new(current_user).execute! do |user|
+    Users::UpdateService.new(current_user, current_user).execute! do |user|
       @codes = user.generate_otp_backup_codes!
     end
   end
diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index d83824fef06e35..9e85997cf6deae 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -10,7 +10,7 @@ def show
 
   def update
     respond_to do |format|
-      result = Users::UpdateService.new(@user, user_params).execute
+      result = Users::UpdateService.new(current_user, @user, user_params).execute
 
       if result[:status] == :success
         message = "Profile was successfully updated"
@@ -25,7 +25,7 @@ def update
   end
 
   def reset_private_token
-    Users::UpdateService.new(@user).execute! do |user|
+    Users::UpdateService.new(current_user, @user).execute! do |user|
       user.reset_authentication_token!
     end
 
@@ -35,7 +35,7 @@ def reset_private_token
   end
 
   def reset_incoming_email_token
-    Users::UpdateService.new(@user).execute! do |user|
+    Users::UpdateService.new(current_user, @user).execute! do |user|
       user.reset_incoming_email_token!
     end
 
@@ -45,7 +45,7 @@ def reset_incoming_email_token
   end
 
   def reset_rss_token
-    Users::UpdateService.new(@user).execute! do |user|
+    Users::UpdateService.new(current_user, @user).execute! do |user|
       user.reset_rss_token!
     end
 
@@ -61,7 +61,7 @@ def audit_log
   end
 
   def update_username
-    result = Users::UpdateService.new(@user, username: user_params[:username]).execute
+    result = Users::UpdateService.new(current_user, @user, username: user_params[:username]).execute
 
     options = if result[:status] == :success
                 { notice: "Username successfully changed" }
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index 89c05aa35dc1f6..c3424398c6b398 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -57,7 +57,7 @@ def check_initial_setup
 
     return unless user && user.require_password_creation?
 
-    Users::UpdateService.new(user).execute do |user|
+    Users::UpdateService.new(current_user, user).execute do |user|
       @token = user.generate_reset_token
     end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 4d1e98fd29c166..00931c9fa39853 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -63,7 +63,7 @@ def update_tracked_fields!(request)
     lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i)
     return unless lease.try_obtain
 
-    Users::UpdateService.new(self).execute(validate: false)
+    Users::UpdateService.new(self, self).execute(validate: false)
   end
 
   attr_accessor :force_random_password
@@ -545,8 +545,8 @@ def owns_public_email
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
-      Emails::DestroyService.new(self, email: email).execute
-      Emails::CreateService.new(self, email: email_was).execute
+      Emails::DestroyService.new(self, self, email: email).execute
+      Emails::CreateService.new(self, self, email: email_was).execute
     end
   end
 
@@ -1023,7 +1023,7 @@ def increment_failed_attempts!
     if attempts_exceeded?
       lock_access! unless access_locked?
     else
-      Users::UpdateService.new(self).execute(validate: false)
+      Users::UpdateService.new(self, self).execute(validate: false)
     end
   end
 
@@ -1209,7 +1209,7 @@ def self.create_unique_internal(scope, username, email_pattern, &creation_block)
       &creation_block
     )
 
-    Users::UpdateService.new(user).execute(validate: false)
+    Users::UpdateService.new(user, user).execute(validate: false)
     user
   ensure
     Gitlab::ExclusiveLease.cancel(lease_key, uuid)
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index d30544a7a145ae..11b9bb4c5ecc17 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -149,7 +149,7 @@ class Internal < Grape::API
 
         codes = nil
 
-        ::Users::UpdateService.new(user).execute! do |user|
+        ::Users::UpdateService.new(current_user, user).execute! do |user|
           codes = user.generate_otp_backup_codes!
         end
 
diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb
index bcc0833aa5c0ba..1c5592d952d762 100644
--- a/lib/api/notification_settings.rb
+++ b/lib/api/notification_settings.rb
@@ -35,7 +35,7 @@ class NotificationSettings < Grape::API
             new_notification_email = params.delete(:notification_email)
 
             if new_notification_email
-              ::Users::UpdateService.new(current_user, notification_email: new_notification_email).execute
+              ::Users::UpdateService.new(current_user, current_user, notification_email: new_notification_email).execute
             end
 
             notification_setting.update(declared_params(include_missing: false))
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 49b7d9139b968f..8a07be5cb9414b 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -172,7 +172,7 @@ def find_user(params)
 
         user_params[:password_expires_at] = Time.now if user_params[:password].present?
 
-        result = ::Users::UpdateService.new(user, user_params.except(:extern_uid, :provider)).execute
+        result = ::Users::UpdateService.new(current_user, user, user_params.except(:extern_uid, :provider)).execute
 
         if result[:status] == :success
           present user, with: Entities::UserPublic
@@ -332,7 +332,7 @@ def find_user(params)
         user = User.find_by(id: params.delete(:id))
         not_found!('User') unless user
 
-        email = Emails::CreateService.new(user, declared_params(include_missing: false)).execute
+        email = Emails::CreateService.new(current_user, user, declared_params(include_missing: false)).execute
 
         if email.errors.blank?
           NotificationService.new.new_email(email)
@@ -373,7 +373,7 @@ def find_user(params)
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user, user, email: email.email).execute
         end
 
         user.update_secondary_emails!
@@ -678,7 +678,7 @@ def find_impersonation_token
         requires :email, type: String, desc: 'The new email'
       end
       post "emails" do
-        email = Emails::CreateService.new(current_user, declared_params).execute
+        email = Emails::CreateService.new(current_user, current_user, declared_params).execute
 
         if email.errors.blank?
           NotificationService.new.new_email(email)
@@ -697,7 +697,7 @@ def find_impersonation_token
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user, current_user, email: email.email).execute
         end
 
         current_user.update_secondary_emails!
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index 58264af840a26a..8d9c63624a5bfa 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -20,7 +20,7 @@ def self.allowed?(user, options = {})
           # permissions to keep things clean
           if access.allowed?
             access.update_user
-            Users::UpdateService.new(user, last_credential_check_at: Time.now).execute
+            Users::UpdateService.new(user, user, last_credential_check_at: Time.now).execute
 
             true
           else
@@ -172,8 +172,9 @@ def update_email
 
         return false if user.email == ldap_email
 
-        user.skip_reconfirmation!
-        user.update(email: ldap_email)
+        Users::UpdateService.new(user, user, email: ldap_email).execute do |user|
+          user.skip_reconfirmation!
+        end
       end
 
       def update_identity
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index 04ff854e26f6fa..60b6ed23c87509 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -34,7 +34,7 @@ def save(provider = 'OAuth')
 
         block_after_save = needs_blocking?
 
-        Users::UpdateService.new(gl_user).execute!
+        Users::UpdateService.new(gl_user, gl_user).execute!
 
         gl_user.block if block_after_save
 
-- 
GitLab


From defa48ea864379fdc9fd6d0c930ba7b3881cb75d Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 3 Jul 2017 12:48:29 +0200
Subject: [PATCH 05/50] update services and specs

---
 app/services/emails/base_service.rb          | 3 ++-
 app/services/emails/destroy_service.rb       | 2 +-
 app/services/users/update_service.rb         | 7 ++++++-
 spec/services/emails/create_service_spec.rb  | 2 +-
 spec/services/emails/destroy_service_spec.rb | 2 +-
 spec/services/users/update_service_spec.rb   | 4 ++--
 6 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index ace498890978f5..8810f6d8803dd0 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -1,6 +1,7 @@
 module Emails
   class BaseService
-    def initialize(user, opts)
+    def initialize(current_user, user, opts)
+      @current_user = current_user
       @user = user
       @email = opts[:email]
     end
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index d586b9dfe0c89e..af381f8ef0dffe 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -7,7 +7,7 @@ def execute
     private
 
     def update_secondary_emails!
-      result = ::Users::UpdateService.new(@user).execute do |user|
+      result = ::Users::UpdateService.new(@current_user, @user).execute do |user|
         user.update_secondary_emails!
       end
 
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 6188b8a43493a3..a39bd0fa688d52 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -2,7 +2,8 @@ module Users
   class UpdateService < BaseService
     include NewUserNotifier
 
-    def initialize(user, params = {})
+    def initialize(current_user, user, params = {})
+      @current_user = current_user
       @user = user
       @params = params.dup
     end
@@ -40,5 +41,9 @@ def assign_attributes(&block)
 
       @user.assign_attributes(params) if params.any?
     end
+
+    def model
+      @user
+    end
   end
 end
diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb
index 641d5538de8b08..1c8b81c222cd87 100644
--- a/spec/services/emails/create_service_spec.rb
+++ b/spec/services/emails/create_service_spec.rb
@@ -4,7 +4,7 @@
   let(:user) { create(:user) }
   let(:opts) { { email: 'new@email.com' } }
 
-  subject(:service) { described_class.new(user, opts) }
+  subject(:service) { described_class.new(user, user, opts) }
 
   describe '#execute' do
     it 'creates an email with valid attributes' do
diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb
index 1f4294dd90529f..138c3ff33b0f61 100644
--- a/spec/services/emails/destroy_service_spec.rb
+++ b/spec/services/emails/destroy_service_spec.rb
@@ -4,7 +4,7 @@
   let!(:user) { create(:user) }
   let!(:email) { create(:email, user: user) }
 
-  subject(:service) { described_class.new(user, email: email.email) }
+  subject(:service) { described_class.new(user, user, email: email.email) }
 
   describe '#execute' do
     it 'removes an email' do
diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb
index 6ee35a33b2d18f..91c8486792d06c 100644
--- a/spec/services/users/update_service_spec.rb
+++ b/spec/services/users/update_service_spec.rb
@@ -31,7 +31,7 @@
     end
 
     def update_user(user, opts)
-      described_class.new(user, opts).execute
+      described_class.new(user, user, opts).execute
     end
   end
 
@@ -65,7 +65,7 @@ def update_user(user, opts)
     end
 
     def update_user(user, opts)
-      described_class.new(user, opts).execute!
+      described_class.new(user, user, opts).execute!
     end
   end
 end
-- 
GitLab


From dff7db5acfb94c63f58b2e73a5e446d51e0467ab Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 4 Jul 2017 09:42:27 +0200
Subject: [PATCH 06/50] refactor changes framework and fix spec

---
 lib/ee/audit/changes.rb           | 29 +++++++++++++++++------------
 spec/lib/ee/audit/changes_spec.rb |  8 ++++----
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/lib/ee/audit/changes.rb b/lib/ee/audit/changes.rb
index 278ae5a2d3360b..977091c32d81d4 100644
--- a/lib/ee/audit/changes.rb
+++ b/lib/ee/audit/changes.rb
@@ -1,13 +1,10 @@
 module EE
   module Audit
     module Changes
-      def audit_changes(column, options = {})
-        return unless model.send("#{column}_changed?")
+      def audit_changes(current_user, column, options = {})
+        return unless changed?(column)
 
-        @column = column
-        @options = generate_options(options)
-
-        audit_event
+        audit_event(current_user, parse_options(column, options))
       end
 
       protected
@@ -18,20 +15,28 @@ def model
 
       private
 
-      def generate_options(options)
+      def changed?(column)
+        model.previous_changes.has_key?(column)
+      end
+
+      def changes(column)
+        model.previous_changes[column]
+      end
+
+      def parse_options(column, options)
         options.tap do |options_hash|
-          options_hash[:column] = @column
+          options_hash[:column] = column
           options_hash[:action] = :update
 
           unless options[:skip_changes]
-            options_hash[:from] = model.public_send("#{@column}_was")
-            options_hash[:to] = model.public_send("#{@column}")
+            options_hash[:from] = changes(column).first
+            options_hash[:to] = changes(column).last
           end
         end
       end
 
-      def audit_event
-        AuditEventService.new(@current_user, model, @options).
+      def audit_event(current_user, options)
+        AuditEventService.new(current_user, model, options).
           for_changes.security_event
       end
     end
diff --git a/spec/lib/ee/audit/changes_spec.rb b/spec/lib/ee/audit/changes_spec.rb
index f96b2ddfd553ea..8e5ae051674514 100644
--- a/spec/lib/ee/audit/changes_spec.rb
+++ b/spec/lib/ee/audit/changes_spec.rb
@@ -13,8 +13,8 @@
       it 'does not call the audit event service' do
         expect_any_instance_of(AuditEventService).not_to receive(:security_event)
 
-        user.name = 'new name'
-        foo_class.new.audit_changes(:email)
+        user.update!(name: 'new name')
+        foo_class.new.audit_changes(user, :email)
       end
     end
 
@@ -22,8 +22,8 @@
       it 'calls the audit event service' do
         expect_any_instance_of(AuditEventService).to receive(:security_event)
 
-        user.name = 'new name'
-        foo_class.new.audit_changes(:name)
+        user.update!(name: 'new name')
+        foo_class.new.audit_changes(user, :name)
       end
     end
   end
-- 
GitLab


From 82f5cd46433640298525a9b6fc43083bf9aac8a4 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 4 Jul 2017 15:55:50 +0200
Subject: [PATCH 07/50] add event changes to users update service

---
 app/services/audit_event_service.rb  | 15 +++++++++++++++
 app/services/users/update_service.rb |  4 ++++
 lib/ee/audit/changes.rb              | 10 ++++++----
 spec/lib/ee/audit/changes_spec.rb    |  9 +++++----
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/app/services/audit_event_service.rb b/app/services/audit_event_service.rb
index 940527b2f3c3ba..9878e218a42a1b 100644
--- a/app/services/audit_event_service.rb
+++ b/app/services/audit_event_service.rb
@@ -16,6 +16,21 @@ def for_authentication
     self
   end
 
+  def for_changes
+    @details =
+      {
+        change: @details[:as] || @details[:column],
+        from: @details[:from],
+        to: @details[:to],
+        author_name: @author.name,
+        target_id: @entity.id,
+        target_type: @entity.class,
+        target_details: @entity.name
+      }
+    self
+  end
+
+
   def security_event
     SecurityEvent.create(
       author_id: @author.id,
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index a39bd0fa688d52..29ec54893f09b4 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -1,5 +1,6 @@
 module Users
   class UpdateService < BaseService
+    include EE::Audit::Changes
     include NewUserNotifier
 
     def initialize(current_user, user, params = {})
@@ -16,6 +17,9 @@ def execute(validate: true, &block)
       user_exists = @user.persisted?
 
       if @user.save(validate: validate)
+        audit_changes :email, as: 'email address', column: :notification_email
+        audit_changes :encrypted_password, as: 'password', skip_changes: true
+
         notify_new_user(@user, nil) unless user_exists
 
         success
diff --git a/lib/ee/audit/changes.rb b/lib/ee/audit/changes.rb
index 977091c32d81d4..d8977e07dd7e10 100644
--- a/lib/ee/audit/changes.rb
+++ b/lib/ee/audit/changes.rb
@@ -1,10 +1,12 @@
 module EE
   module Audit
     module Changes
-      def audit_changes(current_user, column, options = {})
+      def audit_changes(column, options = {})
+        column = options[:column] || column
+
         return unless changed?(column)
 
-        audit_event(current_user, parse_options(column, options))
+        audit_event(parse_options(column, options))
       end
 
       protected
@@ -35,8 +37,8 @@ def parse_options(column, options)
         end
       end
 
-      def audit_event(current_user, options)
-        AuditEventService.new(current_user, model, options).
+      def audit_event(options)
+        AuditEventService.new(@current_user, model, options).
           for_changes.security_event
       end
     end
diff --git a/spec/lib/ee/audit/changes_spec.rb b/spec/lib/ee/audit/changes_spec.rb
index 8e5ae051674514..49e4bb78b8bf56 100644
--- a/spec/lib/ee/audit/changes_spec.rb
+++ b/spec/lib/ee/audit/changes_spec.rb
@@ -3,10 +3,11 @@
 describe EE::Audit::Changes do
   describe '.audit_changes' do
     let(:user) { create(:user) }
-    let(:foo_class) { Class.new { include EE::Audit::Changes } }
+    let(:foo_instance) { Class.new { include EE::Audit::Changes }.new }
 
     before do
-      allow_any_instance_of(foo_class).to receive(:model).and_return(user)
+      foo_instance.instance_variable_set(:@current_user, user)
+      allow(foo_instance).to receive(:model).and_return(user)
     end
 
     describe 'non audit changes' do
@@ -14,7 +15,7 @@
         expect_any_instance_of(AuditEventService).not_to receive(:security_event)
 
         user.update!(name: 'new name')
-        foo_class.new.audit_changes(user, :email)
+        foo_instance.audit_changes(:email)
       end
     end
 
@@ -23,7 +24,7 @@
         expect_any_instance_of(AuditEventService).to receive(:security_event)
 
         user.update!(name: 'new name')
-        foo_class.new.audit_changes(user, :name)
+        foo_instance.audit_changes(:name)
       end
     end
   end
-- 
GitLab


From 15df6af2ad348b8e8935584baeb07dc4d1344f08 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 1 Aug 2017 12:13:50 +0200
Subject: [PATCH 08/50] refactor audit changes after merge, fixed spec

---
 app/services/audit_event_service.rb       | 15 ---------------
 app/services/users/update_service.rb      |  4 ++--
 ee/app/services/ee/audit_event_service.rb | 14 ++++++++++++++
 lib/ee/audit/changes.rb                   |  2 +-
 spec/lib/gitlab/ldap/access_spec.rb       |  1 -
 5 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/app/services/audit_event_service.rb b/app/services/audit_event_service.rb
index 9878e218a42a1b..940527b2f3c3ba 100644
--- a/app/services/audit_event_service.rb
+++ b/app/services/audit_event_service.rb
@@ -16,21 +16,6 @@ def for_authentication
     self
   end
 
-  def for_changes
-    @details =
-      {
-        change: @details[:as] || @details[:column],
-        from: @details[:from],
-        to: @details[:to],
-        author_name: @author.name,
-        target_id: @entity.id,
-        target_type: @entity.class,
-        target_details: @entity.name
-      }
-    self
-  end
-
-
   def security_event
     SecurityEvent.create(
       author_id: @author.id,
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 29ec54893f09b4..57c8817c2893b1 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -17,8 +17,8 @@ def execute(validate: true, &block)
       user_exists = @user.persisted?
 
       if @user.save(validate: validate)
-        audit_changes :email, as: 'email address', column: :notification_email
-        audit_changes :encrypted_password, as: 'password', skip_changes: true
+        audit_changes(:email, as: 'email address', column: :notification_email)
+        audit_changes(:encrypted_password, as: 'password', skip_changes: true)
 
         notify_new_user(@user, nil) unless user_exists
 
diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index 705381230a9823..2b4b5bc65022c3 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -82,6 +82,20 @@ def for_failed_login
       self
     end
 
+    def for_changes
+      @details =
+          {
+              change: @details[:as] || @details[:column],
+              from: @details[:from],
+              to: @details[:to],
+              author_name: @author.name,
+              target_id: @entity.id,
+              target_type: @entity.class,
+              target_details: @entity.name
+          }
+      self
+    end
+
     def security_event
       if admin_audit_log_enabled?
         add_security_event_admin_details!
diff --git a/lib/ee/audit/changes.rb b/lib/ee/audit/changes.rb
index d8977e07dd7e10..0a36ea35bbe998 100644
--- a/lib/ee/audit/changes.rb
+++ b/lib/ee/audit/changes.rb
@@ -38,7 +38,7 @@ def parse_options(column, options)
       end
 
       def audit_event(options)
-        AuditEventService.new(@current_user, model, options).
+        ::AuditEventService.new(@current_user, model, options).
           for_changes.security_event
       end
     end
diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb
index be856d142df1cb..b592307dc12f50 100644
--- a/spec/lib/gitlab/ldap/access_spec.rb
+++ b/spec/lib/gitlab/ldap/access_spec.rb
@@ -203,7 +203,6 @@
 
     it 'updates email address' do
       expect(access).to receive(:update_email).once
-      expect(AuditEvents.count).to eq(1)
 
       subject
     end
-- 
GitLab


From 9cc7764adcea4e812c610f0abb19492663edcf8b Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 12 Sep 2017 09:28:17 +0200
Subject: [PATCH 09/50] refactor and fix spec

---
 spec/lib/ee/audit/changes_spec.rb | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/spec/lib/ee/audit/changes_spec.rb b/spec/lib/ee/audit/changes_spec.rb
index 49e4bb78b8bf56..976cedf287b800 100644
--- a/spec/lib/ee/audit/changes_spec.rb
+++ b/spec/lib/ee/audit/changes_spec.rb
@@ -7,24 +7,23 @@
 
     before do
       foo_instance.instance_variable_set(:@current_user, user)
+      foo_instance.instance_variable_set(:@user, user)
       allow(foo_instance).to receive(:model).and_return(user)
     end
 
     describe 'non audit changes' do
       it 'does not call the audit event service' do
-        expect_any_instance_of(AuditEventService).not_to receive(:security_event)
-
         user.update!(name: 'new name')
-        foo_instance.audit_changes(:email)
+
+        expect{ foo_instance.audit_changes(:email) }.not_to change { SecurityEvent.count }
       end
     end
 
     describe 'audit changes' do
       it 'calls the audit event service' do
-        expect_any_instance_of(AuditEventService).to receive(:security_event)
-
         user.update!(name: 'new name')
-        foo_instance.audit_changes(:name)
+
+        expect{ foo_instance.audit_changes(:name) }.to change { SecurityEvent.count }.by(1)
       end
     end
   end
-- 
GitLab


From e738b80ac41850c058b4f330ba8f60bd3b93d824 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 12 Sep 2017 09:52:56 +0200
Subject: [PATCH 10/50] more spec fixes

---
 spec/services/users/update_service_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb
index 91c8486792d06c..707f83b3359943 100644
--- a/spec/services/users/update_service_spec.rb
+++ b/spec/services/users/update_service_spec.rb
@@ -37,7 +37,7 @@ def update_user(user, opts)
 
   describe '#execute!' do
     it 'updates the name' do
-      service = described_class.new(user, name: 'New Name')
+      service = described_class.new(user, user, name: 'New Name')
       expect(service).not_to receive(:notify_new_user)
 
       result = service.execute!
@@ -55,7 +55,7 @@ def update_user(user, opts)
     it 'fires system hooks when a new user is saved' do
       system_hook_service = spy(:system_hook_service)
       user = build(:user)
-      service = described_class.new(user, name: 'John Doe')
+      service = described_class.new(user, user, name: 'John Doe')
       expect(service).to receive(:notify_new_user).and_call_original
       expect(service).to receive(:system_hook_service).and_return(system_hook_service)
 
-- 
GitLab


From 42f7a58bb945efc5636b8b2d39a4a3c623587ac0 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 12 Sep 2017 12:04:04 +0200
Subject: [PATCH 11/50] refactor email service to log events and added specs

---
 app/services/emails/base_service.rb          | 5 +++++
 app/services/emails/create_service.rb        | 2 ++
 app/services/emails/destroy_service.rb       | 2 ++
 spec/services/emails/create_service_spec.rb  | 4 ++++
 spec/services/emails/destroy_service_spec.rb | 4 ++++
 5 files changed, 17 insertions(+)

diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index 8810f6d8803dd0..5eba641b064496 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -5,5 +5,10 @@ def initialize(current_user, user, opts)
       @user = user
       @email = opts[:email]
     end
+
+    def log_audit_event(options = {})
+      AuditEventService.new(@current_user, @user, options)
+          .for_email(@email).security_event
+    end
   end
 end
diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb
index b6491ee98049f4..041a17126a2955 100644
--- a/app/services/emails/create_service.rb
+++ b/app/services/emails/create_service.rb
@@ -2,6 +2,8 @@ module Emails
   class CreateService < ::Emails::BaseService
     def execute
       @user.emails.create(email: @email)
+
+      log_audit_event(action: :create)
     end
   end
 end
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index af381f8ef0dffe..156b803a94c6af 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -2,6 +2,8 @@ module Emails
   class DestroyService < ::Emails::BaseService
     def execute
       Email.find_by_email!(@email).destroy && update_secondary_emails!
+
+      log_audit_event(action: :destroy)
     end
 
     private
diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb
index 1c8b81c222cd87..0c43aff77576a8 100644
--- a/spec/services/emails/create_service_spec.rb
+++ b/spec/services/emails/create_service_spec.rb
@@ -17,5 +17,9 @@
 
       expect(user.emails).to eq(Email.where(opts))
     end
+
+    it 'registers a security event' do
+      expect { service.execute }.to change { SecurityEvent.count }.by(1)
+    end
   end
 end
diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb
index 138c3ff33b0f61..3cb1c5eb9e549e 100644
--- a/spec/services/emails/destroy_service_spec.rb
+++ b/spec/services/emails/destroy_service_spec.rb
@@ -10,5 +10,9 @@
     it 'removes an email' do
       expect { service.execute }.to change { user.emails.count }.by(-1)
     end
+
+    it 'registers a security event' do
+      expect { service.execute }.to change { SecurityEvent.count }.by(1)
+    end
   end
 end
-- 
GitLab


From bbecfdfc22a4c66ca0620ac428d8742c37fd4e18 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 12 Sep 2017 12:05:22 +0200
Subject: [PATCH 12/50] refactor audit event service

---
 ee/app/services/ee/audit_event_service.rb | 67 ++++++++++++++---------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index 2b4b5bc65022c3..bdf8796e7aa694 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -41,33 +41,6 @@ def for_member(member)
       self
     end
 
-    def for_deploy_key(key_title)
-      action = @details[:action]
-      author_name = @author.name
-
-      @details =
-        case action
-        when :destroy
-          {
-            remove: "deploy_key",
-            author_name: author_name,
-            target_id: key_title,
-            target_type: "DeployKey",
-            target_details: key_title
-          }
-        when :create
-          {
-            add: "deploy_key",
-            author_name: author_name,
-            target_id: key_title,
-            target_type: "DeployKey",
-            target_details: key_title
-          }
-        end
-
-      self
-    end
-
     def for_failed_login
       ip = @details[:ip_address]
       auth = @details[:with] || 'STANDARD'
@@ -133,5 +106,45 @@ def unauth_security_event
         details: @details
       )
     end
+
+    def method_missing(method_sym, *arguments, &block)
+      super(method_sym, *arguments, &block) unless respond_to?(method_sym)
+
+      for_custom_model(method_sym.to_s.slice('for_'), *arguments)
+    end
+
+    def respond_to?(method, include_private = false)
+      method.to_s.start_with?('for_') || super
+    end
+
+    private
+
+    def for_custom_model(model, key_title)
+      action = @details[:action]
+      author_name = @author.name
+      model_class = model.camelize
+
+      @details =
+          case action
+            when :destroy
+              {
+                  remove: model,
+                  author_name: author_name,
+                  target_id: key_title,
+                  target_type: model_class,
+                  target_details: key_title
+              }
+            when :create
+              {
+                  add: model,
+                  author_name: author_name,
+                  target_id: key_title,
+                  target_type: model_class,
+                  target_details: key_title
+              }
+          end
+
+      self
+    end
   end
 end
-- 
GitLab


From f2b624e5a1c6e46363d91e8307bf41d4b2f8a65b Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 15 Sep 2017 12:10:03 +0200
Subject: [PATCH 13/50] fix details and details spec

---
 lib/audit/details.rb           |  9 ++++++++-
 spec/lib/audit/details_spec.rb | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/lib/audit/details.rb b/lib/audit/details.rb
index 8fefdbc9418d5a..52615511afbc79 100644
--- a/lib/audit/details.rb
+++ b/lib/audit/details.rb
@@ -32,7 +32,14 @@ def action_text
       when :failed_login
         "Failed to login with #{Gitlab::OAuth::Provider.label_for(value).upcase} authentication"
       else
-        "Changed #{value} from #{@details[:from]} to #{@details[:to]}"
+        text_for_change(value)
+      end
+    end
+
+    def text_for_change(value)
+      "Changed #{value}".tap do |changed_string|
+        changed_string << " from #{@details[:from]}" if @details[:from]
+        changed_string << " to #{@details[:to]}" if @details[:to]
       end
     end
   end
diff --git a/spec/lib/audit/details_spec.rb b/spec/lib/audit/details_spec.rb
index 9716ed8c7b11d4..d3599ee562e829 100644
--- a/spec/lib/audit/details_spec.rb
+++ b/spec/lib/audit/details_spec.rb
@@ -75,5 +75,23 @@
         expect(described_class.humanize(removal_action)).to eq('Removed deploy key')
       end
     end
+
+    context 'change email' do
+      let(:action) do
+        {
+            change: 'email',
+            from: 'a@b.com',
+            to: 'c@b.com',
+            author_name: 'author',
+            target_id: '',
+            target_type: 'Email',
+            target_details: 'Email'
+        }
+      end
+
+      it 'humanizes the removal action' do
+        expect(described_class.humanize(action)).to eq('Removed deploy key')
+      end
+    end
   end
 end
-- 
GitLab


From 3e267417788ca6abe77766236d0c4eee4dbcfa60 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 15 Sep 2017 12:58:03 +0200
Subject: [PATCH 14/50] fix audit email when a confirmation is required

---
 app/controllers/confirmations_controller.rb | 4 ++++
 lib/ee/audit/changes.rb                     | 7 ++++---
 spec/lib/ee/audit/changes_spec.rb           | 4 ++--
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index 306afb65f10df4..fac42aec912467 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -1,4 +1,6 @@
 class ConfirmationsController < Devise::ConfirmationsController
+  include EE::Audit::Changes
+
   def almost_there
     flash[:notice] = nil
     render layout: "devise_empty"
@@ -12,6 +14,8 @@ def after_resending_confirmation_instructions_path_for(resource)
 
   def after_confirmation_path_for(resource_name, resource)
     if signed_in?(resource_name)
+      audit_changes(:email, as: 'email address', model: resource)
+
       after_sign_in_path_for(resource)
     else
       flash[:notice] += " Please sign in."
diff --git a/lib/ee/audit/changes.rb b/lib/ee/audit/changes.rb
index 0a36ea35bbe998..888876e614b513 100644
--- a/lib/ee/audit/changes.rb
+++ b/lib/ee/audit/changes.rb
@@ -3,6 +3,7 @@ module Audit
     module Changes
       def audit_changes(column, options = {})
         column = options[:column] || column
+        @model = options[:model]
 
         return unless changed?(column)
 
@@ -12,7 +13,7 @@ def audit_changes(column, options = {})
       protected
 
       def model
-        raise NotImplementedError, "#{self} does not implement #{__method__}"
+        @model
       end
 
       private
@@ -38,8 +39,8 @@ def parse_options(column, options)
       end
 
       def audit_event(options)
-        ::AuditEventService.new(@current_user, model, options).
-          for_changes.security_event
+        ::AuditEventService.new(@current_user, model, options)
+          .for_changes.security_event
       end
     end
   end
diff --git a/spec/lib/ee/audit/changes_spec.rb b/spec/lib/ee/audit/changes_spec.rb
index 976cedf287b800..c0e1270c507f67 100644
--- a/spec/lib/ee/audit/changes_spec.rb
+++ b/spec/lib/ee/audit/changes_spec.rb
@@ -15,7 +15,7 @@
       it 'does not call the audit event service' do
         user.update!(name: 'new name')
 
-        expect{ foo_instance.audit_changes(:email) }.not_to change { SecurityEvent.count }
+        expect { foo_instance.audit_changes(:email) }.not_to change { SecurityEvent.count }
       end
     end
 
@@ -23,7 +23,7 @@
       it 'calls the audit event service' do
         user.update!(name: 'new name')
 
-        expect{ foo_instance.audit_changes(:name) }.to change { SecurityEvent.count }.by(1)
+        expect { foo_instance.audit_changes(:name) }.to change { SecurityEvent.count }.by(1)
       end
     end
   end
-- 
GitLab


From 8d5fa8331fb098456c10caf308660ac7c72aaa32 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 15 Sep 2017 14:56:18 +0200
Subject: [PATCH 15/50] fix update service

---
 app/services/users/update_service.rb      |  2 +-
 ee/app/services/ee/audit_event_service.rb | 56 +++++++++++------------
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 57c8817c2893b1..e7d639b1209a67 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -17,7 +17,7 @@ def execute(validate: true, &block)
       user_exists = @user.persisted?
 
       if @user.save(validate: validate)
-        audit_changes(:email, as: 'email address', column: :notification_email)
+        audit_changes(:email, as: 'email address')
         audit_changes(:encrypted_password, as: 'password', skip_changes: true)
 
         notify_new_user(@user, nil) unless user_exists
diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index bdf8796e7aa694..83821902811677 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -57,15 +57,15 @@ def for_failed_login
 
     def for_changes
       @details =
-          {
-              change: @details[:as] || @details[:column],
-              from: @details[:from],
-              to: @details[:to],
-              author_name: @author.name,
-              target_id: @entity.id,
-              target_type: @entity.class,
-              target_details: @entity.name
-          }
+        {
+            change: @details[:as] || @details[:column],
+            from: @details[:from],
+            to: @details[:to],
+            author_name: @author.name,
+            target_id: @entity.id,
+            target_type: @entity.class,
+            target_details: @entity.name
+        }
       self
     end
 
@@ -110,7 +110,7 @@ def unauth_security_event
     def method_missing(method_sym, *arguments, &block)
       super(method_sym, *arguments, &block) unless respond_to?(method_sym)
 
-      for_custom_model(method_sym.to_s.slice('for_'), *arguments)
+      for_custom_model(method_sym.to_s.split('for_').last, *arguments)
     end
 
     def respond_to?(method, include_private = false)
@@ -125,24 +125,24 @@ def for_custom_model(model, key_title)
       model_class = model.camelize
 
       @details =
-          case action
-            when :destroy
-              {
-                  remove: model,
-                  author_name: author_name,
-                  target_id: key_title,
-                  target_type: model_class,
-                  target_details: key_title
-              }
-            when :create
-              {
-                  add: model,
-                  author_name: author_name,
-                  target_id: key_title,
-                  target_type: model_class,
-                  target_details: key_title
-              }
-          end
+        case action
+        when :destroy
+          {
+              remove: model,
+              author_name: author_name,
+              target_id: key_title,
+              target_type: model_class,
+              target_details: key_title
+          }
+        when :create
+          {
+              add: model,
+              author_name: author_name,
+              target_id: key_title,
+              target_type: model_class,
+              target_details: key_title
+          }
+        end
 
       self
     end
-- 
GitLab


From 752579fbb5e09689c5fae9a32746b05e47bfbbd6 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 15 Sep 2017 16:46:30 +0200
Subject: [PATCH 16/50] update audit event service to log password reset events

---
 app/controllers/passwords_controller.rb   | 12 +++++
 ee/app/services/ee/audit_event_service.rb | 59 ++++++++++++++---------
 2 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb
index fda944adecdf58..ae03e7d1358333 100644
--- a/app/controllers/passwords_controller.rb
+++ b/app/controllers/passwords_controller.rb
@@ -2,6 +2,7 @@ class PasswordsController < Devise::PasswordsController
   before_action :resource_from_email, only: [:create]
   before_action :prevent_ldap_reset, only: [:create]
   before_action :throttle_reset,      only: [:create]
+  before_action :log_audit_event,      only: [:create]
 
   def edit
     super
@@ -53,4 +54,15 @@ def throttle_reset
     redirect_to new_user_session_path,
       notice: I18n.t('devise.passwords.send_paranoid_instructions')
   end
+
+  private
+
+  def log_audit_event
+    AuditEventService.new(current_user,
+                          resource,
+                          action: :custom,
+                          custom_message: 'Ask for password reset',
+                          ip_address: request.remote_ip)
+        .for_user(resource_params[:email]).unauth_security_event
+  end
 end
diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index 83821902811677..dda830ab8071a9 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -80,7 +80,7 @@ def security_event
     end
 
     def add_security_event_admin_details!
-      @details.merge!(ip_address: @author.current_sign_in_ip,
+      @details.merge!(ip_address: ip_address,
                       entity_path: @entity.full_path)
     end
 
@@ -97,11 +97,13 @@ def admin_audit_log_enabled?
     def unauth_security_event
       return unless audit_events_enabled?
 
+
       @details.delete(:ip_address) unless admin_audit_log_enabled?
+      @details[:entity_path] = @entity&.full_path
 
       SecurityEvent.create(
-        author_id: -1,
-        entity_id: -1,
+        author_id: @author&.id || -1,
+        entity_id: @entity&.id || -1,
         entity_type: 'User',
         details: @details
       )
@@ -121,30 +123,43 @@ def respond_to?(method, include_private = false)
 
     def for_custom_model(model, key_title)
       action = @details[:action]
-      author_name = @author.name
       model_class = model.camelize
+      custom_message = @details[:custom_message]
 
       @details =
-        case action
-        when :destroy
-          {
-              remove: model,
-              author_name: author_name,
-              target_id: key_title,
-              target_type: model_class,
-              target_details: key_title
-          }
-        when :create
-          {
-              add: model,
-              author_name: author_name,
-              target_id: key_title,
-              target_type: model_class,
-              target_details: key_title
-          }
-        end
+          case action
+          when :destroy
+            {
+                remove: model,
+                author_name: @author.name,
+                target_id: key_title,
+                target_type: model_class,
+                target_details: key_title
+            }
+          when :create
+            {
+                add: model,
+                author_name: @author.name,
+                target_id: key_title,
+                target_type: model_class,
+                target_details: key_title
+            }
+          when :custom
+            {
+                custom_message: custom_message,
+                author_name: @author&.name,
+                target_id: key_title,
+                target_type: model_class,
+                target_details: key_title,
+                ip_address: @details[:ip_address]
+            }
+          end
 
       self
     end
+
+    def ip_address
+      @author&.current_sign_in_ip || @details[:ip_address]
+    end
   end
 end
-- 
GitLab


From f9f32231539ed2611ac25f9e91ea9e3fdf6296d4 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 15 Sep 2017 16:47:15 +0200
Subject: [PATCH 17/50] add custom action to details

---
 lib/audit/details.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/audit/details.rb b/lib/audit/details.rb
index 52615511afbc79..0d2685d93a3b5e 100644
--- a/lib/audit/details.rb
+++ b/lib/audit/details.rb
@@ -1,6 +1,6 @@
 module Audit
   class Details
-    ACTIONS = %i[add remove failed_login change].freeze
+    ACTIONS = %i[add remove failed_login change custom_message].freeze
 
     def self.humanize(*args)
       new(*args).humanize
@@ -31,6 +31,8 @@ def action_text
         "Removed #{value}"
       when :failed_login
         "Failed to login with #{Gitlab::OAuth::Provider.label_for(value).upcase} authentication"
+        when :custom
+          value
       else
         text_for_change(value)
       end
-- 
GitLab


From 12fc5458c05bddb1d8407b64b1248fa95250a8eb Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 18 Sep 2017 10:15:49 +0200
Subject: [PATCH 18/50] fix specs, fix small bug in details

---
 ee/app/services/ee/audit_event_service.rb | 55 +++++++++++------------
 lib/audit/details.rb                      |  4 +-
 spec/requests/api/users_spec.rb           |  8 ----
 3 files changed, 29 insertions(+), 38 deletions(-)

diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index dda830ab8071a9..0a0a985836d120 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -97,7 +97,6 @@ def admin_audit_log_enabled?
     def unauth_security_event
       return unless audit_events_enabled?
 
-
       @details.delete(:ip_address) unless admin_audit_log_enabled?
       @details[:entity_path] = @entity&.full_path
 
@@ -127,33 +126,33 @@ def for_custom_model(model, key_title)
       custom_message = @details[:custom_message]
 
       @details =
-          case action
-          when :destroy
-            {
-                remove: model,
-                author_name: @author.name,
-                target_id: key_title,
-                target_type: model_class,
-                target_details: key_title
-            }
-          when :create
-            {
-                add: model,
-                author_name: @author.name,
-                target_id: key_title,
-                target_type: model_class,
-                target_details: key_title
-            }
-          when :custom
-            {
-                custom_message: custom_message,
-                author_name: @author&.name,
-                target_id: key_title,
-                target_type: model_class,
-                target_details: key_title,
-                ip_address: @details[:ip_address]
-            }
-          end
+        case action
+        when :destroy
+          {
+              remove: model,
+              author_name: @author.name,
+              target_id: key_title,
+              target_type: model_class,
+              target_details: key_title
+          }
+        when :create
+          {
+              add: model,
+              author_name: @author.name,
+              target_id: key_title,
+              target_type: model_class,
+              target_details: key_title
+          }
+        when :custom
+          {
+              custom_message: custom_message,
+              author_name: @author&.name,
+              target_id: key_title,
+              target_type: model_class,
+              target_details: key_title,
+              ip_address: @details[:ip_address]
+          }
+        end
 
       self
     end
diff --git a/lib/audit/details.rb b/lib/audit/details.rb
index 0d2685d93a3b5e..42ead21b00ea9e 100644
--- a/lib/audit/details.rb
+++ b/lib/audit/details.rb
@@ -31,8 +31,8 @@ def action_text
         "Removed #{value}"
       when :failed_login
         "Failed to login with #{Gitlab::OAuth::Provider.label_for(value).upcase} authentication"
-        when :custom
-          value
+      when :custom_message
+        value
       else
         text_for_change(value)
       end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 040458c9c7cdec..4131eae38c3fab 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -489,14 +489,6 @@
       expect(user.reload.organization).to eq('GitLab')
     end
 
-    it 'updates user with a new email' do
-      put api("/users/#{user.id}", admin), email: 'new@email.com'
-
-      expect(response).to have_http_status(200)
-      expect(user.reload.notification_email).to eq('new@email.com')
-      expect(AuditEvent.count).to eq(1)
-    end
-
     it 'updates user with avatar' do
       put api("/users/#{user.id}", admin), { avatar: fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
 
-- 
GitLab


From 16abde42da6a9166ebaac9228108b1c1e14d2f04 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 18 Sep 2017 10:39:41 +0200
Subject: [PATCH 19/50] fix more spec failures

---
 ee/app/services/ee/audit_event_service.rb | 4 ++--
 spec/lib/audit/details_spec.rb            | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index 0a0a985836d120..967015f0f878bb 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -101,8 +101,8 @@ def unauth_security_event
       @details[:entity_path] = @entity&.full_path
 
       SecurityEvent.create(
-        author_id: @author&.id || -1,
-        entity_id: @entity&.id || -1,
+        author_id: @author.respond_to?(:id)? @author.id : -1,
+        entity_id: @entity.respond_to?(:id)? @entity.id : -1,
         entity_type: 'User',
         details: @details
       )
diff --git a/spec/lib/audit/details_spec.rb b/spec/lib/audit/details_spec.rb
index d3599ee562e829..0f9a811ee014d1 100644
--- a/spec/lib/audit/details_spec.rb
+++ b/spec/lib/audit/details_spec.rb
@@ -90,7 +90,7 @@
       end
 
       it 'humanizes the removal action' do
-        expect(described_class.humanize(action)).to eq('Removed deploy key')
+        expect(described_class.humanize(action)).to eq('Changed email from a@b.com to c@b.com')
       end
     end
   end
-- 
GitLab


From c180f007aa36e88d601bd42961fda4de7eccfb30 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 18 Sep 2017 12:15:41 +0200
Subject: [PATCH 20/50] added ssh key event and spec

---
 app/models/key.rb                                 |  2 ++
 spec/controllers/profiles/keys_controller_spec.rb | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/app/models/key.rb b/app/models/key.rb
index 66f147ea467148..2fe2dd908d2508 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -28,6 +28,8 @@ class Key < ActiveRecord::Base
 
   delegate :name, :email, to: :user, prefix: true
 
+  alias_attribute :full_path, :title
+
   after_commit :add_to_shell, on: :create
   after_create :post_create_hook
   after_commit :remove_from_shell, on: :destroy
diff --git a/spec/controllers/profiles/keys_controller_spec.rb b/spec/controllers/profiles/keys_controller_spec.rb
index 363ed410bc03c9..a7d767e4ab1954 100644
--- a/spec/controllers/profiles/keys_controller_spec.rb
+++ b/spec/controllers/profiles/keys_controller_spec.rb
@@ -66,4 +66,14 @@
       end
     end
   end
+
+  describe '#create' do
+    it 'logs the audit event' do
+      sign_in(user)
+
+      key = build(:key)
+      
+      expect { post :create, key: key.attributes }.to change{ SecurityEvent.count }.by(1)
+    end
+  end
 end
-- 
GitLab


From 1c278fd9efca3390031d783255af1188a022cca5 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 18 Sep 2017 15:16:14 +0200
Subject: [PATCH 21/50] update applications controllers to log audit events.
 Added specs.

---
 app/controllers/admin/applications_controller.rb      |  2 ++
 app/controllers/oauth/applications_controller.rb      |  2 ++
 .../controllers/admin/applications_controller_spec.rb |  1 +
 .../controllers/oauth/applications_controller_spec.rb | 11 +++++++++++
 4 files changed, 16 insertions(+)

diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb
index 16590e66d6142d..5866332395269c 100644
--- a/app/controllers/admin/applications_controller.rb
+++ b/app/controllers/admin/applications_controller.rb
@@ -22,6 +22,8 @@ def create
     @application = Doorkeeper::Application.new(application_params)
 
     if @application.save
+      log_audit_event
+
       flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
       redirect_to admin_application_url(@application)
     else
diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb
index 2ae4785b12cc1b..15e6c107ebdc60 100644
--- a/app/controllers/oauth/applications_controller.rb
+++ b/app/controllers/oauth/applications_controller.rb
@@ -21,6 +21,8 @@ def create
     @application.owner = current_user
 
     if @application.save
+      log_audit_event
+
       flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
       redirect_to oauth_application_url(@application)
     else
diff --git a/spec/controllers/admin/applications_controller_spec.rb b/spec/controllers/admin/applications_controller_spec.rb
index 7bd6c0e6117ae5..d65d9c638a8aa0 100644
--- a/spec/controllers/admin/applications_controller_spec.rb
+++ b/spec/controllers/admin/applications_controller_spec.rb
@@ -38,6 +38,7 @@
 
       expect(response).to redirect_to(admin_application_path(application))
       expect(application).to have_attributes(create_params.except(:uid, :owner_type))
+      expect(SecurityEvent.count).to eq(1)
     end
 
     it 'renders the application form on errors' do
diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb
index 552899eb36ce4f..b67c941f2dedb0 100644
--- a/spec/controllers/oauth/applications_controller_spec.rb
+++ b/spec/controllers/oauth/applications_controller_spec.rb
@@ -25,5 +25,16 @@
         expect(response).to redirect_to(profile_path)
       end
     end
+
+    describe 'POST #create' do
+      it 'logs the audit event' do
+        sign_in(user)
+
+        application = build(:oauth_application)
+        application_attributes = application.attributes.merge(scopes: [])
+
+        expect { post :create, doorkeeper_application:  application_attributes }.to change{ SecurityEvent.count }.by(1)
+      end
+    end
   end
 end
-- 
GitLab


From c7cbcd00f8f9e3b6d30b88ae0b9b381a1360995a Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 18 Sep 2017 15:20:38 +0200
Subject: [PATCH 22/50] refactor/fix keys controller code

---
 app/controllers/concerns/oauth_applications.rb | 9 +++++++++
 app/models/key.rb                              | 2 --
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb
index 9849aa93fa61a2..5bb228986b9c67 100644
--- a/app/controllers/concerns/oauth_applications.rb
+++ b/app/controllers/concerns/oauth_applications.rb
@@ -16,4 +16,13 @@ def prepare_scopes
   def load_scopes
     @scopes = Doorkeeper.configuration.scopes
   end
+
+  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
diff --git a/app/models/key.rb b/app/models/key.rb
index 2fe2dd908d2508..66f147ea467148 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -28,8 +28,6 @@ class Key < ActiveRecord::Base
 
   delegate :name, :email, to: :user, prefix: true
 
-  alias_attribute :full_path, :title
-
   after_commit :add_to_shell, on: :create
   after_create :post_create_hook
   after_commit :remove_from_shell, on: :destroy
-- 
GitLab


From a1434b1cf0f0b3db728b0d02ab7e680661433860 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 18 Sep 2017 16:49:18 +0200
Subject: [PATCH 23/50] fix specs

---
 ee/app/services/ee/audit_event_service.rb              | 4 ++--
 spec/controllers/oauth/applications_controller_spec.rb | 2 +-
 spec/controllers/profiles/keys_controller_spec.rb      | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index 967015f0f878bb..688518e1352132 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -101,8 +101,8 @@ def unauth_security_event
       @details[:entity_path] = @entity&.full_path
 
       SecurityEvent.create(
-        author_id: @author.respond_to?(:id)? @author.id : -1,
-        entity_id: @entity.respond_to?(:id)? @entity.id : -1,
+        author_id: @author.respond_to?(:id) ? @author.id : -1,
+        entity_id: @entity.respond_to?(:id) ? @entity.id : -1,
         entity_type: 'User',
         details: @details
       )
diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb
index b67c941f2dedb0..dbb22a0b412af8 100644
--- a/spec/controllers/oauth/applications_controller_spec.rb
+++ b/spec/controllers/oauth/applications_controller_spec.rb
@@ -33,7 +33,7 @@
         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
diff --git a/spec/controllers/profiles/keys_controller_spec.rb b/spec/controllers/profiles/keys_controller_spec.rb
index a7d767e4ab1954..1bd0aa618af628 100644
--- a/spec/controllers/profiles/keys_controller_spec.rb
+++ b/spec/controllers/profiles/keys_controller_spec.rb
@@ -72,8 +72,8 @@
       sign_in(user)
 
       key = build(:key)
-      
-      expect { post :create, key: key.attributes }.to change{ SecurityEvent.count }.by(1)
+
+      expect { post :create, key: key.attributes }.to change { SecurityEvent.count }.by(1)
     end
   end
 end
-- 
GitLab


From 0ad7f5db3ba3626bd1804e751b11d09517402468 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 19 Sep 2017 09:52:40 +0200
Subject: [PATCH 24/50] update license checks for audit events

---
 ee/app/services/ee/audit_event_service.rb     | 37 ++++++++++---------
 .../services/ee/audit_event_service_spec.rb   |  4 ++
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index 688518e1352132..fa9a22299658d3 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -76,29 +76,14 @@ def security_event
         return super
       end
 
-      super if audit_events_enabled?
-    end
-
-    def add_security_event_admin_details!
-      @details.merge!(ip_address: ip_address,
-                      entity_path: @entity.full_path)
-    end
-
-    def audit_events_enabled?
-      return true unless @entity.respond_to?(:feature_available?)
-
-      @entity.feature_available?(:audit_events)
-    end
-
-    def admin_audit_log_enabled?
-      License.feature_available?(:admin_audit_log)
+      super if audit_events_enabled? || entity_audit_events_enabled?
     end
 
     def unauth_security_event
       return unless audit_events_enabled?
 
       @details.delete(:ip_address) unless admin_audit_log_enabled?
-      @details[:entity_path] = @entity&.full_path
+      @details[:entity_path] = @entity&.full_path if admin_audit_log_enabled?
 
       SecurityEvent.create(
         author_id: @author.respond_to?(:id) ? @author.id : -1,
@@ -160,5 +145,23 @@ def for_custom_model(model, key_title)
     def ip_address
       @author&.current_sign_in_ip || @details[:ip_address]
     end
+
+    def add_security_event_admin_details!
+      @details.merge!(ip_address: ip_address,
+                      entity_path: @entity.full_path)
+    end
+
+    def entity_audit_events_enabled?
+      @entity.respond_to?(:feature_available?) && @entity.feature_available?(:audit_events)
+    end
+
+    def audit_events_enabled?
+      # Always log auth events. Log all other events if `extended_audit_events` is enabled
+      @details[:with] || License.feature_available?(:extended_audit_events)
+    end
+
+    def admin_audit_log_enabled?
+      License.feature_available?(:admin_audit_log)
+    end
   end
 end
diff --git a/spec/ee/spec/services/ee/audit_event_service_spec.rb b/spec/ee/spec/services/ee/audit_event_service_spec.rb
index 1dedce22891e4a..6e82246dc0bc5c 100644
--- a/spec/ee/spec/services/ee/audit_event_service_spec.rb
+++ b/spec/ee/spec/services/ee/audit_event_service_spec.rb
@@ -7,6 +7,10 @@
     let(:service) { described_class.new(author_name, nil, ip_address: ip_address) }
     let(:event) { service.for_failed_login.unauth_security_event }
 
+    before do
+      stub_licensed_features(extended_audit_events: true)
+    end
+
     it 'has the right type' do
       expect(event.entity_type).to eq('User')
     end
-- 
GitLab


From 475ab94eb0e6dfd70f759abd38c85ba0870cff54 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 19 Sep 2017 10:57:53 +0200
Subject: [PATCH 25/50] add more license specs

---
 ee/app/services/ee/audit_event_service.rb | 26 +++++++++++------------
 spec/services/audit_event_service_spec.rb | 26 ++++++++++++++++++-----
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb
index fa9a22299658d3..0a25547dee1fbd 100644
--- a/ee/app/services/ee/audit_event_service.rb
+++ b/ee/app/services/ee/audit_event_service.rb
@@ -93,6 +93,19 @@ def unauth_security_event
       )
     end
 
+    def entity_audit_events_enabled?
+      @entity.respond_to?(:feature_available?) && @entity.feature_available?(:audit_events)
+    end
+
+    def audit_events_enabled?
+      # Always log auth events. Log all other events if `extended_audit_events` is enabled
+      @details[:with] || License.feature_available?(:extended_audit_events)
+    end
+
+    def admin_audit_log_enabled?
+      License.feature_available?(:admin_audit_log)
+    end
+
     def method_missing(method_sym, *arguments, &block)
       super(method_sym, *arguments, &block) unless respond_to?(method_sym)
 
@@ -150,18 +163,5 @@ def add_security_event_admin_details!
       @details.merge!(ip_address: ip_address,
                       entity_path: @entity.full_path)
     end
-
-    def entity_audit_events_enabled?
-      @entity.respond_to?(:feature_available?) && @entity.feature_available?(:audit_events)
-    end
-
-    def audit_events_enabled?
-      # Always log auth events. Log all other events if `extended_audit_events` is enabled
-      @details[:with] || License.feature_available?(:extended_audit_events)
-    end
-
-    def admin_audit_log_enabled?
-      License.feature_available?(:admin_audit_log)
-    end
   end
 end
diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb
index 4543277e05b040..35c2b904a572dd 100644
--- a/spec/services/audit_event_service_spec.rb
+++ b/spec/services/audit_event_service_spec.rb
@@ -56,20 +56,20 @@
     end
   end
 
-  describe '#audit_events_enabled?' do
+  describe '#entity_audit_events_enabled??' do
     context 'entity is a project' do
       let(:service) { described_class.new(user, project, { action: :destroy }) }
 
       it 'returns false when project is unlicensed' do
         stub_licensed_features(audit_events: false)
 
-        expect(service.audit_events_enabled?).to be_falsy
+        expect(service.entity_audit_events_enabled?).to be_falsy
       end
 
       it 'returns true when project is licensed' do
         stub_licensed_features(audit_events: true)
 
-        expect(service.audit_events_enabled?).to be_truthy
+        expect(service.entity_audit_events_enabled?).to be_truthy
       end
     end
 
@@ -80,19 +80,35 @@
       it 'returns false when group is unlicensed' do
         stub_licensed_features(audit_events: false)
 
-        expect(service.audit_events_enabled?).to be_falsy
+        expect(service.entity_audit_events_enabled?).to be_falsy
       end
 
       it 'returns true when group is licensed' do
         stub_licensed_features(audit_events: true)
 
-        expect(service.audit_events_enabled?).to be_truthy
+        expect(service.entity_audit_events_enabled?).to be_truthy
       end
     end
 
     context 'entity is a user' do
       let(:service) { described_class.new(user, user, { action: :destroy }) }
 
+      it 'returns false when unlicensed' do
+        stub_licensed_features(audit_events: false, admin_audit_log: false)
+
+        expect(service.audit_events_enabled?).to be_falsey
+      end
+
+      it 'returns true when licensed with extended events' do
+        stub_licensed_features(extended_audit_events: true)
+
+        expect(service.audit_events_enabled?).to be_truthy
+      end
+    end
+
+    context 'auth event' do
+      let(:service) { described_class.new(user, user, { with: 'auth' }) }
+
       it 'returns true when unlicensed' do
         stub_licensed_features(audit_events: false, admin_audit_log: false)
 
-- 
GitLab


From 2ee6f60a8c257250871e935940e2a5881f3da1ee Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 19 Sep 2017 11:12:34 +0200
Subject: [PATCH 26/50] fix  specs

---
 spec/controllers/admin/applications_controller_spec.rb | 2 ++
 spec/controllers/oauth/applications_controller_spec.rb | 2 ++
 spec/controllers/profiles/keys_controller_spec.rb      | 2 ++
 spec/services/emails/create_service_spec.rb            | 4 ++++
 spec/services/emails/destroy_service_spec.rb           | 4 ++++
 5 files changed, 14 insertions(+)

diff --git a/spec/controllers/admin/applications_controller_spec.rb b/spec/controllers/admin/applications_controller_spec.rb
index d65d9c638a8aa0..c7441bb7904da8 100644
--- a/spec/controllers/admin/applications_controller_spec.rb
+++ b/spec/controllers/admin/applications_controller_spec.rb
@@ -28,6 +28,8 @@
 
   describe 'POST #create' do
     it 'creates the application' do
+      stub_licensed_features(extended_audit_events: true)
+
       create_params = attributes_for(:application, trusted: true)
 
       expect do
diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb
index dbb22a0b412af8..7f3bc58a747133 100644
--- a/spec/controllers/oauth/applications_controller_spec.rb
+++ b/spec/controllers/oauth/applications_controller_spec.rb
@@ -28,6 +28,8 @@
 
     describe 'POST #create' do
       it 'logs the audit event' do
+        stub_licensed_features(extended_audit_events: true)
+        
         sign_in(user)
 
         application = build(:oauth_application)
diff --git a/spec/controllers/profiles/keys_controller_spec.rb b/spec/controllers/profiles/keys_controller_spec.rb
index 1bd0aa618af628..aa8dbb66ccb0ac 100644
--- a/spec/controllers/profiles/keys_controller_spec.rb
+++ b/spec/controllers/profiles/keys_controller_spec.rb
@@ -69,6 +69,8 @@
 
   describe '#create' do
     it 'logs the audit event' do
+      stub_licensed_features(extended_audit_events: true)
+
       sign_in(user)
 
       key = build(:key)
diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb
index 0c43aff77576a8..e3eb1fb846d2fc 100644
--- a/spec/services/emails/create_service_spec.rb
+++ b/spec/services/emails/create_service_spec.rb
@@ -7,6 +7,10 @@
   subject(:service) { described_class.new(user, user, opts) }
 
   describe '#execute' do
+    before do
+      stub_licensed_features(extended_audit_events: true)
+    end
+
     it 'creates an email with valid attributes' do
       expect { service.execute }.to change { Email.count }.by(1)
       expect(Email.where(opts)).not_to be_empty
diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb
index 3cb1c5eb9e549e..8811b86b5f8928 100644
--- a/spec/services/emails/destroy_service_spec.rb
+++ b/spec/services/emails/destroy_service_spec.rb
@@ -6,6 +6,10 @@
 
   subject(:service) { described_class.new(user, user, email: email.email) }
 
+  before do
+    stub_licensed_features(extended_audit_events: true)
+  end
+
   describe '#execute' do
     it 'removes an email' do
       expect { service.execute }.to change { user.emails.count }.by(-1)
-- 
GitLab


From 40c2dcb351eb64d2bb33f32c0c1d470d6be7c104 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 19 Sep 2017 11:54:15 +0200
Subject: [PATCH 27/50] fix specs

---
 app/services/emails/create_service.rb                  | 4 +++-
 spec/controllers/oauth/applications_controller_spec.rb | 2 +-
 spec/requests/api/users_spec.rb                        | 2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb
index 041a17126a2955..da9c280ca44ee0 100644
--- a/app/services/emails/create_service.rb
+++ b/app/services/emails/create_service.rb
@@ -1,9 +1,11 @@
 module Emails
   class CreateService < ::Emails::BaseService
     def execute
-      @user.emails.create(email: @email)
+      email = @user.emails.create(email: @email)
 
       log_audit_event(action: :create)
+
+      email
     end
   end
 end
diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb
index 7f3bc58a747133..ba35dc360eb26a 100644
--- a/spec/controllers/oauth/applications_controller_spec.rb
+++ b/spec/controllers/oauth/applications_controller_spec.rb
@@ -29,7 +29,7 @@
     describe 'POST #create' do
       it 'logs the audit event' do
         stub_licensed_features(extended_audit_events: true)
-        
+
         sign_in(user)
 
         application = build(:oauth_application)
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 4131eae38c3fab..d04d6f2f0e7725 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -474,6 +474,8 @@
     end
 
     it "updates user with new password and forces reset on next login" do
+      stub_licensed_features(extended_audit_events: true)
+
       put api("/users/#{user.id}", admin), password: '12345678'
 
       expect(response).to have_http_status(200)
-- 
GitLab


From 7367431de9844a534b6d5d0b04d346523f99656e Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Tue, 19 Sep 2017 17:02:24 +0200
Subject: [PATCH 28/50] refactor emails service

---
 app/services/emails/base_service.rb          |  8 +-------
 app/services/emails/create_service.rb        |  8 +++-----
 app/services/emails/destroy_service.rb       |  4 ++--
 ee/app/services/ee/emails/base_service.rb    | 18 ++++++++++++++++++
 ee/app/services/ee/emails/create_service.rb  | 15 +++++++++++++++
 ee/app/services/ee/emails/destroy_service.rb | 15 +++++++++++++++
 6 files changed, 54 insertions(+), 14 deletions(-)
 create mode 100644 ee/app/services/ee/emails/base_service.rb
 create mode 100644 ee/app/services/ee/emails/create_service.rb
 create mode 100644 ee/app/services/ee/emails/destroy_service.rb

diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index 5eba641b064496..ace498890978f5 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -1,14 +1,8 @@
 module Emails
   class BaseService
-    def initialize(current_user, user, opts)
-      @current_user = current_user
+    def initialize(user, opts)
       @user = user
       @email = opts[:email]
     end
-
-    def log_audit_event(options = {})
-      AuditEventService.new(@current_user, @user, options)
-          .for_email(@email).security_event
-    end
   end
 end
diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb
index da9c280ca44ee0..7f03295c5eb628 100644
--- a/app/services/emails/create_service.rb
+++ b/app/services/emails/create_service.rb
@@ -1,11 +1,9 @@
 module Emails
   class CreateService < ::Emails::BaseService
-    def execute
-      email = @user.emails.create(email: @email)
-
-      log_audit_event(action: :create)
+    prepend EE::Emails::CreateService
 
-      email
+    def execute
+      @user.emails.create(email: @email)
     end
   end
 end
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index 156b803a94c6af..1195e4752c01b1 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -1,9 +1,9 @@
 module Emails
   class DestroyService < ::Emails::BaseService
+    prepend EE::Emails::DestroyService
+
     def execute
       Email.find_by_email!(@email).destroy && update_secondary_emails!
-
-      log_audit_event(action: :destroy)
     end
 
     private
diff --git a/ee/app/services/ee/emails/base_service.rb b/ee/app/services/ee/emails/base_service.rb
new file mode 100644
index 00000000000000..66b420f9ab6135
--- /dev/null
+++ b/ee/app/services/ee/emails/base_service.rb
@@ -0,0 +1,18 @@
+module EE
+  module Emails
+    module BaseService
+      def initialize(current_user, user, opts)
+        @current_user = current_user
+
+        super(user, opts)
+      end
+
+      private
+
+      def log_audit_event(options = {})
+        AuditEventService.new(@current_user, @user, options)
+            .for_email(@email).security_event
+      end
+    end
+  end
+end
diff --git a/ee/app/services/ee/emails/create_service.rb b/ee/app/services/ee/emails/create_service.rb
new file mode 100644
index 00000000000000..88876d4e9c555b
--- /dev/null
+++ b/ee/app/services/ee/emails/create_service.rb
@@ -0,0 +1,15 @@
+module EE
+  module Emails
+    module CreateService
+      include EE::Emails::BaseService
+
+      def execute
+        email = super
+
+        log_audit_event(action: :create) if email.persisted?
+
+        email
+      end
+    end
+  end
+end
diff --git a/ee/app/services/ee/emails/destroy_service.rb b/ee/app/services/ee/emails/destroy_service.rb
new file mode 100644
index 00000000000000..b713c58996ab53
--- /dev/null
+++ b/ee/app/services/ee/emails/destroy_service.rb
@@ -0,0 +1,15 @@
+module EE
+  module Emails
+    module DestroyService
+      include EE::Emails::BaseService
+
+      def execute
+        result = super
+
+        log_audit_event(action: :destroy)
+
+        result
+      end
+    end
+  end
+end
-- 
GitLab


From e348f63b9946f2d5f82d1ab381176376ce359f9d Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 20 Sep 2017 08:23:22 +0200
Subject: [PATCH 29/50] refactor users update service into a EE module

---
 app/services/users/update_service.rb       | 16 ++----------
 ee/app/services/ee/users/update_service.rb | 30 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 14 deletions(-)
 create mode 100644 ee/app/services/ee/users/update_service.rb

diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index e7d639b1209a67..3546e08eb47f5f 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -1,10 +1,9 @@
 module Users
   class UpdateService < BaseService
-    include EE::Audit::Changes
     include NewUserNotifier
+    prepend EE::Users::UpdateService
 
-    def initialize(current_user, user, params = {})
-      @current_user = current_user
+    def initialize(user, params = {})
       @user = user
       @params = params.dup
     end
@@ -17,9 +16,6 @@ def execute(validate: true, &block)
       user_exists = @user.persisted?
 
       if @user.save(validate: validate)
-        audit_changes(:email, as: 'email address')
-        audit_changes(:encrypted_password, as: 'password', skip_changes: true)
-
         notify_new_user(@user, nil) unless user_exists
 
         success
@@ -39,15 +35,7 @@ def execute!(*args, &block)
     private
 
     def assign_attributes(&block)
-      if @user.user_synced_attributes_metadata
-        params.except!(*@user.user_synced_attributes_metadata.read_only_attributes)
-      end
-
       @user.assign_attributes(params) if params.any?
     end
-
-    def model
-      @user
-    end
   end
 end
diff --git a/ee/app/services/ee/users/update_service.rb b/ee/app/services/ee/users/update_service.rb
new file mode 100644
index 00000000000000..1ed5997a9da3cc
--- /dev/null
+++ b/ee/app/services/ee/users/update_service.rb
@@ -0,0 +1,30 @@
+module EE
+  module Users
+    module UpdateService
+      include EE::Audit::Changes
+
+      def initialize(current_user, user, params = {})
+        @current_user = current_user
+
+        super(user, params)
+      end
+
+      def execute(*args, &block)
+        result = super(*args, &block)
+
+        if result[:status] == :success
+          audit_changes(:email, as: 'email address')
+          audit_changes(:encrypted_password, as: 'password', skip_changes: true)
+        end
+
+        result
+      end
+
+      private
+
+      def model
+        @user
+      end
+    end
+  end
+end
-- 
GitLab


From ab60334675f81c00f2def46b8968f6606dfc28b8 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 20 Sep 2017 08:47:28 +0200
Subject: [PATCH 30/50] fix emails base service

---
 ee/app/services/ee/emails/base_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/app/services/ee/emails/base_service.rb b/ee/app/services/ee/emails/base_service.rb
index 66b420f9ab6135..ea449debaa89b8 100644
--- a/ee/app/services/ee/emails/base_service.rb
+++ b/ee/app/services/ee/emails/base_service.rb
@@ -10,7 +10,7 @@ def initialize(current_user, user, opts)
       private
 
       def log_audit_event(options = {})
-        AuditEventService.new(@current_user, @user, options)
+        ::AuditEventService.new(@current_user, @user, options)
             .for_email(@email).security_event
       end
     end
-- 
GitLab


From ca559099cb72bd949b45641df98200f68e72ef62 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 20 Sep 2017 10:33:18 +0200
Subject: [PATCH 31/50] refactor services

---
 app/services/emails/base_service.rb        |  3 ++-
 app/services/users/update_service.rb       | 15 +++++++++++----
 ee/app/services/ee/emails/base_service.rb  |  6 ------
 ee/app/services/ee/users/update_service.rb | 20 ++++++--------------
 4 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index ace498890978f5..8810f6d8803dd0 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -1,6 +1,7 @@
 module Emails
   class BaseService
-    def initialize(user, opts)
+    def initialize(current_user, user, opts)
+      @current_user = current_user
       @user = user
       @email = opts[:email]
     end
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 3546e08eb47f5f..70781cc1b015a2 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -3,7 +3,8 @@ class UpdateService < BaseService
     include NewUserNotifier
     prepend EE::Users::UpdateService
 
-    def initialize(user, params = {})
+    def initialize(current_user, user, params = {})
+      @current_user = current_user
       @user = user
       @params = params.dup
     end
@@ -16,9 +17,7 @@ def execute(validate: true, &block)
       user_exists = @user.persisted?
 
       if @user.save(validate: validate)
-        notify_new_user(@user, nil) unless user_exists
-
-        success
+        notify_success
       else
         error(@user.errors.full_messages.uniq.join('. '))
       end
@@ -32,6 +31,14 @@ def execute!(*args, &block)
       true
     end
 
+    protected
+
+    def notify_success
+      notify_new_user(@user, nil) unless @user.persisted?
+
+      success
+    end
+
     private
 
     def assign_attributes(&block)
diff --git a/ee/app/services/ee/emails/base_service.rb b/ee/app/services/ee/emails/base_service.rb
index ea449debaa89b8..69cdfb0825ab77 100644
--- a/ee/app/services/ee/emails/base_service.rb
+++ b/ee/app/services/ee/emails/base_service.rb
@@ -1,12 +1,6 @@
 module EE
   module Emails
     module BaseService
-      def initialize(current_user, user, opts)
-        @current_user = current_user
-
-        super(user, opts)
-      end
-
       private
 
       def log_audit_event(options = {})
diff --git a/ee/app/services/ee/users/update_service.rb b/ee/app/services/ee/users/update_service.rb
index 1ed5997a9da3cc..7162e5951ddcf5 100644
--- a/ee/app/services/ee/users/update_service.rb
+++ b/ee/app/services/ee/users/update_service.rb
@@ -3,25 +3,17 @@ module Users
     module UpdateService
       include EE::Audit::Changes
 
-      def initialize(current_user, user, params = {})
-        @current_user = current_user
-
-        super(user, params)
-      end
+      private
 
-      def execute(*args, &block)
-        result = super(*args, &block)
+      def notify_success
+        notify_new_user(@user, nil) unless @user.persisted?
 
-        if result[:status] == :success
-          audit_changes(:email, as: 'email address')
-          audit_changes(:encrypted_password, as: 'password', skip_changes: true)
-        end
+        audit_changes(:email, as: 'email address')
+        audit_changes(:encrypted_password, as: 'password', skip_changes: true)
 
-        result
+        success
       end
 
-      private
-
       def model
         @user
       end
-- 
GitLab


From 54c2a9b63ae24fe150cd7be9a14aacbbe06a646a Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 20 Sep 2017 10:35:18 +0200
Subject: [PATCH 32/50] refactor services

---
 app/services/emails/destroy_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index 1195e4752c01b1..3d19a1c2a3119e 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -3,7 +3,7 @@ class DestroyService < ::Emails::BaseService
     prepend EE::Emails::DestroyService
 
     def execute
-      Email.find_by_email!(@email).destroy && update_secondary_emails!
+      update_secondary_emails! if Email.find_by_email!(@email).destroy
     end
 
     private
-- 
GitLab


From 9efeb572e7ea205c4e295edf0dc55ee4c6783e98 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 20 Sep 2017 11:14:19 +0200
Subject: [PATCH 33/50] refactor services

---
 app/services/users/update_service.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 70781cc1b015a2..48ca47c7b543e3 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -14,8 +14,6 @@ def execute(validate: true, &block)
 
       assign_attributes(&block)
 
-      user_exists = @user.persisted?
-
       if @user.save(validate: validate)
         notify_success
       else
-- 
GitLab


From 8df3f70c033f4e9daccab47f62a8d59a97d0381d Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 20 Sep 2017 13:28:25 +0200
Subject: [PATCH 34/50] fix specs

---
 app/services/emails/create_service.rb        |  2 +-
 app/services/emails/destroy_service.rb       |  2 +-
 app/services/users/update_service.rb         | 12 +++++++++---
 ee/app/services/ee/emails/create_service.rb  |  2 +-
 ee/app/services/ee/emails/destroy_service.rb |  2 +-
 ee/app/services/ee/users/update_service.rb   |  4 ++--
 spec/features/login_spec.rb                  |  4 ++++
 spec/lib/ee/audit/changes_spec.rb            |  3 +++
 8 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb
index 7f03295c5eb628..e1ceb68bfe8aa5 100644
--- a/app/services/emails/create_service.rb
+++ b/app/services/emails/create_service.rb
@@ -1,6 +1,6 @@
 module Emails
   class CreateService < ::Emails::BaseService
-    prepend EE::Emails::CreateService
+    prepend ::EE::Emails::CreateService
 
     def execute
       @user.emails.create(email: @email)
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index 3d19a1c2a3119e..6ed9ddde5d68c2 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -1,6 +1,6 @@
 module Emails
   class DestroyService < ::Emails::BaseService
-    prepend EE::Emails::DestroyService
+    prepend ::EE::Emails::DestroyService
 
     def execute
       update_secondary_emails! if Email.find_by_email!(@email).destroy
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 48ca47c7b543e3..0965b1a8b7ce43 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -14,8 +14,10 @@ def execute(validate: true, &block)
 
       assign_attributes(&block)
 
+      user_exists = @user.persisted?
+
       if @user.save(validate: validate)
-        notify_success
+        notify_success(user_exists)
       else
         error(@user.errors.full_messages.uniq.join('. '))
       end
@@ -31,8 +33,8 @@ def execute!(*args, &block)
 
     protected
 
-    def notify_success
-      notify_new_user(@user, nil) unless @user.persisted?
+    def notify_success(user_exists)
+      notify_new_user(@user, nil) unless user_exists
 
       success
     end
@@ -40,6 +42,10 @@ def notify_success
     private
 
     def assign_attributes(&block)
+      if @user.user_synced_attributes_metadata
+        params.except!(*@user.user_synced_attributes_metadata.read_only_attributes)
+      end
+
       @user.assign_attributes(params) if params.any?
     end
   end
diff --git a/ee/app/services/ee/emails/create_service.rb b/ee/app/services/ee/emails/create_service.rb
index 88876d4e9c555b..be4d9d909b0a6b 100644
--- a/ee/app/services/ee/emails/create_service.rb
+++ b/ee/app/services/ee/emails/create_service.rb
@@ -1,7 +1,7 @@
 module EE
   module Emails
     module CreateService
-      include EE::Emails::BaseService
+      include ::EE::Emails::BaseService
 
       def execute
         email = super
diff --git a/ee/app/services/ee/emails/destroy_service.rb b/ee/app/services/ee/emails/destroy_service.rb
index b713c58996ab53..6c317284955868 100644
--- a/ee/app/services/ee/emails/destroy_service.rb
+++ b/ee/app/services/ee/emails/destroy_service.rb
@@ -1,7 +1,7 @@
 module EE
   module Emails
     module DestroyService
-      include EE::Emails::BaseService
+      include ::EE::Emails::BaseService
 
       def execute
         result = super
diff --git a/ee/app/services/ee/users/update_service.rb b/ee/app/services/ee/users/update_service.rb
index 7162e5951ddcf5..6cf2e401dfb92b 100644
--- a/ee/app/services/ee/users/update_service.rb
+++ b/ee/app/services/ee/users/update_service.rb
@@ -5,8 +5,8 @@ module UpdateService
 
       private
 
-      def notify_success
-        notify_new_user(@user, nil) unless @user.persisted?
+      def notify_success(user_exists)
+        notify_new_user(@user, nil) unless user_exists
 
         audit_changes(:email, as: 'email address')
         audit_changes(:encrypted_password, as: 'password', skip_changes: true)
diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb
index 0c897d32e3104c..25e81937e9425c 100644
--- a/spec/features/login_spec.rb
+++ b/spec/features/login_spec.rb
@@ -154,6 +154,8 @@ def enter_code(code)
       end
 
       it 'creates a security event after failed OAuth login' do
+        stub_licensed_features(extended_audit_events: true)
+
         stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [mock_saml_config])
         user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml')
         gitlab_sign_in_via('saml', user, 'wrong-uid')
@@ -177,6 +179,8 @@ def enter_code(code)
     end
 
     it 'blocks invalid login' do
+      stub_licensed_features(extended_audit_events: true)
+
       user = create(:user, password: 'not-the-default')
 
       gitlab_sign_in(user)
diff --git a/spec/lib/ee/audit/changes_spec.rb b/spec/lib/ee/audit/changes_spec.rb
index c0e1270c507f67..a18afc02f5b420 100644
--- a/spec/lib/ee/audit/changes_spec.rb
+++ b/spec/lib/ee/audit/changes_spec.rb
@@ -6,8 +6,11 @@
     let(:foo_instance) { Class.new { include EE::Audit::Changes }.new }
 
     before do
+      stub_licensed_features(extended_audit_events: true)
+
       foo_instance.instance_variable_set(:@current_user, user)
       foo_instance.instance_variable_set(:@user, user)
+
       allow(foo_instance).to receive(:model).and_return(user)
     end
 
-- 
GitLab


From 7a0423ea662e99e4942a97fc153bbe4160fa89fc Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 21 Sep 2017 10:28:43 +0200
Subject: [PATCH 35/50] refactor a bunch of controllers to make them EE modules

---
 app/controllers/admin/applications_controller.rb  | 13 +++++++++----
 app/controllers/confirmations_controller.rb       | 10 ++++++----
 app/controllers/oauth/applications_controller.rb  | 13 +++++++++----
 .../ee/admin/applications_controller.rb           | 15 +++++++++++++++
 ee/app/controllers/ee/confirmations_controller.rb | 15 +++++++++++++++
 .../ee/oauth/applications_controller.rb           | 15 +++++++++++++++
 6 files changed, 69 insertions(+), 12 deletions(-)
 create mode 100644 ee/app/controllers/ee/admin/applications_controller.rb
 create mode 100644 ee/app/controllers/ee/confirmations_controller.rb
 create mode 100644 ee/app/controllers/ee/oauth/applications_controller.rb

diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb
index 5866332395269c..07760fb5c8f627 100644
--- a/app/controllers/admin/applications_controller.rb
+++ b/app/controllers/admin/applications_controller.rb
@@ -1,5 +1,6 @@
 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]
@@ -22,10 +23,7 @@ def create
     @application = Doorkeeper::Application.new(application_params)
 
     if @application.save
-      log_audit_event
-
-      flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
-      redirect_to admin_application_url(@application)
+      redirect_to_admin_page
     else
       render :new
     end
@@ -44,6 +42,13 @@ def destroy
     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
diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index fac42aec912467..c061d5da0425be 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -1,5 +1,5 @@
 class ConfirmationsController < Devise::ConfirmationsController
-  include EE::Audit::Changes
+  prepend ::EE::ConfirmationsController
 
   def almost_there
     flash[:notice] = nil
@@ -14,12 +14,14 @@ def after_resending_confirmation_instructions_path_for(resource)
 
   def after_confirmation_path_for(resource_name, resource)
     if signed_in?(resource_name)
-      audit_changes(:email, as: 'email address', model: resource)
-
-      after_sign_in_path_for(resource)
+      after_sign_in(resource)
     else
       flash[:notice] += " Please sign in."
       new_session_path(resource_name)
     end
   end
+
+  def after_sign_in(resource)
+    after_sign_in_path_for(resource)
+  end
 end
diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb
index 15e6c107ebdc60..e20804fcaa0ed2 100644
--- a/app/controllers/oauth/applications_controller.rb
+++ b/app/controllers/oauth/applications_controller.rb
@@ -3,6 +3,7 @@ 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!
@@ -21,16 +22,20 @@ def create
     @application.owner = current_user
 
     if @application.save
-      log_audit_event
-
-      flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
-      redirect_to oauth_application_url(@application)
+      redirect_to_oauth_application_page
     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
diff --git a/ee/app/controllers/ee/admin/applications_controller.rb b/ee/app/controllers/ee/admin/applications_controller.rb
new file mode 100644
index 00000000000000..6420a156ddd49f
--- /dev/null
+++ b/ee/app/controllers/ee/admin/applications_controller.rb
@@ -0,0 +1,15 @@
+module EE
+  module Admin
+    module ApplicationsController
+      protected
+
+      def redirect_to_admin_page
+        raise NotImplementedError unless defined?(super)
+
+        log_audit_event
+
+        super
+      end
+    end
+  end
+end
diff --git a/ee/app/controllers/ee/confirmations_controller.rb b/ee/app/controllers/ee/confirmations_controller.rb
new file mode 100644
index 00000000000000..49562ea4a83329
--- /dev/null
+++ b/ee/app/controllers/ee/confirmations_controller.rb
@@ -0,0 +1,15 @@
+module EE
+  module ConfirmationsController
+    include EE::Audit::Changes
+
+    protected
+
+    def after_sign_in(resource)
+      raise NotImplementedError unless defined?(super)
+
+      audit_changes(:email, as: 'email address', model: resource)
+
+      super(resource)
+    end
+  end
+end
diff --git a/ee/app/controllers/ee/oauth/applications_controller.rb b/ee/app/controllers/ee/oauth/applications_controller.rb
new file mode 100644
index 00000000000000..385e41cb6c9a38
--- /dev/null
+++ b/ee/app/controllers/ee/oauth/applications_controller.rb
@@ -0,0 +1,15 @@
+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
-- 
GitLab


From 0f6c6b9664a0fafec6dc452fdf797a7389d974ec Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 21 Sep 2017 10:38:03 +0200
Subject: [PATCH 36/50] fix login spec

---
 spec/features/login_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb
index 25e81937e9425c..8769b2c5a638a7 100644
--- a/spec/features/login_spec.rb
+++ b/spec/features/login_spec.rb
@@ -154,9 +154,9 @@ def enter_code(code)
       end
 
       it 'creates a security event after failed OAuth login' do
+        stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [mock_saml_config])
         stub_licensed_features(extended_audit_events: true)
 
-        stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [mock_saml_config])
         user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml')
         gitlab_sign_in_via('saml', user, 'wrong-uid')
 
-- 
GitLab


From 445d4cc0ebf03b184846bba3a7b75f04313ed771 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 21 Sep 2017 10:50:46 +0200
Subject: [PATCH 37/50] refactor password controller

---
 app/controllers/passwords_controller.rb       | 14 ++------------
 ee/app/controllers/ee/passwords_controller.rb | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 12 deletions(-)
 create mode 100644 ee/app/controllers/ee/passwords_controller.rb

diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb
index ae03e7d1358333..8185014e113927 100644
--- a/app/controllers/passwords_controller.rb
+++ b/app/controllers/passwords_controller.rb
@@ -2,7 +2,8 @@ class PasswordsController < Devise::PasswordsController
   before_action :resource_from_email, only: [:create]
   before_action :prevent_ldap_reset, only: [:create]
   before_action :throttle_reset,      only: [:create]
-  before_action :log_audit_event,      only: [:create]
+
+  prepend EE::PasswordsController
 
   def edit
     super
@@ -54,15 +55,4 @@ def throttle_reset
     redirect_to new_user_session_path,
       notice: I18n.t('devise.passwords.send_paranoid_instructions')
   end
-
-  private
-
-  def log_audit_event
-    AuditEventService.new(current_user,
-                          resource,
-                          action: :custom,
-                          custom_message: 'Ask for password reset',
-                          ip_address: request.remote_ip)
-        .for_user(resource_params[:email]).unauth_security_event
-  end
 end
diff --git a/ee/app/controllers/ee/passwords_controller.rb b/ee/app/controllers/ee/passwords_controller.rb
new file mode 100644
index 00000000000000..4e2a2ef0e43063
--- /dev/null
+++ b/ee/app/controllers/ee/passwords_controller.rb
@@ -0,0 +1,18 @@
+module EE
+  module PasswordsController
+    prepended do
+      before_action :log_audit_event, only: [:create]
+    end
+
+    private
+
+    def log_audit_event
+      AuditEventService.new(current_user,
+                            resource,
+                            action: :custom,
+                            custom_message: 'Ask for password reset',
+                            ip_address: request.remote_ip)
+          .for_user(resource_params[:email]).unauth_security_event
+    end
+  end
+end
-- 
GitLab


From 008bf9483b407d7591f712a14e93f6871fa75430 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 21 Sep 2017 10:55:13 +0200
Subject: [PATCH 38/50] refactor keys controller

---
 .../ee/profiles/keys_controller.rb            | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 ee/app/controllers/ee/profiles/keys_controller.rb

diff --git a/ee/app/controllers/ee/profiles/keys_controller.rb b/ee/app/controllers/ee/profiles/keys_controller.rb
new file mode 100644
index 00000000000000..7ee6e6442da263
--- /dev/null
+++ b/ee/app/controllers/ee/profiles/keys_controller.rb
@@ -0,0 +1,26 @@
+module EE
+  module Admin
+    module ApplicationsController
+      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
-- 
GitLab


From d9cc84ec8c5394937f6876cecf126824b53f89c0 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 21 Sep 2017 11:35:55 +0200
Subject: [PATCH 39/50] fix keys controller

---
 app/controllers/profiles/keys_controller.rb       | 10 +++++++++-
 ee/app/controllers/ee/profiles/keys_controller.rb |  4 ++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb
index 4393c652617702..76a128a1215f43 100644
--- a/app/controllers/profiles/keys_controller.rb
+++ b/app/controllers/profiles/keys_controller.rb
@@ -1,4 +1,6 @@
 class Profiles::KeysController < Profiles::ApplicationController
+  prepend ::EE::Profiles::KeysController
+
   skip_before_action :authenticate_user!, only: [:get_keys]
 
   def index
@@ -14,7 +16,7 @@ def create
     @key = Keys::CreateService.new(current_user, key_params).execute
 
     if @key.persisted?
-      redirect_to profile_key_path(@key)
+      redirect_to_profile_key_path
     else
       @keys = current_user.keys.select(&:persisted?)
       render :index
@@ -50,6 +52,12 @@ def get_keys
     end
   end
 
+  protected
+
+  def redirect_to_profile_key_path
+    redirect_to profile_key_path(@key)
+  end
+
   private
 
   def key_params
diff --git a/ee/app/controllers/ee/profiles/keys_controller.rb b/ee/app/controllers/ee/profiles/keys_controller.rb
index 7ee6e6442da263..bc9007f171253e 100644
--- a/ee/app/controllers/ee/profiles/keys_controller.rb
+++ b/ee/app/controllers/ee/profiles/keys_controller.rb
@@ -1,6 +1,6 @@
 module EE
-  module Admin
-    module ApplicationsController
+  module Profiles
+    module KeysController
       protected
 
       def redirect_to_profile_key_path
-- 
GitLab


From 10135396a9909d6ef05019f96e12f26cb2229ee9 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 21 Sep 2017 11:38:44 +0200
Subject: [PATCH 40/50] fix license

---
 app/models/license.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/models/license.rb b/app/models/license.rb
index 1b9fec0a33fabb..7cf959b93559a7 100644
--- a/app/models/license.rb
+++ b/app/models/license.rb
@@ -38,6 +38,7 @@ class License < ActiveRecord::Base
     cross_project_pipelines
     db_load_balancing
     deploy_board
+    extended_audit_events
     file_locks
     geo
     group_issue_boards
-- 
GitLab


From ae0981ee05b154c65fb2ec001ef48d6480b61c08 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 21 Sep 2017 11:39:48 +0200
Subject: [PATCH 41/50] fix license

---
 app/models/license.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/models/license.rb b/app/models/license.rb
index 7cf959b93559a7..e7ad62ba6a00af 100644
--- a/app/models/license.rb
+++ b/app/models/license.rb
@@ -108,6 +108,7 @@ class License < ActiveRecord::Base
     auditor_user
     db_load_balancing
     elastic_search
+    extended_audit_events
     geo
     ldap_extras
     object_storage
-- 
GitLab


From fcb8fb1d1103d05fe358f1235af12b744165ac7f Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 21 Sep 2017 15:10:05 +0200
Subject: [PATCH 42/50] fix spec failures

---
 ee/app/controllers/ee/passwords_controller.rb     | 4 +++-
 ee/app/controllers/ee/profiles/keys_controller.rb | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/ee/app/controllers/ee/passwords_controller.rb b/ee/app/controllers/ee/passwords_controller.rb
index 4e2a2ef0e43063..2c9921c03b2848 100644
--- a/ee/app/controllers/ee/passwords_controller.rb
+++ b/ee/app/controllers/ee/passwords_controller.rb
@@ -1,5 +1,7 @@
 module EE
   module PasswordsController
+    extend ActiveSupport::Concern
+
     prepended do
       before_action :log_audit_event, only: [:create]
     end
@@ -7,7 +9,7 @@ module PasswordsController
     private
 
     def log_audit_event
-      AuditEventService.new(current_user,
+      ::AuditEventService.new(current_user,
                             resource,
                             action: :custom,
                             custom_message: 'Ask for password reset',
diff --git a/ee/app/controllers/ee/profiles/keys_controller.rb b/ee/app/controllers/ee/profiles/keys_controller.rb
index bc9007f171253e..ebcd96edb404ad 100644
--- a/ee/app/controllers/ee/profiles/keys_controller.rb
+++ b/ee/app/controllers/ee/profiles/keys_controller.rb
@@ -14,7 +14,7 @@ def redirect_to_profile_key_path
       private
 
       def log_audit_event
-        AuditEventService.new(current_user,
+        ::AuditEventService.new(current_user,
                               current_user,
                               action: :custom,
                               custom_message: 'Added SSH key',
-- 
GitLab


From f5a69b09a9c858a65680979bd9a424a185bf7cee Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 22 Sep 2017 10:05:31 +0200
Subject: [PATCH 43/50] refactor oauth applications

---
 app/controllers/concerns/oauth_applications.rb     | 10 +---------
 .../controllers/ee/concerns/oauth_applications.rb  | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 9 deletions(-)
 create mode 100644 ee/app/controllers/ee/concerns/oauth_applications.rb

diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb
index 5bb228986b9c67..6c75c5eb8c25ef 100644
--- a/app/controllers/concerns/oauth_applications.rb
+++ b/app/controllers/concerns/oauth_applications.rb
@@ -1,5 +1,6 @@
 module OauthApplications
   extend ActiveSupport::Concern
+  prepend ::EE::OauthApplications
 
   included do
     before_action :prepare_scopes, only: [:create, :update]
@@ -16,13 +17,4 @@ def prepare_scopes
   def load_scopes
     @scopes = Doorkeeper.configuration.scopes
   end
-
-  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
diff --git a/ee/app/controllers/ee/concerns/oauth_applications.rb b/ee/app/controllers/ee/concerns/oauth_applications.rb
new file mode 100644
index 00000000000000..4228b75be9f37b
--- /dev/null
+++ b/ee/app/controllers/ee/concerns/oauth_applications.rb
@@ -0,0 +1,14 @@
+module EE
+  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
-- 
GitLab


From d5886465e4fd87f4732ca9dfb86e21a7abdd9198 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 22 Sep 2017 11:42:40 +0200
Subject: [PATCH 44/50] fix specs

---
 .../concerns/oauth_applications.rb            |  2 +-
 .../ee/concerns/oauth_applications.rb         | 20 ++++++++++---------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb
index 6c75c5eb8c25ef..f7054da00190b6 100644
--- a/app/controllers/concerns/oauth_applications.rb
+++ b/app/controllers/concerns/oauth_applications.rb
@@ -1,6 +1,6 @@
 module OauthApplications
   extend ActiveSupport::Concern
-  prepend ::EE::OauthApplications
+  prepend ::EE::Concerns::OauthApplications
 
   included do
     before_action :prepare_scopes, only: [:create, :update]
diff --git a/ee/app/controllers/ee/concerns/oauth_applications.rb b/ee/app/controllers/ee/concerns/oauth_applications.rb
index 4228b75be9f37b..bcdd03d5f4cfca 100644
--- a/ee/app/controllers/ee/concerns/oauth_applications.rb
+++ b/ee/app/controllers/ee/concerns/oauth_applications.rb
@@ -1,14 +1,16 @@
 module EE
-  module OauthApplications
-    extend ActiveSupport::Concern
+  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
+      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
-- 
GitLab


From 667598077b1459ee4ae6d11efd6210ad30890d3f Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 25 Sep 2017 09:50:07 +0200
Subject: [PATCH 45/50] refactor code

---
 ee/app/services/ee/emails/create_service.rb  | 8 +++-----
 ee/app/services/ee/emails/destroy_service.rb | 8 +++-----
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/ee/app/services/ee/emails/create_service.rb b/ee/app/services/ee/emails/create_service.rb
index be4d9d909b0a6b..c5ce8be46a39eb 100644
--- a/ee/app/services/ee/emails/create_service.rb
+++ b/ee/app/services/ee/emails/create_service.rb
@@ -4,11 +4,9 @@ module CreateService
       include ::EE::Emails::BaseService
 
       def execute
-        email = super
-
-        log_audit_event(action: :create) if email.persisted?
-
-        email
+        super.tap do |email|
+          log_audit_event(action: :create) if email.persisted?
+        end
       end
     end
   end
diff --git a/ee/app/services/ee/emails/destroy_service.rb b/ee/app/services/ee/emails/destroy_service.rb
index 6c317284955868..eb55b7f73d28e5 100644
--- a/ee/app/services/ee/emails/destroy_service.rb
+++ b/ee/app/services/ee/emails/destroy_service.rb
@@ -4,11 +4,9 @@ module DestroyService
       include ::EE::Emails::BaseService
 
       def execute
-        result = super
-
-        log_audit_event(action: :destroy)
-
-        result
+        super.tap do
+          log_audit_event(action: :destroy)
+        end
       end
     end
   end
-- 
GitLab


From 6d272675bbe30786ca0ff8950b2009c561e3bb62 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 27 Sep 2017 11:25:43 +0200
Subject: [PATCH 46/50] refactor users update service

---
 app/controllers/admin/users_controller.rb              |  4 ++--
 app/controllers/profiles/avatars_controller.rb         |  2 +-
 app/controllers/profiles/notifications_controller.rb   |  2 +-
 app/controllers/profiles/passwords_controller.rb       |  6 +++---
 app/controllers/profiles/preferences_controller.rb     |  2 +-
 .../profiles/two_factor_auths_controller.rb            |  6 +++---
 app/controllers/profiles_controller.rb                 | 10 +++++-----
 app/controllers/sessions_controller.rb                 |  2 +-
 app/models/user.rb                                     |  6 +++---
 app/services/emails/destroy_service.rb                 |  2 +-
 app/services/users/update_service.rb                   |  4 ++--
 lib/api/internal.rb                                    |  2 +-
 lib/api/notification_settings.rb                       |  2 +-
 lib/api/users.rb                                       |  2 +-
 lib/gitlab/ldap/access.rb                              |  4 ++--
 lib/gitlab/o_auth/user.rb                              |  2 +-
 spec/services/users/update_service_spec.rb             |  8 ++++----
 17 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 9d2f7a6e9aef87..c0433c00f5db1c 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -128,7 +128,7 @@ def update
     end
 
     respond_to do |format|
-      result = Users::UpdateService.new(current_user, user, user_params_with_pass).execute do |user|
+      result = Users::UpdateService.new(current_user, user_params_with_pass.merge(user: user)).execute do |user|
         user.skip_reconfirmation!
       end
 
@@ -226,7 +226,7 @@ def user_params_ee
   end
 
   def update_user(&block)
-    result = Users::UpdateService.new(current_user, user).execute(&block)
+    result = Users::UpdateService.new(current_user, user: user).execute(&block)
 
     result[:status] == :success
   end
diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb
index 5a94dc88455b00..39b9f8a84d1fb5 100644
--- a/app/controllers/profiles/avatars_controller.rb
+++ b/app/controllers/profiles/avatars_controller.rb
@@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController
   def destroy
     @user = current_user
 
-    Users::UpdateService.new(current_user, @user).execute { |user| user.remove_avatar! }
+    Users::UpdateService.new(current_user, user: @user).execute { |user| user.remove_avatar! }
 
     redirect_to profile_path, status: 302
   end
diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb
index 45d7bca27bbe31..8a38ba65d4ccc0 100644
--- a/app/controllers/profiles/notifications_controller.rb
+++ b/app/controllers/profiles/notifications_controller.rb
@@ -7,7 +7,7 @@ def show
   end
 
   def update
-    result = Users::UpdateService.new(current_user, current_user, user_params).execute
+    result = Users::UpdateService.new(current_user, user_params.merge(user: current_user)).execute
 
     if result[:status] == :success
       flash[:notice] = "Notification settings saved"
diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb
index 08b438de9e25d6..dcfcb855ab55a8 100644
--- a/app/controllers/profiles/passwords_controller.rb
+++ b/app/controllers/profiles/passwords_controller.rb
@@ -21,10 +21,10 @@ def create
       password_automatically_set: false
     }
 
-    result = Users::UpdateService.new(current_user, @user, password_attributes).execute
+    result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute
 
     if result[:status] == :success
-      Users::UpdateService.new(current_user, @user, password_expires_at: nil).execute
+      Users::UpdateService.new(current_user, user: @user, password_expires_at: nil).execute
 
       redirect_to root_path, notice: 'Password successfully changed'
     else
@@ -46,7 +46,7 @@ def update
       return
     end
 
-    result = Users::UpdateService.new(current_user, @user, password_attributes).execute
+    result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute
 
     if result[:status] == :success
       flash[:notice] = "Password was successfully updated. Please login with it"
diff --git a/app/controllers/profiles/preferences_controller.rb b/app/controllers/profiles/preferences_controller.rb
index a13b9a616cbb85..ed0f98179ebbb4 100644
--- a/app/controllers/profiles/preferences_controller.rb
+++ b/app/controllers/profiles/preferences_controller.rb
@@ -6,7 +6,7 @@ def show
 
   def update
     begin
-      result = Users::UpdateService.new(current_user, user, preferences_params).execute
+      result = Users::UpdateService.new(current_user, preferences_params.merge(user: user)).execute
 
       if result[:status] == :success
         flash[:notice] = 'Preferences saved.'
diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb
index b590846257b2fc..aa9789f8a0f3ca 100644
--- a/app/controllers/profiles/two_factor_auths_controller.rb
+++ b/app/controllers/profiles/two_factor_auths_controller.rb
@@ -10,7 +10,7 @@ def show
       current_user.otp_grace_period_started_at = Time.current
     end
 
-    Users::UpdateService.new(current_user, current_user).execute!
+    Users::UpdateService.new(current_user, user: current_user).execute!
 
     if two_factor_authentication_required? && !current_user.two_factor_enabled?
       two_factor_authentication_reason(
@@ -41,7 +41,7 @@ def show
 
   def create
     if current_user.validate_and_consume_otp!(params[:pin_code])
-      Users::UpdateService.new(current_user, current_user, otp_required_for_login: true).execute! do |user|
+      Users::UpdateService.new(current_user, user: current_user, otp_required_for_login: true).execute! do |user|
         @codes = user.generate_otp_backup_codes!
       end
 
@@ -70,7 +70,7 @@ def create_u2f
   end
 
   def codes
-    Users::UpdateService.new(current_user, current_user).execute! do |user|
+    Users::UpdateService.new(current_user, user: current_user).execute! do |user|
       @codes = user.generate_otp_backup_codes!
     end
   end
diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index 9e85997cf6deae..5d87037f01232d 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -10,7 +10,7 @@ def show
 
   def update
     respond_to do |format|
-      result = Users::UpdateService.new(current_user, @user, user_params).execute
+      result = Users::UpdateService.new(current_user, user_params.merge(user: @user)).execute
 
       if result[:status] == :success
         message = "Profile was successfully updated"
@@ -25,7 +25,7 @@ def update
   end
 
   def reset_private_token
-    Users::UpdateService.new(current_user, @user).execute! do |user|
+    Users::UpdateService.new(current_user, user: @user).execute! do |user|
       user.reset_authentication_token!
     end
 
@@ -35,7 +35,7 @@ def reset_private_token
   end
 
   def reset_incoming_email_token
-    Users::UpdateService.new(current_user, @user).execute! do |user|
+    Users::UpdateService.new(current_user, user: @user).execute! do |user|
       user.reset_incoming_email_token!
     end
 
@@ -45,7 +45,7 @@ def reset_incoming_email_token
   end
 
   def reset_rss_token
-    Users::UpdateService.new(current_user, @user).execute! do |user|
+    Users::UpdateService.new(current_user, user: @user).execute! do |user|
       user.reset_rss_token!
     end
 
@@ -61,7 +61,7 @@ def audit_log
   end
 
   def update_username
-    result = Users::UpdateService.new(current_user, @user, username: user_params[:username]).execute
+    result = Users::UpdateService.new(current_user, user: @user, username: user_params[:username]).execute
 
     options = if result[:status] == :success
                 { notice: "Username successfully changed" }
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index c3424398c6b398..8267644b565220 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -57,7 +57,7 @@ def check_initial_setup
 
     return unless user && user.require_password_creation?
 
-    Users::UpdateService.new(current_user, user).execute do |user|
+    Users::UpdateService.new(current_user, user: user).execute do |user|
       @token = user.generate_reset_token
     end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 00931c9fa39853..5b8b27b7991ab8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -63,7 +63,7 @@ def update_tracked_fields!(request)
     lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i)
     return unless lease.try_obtain
 
-    Users::UpdateService.new(self, self).execute(validate: false)
+    Users::UpdateService.new(self, user: self).execute(validate: false)
   end
 
   attr_accessor :force_random_password
@@ -1023,7 +1023,7 @@ def increment_failed_attempts!
     if attempts_exceeded?
       lock_access! unless access_locked?
     else
-      Users::UpdateService.new(self, self).execute(validate: false)
+      Users::UpdateService.new(self, user: self).execute(validate: false)
     end
   end
 
@@ -1209,7 +1209,7 @@ def self.create_unique_internal(scope, username, email_pattern, &creation_block)
       &creation_block
     )
 
-    Users::UpdateService.new(user, user).execute(validate: false)
+    Users::UpdateService.new(user, user: user).execute(validate: false)
     user
   ensure
     Gitlab::ExclusiveLease.cancel(lease_key, uuid)
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index 6ed9ddde5d68c2..88055c25ae4303 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -9,7 +9,7 @@ def execute
     private
 
     def update_secondary_emails!
-      result = ::Users::UpdateService.new(@current_user, @user).execute do |user|
+      result = ::Users::UpdateService.new(@current_user, user: @user).execute do |user|
         user.update_secondary_emails!
       end
 
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 0965b1a8b7ce43..e04f30864bdbc2 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -3,9 +3,9 @@ class UpdateService < BaseService
     include NewUserNotifier
     prepend EE::Users::UpdateService
 
-    def initialize(current_user, user, params = {})
+    def initialize(current_user, params = {})
       @current_user = current_user
-      @user = user
+      @user = params[:user]
       @params = params.dup
     end
 
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 11b9bb4c5ecc17..8276ca48a663e0 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -149,7 +149,7 @@ class Internal < Grape::API
 
         codes = nil
 
-        ::Users::UpdateService.new(current_user, user).execute! do |user|
+        ::Users::UpdateService.new(current_user, user: user).execute! do |user|
           codes = user.generate_otp_backup_codes!
         end
 
diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb
index 1c5592d952d762..0266bf2f71715e 100644
--- a/lib/api/notification_settings.rb
+++ b/lib/api/notification_settings.rb
@@ -35,7 +35,7 @@ class NotificationSettings < Grape::API
             new_notification_email = params.delete(:notification_email)
 
             if new_notification_email
-              ::Users::UpdateService.new(current_user, current_user, notification_email: new_notification_email).execute
+              ::Users::UpdateService.new(current_user, user: current_user, notification_email: new_notification_email).execute
             end
 
             notification_setting.update(declared_params(include_missing: false))
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 8a07be5cb9414b..d196fff13f5343 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -172,7 +172,7 @@ def find_user(params)
 
         user_params[:password_expires_at] = Time.now if user_params[:password].present?
 
-        result = ::Users::UpdateService.new(current_user, user, user_params.except(:extern_uid, :provider)).execute
+        result = ::Users::UpdateService.new(current_user, user_params.except(:extern_uid, :provider).merge(user: user)).execute
 
         if result[:status] == :success
           present user, with: Entities::UserPublic
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index 8d9c63624a5bfa..95d39fce52f08d 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -20,7 +20,7 @@ def self.allowed?(user, options = {})
           # permissions to keep things clean
           if access.allowed?
             access.update_user
-            Users::UpdateService.new(user, user, last_credential_check_at: Time.now).execute
+            Users::UpdateService.new(user, user: user, last_credential_check_at: Time.now).execute
 
             true
           else
@@ -172,7 +172,7 @@ def update_email
 
         return false if user.email == ldap_email
 
-        Users::UpdateService.new(user, user, email: ldap_email).execute do |user|
+        Users::UpdateService.new(user, user: user, email: ldap_email).execute do |user|
           user.skip_reconfirmation!
         end
       end
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index 60b6ed23c87509..619241502a2eea 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -34,7 +34,7 @@ def save(provider = 'OAuth')
 
         block_after_save = needs_blocking?
 
-        Users::UpdateService.new(gl_user, gl_user).execute!
+        Users::UpdateService.new(gl_user, user: gl_user).execute!
 
         gl_user.block if block_after_save
 
diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb
index 707f83b3359943..f8d4a47b212083 100644
--- a/spec/services/users/update_service_spec.rb
+++ b/spec/services/users/update_service_spec.rb
@@ -31,13 +31,13 @@
     end
 
     def update_user(user, opts)
-      described_class.new(user, user, opts).execute
+      described_class.new(user, opts.merge(user: user)).execute
     end
   end
 
   describe '#execute!' do
     it 'updates the name' do
-      service = described_class.new(user, user, name: 'New Name')
+      service = described_class.new(user, user: user, name: 'New Name')
       expect(service).not_to receive(:notify_new_user)
 
       result = service.execute!
@@ -55,7 +55,7 @@ def update_user(user, opts)
     it 'fires system hooks when a new user is saved' do
       system_hook_service = spy(:system_hook_service)
       user = build(:user)
-      service = described_class.new(user, user, name: 'John Doe')
+      service = described_class.new(user, user: user, name: 'John Doe')
       expect(service).to receive(:notify_new_user).and_call_original
       expect(service).to receive(:system_hook_service).and_return(system_hook_service)
 
@@ -65,7 +65,7 @@ def update_user(user, opts)
     end
 
     def update_user(user, opts)
-      described_class.new(user, user, opts).execute!
+      described_class.new(user, opts.merge(user: user)).execute!
     end
   end
 end
-- 
GitLab


From 4b26ed8a37bd4f329c63ad97ef424290cb505e8e Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 27 Sep 2017 12:07:37 +0200
Subject: [PATCH 47/50] fix update service

---
 app/services/users/update_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index e04f30864bdbc2..62cceb0cb7e659 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -5,7 +5,7 @@ class UpdateService < BaseService
 
     def initialize(current_user, params = {})
       @current_user = current_user
-      @user = params[:user]
+      @user = params.delete(:user)
       @params = params.dup
     end
 
-- 
GitLab


From 76c555a059c657840fd8bb3b1a46b339500c11f4 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 27 Sep 2017 16:22:56 +0200
Subject: [PATCH 48/50] update emails service

---
 app/controllers/admin/users_controller.rb     | 2 +-
 app/controllers/profiles/emails_controller.rb | 4 ++--
 app/models/user.rb                            | 4 ++--
 app/services/emails/base_service.rb           | 4 ++--
 lib/api/users.rb                              | 8 ++++----
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index c0433c00f5db1c..36e33e7c690c35 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -155,7 +155,7 @@ def destroy
 
   def remove_email
     email = user.emails.find(params[:email_id])
-    success = Emails::DestroyService.new(current_user, user, email: email.email).execute
+    success = Emails::DestroyService.new(current_user, user: user, email: email.email).execute
 
     respond_to do |format|
       if success
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 8328d23027607d..97db84b92d42df 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -5,7 +5,7 @@ def index
   end
 
   def create
-    @email = Emails::CreateService.new(current_user, current_user, email_params).execute
+    @email = Emails::CreateService.new(current_user, email_params.merge(user: current_user)).execute
 
     if @email.errors.blank?
       NotificationService.new.new_email(@email)
@@ -19,7 +19,7 @@ def create
   def destroy
     @email = current_user.emails.find(params[:id])
 
-    Emails::DestroyService.new(current_user, current_user, email: @email.email).execute
+    Emails::DestroyService.new(current_user, user: current_user, email: @email.email).execute
 
     respond_to do |format|
       format.html { redirect_to profile_emails_url, status: 302 }
diff --git a/app/models/user.rb b/app/models/user.rb
index 5b8b27b7991ab8..21a43c5be9ec96 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -545,8 +545,8 @@ def owns_public_email
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
-      Emails::DestroyService.new(self, self, email: email).execute
-      Emails::CreateService.new(self, self, email: email_was).execute
+      Emails::DestroyService.new(self, user: self, email: email).execute
+      Emails::CreateService.new(self, user: self, email: email_was).execute
     end
   end
 
diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index 8810f6d8803dd0..7f591c89411d1f 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -1,8 +1,8 @@
 module Emails
   class BaseService
-    def initialize(current_user, user, opts)
+    def initialize(current_user, opts)
       @current_user = current_user
-      @user = user
+      @user = opts.delete(:user)
       @email = opts[:email]
     end
   end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index d196fff13f5343..dce47f3a854d2e 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -332,7 +332,7 @@ def find_user(params)
         user = User.find_by(id: params.delete(:id))
         not_found!('User') unless user
 
-        email = Emails::CreateService.new(current_user, user, declared_params(include_missing: false)).execute
+        email = Emails::CreateService.new(current_user,  declared_params(include_missing: false).merge(user: user)).execute
 
         if email.errors.blank?
           NotificationService.new.new_email(email)
@@ -373,7 +373,7 @@ def find_user(params)
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, user, email: email.email).execute
+          Emails::DestroyService.new(current_user, user: user, email: email.email).execute
         end
 
         user.update_secondary_emails!
@@ -678,7 +678,7 @@ def find_impersonation_token
         requires :email, type: String, desc: 'The new email'
       end
       post "emails" do
-        email = Emails::CreateService.new(current_user, current_user, declared_params).execute
+        email = Emails::CreateService.new(current_user, declared_params.merge(user: current_user)).execute
 
         if email.errors.blank?
           NotificationService.new.new_email(email)
@@ -697,7 +697,7 @@ def find_impersonation_token
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user, user: current_user, email: email.email).execute
         end
 
         current_user.update_secondary_emails!
-- 
GitLab


From 481f2a90b1079ac0655b76761234e782e681d5e0 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 27 Sep 2017 16:25:12 +0200
Subject: [PATCH 49/50] fix specs

---
 spec/services/emails/create_service_spec.rb  | 4 ++--
 spec/services/emails/destroy_service_spec.rb | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb
index e3eb1fb846d2fc..e94ccf54412a8e 100644
--- a/spec/services/emails/create_service_spec.rb
+++ b/spec/services/emails/create_service_spec.rb
@@ -2,9 +2,9 @@
 
 describe Emails::CreateService do
   let(:user) { create(:user) }
-  let(:opts) { { email: 'new@email.com' } }
+  let(:opts) { { email: 'new@email.com', user: user} }
 
-  subject(:service) { described_class.new(user, user, opts) }
+  subject(:service) { described_class.new(user, opts) }
 
   describe '#execute' do
     before do
diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb
index 8811b86b5f8928..a69279e3df0d05 100644
--- a/spec/services/emails/destroy_service_spec.rb
+++ b/spec/services/emails/destroy_service_spec.rb
@@ -4,7 +4,7 @@
   let!(:user) { create(:user) }
   let!(:email) { create(:email, user: user) }
 
-  subject(:service) { described_class.new(user, user, email: email.email) }
+  subject(:service) { described_class.new(user, user: user, email: email.email) }
 
   before do
     stub_licensed_features(extended_audit_events: true)
-- 
GitLab


From 40d46d13c3781473509185f9805386e9276b934b Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 28 Sep 2017 08:43:57 +0200
Subject: [PATCH 50/50] fix static analysis error

---
 spec/services/emails/create_service_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb
index e94ccf54412a8e..544f5b228bf2bf 100644
--- a/spec/services/emails/create_service_spec.rb
+++ b/spec/services/emails/create_service_spec.rb
@@ -2,7 +2,7 @@
 
 describe Emails::CreateService do
   let(:user) { create(:user) }
-  let(:opts) { { email: 'new@email.com', user: user} }
+  let(:opts) { { email: 'new@email.com', user: user } }
 
   subject(:service) { described_class.new(user, opts) }
 
-- 
GitLab