From 9c68cffc9e738448a02f317037666a116b09bb4b Mon Sep 17 00:00:00 2001 From: Asmaa Hassan Date: Wed, 11 Dec 2024 17:25:45 +0200 Subject: [PATCH 1/8] Add audit event type for scim provisioned users --- doc/user/compliance/audit_event_types.md | 1 + .../audit_events/types/user_provisioned_by_scim.yml | 10 ++++++++++ 2 files changed, 11 insertions(+) create mode 100644 ee/config/audit_events/types/user_provisioned_by_scim.yml diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index fb1f1479292fa4..3083473d4fc30a 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -604,6 +604,7 @@ Audit event types belong to the following product categories. | [`user_email_changed_and_user_signed_in`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106090) | User email changed and user signed in | **{check-circle}** Yes | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/369331) | User | | [`user_impersonation`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79340) | An instance administrator starts or stops impersonating a user | **{check-circle}** Yes | GitLab [14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/300961) | User, Group | | [`user_password_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106086) | A user password is updated | **{check-circle}** Yes | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/369330) | User | +| [`user_provisioned_by_scim`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174040) | A user is provisioned by SCIM | **{check-circle}** Yes | GitLab [17.8](https://gitlab.com/gitlab-org/gitlab/-/issues/423322) | User, Group | | [`user_rejected`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784) | A user registration is rejected | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | User | ### User profile diff --git a/ee/config/audit_events/types/user_provisioned_by_scim.yml b/ee/config/audit_events/types/user_provisioned_by_scim.yml new file mode 100644 index 00000000000000..0e7daeebe6deae --- /dev/null +++ b/ee/config/audit_events/types/user_provisioned_by_scim.yml @@ -0,0 +1,10 @@ +--- +name: user_provisioned_by_scim +description: A user is provisioned by SCIM +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/423322 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174040 +feature_category: user_management +milestone: '17.8' +saved_to_database: true +streamed: true +scope: [User, Group] -- GitLab From a36c4e9baedc1f18d425c07d30eca6cd813b3043 Mon Sep 17 00:00:00 2001 From: Asmaa Hassan Date: Wed, 11 Dec 2024 18:14:34 +0200 Subject: [PATCH 2/8] Create the audit log function --- .../audit_events/types/user_provisioned_by_scim.yml | 2 +- ee/lib/ee/gitlab/scim/group/provisioning_service.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/ee/config/audit_events/types/user_provisioned_by_scim.yml b/ee/config/audit_events/types/user_provisioned_by_scim.yml index 0e7daeebe6deae..d96211b058c5a8 100644 --- a/ee/config/audit_events/types/user_provisioned_by_scim.yml +++ b/ee/config/audit_events/types/user_provisioned_by_scim.yml @@ -7,4 +7,4 @@ feature_category: user_management milestone: '17.8' saved_to_database: true streamed: true -scope: [User, Group] +scope: [Group] diff --git a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb index fa8fdfa9cc1d65..1ca87cfa3c7dc4 100644 --- a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb +++ b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb @@ -106,6 +106,19 @@ def existing_user? def success_response ProvisioningResponse.new(status: :success, identity: identity) + log_audit_event(user, root_group) + end + + def log_audit_event(user, root_group) + audit_context = { + name: "user_provisioned_by_scim", + author: user, + scope: root_group, + target: user, + target_details: user.username, + message: "User was provisioned by SCIM" + } + ::Gitlab::Audit::Auditor.audit(audit_context) end end end -- GitLab From 6102114e14cf026b69f02caef125b8d97848bf8e Mon Sep 17 00:00:00 2001 From: Asmaa Hassan Date: Wed, 11 Dec 2024 18:18:30 +0200 Subject: [PATCH 3/8] Update Audit event types doc page --- doc/user/compliance/audit_event_types.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 3083473d4fc30a..2601ae552784f6 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -604,7 +604,7 @@ Audit event types belong to the following product categories. | [`user_email_changed_and_user_signed_in`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106090) | User email changed and user signed in | **{check-circle}** Yes | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/369331) | User | | [`user_impersonation`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79340) | An instance administrator starts or stops impersonating a user | **{check-circle}** Yes | GitLab [14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/300961) | User, Group | | [`user_password_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106086) | A user password is updated | **{check-circle}** Yes | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/369330) | User | -| [`user_provisioned_by_scim`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174040) | A user is provisioned by SCIM | **{check-circle}** Yes | GitLab [17.8](https://gitlab.com/gitlab-org/gitlab/-/issues/423322) | User, Group | +| [`user_provisioned_by_scim`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174040) | A user is provisioned by SCIM | **{check-circle}** Yes | GitLab [17.8](https://gitlab.com/gitlab-org/gitlab/-/issues/423322) | Group | | [`user_rejected`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784) | A user registration is rejected | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | User | ### User profile -- GitLab From 389764ef03c8bfaaeef2161acfdd76607492105c Mon Sep 17 00:00:00 2001 From: Asmaa Hassan Date: Fri, 13 Dec 2024 01:07:08 +0200 Subject: [PATCH 4/8] Fix logging --- ee/lib/ee/gitlab/scim/group/provisioning_service.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb index 1ca87cfa3c7dc4..190821b0845429 100644 --- a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb +++ b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb @@ -106,14 +106,14 @@ def existing_user? def success_response ProvisioningResponse.new(status: :success, identity: identity) - log_audit_event(user, root_group) + log_audit_event(user) end - def log_audit_event(user, root_group) + def log_audit_event(user) audit_context = { name: "user_provisioned_by_scim", author: user, - scope: root_group, + scope: @group, target: user, target_details: user.username, message: "User was provisioned by SCIM" -- GitLab From 0ed7254a7651e4aab85f5d10acd6513732762574 Mon Sep 17 00:00:00 2001 From: Asmaa Hassan Date: Sun, 15 Dec 2024 16:45:24 +0200 Subject: [PATCH 5/8] Call log withs success_response --- .../gitlab/scim/group/provisioning_service.rb | 22 ++++++++++---- .../scim/group/provisioning_service_spec.rb | 29 +++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb index 190821b0845429..338a54f7755cd8 100644 --- a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb +++ b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb @@ -7,7 +7,11 @@ module Group class ProvisioningService < BaseProvisioningService def execute return error_response(errors: ["Missing params: #{missing_params}"]) unless missing_params.empty? - return success_response if existing_identity_and_member? + + if existing_identity_and_member? + log_audit_event(user) + return success_response + end clear_memoization(:identity) @@ -24,7 +28,10 @@ def execute private def create_identity - return success_response if identity.save + if identity.save + log_audit_event(user) + return success_response + end error_response(objects: [identity]) end @@ -77,13 +84,19 @@ def group_user_params end def create_identity_and_member - return success_response if member.valid? && identity.save + if member.valid? && identity.save + log_audit_event(user) + return success_response + end error_response(objects: [identity, member]) end def create_user_and_member - return success_response if user.save && member.errors.empty? + if user.save && member.errors.empty? + log_audit_event(user) + return success_response + end error_response(objects: [user, identity, member]) end @@ -106,7 +119,6 @@ def existing_user? def success_response ProvisioningResponse.new(status: :success, identity: identity) - log_audit_event(user) end def log_audit_event(user) diff --git a/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb b/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb index 7d5291355db94f..c6732593262af7 100644 --- a/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb +++ b/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb @@ -175,6 +175,35 @@ def user end end + context 'when user provisoning succeeds' do + before do + stub_licensed_features(extended_audit_events: true) + end + + it 'contains a success status' do + expect(service.execute.status).to eq(:success) + end + + it 'logs an audit event' do + expect { described_class.new(group).success_response }.to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last).to have_attributes({ + attributes: hash_including({ + "entity_id" => group.id, + "entity_type" => "Group", + "author_id" => user.id, + "target_details" => user.username, + "target_id" => user.id + }), + details: hash_including({ + custom_message: "User was provision by SCIM", + target_type: "User", + target_details: user.username + }) + }) + end + end + context 'when a provisioning error occurs' do let(:result) { StandardError.new("testing error") } let(:log_params) do -- GitLab From 12dae478779b16ae33d97b45302292d543ef88c7 Mon Sep 17 00:00:00 2001 From: Asmaa Hassan Date: Tue, 17 Dec 2024 18:52:32 +0200 Subject: [PATCH 6/8] Trying to fix the provisoning service --- ee/lib/ee/gitlab/scim/group/provisioning_service.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb index 338a54f7755cd8..4d8b0466ba6786 100644 --- a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb +++ b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb @@ -5,6 +5,13 @@ module Gitlab module Scim module Group class ProvisioningService < BaseProvisioningService + attr_reader :audit_event + + def initialize(status, identity, audit_event) + super(status, identity) + @audit_event = audit_event + end + def execute return error_response(errors: ["Missing params: #{missing_params}"]) unless missing_params.empty? @@ -118,7 +125,7 @@ def existing_user? end def success_response - ProvisioningResponse.new(status: :success, identity: identity) + ProvisioningResponse.new(status: :success, identity: identity, audit_event: audit_event) end def log_audit_event(user) -- GitLab From 51096ac3a396c8c16ef8c513e57699a2a26d5a2f Mon Sep 17 00:00:00 2001 From: Asmaa Hassan Date: Wed, 18 Dec 2024 18:48:25 +0200 Subject: [PATCH 7/8] Fix provisioning service and its rspec test as per peer-review by Hakeem --- .../gitlab/scim/group/provisioning_service.rb | 33 +++---------- .../scim/group/provisioning_service_spec.rb | 46 +++++++++---------- 2 files changed, 28 insertions(+), 51 deletions(-) diff --git a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb index 4d8b0466ba6786..cdbbffcaef8fae 100644 --- a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb +++ b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb @@ -5,20 +5,9 @@ module Gitlab module Scim module Group class ProvisioningService < BaseProvisioningService - attr_reader :audit_event - - def initialize(status, identity, audit_event) - super(status, identity) - @audit_event = audit_event - end - def execute return error_response(errors: ["Missing params: #{missing_params}"]) unless missing_params.empty? - - if existing_identity_and_member? - log_audit_event(user) - return success_response - end + return success_response if existing_identity_and_member? clear_memoization(:identity) @@ -35,10 +24,7 @@ def execute private def create_identity - if identity.save - log_audit_event(user) - return success_response - end + return success_response if identity.save error_response(objects: [identity]) end @@ -91,19 +77,13 @@ def group_user_params end def create_identity_and_member - if member.valid? && identity.save - log_audit_event(user) - return success_response - end + return success_response if member.valid? && identity.save error_response(objects: [identity, member]) end def create_user_and_member - if user.save && member.errors.empty? - log_audit_event(user) - return success_response - end + return success_response if user.save && member.errors.empty? error_response(objects: [user, identity, member]) end @@ -125,10 +105,11 @@ def existing_user? end def success_response - ProvisioningResponse.new(status: :success, identity: identity, audit_event: audit_event) + log_audit_event + ProvisioningResponse.new(status: :success, identity: identity) end - def log_audit_event(user) + def log_audit_event audit_context = { name: "user_provisioned_by_scim", author: user, diff --git a/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb b/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb index c6732593262af7..836c04b22b4d73 100644 --- a/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb +++ b/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb @@ -173,34 +173,30 @@ def user end end end - end - context 'when user provisoning succeeds' do - before do - stub_licensed_features(extended_audit_events: true) - end - - it 'contains a success status' do - expect(service.execute.status).to eq(:success) - end + context 'when user provisoning succeeds' do + before do + stub_licensed_features(extended_audit_events: true) + end - it 'logs an audit event' do - expect { described_class.new(group).success_response }.to change { AuditEvent.count }.by(1) - - expect(AuditEvent.last).to have_attributes({ - attributes: hash_including({ - "entity_id" => group.id, - "entity_type" => "Group", - "author_id" => user.id, - "target_details" => user.username, - "target_id" => user.id - }), - details: hash_including({ - custom_message: "User was provision by SCIM", - target_type: "User", - target_details: user.username + it 'logs an audit event' do + expect { service.execute }.to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last).to have_attributes({ + attributes: hash_including({ + "entity_id" => group.id, + "entity_type" => "Group", + "author_id" => user.id, + "target_details" => user.username, + "target_id" => user.id + }), + details: hash_including({ + custom_message: "User was provisioned by SCIM", + target_type: "User", + target_details: user.username + }) }) - }) + end end end -- GitLab From d2b05f0dc43f2daceaa29f575db3535885b3e0e1 Mon Sep 17 00:00:00 2001 From: Asmaa Hassan Date: Wed, 1 Jan 2025 20:12:14 +0200 Subject: [PATCH 8/8] Add suggestions by reviwer; Bogdan, to ensure only newly SCIM provisioned users are logged --- .../gitlab/scim/group/provisioning_service.rb | 8 +++++--- .../scim/group/provisioning_service_spec.rb | 19 ++++++++++++++++--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb index cdbbffcaef8fae..cdeb07e0800b3e 100644 --- a/ee/lib/ee/gitlab/scim/group/provisioning_service.rb +++ b/ee/lib/ee/gitlab/scim/group/provisioning_service.rb @@ -83,7 +83,10 @@ def create_identity_and_member end def create_user_and_member - return success_response if user.save && member.errors.empty? + if user.save && member.errors.empty? + log_audit_event + return success_response + end error_response(objects: [user, identity, member]) end @@ -105,14 +108,13 @@ def existing_user? end def success_response - log_audit_event ProvisioningResponse.new(status: :success, identity: identity) end def log_audit_event audit_context = { name: "user_provisioned_by_scim", - author: user, + author: ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)'), scope: @group, target: user, target_details: user.username, diff --git a/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb b/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb index 836c04b22b4d73..06f1eb3f340b8a 100644 --- a/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb +++ b/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb @@ -51,6 +51,14 @@ it 'does not create the SAML identity' do expect { service.execute }.not_to change { Identity.count } end + + it 'does not log user_provisioned_by_scim audit event' do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including({ + name: "user_provisioned_by_scim" + })).and_call_original + + service.execute + end end context 'when valid params' do @@ -174,23 +182,28 @@ def user end end - context 'when user provisoning succeeds' do + context 'for audit' do + let(:author) { ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)') } + before do stub_licensed_features(extended_audit_events: true) end - it 'logs an audit event' do + it 'logs user_provisioned_by_scim audit event' do expect { service.execute }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last).to have_attributes({ attributes: hash_including({ "entity_id" => group.id, "entity_type" => "Group", - "author_id" => user.id, + "author_id" => author.id, "target_details" => user.username, "target_id" => user.id }), details: hash_including({ + event_name: "user_provisioned_by_scim", + author_class: author.class.to_s, + author_name: author.name, custom_message: "User was provisioned by SCIM", target_type: "User", target_details: user.username -- GitLab