diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 8e00ba8bfe082c12d52f2ac9af688663835178a1..b5afc4c99be72880d0df15c139671fff69802266 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -486,6 +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 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/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 a4b016159478de7d202ef593a8b2ea069ed14414..0332a226cd6a3193d39474fb6b8b7802f0d91d23 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_group_secret_push_protection_service.rb b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..9109326e1e2e3a9edcdd38c248d6a1a7c3cf991e --- /dev/null +++ b/ee/app/services/security/configuration/set_group_secret_push_protection_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Security + module Configuration + class SetGroupSecretPushProtectionService < SetNamespaceSecretPushProtectionService + private + + def projects_scope + Project.for_group_and_its_subgroups(@namespace) + end + + def audit + return unless @namespace.is_a?(Group) + + message = build_group_message(fetch_filtered_out_projects) + + 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? + + projects_scope.id_in(@excluded_projects_ids).select(:namespace_id, :path).map(&:full_path) + end + + 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" + + unless filtered_out_projects_full_path.empty? + message += " except for #{filtered_out_projects_full_path.join(', ')}" + end + + 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 index 402732d89cec7970a79ed90bbb505208f8e0ed9f..1a397a14c98d65c52c043f33f78c2be19a6108af 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 @@ -4,45 +4,75 @@ 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 self.execute(namespace:, enable:, excluded_projects_ids: []) - return unless namespace - return unless [true, false].include?(enable) + def execute + return unless valid_request? + any_updated = false 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) - end - else - update_security_setting(Project.id_in(namespace.id).id_not_in(excluded_projects_ids), enable) + projects_scope.each_batch(of: PROJECTS_BATCH_SIZE) do |project_batch| + updated_count = update_security_setting(project_batch.id_not_in(@excluded_projects_ids)) + any_updated ||= updated_count > 0 end + audit if any_updated 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) + protected - create_missing_security_setting(projects, enable) + def valid_request? + @namespace.present? && @current_user.present? && [true, false].include?(@enable) end - def self.create_missing_security_setting(projects, enable) + 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, + pre_receive_secret_detection_enabled: @enable, 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 - private_class_method :create_missing_security_setting, :update_security_setting + 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 + 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 0000000000000000000000000000000000000000..7782fc4045b67e3068fd49bac740ad4d525591aa --- /dev/null +++ b/ee/app/services/security/configuration/set_project_secret_push_protection_service.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Security + module Configuration + class SetProjectSecretPushProtectionService < SetNamespaceSecretPushProtectionService + private + + def projects_scope + # convert project object into a relation for unified logic in parent class + Project.id_in(@namespace.id) + end + + def audit + 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 ad5d998adf38a3140118d9a79809d0ba9930826d..cf72a927515b491e9611ecde76f205dd50a02040 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,28 +3,17 @@ 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 + def self.execute(current_user:, project:, enable:) + 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: SetProjectSecretPushProtectionService.new(current_user: current_user, namespace: project, + 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 67988ee97fa292844c35057cfa8c3be55b656cc0..706e2d86568dd5ff5c34b76963d6be53f712b3c1 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,16 +11,14 @@ class SetGroupSecretPushProtectionWorker feature_category :secret_detection - # rubocop:disable Lint/UnusedMethodArgument -- Added argument for future development, but it is not yet in use - # See https://docs.gitlab.com/ee/development/sidekiq/compatibility_across_updates.html#add-an-argument def perform(group_id, enable, current_user_id = nil, excluded_projects_ids = []) - # rubocop:enable Lint/UnusedMethodArgument group = Group.find_by_id(group_id) + current_user = User.find_by_id(current_user_id) - return unless group + return unless group && current_user - SetNamespaceSecretPushProtectionService.execute(namespace: group, enable: enable, - excluded_projects_ids: excluded_projects_ids) + SetGroupSecretPushProtectionService.new(namespace: group, enable: enable, current_user: current_user, + excluded_projects_ids: excluded_projects_ids).execute end end end 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 0000000000000000000000000000000000000000..4e2e9cfa7d1b3a1f480090a832920f1c3a5a1bb3 --- /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: 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 +milestone: '17.7' +saved_to_database: true +streamed: true +scope: [Group] 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 db87efc27f02b213e6563da0d53b74c91ad5fbb9..0000000000000000000000000000000000000000 --- 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 f445efbf7265e8b37881f2b710fc276c43adddf7..0000000000000000000000000000000000000000 --- 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 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 f2f78b2b5c8d93582d3148076ba253217667a825..21b294fe1ee35a9ceaa808336fba75484a6bc45c 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_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 0000000000000000000000000000000000000000..514b828771a58f6d6c1b9f2f58bf1e5a672c5491 --- /dev/null +++ b/ee/spec/services/security/configuration/set_group_secret_push_protection_service_spec.rb @@ -0,0 +1,154 @@ +# 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 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 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 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 + end + + context 'when security_setting record does not yet exist' do + before do + bottom_level_group_project.security_setting.destroy! + end + + it 'creates security_setting and sets the value appropriately' do + expect { execute_service(namespace: bottom_level_group) }.to change { + bottom_level_group_project.reload.security_setting + }.from(nil).to(be_a(ProjectSecuritySetting)) + + 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 + + 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 index 9f95a257ff1ab0b0d63e7f109161e76b897d9db4..7e89a22d54d1ecdb59de1a6005bd379e9127f3fd 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 @@ -3,176 +3,43 @@ require 'spec_helper' RSpec.describe Security::Configuration::SetNamespaceSecretPushProtectionService, feature_category: :secret_detection do - describe '#execute' do - 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) } - - 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 - 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 } - 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 } - 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 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 } - 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 - - 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) } + 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) } - before do - project_without_security_setting.security_setting.destroy! - 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 } - .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 { described_class.execute(namespace: top_level_group, enable: nil) } - .not_to change { top_level_group_project.reload.security_setting.pre_receive_secret_detection_enabled } - end + 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 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 do - described_class - .execute(namespace: bottom_level_group_project, enable: true) - end.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 do - described_class - .execute(namespace: bottom_level_group_project, enable: false) - end.not_to change { security_setting.reload.pre_receive_secret_detection_enabled } + 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 - it 'changes updated_at timestamp' do - expect { described_class.execute(namespace: mid_level_group_project, enable: true) } - .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]) - end.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_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 { described_class.execute(namespace: project_without_security_setting, enable: true) } - .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 + describe '#audit' do + it 'requires a subclass overrides it' do + expect { service.send(:audit) }.to raise_error(NotImplementedError) + end + end - context 'when arguments are invalid' do - it 'does not change the attribute' do - expect { described_class.execute(namespace: bottom_level_group_project, enable: nil) } - .not_to change { bottom_level_group_project.reload.security_setting.pre_receive_secret_detection_enabled } - 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 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 0000000000000000000000000000000000000000..acc92ce7edf1b372ac8a7a9111d6d39c8911e58d --- /dev/null +++ b/ee/spec/services/security/configuration/set_project_secret_push_protection_service_spec.rb @@ -0,0 +1,99 @@ +# 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 + + describe 'auditing' do + context 'when no excluded_projects ids are provided' 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") + + 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") + 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 + 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) + + 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 + + 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/services/security/configuration/set_secret_push_protection_service_spec.rb b/ee/spec/services/security/configuration/set_secret_push_protection_service_spec.rb index e8a455241ab6ccb7e07891d97cb3658b3256c2bb..c79b71825d48bd63a0eeffc7d5821980c5d85255 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( - "Changed pre_receive_secret_detection_enabled from false to true") - 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( - "Changed pre_receive_secret_detection_enabled from true to false") - 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 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 86cd438c1bbe8144e185d0c22a121277857b9c62..da2b07c38e10bb383c691e3c24b661a2f5d2927b 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,40 +4,57 @@ RSpec.describe Security::Configuration::SetGroupSecretPushProtectionWorker, feature_category: :secret_detection do let_it_be(:group) { create(:group) } + 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, nil, excluded_projects_ids) + described_class.new.perform(group_id, true, user_id, excluded_projects_ids) end before do - allow(Security::Configuration::SetNamespaceSecretPushProtectionService).to receive(:execute) + allow(set_namespace_spp_service).to receive(:execute) + 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(:execute).with( - { enable: true, namespace: group, excluded_projects_ids: excluded_projects_ids } + 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) end end 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(:execute) + expect(Security::Configuration::SetGroupSecretPushProtectionService).not_to have_received(:new) + expect(set_namespace_spp_service).not_to have_received(:execute) + 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, nil, excluded_projects_ids] } + let(:job_args) { [group.id, true, user_id, excluded_projects_ids] } end end end