From a6cbaeadfc5cc07b4dfae9010f43af8bab05e280 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Thu, 21 Aug 2025 15:20:29 +1200 Subject: [PATCH 1/3] Move membership update audit event to Auditor EE: true Changelog: changed --- doc/user/compliance/audit_event_types.md | 1 + .../types/group_saml_member_added.yml | 10 +++ .../auth/group_saml/membership_updater.rb | 19 +++-- .../group_saml/membership_updater_spec.rb | 70 ++++++++++++++++--- 4 files changed, 84 insertions(+), 16 deletions(-) create mode 100644 ee/config/audit_events/types/group_saml_member_added.yml diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 9123907098a483..34a17e5824a5e2 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -186,6 +186,7 @@ Audit event types belong to the following product categories. | [`destroyed_compliance_requirement_control`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/177878) | A compliance requirement control is destroyed. | {{< icon name="check-circle" >}} Yes | GitLab [17.9](https://gitlab.com/gitlab-org/gitlab/-/issues/512381) | Group | | [`external_status_check_name_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106095) | The name of an external status check is updated | {{< icon name="check-circle" >}} Yes | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/369333) | Project | | [`external_status_check_url_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84624) | The URL that is used for external status checks for a pipeline is updated | {{< icon name="check-circle" >}} Yes | GitLab [15.0](https://gitlab.com/gitlab-org/gitlab/-/issues/355805) | Project | +| [`group_saml_member_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/202160) | A user was added as a SAML group member | {{< icon name="check-circle" >}} Yes | GitLab [18.4](https://gitlab.com/gitlab-org/gitlab/-/issues/560237) | Group | | [`group_saml_provider_create`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111227) | A group SAML provider is created | {{< icon name="check-circle" >}} Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373964) | Group | | [`group_saml_provider_update`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111227) | A group SAML provider is updated | {{< icon name="check-circle" >}} Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373964) | Group | | [`inactive_project_scheduled_for_deletion`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130699) | An inactive project is scheduled for deletion | {{< icon name="check-circle" >}} Yes | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/423263) | Project | diff --git a/ee/config/audit_events/types/group_saml_member_added.yml b/ee/config/audit_events/types/group_saml_member_added.yml new file mode 100644 index 00000000000000..b6b177b781c59c --- /dev/null +++ b/ee/config/audit_events/types/group_saml_member_added.yml @@ -0,0 +1,10 @@ +--- +name: group_saml_member_added +description: A user was added as a SAML group member +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/560237 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/202160 +feature_category: compliance_management +milestone: '18.4' +saved_to_database: true +streamed: true +scope: [Group] diff --git a/ee/lib/gitlab/auth/group_saml/membership_updater.rb b/ee/lib/gitlab/auth/group_saml/membership_updater.rb index 38a97439d465ac..aa62557deb59d4 100644 --- a/ee/lib/gitlab/auth/group_saml/membership_updater.rb +++ b/ee/lib/gitlab/auth/group_saml/membership_updater.rb @@ -90,11 +90,20 @@ def group_names_from_saml end def log_audit_event(member:) - ::AuditEventService.new( - user, - member.source, - action: :create - ).for_member(member).security_event + audit_context = { + name: 'group_saml_member_added', + author: user, + scope: group, + target: user, + message: "Added as SAML group member", + additional_details: { + add: 'user_access', + as: member.human_access, + custom_message: "User #{user.name} added to group #{group.name} through SAML authentication" + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) end def microsoft_group_sync_available? diff --git a/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb index 4e9d2fa4363ecd..349431d863350b 100644 --- a/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb +++ b/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb @@ -68,23 +68,71 @@ expect(group.members.pluck(:access_level)).to eq([Gitlab::Access::MAINTAINER]) end - context "for allowed_email_domain restrictions" do - before do - stub_licensed_features(group_allowed_email_domains: true) + context 'when audit events triggered' do + it 'sends the audit streaming event' do + audit_context = { + name: 'group_saml_member_added', + author: user, + scope: group, + target: user, + message: 'Added as SAML group member', + additional_details: { + add: 'user_access', + as: 'Developer', + custom_message: "User #{user.name} added to group #{group.name} through SAML authentication" + } + } + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + + update_membership + end + + it 'does not send audit event when member is not persisted' do + allow(group).to receive(:add_member).and_return(nil) + + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + update_membership end - it "does not log an audit event when domain restrictions are in place" do - create :allowed_email_domain, group: group, domain: 'somethingveryrandom.com' + it 'does not send audit event when user is already a member' do + group.add_guest(user) - expect { subject }.not_to change { AuditEvent.by_entity('Group', group).count } + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + update_membership end - it "logs an audit event if the member's email is accepted" do - expect do - subject - end.to change { AuditEvent.by_entity('Group', group).count }.by(1) + context "for allowed_email_domain restrictions" do + before do + stub_licensed_features(group_allowed_email_domains: true) + end + + it "does not log an audit event when domain restrictions are in place" do + create :allowed_email_domain, group: group, domain: 'somethingveryrandom.com' + + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + update_membership + end + + it "logs an audit event if the member's email is accepted" do + audit_context = { + name: 'group_saml_member_added', + author: user, + scope: group, + target: user, + message: 'Added as SAML group member', + additional_details: { + add: 'user_access', + as: 'Developer', + custom_message: "User #{user.name} added to group #{group.name} through SAML authentication" + } + } + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) - expect(AuditEvent.last.details).to include(add: 'user_access', target_details: user.name, as: 'Developer') + update_membership + end end end -- GitLab From 95ea2dab7127688d40a97fa44722a0b86bf06805 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Thu, 28 Aug 2025 11:10:29 +1200 Subject: [PATCH 2/3] Extract duplicated audit_context --- .../group_saml/membership_updater_spec.rb | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb index 349431d863350b..f614854bbdb4d8 100644 --- a/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb +++ b/ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb @@ -69,8 +69,8 @@ end context 'when audit events triggered' do - it 'sends the audit streaming event' do - audit_context = { + let!(:expected_audit_context) do + { name: 'group_saml_member_added', author: user, scope: group, @@ -82,7 +82,10 @@ custom_message: "User #{user.name} added to group #{group.name} through SAML authentication" } } - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + end + + it 'sends the audit streaming event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_audit_context) update_membership end @@ -117,19 +120,7 @@ end it "logs an audit event if the member's email is accepted" do - audit_context = { - name: 'group_saml_member_added', - author: user, - scope: group, - target: user, - message: 'Added as SAML group member', - additional_details: { - add: 'user_access', - as: 'Developer', - custom_message: "User #{user.name} added to group #{group.name} through SAML authentication" - } - } - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_audit_context) update_membership end -- GitLab From 7c25949d7ee7876798fece7815baa0c8dede6dcd Mon Sep 17 00:00:00 2001 From: nrosandich Date: Thu, 28 Aug 2025 16:13:12 +1200 Subject: [PATCH 3/3] Fix failing test with multi event now --- .../omniauth_callbacks_controller_spec.rb | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb index d87d9566d88ec5..2ec384160d7a58 100644 --- a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb +++ b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb @@ -194,6 +194,17 @@ def stub_last_request_id(id) } ).and_call_original + unless group.member?(user) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including( + name: 'group_saml_member_added', + message: 'Added as SAML group member', + target: user, + scope: group + ) + ).and_call_original + end + expect { post provider, params: { group_id: group } }.to change { AuthenticationEvent.count }.by(1) end @@ -368,11 +379,20 @@ def stub_last_request_id(id) end it 'logs group audit event for being added to the group' do - audit_event_service = instance_double(AuditEventService) - - expect(AuditEventService).to receive(:new).ordered.with(user, group, action: :create) - .and_return(audit_event_service) - expect(audit_event_service).to receive_message_chain(:for_member, :security_event) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including( + name: 'authenticated_with_group_saml' + ) + ).ordered.and_call_original + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including( + name: 'group_saml_member_added', + message: 'Added as SAML group member', + target: user, + scope: group + ) + ).ordered.and_call_original post provider, params: { group_id: group } end -- GitLab