From d2e8de2516e6622401c4be0caf120ba9911975a4 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Thu, 29 Jun 2023 18:10:33 +0800
Subject: [PATCH 01/10] Add user_access_unlocked audit event

Create a user_access_unlocked audit event when the user's access to the
instance is unlocked. There are several ways a user's access can be
unlocked including:

- Unlocked by an admin via users admin page
- Unlocked after the lock expires. In this case the author of the audit
event is set to `GitLab Admin Bot`

EE: true
Changelog: added
---
 app/controllers/admin/users_controller.rb     |  7 +-
 app/models/user.rb                            | 10 +++
 .../controllers/ee/admin/users_controller.rb  |  8 +++
 ee/app/models/ee/user.rb                      | 43 ++++++++----
 .../types/user_access_unlocked.yml            |  8 +++
 ee/spec/models/ee/user_spec.rb                | 67 +++++++++++++++++++
 .../requests/admin/users_controller_spec.rb   | 19 ++++++
 7 files changed, 149 insertions(+), 13 deletions(-)
 create mode 100644 ee/config/audit_events/types/user_access_unlocked.yml

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 3c96e49499f06d..b75ca2649c3f9e 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -159,7 +159,7 @@ def unban
   end
 
   def unlock
-    if update_user(&:unlock_access!)
+    if unlock_user
       redirect_back_or_admin_user(notice: _("Successfully unlocked"))
     else
       redirect_back_or_admin_user(alert: _("Error occurred. User was not unlocked"))
@@ -401,6 +401,11 @@ def impersonation_error_text
       _("You cannot impersonate a user who cannot log in")
     end
   end
+
+  # method overriden in EE
+  def unlock_user
+    update_user(&:unlock_access!)
+  end
 end
 
 Admin::UsersController.prepend_mod_with('Admin::UsersController')
diff --git a/app/models/user.rb b/app/models/user.rb
index 508c0a669b7f93..b7cacac45c966f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2067,6 +2067,13 @@ def lock_access!(opts = {})
     super
   end
 
+  # override, from Devise
+  def unlock_access!
+    audit_unlock_access
+
+    super
+  end
+
   # Determine the maximum access level for a group of projects in bulk.
   #
   # Returns a Hash mapping project ID -> maximum access level.
@@ -2593,6 +2600,9 @@ def prefix_for_feed_token
 
   # method overriden in EE
   def audit_lock_access; end
+
+  # method overriden in EE
+  def audit_unlock_access; end
 end
 
 User.prepend_mod_with('User')
diff --git a/ee/app/controllers/ee/admin/users_controller.rb b/ee/app/controllers/ee/admin/users_controller.rb
index 4824720006387f..6c3121cef227da 100644
--- a/ee/app/controllers/ee/admin/users_controller.rb
+++ b/ee/app/controllers/ee/admin/users_controller.rb
@@ -39,6 +39,14 @@ def log_impersonation_event
         log_audit_event
       end
 
+      override :unlock_user
+      def unlock_user
+        update_user do
+          user.unlock_access_audit_event_author_id = current_user.id
+          user.unlock_access!
+        end
+      end
+
       def log_audit_event
         ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(current_user.id, user.id, request.remote_ip, 'started', DateTime.current)
       end
diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index 5ea80f6b112197..7e67c1072bf7c0 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -154,6 +154,8 @@ module User
           user.block_pending_approval if ::User.user_cap_reached?
         end
       end
+
+      attr_accessor :unlock_access_audit_event_author_id
     end
 
     class_methods do
@@ -697,18 +699,35 @@ def should_delay_delete?(*args)
 
     override :audit_lock_access
     def audit_lock_access
