Skip to content
Snippets Groups Projects
Commit 2c20801d authored by SAM FIGUEROA's avatar SAM FIGUEROA :owl:
Browse files

Rename for reauthentication vs just password

- In an effor to rename require_password_to_approve
  to reflect that it can reference either SAML
  or password authentication we are adding this
  merge request approval setting field as its
  replacement. The require_password_to_approve
  field will be deprecated with API V5.

- The MergeRequestApprovalSettings API Grape module
  needs to mirror the params for the deprecated
  and the new version until the removal is done.
  This is to ensure either setting can be used in the API.
  If both are provided we default to using the new version
  (require_reauthentication_value)

- Refs:
  #431346
  !150710 (comment 1912459777)
  !150710 (comment 1912459768)
  !150710 (comment 1912459750)
  !150710 (comment 1912459735)
  !150710 (comment 1912459720)

Changelog: changed
EE: true
parent edf585ed
No related branches found
No related tags found
No related merge requests found
Showing
with 390 additions and 50 deletions
......@@ -15,6 +15,9 @@
"require_password_to_approve": {
"type": "boolean"
},
"require_reauthentication_to_approve": {
"type": "boolean"
},
"block_branch_modification": {
"type": "boolean"
},
......
......@@ -238,6 +238,12 @@ Example response:
## Project-level MR approvals
## Removals in API v5
These attributes are deprecated, and are scheduled to be removed in v5 of the API:
- `require_password_to_approve`: Use `require_reauthentication_to_approve` (nested in project settings) instead. If you supply both values `require_reauthentication_to_approve` will take precedence.
> - Use the [project level approval rules](#get-project-level-rules) to access this information.
You can request information about a project's approval configuration using the
......@@ -263,7 +269,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 +291,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 17.1. 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 +304,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 +1241,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. |
......
......@@ -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 |
......
......@@ -381,6 +381,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)
......@@ -1262,6 +1265,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
......
......@@ -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
......@@ -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
......
---
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]
......@@ -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
......@@ -19,12 +19,47 @@ 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
raw_params = declared_params
normalized_params = declared_params(include_missing: false)
# if both old and new key are provided prefer the value from the new one
if normalized_params.key?(:require_password_to_approve) && \
normalized_params.key?(:require_reauthentication_to_approve)
normalized_params[:require_password_to_approve] = normalized_params[:require_reauthentication_to_approve]
return normalized_params
end
missing_keys = raw_params.keys - normalized_params.keys
missing_password_param = missing_keys == [:require_password_to_approve]
missing_reauthentication_param = missing_keys == [:require_reauthentication_to_approve]
if missing_password_param && normalized_params.key?(:require_reauthentication_to_approve)
normalized_params[:require_password_to_approve] = normalized_params[:require_reauthentication_to_approve]
end
if missing_reauthentication_param && 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 +96,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 +145,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
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -77,6 +77,17 @@
"inherited_from": {
"type": "string"
}
},
"require_reauthentication_to_approve": {
"value": {
"type": "boolean"
},
"locked": {
"type": "boolean"
},
"inherited_from": {
"type": "string"
}
}
},
"additionalProperties": false
......
......@@ -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
......
......@@ -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
......@@ -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
......@@ -1185,6 +1185,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) }
......@@ -1222,6 +1263,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
......
......@@ -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
......@@ -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
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment