From c1b819d076da7f2c44f81974ca45ed5d4c5cd7e4 Mon Sep 17 00:00:00 2001 From: smriti Date: Mon, 16 Sep 2024 11:11:40 +0530 Subject: [PATCH 01/10] Changes for allowed_email_domain update audit event Group setting allowed_email_domains update is an admin action and must be audited Changelog: added --- doc/user/compliance/audit_event_types.md | 1 + .../allowed_email_domains/update_service.rb | 19 ++++++++++++ .../types/allowed_email_domains_updated.yml | 10 ++++++ .../update_service_spec.rb | 31 +++++++++++++++++++ locale/gitlab.pot | 3 ++ 5 files changed, 64 insertions(+) create mode 100644 ee/config/audit_events/types/allowed_email_domains_updated.yml diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index fe2530c8bd4ad5..c700f8be3e9b85 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -298,6 +298,7 @@ Audit event types belong to the following product categories. |:------------|:------------|:------------------|:---------|:--------------|:--------------| | [`allow_mfa_for_subgroups_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when setting for Subgroups can set up their own two-factor authentication rules updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`allow_runner_registration_token_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when setting Allow members of projects and groups to create runners with runner registration tokens is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group, Project | +| [`allowed_email_domains_updated`](https://gitlab.com/gitlab-org/gitlab/-/issues/386080) | Triggered when group setting allowed email domains is changed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`create_ssh_certificate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134556) | Triggered when an SSH certificate is created | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/427413) | Group | | [`default_branch_name_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when default branch name for the group repository is changed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`delete_ssh_certificate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134556) | Triggered when an SSH certificate is deleted | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/427413) | Group | diff --git a/ee/app/services/ee/allowed_email_domains/update_service.rb b/ee/app/services/ee/allowed_email_domains/update_service.rb index 43251dadf76be5..e160e6ecba755a 100644 --- a/ee/app/services/ee/allowed_email_domains/update_service.rb +++ b/ee/app/services/ee/allowed_email_domains/update_service.rb @@ -28,6 +28,8 @@ def execute mark_deleted_allowed_email_domains_for_destruction build_new_allowed_emails_domains_records + + log_audit_event end private @@ -73,6 +75,23 @@ def existing_domains def domains_changed? existing_domains.sort != current_domains.sort end + + def log_audit_event + message = "Allowed email domain list updated from #{existing_domains} to #{current_domains}" + + ::Gitlab::Audit::Auditor.audit({ + name: "allowed_email_domains_updated", + author: current_user, + scope: group, + target: group, + target_details: group.path, + message: _(message), + additional_details: { + updated_domains: current_domains, + old_domains: existing_domains + } + }) + end end end end diff --git a/ee/config/audit_events/types/allowed_email_domains_updated.yml b/ee/config/audit_events/types/allowed_email_domains_updated.yml new file mode 100644 index 00000000000000..b4c506529954e7 --- /dev/null +++ b/ee/config/audit_events/types/allowed_email_domains_updated.yml @@ -0,0 +1,10 @@ +--- +name: allowed_email_domains_updated +description: Triggered when group setting allowed email domains is changed +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/486532 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/issues/386080 +feature_category: groups_and_projects +milestone: '17.4' +saved_to_database: true +streamed: true +scope: [Group] diff --git a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb index 58a3ef2fdcecdf..ef4b1e9e64bc12 100644 --- a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb +++ b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb @@ -8,6 +8,27 @@ subject { described_class.new(user, group, comma_separated_domains_list).execute } + shared_examples 'logs an audit event' do + it 'logs an audit event' do + allow(::Gitlab::Audit::Auditor).to receive(:audit) + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "allowed_email_domains_updated", + author: user, + scope: group, + target: group, + target_details: group.path, + message: "Allowed email domain list updated from #{domains} to #{comma_separated_domains_list.split(',')}", + additional_details: { + updated_domains: comma_separated_domains_list.split(","), + old_domains: domains + } + })).and_call_original + + subject + end + end + describe '#execute' do context 'as a normal user' do context 'for a group that has no email domain restriction' do @@ -36,6 +57,8 @@ end context 'for a group that has no email domain restriction' do + let(:domains) { [] } + context 'with valid domains' do let(:comma_separated_domains_list) { 'gitlab.com,acme.com' } @@ -51,6 +74,8 @@ expect(group.allowed_email_domains.map(&:domain)).to match_array(comma_separated_domains_list.split(",")) end + + it_behaves_like 'logs an audit event' end end @@ -70,6 +95,8 @@ expect { subject } .to(change { group.allowed_email_domains.select(&:marked_for_destruction?).size }.from(0).to(2)) end + + it_behaves_like 'logs an audit event' end context 'with valid domains' do @@ -107,6 +134,8 @@ expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip)) end end + + it_behaves_like 'logs an audit event' end context 'domains in the list repeats' do @@ -119,6 +148,8 @@ expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip).uniq) end + + it_behaves_like 'logs an audit event' end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0fb3f35065ba80..33bc17536e2fed 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5729,6 +5729,9 @@ msgstr "" msgid "Allowed" msgstr "" +msgid "Allowed email domain list updated" +msgstr "" + msgid "Allowed email domain restriction only permitted for top-level groups" msgstr "" -- GitLab From 5347ef0e5a848d68c08825478ed0657d4f2cec3f Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 18 Sep 2024 18:07:21 +0530 Subject: [PATCH 02/10] Added separate audit event for deleted domain --- doc/user/compliance/audit_event_types.md | 3 +- .../allowed_email_domains/update_service.rb | 28 +++++---- ...ted.yml => allowed_email_domain_added.yml} | 4 +- .../types/allowed_email_domain_deleted.yml | 10 +++ .../update_service_spec.rb | 61 +++++++++++-------- 5 files changed, 66 insertions(+), 40 deletions(-) rename ee/config/audit_events/types/{allowed_email_domains_updated.yml => allowed_email_domain_added.yml} (70%) create mode 100644 ee/config/audit_events/types/allowed_email_domain_deleted.yml diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index c700f8be3e9b85..134f895ccf9ffd 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -298,7 +298,8 @@ Audit event types belong to the following product categories. |:------------|:------------|:------------------|:---------|:--------------|:--------------| | [`allow_mfa_for_subgroups_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when setting for Subgroups can set up their own two-factor authentication rules updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`allow_runner_registration_token_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when setting Allow members of projects and groups to create runners with runner registration tokens is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group, Project | -| [`allowed_email_domains_updated`](https://gitlab.com/gitlab-org/gitlab/-/issues/386080) | Triggered when group setting allowed email domains is changed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | +| [`allowed_email_domain_added`](https://gitlab.com/gitlab-org/gitlab/-/issues/386080) | Triggered when group setting allowed email domain entry is added | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | +| [`allowed_email_domain_deleted`](https://gitlab.com/gitlab-org/gitlab/-/issues/386080) | Triggered when group setting allowed email domain entry is deleted | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`create_ssh_certificate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134556) | Triggered when an SSH certificate is created | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/427413) | Group | | [`default_branch_name_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when default branch name for the group repository is changed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`delete_ssh_certificate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134556) | Triggered when an SSH certificate is deleted | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/427413) | Group | diff --git a/ee/app/services/ee/allowed_email_domains/update_service.rb b/ee/app/services/ee/allowed_email_domains/update_service.rb index e160e6ecba755a..134b513aaf4386 100644 --- a/ee/app/services/ee/allowed_email_domains/update_service.rb +++ b/ee/app/services/ee/allowed_email_domains/update_service.rb @@ -28,8 +28,6 @@ def execute mark_deleted_allowed_email_domains_for_destruction build_new_allowed_emails_domains_records - - log_audit_event end private @@ -39,11 +37,20 @@ def execute def mark_deleted_allowed_email_domains_for_destruction return unless domains_to_be_deleted.present? + domains_marked_for_destruction = [] group.allowed_email_domains.each do |allowed_email_domain| if domains_to_be_deleted.include?(allowed_email_domain.domain) allowed_email_domain.mark_for_destruction + domains_marked_for_destruction << allowed_email_domain.domain end end + + return unless domains_marked_for_destruction + + log_audit_event( + "Domains #{domains_marked_for_destruction.join(',')} are deleted from restricted domains list", + "allowed_email_domain_deleted" + ) end def build_new_allowed_emails_domains_records @@ -52,6 +59,11 @@ def build_new_allowed_emails_domains_records domains_to_be_created.each do |domain| group.allowed_email_domains.build(domain: domain) end + + log_audit_event( + "Domains #{domains_to_be_created.join(',')} are added to restricted domains list", + "allowed_email_domain_added" + ) end def domains_to_be_deleted @@ -76,20 +88,14 @@ def domains_changed? existing_domains.sort != current_domains.sort end - def log_audit_event - message = "Allowed email domain list updated from #{existing_domains} to #{current_domains}" - + def log_audit_event(message, event_name) ::Gitlab::Audit::Auditor.audit({ - name: "allowed_email_domains_updated", + name: event_name, author: current_user, scope: group, target: group, target_details: group.path, - message: _(message), - additional_details: { - updated_domains: current_domains, - old_domains: existing_domains - } + message: _(message) }) end end diff --git a/ee/config/audit_events/types/allowed_email_domains_updated.yml b/ee/config/audit_events/types/allowed_email_domain_added.yml similarity index 70% rename from ee/config/audit_events/types/allowed_email_domains_updated.yml rename to ee/config/audit_events/types/allowed_email_domain_added.yml index b4c506529954e7..e7c969dea3d934 100644 --- a/ee/config/audit_events/types/allowed_email_domains_updated.yml +++ b/ee/config/audit_events/types/allowed_email_domain_added.yml @@ -1,6 +1,6 @@ --- -name: allowed_email_domains_updated -description: Triggered when group setting allowed email domains is changed +name: allowed_email_domain_added +description: Triggered when group setting allowed email domain entry is added introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/486532 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/issues/386080 feature_category: groups_and_projects diff --git a/ee/config/audit_events/types/allowed_email_domain_deleted.yml b/ee/config/audit_events/types/allowed_email_domain_deleted.yml new file mode 100644 index 00000000000000..86a55d88a1a374 --- /dev/null +++ b/ee/config/audit_events/types/allowed_email_domain_deleted.yml @@ -0,0 +1,10 @@ +--- +name: allowed_email_domain_deleted +description: Triggered when group setting allowed email domain entry is deleted +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/486532 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/issues/386080 +feature_category: groups_and_projects +milestone: '17.4' +saved_to_database: true +streamed: true +scope: [Group] diff --git a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb index ef4b1e9e64bc12..b5204f344aacdc 100644 --- a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb +++ b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb @@ -8,27 +8,6 @@ subject { described_class.new(user, group, comma_separated_domains_list).execute } - shared_examples 'logs an audit event' do - it 'logs an audit event' do - allow(::Gitlab::Audit::Auditor).to receive(:audit) - - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ - name: "allowed_email_domains_updated", - author: user, - scope: group, - target: group, - target_details: group.path, - message: "Allowed email domain list updated from #{domains} to #{comma_separated_domains_list.split(',')}", - additional_details: { - updated_domains: comma_separated_domains_list.split(","), - old_domains: domains - } - })).and_call_original - - subject - end - end - describe '#execute' do context 'as a normal user' do context 'for a group that has no email domain restriction' do @@ -75,7 +54,20 @@ expect(group.allowed_email_domains.map(&:domain)).to match_array(comma_separated_domains_list.split(",")) end - it_behaves_like 'logs an audit event' + it "logs an audit event" do + allow(::Gitlab::Audit::Auditor).to receive(:audit) + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "allowed_email_domain_added", + author: user, + scope: group, + target: group, + target_details: group.path, + message: "Domains #{comma_separated_domains_list} are added to restricted domains list" + })).and_call_original + + subject + end end end @@ -96,7 +88,20 @@ .to(change { group.allowed_email_domains.select(&:marked_for_destruction?).size }.from(0).to(2)) end - it_behaves_like 'logs an audit event' + it 'logs an audit event' do + allow(::Gitlab::Audit::Auditor).to receive(:audit) + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "allowed_email_domain_deleted", + author: user, + scope: group, + target: group, + target_details: group.path, + message: "Domains #{domains.join(',')} are deleted from restricted domains list" + })).and_call_original + + subject + end end context 'with valid domains' do @@ -133,9 +138,11 @@ expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip)) end - end - it_behaves_like 'logs an audit event' + it 'logs an audit event' do + expect(AuditEvent.last.details[:custom_message]).to eq("Domains #{comma_separated_domains_list.gsub(/\s+/, '')} are added to restricted domains list") + end + end end context 'domains in the list repeats' do @@ -149,7 +156,9 @@ expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip).uniq) end - it_behaves_like 'logs an audit event' + it 'logs an audit event' do + expect(AuditEvent.last.details[:custom_message]).to eq("Domains hey.com,google.com are added to restricted domains list") + end end end -- GitLab From 4ff27a18265e9e9bbe6438c6d2d25c4fa0d79afa Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 18 Sep 2024 18:11:34 +0530 Subject: [PATCH 03/10] Added separate audit event for deleted domain --- locale/gitlab.pot | 3 --- 1 file changed, 3 deletions(-) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 33bc17536e2fed..0fb3f35065ba80 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5729,9 +5729,6 @@ msgstr "" msgid "Allowed" msgstr "" -msgid "Allowed email domain list updated" -msgstr "" - msgid "Allowed email domain restriction only permitted for top-level groups" msgstr "" -- GitLab From 23d84ae1f58593ac4a908790b7fda5a5684c74a5 Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 18 Sep 2024 18:13:40 +0530 Subject: [PATCH 04/10] Removed extra line --- .../services/ee/allowed_email_domains/update_service_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb index b5204f344aacdc..6b8eda43226ba2 100644 --- a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb +++ b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb @@ -36,8 +36,6 @@ end context 'for a group that has no email domain restriction' do - let(:domains) { [] } - context 'with valid domains' do let(:comma_separated_domains_list) { 'gitlab.com,acme.com' } -- GitLab From ec11147a89af9282dae6328395ab42e5b9298756 Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 25 Sep 2024 23:07:22 +0530 Subject: [PATCH 05/10] Log event called only after save success --- .../allowed_email_domains/update_service.rb | 43 ++++++++----------- ee/app/services/ee/groups/update_service.rb | 10 ++++- .../types/allowed_email_domain_deleted.yml | 10 ----- ...d.yml => allowed_email_domain_updated.yml} | 6 +-- .../update_service_spec.rb | 15 ------- .../services/groups/update_service_spec.rb | 27 ++++++++++++ 6 files changed, 57 insertions(+), 54 deletions(-) delete mode 100644 ee/config/audit_events/types/allowed_email_domain_deleted.yml rename ee/config/audit_events/types/{allowed_email_domain_added.yml => allowed_email_domain_updated.yml} (81%) diff --git a/ee/app/services/ee/allowed_email_domains/update_service.rb b/ee/app/services/ee/allowed_email_domains/update_service.rb index 134b513aaf4386..19ef99822d097b 100644 --- a/ee/app/services/ee/allowed_email_domains/update_service.rb +++ b/ee/app/services/ee/allowed_email_domains/update_service.rb @@ -30,6 +30,24 @@ def execute build_new_allowed_emails_domains_records end + def log_audit_event(updated_domain_list) + raise "Nothing to log, the service must be executed first." unless existing_domains && updated_domain_list + + return unless License.feature_available?(:extended_audit_events) + return if updated_domain_list.sort == existing_domains.sort + + message = "Allowed email domain names updated from '#{existing_domains.join(',')}' to '#{updated_domain_list.join(',')}'" + + ::Gitlab::Audit::Auditor.audit({ + name: "allowed_email_domain_updated", + author: current_user, + scope: group, + target: group, + target_details: group.path, + message: _(message) + }) + end + private attr_reader :current_user, :group, :current_domains @@ -37,20 +55,11 @@ def execute def mark_deleted_allowed_email_domains_for_destruction return unless domains_to_be_deleted.present? - domains_marked_for_destruction = [] group.allowed_email_domains.each do |allowed_email_domain| if domains_to_be_deleted.include?(allowed_email_domain.domain) allowed_email_domain.mark_for_destruction - domains_marked_for_destruction << allowed_email_domain.domain end end - - return unless domains_marked_for_destruction - - log_audit_event( - "Domains #{domains_marked_for_destruction.join(',')} are deleted from restricted domains list", - "allowed_email_domain_deleted" - ) end def build_new_allowed_emails_domains_records @@ -59,11 +68,6 @@ def build_new_allowed_emails_domains_records domains_to_be_created.each do |domain| group.allowed_email_domains.build(domain: domain) end - - log_audit_event( - "Domains #{domains_to_be_created.join(',')} are added to restricted domains list", - "allowed_email_domain_added" - ) end def domains_to_be_deleted @@ -87,17 +91,6 @@ def existing_domains def domains_changed? existing_domains.sort != current_domains.sort end - - def log_audit_event(message, event_name) - ::Gitlab::Audit::Auditor.audit({ - name: event_name, - author: current_user, - scope: group, - target: group, - target_details: group.path, - message: _(message) - }) - end end end end diff --git a/ee/app/services/ee/groups/update_service.rb b/ee/app/services/ee/groups/update_service.rb index 5002fa61290b3a..5bc14103242827 100644 --- a/ee/app/services/ee/groups/update_service.rb +++ b/ee/app/services/ee/groups/update_service.rb @@ -133,7 +133,10 @@ def handle_allowed_email_domains_update comma_separated_domains = params.delete(:allowed_email_domains_list) - AllowedEmailDomains::UpdateService.new(current_user, group, comma_separated_domains).execute + # rubocop:disable Gitlab/ModuleWithInstanceVariables + @allowed_email_domains_list = AllowedEmailDomains::UpdateService.new(current_user, group, comma_separated_domains) + @allowed_email_domains_list.execute + # rubocop:enable Gitlab/ModuleWithInstanceVariables end override :allowed_settings_params @@ -143,8 +146,13 @@ def allowed_settings_params def log_audit_events @ip_restriction_update_service&.log_audit_event # rubocop:disable Gitlab/ModuleWithInstanceVariables + @allowed_email_domains_list&.log_audit_event(group.allowed_email_domains.map(&:domain)) # rubocop:disable Gitlab/ModuleWithInstanceVariables Audit::GroupChangesAuditor.new(current_user, group).execute + + if params.key?(params.key?(:allowed_email_domains_list)) + log_allowed_email_domains_list_event + end end def update_cascading_settings diff --git a/ee/config/audit_events/types/allowed_email_domain_deleted.yml b/ee/config/audit_events/types/allowed_email_domain_deleted.yml deleted file mode 100644 index 86a55d88a1a374..00000000000000 --- a/ee/config/audit_events/types/allowed_email_domain_deleted.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: allowed_email_domain_deleted -description: Triggered when group setting allowed email domain entry is deleted -introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/486532 -introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/issues/386080 -feature_category: groups_and_projects -milestone: '17.4' -saved_to_database: true -streamed: true -scope: [Group] diff --git a/ee/config/audit_events/types/allowed_email_domain_added.yml b/ee/config/audit_events/types/allowed_email_domain_updated.yml similarity index 81% rename from ee/config/audit_events/types/allowed_email_domain_added.yml rename to ee/config/audit_events/types/allowed_email_domain_updated.yml index e7c969dea3d934..b1e395b4c6fed9 100644 --- a/ee/config/audit_events/types/allowed_email_domain_added.yml +++ b/ee/config/audit_events/types/allowed_email_domain_updated.yml @@ -1,10 +1,10 @@ --- -name: allowed_email_domain_added -description: Triggered when group setting allowed email domain entry is added +name: allowed_email_domain_updated +description: Triggered when group setting allowed email domain entry is updated introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/486532 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/issues/386080 feature_category: groups_and_projects -milestone: '17.4' +milestone: '17.5' saved_to_database: true streamed: true scope: [Group] diff --git a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb index 6b8eda43226ba2..7de969a239f423 100644 --- a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb +++ b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb @@ -51,21 +51,6 @@ expect(group.allowed_email_domains.map(&:domain)).to match_array(comma_separated_domains_list.split(",")) end - - it "logs an audit event" do - allow(::Gitlab::Audit::Auditor).to receive(:audit) - - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ - name: "allowed_email_domain_added", - author: user, - scope: group, - target: group, - target_details: group.path, - message: "Domains #{comma_separated_domains_list} are added to restricted domains list" - })).and_call_original - - subject - end end end diff --git a/ee/spec/services/groups/update_service_spec.rb b/ee/spec/services/groups/update_service_spec.rb index 06720a34521645..18e406baf509e2 100644 --- a/ee/spec/services/groups/update_service_spec.rb +++ b/ee/spec/services/groups/update_service_spec.rb @@ -76,6 +76,33 @@ def operation end end end + + describe 'allowed email domain' do + context 'when allowed email domains were changed' do + before do + group.add_owner(user) + end + + include_examples 'audit event logging' do + let(:fail_condition!) do + allow(group).to receive(:save).and_return(false) + end + + def operation + update_group(group, user, allowed_email_domains_list: 'abcd.com,test.com') + end + + let(:attributes) do + audit_event_params.tap do |param| + param[:details].merge!( + event_name: 'allowed_email_domain_updated', + custom_message: "Allowed email domain names updated from '' to 'abcd.com,test.com'" + ) + end + end + end + end + end end context 'sub group' do -- GitLab From 63c8fac51b81ad8ea894c8bb0cda13aa778e9e94 Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 25 Sep 2024 23:11:43 +0530 Subject: [PATCH 06/10] Removed audit events from spec --- doc/user/compliance/audit_event_types.md | 3 +-- .../update_service_spec.rb | 23 ------------------- 2 files changed, 1 insertion(+), 25 deletions(-) diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 134f895ccf9ffd..d9513a00ef37c0 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -298,8 +298,7 @@ Audit event types belong to the following product categories. |:------------|:------------|:------------------|:---------|:--------------|:--------------| | [`allow_mfa_for_subgroups_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when setting for Subgroups can set up their own two-factor authentication rules updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`allow_runner_registration_token_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when setting Allow members of projects and groups to create runners with runner registration tokens is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group, Project | -| [`allowed_email_domain_added`](https://gitlab.com/gitlab-org/gitlab/-/issues/386080) | Triggered when group setting allowed email domain entry is added | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | -| [`allowed_email_domain_deleted`](https://gitlab.com/gitlab-org/gitlab/-/issues/386080) | Triggered when group setting allowed email domain entry is deleted | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | +| [`allowed_email_domain_updated`](https://gitlab.com/gitlab-org/gitlab/-/issues/386080) | Triggered when group setting allowed email domain entry is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`create_ssh_certificate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134556) | Triggered when an SSH certificate is created | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/427413) | Group | | [`default_branch_name_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when default branch name for the group repository is changed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`delete_ssh_certificate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134556) | Triggered when an SSH certificate is deleted | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/427413) | Group | diff --git a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb index 7de969a239f423..58a3ef2fdcecdf 100644 --- a/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb +++ b/ee/spec/services/ee/allowed_email_domains/update_service_spec.rb @@ -70,21 +70,6 @@ expect { subject } .to(change { group.allowed_email_domains.select(&:marked_for_destruction?).size }.from(0).to(2)) end - - it 'logs an audit event' do - allow(::Gitlab::Audit::Auditor).to receive(:audit) - - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ - name: "allowed_email_domain_deleted", - author: user, - scope: group, - target: group, - target_details: group.path, - message: "Domains #{domains.join(',')} are deleted from restricted domains list" - })).and_call_original - - subject - end end context 'with valid domains' do @@ -121,10 +106,6 @@ expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip)) end - - it 'logs an audit event' do - expect(AuditEvent.last.details[:custom_message]).to eq("Domains #{comma_separated_domains_list.gsub(/\s+/, '')} are added to restricted domains list") - end end end @@ -138,10 +119,6 @@ expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip).uniq) end - - it 'logs an audit event' do - expect(AuditEvent.last.details[:custom_message]).to eq("Domains hey.com,google.com are added to restricted domains list") - end end end -- GitLab From 9aa348e802ce48c848994f77732dc841bc1e5d8a Mon Sep 17 00:00:00 2001 From: smriti Date: Fri, 27 Sep 2024 18:12:58 +0530 Subject: [PATCH 07/10] Added reason for disabling cop --- ee/app/services/ee/groups/update_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/services/ee/groups/update_service.rb b/ee/app/services/ee/groups/update_service.rb index 5bc14103242827..890c9144082687 100644 --- a/ee/app/services/ee/groups/update_service.rb +++ b/ee/app/services/ee/groups/update_service.rb @@ -133,7 +133,7 @@ def handle_allowed_email_domains_update comma_separated_domains = params.delete(:allowed_email_domains_list) - # rubocop:disable Gitlab/ModuleWithInstanceVariables + # rubocop:disable Gitlab/ModuleWithInstanceVariables -- Reason: We need this instance to log audit event post save @allowed_email_domains_list = AllowedEmailDomains::UpdateService.new(current_user, group, comma_separated_domains) @allowed_email_domains_list.execute # rubocop:enable Gitlab/ModuleWithInstanceVariables -- GitLab From bc8544b1dbd624dcde396dad72a7ba28e8c93af2 Mon Sep 17 00:00:00 2001 From: smriti Date: Mon, 30 Sep 2024 10:17:28 +0530 Subject: [PATCH 08/10] Removed licensed feature check Removed line of code added by mistake Review comments incorporated --- .../ee/allowed_email_domains/update_service.rb | 3 +-- ee/app/services/ee/groups/update_service.rb | 10 +++------- .../types/allowed_email_domain_updated.yml | 2 +- ee/spec/services/groups/update_service_spec.rb | 12 +++--------- 4 files changed, 8 insertions(+), 19 deletions(-) diff --git a/ee/app/services/ee/allowed_email_domains/update_service.rb b/ee/app/services/ee/allowed_email_domains/update_service.rb index 19ef99822d097b..c0bff0d37f6888 100644 --- a/ee/app/services/ee/allowed_email_domains/update_service.rb +++ b/ee/app/services/ee/allowed_email_domains/update_service.rb @@ -31,9 +31,8 @@ def execute end def log_audit_event(updated_domain_list) - raise "Nothing to log, the service must be executed first." unless existing_domains && updated_domain_list + return unless existing_domains && updated_domain_list - return unless License.feature_available?(:extended_audit_events) return if updated_domain_list.sort == existing_domains.sort message = "Allowed email domain names updated from '#{existing_domains.join(',')}' to '#{updated_domain_list.join(',')}'" diff --git a/ee/app/services/ee/groups/update_service.rb b/ee/app/services/ee/groups/update_service.rb index 890c9144082687..c6cfdf03714781 100644 --- a/ee/app/services/ee/groups/update_service.rb +++ b/ee/app/services/ee/groups/update_service.rb @@ -134,8 +134,8 @@ def handle_allowed_email_domains_update comma_separated_domains = params.delete(:allowed_email_domains_list) # rubocop:disable Gitlab/ModuleWithInstanceVariables -- Reason: We need this instance to log audit event post save - @allowed_email_domains_list = AllowedEmailDomains::UpdateService.new(current_user, group, comma_separated_domains) - @allowed_email_domains_list.execute + @allowed_email_domains_update_service = AllowedEmailDomains::UpdateService.new(current_user, group, comma_separated_domains) + @allowed_email_domains_update_service.execute # rubocop:enable Gitlab/ModuleWithInstanceVariables end @@ -146,13 +146,9 @@ def allowed_settings_params def log_audit_events @ip_restriction_update_service&.log_audit_event # rubocop:disable Gitlab/ModuleWithInstanceVariables - @allowed_email_domains_list&.log_audit_event(group.allowed_email_domains.map(&:domain)) # rubocop:disable Gitlab/ModuleWithInstanceVariables + @allowed_email_domains_update_service&.log_audit_event(group.allowed_email_domains.map(&:domain)) # rubocop:disable Gitlab/ModuleWithInstanceVariables Audit::GroupChangesAuditor.new(current_user, group).execute - - if params.key?(params.key?(:allowed_email_domains_list)) - log_allowed_email_domains_list_event - end end def update_cascading_settings diff --git a/ee/config/audit_events/types/allowed_email_domain_updated.yml b/ee/config/audit_events/types/allowed_email_domain_updated.yml index b1e395b4c6fed9..e71cdd4c7e58ee 100644 --- a/ee/config/audit_events/types/allowed_email_domain_updated.yml +++ b/ee/config/audit_events/types/allowed_email_domain_updated.yml @@ -2,7 +2,7 @@ name: allowed_email_domain_updated description: Triggered when group setting allowed email domain entry is updated introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/486532 -introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/issues/386080 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166105 feature_category: groups_and_projects milestone: '17.5' saved_to_database: true diff --git a/ee/spec/services/groups/update_service_spec.rb b/ee/spec/services/groups/update_service_spec.rb index 18e406baf509e2..db407c22e7e1e0 100644 --- a/ee/spec/services/groups/update_service_spec.rb +++ b/ee/spec/services/groups/update_service_spec.rb @@ -32,9 +32,7 @@ allow(group).to receive(:save).and_return(false) end - def operation - update_group(group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) - end + let(:operation) { update_group(group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } let(:attributes) do audit_event_params.tap do |param| @@ -61,9 +59,7 @@ def operation allow(group).to receive(:save).and_return(false) end - def operation - update_group(group, user, ip_restriction_ranges: '192.168.0.0/24,10.0.0.0/8') - end + let(:operation) { update_group(group, user, ip_restriction_ranges: '192.168.0.0/24,10.0.0.0/8') } let(:attributes) do audit_event_params.tap do |param| @@ -88,9 +84,7 @@ def operation allow(group).to receive(:save).and_return(false) end - def operation - update_group(group, user, allowed_email_domains_list: 'abcd.com,test.com') - end + let(:operation) { update_group(group, user, allowed_email_domains_list: 'abcd.com,test.com') } let(:attributes) do audit_event_params.tap do |param| -- GitLab From 7cd930d9281836193312bfb6f690ca6bf6b9d409 Mon Sep 17 00:00:00 2001 From: smriti Date: Tue, 1 Oct 2024 18:10:34 +0530 Subject: [PATCH 09/10] Review comments incorporated --- 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 d9513a00ef37c0..b9a10ca5d476d8 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -298,7 +298,7 @@ Audit event types belong to the following product categories. |:------------|:------------|:------------------|:---------|:--------------|:--------------| | [`allow_mfa_for_subgroups_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when setting for Subgroups can set up their own two-factor authentication rules updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`allow_runner_registration_token_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when setting Allow members of projects and groups to create runners with runner registration tokens is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group, Project | -| [`allowed_email_domain_updated`](https://gitlab.com/gitlab-org/gitlab/-/issues/386080) | Triggered when group setting allowed email domain entry is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | +| [`allowed_email_domain_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166105) | Triggered when group setting allowed email domain entry is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`create_ssh_certificate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134556) | Triggered when an SSH certificate is created | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/427413) | Group | | [`default_branch_name_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164973) | Triggered when default branch name for the group repository is changed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/486532) | Group | | [`delete_ssh_certificate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134556) | Triggered when an SSH certificate is deleted | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/427413) | Group | -- GitLab From 39b2755d9b824d1121829e3927c0ca8955cdf30e Mon Sep 17 00:00:00 2001 From: smriti Date: Mon, 7 Oct 2024 15:51:36 +0530 Subject: [PATCH 10/10] Updated let statement Updated let statement --- ee/spec/services/groups/update_service_spec.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ee/spec/services/groups/update_service_spec.rb b/ee/spec/services/groups/update_service_spec.rb index db407c22e7e1e0..d5e826b5744cd3 100644 --- a/ee/spec/services/groups/update_service_spec.rb +++ b/ee/spec/services/groups/update_service_spec.rb @@ -32,7 +32,7 @@ allow(group).to receive(:save).and_return(false) end - let(:operation) { update_group(group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } + let(:operation_params) { { visibility_level: Gitlab::VisibilityLevel::PRIVATE } } let(:attributes) do audit_event_params.tap do |param| @@ -59,7 +59,7 @@ allow(group).to receive(:save).and_return(false) end - let(:operation) { update_group(group, user, ip_restriction_ranges: '192.168.0.0/24,10.0.0.0/8') } + let(:operation_params) { { ip_restriction_ranges: '192.168.0.0/24,10.0.0.0/8' } } let(:attributes) do audit_event_params.tap do |param| @@ -84,7 +84,7 @@ allow(group).to receive(:save).and_return(false) end - let(:operation) { update_group(group, user, allowed_email_domains_list: 'abcd.com,test.com') } + let(:operation_params) { { allowed_email_domains_list: 'abcd.com,test.com' } } let(:attributes) do audit_event_params.tap do |param| @@ -97,6 +97,10 @@ end end end + + def operation(update_params = operation_params) + update_group(group, user, **update_params) + end end context 'sub group' do -- GitLab