-      unless access_locked?
-        reason = nil
-        reason = 'excessive failed login attempts' if attempts_exceeded?
-
-        ::Gitlab::Audit::Auditor.audit(
-          name: 'user_access_locked',
-          author: ::User.admin_bot,
-          scope: self,
-          target: self,
-          message: ['User access locked', reason].compact.join(' - ')
-        )
-      end
+      return if access_locked?
+
+      reason = nil
+      reason = 'excessive failed login attempts' if attempts_exceeded?
+
+      ::Gitlab::Audit::Auditor.audit(
+        name: 'user_access_locked',
+        author: ::User.admin_bot,
+        scope: self,
+        target: self,
+        message: ['User access locked', reason].compact.join(' - ')
+      )
+    end
+
+    override :audit_unlock_access
+    def audit_unlock_access
+      return unless access_locked?
+
+      author = ::User.find_by_id(unlock_access_audit_event_author_id) || ::User.admin_bot
+
+      ::Gitlab::Audit::Auditor.audit(
+        name: 'user_access_unlocked',
+        author: author,
+        scope: self,
+        target: self,
+        message: 'User access unlocked'
+      )
+
+      self.unlock_access_audit_event_author_id = nil
     end
   end
 end
diff --git a/ee/config/audit_events/types/user_access_unlocked.yml b/ee/config/audit_events/types/user_access_unlocked.yml
new file mode 100644
index 00000000000000..bf2d6aa3ac0d7c
--- /dev/null
+++ b/ee/config/audit_events/types/user_access_unlocked.yml
@@ -0,0 +1,8 @@
+name: user_access_unlocked
+description: Event triggered when user access to the instance is unlocked
+introduced_by_issue: https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/244
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124169
+feature_category: 'system_access'
+milestone: '16.2'
+saved_to_database: true
+streamed: true
diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb
index 6e5e29d853ad1f..5a53a16ddc03a3 100644
--- a/ee/spec/models/ee/user_spec.rb
+++ b/ee/spec/models/ee/user_spec.rb
@@ -2903,4 +2903,71 @@
       end
     end
   end
+
+  describe '#unlock_access!' do
+    let_it_be(:gitlab_admin_bot) { described_class.admin_bot }
+    let_it_be_with_reload(:user) { create(:user) }
+
+    subject { user.unlock_access! }
+
+    before do
+      stub_licensed_features(admin_audit_log: true)
+
+      user.lock_access!
+    end
+
+    it 'logs a user_access_unlocked audit event' do
+      expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
+        hash_including(
+          name: 'user_access_unlocked',
+          author: gitlab_admin_bot,
+          scope: user,
+          target: user,
+          message: 'User access unlocked'
+        )
+      ).and_call_original
+      expect { subject }.to change { AuditEvent.count }.by(1)
+    end
+
+    context 'when unlock_access_audit_event_author_id is present' do
+      let(:audit_event_author) { create(:user) }
+
+      before do
+        user.unlock_access_audit_event_author_id = audit_event_author.id
+      end
+
+      it 'logs a user_access_unlocked audit event with the correct author' do
+        expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
+          hash_including(author: audit_event_author)
+        ).and_call_original
+
+        expect { subject }.to change { AuditEvent.count }.by(1)
+        expect(user.unlock_access_audit_event_author_id).to be_nil
+      end
+
+      context 'when unlock_access_audit_event_author_id does not correspond to an existing user' do
+        before do
+          user.unlock_access_audit_event_author_id = non_existing_record_id
+        end
+
+        it 'logs a user_access_unlocked audit event with the correct author' do
+          expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
+            hash_including(author: gitlab_admin_bot)
+          ).and_call_original
+
+          expect { subject }.to change { AuditEvent.count }.by(1)
+        end
+      end
+    end
+
+    context 'when user access is not locked' do
+      before do
+        allow(user).to receive(:access_locked?).and_return(false)
+      end
+
+      it 'does not log an audit event' do
+        expect { subject }.not_to change { AuditEvent.count }
+      end
+    end
+  end
 end
diff --git a/ee/spec/requests/admin/users_controller_spec.rb b/ee/spec/requests/admin/users_controller_spec.rb
index dfe00f6058b92e..de4efa13dfe008 100644
--- a/ee/spec/requests/admin/users_controller_spec.rb
+++ b/ee/spec/requests/admin/users_controller_spec.rb
@@ -58,4 +58,23 @@ def send_request
       expect(assigns(:users).first.association(:elevated_members)).to be_loaded
     end
   end
+
+  describe 'PUT #unlock' do
+    before do
+      user.lock_access!
+    end
+
+    subject(:request) { put unlock_admin_user_path(user) }
+
+    it 'logs a user_access_unlock audit event with author set to the current user' do
+      expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
+        hash_including(
+          name: 'user_access_unlocked',
+          author: admin
+        )
+      ).and_call_original
+
+      expect { request }.to change { user.reload.access_locked? }.from(true).to(false)
+    end
+  end
 end
