From 5eb6f94b8282c9511533532ef3b236365ee2cb26 Mon Sep 17 00:00:00 2001
From: Alex Buijs <abuijs@gitlab.com>
Date: Thu, 11 Jan 2024 16:30:16 +0100
Subject: [PATCH] Log audit events when updating and deleting member roles
Whenever a member role is updated or deleted, log an audit event.
Changelog: added
EE: true
---
.../audit_event_types.md | 14 ++++---
ee/app/services/member_roles/base_service.rb | 21 ++++++++++
.../services/member_roles/create_service.rb | 19 +--------
.../services/member_roles/delete_service.rb | 2 +
.../services/member_roles/update_service.rb | 2 +
.../types/member_role_created.yml | 2 +-
.../types/member_role_deleted.yml | 9 +++++
.../types/member_role_updated.yml | 9 +++++
.../member_roles/create_service_spec.rb | 14 +++++--
.../member_roles/delete_service_spec.rb | 31 ++++++++++++++
.../member_roles/update_service_spec.rb | 40 ++++++++++++++++++-
11 files changed, 134 insertions(+), 29 deletions(-)
create mode 100644 ee/config/audit_events/types/member_role_deleted.yml
create mode 100644 ee/config/audit_events/types/member_role_updated.yml
diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md
index ae6d042b89fa9..ff99fc1f6c64e 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -64,12 +64,6 @@ Audit event types belong to the following product categories.
| [`update_event_streaming_destination`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74632) | Event triggered when an external audit event destination is updated| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [14.6](https://gitlab.com/gitlab-org/gitlab/-/issues/344664) |
| [`update_instance_event_streaming_destination`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125846) | Event triggered when an instance level external audit event destination is updated| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.2](https://gitlab.com/gitlab-org/gitlab/-/issues/404730) |
-### Authorization
-
-| Name | Description | Saved to database | Streamed | Introduced in |
-|:-----|:------------|:------------------|:---------|:--------------|
-| [`member_role_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/137087) | Event triggered when a custom role is created.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.7](https://gitlab.com/gitlab-org/gitlab/-/issues/388934) |
-
### Code review
| Name | Description | Saved to database | Streamed | Introduced in |
@@ -332,6 +326,14 @@ Audit event types belong to the following product categories.
|:-----|:------------|:------------------|:---------|:--------------|
| [`experiment_features_enabled_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118222) | Event triggered on toggling setting for enabling experiment AI features| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/404856/) |
+### Permissions
+
+| Name | Description | Saved to database | Streamed | Introduced in |
+|:-----|:------------|:------------------|:---------|:--------------|
+| [`member_role_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/137087) | Event triggered when a custom role is created.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.7](https://gitlab.com/gitlab-org/gitlab/-/issues/388934) |
+| [`member_role_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141630) | Event triggered when a custom role is deleted.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.9](https://gitlab.com/gitlab-org/gitlab/-/issues/437672) |
+| [`member_role_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141630) | Event triggered when a custom role is updated.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.9](https://gitlab.com/gitlab-org/gitlab/-/issues/437672) |
+
### Portfolio management
| Name | Description | Saved to database | Streamed | Introduced in |
diff --git a/ee/app/services/member_roles/base_service.rb b/ee/app/services/member_roles/base_service.rb
index 81635c0d7a1fc..b15633a70e910 100644
--- a/ee/app/services/member_roles/base_service.rb
+++ b/ee/app/services/member_roles/base_service.rb
@@ -24,5 +24,26 @@ def authorized_error
def group
params[:namespace] || member_role&.namespace
end
+
+ def log_audit_event(member_role, action:)
+ audit_context = {
+ name: "member_role_#{action}",
+ author: current_user,
+ scope: audit_event_scope,
+ target: member_role,
+ target_details: {
+ name: member_role.name,
+ description: member_role.description,
+ abilities: member_role.enabled_permissions.join(', ')
+ },
+ message: "Member role was #{action}"
+ }
+
+ ::Gitlab::Audit::Auditor.audit(audit_context)
+ end
+
+ def audit_event_scope
+ group || Gitlab::Audit::InstanceScope.new
+ end
end
end
diff --git a/ee/app/services/member_roles/create_service.rb b/ee/app/services/member_roles/create_service.rb
index 9bc37836e03cd..9ff93603bf4af 100644
--- a/ee/app/services/member_roles/create_service.rb
+++ b/ee/app/services/member_roles/create_service.rb
@@ -46,29 +46,12 @@ def create_member_role
member_role = MemberRole.new(params.merge(namespace: group))
if member_role.save
- log_audit_event(member_role)
+ log_audit_event(member_role, action: :created)
::ServiceResponse.success(payload: { member_role: member_role })
else
::ServiceResponse.error(message: member_role.errors.full_messages.join(', '))
end
end
-
- def log_audit_event(member_role)
- audit_context = {
- name: 'member_role_created',
- author: current_user,
- scope: audit_event_scope,
- target: member_role,
- target_details: member_role.enabled_permissions.join(', '),
- message: "Member role #{member_role.name} was created"
- }
-
- ::Gitlab::Audit::Auditor.audit(audit_context)
- end
-
- def audit_event_scope
- group || Gitlab::Audit::InstanceScope.new
- end
end
end
diff --git a/ee/app/services/member_roles/delete_service.rb b/ee/app/services/member_roles/delete_service.rb
index eeb7be263d6d4..4a232252b64a2 100644
--- a/ee/app/services/member_roles/delete_service.rb
+++ b/ee/app/services/member_roles/delete_service.rb
@@ -8,6 +8,8 @@ def execute(member_role)
return authorized_error unless allowed?
if member_role.destroy
+ log_audit_event(member_role, action: :deleted)
+
::ServiceResponse.success(payload: {
member_role: member_role
})
diff --git a/ee/app/services/member_roles/update_service.rb b/ee/app/services/member_roles/update_service.rb
index c73ce392247bb..fd817d82eefd5 100644
--- a/ee/app/services/member_roles/update_service.rb
+++ b/ee/app/services/member_roles/update_service.rb
@@ -16,6 +16,8 @@ def update_member_role
member_role.assign_attributes(params.slice(:name, :description, *MemberRole.all_customizable_permissions.keys))
if member_role.save
+ log_audit_event(member_role, action: :updated)
+
::ServiceResponse.success(payload: { member_role: member_role })
else
::ServiceResponse.error(message: member_role.errors.full_messages, payload: { member_role: member_role.reset })
diff --git a/ee/config/audit_events/types/member_role_created.yml b/ee/config/audit_events/types/member_role_created.yml
index 26305a0fbd742..84cc54216d984 100644
--- a/ee/config/audit_events/types/member_role_created.yml
+++ b/ee/config/audit_events/types/member_role_created.yml
@@ -3,7 +3,7 @@ name: member_role_created
description: Event triggered when a custom role is created.
introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/388934
introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/137087
-feature_category: authorization
+feature_category: permissions
milestone: '16.7'
saved_to_database: true
streamed: true
diff --git a/ee/config/audit_events/types/member_role_deleted.yml b/ee/config/audit_events/types/member_role_deleted.yml
new file mode 100644
index 0000000000000..888e03fe8a0f6
--- /dev/null
+++ b/ee/config/audit_events/types/member_role_deleted.yml
@@ -0,0 +1,9 @@
+---
+name: member_role_deleted
+description: Event triggered when a custom role is deleted.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/437672
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141630
+feature_category: permissions
+milestone: "16.9"
+saved_to_database: true
+streamed: true
diff --git a/ee/config/audit_events/types/member_role_updated.yml b/ee/config/audit_events/types/member_role_updated.yml
new file mode 100644
index 0000000000000..2cc23ca0711e4
--- /dev/null
+++ b/ee/config/audit_events/types/member_role_updated.yml
@@ -0,0 +1,9 @@
+---
+name: member_role_updated
+description: Event triggered when a custom role is updated.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/437672
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141630
+feature_category: permissions
+milestone: '16.9'
+saved_to_database: true
+streamed: true
diff --git a/ee/spec/services/member_roles/create_service_spec.rb b/ee/spec/services/member_roles/create_service_spec.rb
index e29cfc3430d08..96a7569aed6c6 100644
--- a/ee/spec/services/member_roles/create_service_spec.rb
+++ b/ee/spec/services/member_roles/create_service_spec.rb
@@ -33,6 +33,10 @@
it 'does not create the member role' do
expect { create_member_role }.not_to change { MemberRole.count }
end
+
+ it 'does not log an audit event' do
+ expect { create_member_role }.not_to change { AuditEvent.count }
+ end
end
shared_examples 'member role creation' do
@@ -66,9 +70,13 @@
details: {
author_name: user.name,
target_id: operation.id,
- target_type: 'MemberRole',
- target_details: 'admin_merge_request, read_vulnerability',
- custom_message: "Member role #{operation.name} was created",
+ target_type: operation.class.name,
+ target_details: {
+ name: operation.name,
+ description: operation.description,
+ abilities: operation.enabled_permissions.join(', ')
+ }.to_s,
+ custom_message: 'Member role was created',
author_class: user.class.name
}
}
diff --git a/ee/spec/services/member_roles/delete_service_spec.rb b/ee/spec/services/member_roles/delete_service_spec.rb
index 9ca0946ac96c1..f72c5224d70a7 100644
--- a/ee/spec/services/member_roles/delete_service_spec.rb
+++ b/ee/spec/services/member_roles/delete_service_spec.rb
@@ -37,6 +37,33 @@
expect(member_role).to be_destroyed
end
+
+ include_examples 'audit event logging' do
+ let(:licensed_features_to_stub) { { custom_roles: true } }
+ let(:event_type) { 'member_role_deleted' }
+ let(:operation) { result }
+ let(:fail_condition!) { allow(member_role).to receive(:destroy).and_return(false) }
+
+ let(:attributes) do
+ {
+ author_id: user.id,
+ entity_id: group.id,
+ entity_type: group.class.name,
+ details: {
+ author_name: user.name,
+ target_id: member_role.id,
+ target_type: member_role.class.name,
+ target_details: {
+ name: member_role.name,
+ description: member_role.description,
+ abilities: member_role.enabled_permissions.join(', ')
+ }.to_s,
+ custom_message: 'Member role was deleted',
+ author_class: user.class.name
+ }
+ }
+ end
+ end
end
context 'when failing to destroy the member role' do
@@ -50,6 +77,10 @@
expect(result).to be_error
expect(result.message).to match_array(['error message'])
end
+
+ it 'does not log an audit event' do
+ expect { result }.not_to change { AuditEvent.count }
+ end
end
end
end
diff --git a/ee/spec/services/member_roles/update_service_spec.rb b/ee/spec/services/member_roles/update_service_spec.rb
index 1ca6344e226f1..4c7cf5b684d45 100644
--- a/ee/spec/services/member_roles/update_service_spec.rb
+++ b/ee/spec/services/member_roles/update_service_spec.rb
@@ -8,7 +8,14 @@
let_it_be(:member_role) { create(:member_role, :guest, namespace: group, read_vulnerability: true) }
describe '#execute' do
- let(:params) { { name: 'new name', read_vulnerability: false, base_access_level: Gitlab::Access::DEVELOPER } }
+ let(:params) do
+ {
+ name: 'new name',
+ description: 'new description',
+ read_vulnerability: false,
+ base_access_level: Gitlab::Access::DEVELOPER
+ }
+ end
subject(:result) { described_class.new(user, params).execute(member_role) }
@@ -45,6 +52,33 @@
it 'does not update unpermitted attributes' do
expect { result }.not_to change { member_role.reload.base_access_level }
end
+
+ include_examples 'audit event logging' do
+ let(:licensed_features_to_stub) { { custom_roles: true } }
+ let(:event_type) { 'member_role_updated' }
+ let(:operation) { result }
+ let(:fail_condition!) { allow(member_role).to receive(:save).and_return(false) }
+
+ let(:attributes) do
+ {
+ author_id: user.id,
+ entity_id: group.id,
+ entity_type: group.class.name,
+ details: {
+ author_name: user.name,
+ target_id: member_role.id,
+ target_type: member_role.class.name,
+ target_details: {
+ name: 'new name',
+ description: 'new description',
+ abilities: ''
+ }.to_s,
+ custom_message: 'Member role was updated',
+ author_class: user.class.name
+ }
+ }
+ end
+ end
end
context 'when member role can not be updated' do
@@ -63,6 +97,10 @@
it 'includes the object errors' do
expect(result.message).to eq(['this is wrong'])
end
+
+ it 'does not log an audit event' do
+ expect { result }.not_to change { AuditEvent.count }
+ end
end
end
end
--
GitLab