From e12e127a1a6dd4ca6dae941181cd2e380ecbbaab Mon Sep 17 00:00:00 2001 From: Eugie Limpin Date: Wed, 16 Apr 2025 14:22:59 +0800 Subject: [PATCH] Add audit event types for creating, updating, and deleting admin roles --- doc/user/compliance/audit_event_types.md | 3 + ee/app/services/member_roles/base_service.rb | 30 ++++--- .../audit_events/types/admin_role_created.yml | 10 +++ .../audit_events/types/admin_role_deleted.yml | 10 +++ .../audit_events/types/admin_role_updated.yml | 10 +++ .../member_roles/create_service_spec.rb | 14 ++-- .../member_roles/delete_service_spec.rb | 56 ++++++++----- .../member_roles/update_service_spec.rb | 80 +++++++++++++------ 8 files changed, 153 insertions(+), 60 deletions(-) create mode 100644 ee/config/audit_events/types/admin_role_created.yml create mode 100644 ee/config/audit_events/types/admin_role_deleted.yml create mode 100644 ee/config/audit_events/types/admin_role_updated.yml diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 947a463e41c229..cc7c8c13e42ed4 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -456,7 +456,10 @@ Audit event types belong to the following product categories. | Type name | Event triggered when | Saved to database | Introduced in | Scope | |:----------|:---------------------|:------------------|:--------------|:------| | [`admin_role_assigned_to_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186570) | A custom admin role is assigned to a user | {{< icon name="check-circle" >}} Yes | GitLab [18.0](https://gitlab.com/gitlab-org/gitlab/-/issues/507958) | User | +| [`admin_role_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188367) | A custom admin role is created | {{< icon name="check-circle" >}} Yes | GitLab [18.0](https://gitlab.com/gitlab-org/gitlab/-/issues/536131) | Instance | +| [`admin_role_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188367) | A custom admin role is deleted | {{< icon name="check-circle" >}} Yes | GitLab [18.0](https://gitlab.com/gitlab-org/gitlab/-/issues/536131) | Instance | | [`admin_role_unassigned_from_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186570) | A custom admin role is unassigned from a user | {{< icon name="check-circle" >}} Yes | GitLab [18.0](https://gitlab.com/gitlab-org/gitlab/-/issues/507958) | User | +| [`admin_role_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188367) | A custom admin role is updated | {{< icon name="check-circle" >}} Yes | GitLab [18.0](https://gitlab.com/gitlab-org/gitlab/-/issues/536131) | Instance | | [`member_role_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/137087) | A custom role is created | {{< icon name="check-circle" >}} Yes | GitLab [16.7](https://gitlab.com/gitlab-org/gitlab/-/issues/388934) | Group, Instance | | [`member_role_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141630) | A custom role is deleted | {{< icon name="check-circle" >}} Yes | GitLab [16.9](https://gitlab.com/gitlab-org/gitlab/-/issues/437672) | Group, Instance | | [`member_role_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141630) | A custom role is updated | {{< icon name="check-circle" >}} Yes | GitLab [16.9](https://gitlab.com/gitlab-org/gitlab/-/issues/437672) | Group, Instance | diff --git a/ee/app/services/member_roles/base_service.rb b/ee/app/services/member_roles/base_service.rb index b3079457bd38e1..7fd217f112e37c 100644 --- a/ee/app/services/member_roles/base_service.rb +++ b/ee/app/services/member_roles/base_service.rb @@ -25,25 +25,35 @@ def group params[:namespace] || member_role&.namespace end - def log_audit_event(member_role, action:) + def log_audit_event(role, action:) audit_context = { - name: "member_role_#{action}", author: current_user, - scope: audit_event_scope, - target: member_role, + target: role, target_details: { - name: member_role.name, - description: member_role.description, - abilities: member_role.enabled_permissions(current_user).keys.join(', ') + name: role.name, + description: role.description, + abilities: role.enabled_permissions(current_user).keys.join(', ') }, - message: "Member role was #{action}" + **audit_event_attributes(role, action) } ::Gitlab::Audit::Auditor.audit(audit_context) end - def audit_event_scope - group || Gitlab::Audit::InstanceScope.new + def audit_event_attributes(role, action) + if role.admin_related_role? + { + name: "admin_role_#{action}", + scope: Gitlab::Audit::InstanceScope.new, + message: "Admin role was #{action}" + } + else + { + name: "member_role_#{action}", + scope: group || Gitlab::Audit::InstanceScope.new, + message: "Member role was #{action}" + } + end end end end diff --git a/ee/config/audit_events/types/admin_role_created.yml b/ee/config/audit_events/types/admin_role_created.yml new file mode 100644 index 00000000000000..58e07a16b13c92 --- /dev/null +++ b/ee/config/audit_events/types/admin_role_created.yml @@ -0,0 +1,10 @@ +--- +name: admin_role_created +description: A custom admin role is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/536131 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188367 +feature_category: permissions +milestone: "18.0" +saved_to_database: true +streamed: true +scope: [Instance] diff --git a/ee/config/audit_events/types/admin_role_deleted.yml b/ee/config/audit_events/types/admin_role_deleted.yml new file mode 100644 index 00000000000000..c18c045522c809 --- /dev/null +++ b/ee/config/audit_events/types/admin_role_deleted.yml @@ -0,0 +1,10 @@ +--- +name: admin_role_deleted +description: A custom admin role is deleted +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/536131 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188367 +feature_category: permissions +milestone: "18.0" +saved_to_database: true +streamed: true +scope: [Instance] diff --git a/ee/config/audit_events/types/admin_role_updated.yml b/ee/config/audit_events/types/admin_role_updated.yml new file mode 100644 index 00000000000000..8fb85ddaa24a99 --- /dev/null +++ b/ee/config/audit_events/types/admin_role_updated.yml @@ -0,0 +1,10 @@ +--- +name: admin_role_updated +description: A custom admin role is updated +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/536131 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188367 +feature_category: permissions +milestone: "18.0" +saved_to_database: true +streamed: true +scope: [Instance] diff --git a/ee/spec/services/member_roles/create_service_spec.rb b/ee/spec/services/member_roles/create_service_spec.rb index 0fed99109e5ffb..b21c172151d7a0 100644 --- a/ee/spec/services/member_roles/create_service_spec.rb +++ b/ee/spec/services/member_roles/create_service_spec.rb @@ -40,7 +40,7 @@ end end - shared_examples 'member role creation' do + shared_examples 'member role creation' do |audit_event_type, audit_event_message| context 'with valid params' do it 'is successful' do expect(create_member_role).to be_success @@ -60,7 +60,7 @@ include_examples 'audit event logging' do let(:licensed_features_to_stub) { { custom_roles: true } } - let_it_be(:event_type) { 'member_role_created' } + let_it_be(:event_type) { audit_event_type } let(:operation) { create_member_role.payload[:member_role] } let(:attributes) do @@ -70,7 +70,7 @@ entity_type: audit_entity_type, details: { author_name: user.name, - event_name: "member_role_created", + event_name: audit_event_type, target_id: operation.id, target_type: operation.class.name, target_details: { @@ -78,7 +78,7 @@ description: operation.description, abilities: abilities.keys.join(', ') }.to_s, - custom_message: 'Member role was created', + custom_message: audit_event_message, author_class: user.class.name } } @@ -111,7 +111,7 @@ context 'with root group' do context 'when on SaaS', :saas do - it_behaves_like 'member role creation' + it_behaves_like 'member role creation', 'member_role_created', 'Member role was created' context 'with a missing param' do before do @@ -178,7 +178,7 @@ end context 'when on self-managed' do - it_behaves_like 'member role creation' + it_behaves_like 'member role creation', 'member_role_created', 'Member role was created' context 'with a missing param' do before do @@ -205,7 +205,7 @@ let(:abilities) { { read_admin_dashboard: true } } - it_behaves_like 'member role creation' + it_behaves_like 'member role creation', 'admin_role_created', 'Admin role was created' end end end diff --git a/ee/spec/services/member_roles/delete_service_spec.rb b/ee/spec/services/member_roles/delete_service_spec.rb index 07648ecc2f0120..2dfc775864e878 100644 --- a/ee/spec/services/member_roles/delete_service_spec.rb +++ b/ee/spec/services/member_roles/delete_service_spec.rb @@ -4,6 +4,7 @@ RSpec.describe MemberRoles::DeleteService, feature_category: :system_access do let_it_be(:group) { create(:group) } + let_it_be(:admin) { create(:admin) } let_it_be(:user) { create(:user) } subject(:service) { described_class.new(user) } @@ -15,7 +16,7 @@ describe '#execute' do subject(:result) { service.execute(member_role) } - shared_examples 'deleting member role' do + shared_examples 'deleting member role' do |audit_event_type, audit_event_message, audit_event_abilities| it 'is successful' do expect(result).to be_success end @@ -28,7 +29,7 @@ include_examples 'audit event logging' do let(:licensed_features_to_stub) { { custom_roles: true } } - let(:event_type) { 'member_role_deleted' } + let(:event_type) { audit_event_type } let(:operation) { result } let(:fail_condition!) { allow(member_role).to receive(:destroy).and_return(false) } @@ -41,13 +42,13 @@ author_name: user.name, target_id: member_role.id, target_type: member_role.class.name, - event_name: "member_role_deleted", + event_name: audit_event_type, target_details: { name: member_role.name, description: member_role.description, - abilities: "read_code" + abilities: audit_event_abilities }.to_s, - custom_message: 'Member role was deleted', + custom_message: audit_event_message, author_class: user.class.name } } @@ -68,11 +69,9 @@ end context 'with admin', :enable_admin_mode do - before_all do - user.update!(admin: true) - end + let_it_be(:user) { admin } - it_behaves_like 'deleting member role' + it_behaves_like 'deleting member role', 'member_role_deleted', 'Member role was deleted', 'read_code' context 'when the member role is linked to a security policy' do before do @@ -105,27 +104,44 @@ expect { result }.not_to change { AuditEvent.count } end end + + context 'with admin role' do + let(:member_role) { create(:member_role, :admin) } + + it_behaves_like 'deleting member role', 'admin_role_deleted', 'Admin role was deleted', 'read_admin_dashboard' + end end end context 'for SaaS', :saas do - let_it_be_with_reload(:member_role) { create(:member_role, :guest, namespace: group) } + context 'when member role' do + let_it_be_with_reload(:member_role) { create(:member_role, :guest, namespace: group) } - let(:audit_entity_id) { group.id } - let(:audit_entity_type) { group.class.name } + let(:audit_entity_id) { group.id } + let(:audit_entity_type) { group.class.name } - context 'with unauthorized user' do - it 'returns an error' do - expect(result).to be_error + context 'with unauthorized user' do + it 'returns an error' do + expect(result).to be_error + end end - end - context 'with owner' do - before_all do - group.add_owner(user) + context 'with owner' do + before_all do + group.add_owner(user) + end + + it_behaves_like 'deleting member role', 'member_role_deleted', 'Member role was deleted', 'read_code' end + end + + context 'with admin role', :enable_admin_mode do + let(:audit_entity_id) { Gitlab::Audit::InstanceScope.new.id } + let(:audit_entity_type) { 'Gitlab::Audit::InstanceScope' } + let(:member_role) { create(:member_role, :admin) } + let_it_be(:user) { admin } - it_behaves_like 'deleting member role' + it_behaves_like 'deleting member role', 'admin_role_deleted', 'Admin role was deleted', 'read_admin_dashboard' 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 8b347a6a4d6a19..ffd12b6f04cc8c 100644 --- a/ee/spec/services/member_roles/update_service_spec.rb +++ b/ee/spec/services/member_roles/update_service_spec.rb @@ -10,13 +10,14 @@ let(:user) { regular_user } describe '#execute' do + let(:existing_abilities) { { read_vulnerability: true } } + let(:updated_abilities) { { read_vulnerability: false, read_code: true } } let(:params) do { name: 'new name', description: 'new description', - read_vulnerability: false, - read_code: true, - base_access_level: Gitlab::Access::DEVELOPER + base_access_level: Gitlab::Access::DEVELOPER, + **updated_abilities } end @@ -26,7 +27,7 @@ stub_licensed_features(custom_roles: true) end - shared_examples 'member role update' do + shared_examples 'member role update' do |audit_event_type, audit_event_message| context 'with valid params' do it 'is successful' do expect(result).to be_success @@ -35,7 +36,7 @@ it 'updates the provided (permitted) attributes' do expect { result } .to change { member_role.reload.name }.to('new name') - .and change { member_role.reload.read_vulnerability }.to(false) + .and change { member_role.reload.permissions[existing_abilities.each_key.first.to_s] }.to(false) end it 'does not update unpermitted attributes' do @@ -44,7 +45,7 @@ include_examples 'audit event logging' do let(:licensed_features_to_stub) { { custom_roles: true } } - let(:event_type) { 'member_role_updated' } + let(:event_type) { audit_event_type } let(:operation) { result } let(:fail_condition!) { allow(member_role).to receive(:save).and_return(false) } @@ -55,15 +56,15 @@ entity_type: audit_entity_type, details: { author_name: user.name, - event_name: 'member_role_updated', + event_name: audit_event_type, target_id: member_role.id, target_type: member_role.class.name, target_details: { name: 'new name', description: 'new description', - abilities: 'read_code' + abilities: updated_abilities.filter { |_, v| v }.keys.join(', ') }.to_s, - custom_message: 'Member role was updated', + custom_message: audit_event_message, author_class: user.class.name } } @@ -95,7 +96,7 @@ end context 'for self-managed' do - let_it_be(:member_role) { create(:member_role, :guest, :instance, read_vulnerability: true) } + let(:member_role) { create(:member_role, :guest, :instance, **existing_abilities) } let(:audit_entity_id) { Gitlab::Audit::InstanceScope.new.id } let(:audit_entity_type) { 'Gitlab::Audit::InstanceScope' } @@ -109,32 +110,65 @@ context 'with authorized user', :enable_admin_mode do let(:user) { admin } - it_behaves_like 'member role update' + it_behaves_like 'member role update', 'member_role_updated', 'Member role was updated' + + context 'with admin roles' do + let(:existing_abilities) { { read_admin_dashboard: true } } + let(:updated_abilities) { { read_admin_dashboard: false, read_admin_users: true } } + let(:member_role) { create(:member_role, :admin, **existing_abilities) } + + it_behaves_like 'member role update', 'admin_role_updated', 'Admin role was updated' + end end end context 'for SaaS', :saas do - let_it_be(:member_role) { create(:member_role, :guest, namespace: group, read_vulnerability: true) } + context 'when member role' do + let(:member_role) { create(:member_role, :guest, namespace: group, **existing_abilities) } - let(:audit_entity_id) { group.id } - let(:audit_entity_type) { group.class.name } + let(:audit_entity_id) { group.id } + let(:audit_entity_type) { group.class.name } - context 'with unauthorized user' do - before_all do - group.add_maintainer(regular_user) + context 'with unauthorized user' do + before_all do + group.add_maintainer(regular_user) + end + + it 'returns an error' do + expect(result).to be_error + end end - it 'returns an error' do - expect(result).to be_error + context 'with authorized user' do + before_all do + group.add_owner(regular_user) + end + + it_behaves_like 'member role update', 'member_role_updated', 'Member role was updated' end end - context 'with authorized user' do - before_all do - group.add_owner(regular_user) + context 'when admin role', :enable_admin_mode do + let(:audit_entity_id) { Gitlab::Audit::InstanceScope.new.id } + let(:audit_entity_type) { 'Gitlab::Audit::InstanceScope' } + + let(:existing_abilities) { { read_admin_dashboard: true } } + let(:updated_abilities) { { read_admin_dashboard: false, read_admin_users: true } } + let(:member_role) { create(:member_role, :admin, **existing_abilities) } + + context 'with unauthorized user' do + let(:user) { regular_user } + + it 'returns an error' do + expect(result).to be_error + end end - it_behaves_like 'member role update' + context 'with authorized user' do + let(:user) { admin } + + it_behaves_like 'member role update', 'admin_role_updated', 'Admin role was updated' + end end end end -- GitLab