-- 
GitLab


From 968a7f99960d193d0bab11ea8d7e1b9d10313fee Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Thu, 29 Jun 2023 18:16:31 +0800
Subject: [PATCH 02/10] Update audit event type definition file with the
 correct MR URL

---
 ee/config/audit_events/types/user_access_unlocked.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/config/audit_events/types/user_access_unlocked.yml b/ee/config/audit_events/types/user_access_unlocked.yml
index bf2d6aa3ac0d7c..35462fc9d179d7 100644
--- a/ee/config/audit_events/types/user_access_unlocked.yml
+++ b/ee/config/audit_events/types/user_access_unlocked.yml
@@ -1,7 +1,7 @@
 name: user_access_unlocked
 description: Event triggered when user access to the instance is unlocked
 introduced_by_issue: https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/244
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124169
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124973
 feature_category: 'system_access'
 milestone: '16.2'
 saved_to_database: true
-- 
GitLab


From 64da9d8bce2765262d8ac99edd154a4da9dc3e03 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Thu, 29 Jun 2023 20:50:18 +0800
Subject: [PATCH 03/10] Log an audit event when user unlocks their own user
 access

---
 .../concerns/verifies_with_email.rb           |  8 ++++-
 .../concerns/ee/verifies_with_email.rb        | 14 ++++++++
 .../requests/ee/verifies_with_email_spec.rb   | 32 +++++++++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 ee/app/controllers/concerns/ee/verifies_with_email.rb
 create mode 100644 ee/spec/requests/ee/verifies_with_email_spec.rb

diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb
index 45869c05f41ea5..e0d88f70fe7ed2 100644
--- a/app/controllers/concerns/verifies_with_email.rb
+++ b/app/controllers/concerns/verifies_with_email.rb
@@ -129,7 +129,7 @@ def handle_verification_failure(user, reason, message)
   end
 
   def handle_verification_success(user)
-    user.unlock_access!
+    unlock_access(user)
     log_verification(user, :successful)
 
     sign_in(user)
@@ -166,4 +166,10 @@ def require_email_verification_enabled?(user)
     Feature.enabled?(:require_email_verification, user) &&
       Feature.disabled?(:skip_require_email_verification, user, type: :ops)
   end
+
+  def unlock_access(user)
+    user.unlock_access!
+  end
 end
+
+VerifiesWithEmail.prepend_mod_with('VerifiesWithEmail')
diff --git a/ee/app/controllers/concerns/ee/verifies_with_email.rb b/ee/app/controllers/concerns/ee/verifies_with_email.rb
new file mode 100644
index 00000000000000..eddd6b9227c3c3
--- /dev/null
+++ b/ee/app/controllers/concerns/ee/verifies_with_email.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module EE
+  module VerifiesWithEmail
+    extend ::Gitlab::Utils::Override
+
+    override :unlock_access
+    def unlock_access(user)
+      user.unlock_access_audit_event_author_id = user.id
+
+      super
+    end
+  end
+end
diff --git a/ee/spec/requests/ee/verifies_with_email_spec.rb b/ee/spec/requests/ee/verifies_with_email_spec.rb
new file mode 100644
index 00000000000000..d07457615f19f0
--- /dev/null
+++ b/ee/spec/requests/ee/verifies_with_email_spec.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'VerifiesWithEmail', :clean_gitlab_redis_sessions, feature_category: :user_management do
+  include SessionHelpers
+
+  let_it_be(:user) { create(:user) }
+
+  describe '#verify_with_email' do
+    context 'when user is locked and a verification_user_id session variable exists' do
+      before do
+        encrypted_token = Devise.token_generator.digest(User, user.email, 'token')
+        user.update!(locked_at: Time.current, unlock_token: encrypted_token)
+        stub_session(verification_user_id: user.id)
+      end
+
+      context 'when a valid verification_token param exists' do
+        it 'logs a user_access_unlock audit event with author set to the user' do
+          expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
+            hash_including(
+              name: 'user_access_unlocked',
+              author: user
+            )
+          )
+
+          post(user_session_path(user: { verification_token: 'token' }))
+        end
+      end
+    end
+  end
+end
-- 
GitLab


