From 5163056a3b81b6e1d2a212eb8929249b1c96bd9b Mon Sep 17 00:00:00 2001 From: Sam Figueroa Date: Fri, 24 May 2024 18:40:33 +0200 Subject: [PATCH] Audit topic changes on projects - Explicitly load a new project from the DB when checking for changes on topics to avoid side effects of resetting changes when events depend on them still being present on the shared project variable. - Refs: https://gitlab.com/gitlab-org/gitlab/-/issues/343204 Changelog: added EE: true --- app/services/projects/update_service.rb | 8 ++++++ doc/user/compliance/audit_event_types.md | 1 + ee/app/services/ee/projects/update_service.rb | 25 +++++++++++++++++++ .../types/project_topics_updated.yml | 10 ++++++++ .../services/projects/update_service_spec.rb | 19 ++++++++++++++ 5 files changed, 63 insertions(+) create mode 100644 ee/config/audit_events/types/project_topics_updated.yml diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index a0748015e4834c..b599cb65612810 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -143,6 +143,9 @@ def after_default_branch_change(previous_default_branch) # overridden by EE module end + # overridden by EE module + def audit_topic_change(from:); end + # overridden by EE module def remove_unallowed_params params.delete(:emails_enabled) unless can?(current_user, :set_emails_disabled, project) @@ -174,6 +177,8 @@ def after_update update_pending_builds if runners_settings_toggled? + audit_topic_change(from: @previous_topics) + publish_events end @@ -246,6 +251,9 @@ def changing_repository_storage? end def build_topics + # Used in EE. Can't be cached in override due to Gitlab/ModuleWithInstanceVariables cop + @previous_topics = project.topic_list + topics = params.delete(:topics) tag_list = params.delete(:tag_list) topic_list = topics || tag_list diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 46e9712b23dc33..cc7703ac8cd612 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -169,6 +169,7 @@ Audit event types belong to the following product categories. | [`project_group_link_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108918) | Event triggered when a project group link is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374114) | Project | | [`project_imported`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/117528) | Event triggered when a project is imported. | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374105) | Group | | [`project_restored`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/117528) | Event triggered when a project is restored. | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374105) | Project | +| [`project_topics_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/154158) | Event triggered when a project topics assignments are changed. | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/343204) | Project | | [`project_unarchived`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/117528) | Event triggered when a project is unarchived. | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374105) | Project | | [`protected_branch_allow_force_push_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68869) | This audit event is created when a protected branch has its ability to allow force pushes is toggled | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [14.3](https://gitlab.com/gitlab-org/gitlab/-/issues/338873) | Project | | [`public_repository_download_operation`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/149842) | Event triggered when a Git repository for a public project is downloaded | **{dotted-circle}** No | **{check-circle}** Yes | GitLab [17.0](https://gitlab.com/gitlab-org/gitlab/-/issues/383218) | Project | diff --git a/ee/app/services/ee/projects/update_service.rb b/ee/app/services/ee/projects/update_service.rb index 4f4c70b48fe86e..07f0217d2123ba 100644 --- a/ee/app/services/ee/projects/update_service.rb +++ b/ee/app/services/ee/projects/update_service.rb @@ -7,6 +7,7 @@ module UpdateService DEFAULT_BRANCH_CHANGE_AUDIT_TYPE = 'project_default_branch_updated' DEFAULT_BRANCH_CHANGE_AUDIT_MESSAGE = "Default branch changed from %s to %s" + PROJECT_TOPIC_CHANGE_AUDIT_TYPE = 'project_topics_updated' PULL_MIRROR_ATTRIBUTES = %i[ mirror @@ -141,6 +142,30 @@ def after_default_branch_change(previous_default_branch) ::Security::ScanResultPolicies::SyncProjectWorker.perform_async(project.id) end + override :audit_topic_change + def audit_topic_change(from:) + # reload project topics without affecting shared project variable, as that leads to unwanted side effects + # when publishing events + to = ::Project.find(project.id).topic_list + topics_changed = from != to + + return unless topics_changed + + ::Gitlab::Audit::Auditor.audit( + name: PROJECT_TOPIC_CHANGE_AUDIT_TYPE, + author: current_user, + scope: project, + target: project, + message: "topics changed to: '#{to.join(',')}'", + target_details: project.full_path, + additional_details: { + event_name: PROJECT_TOPIC_CHANGE_AUDIT_TYPE, + from: from, + to: to + } + ) + end + def default_branch_update_blocked_by_security_policy? ::Security::SecurityOrchestrationPolicies::DefaultBranchUpdationCheckService.new(project: project).execute end diff --git a/ee/config/audit_events/types/project_topics_updated.yml b/ee/config/audit_events/types/project_topics_updated.yml new file mode 100644 index 00000000000000..cfd91964a7eda0 --- /dev/null +++ b/ee/config/audit_events/types/project_topics_updated.yml @@ -0,0 +1,10 @@ +--- +name: project_topics_updated +description: Event triggered when a project topics assignments are changed. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/343204 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/154158 +feature_category: compliance_management +milestone: '17.4' +saved_to_database: true +streamed: true +scope: ["Project"] diff --git a/ee/spec/services/projects/update_service_spec.rb b/ee/spec/services/projects/update_service_spec.rb index 78175d7dacadd7..378c6ca0279f93 100644 --- a/ee/spec/services/projects/update_service_spec.rb +++ b/ee/spec/services/projects/update_service_spec.rb @@ -199,6 +199,25 @@ } end + describe '#topics' do + let(:topics) { %w[foo bar] } + let(:topic_changed_audits) { AuditEvent.where(%q("details" LIKE '%:event_name: project_topics_updated%')) } + + it 'audits when topics change' do + expect { update_project(project, user, topics: topics) }.to change { topic_changed_audits.count }.by(1) + expect(topic_changed_audits.last.details[:to]).to match_array(topics) + end + + it 'does not audit when topics are unchanged' do + # with unrleated field change + expect { update_project(project, user, name: 'foo') }.not_to change { topic_changed_audits.count } + + update_project(project, user, topics: topics) + # with change where topic list is stable across update + expect { update_project(project, user, topics: topics) }.not_to change { topic_changed_audits.count } + end + end + describe '#name' do include_examples 'audit event logging' do let!(:old_name) { project.full_name } -- GitLab