Commit b105d78f authored by Jiaan Louw's avatar Jiaan Louw Committed by Savas Vedova

Enforce group approval settings in projects

parent 77eebce7
......@@ -3,7 +3,8 @@ import { GlAlert, GlButton, GlForm, GlFormGroup, GlLoadingIcon, GlLink } from '@
import { isEmpty } from 'lodash';
import { mapActions, mapGetters, mapState } from 'vuex';
import { helpPagePath } from '~/helpers/help_page_helper';
import { APPROVAL_SETTINGS_I18N } from '../constants';
import { sprintf } from '~/locale';
import { APPROVAL_SETTINGS_I18N, TYPE_GROUP } from '../constants';
import ApprovalSettingsCheckbox from './approval_settings_checkbox.vue';
export default {
......@@ -54,6 +55,7 @@ export default {
state.approvalSettings.settings.preventMrApprovalRuleEdit,
removeApprovalsOnPush: (state) => state.approvalSettings.settings.removeApprovalsOnPush,
requireUserPassword: (state) => state.approvalSettings.settings.requireUserPassword,
groupName: (state) => state.settings.groupName,
}),
...mapGetters(['settingChanged']),
hasSettings() {
......@@ -81,6 +83,16 @@ export default {
async onSubmit() {
await this.updateSettings(this.approvalSettingsPath);
},
lockedText({ locked, inheritedFrom }) {
if (!locked) {
return null;
}
if (inheritedFrom === TYPE_GROUP) {
const { groupName } = this;
return sprintf(APPROVAL_SETTINGS_I18N.lockedByGroupOwner, { groupName });
}
return APPROVAL_SETTINGS_I18N.lockedByAdmin;
},
},
i18n: APPROVAL_SETTINGS_I18N,
links: {
......@@ -127,7 +139,7 @@ export default {
:checked="preventAuthorApproval.value"
:label="settingsLabels.authorApprovalLabel"
:locked="!canPreventAuthorApproval || preventAuthorApproval.locked"
:locked-text="$options.i18n.lockedByAdmin"
:locked-text="lockedText(preventAuthorApproval)"
data-testid="prevent-author-approval"
@input="setPreventAuthorApproval"
/>
......@@ -135,7 +147,7 @@ export default {
:checked="preventCommittersApproval.value"
:label="settingsLabels.preventCommittersApprovalLabel"
:locked="!canPreventCommittersApproval || preventCommittersApproval.locked"
:locked-text="$options.i18n.lockedByAdmin"
:locked-text="lockedText(preventCommittersApproval)"
data-testid="prevent-committers-approval"
@input="setPreventCommittersApproval"
/>
......@@ -143,7 +155,7 @@ export default {
:checked="preventMrApprovalRuleEdit.value"
:label="settingsLabels.preventMrApprovalRuleEditLabel"
:locked="!canPreventMrApprovalRuleEdit || preventMrApprovalRuleEdit.locked"
:locked-text="$options.i18n.lockedByAdmin"
:locked-text="lockedText(preventMrApprovalRuleEdit)"
data-testid="prevent-mr-approval-rule-edit"
@input="setPreventMrApprovalRuleEdit"
/>
......@@ -151,7 +163,7 @@ export default {
:checked="requireUserPassword.value"
:label="settingsLabels.requireUserPasswordLabel"
:locked="requireUserPassword.locked"
:locked-text="$options.i18n.lockedByAdmin"
:locked-text="lockedText(requireUserPassword)"
data-testid="require-user-password"
@input="setRequireUserPassword"
/>
......@@ -159,7 +171,7 @@ export default {
:checked="removeApprovalsOnPush.value"
:label="settingsLabels.removeApprovalsOnPushLabel"
:locked="removeApprovalsOnPush.locked"
:locked-text="$options.i18n.lockedByAdmin"
:locked-text="lockedText(removeApprovalsOnPush)"
data-testid="remove-approvals-on-push"
@input="setRemoveApprovalsOnPush"
/>
......
......@@ -64,6 +64,9 @@ export const APPROVAL_SETTINGS_I18N = {
lockedByAdmin: s__(
'ApprovalSettings|This setting is configured at the instance level and can only be changed by an administrator.',
),
lockedByGroupOwner: s__(
'ApprovalSettings|This setting is configured in %{groupName} and can only be changed by an administrator or group owner.',
),
};
export const PROJECT_APPROVAL_SETTINGS_LABELS_I18N = {
......
import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
import { RULE_TYPE_REGULAR, RULE_TYPE_ANY_APPROVER } from './constants';
const visibleTypes = new Set([RULE_TYPE_ANY_APPROVER, RULE_TYPE_REGULAR]);
......@@ -102,16 +103,20 @@ export const mapMRApprovalSettingsResponse = (res) => {
const invertApprovalSetting = ({ value, ...rest }) => ({ value: !value, ...rest });
export const groupApprovalsMappers = {
mapDataToState: (data) => ({
preventAuthorApproval: invertApprovalSetting(data.allow_author_approval),
preventMrApprovalRuleEdit: invertApprovalSetting(
data.allow_overrides_to_approver_list_per_merge_request,
export const mergeRequestApprovalSettingsMappers = {
mapDataToState: (data) =>
convertObjectPropsToCamelCase(
{
preventAuthorApproval: invertApprovalSetting(data.allow_author_approval),
preventMrApprovalRuleEdit: invertApprovalSetting(
data.allow_overrides_to_approver_list_per_merge_request,
),
requireUserPassword: data.require_password_to_approve,
removeApprovalsOnPush: invertApprovalSetting(data.retain_approvals_on_push),
preventCommittersApproval: invertApprovalSetting(data.allow_committer_approval),
},
{ deep: true },
),
requireUserPassword: data.require_password_to_approve,
removeApprovalsOnPush: invertApprovalSetting(data.retain_approvals_on_push),
preventCommittersApproval: invertApprovalSetting(data.allow_committer_approval),
}),
mapStateToPayload: ({ settings }) => ({
allow_author_approval: !settings.preventAuthorApproval.value,
allow_overrides_to_approver_list_per_merge_request: !settings.preventMrApprovalRuleEdit.value,
......
import Vue from 'vue';
import { parseBoolean } from '~/lib/utils/common_utils';
import GroupSettingsApp from './components/group_settings/app.vue';
import { groupApprovalsMappers } from './mappers';
import { mergeRequestApprovalSettingsMappers } from './mappers';
import createStore from './stores';
import approvalSettingsModule from './stores/modules/approval_settings';
......@@ -12,7 +12,7 @@ const mountGroupApprovalSettings = (el) => {
const { defaultExpanded, approvalSettingsPath } = el.dataset;
const store = createStore({
approvalSettings: approvalSettingsModule(groupApprovalsMappers),
approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
});
return new Vue({
......
import Vue from 'vue';
import { parseBoolean } from '~/lib/utils/common_utils';
import ProjectSettingsApp from './components/project_settings/app.vue';
import { projectApprovalsMappers } from './mappers';
import { projectApprovalsMappers, mergeRequestApprovalSettingsMappers } from './mappers';
import createStore from './stores';
import approvalSettingsModule from './stores/modules/approval_settings';
import projectSettingsModule from './stores/modules/project_settings';
......@@ -18,10 +18,18 @@ export default function mountProjectSettingsApprovals(el) {
} = el.dataset;
const modules = {
approvalSettings: approvalSettingsModule({ updateMethod: 'post', ...projectApprovalsMappers }),
approvals: projectSettingsModule(),
};
if (gon.features.groupMergeRequestApprovalSettingsFeatureFlag) {
modules.approvalSettings = approvalSettingsModule(mergeRequestApprovalSettingsMappers);
} else {
modules.approvalSettings = approvalSettingsModule({
updateMethod: 'post',
...projectApprovalsMappers,
});
}
const store = createStore(modules, {
...el.dataset,
prefix: 'project-settings',
......
......@@ -8,7 +8,7 @@ export default (mapDataToState) => ({
state.errorMessage = '';
},
[types.RECEIVE_SETTINGS_SUCCESS](state, data) {
state.settings = { ...mapDataToState(data) };
state.settings = mapDataToState(data);
state.initialSettings = cloneDeep(state.settings);
state.isLoading = false;
},
......@@ -22,7 +22,7 @@ export default (mapDataToState) => ({
state.errorMessage = '';
},
[types.UPDATE_SETTINGS_SUCCESS](state, data) {
state.settings = { ...mapDataToState(data) };
state.settings = mapDataToState(data);
state.initialSettings = cloneDeep(state.settings);
state.isLoading = false;
state.isUpdated = true;
......
......@@ -14,6 +14,10 @@ module EE
enable_sast_entry_points_experiment
end
before_action only: :edit do
push_frontend_feature_flag(:group_merge_request_approval_settings_feature_flag, project.root_ancestor)
end
feature_category :projects, [:restore]
end
......
......@@ -59,24 +59,27 @@ module EE
def approvals_app_data(project = @project)
{
data: {
'project_id': project.id,
'can_edit': can_modify_approvers.to_s,
'can_modify_author_settings': can_modify_author_settings.to_s,
'can_modify_commiter_settings': can_modify_commiter_settings.to_s,
'project_path': expose_path(api_v4_projects_path(id: project.id)),
'settings_path': expose_path(api_v4_projects_approval_settings_path(id: project.id)),
'approvals_path': expose_path(api_v4_projects_approvals_path(id: project.id)),
'rules_path': expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)),
'allow_multi_rule': project.multiple_approval_rules_available?.to_s,
'eligible_approvers_docs_path': help_page_path('user/project/merge_requests/approvals/rules', anchor: 'eligible-approvers'),
'security_approvals_help_page_path': help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
'security_configuration_path': project_security_configuration_path(project),
'vulnerability_check_help_page_path': help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
'license_check_help_page_path': help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
'coverage_check_help_page_path': help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule')
}
}
project_id: project.id,
can_edit: can_modify_approvers.to_s,
can_modify_author_settings: can_modify_author_settings.to_s,
can_modify_commiter_settings: can_modify_commiter_settings.to_s,
project_path: expose_path(api_v4_projects_path(id: project.id)),
settings_path: expose_path(api_v4_projects_approval_settings_path(id: project.id)),
approvals_path: expose_path(api_v4_projects_approvals_path(id: project.id)),
rules_path: expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)),
allow_multi_rule: project.multiple_approval_rules_available?.to_s,
eligible_approvers_docs_path: help_page_path('user/project/merge_requests/approvals/rules', anchor: 'eligible-approvers'),
security_approvals_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
security_configuration_path: project_security_configuration_path(project),
vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule')
}.tap do |data|
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, project.root_ancestor)
data[:approvals_path] = expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id))
data[:group_name] = project.root_ancestor.name
end
end
end
def status_checks_app_data(project)
......
.form-group
= label :approver_ids, class: 'label-bold' do
= _("Approval rules")
#js-mr-approvals-settings{ approvals_app_data }
#js-mr-approvals-settings{ data: approvals_app_data }
.text-center.gl-mt-3
= sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
......@@ -133,34 +133,42 @@ RSpec.describe 'Projects > Audit Events', :js do
end
end
describe 'changing merge request approval permission for authors and reviewers' do
before do
project.add_developer(pete)
end
it "appears in the project's audit events", :js do
visit edit_project_path(project)
page.within('[data-testid="merge-request-approval-settings"]') do
find('[data-testid="prevent-author-approval"] > input').set(false)
find('[data-testid="prevent-committers-approval"] > input').set(true)
click_button 'Save changes'
end
wait_for_all_requests
page.within('.sidebar-top-level-items') do
click_link 'Security & Compliance'
click_link 'Audit Events'
end
wait_for_all_requests
page.within('.audit-log-table') do
expect(page).to have_content(project.owner.name)
expect(page).to have_content('Changed prevent merge request approval from authors')
expect(page).to have_content('Changed prevent merge request approval from reviewers')
expect(page).to have_content(project.name)
context 'with the group_merge_request_approval_settings feature' do
where(feature_enabled: [true, false])
with_them do
describe 'changing merge request approval permission for authors and reviewers' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: feature_enabled)
stub_licensed_features(group_merge_request_approval_settings: feature_enabled)
project.add_developer(pete)
end
it "appears in the project's audit events", :js do
visit edit_project_path(project)
page.within('[data-testid="merge-request-approval-settings"]') do
find('[data-testid="prevent-author-approval"] > input').set(false)
find('[data-testid="prevent-committers-approval"] > input').set(true)
click_button 'Save changes'
end
wait_for_all_requests
page.within('.sidebar-top-level-items') do
click_link 'Security & Compliance'
click_link 'Audit Events'
end
wait_for_all_requests
page.within('.audit-log-table') do
expect(page).to have_content(project.owner.name)
expect(page).to have_content('Changed prevent merge request approval from authors')
expect(page).to have_content('Changed prevent merge request approval from reviewers')
expect(page).to have_content(project.name)
end
end
end
end
end
......
......@@ -7,11 +7,13 @@ import {
PROJECT_APPROVAL_SETTINGS_LABELS_I18N,
APPROVAL_SETTINGS_I18N,
} from 'ee/approvals/constants';
import { groupApprovalsMappers } from 'ee/approvals/mappers';
import { mergeRequestApprovalSettingsMappers } from 'ee/approvals/mappers';
import createStore from 'ee/approvals/stores';
import approvalSettingsModule from 'ee/approvals/stores/modules/approval_settings/';
import projectSettingsModule from 'ee/approvals/stores/modules/project_settings/';
import { extendedWrapper } from 'helpers/vue_test_utils_helper';
import waitForPromises from 'helpers/wait_for_promises';
import { sprintf } from '~/locale';
import { createGroupApprovalsPayload, createGroupApprovalsState } from '../mocks';
const localVue = createLocalVue();
......@@ -26,7 +28,7 @@ describe('ApprovalSettings', () => {
const approvalSettingsPath = 'groups/22/merge_request_approval_settings';
const setupStore = (data = {}, initialData) => {
const module = approvalSettingsModule(groupApprovalsMappers);
const module = approvalSettingsModule(mergeRequestApprovalSettingsMappers);
module.state.settings = data;
module.state.initialSettings = initialData || data;
......@@ -36,7 +38,7 @@ describe('ApprovalSettings', () => {
jest.spyOn(actions, 'dismissErrorMessage').mockImplementation();
jest.spyOn(actions, 'dismissSuccessMessage').mockImplementation();
store = createStore({ approvalSettings: module });
store = createStore({ approvalSettings: module, approvals: projectSettingsModule() });
};
const createWrapper = (props = {}) => {
......@@ -185,8 +187,10 @@ describe('ApprovalSettings', () => {
${'remove-approvals-on-push'} | ${'setRemoveApprovalsOnPush'} | ${'removeApprovalsOnPush'} | ${'removeApprovalsOnPushLabel'}
`('with the $testid checkbox', ({ testid, action, setting, labelKey }) => {
let checkbox = null;
const groupName = 'GitLab Org';
beforeEach(async () => {
store.state.settings.groupName = groupName;
jest.spyOn(store, 'dispatch').mockImplementation();
createWrapper();
await waitForPromises();
......@@ -205,11 +209,22 @@ describe('ApprovalSettings', () => {
expect(checkbox.props('label')).toBe(PROJECT_APPROVAL_SETTINGS_LABELS_I18N[labelKey]);
});
it('sets the locked and lockedText based on the setting values', () => {
expect(checkbox.props()).toMatchObject({
locked: settings[setting].locked,
lockedText: APPROVAL_SETTINGS_I18N.lockedByAdmin,
});
it('sets the locked prop', () => {
expect(checkbox.props('locked')).toBe(settings[setting].locked);
});
it('sets the lockedText prop', () => {
const { inheritedFrom, locked } = settings[setting];
let expectedText = null;
if (locked && inheritedFrom === 'group') {
expectedText = sprintf(APPROVAL_SETTINGS_I18N.lockedByGroupOwner, { groupName });
} else if (locked && inheritedFrom === 'instance') {
expectedText = APPROVAL_SETTINGS_I18N.lockedByAdmin;
}
expect(checkbox.props('lockedText')).toBe(expectedText);
});
it(`triggers the action ${action} when the value is changed`, async () => {
......@@ -273,13 +288,7 @@ describe('ApprovalSettings', () => {
describe('locked settings', () => {
beforeEach(() => {
setupStore({
...settings,
preventAuthorApproval: {
...settings.preventAuthorApproval,
locked: false,
},
});
setupStore(createGroupApprovalsState(false).settings);
});
it.each`
......
......@@ -7,7 +7,7 @@ import Vuex from 'vuex';
import ApprovalSettings from 'ee/approvals/components/approval_settings.vue';
import GroupSettingsApp from 'ee/approvals/components/group_settings/app.vue';
import { GROUP_APPROVAL_SETTINGS_LABELS_I18N } from 'ee/approvals/constants';
import { groupApprovalsMappers } from 'ee/approvals/mappers';
import { mergeRequestApprovalSettingsMappers } from 'ee/approvals/mappers';
import { createStoreOptions } from 'ee/approvals/stores';
import approvalSettingsModule from 'ee/approvals/stores/modules/approval_settings';
import SettingsBlock from '~/vue_shared/components/settings/settings_block.vue';
......@@ -44,7 +44,9 @@ describe('EE Approvals Group Settings App', () => {
axiosMock = new MockAdapter(axios);
axiosMock.onGet('*');
store = createStoreOptions({ approvalSettings: approvalSettingsModule(groupApprovalsMappers) });
store = createStoreOptions({
approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
});
});
afterEach(() => {
......
import { groupApprovalsMappers } from 'ee/approvals/mappers';
import { mergeRequestApprovalSettingsMappers } from 'ee/approvals/mappers';
import { createGroupApprovalsPayload, createGroupApprovalsState } from './mocks';
describe('approvals mappers', () => {
describe('groupApprovalsMappers', () => {
describe('mergeRequestApprovalSettingsMappers', () => {
const approvalsState = createGroupApprovalsState();
const approvalsFetchPayload = createGroupApprovalsPayload();
const approvalsUpdatePayload = {
......@@ -14,13 +14,13 @@ describe('approvals mappers', () => {
};
it('maps data to state', () => {
expect(groupApprovalsMappers.mapDataToState(approvalsFetchPayload)).toStrictEqual(
approvalsState.settings,
);
expect(
mergeRequestApprovalSettingsMappers.mapDataToState(approvalsFetchPayload),
).toStrictEqual(approvalsState.settings);
});
it('maps state to payload', () => {
expect(groupApprovalsMappers.mapStateToPayload(approvalsState)).toStrictEqual(
expect(mergeRequestApprovalSettingsMappers.mapStateToPayload(approvalsState)).toStrictEqual(
approvalsUpdatePayload,
);
});
......
......@@ -44,8 +44,8 @@ export const createGroupApprovalsPayload = () => ({
},
allow_committer_approval: {
value: true,
locked: false,
inherited_from: null,
locked: true,
inherited_from: 'group',
},
allow_overrides_to_approver_list_per_merge_request: {
value: true,
......@@ -64,31 +64,31 @@ export const createGroupApprovalsPayload = () => ({
},
});
export const createGroupApprovalsState = () => ({
export const createGroupApprovalsState = (locked = null) => ({
settings: {
preventAuthorApproval: {
inherited_from: 'instance',
locked: true,
inheritedFrom: 'instance',
locked: locked ?? true,
value: false,
},
preventCommittersApproval: {
inherited_from: null,
locked: false,
inheritedFrom: 'group',
locked: locked ?? true,
value: false,
},
preventMrApprovalRuleEdit: {
inherited_from: null,
locked: false,
inheritedFrom: null,
locked: locked ?? false,
value: false,
},
removeApprovalsOnPush: {
inherited_from: null,
locked: null,
inheritedFrom: null,
locked,
value: false,
},
requireUserPassword: {
inherited_from: null,
locked: null,
inheritedFrom: null,
locked,
value: true,
},
},
......
......@@ -373,24 +373,43 @@ RSpec.describe ProjectsHelper do
allow(helper).to receive(:can?).and_return(true)
end
it 'returns the correct data' do
expect(subject[:data]).to eq({
project_id: project.id,
can_edit: 'true',
can_modify_author_settings: 'true',
can_modify_commiter_settings: 'true',
approvals_path: expose_path(api_v4_projects_approvals_path(id: project.id)),
project_path: expose_path(api_v4_projects_path(id: project.id)),
settings_path: expose_path(api_v4_projects_approval_settings_path(id: project.id)),
rules_path: expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)),
allow_multi_rule: project.multiple_approval_rules_available?.to_s,
eligible_approvers_docs_path: help_page_path('user/project/merge_requests/approvals/rules', anchor: 'eligible-approvers'),
security_approvals_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
security_configuration_path: project_security_configuration_path(project),
vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule')
})
context 'with group_merge_request_approval_settings_feature_flag disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'returns the correct data' do
expect(subject).to eq({
project_id: project.id,
can_edit: 'true',
can_modify_author_settings: 'true',
can_modify_commiter_settings: 'true',
approvals_path: expose_path(api_v4_projects_approvals_path(id: project.id)),
project_path: expose_path(api_v4_projects_path(id: project.id)),
settings_path: expose_path(api_v4_projects_approval_settings_path(id: project.id)),
rules_path: expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)),
allow_multi_rule: project.multiple_approval_rules_available?.to_s,
eligible_approvers_docs_path: help_page_path('user/project/merge_requests/approvals/rules', anchor: 'eligible-approvers'),
security_approvals_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
security_configuration_path: project_security_configuration_path(project),
vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule')
})
end
end
context 'with group_merge_request_approval_settings_feature_flag enabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
it 'returns the correct data' do
expect(subject).to include(
approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)),
group_name: project.root_ancestor.name
)
end
end
end
......
......@@ -4276,6 +4276,9 @@ msgstr ""
msgid "ApprovalSettings|This setting is configured at the instance level and can only be changed by an administrator."
msgstr ""
msgid "ApprovalSettings|This setting is configured in %{groupName} and can only be changed by an administrator or group owner."
msgstr ""
msgid "ApprovalStatusTooltip|Adheres to separation of duties"
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