diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 9123907098a4836cf5592e2c1de7f94bf01fc6aa..34a17e5824a5e2aae01fa3ec8288b0f2391e715c 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 0000000000000000000000000000000000000000..b6b177b781c59c4365fc533ab2a61e0d7b658fca --- /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 38a97439d465acb634bde2603d47099f77411dea..aa62557deb59d4f26f91a08428e23dcb3ef6f5e5 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/controllers/groups/omniauth_callbacks_controller_spec.rb b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb index d87d9566d88ec5a91561f23ac50331844853d074..2ec384160d7a58614ac8b30e68c4adeae8447940 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 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 4e9d2fa4363ecd235c4b9b00040d4a8f9e8d2607..f614854bbdb4d8c781287d612c16b845ead375dc 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,62 @@ 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 + let!(:expected_audit_context) do + { + 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" + } + } + end + + it 'sends the audit streaming event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_audit_context) + + 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 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 send audit event when user is already a member' do + group.add_guest(user) + + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) - expect { subject }.not_to change { AuditEvent.by_entity('Group', group).count } + 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) - expect(AuditEvent.last.details).to include(add: 'user_access', target_details: user.name, as: 'Developer') + update_membership + end + + it "logs an audit event if the member's email is accepted" do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_audit_context) + + update_membership + end end end