From a055c49f7a42ab7fbb0c36e05b325844e7d7f436 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Mon, 2 Dec 2024 13:25:46 +0200 Subject: [PATCH 01/25] Register new audit event Adds new audit event for group-level Secret Push Protection modifications Changelog: added MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412 EE: true --- doc/user/compliance/audit_event_types.md | 1 + .../types/group_secret_push_protection_updated.yml | 10 ++++++++++ 2 files changed, 11 insertions(+) create mode 100644 ee/config/audit_events/types/group_secret_push_protection_updated.yml diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 9ae192fc47d4f2..d172b074fbe432 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -485,6 +485,7 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| +| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/XXXXXX) | Triggered when a group and all of it's inherited groups/projects secret push protection is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | | [`project_security_exclusion_applied`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is applied in one of the security scanners | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/492465) | Project | | [`project_security_exclusion_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is created | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | | [`project_security_exclusion_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is deleted | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | diff --git a/ee/config/audit_events/types/group_secret_push_protection_updated.yml b/ee/config/audit_events/types/group_secret_push_protection_updated.yml new file mode 100644 index 00000000000000..30c8c18ed3ce24 --- /dev/null +++ b/ee/config/audit_events/types/group_secret_push_protection_updated.yml @@ -0,0 +1,10 @@ +--- +name: group_secret_push_protection_updated +description: Triggered when a group and all of it's inherited groups/projects secret push protection is updated +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/502829 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/XXXXXX +feature_category: secret_detection +milestone: '17.7' +saved_to_database: true +streamed: true +scope: [Group] -- GitLab From e3e5528c7b8a86dcb2d47eb5cafee9891e4fba5a Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Mon, 2 Dec 2024 15:20:27 +0200 Subject: [PATCH 02/25] Add audit for SPP changes --- ...amespace_secret_push_protection_service.rb | 97 +++++++++-- .../set_secret_push_protection_service.rb | 21 +-- ...set_group_secret_push_protection_worker.rb | 6 +- ...ace_secret_push_protection_service_spec.rb | 151 +++++++++++------- ...set_secret_push_protection_service_spec.rb | 4 +- ...roup_secret_push_protection_worker_spec.rb | 18 ++- 6 files changed, 199 insertions(+), 98 deletions(-) diff --git a/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb index 402732d89cec79..7cc5c688bb2f0b 100644 --- a/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb @@ -5,35 +5,52 @@ module Configuration class SetNamespaceSecretPushProtectionService PROJECTS_BATCH_SIZE = 100 - def self.execute(namespace:, enable:, excluded_projects_ids: []) - return unless namespace - return unless [true, false].include?(enable) + def initialize(namespace:, enable:, current_user:, excluded_projects_ids: []) + @namespace = namespace + @enable = enable + @current_user = current_user + @excluded_projects_ids = excluded_projects_ids + end + + def execute + return unless @namespace + return unless @current_user + return unless [true, false].include?(@enable) ApplicationRecord.transaction do - if namespace.is_a?(Group) - Project.for_group_and_its_subgroups(namespace).each_batch(of: PROJECTS_BATCH_SIZE) do |project_batch| - update_security_setting(project_batch.id_not_in(excluded_projects_ids), enable) + if @namespace.is_a?(Group) + Project.for_group_and_its_subgroups(@namespace).each_batch(of: PROJECTS_BATCH_SIZE) do |project_batch| + update_security_setting(project_batch.id_not_in(@excluded_projects_ids)) end + audit_group else - update_security_setting(Project.id_in(namespace.id).id_not_in(excluded_projects_ids), enable) + updated = update_security_setting(Project.id_in(@namespace.id).id_not_in(@excluded_projects_ids)) + audit_project unless updated == 0 end end + @enable end - def self.update_security_setting(projects, enable) - ProjectSecuritySetting.for_projects(projects.select(:id)) - .update_all(pre_receive_secret_detection_enabled: enable, - updated_at: Time.current) + private + + def update_security_setting(projects) + # rubocop:disable CodeReuse/ActiveRecord -- Specific use-case for this service + updated_records = ProjectSecuritySetting.for_projects(projects.select(:id)) + .where(pre_receive_secret_detection_enabled: !@enable) + .update_all(pre_receive_secret_detection_enabled: @enable, + updated_at: Time.current) + # rubocop:enable CodeReuse/ActiveRecord - create_missing_security_setting(projects, enable) + create_missing_security_setting(projects) + updated_records end - def self.create_missing_security_setting(projects, enable) + def create_missing_security_setting(projects) projects_without_security_setting = projects.without_security_setting security_setting_attributes = projects_without_security_setting.map do |project| { project_id: project.id, - pre_receive_secret_detection_enabled: enable, + pre_receive_secret_detection_enabled: @enable, updated_at: Time.current } end @@ -42,7 +59,57 @@ def self.create_missing_security_setting(projects, enable) ProjectSecuritySetting.upsert_all(security_setting_attributes) end - private_class_method :create_missing_security_setting, :update_security_setting + def audit_group + return unless @namespace.is_a?(Group) + + filtered_out_projects_full_path = fetch_filtered_out_projects + message = build_group_message(filtered_out_projects_full_path) + + audit_context = build_audit_context( + name: 'group_secret_push_protection_updated', + message: message + ) + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + def audit_project + return unless @namespace.is_a?(Project) + + message = "Secret push protection has been #{@enable ? 'enabled' : 'disabled'}" + audit_context = build_audit_context( + name: 'project_security_setting_updated', + message: message + ) + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + def fetch_filtered_out_projects + return [] unless @excluded_projects_ids.present? + + Project.id_in(@excluded_projects_ids).to_a.map(&:full_path) + end + + def build_group_message(filtered_out_projects_full_path) + if filtered_out_projects_full_path.empty? + "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ +groups/projects" + else + "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ +groups/projects expect for #{filtered_out_projects_full_path.join(', ')}" + end + end + + def build_audit_context(name:, message:) + { + name: name, + author: @current_user, + scope: @namespace, + target: @namespace, + message: message + } + end end end end diff --git a/ee/app/services/security/configuration/set_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_secret_push_protection_service.rb index ad5d998adf38a3..d602e9e772c227 100644 --- a/ee/app/services/security/configuration/set_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_secret_push_protection_service.rb @@ -4,27 +4,16 @@ module Security module Configuration class SetSecretPushProtectionService def self.execute(current_user:, namespace:, enable:) - # Some projects do not have the necessary security_setting, - # so we create it when it is missing - if namespace.is_a?(Project) && namespace.security_setting.nil? - namespace.security_setting = ProjectSecuritySetting.new - namespace.security_setting.save! - end + raise ArgumentError, 'Invalid argument. Either true or false should be passed.' unless [true, + false].include?(enable) - response = ServiceResponse.success( + ServiceResponse.success( payload: { - enabled: namespace.security_setting.set_pre_receive_secret_detection!( - enabled: enable - ), + enabled: SetNamespaceSecretPushProtectionService.new(current_user: current_user, namespace: namespace, + enable: enable).execute, errors: [] }) - if response.success? - Projects::ProjectSecuritySettingChangesAuditor.new( - current_user: current_user, model: namespace.security_setting).execute - end - - response rescue StandardError => e ServiceResponse.error( message: e.message, diff --git a/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb b/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb index 65186a47a74025..67b327392031ae 100644 --- a/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb +++ b/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb @@ -11,13 +11,13 @@ class SetGroupSecretPushProtectionWorker feature_category :secret_detection - def perform(group_id, enable, excluded_projects_ids = []) + def perform(group_id, enable, current_user, excluded_projects_ids = []) group = Group.find_by_id(group_id) return unless group - SetNamespaceSecretPushProtectionService.execute(namespace: group, enable: enable, - excluded_projects_ids: excluded_projects_ids) + SetNamespaceSecretPushProtectionService.new(namespace: group, enable: enable, current_user: current_user, + excluded_projects_ids: excluded_projects_ids).execute end end end diff --git a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb index 9f95a257ff1ab0..ef19b951cc66b7 100644 --- a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb @@ -4,6 +4,8 @@ RSpec.describe Security::Configuration::SetNamespaceSecretPushProtectionService, feature_category: :secret_detection do describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be(:top_level_group) { create(:group) } let_it_be(:mid_level_group) { create(:group, parent: top_level_group) } let_it_be(:bottom_level_group) { create(:group, parent: mid_level_group) } @@ -13,52 +15,47 @@ let_it_be_with_reload(:bottom_level_group_project) { create(:project, namespace: bottom_level_group) } let_it_be_with_reload(:excluded_project) { create(:project, namespace: mid_level_group) } + def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_project.id]) + described_class + .new(namespace: namespace, enable: enable, current_user: user, excluded_projects_ids: excluded_projects_ids) + .execute + end + context 'when namespace is a group' do let(:projects_to_change) { [top_level_group_project, mid_level_group_project, bottom_level_group_project] } it 'changes the attribute for nested projects' do + boolean_values = [true, false] + projects_to_change.each do |project| security_setting = project.security_setting - expect do - described_class - .execute(namespace: top_level_group, enable: true, excluded_projects_ids: [excluded_project.id]) - end.to change { security_setting.reload.pre_receive_secret_detection_enabled }.from(false).to(true) - - expect do - described_class - .execute(namespace: top_level_group, enable: true, excluded_projects_ids: [excluded_project.id]) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } - - expect do - described_class - .execute(namespace: top_level_group, enable: false, excluded_projects_ids: [excluded_project.id]) - end.to change { security_setting.reload.pre_receive_secret_detection_enabled } - .from(true).to(false) - - expect do - described_class - .execute(namespace: top_level_group, enable: false, excluded_projects_ids: [excluded_project.id]) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + boolean_values.each do |enable_value| + expect { execute_service(namespace: top_level_group, enable: enable_value) }.to change { + security_setting.reload.pre_receive_secret_detection_enabled + }.from(!enable_value).to(enable_value) + + expect { execute_service(namespace: top_level_group, enable: enable_value) } + .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + end end end it 'changes updated_at timestamp' do - expect { described_class.execute(namespace: top_level_group, enable: true) } - .to change { mid_level_group_project.reload.security_setting.updated_at } + expect { execute_service(namespace: top_level_group) }.to change { + mid_level_group_project.reload.security_setting.updated_at + } end it 'doesnt change the attribute for projects in excluded list' do security_setting = excluded_project.security_setting - expect do - described_class.execute(namespace: top_level_group, enable: true, - excluded_projects_ids: [excluded_project.id]) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + expect { execute_service(namespace: top_level_group) }.not_to change { + security_setting.reload.pre_receive_secret_detection_enabled + } - expect do - described_class.execute(namespace: mid_level_group, enable: false, - excluded_projects_ids: [excluded_project.id]) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + expect { execute_service(namespace: mid_level_group, enable: false) }.not_to change { + security_setting.reload.pre_receive_secret_detection_enabled + } end it 'rolls back changes when an error occurs' do @@ -86,6 +83,27 @@ end end + describe 'auditing' do + context 'when no excluded projects ids are provided' do + it 'audits using the correct message' do + expect { execute_service(namespace: top_level_group, excluded_projects_ids: []) } + .to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ +groups/projects") + end + end + + context 'when excluded projects ids are provided' do + it 'audits using the correct message' do + expect { execute_service(namespace: top_level_group) }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ +groups/projects expect for #{excluded_project.full_path}") + end + end + end + context 'when security_setting record does not yet exist' do let_it_be_with_reload(:project_without_security_setting) { create(:project, namespace: top_level_group) } @@ -94,8 +112,9 @@ end it 'creates security_setting and sets the value appropriately' do - expect { described_class.execute(namespace: top_level_group, enable: true) } - .to change { project_without_security_setting.reload.security_setting } + expect { execute_service(namespace: top_level_group) }.to change { + project_without_security_setting.reload.security_setting + } .from(nil).to(be_a(ProjectSecuritySetting)) expect(project_without_security_setting.reload.security_setting.pre_receive_secret_detection_enabled) @@ -105,7 +124,7 @@ context 'when arguments are invalid' do it 'does not change the attribute' do - expect { described_class.execute(namespace: top_level_group, enable: nil) } + expect { execute_service(namespace: top_level_group, enable: nil) } .not_to change { top_level_group_project.reload.security_setting.pre_receive_secret_detection_enabled } end end @@ -114,42 +133,62 @@ context 'when namespace is a project' do it 'changes the attribute' do security_setting = bottom_level_group_project.security_setting - expect do - described_class - .execute(namespace: bottom_level_group_project, enable: true) - end.to change { security_setting.reload.pre_receive_secret_detection_enabled }.from(false).to(true) + expect { execute_service(namespace: bottom_level_group_project) }.to change { + security_setting.reload.pre_receive_secret_detection_enabled + }.from(false).to(true) - expect do - described_class - .execute(namespace: bottom_level_group_project, enable: true) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + expect { execute_service(namespace: bottom_level_group_project) }.not_to change { + security_setting.reload.pre_receive_secret_detection_enabled + } - expect do - described_class - .execute(namespace: bottom_level_group_project, enable: false) - end.to change { security_setting.reload.pre_receive_secret_detection_enabled } - .from(true).to(false) + expect { execute_service(namespace: bottom_level_group_project, enable: false) }.to change { + security_setting.reload.pre_receive_secret_detection_enabled + }.from(true).to(false) - expect do - described_class - .execute(namespace: bottom_level_group_project, enable: false) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + expect { execute_service(namespace: bottom_level_group_project, enable: false) }.not_to change { + security_setting.reload.pre_receive_secret_detection_enabled + } end it 'changes updated_at timestamp' do - expect { described_class.execute(namespace: mid_level_group_project, enable: true) } + expect { execute_service(namespace: mid_level_group) } .to change { mid_level_group_project.reload.security_setting.updated_at } end it 'doesnt change the attribute for a project when it is in the excluded list' do security_setting = excluded_project.security_setting expect do - described_class.execute(namespace: excluded_project, - enable: !security_setting.pre_receive_secret_detection_enabled, - excluded_projects_ids: [excluded_project.id]) + execute_service(namespace: excluded_project, + enable: !security_setting.pre_receive_secret_detection_enabled) end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } end + describe 'auditing' do + context 'when no excluded_projects ids are provided' do + it 'audits using the correct message' do + expect { execute_service(namespace: bottom_level_group_project) }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been enabled") + + expect { execute_service(namespace: bottom_level_group_project, enable: false) }.to change { + AuditEvent.count + }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been disabled") + end + + it 'doesnt create audit if no change was made' do + expect { execute_service(namespace: bottom_level_group_project) }.to change { AuditEvent.count }.by(1) + # executing again with the same value should not create audit as there is no change + expect { execute_service(namespace: bottom_level_group_project) }.not_to change { AuditEvent.count } + end + end + + context 'when excluded_projects ids includes the project id' do + it 'doesnt create audit' do + expect { execute_service(namespace: excluded_project) }.not_to change { AuditEvent.count } + end + end + end + context 'when security_setting record does not yet exist' do let_it_be_with_reload(:project_without_security_setting) { create(:project, namespace: top_level_group) } @@ -158,7 +197,7 @@ end it 'creates security_setting and sets the value appropriately' do - expect { described_class.execute(namespace: project_without_security_setting, enable: true) } + expect { execute_service(namespace: project_without_security_setting) } .to change { project_without_security_setting.reload.security_setting } .from(nil).to(be_a(ProjectSecuritySetting)) @@ -169,7 +208,7 @@ context 'when arguments are invalid' do it 'does not change the attribute' do - expect { described_class.execute(namespace: bottom_level_group_project, enable: nil) } + expect { execute_service(namespace: bottom_level_group, enable: nil) } .not_to change { bottom_level_group_project.reload.security_setting.pre_receive_secret_detection_enabled } end end diff --git a/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb index e8a455241ab6cc..1a4bee39b60caf 100644 --- a/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb @@ -48,7 +48,7 @@ expect { described_class.execute(current_user: current_user, namespace: namespace, enable: true) } .to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq( - "Changed pre_receive_secret_detection_enabled from false to true") + "Secret push protection has been enabled") end end @@ -60,7 +60,7 @@ expect { described_class.execute(current_user: current_user, namespace: namespace2, enable: false) } .to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq( - "Changed pre_receive_secret_detection_enabled from true to false") + "Secret push protection has been disabled") end end diff --git a/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb b/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb index f58427915a6a97..2b0387d6ef1ade 100644 --- a/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb +++ b/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb @@ -5,25 +5,30 @@ RSpec.describe Security::Configuration::SetGroupSecretPushProtectionWorker, feature_category: :secret_detection do let_it_be(:group) { create(:group) } let_it_be(:group_id) { group.id } + let_it_be(:user) { create(:user) } let(:excluded_projects_ids) { [1, 2, 3] } + let(:set_namespace_spp_service) { instance_double(Security::Configuration::SetNamespaceSecretPushProtectionService) } describe '#perform' do subject(:run_worker) do - described_class.new.perform(group_id, true, excluded_projects_ids) + described_class.new.perform(group_id, true, user, excluded_projects_ids) end before do - allow(Security::Configuration::SetNamespaceSecretPushProtectionService).to receive(:execute) + allow(set_namespace_spp_service).to receive(:execute) + allow(Security::Configuration::SetNamespaceSecretPushProtectionService) + .to receive(:new).and_return(set_namespace_spp_service) end context 'when group exists' do it 'calls the `Security::Configuration::SetNamespaceSecretPushProtectionService` for the group' do run_worker - expect(Security::Configuration::SetNamespaceSecretPushProtectionService).to have_received(:execute).with( - { enable: true, namespace: group, excluded_projects_ids: excluded_projects_ids } + expect(Security::Configuration::SetNamespaceSecretPushProtectionService).to have_received(:new).with( + { enable: true, namespace: group, current_user: user, excluded_projects_ids: excluded_projects_ids } ) + expect(set_namespace_spp_service).to have_received(:execute) end end @@ -32,12 +37,13 @@ it 'does not call SetNamespaceSecretPushProtectionService' do run_worker - expect(Security::Configuration::SetNamespaceSecretPushProtectionService).not_to have_received(:execute) + expect(Security::Configuration::SetNamespaceSecretPushProtectionService).not_to have_received(:new) + expect(set_namespace_spp_service).not_to have_received(:execute) end end include_examples 'an idempotent worker' do - let(:job_args) { [group.id, true, excluded_projects_ids] } + let(:job_args) { [group.id, true, user, excluded_projects_ids] } end end end -- GitLab From 8904dfd626f918f118dcf8b8c20d8f18ed793e1a Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Mon, 2 Dec 2024 15:25:32 +0200 Subject: [PATCH 03/25] Fix audit_event_types.md file --- 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 d172b074fbe432..5beb1d6cb2ebd4 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -485,7 +485,7 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| -| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/XXXXXX) | Triggered when a group and all of it's inherited groups/projects secret push protection is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | +| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/XXXXXX) | Triggered when a group and all of it's inherited groups/projects secret push protection is updated | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | | [`project_security_exclusion_applied`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is applied in one of the security scanners | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/492465) | Project | | [`project_security_exclusion_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is created | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | | [`project_security_exclusion_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is deleted | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | -- GitLab From 6ac124bc8e8d675f1a92e307f4b1c7607ef64665 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Mon, 2 Dec 2024 16:37:49 +0200 Subject: [PATCH 04/25] Separate logic into base, group and project level services --- ...base_set_secret_push_protection_service.rb | 78 +++++++ ...et_group_secret_push_protection_service.rb | 53 +++++ ...amespace_secret_push_protection_service.rb | 115 ---------- ..._project_secret_push_protection_service.rb | 25 ++ .../set_secret_push_protection_service.rb | 2 +- ...set_group_secret_push_protection_worker.rb | 2 +- ...oup_secret_push_protection_service_spec.rb | 131 +++++++++++ ...ace_secret_push_protection_service_spec.rb | 217 ------------------ ...ect_secret_push_protection_service_spec.rb | 101 ++++++++ ...roup_secret_push_protection_worker_spec.rb | 12 +- 10 files changed, 396 insertions(+), 340 deletions(-) create mode 100644 ee/app/services/security/configuration/base_set_secret_push_protection_service.rb create mode 100644 ee/app/services/security/configuration/set_group_secret_push_protection_service.rb delete mode 100644 ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb create mode 100644 ee/app/services/security/configuration/set_project_secret_push_protection_service.rb create mode 100644 ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb delete mode 100644 ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb create mode 100644 ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb diff --git a/ee/app/services/security/configuration/base_set_secret_push_protection_service.rb b/ee/app/services/security/configuration/base_set_secret_push_protection_service.rb new file mode 100644 index 00000000000000..b54364467bc987 --- /dev/null +++ b/ee/app/services/security/configuration/base_set_secret_push_protection_service.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +module Security + module Configuration + class BaseSetSecretPushProtectionService + PROJECTS_BATCH_SIZE = 100 + def initialize(namespace:, enable:, current_user:, excluded_projects_ids: []) + @namespace = namespace + @enable = enable + @current_user = current_user + @excluded_projects_ids = excluded_projects_ids + end + + def execute + return unless valid_request? + + updated = false + ApplicationRecord.transaction do + projects_scope.each_batch(of: PROJECTS_BATCH_SIZE) do |project_batch| + updated = update_security_setting(project_batch.id_not_in(@excluded_projects_ids)) > 0 || updated + end + audit if updated + end + @enable + end + + protected + + def valid_request? + @namespace.present? && @current_user.present? && [true, false].include?(@enable) + end + + def update_security_setting(projects) + # rubocop:disable CodeReuse/ActiveRecord -- Specific use-case for this service + updated_records = ProjectSecuritySetting.for_projects(projects.select(:id)) + .where(pre_receive_secret_detection_enabled: !@enable) + .update_all(pre_receive_secret_detection_enabled: @enable, + updated_at: Time.current) + # rubocop:enable CodeReuse/ActiveRecord + + create_missing_security_setting(projects) + updated_records + end + + def create_missing_security_setting(projects) + projects_without_security_setting = projects.without_security_setting + security_setting_attributes = projects_without_security_setting.map do |project| + { + project_id: project.id, + pre_receive_secret_detection_enabled: @enable, + updated_at: Time.current + } + end + return unless security_setting_attributes.any? + + ProjectSecuritySetting.upsert_all(security_setting_attributes) + end + + def build_audit_context(name:, message:) + { + name: name, + author: @current_user, + scope: @namespace, + target: @namespace, + message: message + } + end + + def audit + raise NotImplementedError + end + + def projects_scope + raise NotImplementedError, "Subclasses must implement the `projects_scope` method" + end + end + end +end diff --git a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb new file mode 100644 index 00000000000000..5dd45a70439521 --- /dev/null +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module Security + module Configuration + class SetGroupSecretPushProtectionService < BaseSetSecretPushProtectionService + private + + def projects_scope + Project.for_group_and_its_subgroups(@namespace) + end + + def audit + return unless @namespace.is_a?(Group) + + filtered_out_projects_full_path = fetch_filtered_out_projects + message = build_group_message(filtered_out_projects_full_path) + + audit_context = build_audit_context( + name: 'group_secret_push_protection_updated', + message: message + ) + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + def fetch_filtered_out_projects + return [] unless @excluded_projects_ids.present? + + Project.id_in(@excluded_projects_ids).to_a.map(&:full_path) + end + + def build_group_message(filtered_out_projects_full_path) + if filtered_out_projects_full_path.empty? + "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ +groups/projects" + else + "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ +groups/projects expect for #{filtered_out_projects_full_path.join(', ')}" + end + end + + def build_audit_context(name:, message:) + { + name: name, + author: @current_user, + scope: @namespace, + target: @namespace, + message: message + } + end + end + end +end diff --git a/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb deleted file mode 100644 index 7cc5c688bb2f0b..00000000000000 --- a/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb +++ /dev/null @@ -1,115 +0,0 @@ -# frozen_string_literal: true - -module Security - module Configuration - class SetNamespaceSecretPushProtectionService - PROJECTS_BATCH_SIZE = 100 - - def initialize(namespace:, enable:, current_user:, excluded_projects_ids: []) - @namespace = namespace - @enable = enable - @current_user = current_user - @excluded_projects_ids = excluded_projects_ids - end - - def execute - return unless @namespace - return unless @current_user - return unless [true, false].include?(@enable) - - ApplicationRecord.transaction do - if @namespace.is_a?(Group) - Project.for_group_and_its_subgroups(@namespace).each_batch(of: PROJECTS_BATCH_SIZE) do |project_batch| - update_security_setting(project_batch.id_not_in(@excluded_projects_ids)) - end - audit_group - else - updated = update_security_setting(Project.id_in(@namespace.id).id_not_in(@excluded_projects_ids)) - audit_project unless updated == 0 - end - end - @enable - end - - private - - def update_security_setting(projects) - # rubocop:disable CodeReuse/ActiveRecord -- Specific use-case for this service - updated_records = ProjectSecuritySetting.for_projects(projects.select(:id)) - .where(pre_receive_secret_detection_enabled: !@enable) - .update_all(pre_receive_secret_detection_enabled: @enable, - updated_at: Time.current) - # rubocop:enable CodeReuse/ActiveRecord - - create_missing_security_setting(projects) - updated_records - end - - def create_missing_security_setting(projects) - projects_without_security_setting = projects.without_security_setting - security_setting_attributes = projects_without_security_setting.map do |project| - { - project_id: project.id, - pre_receive_secret_detection_enabled: @enable, - updated_at: Time.current - } - end - return unless security_setting_attributes.any? - - ProjectSecuritySetting.upsert_all(security_setting_attributes) - end - - def audit_group - return unless @namespace.is_a?(Group) - - filtered_out_projects_full_path = fetch_filtered_out_projects - message = build_group_message(filtered_out_projects_full_path) - - audit_context = build_audit_context( - name: 'group_secret_push_protection_updated', - message: message - ) - - ::Gitlab::Audit::Auditor.audit(audit_context) - end - - def audit_project - return unless @namespace.is_a?(Project) - - message = "Secret push protection has been #{@enable ? 'enabled' : 'disabled'}" - audit_context = build_audit_context( - name: 'project_security_setting_updated', - message: message - ) - - ::Gitlab::Audit::Auditor.audit(audit_context) - end - - def fetch_filtered_out_projects - return [] unless @excluded_projects_ids.present? - - Project.id_in(@excluded_projects_ids).to_a.map(&:full_path) - end - - def build_group_message(filtered_out_projects_full_path) - if filtered_out_projects_full_path.empty? - "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ -groups/projects" - else - "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ -groups/projects expect for #{filtered_out_projects_full_path.join(', ')}" - end - end - - def build_audit_context(name:, message:) - { - name: name, - author: @current_user, - scope: @namespace, - target: @namespace, - message: message - } - end - end - end -end diff --git a/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb new file mode 100644 index 00000000000000..6bdc502d8bf86b --- /dev/null +++ b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Security + module Configuration + class SetProjectSecretPushProtectionService < BaseSetSecretPushProtectionService + private + + def projects_scope + Project.id_in(@namespace.id) + end + + def audit + return unless @namespace.is_a?(Project) + + message = "Secret push protection has been #{@enable ? 'enabled' : 'disabled'}" + audit_context = build_audit_context( + name: 'project_security_setting_updated', + message: message + ) + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end +end diff --git a/ee/app/services/security/configuration/set_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_secret_push_protection_service.rb index d602e9e772c227..728bfb761f6ba1 100644 --- a/ee/app/services/security/configuration/set_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_secret_push_protection_service.rb @@ -9,7 +9,7 @@ def self.execute(current_user:, namespace:, enable:) ServiceResponse.success( payload: { - enabled: SetNamespaceSecretPushProtectionService.new(current_user: current_user, namespace: namespace, + enabled: SetProjectSecretPushProtectionService.new(current_user: current_user, namespace: namespace, enable: enable).execute, errors: [] }) diff --git a/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb b/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb index 67b327392031ae..b3d6f352f8106f 100644 --- a/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb +++ b/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb @@ -16,7 +16,7 @@ def perform(group_id, enable, current_user, excluded_projects_ids = []) return unless group - SetNamespaceSecretPushProtectionService.new(namespace: group, enable: enable, current_user: current_user, + SetGroupSecretPushProtectionService.new(namespace: group, enable: enable, current_user: current_user, excluded_projects_ids: excluded_projects_ids).execute end end diff --git a/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb new file mode 100644 index 00000000000000..e8615dd9d6763c --- /dev/null +++ b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::Configuration::SetGroupSecretPushProtectionService, feature_category: :secret_detection do + describe '#execute' do + let_it_be(:user) { create(:user) } + + let_it_be(:top_level_group) { create(:group) } + let_it_be(:mid_level_group) { create(:group, parent: top_level_group) } + let_it_be(:bottom_level_group) { create(:group, parent: mid_level_group) } + + let_it_be_with_reload(:top_level_group_project) { create(:project, namespace: top_level_group) } + let_it_be_with_reload(:mid_level_group_project) { create(:project, namespace: mid_level_group) } + let_it_be_with_reload(:bottom_level_group_project) { create(:project, namespace: bottom_level_group) } + let_it_be_with_reload(:excluded_project) { create(:project, namespace: mid_level_group) } + + let(:projects_to_change) { [top_level_group_project, mid_level_group_project, bottom_level_group_project] } + + def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_project.id]) + described_class + .new(namespace: namespace, enable: enable, current_user: user, excluded_projects_ids: excluded_projects_ids) + .execute + end + + it 'changes the attribute for nested projects' do + boolean_values = [true, false] + + projects_to_change.each do |project| + security_setting = project.security_setting + + boolean_values.each do |enable_value| + expect { execute_service(namespace: top_level_group, enable: enable_value) }.to change { + security_setting.reload.pre_receive_secret_detection_enabled + }.from(!enable_value).to(enable_value) + + expect { execute_service(namespace: top_level_group, enable: enable_value) } + .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + end + end + end + + it 'changes updated_at timestamp' do + expect { execute_service(namespace: top_level_group) }.to change { + mid_level_group_project.reload.security_setting.updated_at + } + end + + it 'doesnt change the attribute for projects in excluded list' do + security_setting = excluded_project.security_setting + expect { execute_service(namespace: top_level_group) }.not_to change { + security_setting.reload.pre_receive_secret_detection_enabled + } + + expect { execute_service(namespace: mid_level_group, enable: false) }.not_to change { + security_setting.reload.pre_receive_secret_detection_enabled + } + end + + it 'rolls back changes when an error occurs' do + initial_values = projects_to_change.map do |project| + project.security_setting.pre_receive_secret_detection_enabled + end + + call_counter = 0 + # Simulate an error on the last call to `update_security_setting` to make sure some changes were already made + allow(described_class).to receive(:update_security_setting) do |projects, enable, excluded_projects_ids| + call_counter += 1 + raise StandardError, "Simulated error on the third project" if call_counter == (projects_to_change.length - 1) + + described_class.send(:super, projects, enable, excluded_projects_ids) + end + + expect do + described_class.execute(namespace: top_level_group, enable: true, + excluded_projects_ids: [excluded_project.id]) + end.to raise_error(StandardError) + + projects_to_change.each_with_index do |project, index| + project.reload + expect(project.security_setting.pre_receive_secret_detection_enabled).to eq(initial_values[index]) + end + end + + describe 'auditing' do + context 'when no excluded projects ids are provided' do + it 'audits using the correct message' do + expect { execute_service(namespace: top_level_group, excluded_projects_ids: []) } + .to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ +groups/projects") + end + end + + context 'when excluded projects ids are provided' do + it 'audits using the correct message' do + expect { execute_service(namespace: top_level_group) }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ +groups/projects expect for #{excluded_project.full_path}") + end + end + end + + context 'when security_setting record does not yet exist' do + let_it_be_with_reload(:project_without_security_setting) { create(:project, namespace: top_level_group) } + + before do + project_without_security_setting.security_setting.destroy! + end + + it 'creates security_setting and sets the value appropriately' do + expect { execute_service(namespace: top_level_group) }.to change { + project_without_security_setting.reload.security_setting + } + .from(nil).to(be_a(ProjectSecuritySetting)) + + expect(project_without_security_setting.reload.security_setting.pre_receive_secret_detection_enabled) + .to be(true) + end + end + + context 'when arguments are invalid' do + it 'does not change the attribute' do + expect { execute_service(namespace: top_level_group, enable: nil) } + .not_to change { top_level_group_project.reload.security_setting.pre_receive_secret_detection_enabled } + end + end + end +end diff --git a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb deleted file mode 100644 index ef19b951cc66b7..00000000000000 --- a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb +++ /dev/null @@ -1,217 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Security::Configuration::SetNamespaceSecretPushProtectionService, feature_category: :secret_detection do - describe '#execute' do - let_it_be(:user) { create(:user) } - - let_it_be(:top_level_group) { create(:group) } - let_it_be(:mid_level_group) { create(:group, parent: top_level_group) } - let_it_be(:bottom_level_group) { create(:group, parent: mid_level_group) } - - let_it_be_with_reload(:top_level_group_project) { create(:project, namespace: top_level_group) } - let_it_be_with_reload(:mid_level_group_project) { create(:project, namespace: mid_level_group) } - let_it_be_with_reload(:bottom_level_group_project) { create(:project, namespace: bottom_level_group) } - let_it_be_with_reload(:excluded_project) { create(:project, namespace: mid_level_group) } - - def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_project.id]) - described_class - .new(namespace: namespace, enable: enable, current_user: user, excluded_projects_ids: excluded_projects_ids) - .execute - end - - context 'when namespace is a group' do - let(:projects_to_change) { [top_level_group_project, mid_level_group_project, bottom_level_group_project] } - - it 'changes the attribute for nested projects' do - boolean_values = [true, false] - - projects_to_change.each do |project| - security_setting = project.security_setting - - boolean_values.each do |enable_value| - expect { execute_service(namespace: top_level_group, enable: enable_value) }.to change { - security_setting.reload.pre_receive_secret_detection_enabled - }.from(!enable_value).to(enable_value) - - expect { execute_service(namespace: top_level_group, enable: enable_value) } - .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } - end - end - end - - it 'changes updated_at timestamp' do - expect { execute_service(namespace: top_level_group) }.to change { - mid_level_group_project.reload.security_setting.updated_at - } - end - - it 'doesnt change the attribute for projects in excluded list' do - security_setting = excluded_project.security_setting - expect { execute_service(namespace: top_level_group) }.not_to change { - security_setting.reload.pre_receive_secret_detection_enabled - } - - expect { execute_service(namespace: mid_level_group, enable: false) }.not_to change { - security_setting.reload.pre_receive_secret_detection_enabled - } - end - - it 'rolls back changes when an error occurs' do - initial_values = projects_to_change.map do |project| - project.security_setting.pre_receive_secret_detection_enabled - end - - call_counter = 0 - # Simulate an error on the last call to `update_security_setting` to make sure some changes were already made - allow(described_class).to receive(:update_security_setting) do |projects, enable, excluded_projects_ids| - call_counter += 1 - raise StandardError, "Simulated error on the third project" if call_counter == (projects_to_change.length - 1) - - described_class.send(:super, projects, enable, excluded_projects_ids) - end - - expect do - described_class.execute(namespace: top_level_group, enable: true, - excluded_projects_ids: [excluded_project.id]) - end.to raise_error(StandardError) - - projects_to_change.each_with_index do |project, index| - project.reload - expect(project.security_setting.pre_receive_secret_detection_enabled).to eq(initial_values[index]) - end - end - - describe 'auditing' do - context 'when no excluded projects ids are provided' do - it 'audits using the correct message' do - expect { execute_service(namespace: top_level_group, excluded_projects_ids: []) } - .to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq( - "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ -groups/projects") - end - end - - context 'when excluded projects ids are provided' do - it 'audits using the correct message' do - expect { execute_service(namespace: top_level_group) }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq( - "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ -groups/projects expect for #{excluded_project.full_path}") - end - end - end - - context 'when security_setting record does not yet exist' do - let_it_be_with_reload(:project_without_security_setting) { create(:project, namespace: top_level_group) } - - before do - project_without_security_setting.security_setting.destroy! - end - - it 'creates security_setting and sets the value appropriately' do - expect { execute_service(namespace: top_level_group) }.to change { - project_without_security_setting.reload.security_setting - } - .from(nil).to(be_a(ProjectSecuritySetting)) - - expect(project_without_security_setting.reload.security_setting.pre_receive_secret_detection_enabled) - .to be(true) - end - end - - context 'when arguments are invalid' do - it 'does not change the attribute' do - expect { execute_service(namespace: top_level_group, enable: nil) } - .not_to change { top_level_group_project.reload.security_setting.pre_receive_secret_detection_enabled } - end - end - end - - context 'when namespace is a project' do - it 'changes the attribute' do - security_setting = bottom_level_group_project.security_setting - expect { execute_service(namespace: bottom_level_group_project) }.to change { - security_setting.reload.pre_receive_secret_detection_enabled - }.from(false).to(true) - - expect { execute_service(namespace: bottom_level_group_project) }.not_to change { - security_setting.reload.pre_receive_secret_detection_enabled - } - - expect { execute_service(namespace: bottom_level_group_project, enable: false) }.to change { - security_setting.reload.pre_receive_secret_detection_enabled - }.from(true).to(false) - - expect { execute_service(namespace: bottom_level_group_project, enable: false) }.not_to change { - security_setting.reload.pre_receive_secret_detection_enabled - } - end - - it 'changes updated_at timestamp' do - expect { execute_service(namespace: mid_level_group) } - .to change { mid_level_group_project.reload.security_setting.updated_at } - end - - it 'doesnt change the attribute for a project when it is in the excluded list' do - security_setting = excluded_project.security_setting - expect do - execute_service(namespace: excluded_project, - enable: !security_setting.pre_receive_secret_detection_enabled) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } - end - - describe 'auditing' do - context 'when no excluded_projects ids are provided' do - it 'audits using the correct message' do - expect { execute_service(namespace: bottom_level_group_project) }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been enabled") - - expect { execute_service(namespace: bottom_level_group_project, enable: false) }.to change { - AuditEvent.count - }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been disabled") - end - - it 'doesnt create audit if no change was made' do - expect { execute_service(namespace: bottom_level_group_project) }.to change { AuditEvent.count }.by(1) - # executing again with the same value should not create audit as there is no change - expect { execute_service(namespace: bottom_level_group_project) }.not_to change { AuditEvent.count } - end - end - - context 'when excluded_projects ids includes the project id' do - it 'doesnt create audit' do - expect { execute_service(namespace: excluded_project) }.not_to change { AuditEvent.count } - end - end - end - - context 'when security_setting record does not yet exist' do - let_it_be_with_reload(:project_without_security_setting) { create(:project, namespace: top_level_group) } - - before do - project_without_security_setting.security_setting.destroy! - end - - it 'creates security_setting and sets the value appropriately' do - expect { execute_service(namespace: project_without_security_setting) } - .to change { project_without_security_setting.reload.security_setting } - .from(nil).to(be_a(ProjectSecuritySetting)) - - expect(project_without_security_setting.reload.security_setting.pre_receive_secret_detection_enabled) - .to be(true) - end - end - - context 'when arguments are invalid' do - it 'does not change the attribute' do - expect { execute_service(namespace: bottom_level_group, enable: nil) } - .not_to change { bottom_level_group_project.reload.security_setting.pre_receive_secret_detection_enabled } - end - end - end - end -end diff --git a/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb new file mode 100644 index 00000000000000..ba292971717f15 --- /dev/null +++ b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::Configuration::SetProjectSecretPushProtectionService, feature_category: :secret_detection do + describe '#execute' do + let_it_be(:user) { create(:user) } + + let_it_be_with_reload(:project_1) { create(:project) } + let_it_be_with_reload(:project_2) { create(:project) } + let_it_be_with_reload(:excluded_project) { create(:project) } + + def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_project.id]) + described_class + .new(namespace: namespace, enable: enable, current_user: user, excluded_projects_ids: excluded_projects_ids) + .execute + end + + it 'changes the attribute' do + security_setting = project_2.security_setting + expect { execute_service(namespace: project_2) }.to change { + security_setting.reload.pre_receive_secret_detection_enabled + }.from(false).to(true) + + expect { execute_service(namespace: project_2) }.not_to change { + security_setting.reload.pre_receive_secret_detection_enabled + } + + expect { execute_service(namespace: project_2, enable: false) }.to change { + security_setting.reload.pre_receive_secret_detection_enabled + }.from(true).to(false) + + expect { execute_service(namespace: project_2, enable: false) }.not_to change { + security_setting.reload.pre_receive_secret_detection_enabled + } + end + + it 'changes updated_at timestamp' do + expect { execute_service(namespace: project_1) } + .to change { project_1.reload.security_setting.updated_at } + end + + it 'doesnt change the attribute for a project when it is in the excluded list' do + security_setting = excluded_project.security_setting + expect do + execute_service(namespace: excluded_project, + enable: !security_setting.pre_receive_secret_detection_enabled) + end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + end + + describe 'auditing' do + context 'when no excluded_projects ids are provided' do + it 'audits using the correct message' do + expect { execute_service(namespace: project_2) }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been enabled") + + expect { execute_service(namespace: project_2, enable: false) }.to change { + AuditEvent.count + }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been disabled") + end + + it 'doesnt create audit if no change was made' do + expect { execute_service(namespace: project_2) }.to change { AuditEvent.count }.by(1) + # executing again with the same value should not create audit as there is no change + expect { execute_service(namespace: project_2) }.not_to change { AuditEvent.count } + end + end + + context 'when excluded_projects ids includes the project id' do + it 'doesnt create audit' do + expect { execute_service(namespace: excluded_project) }.not_to change { AuditEvent.count } + end + end + end + + context 'when security_setting record does not yet exist' do + let_it_be_with_reload(:project_without_security_setting) { create(:project) } + + before do + project_without_security_setting.security_setting.destroy! + end + + it 'creates security_setting and sets the value appropriately' do + expect { execute_service(namespace: project_without_security_setting) } + .to change { project_without_security_setting.reload.security_setting } + .from(nil).to(be_a(ProjectSecuritySetting)) + + expect(project_without_security_setting.reload.security_setting.pre_receive_secret_detection_enabled) + .to be(true) + end + end + + context 'when arguments are invalid' do + it 'does not change the attribute' do + expect { execute_service(namespace: project_2, enable: nil) } + .not_to change { project_2.reload.security_setting.pre_receive_secret_detection_enabled } + end + end + end +end diff --git a/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb b/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb index 2b0387d6ef1ade..7ad87d1de5f9fa 100644 --- a/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb +++ b/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb @@ -8,7 +8,7 @@ let_it_be(:user) { create(:user) } let(:excluded_projects_ids) { [1, 2, 3] } - let(:set_namespace_spp_service) { instance_double(Security::Configuration::SetNamespaceSecretPushProtectionService) } + let(:set_namespace_spp_service) { instance_double(Security::Configuration::SetGroupSecretPushProtectionService) } describe '#perform' do subject(:run_worker) do @@ -17,15 +17,15 @@ before do allow(set_namespace_spp_service).to receive(:execute) - allow(Security::Configuration::SetNamespaceSecretPushProtectionService) + allow(Security::Configuration::SetGroupSecretPushProtectionService) .to receive(:new).and_return(set_namespace_spp_service) end context 'when group exists' do - it 'calls the `Security::Configuration::SetNamespaceSecretPushProtectionService` for the group' do + it 'calls the `Security::Configuration::SetGroupSecretPushProtectionService` for the group' do run_worker - expect(Security::Configuration::SetNamespaceSecretPushProtectionService).to have_received(:new).with( + expect(Security::Configuration::SetGroupSecretPushProtectionService).to have_received(:new).with( { enable: true, namespace: group, current_user: user, excluded_projects_ids: excluded_projects_ids } ) expect(set_namespace_spp_service).to have_received(:execute) @@ -35,9 +35,9 @@ context 'when no such a group with group_id exists' do let_it_be(:group_id) { Time.now.to_i } - it 'does not call SetNamespaceSecretPushProtectionService' do + it 'does not call SetGroupSecretPushProtectionService' do run_worker - expect(Security::Configuration::SetNamespaceSecretPushProtectionService).not_to have_received(:new) + expect(Security::Configuration::SetGroupSecretPushProtectionService).not_to have_received(:new) expect(set_namespace_spp_service).not_to have_received(:execute) end end -- GitLab From 725e3107cd22f0191eee98612eb0cf6021a1fca8 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Wed, 4 Dec 2024 16:10:08 +0200 Subject: [PATCH 05/25] Add find user by current_user_id --- .../set_group_secret_push_protection_worker.rb | 5 +++-- ...t_group_secret_push_protection_worker_spec.rb | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb b/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb index b3d6f352f8106f..706e2d86568dd5 100644 --- a/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb +++ b/ee/app/workers/security/configuration/set_group_secret_push_protection_worker.rb @@ -11,10 +11,11 @@ class SetGroupSecretPushProtectionWorker feature_category :secret_detection - def perform(group_id, enable, current_user, excluded_projects_ids = []) + def perform(group_id, enable, current_user_id = nil, excluded_projects_ids = []) group = Group.find_by_id(group_id) + current_user = User.find_by_id(current_user_id) - return unless group + return unless group && current_user SetGroupSecretPushProtectionService.new(namespace: group, enable: enable, current_user: current_user, excluded_projects_ids: excluded_projects_ids).execute diff --git a/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb b/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb index 7ad87d1de5f9fa..02ddf24d20321a 100644 --- a/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb +++ b/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb @@ -6,13 +6,15 @@ let_it_be(:group) { create(:group) } let_it_be(:group_id) { group.id } let_it_be(:user) { create(:user) } + let_it_be(:group_id) { group.id } + let_it_be(:user_id) { user.id } let(:excluded_projects_ids) { [1, 2, 3] } let(:set_namespace_spp_service) { instance_double(Security::Configuration::SetGroupSecretPushProtectionService) } describe '#perform' do subject(:run_worker) do - described_class.new.perform(group_id, true, user, excluded_projects_ids) + described_class.new.perform(group_id, true, user_id, excluded_projects_ids) end before do @@ -42,8 +44,18 @@ end end + context 'when no such a user with user_id exists' do + let_it_be(:user_id) { Time.now.to_i } + + it 'does not call SetGroupSecretPushProtectionService' do + run_worker + expect(Security::Configuration::SetGroupSecretPushProtectionService).not_to have_received(:new) + expect(set_namespace_spp_service).not_to have_received(:execute) + end + end + include_examples 'an idempotent worker' do - let(:job_args) { [group.id, true, user, excluded_projects_ids] } + let(:job_args) { [group.id, true, user_id, excluded_projects_ids] } end end end -- GitLab From 1842dfb8cf1b9a2ea13353097a690ce33aeb1f01 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Wed, 4 Dec 2024 16:11:51 +0200 Subject: [PATCH 06/25] Rename base service for clarity --- .../set_group_secret_push_protection_service.rb | 2 +- ...> set_namespace_secret_push_protection_service.rb} | 11 ++++++----- .../set_project_secret_push_protection_service.rb | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) rename ee/app/services/security/configuration/{base_set_secret_push_protection_service.rb => set_namespace_secret_push_protection_service.rb} (87%) diff --git a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb index 5dd45a70439521..f17fd15f41125e 100644 --- a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -2,7 +2,7 @@ module Security module Configuration - class SetGroupSecretPushProtectionService < BaseSetSecretPushProtectionService + class SetGroupSecretPushProtectionService < SetNamespaceSecretPushProtectionService private def projects_scope diff --git a/ee/app/services/security/configuration/base_set_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb similarity index 87% rename from ee/app/services/security/configuration/base_set_secret_push_protection_service.rb rename to ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb index b54364467bc987..4e60f563819c5e 100644 --- a/ee/app/services/security/configuration/base_set_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb @@ -2,7 +2,7 @@ module Security module Configuration - class BaseSetSecretPushProtectionService + class SetNamespaceSecretPushProtectionService PROJECTS_BATCH_SIZE = 100 def initialize(namespace:, enable:, current_user:, excluded_projects_ids: []) @namespace = namespace @@ -14,12 +14,13 @@ def initialize(namespace:, enable:, current_user:, excluded_projects_ids: []) def execute return unless valid_request? - updated = false + any_updated = false ApplicationRecord.transaction do projects_scope.each_batch(of: PROJECTS_BATCH_SIZE) do |project_batch| - updated = update_security_setting(project_batch.id_not_in(@excluded_projects_ids)) > 0 || updated + updated_count = update_security_setting(project_batch.id_not_in(@excluded_projects_ids)) + any_updated ||= updated_count > 0 end - audit if updated + audit if any_updated end @enable end @@ -71,7 +72,7 @@ def audit end def projects_scope - raise NotImplementedError, "Subclasses must implement the `projects_scope` method" + raise NotImplementedError end end end diff --git a/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb index 6bdc502d8bf86b..bc49ec0873aef2 100644 --- a/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb @@ -2,7 +2,7 @@ module Security module Configuration - class SetProjectSecretPushProtectionService < BaseSetSecretPushProtectionService + class SetProjectSecretPushProtectionService < SetNamespaceSecretPushProtectionService private def projects_scope -- GitLab From b4e2677e67fd5ff40ea4aec5fa44999423bc0dd3 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Sun, 8 Dec 2024 14:22:21 +0000 Subject: [PATCH 07/25] Fix audit message typo --- .../configuration/set_group_secret_push_protection_service.rb | 2 +- .../set_group_secret_push_protection_service_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb index f17fd15f41125e..f676431981c888 100644 --- a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -35,7 +35,7 @@ def build_group_message(filtered_out_projects_full_path) groups/projects" else "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ -groups/projects expect for #{filtered_out_projects_full_path.join(', ')}" +groups/projects except for #{filtered_out_projects_full_path.join(', ')}" end end diff --git a/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb index e8615dd9d6763c..bd9e50c585dad0 100644 --- a/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb @@ -98,7 +98,7 @@ def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_p expect { execute_service(namespace: top_level_group) }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq( "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ -groups/projects expect for #{excluded_project.full_path}") +groups/projects except for #{excluded_project.full_path}") end end end -- GitLab From 22f986570c99cce23d879c2e323d3b370fd98be6 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Sun, 8 Dec 2024 16:24:03 +0200 Subject: [PATCH 08/25] Add MR URL to audit yml --- .../audit_events/types/group_secret_push_protection_updated.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/config/audit_events/types/group_secret_push_protection_updated.yml b/ee/config/audit_events/types/group_secret_push_protection_updated.yml index 30c8c18ed3ce24..d8af27e13236c2 100644 --- a/ee/config/audit_events/types/group_secret_push_protection_updated.yml +++ b/ee/config/audit_events/types/group_secret_push_protection_updated.yml @@ -2,7 +2,7 @@ name: group_secret_push_protection_updated description: Triggered when a group and all of it's inherited groups/projects secret push protection is updated introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/502829 -introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/XXXXXX +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412 feature_category: secret_detection milestone: '17.7' saved_to_database: true -- GitLab From 185543d588608c97602dbd63bd989c3f3b5b7441 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Sun, 8 Dec 2024 17:10:29 +0200 Subject: [PATCH 09/25] Recompile audit docs --- 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 5beb1d6cb2ebd4..7251e1c9efe71f 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -485,7 +485,7 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| -| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/XXXXXX) | Triggered when a group and all of it's inherited groups/projects secret push protection is updated | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | +| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | Triggered when a group and all of it's inherited groups/projects secret push protection is updated | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | | [`project_security_exclusion_applied`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is applied in one of the security scanners | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/492465) | Project | | [`project_security_exclusion_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is created | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | | [`project_security_exclusion_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is deleted | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | -- GitLab From eaa72cba78d790f171f6535b145c7d1bfacffdb9 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Mon, 9 Dec 2024 14:13:02 +0200 Subject: [PATCH 10/25] Fix merging conflict --- .../set_group_secret_push_protection_worker_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb b/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb index 02ddf24d20321a..da2b07c38e10bb 100644 --- a/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb +++ b/ee/spec/workers/security/configuration/set_group_secret_push_protection_worker_spec.rb @@ -4,7 +4,6 @@ RSpec.describe Security::Configuration::SetGroupSecretPushProtectionWorker, feature_category: :secret_detection do let_it_be(:group) { create(:group) } - let_it_be(:group_id) { group.id } let_it_be(:user) { create(:user) } let_it_be(:group_id) { group.id } let_it_be(:user_id) { user.id } -- GitLab From 064c4625d4ebb26e500c66a347fed641e1489357 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Mon, 9 Dec 2024 14:25:58 +0200 Subject: [PATCH 11/25] Recompile audit docs --- doc/user/compliance/audit_event_types.md | 12 ++++++------ .../types/group_secret_push_protection_updated.yml | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index c5611383ec26cf..ff5368ba6257a3 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -486,12 +486,12 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| -| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | Triggered when a group and all of it's inherited groups/projects secret push protection is updated | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | -| [`project_security_exclusion_applied`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is applied in one of the security scanners | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/492465) | Project | -| [`project_security_exclusion_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is created | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | -| [`project_security_exclusion_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is deleted | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | -| [`project_security_exclusion_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | Triggered when a project security exclusion is updated | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | -| [`skip_secret_push_protection`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147855) | Triggered when secret push protection is skipped by the user | **{check-circle}** Yes | GitLab [16.11](https://gitlab.com/gitlab-org/gitlab/-/issues/441185) | Project | +| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | A group and some of it's inherited groups/projects secret push protection is updated | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | +| [`project_security_exclusion_applied`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is applied in one of the security scanners | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/492465) | Project | +| [`project_security_exclusion_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is created | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | +| [`project_security_exclusion_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is deleted | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | +| [`project_security_exclusion_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is updated | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | +| [`skip_secret_push_protection`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147855) | Secret push protection is skipped by the user | **{check-circle}** Yes | GitLab [16.11](https://gitlab.com/gitlab-org/gitlab/-/issues/441185) | Project | ### Secrets management diff --git a/ee/config/audit_events/types/group_secret_push_protection_updated.yml b/ee/config/audit_events/types/group_secret_push_protection_updated.yml index d8af27e13236c2..ef947fd4d2ce02 100644 --- a/ee/config/audit_events/types/group_secret_push_protection_updated.yml +++ b/ee/config/audit_events/types/group_secret_push_protection_updated.yml @@ -1,6 +1,6 @@ --- name: group_secret_push_protection_updated -description: Triggered when a group and all of it's inherited groups/projects secret push protection is updated +description: A group and some of it's inherited groups/projects secret push protection is updated introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/502829 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412 feature_category: secret_detection -- GitLab From 4602a33e5cab5bd7cd3e5ffad441f9e1ab521141 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Mon, 9 Dec 2024 14:46:48 +0200 Subject: [PATCH 12/25] Fix missing audit when creating security_setting --- .../set_namespace_secret_push_protection_service.rb | 7 +++---- .../set_project_secret_push_protection_service_spec.rb | 3 +++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb index 4e60f563819c5e..1a397a14c98d65 100644 --- a/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_namespace_secret_push_protection_service.rb @@ -39,8 +39,7 @@ def update_security_setting(projects) updated_at: Time.current) # rubocop:enable CodeReuse/ActiveRecord - create_missing_security_setting(projects) - updated_records + create_missing_security_setting(projects) + updated_records end def create_missing_security_setting(projects) @@ -52,9 +51,9 @@ def create_missing_security_setting(projects) updated_at: Time.current } end - return unless security_setting_attributes.any? + return 0 unless security_setting_attributes.any? - ProjectSecuritySetting.upsert_all(security_setting_attributes) + ProjectSecuritySetting.upsert_all(security_setting_attributes).length end def build_audit_context(name:, message:) diff --git a/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb index ba292971717f15..9cc5ff99c97208 100644 --- a/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb @@ -88,6 +88,9 @@ def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_p expect(project_without_security_setting.reload.security_setting.pre_receive_secret_detection_enabled) .to be(true) + + expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been enabled") + expect(AuditEvent.last.details[:target_id]).to eq(project_without_security_setting.id) end end -- GitLab From 2ef0214861cb8b252279a383e9b737e14690cb37 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Tue, 10 Dec 2024 13:48:25 +0200 Subject: [PATCH 13/25] Remove redundant duplicate function --- .../set_group_secret_push_protection_service.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb index f676431981c888..eee5846c73dd92 100644 --- a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -38,16 +38,6 @@ def build_group_message(filtered_out_projects_full_path) groups/projects except for #{filtered_out_projects_full_path.join(', ')}" end end - - def build_audit_context(name:, message:) - { - name: name, - author: @current_user, - scope: @namespace, - target: @namespace, - message: message - } - end end end end -- GitLab From 1292b14bb165f2e5cf282d2bcd3750faed3edcbd Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Tue, 10 Dec 2024 16:31:56 +0200 Subject: [PATCH 14/25] Fix audit includes non nested project names --- ...et_group_secret_push_protection_service.rb | 2 +- ...oup_secret_push_protection_service_spec.rb | 38 +++++++++++++------ 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb index eee5846c73dd92..e62964b4ffc28d 100644 --- a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -26,7 +26,7 @@ def audit def fetch_filtered_out_projects return [] unless @excluded_projects_ids.present? - Project.id_in(@excluded_projects_ids).to_a.map(&:full_path) + Project.for_group_and_its_subgroups(@namespace).id_in(@excluded_projects_ids).to_a.map(&:full_path) end def build_group_message(filtered_out_projects_full_path) diff --git a/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb index bd9e50c585dad0..849a07a6999916 100644 --- a/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb @@ -94,30 +94,44 @@ def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_p end context 'when excluded projects ids are provided' do - it 'audits using the correct message' do - expect { execute_service(namespace: top_level_group) }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq( - "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ + context 'when excluded ids matches projects in that group' do + it 'audits using the correct message' do + expect { execute_service(namespace: top_level_group) }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ groups/projects except for #{excluded_project.full_path}") + end + end + + context 'when excluded ids does not match projects in that group' do + it 'audits using the correct message' do + expect do + execute_service(namespace: top_level_group, excluded_projects_ids: [Time.now.to_i]) + end.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ +groups/projects") + end end end end context 'when security_setting record does not yet exist' do - let_it_be_with_reload(:project_without_security_setting) { create(:project, namespace: top_level_group) } - before do - project_without_security_setting.security_setting.destroy! + bottom_level_group_project.security_setting.destroy! end it 'creates security_setting and sets the value appropriately' do - expect { execute_service(namespace: top_level_group) }.to change { - project_without_security_setting.reload.security_setting - } - .from(nil).to(be_a(ProjectSecuritySetting)) + expect { execute_service(namespace: bottom_level_group) }.to change { + bottom_level_group_project.reload.security_setting + }.from(nil).to(be_a(ProjectSecuritySetting)) - expect(project_without_security_setting.reload.security_setting.pre_receive_secret_detection_enabled) + expect(bottom_level_group_project.reload.security_setting.pre_receive_secret_detection_enabled) .to be(true) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been enabled for group #{bottom_level_group.name} and all of its inherited \ +groups/projects") + expect(AuditEvent.last.details[:target_id]).to eq(bottom_level_group.id) end end -- GitLab From 9ae53744eff6b79932cecd44b9a86687e7e214db Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Wed, 11 Dec 2024 16:41:43 +0000 Subject: [PATCH 15/25] Apply CR suggestions --- doc/user/compliance/audit_event_types.md | 2 +- .../set_group_secret_push_protection_service.rb | 15 ++++++--------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index ff5368ba6257a3..86b100a33abf46 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -486,7 +486,7 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| -| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | A group and some of it's inherited groups/projects secret push protection is updated | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | +| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | Secret push protection is enabled or disabled for a group and its child groups and projects, except those projects specified as exclusions. | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | | [`project_security_exclusion_applied`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is applied in one of the security scanners | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/492465) | Project | | [`project_security_exclusion_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is created | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | | [`project_security_exclusion_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is deleted | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | diff --git a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb index e62964b4ffc28d..ff5318a8beee9f 100644 --- a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -12,8 +12,7 @@ def projects_scope def audit return unless @namespace.is_a?(Group) - filtered_out_projects_full_path = fetch_filtered_out_projects - message = build_group_message(filtered_out_projects_full_path) + message = build_group_message(fetch_filtered_out_projects) audit_context = build_audit_context( name: 'group_secret_push_protection_updated', @@ -26,17 +25,15 @@ def audit def fetch_filtered_out_projects return [] unless @excluded_projects_ids.present? - Project.for_group_and_its_subgroups(@namespace).id_in(@excluded_projects_ids).to_a.map(&:full_path) + projects_scope.id_in(@excluded_projects_ids).to_a.map(&:full_path) end def build_group_message(filtered_out_projects_full_path) - if filtered_out_projects_full_path.empty? - "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ + message = "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ groups/projects" - else - "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ -groups/projects except for #{filtered_out_projects_full_path.join(', ')}" - end + message += " except for #{filtered_out_projects_full_path.join(', ')}" unless filtered_out_projects_full_path.empty? + + message end end end -- GitLab From af5ba54e7f596f8eaf9d10192b8dd62961750ecc Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Wed, 11 Dec 2024 18:44:23 +0200 Subject: [PATCH 16/25] Fix rubocop offense --- .../set_group_secret_push_protection_service.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb index ff5318a8beee9f..1e17922ba36ae2 100644 --- a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -31,7 +31,10 @@ def fetch_filtered_out_projects def build_group_message(filtered_out_projects_full_path) message = "Secret push protection has been enabled for group #{@namespace.name} and all of its inherited \ groups/projects" - message += " except for #{filtered_out_projects_full_path.join(', ')}" unless filtered_out_projects_full_path.empty? + + unless filtered_out_projects_full_path.empty? + message += " except for #{filtered_out_projects_full_path.join(', ')}" + end message end -- GitLab From f7314fbd248526d34d3fd7337d007f9fe97e689d Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Thu, 12 Dec 2024 08:53:25 +0200 Subject: [PATCH 17/25] Add base service tests to fix non implemented methods undercoverage error --- ...ace_secret_push_protection_service_spec.rb | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb diff --git a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb new file mode 100644 index 00000000000000..c402b380f98161 --- /dev/null +++ b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::Configuration::SetNamespaceSecretPushProtectionService, feature_category: :secret_detection do + let_it_be(:user) { create(:user) } + let_it_be(:project_1) { create(:project) } + let(:service) { described_class.new(namespace: project_1, enable: true, current_user: user) } + + describe '#execute' do + context 'when the call is valid' do + it 'executes the transaction and returns the enable value' do + allow(service).to receive_messages(valid_request?: true, projects_scope: Project.id_in(project_1.id), + audit: nil) + expect { service.execute }.to change { + project_1.security_setting.reload.pre_receive_secret_detection_enabled + }.from(false).to(true) + expect(service.execute).to be(true) + end + end + + context 'when the call is invalid' do + it 'does nothing and returns nil' do + allow(service).to receive_messages(valid_request?: false, projects_scope: Project.id_in(project_1.id), + audit: nil) + expect { service.execute }.not_to change { + project_1.security_setting.reload.pre_receive_secret_detection_enabled + } + expect(service.execute).to be_nil + end + end + end + + describe '#valid_request?' do + it 'returns true for valid input' do + valid_service = described_class.new(namespace: project_1, enable: true, current_user: user) + + expect(valid_service.send(:valid_request?)).to be true + end + + it 'returns false for invalid namespace' do + invalid_service = described_class.new(namespace: nil, enable: true, current_user: user) + + expect(invalid_service.send(:valid_request?)).to be false + end + + it 'returns false for invalid enable value' do + invalid_service = described_class.new(namespace: project_1, enable: nil, current_user: user) + + expect(invalid_service.send(:valid_request?)).to be false + end + + it 'returns false for invalid user' do + invalid_service = described_class.new(namespace: project_1, enable: true, current_user: nil) + + expect(invalid_service.send(:valid_request?)).to be false + end + end + + describe '#build_audit_context' do + it 'builds the audit context with correct fields' do + audit_context = service.send(:build_audit_context, name: 'Secret Push Protection', message: 'Enabled') + + expect(audit_context).to include(:name, :author, :scope, :target, :message) + expect(audit_context[:author]).to eq(user) + expect(audit_context[:scope]).to eq(project_1) + expect(audit_context[:message]).to eq('Enabled') + end + end + + describe '#audit' do + it 'requires a subclass overrides it' do + expect { service.send(:audit) }.to raise_error(NotImplementedError) + end + end + + describe '#projects_scope' do + it 'requires a subclass overrides it' do + expect { service.send(:projects_scope) }.to raise_error(NotImplementedError) + end + end +end -- GitLab From bc98e4ea75b555d1f71fb7f6a39fe480ebe523ca Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Thu, 12 Dec 2024 14:52:04 +0200 Subject: [PATCH 18/25] Recompile audit docs --- doc/user/compliance/audit_event_types.md | 2 +- .../audit_events/types/group_secret_push_protection_updated.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 86b100a33abf46..f762e130fd0d04 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -486,7 +486,7 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| -| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | Secret push protection is enabled or disabled for a group and its child groups and projects, except those projects specified as exclusions. | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | +| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | Secret push protection is enabled or disabled for a group and its child groups and projects, except those projects specified as exclusions | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | | [`project_security_exclusion_applied`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is applied in one of the security scanners | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/492465) | Project | | [`project_security_exclusion_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is created | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | | [`project_security_exclusion_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is deleted | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | diff --git a/ee/config/audit_events/types/group_secret_push_protection_updated.yml b/ee/config/audit_events/types/group_secret_push_protection_updated.yml index ef947fd4d2ce02..d81ed799c618e9 100644 --- a/ee/config/audit_events/types/group_secret_push_protection_updated.yml +++ b/ee/config/audit_events/types/group_secret_push_protection_updated.yml @@ -1,6 +1,6 @@ --- name: group_secret_push_protection_updated -description: A group and some of it's inherited groups/projects secret push protection is updated +description: Secret push protection is enabled or disabled for a group and its child groups and projects, except those projects specified as exclusions introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/502829 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412 feature_category: secret_detection -- GitLab From 12c4e7c08e5dc3c3225853670592ecb1cb9d9b0b Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Thu, 12 Dec 2024 18:05:58 +0200 Subject: [PATCH 19/25] Fix audit event description --- doc/user/compliance/audit_event_types.md | 2 +- .../audit_events/types/group_secret_push_protection_updated.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index f762e130fd0d04..b5afc4c99be728 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -486,7 +486,7 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| -| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | Secret push protection is enabled or disabled for a group and its child groups and projects, except those projects specified as exclusions | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | +| [`group_secret_push_protection_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412) | Secret push protection is enabled or disabled for a group and its child groups and projects, except those projects specified as excluded | **{check-circle}** Yes | GitLab [17.7](https://gitlab.com/gitlab-org/gitlab/-/issues/502829) | Group | | [`project_security_exclusion_applied`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is applied in one of the security scanners | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/492465) | Project | | [`project_security_exclusion_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is created | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | | [`project_security_exclusion_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511) | A project security exclusion is deleted | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/492464) | Project | diff --git a/ee/config/audit_events/types/group_secret_push_protection_updated.yml b/ee/config/audit_events/types/group_secret_push_protection_updated.yml index d81ed799c618e9..4e2e9cfa7d1b3a 100644 --- a/ee/config/audit_events/types/group_secret_push_protection_updated.yml +++ b/ee/config/audit_events/types/group_secret_push_protection_updated.yml @@ -1,6 +1,6 @@ --- name: group_secret_push_protection_updated -description: Secret push protection is enabled or disabled for a group and its child groups and projects, except those projects specified as exclusions +description: Secret push protection is enabled or disabled for a group and its child groups and projects, except those projects specified as excluded introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/502829 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174412 feature_category: secret_detection -- GitLab From 5cd8395ca017b3738ef62271a46a579e7cba40f3 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Thu, 12 Dec 2024 18:06:43 +0200 Subject: [PATCH 20/25] Add a comment --- .../configuration/set_project_secret_push_protection_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb index bc49ec0873aef2..6cd1a565446ce1 100644 --- a/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb @@ -6,6 +6,7 @@ class SetProjectSecretPushProtectionService < SetNamespaceSecretPushProtectionSe private def projects_scope + # convert project object into a relation for unified logic in parent class Project.id_in(@namespace.id) end -- GitLab From 532384d8f6e2a71e6f58c6e779db18a3dfb9a85b Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Sun, 15 Dec 2024 13:05:55 +0200 Subject: [PATCH 21/25] Remove unused files --- ...roject_security_setting_changes_auditor.rb | 42 ---------------- ...t_security_setting_changes_auditor_spec.rb | 48 ------------------- 2 files changed, 90 deletions(-) delete mode 100644 ee/lib/projects/project_security_setting_changes_auditor.rb delete mode 100644 ee/spec/lib/projects/project_security_setting_changes_auditor_spec.rb diff --git a/ee/lib/projects/project_security_setting_changes_auditor.rb b/ee/lib/projects/project_security_setting_changes_auditor.rb deleted file mode 100644 index db87efc27f02b2..00000000000000 --- a/ee/lib/projects/project_security_setting_changes_auditor.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -module Projects - class ProjectSecuritySettingChangesAuditor < ::AuditEvents::BaseChangesAuditor - def initialize(current_user:, model:) - super(current_user, model) - end - - def execute - return if model.blank? - - changed_columns = model.previous_changes.except!(:updated_at) - - changed_columns.each_key do |column| - audit_change(column) - end - end - - private - - def audit_change(column) - before, after = attributes_from_auditable_model(column).values_at(:from, :to) - - audit_context = { - name: 'project_security_setting_updated', - author: @current_user, - scope: model.project, - target: model.project, - message: "Changed #{column} from #{before} to #{after}" - } - - ::Gitlab::Audit::Auditor.audit(audit_context) - end - - def attributes_from_auditable_model(column) - { - from: model.previous_changes[column].first, - to: model.previous_changes[column].last - } - end - end -end diff --git a/ee/spec/lib/projects/project_security_setting_changes_auditor_spec.rb b/ee/spec/lib/projects/project_security_setting_changes_auditor_spec.rb deleted file mode 100644 index f445efbf7265e8..00000000000000 --- a/ee/spec/lib/projects/project_security_setting_changes_auditor_spec.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Projects::ProjectSecuritySettingChangesAuditor, feature_category: :groups_and_projects do - using RSpec::Parameterized::TableSyntax - describe '#execute' do - let_it_be(:user) { create(:user) } - let_it_be(:project_security_setting) { create(:project_security_setting) } - - let_it_be(:project_security_setting_auditor_instance) do - described_class.new(current_user: user, model: project_security_setting) - end - - before do - stub_licensed_features(extended_audit_events: true, external_audit_events: true) - end - - context 'when project_security setting is updated' do - where(:column, :event, :change_from, :change_to) do - 'pre_receive_secret_detection_enabled' | 'project_security_setting_updated' | true | false - 'pre_receive_secret_detection_enabled' | 'project_security_setting_updated' | false | true - end - - with_them do - before do - project_security_setting.update!(column.to_sym => change_from) - end - - it 'calls auditor' do - project_security_setting.update!(column.to_sym => change_to) - - expect(Gitlab::Audit::Auditor).to receive(:audit).with( - { - name: event, - author: user, - scope: project_security_setting.project, - target: project_security_setting.project, - message: "Changed #{column} from #{change_from} to #{change_to}" - } - ).and_call_original - - project_security_setting_auditor_instance.execute - end - end - end - end -end -- GitLab From f4bc33866ae4c26206740302a65d308acf996a10 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Sun, 15 Dec 2024 13:50:26 +0200 Subject: [PATCH 22/25] Fix CR comments --- .../set_group_secret_push_protection_service.rb | 2 +- .../set_project_secret_push_protection_service.rb | 2 -- .../set_project_secret_push_protection_service_spec.rb | 8 -------- 3 files changed, 1 insertion(+), 11 deletions(-) diff --git a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb index 1e17922ba36ae2..9109326e1e2e3a 100644 --- a/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -25,7 +25,7 @@ def audit def fetch_filtered_out_projects return [] unless @excluded_projects_ids.present? - projects_scope.id_in(@excluded_projects_ids).to_a.map(&:full_path) + projects_scope.id_in(@excluded_projects_ids).select(:namespace_id, :path).map(&:full_path) end def build_group_message(filtered_out_projects_full_path) diff --git a/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb index 6cd1a565446ce1..7782fc4045b67e 100644 --- a/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb @@ -11,8 +11,6 @@ def projects_scope end def audit - return unless @namespace.is_a?(Project) - message = "Secret push protection has been #{@enable ? 'enabled' : 'disabled'}" audit_context = build_audit_context( name: 'project_security_setting_updated', diff --git a/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb index 9cc5ff99c97208..3a988d4b818810 100644 --- a/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb @@ -40,14 +40,6 @@ def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_p .to change { project_1.reload.security_setting.updated_at } end - it 'doesnt change the attribute for a project when it is in the excluded list' do - security_setting = excluded_project.security_setting - expect do - execute_service(namespace: excluded_project, - enable: !security_setting.pre_receive_secret_detection_enabled) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } - end - describe 'auditing' do context 'when no excluded_projects ids are provided' do it 'audits using the correct message' do -- GitLab From aff0dbf510d8e2f22bb11799eaebdb5ce7686e51 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Sun, 15 Dec 2024 14:13:03 +0200 Subject: [PATCH 23/25] Move audit related tests --- ...t_group_secret_push_protection_service_spec.rb | 15 ++++++++++++--- ...mespace_secret_push_protection_service_spec.rb | 11 ----------- ...project_secret_push_protection_service_spec.rb | 5 ++++- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb index 849a07a6999916..514b828771a58f 100644 --- a/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb @@ -84,33 +84,42 @@ def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_p describe 'auditing' do context 'when no excluded projects ids are provided' do - it 'audits using the correct message' do + it 'audits using the correct properties' do expect { execute_service(namespace: top_level_group, excluded_projects_ids: []) } .to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq( "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ groups/projects") + expect(AuditEvent.last.details[:author_name]).to eq(user.name) + expect(AuditEvent.last.details[:event_name]).to eq("group_secret_push_protection_updated") + expect(AuditEvent.last.details[:target_details]).to eq(top_level_group.name) end end context 'when excluded projects ids are provided' do context 'when excluded ids matches projects in that group' do - it 'audits using the correct message' do + it 'audits using the correct properties' do expect { execute_service(namespace: top_level_group) }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq( "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ groups/projects except for #{excluded_project.full_path}") + expect(AuditEvent.last.details[:author_name]).to eq(user.name) + expect(AuditEvent.last.details[:event_name]).to eq("group_secret_push_protection_updated") + expect(AuditEvent.last.details[:target_details]).to eq(top_level_group.name) end end context 'when excluded ids does not match projects in that group' do - it 'audits using the correct message' do + it 'audits using the correct properties' do expect do execute_service(namespace: top_level_group, excluded_projects_ids: [Time.now.to_i]) end.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq( "Secret push protection has been enabled for group #{top_level_group.name} and all of its inherited \ groups/projects") + expect(AuditEvent.last.details[:author_name]).to eq(user.name) + expect(AuditEvent.last.details[:event_name]).to eq("group_secret_push_protection_updated") + expect(AuditEvent.last.details[:target_details]).to eq(top_level_group.name) end end end diff --git a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb index c402b380f98161..2f1f9b80e80d41 100644 --- a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb @@ -57,17 +57,6 @@ end end - describe '#build_audit_context' do - it 'builds the audit context with correct fields' do - audit_context = service.send(:build_audit_context, name: 'Secret Push Protection', message: 'Enabled') - - expect(audit_context).to include(:name, :author, :scope, :target, :message) - expect(audit_context[:author]).to eq(user) - expect(audit_context[:scope]).to eq(project_1) - expect(audit_context[:message]).to eq('Enabled') - end - end - describe '#audit' do it 'requires a subclass overrides it' do expect { service.send(:audit) }.to raise_error(NotImplementedError) diff --git a/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb index 3a988d4b818810..acc92ce7edf1b3 100644 --- a/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb @@ -42,7 +42,7 @@ def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_p describe 'auditing' do context 'when no excluded_projects ids are provided' do - it 'audits using the correct message' do + it 'audits using the correct properties' do expect { execute_service(namespace: project_2) }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been enabled") @@ -50,6 +50,9 @@ def execute_service(namespace:, enable: true, excluded_projects_ids: [excluded_p AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq("Secret push protection has been disabled") + expect(AuditEvent.last.details[:author_name]).to eq(user.name) + expect(AuditEvent.last.details[:event_name]).to eq("project_security_setting_updated") + expect(AuditEvent.last.details[:target_details]).to eq(project_2.name) end it 'doesnt create audit if no change was made' do -- GitLab From dac6498da1f094c6dfb6f3d352e5d6d6d17e2997 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Sun, 15 Dec 2024 14:14:29 +0200 Subject: [PATCH 24/25] Remove protected method tests --- ...ace_secret_push_protection_service_spec.rb | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb index 2f1f9b80e80d41..7e89a22d54d1ec 100644 --- a/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_namespace_secret_push_protection_service_spec.rb @@ -31,32 +31,6 @@ end end - describe '#valid_request?' do - it 'returns true for valid input' do - valid_service = described_class.new(namespace: project_1, enable: true, current_user: user) - - expect(valid_service.send(:valid_request?)).to be true - end - - it 'returns false for invalid namespace' do - invalid_service = described_class.new(namespace: nil, enable: true, current_user: user) - - expect(invalid_service.send(:valid_request?)).to be false - end - - it 'returns false for invalid enable value' do - invalid_service = described_class.new(namespace: project_1, enable: nil, current_user: user) - - expect(invalid_service.send(:valid_request?)).to be false - end - - it 'returns false for invalid user' do - invalid_service = described_class.new(namespace: project_1, enable: true, current_user: nil) - - expect(invalid_service.send(:valid_request?)).to be false - end - end - describe '#audit' do it 'requires a subclass overrides it' do expect { service.send(:audit) }.to raise_error(NotImplementedError) -- GitLab From 5da85b11d53deb678b58726c72991461aa97f093 Mon Sep 17 00:00:00 2001 From: Gal Katz Date: Sun, 15 Dec 2024 15:13:17 +0200 Subject: [PATCH 25/25] Namespace usage cleanup --- .../set_pre_receive_secret_detection.rb | 16 +-- .../set_secret_push_protection_service.rb | 4 +- .../set_pre_receive_secret_detection_spec.rb | 8 +- ...set_secret_push_protection_service_spec.rb | 107 +++++++++--------- 4 files changed, 61 insertions(+), 74 deletions(-) diff --git a/ee/app/graphql/mutations/security/ci_configuration/set_pre_receive_secret_detection.rb b/ee/app/graphql/mutations/security/ci_configuration/set_pre_receive_secret_detection.rb index a4b016159478de..0332a226cd6a31 100644 --- a/ee/app/graphql/mutations/security/ci_configuration/set_pre_receive_secret_detection.rb +++ b/ee/app/graphql/mutations/security/ci_configuration/set_pre_receive_secret_detection.rb @@ -6,7 +6,7 @@ module CiConfiguration class SetPreReceiveSecretDetection < BaseMutation graphql_name 'SetPreReceiveSecretDetection' - include FindsNamespace + include ResolvesProject description <<~DESC Enable/disable secret push protection for the given project. @@ -27,24 +27,18 @@ class SetPreReceiveSecretDetection < BaseMutation authorize :enable_pre_receive_secret_detection def resolve(namespace_path:, enable:) - namespace = find_namespace(namespace_path) + project = authorized_find!(project_path: namespace_path) response = ::Security::Configuration::SetSecretPushProtectionService - .execute(current_user: current_user, namespace: namespace, enable: enable) + .execute(current_user: current_user, project: project, enable: enable) { pre_receive_secret_detection_enabled: response.payload[:enabled], errors: response.errors } end private - def find_namespace(namespace_path) - namespace = authorized_find!(namespace_path) - # This will be removed following the completion of https://gitlab.com/gitlab-org/gitlab/-/issues/451357 - unless namespace.is_a? Project - raise_resource_not_available_error! 'Setting only available for project namespaces.' - end - - namespace + def find_object(project_path:) + resolve_project(full_path: project_path) end end end diff --git a/ee/app/services/security/configuration/set_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_secret_push_protection_service.rb index 728bfb761f6ba1..cf72a927515b49 100644 --- a/ee/app/services/security/configuration/set_secret_push_protection_service.rb +++ b/ee/app/services/security/configuration/set_secret_push_protection_service.rb @@ -3,13 +3,13 @@ module Security module Configuration class SetSecretPushProtectionService - def self.execute(current_user:, namespace:, enable:) + def self.execute(current_user:, project:, enable:) raise ArgumentError, 'Invalid argument. Either true or false should be passed.' unless [true, false].include?(enable) ServiceResponse.success( payload: { - enabled: SetProjectSecretPushProtectionService.new(current_user: current_user, namespace: namespace, + enabled: SetProjectSecretPushProtectionService.new(current_user: current_user, namespace: project, enable: enable).execute, errors: [] }) diff --git a/ee/spec/requests/api/graphql/mutations/security/configuration/set_pre_receive_secret_detection_spec.rb b/ee/spec/requests/api/graphql/mutations/security/configuration/set_pre_receive_secret_detection_spec.rb index f2f78b2b5c8d93..21b294fe1ee35a 100644 --- a/ee/spec/requests/api/graphql/mutations/security/configuration/set_pre_receive_secret_detection_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/security/configuration/set_pre_receive_secret_detection_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe 'Setting Project and Group Pre Receive Secret Detection', feature_category: :secret_detection do +RSpec.describe 'Setting Project Pre Receive Secret Detection', feature_category: :secret_detection do using RSpec::Parameterized::TableSyntax include GraphqlHelpers @@ -103,11 +103,7 @@ ) end - it 'raises ResourceNotAvailable' do - post_graphql_mutation(mutation, current_user: current_user) - - expect_graphql_errors_to_include('Setting only available for project namespaces.') - end + it_behaves_like 'a mutation that returns a top-level access error' end end end diff --git a/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb index 1a4bee39b60caf..c79b71825d48bd 100644 --- a/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb +++ b/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb @@ -6,74 +6,71 @@ describe '#execute' do let_it_be(:security_setting) { create(:project_security_setting, pre_receive_secret_detection_enabled: false) } let_it_be(:current_user) { create(:user, :admin) } + let_it_be(:project) { security_setting.project } - context 'when namespace is project' do - let_it_be(:namespace) { security_setting.project } - - it 'returns attribute value' do - expect(described_class.execute(current_user: current_user, namespace: namespace, - enable: true)).to have_attributes(errors: be_blank, payload: include(enabled: true)) - expect(described_class.execute(current_user: current_user, namespace: namespace, - enable: false)).to have_attributes(errors: be_blank, payload: include(enabled: false)) - end + it 'returns attribute value' do + expect(described_class.execute(current_user: current_user, project: project, + enable: true)).to have_attributes(errors: be_blank, payload: include(enabled: true)) + expect(described_class.execute(current_user: current_user, project: project, + enable: false)).to have_attributes(errors: be_blank, payload: include(enabled: false)) + end - it 'changes the attribute' do - expect { described_class.execute(current_user: current_user, namespace: namespace, enable: true) } - .to change { security_setting.reload.pre_receive_secret_detection_enabled } - .from(false).to(true) - expect { described_class.execute(current_user: current_user, namespace: namespace, enable: true) } - .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } - expect { described_class.execute(current_user: current_user, namespace: namespace, enable: false) } - .to change { security_setting.reload.pre_receive_secret_detection_enabled } - .from(true).to(false) - expect { described_class.execute(current_user: current_user, namespace: namespace, enable: false) } - .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } - end + it 'changes the attribute' do + expect { described_class.execute(current_user: current_user, project: project, enable: true) } + .to change { security_setting.reload.pre_receive_secret_detection_enabled } + .from(false).to(true) + expect { described_class.execute(current_user: current_user, project: project, enable: true) } + .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + expect { described_class.execute(current_user: current_user, project: project, enable: false) } + .to change { security_setting.reload.pre_receive_secret_detection_enabled } + .from(true).to(false) + expect { described_class.execute(current_user: current_user, project: project, enable: false) } + .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + end - context 'when security_setting record does not yet exist' do - let_it_be(:project_without_security_setting) { create(:project) } + context 'when security_setting record does not yet exist' do + let_it_be(:project_without_security_setting) { create(:project) } - before do - project_without_security_setting.security_setting.delete - end + before do + project_without_security_setting.security_setting.delete + end - it 'creates the necessary record and updates the record appropriately' do - expect(described_class.execute(current_user: current_user, namespace: project_without_security_setting.reload, - enable: true)).to have_attributes(errors: be_blank, payload: include(enabled: true)) - end + it 'creates the necessary record and updates the record appropriately' do + expect(described_class.execute(current_user: current_user, project: project_without_security_setting.reload, + enable: true)).to have_attributes(errors: be_blank, payload: include(enabled: true)) end + end - context 'when attribute changes from false to true' do - it 'creates an audit event with the correct message' do - expect { described_class.execute(current_user: current_user, namespace: namespace, enable: true) } - .to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq( - "Secret push protection has been enabled") - end + context 'when attribute changes from false to true' do + it 'creates an audit event with the correct message' do + expect { described_class.execute(current_user: current_user, project: project, enable: true) } + .to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been enabled") end + end - context 'when attribute changes from true to false' do - let(:security_setting2) { create(:project_security_setting, pre_receive_secret_detection_enabled: true) } - let(:namespace2) { security_setting2.project } + context 'when attribute changes from true to false' do + let(:security_setting2) { create(:project_security_setting, pre_receive_secret_detection_enabled: true) } + let(:project2) { security_setting2.project } - it 'creates an audit event with the correct message' do - expect { described_class.execute(current_user: current_user, namespace: namespace2, enable: false) } - .to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to eq( - "Secret push protection has been disabled") - end + it 'creates an audit event with the correct message' do + expect { described_class.execute(current_user: current_user, project: project2, enable: false) } + .to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq( + "Secret push protection has been disabled") end + end - context 'when fields are invalid' do - it 'returns nil and error' do - expect(described_class.execute(current_user: current_user, namespace: namespace, - enable: nil)).to have_attributes(errors: be_present, payload: include(enabled: nil)) - end + context 'when fields are invalid' do + it 'returns nil and error' do + expect(described_class.execute(current_user: current_user, project: project, + enable: nil)).to have_attributes(errors: be_present, payload: include(enabled: nil)) + end - it 'does not change the attribute' do - expect { described_class.execute(current_user: current_user, namespace: namespace, enable: nil) } - .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } - end + it 'does not change the attribute' do + expect { described_class.execute(current_user: current_user, project: project, enable: nil) } + .not_to change { security_setting.reload.pre_receive_secret_detection_enabled } end end end -- GitLab