From 3b0c6c630700561660b956cfc12fc550fd72e425 Mon Sep 17 00:00:00 2001 From: Sam Figueroa Date: Mon, 14 Aug 2023 16:16:44 +0200 Subject: [PATCH 1/2] Audit delivery of confirmation emails - Adds audit events when an email confirmation is requested by the Emails::Confirm service or an email was changed on the User model. - Refs: https://gitlab.com/gitlab-org/gitlab/-/issues/377625 Changelog: added EE: true --- .../audit_event_types.md | 1 + ee/app/models/ee/user.rb | 18 ++++++++++++++ ee/app/services/ee/emails/base_service.rb | 10 ++++++++ ee/app/services/ee/emails/confirm_service.rb | 15 ++++++++++++ .../types/email_confirmation_sent.yml | 10 ++++++++ ee/spec/models/ee/user_spec.rb | 24 +++++++++++++++++++ .../services/emails/confirm_service_spec.rb | 24 +++++++++++++++++++ 7 files changed, 102 insertions(+) create mode 100644 ee/app/services/ee/emails/confirm_service.rb create mode 100644 ee/config/audit_events/types/email_confirmation_sent.yml create mode 100644 ee/spec/services/emails/confirm_service_spec.rb diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md index 7bb1f8b8242989..9b8ee55c7f913b 100644 --- a/doc/administration/audit_event_streaming/audit_event_types.md +++ b/doc/administration/audit_event_streaming/audit_event_types.md @@ -88,6 +88,7 @@ Every audit event is associated with an event type. The association with the eve | [`destroy_compliance_framework`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74292) | Triggered on successful compliance framework deletion | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [14.6](https://gitlab.com/gitlab-org/gitlab/-/issues/340649) | | [`destroy_event_streaming_destination`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74632) | Event triggered when an external audit event destination is deleted | **{check-circle}** Yes | **{check-circle}** Yes | `audit_events` | GitLab [14.6](https://gitlab.com/gitlab-org/gitlab/-/issues/344664) | | [`destroy_instance_event_streaming_destination`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125846) | Event triggered when an instance level external audit event destination is deleted | **{check-circle}** Yes | **{check-circle}** Yes | `audit_events` | GitLab [16.2](https://gitlab.com/gitlab-org/gitlab/-/issues/404730) | +| [`email_confirmation_sent`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129261) | Triggered when users add or change and email address and it needs to be confirmed. | **{dotted-circle}** No | **{check-circle}** Yes | `user_profile` | GitLab [16.3](https://gitlab.com/gitlab-org/gitlab/-/issues/377625) | | [`email_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114546) | Event triggered when an email is created | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | | [`email_destroyed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114546) | Event triggered when an email is destroyed | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | | [`environment_protected`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108247) | This event is triggered when a protected environment is created. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/216164) | diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index b840b73570a774..19cc1fef12650a 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -635,6 +635,24 @@ def password_required?(*) super end + # override, from Devise::Confirmable + def send_confirmation_instructions + super + + ::Gitlab::Audit::Auditor.audit({ + name: 'email_confirmation_sent', + author: self, + scope: self, + message: "Confirmation instructions sent to: #{unconfirmed_email}", + target: self, + additional_details: { + target_type: "Email", + current_email: email, + unconfirmed_email: unconfirmed_email + } + }) + end + private def gitlab_com_member? diff --git a/ee/app/services/ee/emails/base_service.rb b/ee/app/services/ee/emails/base_service.rb index d978054e87f273..42bcaaa763e49d 100644 --- a/ee/app/services/ee/emails/base_service.rb +++ b/ee/app/services/ee/emails/base_service.rb @@ -19,6 +19,16 @@ def log_audit_event(options = {}) message: "Email destroyed", additional_details: { remove: "email" } } + when :confirm + { + name: "email_confirmation_sent", + message: "Confirmation instructions sent to: #{options[:unconfirmed_email]}", + additional_details: { + current_email: @current_user.email, + target_type: "Email", + unconfirmed_email: options[:unconfirmed_email] + } + } end ::Gitlab::Audit::Auditor.audit(audit_context.deep_merge({ diff --git a/ee/app/services/ee/emails/confirm_service.rb b/ee/app/services/ee/emails/confirm_service.rb new file mode 100644 index 00000000000000..c6071800ad6b33 --- /dev/null +++ b/ee/app/services/ee/emails/confirm_service.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module EE + module Emails + module ConfirmService + extend ::Gitlab::Utils::Override + + override :execute + def execute(email) + super + log_audit_event(action: :confirm, target: email, unconfirmed_email: email.email) + end + end + end +end diff --git a/ee/config/audit_events/types/email_confirmation_sent.yml b/ee/config/audit_events/types/email_confirmation_sent.yml new file mode 100644 index 00000000000000..afe028c3c1ff13 --- /dev/null +++ b/ee/config/audit_events/types/email_confirmation_sent.yml @@ -0,0 +1,10 @@ +--- +name: email_confirmation_sent +description: Triggered when users add or change and email address and it needs to + be confirmed. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/377625 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129261 +feature_category: user_profile +milestone: '16.3' +saved_to_database: false +streamed: true diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index b0760bb3cce351..e3e9e615e37ec4 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -3155,4 +3155,28 @@ expect(subject).to eql(details_hash) end end + + describe 'audits' do + context 'audit events' do + it 'audits the confirmation request' do + user = create :user + unconfirmed_email = 'first-unconfirmed-email@example.com' + + expect(::Gitlab::Audit::Auditor).to(receive(:audit).with(hash_including({ + author: user, + scope: user, + target: user, + name: 'email_confirmation_sent', + message: "Confirmation instructions sent to: #{unconfirmed_email}", + additional_details: hash_including({ + current_email: user.email, + target_type: 'Email', + unconfirmed_email: unconfirmed_email + }) + })).and_call_original) + + user.update!(email: unconfirmed_email) + end + end + end end diff --git a/ee/spec/services/emails/confirm_service_spec.rb b/ee/spec/services/emails/confirm_service_spec.rb new file mode 100644 index 00000000000000..96b8ee81da3e25 --- /dev/null +++ b/ee/spec/services/emails/confirm_service_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Emails::ConfirmService, feature_category: :user_management do + describe '#execute' do + it 'records an audit event when confirmation is sent' do + user = create(:user) + unconfirmed_email = 'foobar@gitlab.com' + + expect(::Gitlab::Audit::Auditor).to(receive(:audit).with(hash_including({ + name: 'email_confirmation_sent', + message: 'Confirmation instructions sent to: foobar@gitlab.com', + additional_details: hash_including({ + current_email: user.email, + target_type: 'Email', + unconfirmed_email: unconfirmed_email + }) + })).and_call_original) + + described_class.new(user).execute(user.emails.create!(email: unconfirmed_email)) + end + end +end -- GitLab From 1b9b686382ff939794c02efb65f2624ac50ae3dd Mon Sep 17 00:00:00 2001 From: Sam Figueroa Date: Thu, 17 Aug 2023 11:52:45 +0200 Subject: [PATCH 2/2] Fix failing specs - The User create spe for ee needed to have the shared examples internalized because they needed more customization than was feasible with some elusive `let` magic. - Previous 'when audit is not required' spec always passed because the license feature was not active. --- ee/app/services/ee/emails/confirm_service.rb | 5 +- .../types/email_confirmation_sent.yml | 3 +- .../services/ee/users/create_service_spec.rb | 74 +++++++++++++------ 3 files changed, 57 insertions(+), 25 deletions(-) diff --git a/ee/app/services/ee/emails/confirm_service.rb b/ee/app/services/ee/emails/confirm_service.rb index c6071800ad6b33..1cd888c2b0efe8 100644 --- a/ee/app/services/ee/emails/confirm_service.rb +++ b/ee/app/services/ee/emails/confirm_service.rb @@ -7,8 +7,9 @@ module ConfirmService override :execute def execute(email) - super - log_audit_event(action: :confirm, target: email, unconfirmed_email: email.email) + super.tap do + log_audit_event(action: :confirm, target: email, unconfirmed_email: email.email) + end end end end diff --git a/ee/config/audit_events/types/email_confirmation_sent.yml b/ee/config/audit_events/types/email_confirmation_sent.yml index afe028c3c1ff13..ee8c0f8f3f790f 100644 --- a/ee/config/audit_events/types/email_confirmation_sent.yml +++ b/ee/config/audit_events/types/email_confirmation_sent.yml @@ -1,7 +1,6 @@ --- name: email_confirmation_sent -description: Triggered when users add or change and email address and it needs to - be confirmed. +description: Triggered when users add or change and email address and it needs to be confirmed. introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/377625 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129261 feature_category: user_profile diff --git a/ee/spec/services/ee/users/create_service_spec.rb b/ee/spec/services/ee/users/create_service_spec.rb index 1b0bd062f91c64..0bf693f8631d37 100644 --- a/ee/spec/services/ee/users/create_service_spec.rb +++ b/ee/spec/services/ee/users/create_service_spec.rb @@ -17,39 +17,55 @@ subject(:service) { described_class.new(current_user, params) } describe '#execute' do - let(:operation) { service.execute } + context "when licenesed" do + before do + stub_licensed_features(extended_audit_events: true) + end - context 'audit events' do - include_examples 'audit event logging' do - let(:audit_event_name) { 'user_created' } + context 'audit events' do + it 'logs the audit event info' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including( + name: 'user_created' + )).and_call_original - let(:fail_condition!) do - expect_any_instance_of(User) - .to receive(:save).and_return(false) - end + # user creation will also send confirmaiton instructions which is also audited + allow(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including(name: 'email_confirmation_sent')) - let(:attributes) do - { + user = service.execute + + expect(AuditEvent.last).to have_attributes( author_id: current_user.id, - entity_id: @resource.id, + entity_id: user.id, entity_type: 'User', details: { add: 'user', author_class: 'User', author_name: current_user.name, - custom_message: "User #{@resource.username} created", - target_id: @resource.id, + custom_message: "User #{user.username} created", + target_id: user.id, target_type: 'User', - target_details: @resource.full_path, + target_details: user.full_path, registration_details: { - id: @resource.id, - name: @resource.name, - username: @resource.username, - email: @resource.email, - access_level: @resource.access_level + id: user.id, + name: user.name, + username: user.username, + email: user.email, + access_level: user.access_level } } - } + ) + end + + it 'does not log audit event if operation fails' do + expect_any_instance_of(User).to receive(:save).and_return(false) + + expect { service.execute }.not_to change { AuditEvent.count } + end + + it 'does not log audit event if operation results in no change' do + service.execute + + expect { service.execute }.not_to change(AuditEvent, :count) end end @@ -57,9 +73,25 @@ let(:current_user) { nil } it 'does not log any audit event' do - expect { operation }.not_to change(AuditEvent, :count) + allow(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including(name: 'email_created')) + + expect { service.execute }.not_to change(AuditEvent, :count) end end end end + + context 'when not licensed' do + before do + stub_licensed_features( + admin_audit_log: false, + audit_events: false, + extended_audit_events: false + ) + end + + it 'does not log audit event' do + expect { service.execute }.not_to change(AuditEvent, :count) + end + end end -- GitLab