From 6ca48a8875e2aa43877ae49655eb26b2b36461a5 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Fri, 30 Jun 2023 16:51:57 +0800
Subject: [PATCH 04/10] Update check to see if a user's access is locked

---
 ee/app/models/ee/user.rb       | 6 +++++-
 ee/spec/models/ee/user_spec.rb | 6 ++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index 7e67c1072bf7c0..2064b7f1622edc 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -715,7 +715,11 @@ def audit_lock_access
 
     override :audit_unlock_access
     def audit_unlock_access
-      return unless access_locked?
+      # We can't use access_locked? because it checks if locked_at <
+      # User.unlock_in.ago. If we use access_locked? and the lock is already
+      # expired the call to unlock_access! when a user tries to login will not
+      # log an audit event as expected
+      return unless locked_at.present?
 
       author = ::User.find_by_id(unlock_access_audit_event_author_id) || ::User.admin_bot
 
diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb
index 5a53a16ddc03a3..a09e40d670a492 100644
--- a/ee/spec/models/ee/user_spec.rb
+++ b/ee/spec/models/ee/user_spec.rb
@@ -2961,12 +2961,10 @@
     end
 
     context 'when user access is not locked' do
-      before do
-        allow(user).to receive(:access_locked?).and_return(false)
-      end
+      let_it_be(:active_user) { create(:user) }
 
       it 'does not log an audit event' do
-        expect { subject }.not_to change { AuditEvent.count }
+        expect { active_user.unlock_access! }.not_to change { AuditEvent.count }
       end
     end
   end
-- 
GitLab


From c85292c7c1fc750a8cd35dd005a1365d763e9da9 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Tue, 4 Jul 2023 14:46:13 +0800
Subject: [PATCH 05/10] Add comment to indicate method is overridden in EE

---
 app/controllers/concerns/verifies_with_email.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb
index e0d88f70fe7ed2..4572fd0b5bcd84 100644
--- a/app/controllers/concerns/verifies_with_email.rb
+++ b/app/controllers/concerns/verifies_with_email.rb
@@ -167,6 +167,7 @@ def require_email_verification_enabled?(user)
       Feature.disabled?(:skip_require_email_verification, user, type: :ops)
   end
 
+  # method overriden in EE
   def unlock_access(user)
     user.unlock_access!
   end
-- 
GitLab


From 9af8bfa3ac97baec6406daaf265acf77743ab1dc Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Tue, 4 Jul 2023 16:08:40 +0800
Subject: [PATCH 06/10] Document user_access_unlocked audit event

---
 doc/administration/audit_events.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md
index a80d3421d348fb..6f91a6aa4d16a2 100644
--- a/doc/administration/audit_events.md
+++ b/doc/administration/audit_events.md
@@ -365,6 +365,7 @@ GitLab generates audit events when a cluster agent token is created or revoked.
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/238177) in
   GitLab 15.1, audit events when a user's two-factor authentication is disabled.
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124169) in GitLab 16.2, audit events when a user's access is locked.
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124973) in GitLab 16.2, audit events when a user's access is unlocked.
 
 The following user actions on a GitLab instance generate instance audit events:
 
@@ -378,6 +379,7 @@ The following user actions on a GitLab instance generate instance audit events:
 - A user's personal access token was successfully or unsuccessfully created or revoked.
 - A user's two-factor authentication was disabled.
 - A user's access is locked.
+- A user's access is unlocked.
 
 #### User management
 
-- 
GitLab


From 09038da0f1a6d9cfa2a962563d52d7bb383e55f6 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 5 Jul 2023 10:18:03 +0800
Subject: [PATCH 07/10] Set user as the default user_access_unlocked audit
 event author

---
 .../concerns/verifies_with_email.rb           |  9 +---
 .../concerns/ee/verifies_with_email.rb        | 14 -----
 ee/app/models/ee/user.rb                      |  4 +-
 ee/spec/models/ee/user_spec.rb                | 52 +++++++++----------
 .../requests/ee/verifies_with_email_spec.rb   | 32 ------------
 5 files changed, 27 insertions(+), 84 deletions(-)
 delete mode 100644 ee/app/controllers/concerns/ee/verifies_with_email.rb
 delete mode 100644 ee/spec/requests/ee/verifies_with_email_spec.rb

diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb
index 4572fd0b5bcd84..45869c05f41ea5 100644
--- a/app/controllers/concerns/verifies_with_email.rb
+++ b/app/controllers/concerns/verifies_with_email.rb
@@ -129,7 +129,7 @@ def handle_verification_failure(user, reason, message)
   end
 
   def handle_verification_success(user)
-    unlock_access(user)
+    user.unlock_access!
     log_verification(user, :successful)
 
     sign_in(user)
@@ -166,11 +166,4 @@ def require_email_verification_enabled?(user)
     Feature.enabled?(:require_email_verification, user) &&
       Feature.disabled?(:skip_require_email_verification, user, type: :ops)
   end
-
-  # method overriden in EE
-  def unlock_access(user)
-    user.unlock_access!
-  end
 end
-
-VerifiesWithEmail.prepend_mod_with('VerifiesWithEmail')
diff --git a/ee/app/controllers/concerns/ee/verifies_with_email.rb b/ee/app/controllers/concerns/ee/verifies_with_email.rb
deleted file mode 100644
index eddd6b9227c3c3..00000000000000
--- a/ee/app/controllers/concerns/ee/verifies_with_email.rb
+++ /dev/null
@@ -1,14 +0,0 @@
-# frozen_string_literal: true
-
-module EE
-  module VerifiesWithEmail
-    extend ::Gitlab::Utils::Override
-
-    override :unlock_access
-    def unlock_access(user)
-      user.unlock_access_audit_event_author_id = user.id
-
-      super
-    end
-  end
-end
diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index 2064b7f1622edc..8421f076afd071 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -721,11 +721,11 @@ def audit_unlock_access
       # log an audit event as expected
       return unless locked_at.present?
 
-      author = ::User.find_by_id(unlock_access_audit_event_author_id) || ::User.admin_bot
+      author = ::User.find_by_id(unlock_access_audit_event_author_id) if unlock_access_audit_event_author_id
 
       ::Gitlab::Audit::Auditor.audit(
         name: 'user_access_unlocked',
-        author: author,
+        author: author || self,
         scope: self,
         target: self,
         message: 'User access unlocked'
diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb
index a09e40d670a492..aaf8fe92b0d1fb 100644
--- a/ee/spec/models/ee/user_spec.rb
+++ b/ee/spec/models/ee/user_spec.rb
@@ -2905,7 +2905,6 @@
   end
 
   describe '#unlock_access!' do
-    let_it_be(:gitlab_admin_bot) { described_class.admin_bot }
     let_it_be_with_reload(:user) { create(:user) }
 
     subject { user.unlock_access! }
@@ -2916,46 +2915,43 @@
       user.lock_access!
     end
 
-    it 'logs a user_access_unlocked audit event' do
-      expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
-        hash_including(
-          name: 'user_access_unlocked',
-          author: gitlab_admin_bot,
-          scope: user,
-          target: user,
-          message: 'User access unlocked'
-        )
-      ).and_call_original
-      expect { subject }.to change { AuditEvent.count }.by(1)
-    end
-
-    context 'when unlock_access_audit_event_author_id is present' do
-      let(:audit_event_author) { create(:user) }
-
+    shared_examples 'logs a user_access_unlocked audit event with the correct author' do
       before do
-        user.unlock_access_audit_event_author_id = audit_event_author.id
+        user.unlock_access_audit_event_author_id = author_id
       end
 
       it 'logs a user_access_unlocked audit event with the correct author' do
         expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
-          hash_including(author: audit_event_author)
+          hash_including(
+            name: 'user_access_unlocked',
+            author: expected_author,
+            scope: user,
+            target: user,
+            message: 'User access unlocked'
+          )
         ).and_call_original
 
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(user.unlock_access_audit_event_author_id).to be_nil
       end
+    end
 
-      context 'when unlock_access_audit_event_author_id does not correspond to an existing user' do
-        before do
-          user.unlock_access_audit_event_author_id = non_existing_record_id
-        end
+    it_behaves_like 'logs a user_access_unlocked audit event with the correct author' do
+      let(:expected_author) { user }
+      let(:author_id) { nil }
+    end
 
