From 9852c5c7b97406749644f404927685724ee0ed6f Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 14 Feb 2023 01:21:34 -0600 Subject: [PATCH 1/9] Add audit event type for member_created --- ee/app/services/ee/members/create_service.rb | 27 +++++++++++++++---- .../audit_events/types/member_created.yml | 9 +++++++ 2 files changed, 31 insertions(+), 5 deletions(-) create mode 100644 ee/config/audit_events/types/member_created.yml diff --git a/ee/app/services/ee/members/create_service.rb b/ee/app/services/ee/members/create_service.rb index fb209e6c3526a6..c1eb75a669d48c 100644 --- a/ee/app/services/ee/members/create_service.rb +++ b/ee/app/services/ee/members/create_service.rb @@ -44,11 +44,28 @@ def after_execute(member:) end def log_audit_event(member:) - ::AuditEventService.new( - current_user, - member.source, - action: :create - ).for_member(member).security_event + audit_context = { + name: 'member_created', + author: current_user, + scope: member.source, + target_id: member.user_id, + target_type: 'User', + target_details: member.user ? member.user.name : 'Deleted User', + message: 'Membership created', + additional_details: { + add: 'user_access', + as: ::Gitlab::Access.options_with_owner.key(member.access_level.to_i), + member_id: member.id + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + + # ::AuditEventService.new( + # current_user, + # member.source, + # action: :create + # ).for_member(member).security_event end end end diff --git a/ee/config/audit_events/types/member_created.yml b/ee/config/audit_events/types/member_created.yml new file mode 100644 index 00000000000000..06132d317f0a8a --- /dev/null +++ b/ee/config/audit_events/types/member_created.yml @@ -0,0 +1,9 @@ +--- +name: member_created +description: Event triggered when a membership is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374112 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/109711 +feature_category: compliance_management +milestone: '15.9' +saved_to_database: true +streamed: true -- GitLab From d175ebf97a147d8a991d1be89b2e67b261a67e8d Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 14 Feb 2023 11:03:26 -0600 Subject: [PATCH 2/9] Refactor audit events for Members update service --- ee/app/services/ee/members/create_service.rb | 9 +----- ee/app/services/ee/members/update_service.rb | 30 +++++++++++++------ .../audit_events/types/member_updated.yml | 9 ++++++ 3 files changed, 31 insertions(+), 17 deletions(-) create mode 100644 ee/config/audit_events/types/member_updated.yml diff --git a/ee/app/services/ee/members/create_service.rb b/ee/app/services/ee/members/create_service.rb index c1eb75a669d48c..78d5f152142335 100644 --- a/ee/app/services/ee/members/create_service.rb +++ b/ee/app/services/ee/members/create_service.rb @@ -48,8 +48,7 @@ def log_audit_event(member:) name: 'member_created', author: current_user, scope: member.source, - target_id: member.user_id, - target_type: 'User', + target: member.user, target_details: member.user ? member.user.name : 'Deleted User', message: 'Membership created', additional_details: { @@ -60,12 +59,6 @@ def log_audit_event(member:) } ::Gitlab::Audit::Auditor.audit(audit_context) - - # ::AuditEventService.new( - # current_user, - # member.source, - # action: :create - # ).for_member(member).security_event end end end diff --git a/ee/app/services/ee/members/update_service.rb b/ee/app/services/ee/members/update_service.rb index 96096f5466f2eb..33f0c3f4f1347e 100644 --- a/ee/app/services/ee/members/update_service.rb +++ b/ee/app/services/ee/members/update_service.rb @@ -8,7 +8,7 @@ module UpdateService def after_execute(action:, old_access_level:, old_expiry:, member:) super - log_audit_event(action: action, old_access_level: old_access_level, old_expiry: old_expiry, member: member) + log_audit_event(old_access_level: old_access_level, old_expiry: old_expiry, member: member) end private @@ -27,14 +27,26 @@ def update_member(member, permission) super end - def log_audit_event(action:, old_access_level:, old_expiry:, member:) - ::AuditEventService.new( - current_user, - member.source, - action: action, - old_access_level: old_access_level, - old_expiry: old_expiry - ).for_member(member).security_event + def log_audit_event(old_access_level:, old_expiry:, member:) + audit_context = { + name: 'member_updated', + author: current_user, + scope: member.source, + target: member.user, + target_details: member.user ? member.user.name : 'Deleted User', + message: 'Membership updated', + additional_details: { + change: 'access_level', + from: old_access_level, + to: member.human_access, + expiry_from: old_expiry, + expiry_to: member.expires_at, + as: ::Gitlab::Access.options_with_owner.key(member.access_level.to_i), + member_id: member.id + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) end end end diff --git a/ee/config/audit_events/types/member_updated.yml b/ee/config/audit_events/types/member_updated.yml new file mode 100644 index 00000000000000..504475c538f3e8 --- /dev/null +++ b/ee/config/audit_events/types/member_updated.yml @@ -0,0 +1,9 @@ +--- +name: member_updated +description: Event triggered when a membership is updated +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374112 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/109711 +feature_category: compliance_management +milestone: '15.9' +saved_to_database: true +streamed: true -- GitLab From 2efaed8e1b57e674f7a5f155535911be5b758648 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 14 Feb 2023 11:53:22 -0600 Subject: [PATCH 3/9] Refactor audit events for Members destroy service --- ee/app/services/ee/members/destroy_service.rb | 35 ++++++++++++++++--- .../audit_events/types/member_destroyed.yml | 9 +++++ 2 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 ee/config/audit_events/types/member_destroyed.yml diff --git a/ee/app/services/ee/members/destroy_service.rb b/ee/app/services/ee/members/destroy_service.rb index b58629f31546ee..4ceee289ec2778 100644 --- a/ee/app/services/ee/members/destroy_service.rb +++ b/ee/app/services/ee/members/destroy_service.rb @@ -29,11 +29,36 @@ def system_event? end def log_audit_event(member:, author:, action:) - ::AuditEventService.new( - author, - member.source, - action: action - ).for_member(member).security_event + audit_context = { + name: 'member_destroyed', + scope: member.source, + target: member.user, + target_details: member.user ? member.user.name : 'Deleted User', + additional_details: { + remove: "user_access", + member_id: member.id + } + } + audit_context[:additional_details].update(system_event: true) if action == :expired + + case action + when :destroy + audit_context.update( + author: author, + message: 'Membership destroyed' + ) + when :expired + audit_context.update( + author: ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)'), + message: "Membership expired on #{member.expires_at}" + ) + audit_context[:additional_details].update( + system_event: true, + reason: "access expired on #{member.expires_at}" + ) + end + + ::Gitlab::Audit::Auditor.audit(audit_context) end def cleanup_group_identity(member) diff --git a/ee/config/audit_events/types/member_destroyed.yml b/ee/config/audit_events/types/member_destroyed.yml new file mode 100644 index 00000000000000..241ddf79af765d --- /dev/null +++ b/ee/config/audit_events/types/member_destroyed.yml @@ -0,0 +1,9 @@ +--- +name: member_destroyed +description: Event triggered when a membership is destroyed +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374112 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/109711 +feature_category: compliance_management +milestone: '15.9' +saved_to_database: true +streamed: true -- GitLab From 7b8010f867be98be8db9037eb2c1820018601727 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 14 Feb 2023 12:31:56 -0600 Subject: [PATCH 4/9] Refactor audit events for Member approve access request service --- .../members/approve_access_request_service.rb | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/ee/app/services/ee/members/approve_access_request_service.rb b/ee/app/services/ee/members/approve_access_request_service.rb index f9335b946d2c9f..d46f430b8eb7a8 100644 --- a/ee/app/services/ee/members/approve_access_request_service.rb +++ b/ee/app/services/ee/members/approve_access_request_service.rb @@ -12,11 +12,21 @@ def after_execute(member:, skip_log_audit_event: false) private def log_audit_event(member:) - ::AuditEventService.new( - current_user, - member.source, - action: :create - ).for_member(member).security_event + audit_context = { + name: 'member_created', + author: current_user, + scope: member.source, + target: member.user, + target_details: member.user ? member.user.name : 'Deleted User', + message: 'Membership created', + additional_details: { + add: 'user_access', + as: ::Gitlab::Access.options_with_owner.key(member.access_level.to_i), + member_id: member.id + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) end end end -- GitLab From 6c2c56475387e84666e16c3eda2e93296239151c Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 14 Feb 2023 13:49:16 -0600 Subject: [PATCH 5/9] Add defaults for missing author/target --- ee/app/services/ee/members/approve_access_request_service.rb | 4 ++-- ee/app/services/ee/members/create_service.rb | 4 ++-- ee/app/services/ee/members/destroy_service.rb | 2 +- ee/app/services/ee/members/update_service.rb | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ee/app/services/ee/members/approve_access_request_service.rb b/ee/app/services/ee/members/approve_access_request_service.rb index d46f430b8eb7a8..c38d3d6410bd02 100644 --- a/ee/app/services/ee/members/approve_access_request_service.rb +++ b/ee/app/services/ee/members/approve_access_request_service.rb @@ -14,9 +14,9 @@ def after_execute(member:, skip_log_audit_event: false) def log_audit_event(member:) audit_context = { name: 'member_created', - author: current_user, + author: current_user || ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)'), scope: member.source, - target: member.user, + target: member.user || ::Gitlab::Audit::NullTarget.new, target_details: member.user ? member.user.name : 'Deleted User', message: 'Membership created', additional_details: { diff --git a/ee/app/services/ee/members/create_service.rb b/ee/app/services/ee/members/create_service.rb index 78d5f152142335..c2188329f39c4c 100644 --- a/ee/app/services/ee/members/create_service.rb +++ b/ee/app/services/ee/members/create_service.rb @@ -46,9 +46,9 @@ def after_execute(member:) def log_audit_event(member:) audit_context = { name: 'member_created', - author: current_user, + author: current_user || ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)'), scope: member.source, - target: member.user, + target: member.user || ::Gitlab::Audit::NullTarget.new, target_details: member.user ? member.user.name : 'Deleted User', message: 'Membership created', additional_details: { diff --git a/ee/app/services/ee/members/destroy_service.rb b/ee/app/services/ee/members/destroy_service.rb index 4ceee289ec2778..8b8c9250647150 100644 --- a/ee/app/services/ee/members/destroy_service.rb +++ b/ee/app/services/ee/members/destroy_service.rb @@ -32,7 +32,7 @@ def log_audit_event(member:, author:, action:) audit_context = { name: 'member_destroyed', scope: member.source, - target: member.user, + target: member.user || ::Gitlab::Audit::NullTarget.new, target_details: member.user ? member.user.name : 'Deleted User', additional_details: { remove: "user_access", diff --git a/ee/app/services/ee/members/update_service.rb b/ee/app/services/ee/members/update_service.rb index 33f0c3f4f1347e..ea9d5dcbafa613 100644 --- a/ee/app/services/ee/members/update_service.rb +++ b/ee/app/services/ee/members/update_service.rb @@ -30,9 +30,9 @@ def update_member(member, permission) def log_audit_event(old_access_level:, old_expiry:, member:) audit_context = { name: 'member_updated', - author: current_user, + author: current_user || ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)'), scope: member.source, - target: member.user, + target: member.user || ::Gitlab::Audit::NullTarget.new, target_details: member.user ? member.user.name : 'Deleted User', message: 'Membership updated', additional_details: { -- GitLab From f6222adb0607134598aef399674f4554fd2749a2 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Thu, 16 Feb 2023 12:57:17 -0600 Subject: [PATCH 6/9] Remove extraneous detail assignment --- ee/app/services/ee/members/destroy_service.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/app/services/ee/members/destroy_service.rb b/ee/app/services/ee/members/destroy_service.rb index 8b8c9250647150..ff3c41f2791ed7 100644 --- a/ee/app/services/ee/members/destroy_service.rb +++ b/ee/app/services/ee/members/destroy_service.rb @@ -39,7 +39,6 @@ def log_audit_event(member:, author:, action:) member_id: member.id } } - audit_context[:additional_details].update(system_event: true) if action == :expired case action when :destroy -- GitLab From c32be1513bf00425a597181a11980a6ee05d1c41 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 13 Mar 2023 16:52:10 -0500 Subject: [PATCH 7/9] Update specs to check audit event name --- .../approve_access_request_service_spec.rb | 48 +++++++++++++++++++ .../ee/members/create_service_spec.rb | 8 ++++ .../ee/members/destroy_service_spec.rb | 12 +++++ .../ee/members/update_service_spec.rb | 4 ++ 4 files changed, 72 insertions(+) create mode 100644 ee/spec/services/ee/members/approve_access_request_service_spec.rb diff --git a/ee/spec/services/ee/members/approve_access_request_service_spec.rb b/ee/spec/services/ee/members/approve_access_request_service_spec.rb new file mode 100644 index 00000000000000..f2a959e31ca451 --- /dev/null +++ b/ee/spec/services/ee/members/approve_access_request_service_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::ApproveAccessRequestService, feature_category: :subgroups do + let(:project) { create(:project, :public) } + let(:group) { create(:group, :public) } + let(:current_user) { create(:user) } + let(:access_requester_user) { create(:user) } + let(:access_requester) { source.requesters.find_by!(user_id: access_requester_user.id) } + let(:opts) { {} } + let(:params) { {} } + let(:custom_access_level) { Gitlab::Access::MAINTAINER } + + shared_examples "auditor with context" do + it "creates audit event with name" do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(name: "member_created") + ).and_call_original + + described_class.new(current_user, params).execute(access_requester, **opts) + end + end + + context "with auditing" do + context "for project access" do + let(:source) { project } + + before do + project.add_maintainer(current_user) + project.request_access(access_requester_user) + end + + it_behaves_like "auditor with context" + end + + context "for group access" do + let(:source) { group } + + before do + group.add_owner(current_user) + group.request_access(access_requester_user) + end + + it_behaves_like "auditor with context" + end + end +end diff --git a/ee/spec/services/ee/members/create_service_spec.rb b/ee/spec/services/ee/members/create_service_spec.rb index d8067ac0523e2f..42105976c66b52 100644 --- a/ee/spec/services/ee/members/create_service_spec.rb +++ b/ee/spec/services/ee/members/create_service_spec.rb @@ -135,6 +135,14 @@ } end + it 'audits event with name' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(name: "member_created") + ).and_call_original + + subject + end + include_examples 'sends streaming audit event' end diff --git a/ee/spec/services/ee/members/destroy_service_spec.rb b/ee/spec/services/ee/members/destroy_service_spec.rb index 0374d883fb8059..c080d56ff5f50d 100644 --- a/ee/spec/services/ee/members/destroy_service_spec.rb +++ b/ee/spec/services/ee/members/destroy_service_spec.rb @@ -15,6 +15,10 @@ shared_examples_for 'logs an audit event' do specify do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(name: "member_destroyed") + ).and_call_original + expect { event }.to change { AuditEvent.count }.by(1) end end @@ -61,6 +65,14 @@ context 'streaming audit event' do subject { described_class.new(current_user).execute(member, skip_authorization: true) } + it 'audits event with name' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(name: "member_destroyed") + ).and_call_original + + subject + end + include_examples 'sends streaming audit event' end diff --git a/ee/spec/services/ee/members/update_service_spec.rb b/ee/spec/services/ee/members/update_service_spec.rb index e3347f9b219e51..fac5aea7ba5366 100644 --- a/ee/spec/services/ee/members/update_service_spec.rb +++ b/ee/spec/services/ee/members/update_service_spec.rb @@ -20,6 +20,10 @@ shared_examples_for 'logs an audit event' do specify do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(name: "member_updated") + ).and_call_original + expect do described_class.new(current_user, params).execute(member, permission: permission) end.to change { AuditEvent.count }.by(1) -- GitLab From ceff650de75efa93c838f1874e473e14e859eaf5 Mon Sep 17 00:00:00 2001 From: Danger bot Date: Tue, 14 Mar 2023 15:32:39 +0000 Subject: [PATCH 8/9] Apply 1 suggestion(s) to 1 file(s) --- .../services/ee/members/approve_access_request_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/services/ee/members/approve_access_request_service_spec.rb b/ee/spec/services/ee/members/approve_access_request_service_spec.rb index f2a959e31ca451..b0d348d8c76ea7 100644 --- a/ee/spec/services/ee/members/approve_access_request_service_spec.rb +++ b/ee/spec/services/ee/members/approve_access_request_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Members::ApproveAccessRequestService, feature_category: :subgroups do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } let(:group) { create(:group, :public) } let(:current_user) { create(:user) } let(:access_requester_user) { create(:user) } -- GitLab From dd7d71410e388c2cd067c24ae15f0b37d61c70f6 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 20 Mar 2023 17:27:22 -0500 Subject: [PATCH 9/9] use let_it_be over let --- .../ee/members/approve_access_request_service_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/spec/services/ee/members/approve_access_request_service_spec.rb b/ee/spec/services/ee/members/approve_access_request_service_spec.rb index b0d348d8c76ea7..d12fbb12454a14 100644 --- a/ee/spec/services/ee/members/approve_access_request_service_spec.rb +++ b/ee/spec/services/ee/members/approve_access_request_service_spec.rb @@ -4,9 +4,9 @@ RSpec.describe Members::ApproveAccessRequestService, feature_category: :subgroups do let_it_be(:project) { create(:project, :public) } - let(:group) { create(:group, :public) } - let(:current_user) { create(:user) } - let(:access_requester_user) { create(:user) } + let_it_be(:group) { create(:group, :public) } + let_it_be(:current_user) { create(:user) } + let_it_be(:access_requester_user) { create(:user) } let(:access_requester) { source.requesters.find_by!(user_id: access_requester_user.id) } let(:opts) { {} } let(:params) { {} } -- GitLab