Commit 5f2f2650 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'master' into 'master'

Add a "Force authentication for approval" option for merge request approvals.

See merge request gitlab-org/gitlab-ee!10364
parents 3ef89f7e fe64748d
...@@ -2577,6 +2577,7 @@ ActiveRecord::Schema.define(version: 20190506135400) do ...@@ -2577,6 +2577,7 @@ ActiveRecord::Schema.define(version: 20190506135400) do
t.boolean "merge_requests_require_code_owner_approval" t.boolean "merge_requests_require_code_owner_approval"
t.boolean "detected_repository_languages" t.boolean "detected_repository_languages"
t.boolean "merge_requests_disable_committers_approval" t.boolean "merge_requests_disable_committers_approval"
t.boolean "require_password_to_approve"
t.index ["archived", "pending_delete", "merge_requests_require_code_owner_approval"], name: "projects_requiring_code_owner_approval", where: "((pending_delete = false) AND (archived = false) AND (merge_requests_require_code_owner_approval = true))", using: :btree t.index ["archived", "pending_delete", "merge_requests_require_code_owner_approval"], name: "projects_requiring_code_owner_approval", where: "((pending_delete = false) AND (archived = false) AND (merge_requests_require_code_owner_approval = true))", using: :btree
t.index ["created_at"], name: "index_projects_on_created_at", using: :btree t.index ["created_at"], name: "index_projects_on_created_at", using: :btree
t.index ["creator_id"], name: "index_projects_on_creator_id", using: :btree t.index ["creator_id"], name: "index_projects_on_creator_id", using: :btree
......
...@@ -353,10 +353,11 @@ POST /projects/:id/merge_requests/:merge_request_iid/approve ...@@ -353,10 +353,11 @@ POST /projects/:id/merge_requests/:merge_request_iid/approve
**Parameters:** **Parameters:**
| Attribute | Type | Required | Description | | Attribute | Type | Required | Description |
|---------------------|---------|----------|---------------------| |---------------------|---------|----------|-------------------------|
| `id` | integer | yes | The ID of a project | | `id` | integer | yes | The ID of a project |
| `merge_request_iid` | integer | yes | The IID of MR | | `merge_request_iid` | integer | yes | The IID of MR |
| `sha` | string | no | The HEAD of the MR | | `sha` | string | no | The HEAD of the MR |
| `approval_password` **[STARTER]** | string | no | Current user's password. Required if [**Require user password to approve**](../user/project/merge_requests/merge_request_approvals.md#require-authentication-when-approving-a-merge-request-starter) is enabled in the project settings. |
The `sha` parameter works in the same way as The `sha` parameter works in the same way as
when [accepting a merge request](merge_requests.md#accept-mr): if it is passed, then it must when [accepting a merge request](merge_requests.md#accept-mr): if it is passed, then it must
......
...@@ -294,6 +294,18 @@ enabling [**Prevent approval of merge requests by their committers**](#prevent-a ...@@ -294,6 +294,18 @@ enabling [**Prevent approval of merge requests by their committers**](#prevent-a
1. Tick the checkbox **Prevent approval of merge requests by their committers**. 1. Tick the checkbox **Prevent approval of merge requests by their committers**.
1. Click **Save changes**. 1. Click **Save changes**.
## Require authentication when approving a merge request **[STARTER]**
> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/5981) in [GitLab Starter](https://about.gitlab.com/pricing/) 11.11.
You can force the approver to enter a password in order to authenticate who is approving the merge request by
enabling **Require user password to approve**. This enables an Electronic Signature
for approvals such as the one defined by [CFR Part 11](https://www.accessdata.fda.gov/scripts/cdrh/cfdocs/cfcfr/CFRSearch.cfm?CFRPart=11&showFR=1&subpartNode=21:1.0.1.1.8.3)):
1. Navigate to your project's **Settings > General** and expand **Merge request approvals**.
1. Tick the checkbox **Require user password to approve**.
1. Click **Save changes**.
## Merge requests with different source branch and target branch projects ## Merge requests with different source branch and target branch projects
If the merge request source branch and target branch belong to different If the merge request source branch and target branch belong to different
......
<script> <script>
import { GlButton, GlLoadingIcon } from '@gitlab/ui'; import { GlButton, GlLoadingIcon } from '@gitlab/ui';
import createFlash from '~/flash'; import createFlash, { hideFlash } from '~/flash';
import { s__ } from '~/locale'; import { s__ } from '~/locale';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue'; import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import eventHub from '~/vue_merge_request_widget/event_hub'; import eventHub from '~/vue_merge_request_widget/event_hub';
...@@ -9,6 +9,7 @@ import MrWidgetIcon from '~/vue_merge_request_widget/components/mr_widget_icon.v ...@@ -9,6 +9,7 @@ import MrWidgetIcon from '~/vue_merge_request_widget/components/mr_widget_icon.v
import ApprovalsSummary from './approvals_summary.vue'; import ApprovalsSummary from './approvals_summary.vue';
import ApprovalsSummaryOptional from './approvals_summary_optional.vue'; import ApprovalsSummaryOptional from './approvals_summary_optional.vue';
import ApprovalsFooter from './approvals_footer.vue'; import ApprovalsFooter from './approvals_footer.vue';
import ApprovalsAuth from './approvals_auth.vue';
import { FETCH_LOADING, FETCH_ERROR, APPROVE_ERROR, UNAPPROVE_ERROR } from '../messages'; import { FETCH_LOADING, FETCH_ERROR, APPROVE_ERROR, UNAPPROVE_ERROR } from '../messages';
export default { export default {
...@@ -20,6 +21,7 @@ export default { ...@@ -20,6 +21,7 @@ export default {
ApprovalsSummary, ApprovalsSummary,
ApprovalsSummaryOptional, ApprovalsSummaryOptional,
ApprovalsFooter, ApprovalsFooter,
ApprovalsAuth,
GlButton, GlButton,
GlLoadingIcon, GlLoadingIcon,
}, },
...@@ -39,6 +41,8 @@ export default { ...@@ -39,6 +41,8 @@ export default {
isApproving: false, isApproving: false,
isExpanded: false, isExpanded: false,
isLoadingRules: false, isLoadingRules: false,
showApprovalAuth: false,
hasApprovalAuthError: false,
}; };
}, },
computed: { computed: {
...@@ -72,16 +76,20 @@ export default { ...@@ -72,16 +76,20 @@ export default {
showUnapprove() { showUnapprove() {
return this.userHasApproved && !this.userCanApprove && this.mr.state !== 'merged'; return this.userHasApproved && !this.userCanApprove && this.mr.state !== 'merged';
}, },
requirePasswordToApprove() {
return this.mr.approvals.require_password_to_approve;
},
approvalText() {
return this.isApproved && this.approvedBy.length > 0
? s__('mrWidget|Approve additionally')
: s__('mrWidget|Approve');
},
action() { action() {
// Use the default approve action, only if we aren't using the auth component for it
if (this.showApprove) { if (this.showApprove) {
const inverted = this.isApproved; const inverted = this.isApproved;
const text =
this.isApproved && this.approvedBy.length > 0
? s__('mrWidget|Approve additionally')
: s__('mrWidget|Approve');
return { return {
text, text: this.approvalText,
inverted, inverted,
variant: 'primary', variant: 'primary',
action: () => this.approve(), action: () => this.approve(),
...@@ -116,6 +124,13 @@ export default { ...@@ -116,6 +124,13 @@ export default {
.catch(() => createFlash(FETCH_ERROR)); .catch(() => createFlash(FETCH_ERROR));
}, },
methods: { methods: {
clearError() {
this.hasApprovalAuthError = false;
const flashEl = document.querySelector('.flash-alert');
if (flashEl) {
hideFlash(flashEl);
}
},
refreshAll() { refreshAll() {
return Promise.all([this.refreshRules(), this.refreshApprovals()]).catch(() => return Promise.all([this.refreshRules(), this.refreshApprovals()]).catch(() =>
createFlash(FETCH_ERROR), createFlash(FETCH_ERROR),
...@@ -135,36 +150,69 @@ export default { ...@@ -135,36 +150,69 @@ export default {
}); });
}, },
approve() { approve() {
this.updateApproval(() => this.service.approveMergeRequest(), APPROVE_ERROR); if (this.requirePasswordToApprove) {
this.showApprovalAuth = true;
return;
}
this.updateApproval(
() => this.service.approveMergeRequest(),
() => createFlash(APPROVE_ERROR),
);
}, },
unapprove() { unapprove() {
this.updateApproval(() => this.service.unapproveMergeRequest(), UNAPPROVE_ERROR); this.updateApproval(
() => this.service.unapproveMergeRequest(),
() => createFlash(UNAPPROVE_ERROR),
);
},
approveWithAuth(data) {
this.updateApproval(
() => this.service.approveMergeRequestWithAuth(data),
error => {
if (error && error.response && error.response.status === 401) {
this.hasApprovalAuthError = true;
return;
}
createFlash(APPROVE_ERROR);
},
);
}, },
updateApproval(serviceFn, error) { updateApproval(serviceFn, errFn) {
this.isApproving = true; this.isApproving = true;
this.clearError();
return serviceFn() return serviceFn()
.then(data => { .then(data => {
this.mr.setApprovals(data); this.mr.setApprovals(data);
eventHub.$emit('MRWidgetUpdateRequested'); eventHub.$emit('MRWidgetUpdateRequested');
this.isApproving = false; this.showApprovalAuth = false;
}) })
.catch(() => { .catch(errFn)
createFlash(error); .then(() => {
this.isApproving = false; this.isApproving = false;
}) this.refreshRules();
.then(() => this.refreshRules()); });
},
onApprovalAuthCancel() {
this.showApprovalAuth = false;
this.clearError();
}, },
}, },
FETCH_LOADING, FETCH_LOADING,
}; };
</script> </script>
<template> <template>
<mr-widget-container> <mr-widget-container>
<div class="js-mr-approvals d-flex align-items-start align-items-md-center"> <div class="js-mr-approvals d-flex align-items-start align-items-md-center">
<mr-widget-icon name="approval" /> <mr-widget-icon name="approval" />
<div v-if="fetchingApprovals">{{ $options.FETCH_LOADING }}</div> <div v-if="fetchingApprovals">{{ $options.FETCH_LOADING }}</div>
<template v-else>
<approvals-auth
v-if="showApprovalAuth"
:is-approving="isApproving"
:has-error="hasApprovalAuthError"
@approve="approveWithAuth"
@cancel="onApprovalAuthCancel"
/>
<template v-else> <template v-else>
<gl-button <gl-button
v-if="action" v-if="action"
...@@ -190,6 +238,7 @@ export default { ...@@ -190,6 +238,7 @@ export default {
:approvers="approvedBy" :approvers="approvedBy"
/> />
</template> </template>
</template>
</div> </div>
<approvals-footer <approvals-footer
v-if="hasFooter" v-if="hasFooter"
......
<script>
import { GlButton, GlLoadingIcon } from '@gitlab/ui';
export default {
components: {
GlButton,
GlLoadingIcon,
},
props: {
isApproving: {
type: Boolean,
default: false,
required: false,
},
hasError: {
type: Boolean,
default: false,
required: false,
},
},
data() {
return {
approvalPassword: '',
};
},
mounted() {
this.$nextTick(() => this.$refs.approvalPassword.focus());
},
methods: {
approve() {
this.$emit('approve', this.approvalPassword);
},
cancel() {
this.$emit('cancel');
},
},
};
</script>
<template>
<form class="form-inline align-items-center" @submit.prevent="approve">
<div class="form-group mb-2 mr-2 mb-sm-0">
<input
ref="approvalPassword"
v-model="approvalPassword"
type="password"
class="form-control"
:class="{ 'is-invalid': hasError }"
autocomplete="new-password"
:placeholder="s__('Password')"
/>
</div>
<div class="form-group mb-2 mr-2 mb-sm-0">
<gl-button
variant="primary"
:disabled="isApproving"
size="sm"
class="mr-1 js-confirm"
@click="approve"
>
<gl-loading-icon v-if="isApproving" inline />
{{ s__('Confirm') }}
</gl-button>
<gl-button
variant="default"
:disabled="isApproving"
size="sm"
class="js-cancel"
@click="cancel"
>
{{ s__('Cancel') }}
</gl-button>
</div>
<div v-if="hasError">
<span class="gl-field-error">
{{ s__('mrWidget|Approval password is invalid.') }}
</span>
</div>
</form>
</template>
...@@ -19,6 +19,10 @@ export default class MRWidgetService extends CEWidgetService { ...@@ -19,6 +19,10 @@ export default class MRWidgetService extends CEWidgetService {
this.fetchApprovalSettings = () => this.fetchApprovalSettings = () =>
axios.get(this.apiApprovalSettingsPath).then(res => res.data); axios.get(this.apiApprovalSettingsPath).then(res => res.data);
this.approveMergeRequest = () => axios.post(this.apiApprovePath).then(res => res.data); this.approveMergeRequest = () => axios.post(this.apiApprovePath).then(res => res.data);
this.approveMergeRequestWithAuth = approvalPassword =>
axios
.post(this.apiApprovePath, { approval_password: approvalPassword })
.then(res => res.data);
this.unapproveMergeRequest = () => axios.post(this.apiUnapprovePath).then(res => res.data); this.unapproveMergeRequest = () => axios.post(this.apiUnapprovePath).then(res => res.data);
} }
} }
......
...@@ -42,6 +42,7 @@ module EE ...@@ -42,6 +42,7 @@ module EE
packages_enabled packages_enabled
merge_requests_author_approval merge_requests_author_approval
merge_requests_disable_committers_approval merge_requests_disable_committers_approval
require_password_to_approve
merge_requests_require_code_owner_approval merge_requests_require_code_owner_approval
group_with_project_templates_id group_with_project_templates_id
] ]
......
...@@ -365,6 +365,11 @@ module EE ...@@ -365,6 +365,11 @@ module EE
super && code_owner_approval_required_available? super && code_owner_approval_required_available?
end end
def require_password_to_approve
super && password_authentication_enabled_for_web?
end
alias_method :require_password_to_approve?, :require_password_to_approve
def find_path_lock(path, exact_match: false, downstream: false) def find_path_lock(path, exact_match: false, downstream: false)
path_lock_finder = strong_memoize(:path_lock_finder) do path_lock_finder = strong_memoize(:path_lock_finder) do
::Gitlab::PathLocksFinder.new(self) ::Gitlab::PathLocksFinder.new(self)
......
...@@ -2,7 +2,13 @@ ...@@ -2,7 +2,13 @@
module MergeRequests module MergeRequests
class ApprovalService < MergeRequests::BaseService class ApprovalService < MergeRequests::BaseService
IncorrectApprovalPasswordError = Class.new(StandardError)
def execute(merge_request) def execute(merge_request)
if incorrect_approval_password?(merge_request)
raise IncorrectApprovalPasswordError
end
approval = merge_request.approvals.new(user: current_user) approval = merge_request.approvals.new(user: current_user)
if save_approval(approval) if save_approval(approval)
...@@ -22,6 +28,11 @@ module MergeRequests ...@@ -22,6 +28,11 @@ module MergeRequests
private private
def incorrect_approval_password?(merge_request)
merge_request.project.require_password_to_approve? &&
!Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password])
end
def save_approval(approval) def save_approval(approval)
Approval.safe_ensure_unique do Approval.safe_ensure_unique do
approval.save approval.save
......
...@@ -41,3 +41,12 @@ ...@@ -41,3 +41,12 @@
%span= _('Prevent approval of merge requests by merge request committers') %span= _('Prevent approval of merge requests by merge request committers')
= link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals', = link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals',
anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank' anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank'
- if Feature.enabled?(:approval_rules, project) && password_authentication_enabled_for_web?
.form-group.self-approval
.form-check
= form.check_box :require_password_to_approve, class: 'form-check-input'
= form.label :require_password_to_approve, class: 'form-check-label' do
%span= _('Require user password to approve')
= link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals',
anchor: 'require-authentication-when-approving-a-merge-request-starter'), target: '_blank'
---
title: Added a "Require user password to approve" option on projects for merge request
approvals to enable compliance in FDA regulated fields"
merge_request: 10364
author: James Davila, Paul Knopf, Greg Smethells
type: added
# frozen_string_literal: true
class AddRequirePasswordToApprove < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column :projects, :require_password_to_approve, :boolean
end
end
...@@ -117,6 +117,7 @@ module API ...@@ -117,6 +117,7 @@ module API
end end
params do params do
optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch' optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch'
optional :approval_password, type: String, desc: 'Current user\'s password if project is set to require explicit auth on approval'
end end
post 'approve' do post 'approve' do
merge_request = find_project_merge_request(params[:merge_request_iid]) merge_request = find_project_merge_request(params[:merge_request_iid])
...@@ -125,10 +126,13 @@ module API ...@@ -125,10 +126,13 @@ module API
check_sha_param!(params, merge_request) check_sha_param!(params, merge_request)
begin
::MergeRequests::ApprovalService ::MergeRequests::ApprovalService
.new(user_project, current_user) .new(user_project, current_user, params)
.execute(merge_request) .execute(merge_request)
rescue ::MergeRequests::ApprovalService::IncorrectApprovalPasswordError
unauthorized!
end
present_approval(merge_request) present_approval(merge_request)
end end
......
...@@ -387,6 +387,10 @@ module EE ...@@ -387,6 +387,10 @@ module EE
expose :approvals_left expose :approvals_left
expose :require_password_to_approve do |approval_state|
approval_state.project.require_password_to_approve?
end
expose :approved_by, using: EE::API::Entities::Approvals do |approval_state| expose :approved_by, using: EE::API::Entities::Approvals do |approval_state|
approval_state.merge_request.approvals approval_state.merge_request.approvals
end end
......
...@@ -12,7 +12,7 @@ module EE ...@@ -12,7 +12,7 @@ module EE
explanation 'Approve the current merge request.' explanation 'Approve the current merge request.'
types MergeRequest types MergeRequest
condition do condition do
quick_action_target.persisted? && quick_action_target.can_approve?(current_user) quick_action_target.persisted? && quick_action_target.can_approve?(current_user) && !quick_action_target.project.require_password_to_approve?
end end
command :approve do command :approve do
if quick_action_target.can_approve?(current_user) if quick_action_target.can_approve?(current_user)
......
# frozen_string_literal: true
require 'rails_helper'
describe 'Merge request > User approves with password', :js do
let(:user) { create(:user) }
let(:project) { create(:project, :public, :repository, approvals_before_merge: 1, require_password_to_approve: true, merge_requests_author_approval: true) }
let(:merge_request) { create(:merge_request, source_project: project) }
before do
project.add_developer(user)
sign_in(user)
visit project_merge_request_path(project, merge_request)
end
it 'works, when user approves and enters correct password' do
page.within('.js-mr-approvals') do
approve_with_password '12345678'
expect(page).not_to have_button('Approve')
expect(page).to have_text('Approved by')
end
end
it 'does not need password to unapprove' do
approve_with_password '12345678'
unapprove
expect(page).to have_button('Approve')
expect(page).not_to have_text('Approved by')
end
it 'shows error, when user approves and enters incorrect password' do
page.within('.js-mr-approvals') do
approve_with_password 'nottherightpassword'
expect(page).to have_text('Approval password is invalid.')
click_button 'Cancel'
expect(page).not_to have_text('Approved by')
end
end
end
def approve_with_password(password)
click_button('Approve')
fill_in(type: 'password', with: password)
click_button('Confirm')
wait_for_requests
end
def unapprove
click_button('Revoke approval')
wait_for_requests
end
import { createLocalVue, shallowMount } from '@vue/test-utils';
import { GlLoadingIcon } from '@gitlab/ui';
import ApprovalsAuth from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_auth.vue';
const TEST_PASSWORD = 'password';
const localVue = createLocalVue();
// For some reason, the `localVue.nextTick` needs to be deferred
// or the timing doesn't work.
const tick = () => Promise.resolve().then(localVue.nextTick);
const waitForTick = done =>
tick()
.then(done)
.catch(done.fail);
describe('Approval auth component', () => {
let wrapper;
const createComponent = (props = {}) => {
wrapper = shallowMount(localVue.extend(ApprovalsAuth), {
propsData: {
...props,
},
localVue,
sync: false,
});
};
afterEach(() => {
wrapper.destroy();
wrapper = null;
});
const findConfirm = () => wrapper.find('.js-confirm');
const findCancel = () => wrapper.find('.js-cancel');
const findLoading = () => findConfirm().find(GlLoadingIcon);
const findInput = () => wrapper.find('input[type=password]');
const findErrorMessage = () => wrapper.find('.gl-field-error');
describe('when created', () => {
beforeEach(done => {
createComponent();
waitForTick(done);
});
it('approve button, cancel button, and password input controls are rendered', () => {
expect(findConfirm().exists()).toBe(true);
expect(findCancel().exists()).toBe(true);
expect(wrapper.find('input').exists()).toBe(true);
});
it('does not show loading icon', () => {
expect(findLoading().exists()).toBe(false);
});
it('does not show error message', () => {
expect(findErrorMessage().exists()).toBe(false);
});
it('does not emit anything', () => {
expect(wrapper.emittedByOrder()).toEqual([]);
});
});
describe('when approve clicked', () => {
beforeEach(done => {
createComponent();
findInput().setValue(TEST_PASSWORD);
findConfirm().vm.$emit('click');
waitForTick(done);
});
it('emits the approve event', () => {
expect(wrapper.emittedByOrder()).toEqual([{ name: 'approve', args: [TEST_PASSWORD] }]);
});
});
describe('when cancel is clicked', () => {
beforeEach(done => {
createComponent();
findCancel().vm.$emit('click');
waitForTick(done);
});
it('emits the cancel event', () => {
expect(wrapper.emittedByOrder()).toEqual([{ name: 'cancel', args: [] }]);
});
});
describe('when isApproving is true', () => {
beforeEach(done => {
createComponent({ isApproving: true });
waitForTick(done);
});
it('shows loading icon when isApproving is true', () => {
expect(findLoading().exists()).toBe(true);
});
});
describe('when hasError is true', () => {
beforeEach(done => {
createComponent({ hasError: true });
waitForTick(done);
});
it('shows the invalid password message', () => {
expect(findErrorMessage().exists()).toBe(true);
});
});
});
...@@ -5,6 +5,8 @@ import Approvals from 'ee/vue_merge_request_widget/components/approvals/multiple ...@@ -5,6 +5,8 @@ import Approvals from 'ee/vue_merge_request_widget/components/approvals/multiple
import ApprovalsSummary from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary.vue'; import ApprovalsSummary from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary.vue';
import ApprovalsSummaryOptional from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue'; import ApprovalsSummaryOptional from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue';
import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_footer.vue'; import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_footer.vue';
import ApprovalsAuth from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_auth.vue';
import { import {
FETCH_LOADING, FETCH_LOADING,
FETCH_ERROR, FETCH_ERROR,
...@@ -14,6 +16,7 @@ import { ...@@ -14,6 +16,7 @@ import {
const localVue = createLocalVue(); const localVue = createLocalVue();
const TEST_HELP_PATH = 'help/path'; const TEST_HELP_PATH = 'help/path';
const TEST_PASSWORD = 'password';
const testApprovedBy = () => [1, 7, 10].map(id => ({ id })); const testApprovedBy = () => [1, 7, 10].map(id => ({ id }));
const testApprovals = () => ({ const testApprovals = () => ({
has_approval_rules: true, has_approval_rules: true,
...@@ -24,6 +27,7 @@ const testApprovals = () => ({ ...@@ -24,6 +27,7 @@ const testApprovals = () => ({
suggested_approvers: [], suggested_approvers: [],
user_can_approve: true, user_can_approve: true,
user_has_approved: true, user_has_approved: true,
require_password_to_approve: false,
}); });
const testApprovalRulesResponse = () => ({ rules: [{ id: 2 }] }); const testApprovalRulesResponse = () => ({ rules: [{ id: 2 }] });
...@@ -75,6 +79,7 @@ describe('EE MRWidget approvals', () => { ...@@ -75,6 +79,7 @@ describe('EE MRWidget approvals', () => {
fetchApprovalSettings: Promise.resolve(testApprovalRulesResponse()), fetchApprovalSettings: Promise.resolve(testApprovalRulesResponse()),
approveMergeRequest: Promise.resolve(testApprovals()), approveMergeRequest: Promise.resolve(testApprovals()),
unapproveMergeRequest: Promise.resolve(testApprovals()), unapproveMergeRequest: Promise.resolve(testApprovals()),
approveMergeRequestWithAuth: Promise.resolve(testApprovals()),
}); });
mr = { mr = {
...jasmine.createSpyObj('Store', ['setApprovals', 'setApprovalRules']), ...jasmine.createSpyObj('Store', ['setApprovals', 'setApprovalRules']),
...@@ -283,6 +288,73 @@ describe('EE MRWidget approvals', () => { ...@@ -283,6 +288,73 @@ describe('EE MRWidget approvals', () => {
}); });
}); });
}); });
describe('when project requires password to approve', () => {
beforeEach(done => {
mr.approvals.require_password_to_approve = true;
createComponent();
waitForTick(done);
});
it('does not initially show approvals auth component', () => {
expect(wrapper.find(ApprovalsAuth).exists()).toBe(false);
});
describe('when approve is clicked', () => {
beforeEach(done => {
findAction().vm.$emit('click');
waitForTick(done);
});
it('shows approvals auth component', () => {
expect(wrapper.find(ApprovalsAuth).exists()).toBe(true);
});
describe('when emits approve', () => {
let authReject;
beforeEach(done => {
service.approveMergeRequestWithAuth.and.returnValue(
new Promise((resolve, reject) => {
authReject = reject;
}),
);
wrapper.find(ApprovalsAuth).vm.$emit('approve', TEST_PASSWORD);
waitForTick(done);
});
it('calls service when emits approve', () => {
expect(service.approveMergeRequestWithAuth).toHaveBeenCalledWith(TEST_PASSWORD);
});
it('sets isLoading on auth', () => {
expect(wrapper.find(ApprovalsAuth).props('isApproving')).toBe(true);
});
it('sets hasError when auth fails', done => {
authReject({ response: { status: 401 } });
tick()
.then(() => {
expect(wrapper.find(ApprovalsAuth).props('hasError')).toBe(true);
})
.then(done)
.catch(done.fail);
});
it('shows flash if general error', done => {
authReject('something really bad!');
tick()
.then(() => {
expect(createFlash).toHaveBeenCalledWith(APPROVE_ERROR);
})
.then(done)
.catch(done.fail);
});
});
});
});
}); });
describe('when user has approved', () => { describe('when user has approved', () => {
......
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
Project: Project:
- merge_requests_disable_committers_approval - merge_requests_disable_committers_approval
- merge_requests_require_code_owner_approval - merge_requests_require_code_owner_approval
- require_password_to_approve
ProjectTracingSetting: ProjectTracingSetting:
- external_url - external_url
Note: Note:
......
...@@ -319,6 +319,39 @@ describe API::MergeRequestApprovals do ...@@ -319,6 +319,39 @@ describe API::MergeRequestApprovals do
end end
end end
context 'when project requires force auth for approval' do
before do
project.update(require_password_to_approve: true)
approver.update(password: 'password')
end
it 'returns a 401 with no password' do
approve
expect(response).to have_gitlab_http_status(401)
end
it 'does not approve the merge request with no password' do
approve
expect(merge_request.reload.approvals_left).to eq(2)
end
it 'returns a 401 with incorrect password' do
approve
expect(response).to have_gitlab_http_status(401)
end
it 'does not approve the merge request with incorrect password' do
approve
expect(merge_request.reload.approvals_left).to eq(2)
end
it 'approves the merge request with correct password' do
approve(approval_password: 'password')
expect(response).to have_gitlab_http_status(201)
expect(merge_request.reload.approvals_left).to eq(1)
end
end
it 'only shows group approvers visible to the user' do it 'only shows group approvers visible to the user' do
private_group = create(:group, :private) private_group = create(:group, :private)
merge_request.approver_groups.create(group: private_group) merge_request.approver_groups.create(group: private_group)
......
...@@ -88,5 +88,37 @@ describe MergeRequests::ApprovalService do ...@@ -88,5 +88,37 @@ describe MergeRequests::ApprovalService do
end end
end end
end end
context 'when project requires force auth for approval' do
before do
project.update(require_password_to_approve: true)
user.update(password: 'password')
end
context 'when password not specified' do
it 'raises an error' do
expect { service.execute(merge_request) }.to raise_error(::MergeRequests::ApprovalService::IncorrectApprovalPasswordError)
end
end
context 'when incorrect password is specified' do
let(:params) do
{ approval_password: 'incorrect' }
end
it 'raises an error' do
service_with_params = described_class.new(project, user, params)
expect { service_with_params.execute(merge_request) }.to raise_error(::MergeRequests::ApprovalService::IncorrectApprovalPasswordError)
end
end
context 'when correct password is specified' do
let(:params) do
{ approval_password: 'password' }
end
it 'does not raise an error' do
service_with_params = described_class.new(project, user, params)
expect { service_with_params.execute(merge_request) }.not_to raise_error(::MergeRequests::ApprovalService::IncorrectApprovalPasswordError)
end
end
end
end end
end end
...@@ -10521,6 +10521,9 @@ msgstr "" ...@@ -10521,6 +10521,9 @@ msgstr ""
msgid "Require approval from code owners" msgid "Require approval from code owners"
msgstr "" msgstr ""
msgid "Require user password to approve"
msgstr ""
msgid "Require users to prove ownership of custom domains" msgid "Require users to prove ownership of custom domains"
msgstr "" msgstr ""
...@@ -15113,6 +15116,9 @@ msgstr "" ...@@ -15113,6 +15116,9 @@ msgstr ""
msgid "mrWidget|An error occurred while submitting your approval." msgid "mrWidget|An error occurred while submitting your approval."
msgstr "" msgstr ""
msgid "mrWidget|Approval password is invalid."
msgstr ""
msgid "mrWidget|Approve" msgid "mrWidget|Approve"
msgstr "" msgstr ""
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment