diff --git a/app/validators/json_schemas/scan_result_policy_project_approval_settings.json b/app/validators/json_schemas/scan_result_policy_project_approval_settings.json index b4bebed3d1cb1e69baf52a17415bfa6058945cf7..01b7d4f78aa0dec561d71c800b7ae32a132d825e 100644 --- a/app/validators/json_schemas/scan_result_policy_project_approval_settings.json +++ b/app/validators/json_schemas/scan_result_policy_project_approval_settings.json @@ -15,6 +15,9 @@ "require_password_to_approve": { "type": "boolean" }, + "require_reauthentication_to_approve": { + "type": "boolean" + }, "block_branch_modification": { "type": "boolean" }, diff --git a/doc/api/merge_request_approvals.md b/doc/api/merge_request_approvals.md index 20ccc2af3234af1027a8db1d719cf0404a5932c8..fb238fe8d8b2c5c3935edbccad55c073015cf669 100644 --- a/doc/api/merge_request_approvals.md +++ b/doc/api/merge_request_approvals.md @@ -263,7 +263,8 @@ Supported attributes: "disable_overriding_approvers_per_merge_request": false, "merge_requests_author_approval": true, "merge_requests_disable_committers_approval": false, - "require_password_to_approve": true + "require_password_to_approve": true, // Deprecated in 16.9, use require_reauthentication_to_approve instead + "require_reauthentication_to_approve": true } ``` @@ -284,7 +285,8 @@ Supported attributes: | `disable_overriding_approvers_per_merge_request` | boolean | No | Allow or prevent overriding approvers per merge request. | | `merge_requests_author_approval` | boolean | No | Allow or prevent authors from self approving merge requests; `true` means authors can self approve. | | `merge_requests_disable_committers_approval` | boolean | No | Allow or prevent committers from self approving merge requests. | -| `require_password_to_approve` | boolean | No | Require approver to enter a password to authenticate before adding the approval. | +| `require_password_to_approve` (deprecated) | boolean | No | Require approver to enter a password to authenticate before adding the approval. [Deprecated](https://gitlab.com/gitlab-org/gitlab/-/issues/431346) in GitLab 16.9. Use `require_reauthentication_to_approve` instead. | +| `require_reauthentication_to_approve` | boolean | No | Require approver to enter a to authenticate before adding the approval. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/431346) in GitLab 17.1. | | `reset_approvals_on_push` | boolean | No | Reset approvals on a new push. | | `selective_code_owner_removals` | boolean | No | Reset approvals from Code Owners if their files changed. You must disable the `reset_approvals_on_push` field to use this field. | @@ -296,7 +298,8 @@ Supported attributes: "disable_overriding_approvers_per_merge_request": false, "merge_requests_author_approval": false, "merge_requests_disable_committers_approval": false, - "require_password_to_approve": true + "require_password_to_approve": true, + "require_reauthentication_to_approve": true } ``` @@ -1232,7 +1235,7 @@ Supported attributes: | Attribute | Type | Required | Description | |---------------------|-------------------|----------|-------------| | `id` | integer or string | Yes | The ID or [URL-encoded path of a project](rest/index.md#namespaced-path-encoding). | -| `approval_password` | string | No | Current user's password. Required if [**Require user password to approve**](../user/project/merge_requests/approvals/settings.md#require-user-re-authentication-to-approve) is enabled in the project settings. | +| `approval_password` | string | No | Current user's password. Required if [**Require user re-authentication to approve**](../user/project/merge_requests/approvals/settings.md#require-user-re-authentication-to-approve) is enabled in the project settings. Always fails if the group or self-managed instance is configured to force SAML authentication. | | `merge_request_iid` | integer | Yes | The IID of the merge request. | | `sha` | string | No | The `HEAD` of the merge request. | diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 6bd3d028095753f1f7aba10d7323e9b25ce134e8..4891f0a5f96fac9cd0ceffb670e0c28466a3556a 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -455,6 +455,7 @@ Audit event types belong to the following product categories. | [`protected_branch_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is removed| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) | Project | | [`protected_branch_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107530) | Event triggered on the setting for protected branches is update| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/369318) | Project | | [`repository_git_operation`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76719) | Triggered when authenticated users push, pull, or clone a project using SSH, HTTP(S), or the UI| **{dotted-circle}** No | **{check-circle}** Yes | GitLab [14.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373950) | Project | +| [`require_reauthentication_to_approve_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/150710) | Logged when the setting for requiring reauthentication for merge requqest approvals is toggled.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.1](https://gitlab.com/gitlab-org/gitlab/-/issues/431346) | Group, Project | | [`manually_trigger_housekeeping`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112095) | Triggered when manually triggering housekeeping via API or admin UI| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/390761) | Project | | [`project_blobs_removal`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/152522) | Triggered when removing blobs via the GraphQL API or project settings UI| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.0](https://gitlab.com/gitlab-org/gitlab/-/issues/450701) | Project | | [`project_text_replacement`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/152522) | Triggered when replacing text via the GraphQL API or project settings UI| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.1](https://gitlab.com/gitlab-org/gitlab/-/issues/450701) | Project | diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index bc1052116146b7c599f59001abfa79b6b69f7b18..3a0c933e92c2b191ac138fc7fe0df573077f10b7 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -385,6 +385,9 @@ def lock_for_confirmation!(id) :allow_pipeline_trigger_approve_deployment=, :product_analytics_instrumentation_key, to: :project_setting + with_options prefix: :delegated, to: :project_setting do + delegate :require_reauthentication_to_approve= + end delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :project_state) @@ -1266,6 +1269,27 @@ def supports_saved_replies? ::Feature.enabled?(:project_saved_replies_flag, self, type: :beta) && licensed_feature_available?(:project_saved_replies) end + # Temporary code to facilitate: https://gitlab.com/gitlab-org/gitlab/-/issues/431346 + def require_password_to_approve=(status) + write_attribute(:require_password_to_approve, status) + self.delegated_require_reauthentication_to_approve = status + end + + # Temporary code to facilitate: https://gitlab.com/gitlab-org/gitlab/-/issues/431346 + def require_reauthentication_to_approve=(status) + write_attribute(:require_password_to_approve, status) + self.delegated_require_reauthentication_to_approve = status + end + + # Temporary code to facilitate: https://gitlab.com/gitlab-org/gitlab/-/issues/431346 + def require_reauthentication_to_approve + require_password_to_approve + end + + def require_reauthentication_to_approve? + !!require_reauthentication_to_approve + end + private def latest_ingested_sbom_pipeline_id_redis_key diff --git a/ee/app/models/group_merge_request_approval_setting.rb b/ee/app/models/group_merge_request_approval_setting.rb index 039a71745df2063359b7be0d247c2db997ae3390..4adea584d823a301a1dd42b7c4ae1418caee695c 100644 --- a/ee/app/models/group_merge_request_approval_setting.rb +++ b/ee/app/models/group_merge_request_approval_setting.rb @@ -11,7 +11,8 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord :allow_overrides_to_approver_list_per_merge_request, :retain_approvals_on_push, :require_password_to_approve, - :require_saml_auth_to_approve, # I think this is unused + :require_saml_auth_to_approve, # TODO: Deprecated, can be removed along with DB column + :require_reauthentication_to_approve, inclusion: { in: [true, false], message: N_('must be a boolean value') } scope :find_or_initialize_by_group, ->(group) { find_or_initialize_by(group: group) } @@ -22,10 +23,25 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord allow_overrides_to_approver_list_per_merge_request: 'prevent users from modifying MR approval rules in merge requests', retain_approvals_on_push: 'require new approvals when new commits are added to an MR', - require_password_to_approve: 'require user password for approvals' + require_password_to_approve: 'require user password for approvals', + require_reauthentication_to_approve: 'require user authentication for approvals' }.freeze def selective_code_owner_removals false end + + def require_password_to_approve=(status) + write_attribute(:require_password_to_approve, status) + write_attribute(:require_reauthentication_to_approve, status) + end + + def require_reauthentication_to_approve=(status) + write_attribute(:require_reauthentication_to_approve, status) + write_attribute(:require_password_to_approve, status) + end + + def require_reauthentication_to_approve + require_password_to_approve + end end diff --git a/ee/app/services/merge_request_approval_settings/update_service.rb b/ee/app/services/merge_request_approval_settings/update_service.rb index a0fdebc4d952d4dff9c5ded899ef2d8b13ea3238..db06dbb71180ce28e873ba002031e7050030d002 100644 --- a/ee/app/services/merge_request_approval_settings/update_service.rb +++ b/ee/app/services/merge_request_approval_settings/update_service.rb @@ -18,23 +18,6 @@ def execute ServiceResponse.error(message: setting.errors.messages) end when Project - resolved_params = { - merge_requests_author_approval: params[:allow_author_approval], - merge_requests_disable_committers_approval: !params[:allow_committer_approval], - disable_overriding_approvers_per_merge_request: !params[:allow_overrides_to_approver_list_per_merge_request], - reset_approvals_on_push: !params[:retain_approvals_on_push], - project_setting_attributes: { - selective_code_owner_removals: params[:selective_code_owner_removals] || false - }, - require_password_to_approve: params[:require_password_to_approve] - } - - approval_removal_settings = MergeRequest::ApprovalRemovalSettings.new( - container, - !params[:retain_approvals_on_push], - params[:selective_code_owner_removals] - ) - unless approval_removal_settings.valid? return ServiceResponse.error(message: _('selective_code_owner_removals can only be enabled when retain_approvals_on_push is enabled')) @@ -53,6 +36,28 @@ def execute private + def resolved_params + @resolved_params ||= { + merge_requests_author_approval: params[:allow_author_approval], + merge_requests_disable_committers_approval: !params[:allow_committer_approval], + disable_overriding_approvers_per_merge_request: !params[:allow_overrides_to_approver_list_per_merge_request], + reset_approvals_on_push: !params[:retain_approvals_on_push], + require_password_to_approve: params[:require_password_to_approve], + project_setting_attributes: { + selective_code_owner_removals: params[:selective_code_owner_removals] || false, + require_reauthentication_to_approve: params[:require_reauthentication_to_approve] + } + } + end + + def approval_removal_settings + MergeRequest::ApprovalRemovalSettings.new( + container, + !params[:retain_approvals_on_push], + params[:selective_code_owner_removals] + ) + end + def allowed? can?(current_user, :admin_merge_request_approval_settings, container) end diff --git a/ee/config/audit_events/types/require_reauthentication_to_approve_updated.yml b/ee/config/audit_events/types/require_reauthentication_to_approve_updated.yml new file mode 100644 index 0000000000000000000000000000000000000000..aacfa87e6495b7cf82463c46433c7e051c17c868 --- /dev/null +++ b/ee/config/audit_events/types/require_reauthentication_to_approve_updated.yml @@ -0,0 +1,10 @@ +--- +name: require_reauthentication_to_approve_updated +description: Logged when the setting for requiring reauthentication for merge requqest approvals is toggled. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/431346 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/150710 +feature_category: source_code_management +milestone: '17.1' +saved_to_database: true +streamed: true +scope: [Group, Project] diff --git a/ee/lib/api/entities/merge_request_approval_setting.rb b/ee/lib/api/entities/merge_request_approval_setting.rb index e4c7817b2a6cf72be6a2da63081fe59f8fb2f957..74f7f823d2ad5a036fa42a661b216c10df5eaa99 100644 --- a/ee/lib/api/entities/merge_request_approval_setting.rb +++ b/ee/lib/api/entities/merge_request_approval_setting.rb @@ -9,6 +9,7 @@ class MergeRequestApprovalSetting < Grape::Entity expose :retain_approvals_on_push, documentation: { type: 'boolean', example: true } expose :selective_code_owner_removals, documentation: { type: 'boolean', example: true } expose :require_password_to_approve, documentation: { type: 'boolean', example: true } + expose :require_reauthentication_to_approve, documentation: { type: 'boolean', example: true } end end end diff --git a/ee/lib/api/merge_request_approval_settings.rb b/ee/lib/api/merge_request_approval_settings.rb index 80541748fe6167ab3c5c9a1e460d1f8b40283f49..2de31f15dee4db65ba2334dac4f125cdbef66cb0 100644 --- a/ee/lib/api/merge_request_approval_settings.rb +++ b/ee/lib/api/merge_request_approval_settings.rb @@ -19,12 +19,32 @@ class MergeRequestApprovalSettings < ::API::Base optional :require_password_to_approve, type: Boolean, desc: 'Require approver to authenticate before approving', allow_blank: false + optional :require_reauthentication_to_approve, + type: Boolean, desc: 'Require approver to authenticate before approving', allow_blank: false + at_least_one_of :allow_author_approval, :allow_committer_approval, :allow_overrides_to_approver_list_per_merge_request, :retain_approvals_on_push, :selective_code_owner_removals, - :require_password_to_approve + :require_password_to_approve, + :require_reauthentication_to_approve + end + + # If the new version of the parameter is used mirror it to the old param + # in order to keep both field in sync until full removal + # Mirror reauthentication to password for approval + # https://gitlab.com/gitlab-org/gitlab/-/issues/431346 + def setting_params + normalized_params = declared_params(include_missing: false) + + if normalized_params.key?(:require_reauthentication_to_approve) + normalized_params[:require_password_to_approve] = normalized_params[:require_reauthentication_to_approve] + elsif normalized_params.key?(:require_password_to_approve) + normalized_params[:require_reauthentication_to_approve] = normalized_params[:require_password_to_approve] + end + + normalized_params end end @@ -61,8 +81,6 @@ class MergeRequestApprovalSettings < ::API::Base use :merge_request_approval_settings end put do - setting_params = declared_params(include_missing: false) - response = ::MergeRequestApprovalSettings::UpdateService .new(container: user_project, current_user: current_user, params: setting_params).execute @@ -112,8 +130,6 @@ class MergeRequestApprovalSettings < ::API::Base use :merge_request_approval_settings end put do - setting_params = declared_params(include_missing: false) - response = ::MergeRequestApprovalSettings::UpdateService .new(container: user_group, current_user: current_user, params: setting_params).execute diff --git a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb index 325ecdee5ea0a2de81effdbf29796821ea8e3157..4022090fdd97e655028b21d0a2a37ddef3c1aa62 100644 --- a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -44,7 +44,7 @@ def audit_new_record(column, description) end def should_audit_params_column?(column) - if column == :require_password_to_approve + if column == :require_password_to_approve || column == :require_reauthentication_to_approve @params[column] else # we are comparing with false here because on UI we show negative statements. @@ -55,7 +55,7 @@ def should_audit_params_column?(column) end def attributes_from_auditable_model(column) - if column == :require_password_to_approve + if column == :require_password_to_approve || column == :require_reauthentication_to_approve { from: model.previous_changes[column].first, to: model.previous_changes[column].last diff --git a/ee/lib/audit/project_setting_changes_auditor.rb b/ee/lib/audit/project_setting_changes_auditor.rb index ee552cc85321d9a6d1a606f0ed705d3bc0b1dab9..f084208825777e64af9f698ceddc302a32061636 100644 --- a/ee/lib/audit/project_setting_changes_auditor.rb +++ b/ee/lib/audit/project_setting_changes_auditor.rb @@ -46,6 +46,13 @@ def execute model: model, event_type: 'selective_code_owner_removals_updated' ) + audit_changes( + :require_reauthentication_to_approve, + as: 'require_reauthentication_to_approve', + entity: @project, + model: model, + event_type: 'require_reauthentication_to_approve_updated' + ) end def attributes_from_auditable_model(column) diff --git a/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb b/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb index a05720a6b326792cba08d94c4616b74852dcf646..23256963509e698213815d9b4e3dea105f2f0d0f 100644 --- a/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb +++ b/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb @@ -14,7 +14,8 @@ def execute allow_overrides_to_approver_list_per_merge_request: allow_overrides_to_approver_list_per_merge_request, retain_approvals_on_push: retain_approvals_on_push, selective_code_owner_removals: selective_code_owner_removals, - require_password_to_approve: require_password_to_approve + require_password_to_approve: require_password_to_approve, + require_reauthentication_to_approve: require_reauthentication_to_approve } end @@ -59,6 +60,17 @@ def selective_code_owner_removals ) end + def require_reauthentication_to_approve + group_value = group_settings&.require_reauthentication_to_approve + project_value = @project && @project.project_setting.read_attribute(:require_reauthentication_to_approve) + + ComplianceManagement::MergeRequestApprovalSettings::Setting.new( + value: require_password_value(group_value, project_value), + locked: require_password_locked?(group_value, false), + inherited_from: (:group if require_password_locked?(group_value, false)) + ) + end + def require_password_to_approve group_value = group_settings&.require_password_to_approve project_value = @project && @project.read_attribute(:require_password_to_approve) @@ -80,6 +92,7 @@ def group_settings @group&.group_merge_request_approval_setting end + # TODO: rename to require_reauthentication_value after: https://gitlab.com/gitlab-org/gitlab/-/issues/431346 def require_password_value(group_value, project_value) [group_value, project_value].any? end diff --git a/ee/lib/ee/api/entities/approval_settings.rb b/ee/lib/ee/api/entities/approval_settings.rb index 7e54d530e31553ed48a99647c67a7f2ccdaf28bc..d392c28940f6f2d3a5faf70a7590abee96b170ee 100644 --- a/ee/lib/ee/api/entities/approval_settings.rb +++ b/ee/lib/ee/api/entities/approval_settings.rb @@ -22,6 +22,9 @@ class ApprovalSettings < Grape::Entity expose :require_password_to_approve?, as: :require_password_to_approve + + expose :require_reauthentication_to_approve?, + as: :require_reauthentication_to_approve end end end diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json b/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json index af1b84f1798f6fcec052ad6d775d07cdf326d0d7..a2d21f86bcab1b77e7e771521df1a1d4cf9ccfbb 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json @@ -77,6 +77,17 @@ "inherited_from": { "type": "string" } + }, + "require_reauthentication_to_approve": { + "value": { + "type": "boolean" + }, + "locked": { + "type": "boolean" + }, + "inherited_from": { + "type": "string" + } } }, "additionalProperties": false diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json index 83813baf1a681dcd68b69e2e74a4cb8ba892d2be..196dfe5646e5277effd408b132381064d1356a88 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json @@ -28,6 +28,12 @@ "null" ] }, + "require_reauthentication_to_approve": { + "type": [ + "boolean", + "null" + ] + }, "require_password_to_approve": { "type": [ "boolean", diff --git a/ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb b/ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb index 76c4e0e3e0ec825d039c46e8e1de9c3174f168a5..c9fcb9a11d5dc3e2b70b79bd777cdbd4100eadb7 100644 --- a/ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb +++ b/ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb @@ -15,7 +15,8 @@ :allow_overrides_to_approver_list_per_merge_request, :retain_approvals_on_push, :selective_code_owner_removals, - :require_password_to_approve + :require_password_to_approve, + :require_reauthentication_to_approve ] ) end diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb index 25fc70b733944d0daeed1263034fb62ae4d6aec0..9a1e8e91614d04aedbfad4459d19b9fa6cf2d9cb 100644 --- a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Audit::GroupMergeRequestApprovalSettingChangesAuditor do +RSpec.describe Audit::GroupMergeRequestApprovalSettingChangesAuditor, feature_category: :compliance_management do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } @@ -12,6 +12,7 @@ allow_committer_approval: false, allow_overrides_to_approver_list_per_merge_request: false, retain_approvals_on_push: false, + require_reauthentication_to_approve: true, require_password_to_approve: true } end @@ -20,14 +21,18 @@ subject { described_class.new(user, approval_setting, params) } it 'creates audit events' do - expect { subject.execute }.to change { AuditEvent.count }.by(5) - expect(AuditEvent.last(5).map { |e| e.details[:custom_message] }) + expect { subject.execute }.to change { AuditEvent.count }.by(6) + + events = AuditEvent.last(6).map { |e| e.details[:custom_message] } + + expect(events.sort) .to match_array ["Changed prevent merge request approval from committers from false to true", "Changed prevent users from modifying MR approval rules in merge requests "\ "from false to true", "Changed prevent merge request approval from authors from false to true", "Changed require new approvals when new commits are added to an MR from false to true", - "Changed require user password for approvals from false to true"] + "Changed require user authentication for approvals from false to true", + "Changed require user password for approvals from false to true"].sort end end @@ -40,6 +45,7 @@ allow_committer_approval: false, allow_overrides_to_approver_list_per_merge_request: false, retain_approvals_on_push: false, + require_reauthentication_to_approve: false, require_password_to_approve: false ) end @@ -47,24 +53,31 @@ let_it_be(:subject) { described_class.new(user, approval_setting, {}) } ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| - it 'creates an audit event' do + it "creates an audit event for #{column}" do approval_setting.update_attribute(column, true) - expect { subject.execute }.to change { AuditEvent.count }.by(1) + if column == :require_password_to_approve || column == :require_reauthentication_to_approve + # both values shoudl be kept in sync + expect { subject.execute }.to change { AuditEvent.count }.by(2) - if column == :require_password_to_approve - expect(AuditEvent.last.details).to include({ change: desc, from: false, to: true }) + last_two = AuditEvent.last(2) + reauth = last_two.detect do |event| + event.details[:event_name] == "require_reauthentication_to_approve_updated" + end + password = last_two.detect { |event| event.details[:event_name] == "require_password_to_approve_updated" } + if column == :require_reauthentication_to_approve + expect(reauth.details).to include({ change: desc, from: false, to: true }) + else + expect(password.details).to include({ change: desc, from: false, to: true }) + end else + expect(::Gitlab::Audit::Auditor) + .to receive(:audit).with(hash_including({ name: "#{column}_updated" })).and_call_original + + expect { subject.execute }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details).to include({ change: desc, from: true, to: false }) end end - - it 'passes correct event type to auditor' do - expect(::Gitlab::Audit::Auditor) - .to receive(:audit).with(hash_including({ name: "#{column}_updated" })).and_call_original - - subject.execute - end end end end diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index a88da3c0365b68be7cc9aea440788280cf82d9af..587db8521cad8845387721c7f70459e64bf5ad2b 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -222,9 +222,55 @@ it_behaves_like 'project_audit_events_from_to' end + context 'when project require_reauthentication_to_approve is updated' do + let(:change_from) { false } + let(:change_to) { true } + + before do + project.update!(require_reauthentication_to_approve: true) + end + + it 'audits require_reauthentication_to_approve and require_password_to_approve in sync' do + pw_change = "require user password for approvals" + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + { + name: "project_require_password_to_approve_updated", + author: user, + scope: project, + target: project, + message: "Changed #{pw_change} from #{change_from} to #{change_to}", + additional_details: { + change: pw_change.to_s, + from: change_from, + target_details: project.full_path.to_s, + to: change_to + }, + target_details: project.full_path.to_s + } + ).and_call_original + + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + { + name: 'require_reauthentication_to_approve_updated', + author: user, + scope: project, + target: project.project_setting, + message: "Changed require_reauthentication_to_approve from false to true", + additional_details: { + change: 'require_reauthentication_to_approve', + from: change_from, + target_details: project.full_path.to_s, + to: change_to + }, + target_details: project.full_path.to_s + } + ).and_call_original + + auditor_instance.execute + end + end + context 'when project require_password_to_approve is updated' do - let(:change) { "require user password for approvals" } - let(:event) { "project_require_password_to_approve_updated" } let(:change_from) { false } let(:change_to) { true } @@ -232,7 +278,44 @@ project.update!(require_password_to_approve: true) end - it_behaves_like 'project_audit_events_from_to' + it 'audits require_password_to_approve and require_reauthentication_to_approve in sync' do + pw_change = "require user password for approvals" + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + { + name: "project_require_password_to_approve_updated", + author: user, + scope: project, + target: project, + message: "Changed #{pw_change} from #{change_from} to #{change_to}", + additional_details: { + change: pw_change.to_s, + from: change_from, + target_details: project.full_path.to_s, + to: change_to + }, + target_details: project.full_path.to_s + } + ).and_call_original + + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + { + name: 'require_reauthentication_to_approve_updated', + author: user, + scope: project, + target: project.project_setting, + message: "Changed require_reauthentication_to_approve from false to true", + additional_details: { + change: 'require_reauthentication_to_approve', + from: change_from, + target_details: project.full_path.to_s, + to: change_to + }, + target_details: project.full_path.to_s + } + ).and_call_original + + auditor_instance.execute + end end context 'when project disable_overriding_approvers_per_merge_request is updated' do diff --git a/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb b/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb index 725317af0a89b3effe2d349c080824fa34bcb952..704019c6341f3ba6760586d7a0f2e1ac9f74b97a 100644 --- a/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb +++ b/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb @@ -224,4 +224,41 @@ it_behaves_like 'a MR approval setting' end end + + describe '#require_reauthentication_to_approve' do + where(:group_requires_reauth, :project_requires_reauth, :value, :locked, :inherited_from) do + true | nil | true | false | nil + false | nil | false | false | nil + + # Cases which do not include a group + nil | true | true | false | nil + nil | false | false | false | nil + + # Cases with a project + true | false | true | true | :group + true | true | true | true | :group + false | false | false | false | nil + false | true | true | false | nil + end + + with_them do + before do + group.group_merge_request_approval_setting + .update!(require_reauthentication_to_approve: !!group_requires_reauth) + project.update!(require_reauthentication_to_approve: project_requires_reauth) + end + + let(:object) do + if project_requires_reauth.nil? + described_class.new(group).require_reauthentication_to_approve + elsif group_requires_reauth.nil? + described_class.new(nil, project: project).require_reauthentication_to_approve + else + described_class.new(group, project: project).require_reauthentication_to_approve + end + end + + it_behaves_like 'a MR approval setting' + end + end end diff --git a/ee/spec/models/ee/project_spec.rb b/ee/spec/models/ee/project_spec.rb index 7859b37ba9443997349784dbb4c455031b72bbef..d52a622b91a5f0d0f5dbe6ab976d6f6868ac786f 100644 --- a/ee/spec/models/ee/project_spec.rb +++ b/ee/spec/models/ee/project_spec.rb @@ -1203,6 +1203,47 @@ end end + describe '#require_reauthentication_to_approve?' do + let_it_be(:root_ancestor) { create(:group) } + let_it_be(:sub_group) { create(:group, parent: root_ancestor) } + + before do + allow(project).to receive(:group).and_return(sub_group) + end + + it 'returns true when the resolver returns true' do + expect(ComplianceManagement::MergeRequestApprovalSettings::Resolver).to receive(:new).with(root_ancestor, project: project) + + allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| + # internally still maps to require_password_to_approve so mock that call + allow(resolver).to receive(:require_password_to_approve) + .and_return(ComplianceManagement::MergeRequestApprovalSettings::Setting.new( + value: true, + locked: false, + inherited_from: nil + )) + end + + expect(project.require_reauthentication_to_approve).to be true + end + + it 'returns false when the resolver returns false' do + expect(ComplianceManagement::MergeRequestApprovalSettings::Resolver).to receive(:new).with(root_ancestor, project: project) + + allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| + # internally still maps to require_password_to_approve so mock that call + allow(resolver).to receive(:require_password_to_approve) + .and_return(ComplianceManagement::MergeRequestApprovalSettings::Setting.new( + value: false, + locked: false, + inherited_from: nil + )) + end + + expect(project.require_reauthentication_to_approve).to be false + end + end + describe '#require_password_to_approve?' do let_it_be(:root_ancestor) { create(:group) } let_it_be(:sub_group) { create(:group, parent: root_ancestor) } @@ -1240,6 +1281,43 @@ expect(project.require_password_to_approve).to be false end + + it 'sets require_reauthentication_to_approve along with require_password_to_approve' do + project.require_password_to_approve = false + + expect(project.project_setting.require_reauthentication_to_approve).to be_falsy + expect(project.require_password_to_approve).to be_falsy + + project.require_password_to_approve = true + + expect(project.project_setting.require_reauthentication_to_approve).to be_truthy + + project.save! + project.reload + + # persisted the change + expect(project.project_setting.require_reauthentication_to_approve).to be_truthy + expect(project.require_password_to_approve).to be_truthy + end + + it 'sets require_password_to_approve along with require_reauthentication_to_approve' do + project.require_reauthentication_to_approve = false + + expect(project.project_setting.require_reauthentication_to_approve).to be_falsy + expect(project.require_password_to_approve).to be_falsy + + project.require_reauthentication_to_approve = true + + expect(project.require_password_to_approve).to be_truthy + expect(project.project_setting.require_reauthentication_to_approve).to be_truthy + + project.save! + project.reload + + # persisted the change + expect(project.project_setting.require_reauthentication_to_approve).to be_truthy + expect(project.require_password_to_approve).to be_truthy + end end describe '#merge_requests_author_approval' do diff --git a/ee/spec/models/group_merge_request_approval_setting_spec.rb b/ee/spec/models/group_merge_request_approval_setting_spec.rb index bfe3edc514084b27dbe2542a7b3b86714cb4291f..b4526a3e95145452fa27b885d7d97d7499ea72db 100644 --- a/ee/spec/models/group_merge_request_approval_setting_spec.rb +++ b/ee/spec/models/group_merge_request_approval_setting_spec.rb @@ -10,7 +10,7 @@ describe 'Validations' do let_it_be(:setting) { create(:group_merge_request_approval_setting) } - subject { setting } + subject(:approval_setting) { setting } options = [true, false] it { is_expected.to validate_presence_of(:group) } @@ -19,12 +19,13 @@ it { is_expected.to validate_inclusion_of(:allow_overrides_to_approver_list_per_merge_request).in_array(options) } it { is_expected.to validate_inclusion_of(:retain_approvals_on_push).in_array(options) } it { is_expected.to validate_inclusion_of(:require_password_to_approve).in_array(options) } + it { is_expected.to validate_inclusion_of(:require_reauthentication_to_approve).in_array(options) } end describe '.find_or_initialize_by_group' do let_it_be(:group) { create(:group) } - subject { described_class.find_or_initialize_by_group(group) } + subject(:approval_setting_for_group) { described_class.find_or_initialize_by_group(group) } context 'with no existing setting' do it { is_expected.to be_a_new_record } @@ -36,4 +37,47 @@ it { is_expected.to eq(setting) } end end + + describe 'require authentication for approval' do + let(:setting) { build(:group_merge_request_approval_setting) } + + it 'sets require_reauthentication_to_approve along with require_password_to_approve' do + setting.require_password_to_approve = false + + expect(setting.require_reauthentication_to_approve).to be_falsy + expect(setting.require_password_to_approve).to be_falsy + + setting.require_password_to_approve = true + + expect(setting.require_reauthentication_to_approve).to be_truthy + expect(setting.require_password_to_approve).to be_truthy + + setting.save! + setting.reload + + # persisted the change + expect(setting.require_reauthentication_to_approve).to be_truthy + expect(setting.require_password_to_approve).to be_truthy + end + + it 'sets require_password_to_approve along with require_reauthentication_to_approve' do + setting.require_reauthentication_to_approve = false + + expect(setting.require_reauthentication_to_approve).to be_falsy + expect(setting.require_password_to_approve).to be_falsy + + setting.require_reauthentication_to_approve = true + + expect(setting.require_password_to_approve).to be_truthy + expect(setting.require_reauthentication_to_approve).to be_truthy + + expect(setting).to be_valid + setting.save! + setting.reload + + # persisted the change + expect(setting.require_password_to_approve).to be_truthy + expect(setting.require_reauthentication_to_approve).to be_truthy + end + end end diff --git a/ee/spec/requests/api/merge_request_approval_settings_spec.rb b/ee/spec/requests/api/merge_request_approval_settings_spec.rb index e2af870fef229703535ba446d0764fb371d84de4..68f8f825716746e774d32210c0f38cdd12db868a 100644 --- a/ee/spec/requests/api/merge_request_approval_settings_spec.rb +++ b/ee/spec/requests/api/merge_request_approval_settings_spec.rb @@ -95,6 +95,7 @@ expect(json_response['retain_approvals_on_push']['value']).to eq(true) expect(json_response['selective_code_owner_removals']['value']).to eq(false) expect(json_response['require_password_to_approve']['value']).to eq(false) + expect(json_response['require_reauthentication_to_approve']['value']).to eq(false) end end @@ -116,7 +117,7 @@ describe 'PUT /groups/:id/merge_request_approval_setting' do let(:url) { "/groups/#{group.id}/merge_request_approval_setting" } - let(:params) { { allow_author_approval: true } } + let(:params) { { require_reauthentication_to_approve: true, allow_author_approval: true } } before do allow(Ability).to receive(:allowed?).and_call_original @@ -134,6 +135,7 @@ expect(response).to have_gitlab_http_status(:ok) expect(json_response['allow_author_approval']['value']).to eq(true) + expect(json_response['require_reauthentication_to_approve']['value']).to eq(true) end it 'matches the response schema' do @@ -142,6 +144,35 @@ expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee') end + context 'when password and reauthentication are set' do + using RSpec::Parameterized::TableSyntax + where(:reauth, :password, :outcome) do + false | false | false + false | nil | false + false | true | false + nil | false | false + nil | true | true + true | false | true + true | nil | true + true | true | true + end + + with_them do + let(:params) { {} } + + it "sets with precedence for require_reauthentication_to_approve and mirors" do + params[:require_password_to_approve] = password unless password.nil? + params[:require_reauthentication_to_approve] = reauth unless reauth.nil? + + put api(url, user), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['require_password_to_approve']['value']).to eq(outcome) + expect(json_response['require_reauthentication_to_approve']['value']).to eq(outcome) + end + end + end + context 'when update fails' do let(:params) { { allow_author_approval: 45 } } @@ -205,7 +236,8 @@ merge_requests_disable_committers_approval: nil, disable_overriding_approvers_per_merge_request: nil, reset_approvals_on_push: nil, - require_password_to_approve: nil + require_password_to_approve: nil, + require_reauthentication_to_approve: nil ) end @@ -219,6 +251,7 @@ expect(json_response['retain_approvals_on_push']['value']).to eq(false) expect(json_response['selective_code_owner_removals']['value']).to eq(false) expect(json_response['require_password_to_approve']['value']).to eq(false) + expect(json_response['require_reauthentication_to_approve']['value']).to eq(false) end end end diff --git a/qa/qa/specs/features/ee/browser_ui/10_govern/group/saml_sso_merge_request_approve_spec.rb b/qa/qa/specs/features/ee/browser_ui/10_govern/group/saml_sso_merge_request_approve_spec.rb index 3e5c8485194749eb4edfee8bd273e307b18bfb1f..d839c18f777aaae0be9517eb754ecf3d72c79563 100644 --- a/qa/qa/specs/features/ee/browser_ui/10_govern/group/saml_sso_merge_request_approve_spec.rb +++ b/qa/qa/specs/features/ee/browser_ui/10_govern/group/saml_sso_merge_request_approve_spec.rb @@ -38,7 +38,10 @@ module QA group.update_members(author.reload!, approver.reload!, access_level: Resource::Members::AccessLevel::MAINTAINER) + # TODO: remove with DB field https://gitlab.com/gitlab-org/gitlab/-/issues/431346 project.update_approval_configuration(require_password_to_approve: true) + + project.update_approval_configuration(require_reauthentication_to_approve: true) end after do