-        it 'logs a user_access_unlocked audit event with the correct author' do
-          expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
-            hash_including(author: gitlab_admin_bot)
-          ).and_call_original
+    context 'when unlock_access_audit_event_author_id is present' do
+      it_behaves_like 'logs a user_access_unlocked audit event with the correct author' do
+        let_it_be(:expected_author) { create(:user) }
 
-          expect { subject }.to change { AuditEvent.count }.by(1)
+        let(:author_id) { expected_author.id }
+      end
+
+      context 'when unlock_access_audit_event_author_id does not correspond to an existing user' do
+        it_behaves_like 'logs a user_access_unlocked audit event with the correct author' do
+          let(:expected_author) { user }
+          let(:author_id) { non_existing_record_id }
         end
       end
     end
diff --git a/ee/spec/requests/ee/verifies_with_email_spec.rb b/ee/spec/requests/ee/verifies_with_email_spec.rb
deleted file mode 100644
index d07457615f19f0..00000000000000
--- a/ee/spec/requests/ee/verifies_with_email_spec.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe 'VerifiesWithEmail', :clean_gitlab_redis_sessions, feature_category: :user_management do
-  include SessionHelpers
-
-  let_it_be(:user) { create(:user) }
-
-  describe '#verify_with_email' do
-    context 'when user is locked and a verification_user_id session variable exists' do
-      before do
-        encrypted_token = Devise.token_generator.digest(User, user.email, 'token')
-        user.update!(locked_at: Time.current, unlock_token: encrypted_token)
-        stub_session(verification_user_id: user.id)
-      end
-
-      context 'when a valid verification_token param exists' do
-        it 'logs a user_access_unlock audit event with author set to the user' do
-          expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
-            hash_including(
-              name: 'user_access_unlocked',
-              author: user
-            )
-          )
-
-          post(user_session_path(user: { verification_token: 'token' }))
-        end
-      end
-    end
-  end
-end
-- 
GitLab


From 080e8811f1f37f9ad32f96ecb5a0931a5ad28b97 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 5 Jul 2023 10:18:31 +0800
Subject: [PATCH 08/10] Remove unnecessary initialization

---
 ee/app/models/ee/user.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index 8421f076afd071..74e0bb2b5389fa 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -701,7 +701,6 @@ def should_delay_delete?(*args)
     def audit_lock_access
       return if access_locked?
 
