From 7595546ec34bf97b7925bd12b753e80abd544229 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Fri, 31 Mar 2023 04:49:54 -0500 Subject: [PATCH 1/5] Add audit event detail spec for PasswordsController --- .../controllers/passwords_controller_spec.rb | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/ee/spec/controllers/passwords_controller_spec.rb b/ee/spec/controllers/passwords_controller_spec.rb index 75ad364e3073af..efc33cf92dd0aa 100644 --- a/ee/spec/controllers/passwords_controller_spec.rb +++ b/ee/spec/controllers/passwords_controller_spec.rb @@ -13,6 +13,23 @@ subject { post :create, params: { user: { email: user.email } } } - it { expect { subject }.to change { AuditEvent.count }.by(1) } + it do + expect { subject }.to change { AuditEvent.count }.by(1) + + audit_event = AuditEvent.last + expect(audit_event.attributes).to include({ + "entity_id" => user.id, + "entity_type" => "User", + "author_name" => "An unauthenticated user", + "target_type" => "User", + "target_details" => user.email + }) + expect(audit_event.details).to include({ + custom_message: "Ask for password reset", + author_name: "An unauthenticated user", + target_type: "User", + target_details: user.email + }) + end end end -- GitLab From 0775298de932456a3834ab6b284e203f9dcf729a Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Fri, 31 Mar 2023 05:16:09 -0500 Subject: [PATCH 2/5] Refactor PasswordsController audit events --- ee/app/controllers/ee/passwords_controller.rb | 17 ++++++++--------- .../types/password_reset_requested.yml | 10 ++++++++++ .../controllers/passwords_controller_spec.rb | 1 + 3 files changed, 19 insertions(+), 9 deletions(-) create mode 100644 ee/config/audit_events/types/password_reset_requested.yml diff --git a/ee/app/controllers/ee/passwords_controller.rb b/ee/app/controllers/ee/passwords_controller.rb index b686286e1178ef..e864dac64c27c7 100644 --- a/ee/app/controllers/ee/passwords_controller.rb +++ b/ee/app/controllers/ee/passwords_controller.rb @@ -11,16 +11,15 @@ module PasswordsController private def log_audit_event - ::AuditEventService.new( - current_user, - resource, - action: :custom, - custom_message: 'Ask for password reset', + ::Gitlab::Audit::Auditor.audit({ + name: "password_reset_requested", + author: ::Gitlab::Audit::UnauthenticatedAuthor.new, + scope: resource, + target: ::User.new, + target_details: resource_params[:email], + message: "Ask for password reset", ip_address: request.remote_ip - ).for_user( - full_path: resource_params[:email], - entity_id: nil - ).unauth_security_event + }) end end end diff --git a/ee/config/audit_events/types/password_reset_requested.yml b/ee/config/audit_events/types/password_reset_requested.yml new file mode 100644 index 00000000000000..970a446287e115 --- /dev/null +++ b/ee/config/audit_events/types/password_reset_requested.yml @@ -0,0 +1,10 @@ +--- +name: password_reset_requested +description: Event triggered when a user requests a password reset using a registered + email address +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114548 +feature_category: compliance_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/spec/controllers/passwords_controller_spec.rb b/ee/spec/controllers/passwords_controller_spec.rb index efc33cf92dd0aa..36c12ad08a4cd1 100644 --- a/ee/spec/controllers/passwords_controller_spec.rb +++ b/ee/spec/controllers/passwords_controller_spec.rb @@ -20,6 +20,7 @@ expect(audit_event.attributes).to include({ "entity_id" => user.id, "entity_type" => "User", + "entity_path" => nil, "author_name" => "An unauthenticated user", "target_type" => "User", "target_details" => user.email -- GitLab From aa76215359b91974b27cb1dc90cd0d8f964108ef Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Fri, 31 Mar 2023 07:18:16 -0500 Subject: [PATCH 3/5] Refactor audit events for OmniauthCallbacksController --- .../ee/omniauth_callbacks_controller.rb | 25 +++++++++++++++---- .../types/omniauth_login_failed.yml | 9 +++++++ .../ee/omniauth_callbacks_controller_spec.rb | 17 +++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-) create mode 100644 ee/config/audit_events/types/omniauth_login_failed.yml diff --git a/ee/app/controllers/ee/omniauth_callbacks_controller.rb b/ee/app/controllers/ee/omniauth_callbacks_controller.rb index 7f3c81bc14f705..c3cb27f88d1684 100644 --- a/ee/app/controllers/ee/omniauth_callbacks_controller.rb +++ b/ee/app/controllers/ee/omniauth_callbacks_controller.rb @@ -17,11 +17,26 @@ def openid_connect override :log_failed_login def log_failed_login(author, provider) - ::AuditEventService.new( - author, - nil, - with: provider - ).for_failed_login.unauth_security_event + # ::AuditEventService.new( + # author, + # with: provider + # nil, + # ).for_failed_login.unauth_security_event + + unauth_author = ::Gitlab::Audit::UnauthenticatedAuthor.new(name: author) + user = ::User.new(id: unauth_author.id, name: author) + ::Gitlab::Audit::Auditor.audit({ + name: "omniauth_login_failed", + author: unauth_author, + scope: user, + target: user, + additional_details: { + failed_login: provider.upcase, + author_name: user.name, + target_details: user.name + }, + message: "#{provider.upcase} login failed" + }) end override :sign_in_and_redirect_or_verify_identity diff --git a/ee/config/audit_events/types/omniauth_login_failed.yml b/ee/config/audit_events/types/omniauth_login_failed.yml new file mode 100644 index 00000000000000..9415084861f949 --- /dev/null +++ b/ee/config/audit_events/types/omniauth_login_failed.yml @@ -0,0 +1,9 @@ +--- +name: omniauth_login_failed +description: Event triggered when an OmniAuth login fails +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114548 +feature_category: compliance_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/spec/controllers/ee/omniauth_callbacks_controller_spec.rb b/ee/spec/controllers/ee/omniauth_callbacks_controller_spec.rb index 1312db4151bf02..c145bfb376368f 100644 --- a/ee/spec/controllers/ee/omniauth_callbacks_controller_spec.rb +++ b/ee/spec/controllers/ee/omniauth_callbacks_controller_spec.rb @@ -30,7 +30,24 @@ it 'audits provider failed login when licensed' do stub_licensed_features(extended_audit_events: true) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "omniauth_login_failed" + })).and_call_original + expect { subject.failure }.to change { AuditEvent.count }.by(1) + + audit_event = AuditEvent.last + expect(audit_event.attributes).to include({ + "author_name" => user.username, + "entity_type" => "User", + "target_details" => user.username + }) + expect(audit_event.details).to include({ + failed_login: "LDAP", + author_name: user.username, + target_details: user.username, + custom_message: "LDAP login failed" + }) end it 'does not audit provider failed login when unlicensed' do -- GitLab From 17ca397640829692d6566ab144c0fc89bf6cf902 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Sun, 2 Apr 2023 09:21:36 -0500 Subject: [PATCH 4/5] Refactor RegistrationsController audit events --- .../ee/omniauth_callbacks_controller.rb | 6 ------ .../controllers/ee/registrations_controller.rb | 14 ++++++++------ .../types/registration_created.yml | 9 +++++++++ .../ee/registrations_controller_spec.rb | 18 ++++++++++++++++-- 4 files changed, 33 insertions(+), 14 deletions(-) create mode 100644 ee/config/audit_events/types/registration_created.yml diff --git a/ee/app/controllers/ee/omniauth_callbacks_controller.rb b/ee/app/controllers/ee/omniauth_callbacks_controller.rb index c3cb27f88d1684..b0cba893a5acc6 100644 --- a/ee/app/controllers/ee/omniauth_callbacks_controller.rb +++ b/ee/app/controllers/ee/omniauth_callbacks_controller.rb @@ -17,12 +17,6 @@ def openid_connect override :log_failed_login def log_failed_login(author, provider) - # ::AuditEventService.new( - # author, - # with: provider - # nil, - # ).for_failed_login.unauth_security_event - unauth_author = ::Gitlab::Audit::UnauthenticatedAuthor.new(name: author) user = ::User.new(id: unauth_author.id, name: author) ::Gitlab::Audit::Auditor.audit({ diff --git a/ee/app/controllers/ee/registrations_controller.rb b/ee/app/controllers/ee/registrations_controller.rb index 77b1006135e361..7fb339986acbc4 100644 --- a/ee/app/controllers/ee/registrations_controller.rb +++ b/ee/app/controllers/ee/registrations_controller.rb @@ -87,12 +87,14 @@ def ensure_can_remove_self def log_audit_event(user) return unless user&.persisted? - ::AuditEventService.new( - user, - user, - action: :custom, - custom_message: _('Instance access request') - ).for_user.security_event + ::Gitlab::Audit::Auditor.audit({ + name: "registration_created", + author: user, + scope: user, + target: user, + target_details: user.username, + message: _("Instance access request") + }) end def verify_arkose_labs_token diff --git a/ee/config/audit_events/types/registration_created.yml b/ee/config/audit_events/types/registration_created.yml new file mode 100644 index 00000000000000..39d940b3588472 --- /dev/null +++ b/ee/config/audit_events/types/registration_created.yml @@ -0,0 +1,9 @@ +--- +name: registration_created +description: Event triggered when a user registers for instance access +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114548 +feature_category: compliance_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/spec/controllers/ee/registrations_controller_spec.rb b/ee/spec/controllers/ee/registrations_controller_spec.rb index 71344db706d6de..3a0f796a41dbb3 100644 --- a/ee/spec/controllers/ee/registrations_controller_spec.rb +++ b/ee/spec/controllers/ee/registrations_controller_spec.rb @@ -120,14 +120,28 @@ end it 'logs the audit event info', :aggregate_failures do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "registration_created" + })).and_call_original + subject created_user = User.find_by(email: new_user_email) audit_event = AuditEvent.where(author_id: created_user.id).last + expect(audit_event.entity).to eq(created_user) + expect(audit_event.author).to eq(created_user) expect(audit_event.ip_address).to eq(created_user.current_sign_in_ip) - expect(audit_event.details[:target_details]).to eq(created_user.username) - expect(audit_event.details[:custom_message]).to eq('Instance access request') + expect(audit_event.attributes).to include({ + "target_details" => created_user.username, + "target_id" => created_user.id, + "target_type" => "User", + "entity_path" => created_user.full_path + }) + expect(audit_event.details).to include({ + target_details: created_user.username, + custom_message: "Instance access request" + }) end context 'with invalid user' do -- GitLab From b48994359b3878dc243a365ccc41522492718d91 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 1 May 2023 13:19:45 -0500 Subject: [PATCH 5/5] Set User ID in PasswordsController auditing --- ee/app/controllers/ee/passwords_controller.rb | 2 +- ee/spec/controllers/passwords_controller_spec.rb | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ee/app/controllers/ee/passwords_controller.rb b/ee/app/controllers/ee/passwords_controller.rb index e864dac64c27c7..c531ea4ba6da91 100644 --- a/ee/app/controllers/ee/passwords_controller.rb +++ b/ee/app/controllers/ee/passwords_controller.rb @@ -15,7 +15,7 @@ def log_audit_event name: "password_reset_requested", author: ::Gitlab::Audit::UnauthenticatedAuthor.new, scope: resource, - target: ::User.new, + target: resource || ::User.new, target_details: resource_params[:email], message: "Ask for password reset", ip_address: request.remote_ip diff --git a/ee/spec/controllers/passwords_controller_spec.rb b/ee/spec/controllers/passwords_controller_spec.rb index 36c12ad08a4cd1..d4a4a338ce4834 100644 --- a/ee/spec/controllers/passwords_controller_spec.rb +++ b/ee/spec/controllers/passwords_controller_spec.rb @@ -13,7 +13,7 @@ subject { post :create, params: { user: { email: user.email } } } - it do + it "generates audit events" do expect { subject }.to change { AuditEvent.count }.by(1) audit_event = AuditEvent.last @@ -23,7 +23,8 @@ "entity_path" => nil, "author_name" => "An unauthenticated user", "target_type" => "User", - "target_details" => user.email + "target_details" => user.email, + "target_id" => user.id }) expect(audit_event.details).to include({ custom_message: "Ask for password reset", -- GitLab