diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index 38839497fb6e2436628bda6351caa61c8f6a5cdf..d1ca16bd8fb79c693f142b3b5099df1096a89087 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -43,6 +43,7 @@ def update resource.password_expires_at = nil resource.save(validate: false) if resource.changed? else + log_audit_reset_failure(@user) track_weak_password_error(@user, self.class.name, 'create') end end @@ -50,6 +51,9 @@ def update protected + # overriden in EE + def log_audit_reset_failure(_user); end + def resource_from_email email = resource_params[:email] self.resource = resource_class.find_by_email(email) diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md index 2a7c3367f6a46645f3d2fdd6c2e5ef7c16e27124..331e991aeb325fc1d7d0714de36986badcc577a8 100644 --- a/doc/administration/audit_event_streaming/audit_event_types.md +++ b/doc/administration/audit_event_streaming/audit_event_types.md @@ -170,6 +170,7 @@ Every audit event is associated with an event type. The association with the eve | [`merged_merge_request_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118793) | Audit event triggered when a merged merge request is deleted | **{dotted-circle}** No | **{check-circle}** Yes | `source_code_management` | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/408288) | | [`merged_merge_request_deletion_started`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118793) | Audit event triggered when a merged merge request's deletion is started | **{dotted-circle}** No | **{check-circle}** Yes | `source_code_management` | GitLab [16.1](https://gitlab.com/gitlab-org/gitlab/-/issues/408288) | | [`omniauth_login_failed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123080) | Event triggered when an OmniAuth login fails | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [16.3](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | +| [`password_reset_failed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129079) | Event triggered when a password reset fails for a user | **{dotted-circle}** No | **{check-circle}** Yes | `user_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/377762) | | [`password_reset_requested`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114548) | Event triggered when a user requests a password reset using a registered email address | **{check-circle}** Yes | **{dotted-circle}** No | `compliance_management` | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | | [`personal_access_token_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108952) | Event triggered when a user creates a personal access token | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374113) | | [`personal_access_token_revoked`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108952) | Event triggered when a personal access token is revoked | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374113) | diff --git a/ee/app/controllers/ee/passwords_controller.rb b/ee/app/controllers/ee/passwords_controller.rb index 5c1282a2dc5a507d2485ce671be43e2d1fc88606..c6197a879b3ffa23edcd2c5050de230239fb131e 100644 --- a/ee/app/controllers/ee/passwords_controller.rb +++ b/ee/app/controllers/ee/passwords_controller.rb @@ -3,11 +3,17 @@ module EE module PasswordsController extend ActiveSupport::Concern + extend ::Gitlab::Utils::Override prepended do before_action :log_audit_event, only: [:create] end + override :log_audit_reset_failure + def log_audit_reset_failure(user) + ::Audit::UserPasswordResetAuditor.new(user, user, request.remote_ip).audit_reset_failure + end + private def log_audit_event diff --git a/ee/config/audit_events/types/password_reset_failed.yml b/ee/config/audit_events/types/password_reset_failed.yml new file mode 100644 index 0000000000000000000000000000000000000000..418c6b1faf7e3685d5d81356e301283d2c390cf4 --- /dev/null +++ b/ee/config/audit_events/types/password_reset_failed.yml @@ -0,0 +1,9 @@ +--- +name: password_reset_failed +description: Event triggered when a password reset fails for a user +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/377762 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129079 +feature_category: user_management +milestone: '16.4' +saved_to_database: false +streamed: true diff --git a/ee/lib/audit/user_password_reset_auditor.rb b/ee/lib/audit/user_password_reset_auditor.rb new file mode 100644 index 0000000000000000000000000000000000000000..3c2f9146133a823335e913634eacd6e80ea52ff7 --- /dev/null +++ b/ee/lib/audit/user_password_reset_auditor.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Audit + class UserPasswordResetAuditor < BaseChangesAuditor + def initialize(current_user, model, remote_ip) + super(current_user, model) + + @remote_ip = remote_ip + end + + def audit_reset_failure + errors = @model.errors[:password] + return if errors.blank? + + ::Gitlab::Audit::Auditor.audit({ + name: "password_reset_failed", + author: @current_user, + scope: @model, + target: @model, + target_details: @current_user.email, + message: failure_message(errors), + ip_address: @remote_ip + }) + end + + private + + def failure_message(errors) + "Password reset failed with reason#{errors.count > 1 ? 's' : nil}: #{errors.to_sentence}" + end + end +end diff --git a/ee/spec/controllers/passwords_controller_spec.rb b/ee/spec/controllers/passwords_controller_spec.rb index 9c4d10497ddca0fdabf0c1c313c5a0c3186f3971..12a247450ef5120cc9985f0170a85675e2e319d9 100644 --- a/ee/spec/controllers/passwords_controller_spec.rb +++ b/ee/spec/controllers/passwords_controller_spec.rb @@ -13,6 +13,32 @@ subject(:post_create) { post :create, params: { user: { email: email } } } + describe "#update" do + context "when there is error updating the password" do + subject do + put :update, params: { + user: { + password: password, + password_confirmation: password_confirmation, + reset_password_token: reset_password_token + } + } + end + + let(:password) { "short" } # short invalid password + let(:password_confirmation) { password } + let(:reset_password_token) { user.send_reset_password_instructions } + let(:user) { create(:user, password_automatically_set: true, password_expires_at: 10.minutes.ago) } + + it "calls `::Audit::UserPasswordResetAuditor` with correct args" do + expect(::Audit::UserPasswordResetAuditor).to receive(:new).with(user, user, + instance_of(String)).and_call_original + + subject + end + end + end + context "when email exists" do let(:email) { user.email } diff --git a/ee/spec/lib/audit/user_password_reset_auditor_spec.rb b/ee/spec/lib/audit/user_password_reset_auditor_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b63f755b1a0383efd9ca1f450627a38e8e24a109 --- /dev/null +++ b/ee/spec/lib/audit/user_password_reset_auditor_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Audit::UserPasswordResetAuditor, feature_category: :audit_events do + let_it_be(:user) { create(:user) } + let_it_be(:remote_ip) { "127.0.0.1" } + + describe "#audit_reset_failure" do + subject(:audit_reset_failure) { described_class.new(user, user, remote_ip).audit_reset_failure } + + context "when there are no errors in password" do + before do + allow(user).to receive(:errors).and_return({}) + end + + it "doesn't audit" do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + audit_reset_failure + end + end + + shared_examples "logs audit event with correct reason" do |reason| + it "does audit with correct reason" do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + { name: "password_reset_failed", + author: user, + scope: user, + target: user, + target_details: user.email, + message: reason, + ip_address: remote_ip } + ).and_call_original + + audit_reset_failure + end + end + + context "when there is a single error in password" do + before do + allow(user).to receive(:errors).and_return({ password: ["must contain a letter"] }) + end + + it_behaves_like "logs audit event with correct reason", + "Password reset failed with reason: must contain a letter" + end + + context "when there are multiple errors in password" do + before do + allow(user).to receive(:errors).and_return({ + password: ["must contain a letter", + "must not contain commonly used characters"] + }) + end + + it_behaves_like "logs audit event with correct reason", + "Password reset failed with reasons: must contain a letter and must not contain commonly used characters" + end + end +end