-      reason = nil
       reason = 'excessive failed login attempts' if attempts_exceeded?
 
       ::Gitlab::Audit::Auditor.audit(
-- 
GitLab


From d7d4a7b155497f0aa54a006867118204b3cc9278 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 5 Jul 2023 11:31:04 +0800
Subject: [PATCH 09/10] Add specs for unlock controller action

---
 spec/requests/admin/users_controller_spec.rb | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb
index 5344a2c2bb7de9..21cf8ab2c79c82 100644
--- a/spec/requests/admin/users_controller_spec.rb
+++ b/spec/requests/admin/users_controller_spec.rb
@@ -6,12 +6,12 @@
   let_it_be(:admin) { create(:admin) }
   let_it_be(:user) { create(:user) }
 
+  before do
+    sign_in(admin)
+  end
+
   describe 'PUT #block' do
     context 'when request format is :json' do
-      before do
-        sign_in(admin)
-      end
-
       subject(:request) { put block_admin_user_path(user, format: :json) }
 
       context 'when user was blocked' do
@@ -39,4 +39,16 @@
       end
     end
   end
+
+  describe 'PUT #unlock' do
+    before do
+      user.lock_access!
+    end
+
+    subject(:request) { put unlock_admin_user_path(user) }
+
+    it 'unlocks the user' do
+      expect { request }.to change { user.reload.access_locked? }.from(true).to(false)
+    end
+  end
 end
-- 
GitLab


From d0a4f6dee53c405d68c58576a3bdcb7202acd68b Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Thu, 6 Jul 2023 15:55:23 +0800
Subject: [PATCH 10/10] Remove use of virtual attribute
 unlock_access_audit_event_author_id

---
 app/models/user.rb                              |  8 ++++----
 ee/app/controllers/ee/admin/users_controller.rb |  3 +--
 ee/app/models/ee/user.rb                        | 10 ++--------
 ee/spec/models/ee/user_spec.rb                  | 17 ++---------------
 4 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index b7cacac45c966f..3833da40865219 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2068,10 +2068,10 @@ def lock_access!(opts = {})
   end
 
   # override, from Devise
-  def unlock_access!
-    audit_unlock_access
+  def unlock_access!(unlocked_by: self)
+    audit_unlock_access(author: unlocked_by)
 
-    super
+    super()
   end
 
   # Determine the maximum access level for a group of projects in bulk.
@@ -2602,7 +2602,7 @@ def prefix_for_feed_token
   def audit_lock_access; end
 
   # method overriden in EE
-  def audit_unlock_access; end
+  def audit_unlock_access(author: self); end
 end
 
 User.prepend_mod_with('User')
diff --git a/ee/app/controllers/ee/admin/users_controller.rb b/ee/app/controllers/ee/admin/users_controller.rb
index 6c3121cef227da..c7d4ff273573ab 100644
--- a/ee/app/controllers/ee/admin/users_controller.rb
+++ b/ee/app/controllers/ee/admin/users_controller.rb
@@ -42,8 +42,7 @@ def log_impersonation_event
       override :unlock_user
       def unlock_user
         update_user do
-          user.unlock_access_audit_event_author_id = current_user.id
-          user.unlock_access!
+          user.unlock_access!(unlocked_by: current_user)
         end
       end
 
diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index 74e0bb2b5389fa..dae821683f055e 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -154,8 +154,6 @@ module User
           user.block_pending_approval if ::User.user_cap_reached?
         end
       end
-
-      attr_accessor :unlock_access_audit_event_author_id
     end
 
     class_methods do
@@ -713,24 +711,20 @@ def audit_lock_access
     end
 
     override :audit_unlock_access
-    def audit_unlock_access
+    def audit_unlock_access(author: self)
       # We can't use access_locked? because it checks if locked_at <
       # User.unlock_in.ago. If we use access_locked? and the lock is already
       # expired the call to unlock_access! when a user tries to login will not
       # log an audit event as expected
       return unless locked_at.present?
 
-      author = ::User.find_by_id(unlock_access_audit_event_author_id) if unlock_access_audit_event_author_id
-
       ::Gitlab::Audit::Auditor.audit(
         name: 'user_access_unlocked',
-        author: author || self,
+        author: author,
         scope: self,
         target: self,
         message: 'User access unlocked'
       )
-
-      self.unlock_access_audit_event_author_id = nil
     end
   end
 end
diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb
index aaf8fe92b0d1fb..13d60c849f5be3 100644
--- a/ee/spec/models/ee/user_spec.rb
+++ b/ee/spec/models/ee/user_spec.rb
@@ -2916,10 +2916,6 @@
     end
 
     shared_examples 'logs a user_access_unlocked audit event with the correct author' do
-      before do
-        user.unlock_access_audit_event_author_id = author_id
-      end
-
       it 'logs a user_access_unlocked audit event with the correct author' do
         expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
           hash_including(
@@ -2932,27 +2928,18 @@
         ).and_call_original
 
         expect { subject }.to change { AuditEvent.count }.by(1)
-        expect(user.unlock_access_audit_event_author_id).to be_nil
       end
     end
 
     it_behaves_like 'logs a user_access_unlocked audit event with the correct author' do
       let(:expected_author) { user }
-      let(:author_id) { nil }
     end
 
-    context 'when unlock_access_audit_event_author_id is present' do
+    context 'when unlocked_by is specified' do
       it_behaves_like 'logs a user_access_unlocked audit event with the correct author' do
         let_it_be(:expected_author) { create(:user) }
 
-        let(:author_id) { expected_author.id }
-      end
-
-      context 'when unlock_access_audit_event_author_id does not correspond to an existing user' do
-        it_behaves_like 'logs a user_access_unlocked audit event with the correct author' do
-          let(:expected_author) { user }
-          let(:author_id) { non_existing_record_id }
-        end
+        subject { user.unlock_access!(unlocked_by: expected_author) }
       end
     end
 
-- 
GitLab