diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 3c96e49499f06d3eb077f57e12e2d6f4061f553a..b75ca2649c3f9ee9aea46e0ff3d46e6dbf83a69b 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 508c0a669b7f93c7638a8c504321584264d2f1ee..3833da40865219f92b7345011cf66d7a0f329c7a 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!(unlocked_by: self) + audit_unlock_access(author: unlocked_by) + + 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(author: self); end end User.prepend_mod_with('User') diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index a80d3421d348fb61556832e3bc33f040f2d6fbb0..6f91a6aa4d16a2fd90622aeab24764b8e3cbd22e 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 diff --git a/ee/app/controllers/ee/admin/users_controller.rb b/ee/app/controllers/ee/admin/users_controller.rb index 4824720006387fa021f9cd83eef13887f752d798..c7d4ff273573abb934c3a4a8ced05be4ac5b682b 100644 --- a/ee/app/controllers/ee/admin/users_controller.rb +++ b/ee/app/controllers/ee/admin/users_controller.rb @@ -39,6 +39,13 @@ def log_impersonation_event log_audit_event end + override :unlock_user + def unlock_user + update_user do + user.unlock_access!(unlocked_by: current_user) + 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 5ea80f6b1121978ac76ad203d7bf8958de7a7238..dae821683f055eda6420159eeccab0f43641307e 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -697,18 +697,34 @@ 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 = '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(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? + + ::Gitlab::Audit::Auditor.audit( + name: 'user_access_unlocked', + author: author, + scope: self, + target: self, + message: 'User access unlocked' + ) 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 0000000000000000000000000000000000000000..35462fc9d179d7041501dde1ed6756a76f328ac9 --- /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/124973 +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 6e5e29d853ad1f31a87616383f54d6550554295c..13d60c849f5be33a90e022b9a7aca20b72063473 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -2903,4 +2903,52 @@ end end end + + describe '#unlock_access!' do + 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 + + shared_examples 'logs a user_access_unlocked audit event with the correct author' do + it 'logs a user_access_unlocked audit event with the correct author' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + 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) + end + end + + it_behaves_like 'logs a user_access_unlocked audit event with the correct author' do + let(:expected_author) { user } + end + + 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) } + + subject { user.unlock_access!(unlocked_by: expected_author) } + end + end + + context 'when user access is not locked' do + let_it_be(:active_user) { create(:user) } + + it 'does not log an audit event' do + expect { active_user.unlock_access! }.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 dfe00f6058b92eb04a15632559f89b8c34165076..de4efa13dfe008514c8e465f214f427a2635d709 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 diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index 5344a2c2bb7de9afe0ccbad7f146ccc42be59edb..21cf8ab2c79c821024f827c1adb6c07fd48ea377 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