Commit be02740f authored by Mycroft's avatar Mycroft

Add Allow force pushes option to Protected branches

parent 79e19c5e
......@@ -15,17 +15,23 @@ export default class ProtectedBranchCreate {
this.isLocalStorageAvailable = AccessorUtilities.isLocalStorageAccessSafe();
this.currentProjectUserDefaults = {};
this.buildDropdowns();
this.$forcePushToggle = this.$form.find('.js-force-push-toggle');
this.$codeOwnerToggle = this.$form.find('.js-code-owner-toggle');
this.bindEvents();
}
bindEvents() {
this.$forcePushToggle.on('click', this.onForcePushToggleClick.bind(this));
if (this.hasLicense) {
this.$codeOwnerToggle.on('click', this.onCodeOwnerToggleClick.bind(this));
}
this.$form.on('submit', this.onFormSubmit.bind(this));
}
onForcePushToggleClick() {
this.$forcePushToggle.toggleClass('is-checked');
}
onCodeOwnerToggleClick() {
this.$codeOwnerToggle.toggleClass('is-checked');
}
......@@ -86,6 +92,7 @@ export default class ProtectedBranchCreate {
authenticity_token: this.$form.find('input[name="authenticity_token"]').val(),
protected_branch: {
name: this.$form.find('input[name="protected_branch[name]"]').val(),
allow_force_push: this.$forcePushToggle.hasClass('is-checked'),
code_owner_approval_required: this.$codeOwnerToggle.hasClass('is-checked'),
},
};
......
......@@ -14,6 +14,7 @@ export default class ProtectedBranchEdit {
this.$wrap = options.$wrap;
this.$allowedToMergeDropdown = this.$wrap.find('.js-allowed-to-merge');
this.$allowedToPushDropdown = this.$wrap.find('.js-allowed-to-push');
this.$forcePushToggle = this.$wrap.find('.js-force-push-toggle');
this.$codeOwnerToggle = this.$wrap.find('.js-code-owner-toggle');
this.$wraps[ACCESS_LEVELS.MERGE] = this.$allowedToMergeDropdown.closest(
......@@ -28,11 +29,23 @@ export default class ProtectedBranchEdit {
}
bindEvents() {
this.$forcePushToggle.on('click', this.onForcePushToggleClick.bind(this));
if (this.hasLicense) {
this.$codeOwnerToggle.on('click', this.onCodeOwnerToggleClick.bind(this));
}
}
onForcePushToggleClick() {
this.$forcePushToggle.toggleClass('is-checked');
this.$forcePushToggle.prop('disabled', true);
const formData = {
allow_force_push: this.$forcePushToggle.hasClass('is-checked'),
};
this.updateProtectedBranch(formData, () => this.$forcePushToggle.prop('disabled', false));
}
onCodeOwnerToggleClick() {
this.$codeOwnerToggle.toggleClass('is-checked');
this.$codeOwnerToggle.prop('disabled', true);
......@@ -41,17 +54,15 @@ export default class ProtectedBranchEdit {
code_owner_approval_required: this.$codeOwnerToggle.hasClass('is-checked'),
};
this.updateCodeOwnerApproval(formData);
this.updateProtectedBranch(formData, () => this.$codeOwnerToggle.prop('disabled', false));
}
updateCodeOwnerApproval(formData) {
updateProtectedBranch(formData, callback) {
axios
.patch(this.$wrap.data('url'), {
protected_branch: formData,
})
.then(() => {
this.$codeOwnerToggle.prop('disabled', false);
})
.then(callback)
.catch(() => {
flash(__('Failed to update branch!'));
});
......
......@@ -21,6 +21,7 @@ class Projects::ProtectedBranchesController < Projects::ProtectedRefsController
def protected_ref_params(*attrs)
attrs = ([:name,
:allow_force_push,
merge_access_levels_attributes: access_level_attributes,
push_access_levels_attributes: access_level_attributes] + attrs).uniq
......
......@@ -7,6 +7,9 @@ class ProtectedBranch < ApplicationRecord
scope :requiring_code_owner_approval,
-> { where(code_owner_approval_required: true) }
scope :allowing_force_push,
-> { where(allow_force_push: true) }
protected_ref_access_levels :merge, :push
def self.protected_ref_accessible_to?(ref, user, project:, action:, protected_refs: nil)
......@@ -26,6 +29,12 @@ class ProtectedBranch < ApplicationRecord
self.matching(ref_name, protected_refs: protected_refs(project)).present?
end
def self.allow_force_push?(project, ref_name)
return false unless ::Feature.enabled?(:allow_force_push_to_protected_branches, project)
project.protected_branches.allowing_force_push.matching(ref_name).any?
end
def self.any_protected?(project, ref_names)
protected_refs(project).any? do |protected_ref|
ref_names.any? do |ref_name|
......
......@@ -9,10 +9,15 @@ module ProtectedBranches
def protected_branch_params
{
name: params[:name],
allow_force_push: allow_force_push?,
push_access_levels_attributes: AccessLevelParams.new(:push, params).access_levels,
merge_access_levels_attributes: AccessLevelParams.new(:merge, params).access_levels
}
end
def allow_force_push?
params[:allow_force_push] || false
end
end
end
......
......@@ -23,6 +23,12 @@
%th
= s_("ProtectedBranch|Allowed to push")
- if ::Feature.enabled?(:allow_force_push_to_protected_branches, @project)
%th
= s_("ProtectedBranch|Allow force push")
%span.has-tooltip{ data: { container: 'body' }, title: s_('ProtectedBranch|Allow force push for all users with push access.'), 'aria-hidden': 'true' }
= sprite_icon('question', size: 16, css_class: 'gl-text-gray-500')
= render_if_exists 'projects/protected_branches/ee/code_owner_approval_table_head'
- if can_admin_project
......
......@@ -21,6 +21,13 @@
= f.label :push_access_levels_attributes, s_("ProtectedBranch|Allowed to push:"), class: 'col-md-2 text-left text-md-right'
.col-md-10
= yield :push_access_levels
- if ::Feature.enabled?(:allow_force_push_to_protected_branches, @project)
.form-group.row
= f.label :allow_force_push, s_("ProtectedBranch|Allow force push:"), class: 'col-md-2 gl-text-left text-md-right'
.col-md-10
= render "shared/buttons/project_feature_toggle", class_list: "js-force-push-toggle project-feature-toggle"
.form-text.gl-text-gray-600.gl-mt-0
= s_("ProtectedBranch|Allow force push for all users with push access.")
= render_if_exists 'projects/protected_branches/ee/code_owner_approval_form', f: f
.card-footer
= f.submit s_('ProtectedBranch|Protect'), class: 'btn-success gl-button btn', disabled: true, data: { qa_selector: 'protect_button' }
......@@ -33,3 +33,6 @@
%p.small
= _('Members of %{group} can also push to this branch: %{branch}') % { group: (group_push_access_levels.size > 1 ? 'these groups' : 'this group'), branch: group_push_access_levels.map(&:humanize).to_sentence }
- if ::Feature.enabled?(:allow_force_push_to_protected_branches, @project)
%td
= render "shared/buttons/project_feature_toggle", is_checked: protected_branch.allow_force_push, label: s_("ProtectedBranch|Toggle allow force push"), class_list: "js-force-push-toggle project-feature-toggle", data: { qa_selector: 'force_push_toggle_button', qa_branch_name: protected_branch.name }
---
title: Add Allow force push option to Protected branches
merge_request: 55261
author: Mycroft Kang @TaehyeokKang
type: added
---
name: allow_force_push_to_protected_branches
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55261
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/323431
milestone: '13.10'
type: development
group: group::source code
default_enabled: false
# frozen_string_literal: true
class AddAllowForcePushToProtectedBranches < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def up
with_lock_retries do
add_column :protected_branches, :allow_force_push, :boolean, default: false, null: false
end
end
def down
with_lock_retries do
remove_column :protected_branches, :allow_force_push
end
end
end
ec071087de45291ae8fc0d6d6e778d16a7411a934e4a301f62890061abcaed4c
\ No newline at end of file
......@@ -16675,7 +16675,8 @@ CREATE TABLE protected_branches (
name character varying NOT NULL,
created_at timestamp without time zone,
updated_at timestamp without time zone,
code_owner_approval_required boolean DEFAULT false NOT NULL
code_owner_approval_required boolean DEFAULT false NOT NULL,
allow_force_push boolean DEFAULT false NOT NULL
);
CREATE SEQUENCE protected_branches_id_seq
......@@ -56,7 +56,8 @@ Example response:
"access_level_description": "Maintainers"
}
],
"code_owner_approval_required": "false"
"allow_force_push":false,
"code_owner_approval_required": false
},
...
]
......@@ -88,7 +89,8 @@ Example response:
"access_level_description": "Example Merge Group"
}
],
"code_owner_approval_required": "false"
"allow_force_push":false,
"code_owner_approval_required": false
},
...
]
......@@ -129,7 +131,8 @@ Example response:
"access_level_description": "Maintainers"
}
],
"code_owner_approval_required": "false"
"allow_force_push":false,
"code_owner_approval_required": false
}
```
......@@ -158,7 +161,8 @@ Example response:
"access_level_description": "Example Merge Group"
}
],
"code_owner_approval_required": "false"
"allow_force_push":false,
"code_owner_approval_required": false
}
```
......@@ -182,6 +186,7 @@ curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" "https://gitla
| `push_access_level` | string | no | Access levels allowed to push (defaults: `40`, maintainer access level) |
| `merge_access_level` | string | no | Access levels allowed to merge (defaults: `40`, maintainer access level) |
| `unprotect_access_level` | string | no | Access levels allowed to unprotect (defaults: `40`, maintainer access level) |
| `allow_force_push` | boolean | no | Allow force push for all users with push access. (defaults: false) |
| `allowed_to_push` | array | no | **(PREMIUM)** Array of access levels allowed to push, with each described by a hash |
| `allowed_to_merge` | array | no | **(PREMIUM)** Array of access levels allowed to merge, with each described by a hash |
| `allowed_to_unprotect` | array | no | **(PREMIUM)** Array of access levels allowed to unprotect, with each described by a hash |
......@@ -211,7 +216,8 @@ Example response:
"access_level_description": "Maintainers"
}
],
"code_owner_approval_required": "false"
"allow_force_push":false,
"code_owner_approval_required": false
}
```
......@@ -248,7 +254,8 @@ Example response:
"access_level_description": "Maintainers"
}
],
"code_owner_approval_required": "false"
"allow_force_push":false,
"code_owner_approval_required": false
}
```
......@@ -291,7 +298,8 @@ Example response:
"access_level_description": "Maintainers"
}
],
"code_owner_approval_required": "false"
"allow_force_push":false,
"code_owner_approval_required": false
}
```
......@@ -354,6 +362,7 @@ Example response:
"group_id": null
}
],
"allow_force_push":false,
"code_owner_approval_required": false
}
```
......
......@@ -5,3 +5,7 @@
%td
= render partial: 'projects/settings/ee/access_level_dropdown', locals: { protected_branch: protected_branch, access_levels: protected_branch.push_access_levels, level_frequencies: access_level_frequencies(protected_branch.push_access_levels), input_basic_name: 'push_access_levels', disabled: !can_unprotect, toggle_class: 'js-allowed-to-push' }
- if ::Feature.enabled?(:allow_force_push_to_protected_branches, @project)
%td
= render "shared/buttons/project_feature_toggle", is_checked: protected_branch.allow_force_push, label: s_("ProtectedBranch|Toggle allow force push"), class_list: "js-force-push-toggle project-feature-toggle", data: { qa_selector: 'force_push_toggle_button', qa_branch_name: protected_branch.name }
......@@ -7,6 +7,7 @@ module API
expose :name
expose :push_access_levels, using: Entities::ProtectedRefAccess
expose :merge_access_levels, using: Entities::ProtectedRefAccess
expose :allow_force_push
end
end
end
......
......@@ -60,6 +60,9 @@ module API
optional :merge_access_level, type: Integer,
values: ProtectedBranch::MergeAccessLevel.allowed_access_levels,
desc: 'Access levels allowed to merge (defaults: `40`, maintainer access level)'
optional :allow_force_push, type: Boolean,
default: false,
desc: 'Allow force push for all users with push access.'
use :optional_params_ee
end
......
......@@ -51,7 +51,7 @@ module Gitlab
logger.log_timed(LOG_MESSAGES[:protected_branch_checks]) do
return unless ProtectedBranch.protected?(project, branch_name) # rubocop:disable Cop/AvoidReturnFromBlocks
if forced_push?
if forced_push? && !ProtectedBranch.allow_force_push?(project, branch_name)
raise GitAccess::ForbiddenError, ERROR_MESSAGES[:force_push_protected_branch]
end
end
......
......@@ -24446,6 +24446,15 @@ msgstr ""
msgid "ProtectedBranch|%{wildcards_link_start}Wildcards%{wildcards_link_end} such as %{code_tag_start}*-stable%{code_tag_end} or %{code_tag_start}production/*%{code_tag_end} are supported."
msgstr ""
msgid "ProtectedBranch|Allow force push"
msgstr ""
msgid "ProtectedBranch|Allow force push for all users with push access."
msgstr ""
msgid "ProtectedBranch|Allow force push:"
msgstr ""
msgid "ProtectedBranch|Allowed to merge"
msgstr ""
......@@ -24488,6 +24497,9 @@ msgstr ""
msgid "ProtectedBranch|There are currently no protected branches, protect a branch with the form above."
msgstr ""
msgid "ProtectedBranch|Toggle allow force push"
msgstr ""
msgid "ProtectedBranch|Toggle code owner approval"
msgstr ""
......
......@@ -7032,7 +7032,8 @@
"created_at": "2016-08-30T07:32:52.490Z",
"updated_at": "2016-08-30T07:32:52.490Z"
}
]
],
"allow_force_push":false
}
],
"protected_environments": [
......
import MockAdapter from 'axios-mock-adapter';
import $ from 'jquery';
import { TEST_HOST } from 'helpers/test_constants';
import { deprecatedCreateFlash as flash } from '~/flash';
import axios from '~/lib/utils/axios_utils';
import ProtectedBranchEdit from '~/protected_branches/protected_branch_edit';
jest.mock('~/flash');
const TEST_URL = `${TEST_HOST}/url`;
const IS_CHECKED_CLASS = 'is-checked';
describe('ProtectedBranchEdit', () => {
let mock;
beforeEach(() => {
setFixtures(`<div id="wrap" data-url="${TEST_URL}">
<button class="js-force-push-toggle">Toggle</button>
</div>`);
jest.spyOn(ProtectedBranchEdit.prototype, 'buildDropdowns').mockImplementation();
mock = new MockAdapter(axios);
});
const findForcePushesToggle = () => document.querySelector('.js-force-push-toggle');
const create = ({ isChecked = false }) => {
if (isChecked) {
findForcePushesToggle().classList.add(IS_CHECKED_CLASS);
}
return new ProtectedBranchEdit({ $wrap: $('#wrap'), hasLicense: false });
};
afterEach(() => {
mock.restore();
});
describe('when unchecked toggle button', () => {
let toggle;
beforeEach(() => {
create({ isChecked: false });
toggle = findForcePushesToggle();
});
it('is not changed', () => {
expect(toggle).not.toHaveClass(IS_CHECKED_CLASS);
expect(toggle).not.toBeDisabled();
});
describe('when clicked', () => {
beforeEach(() => {
mock.onPatch(TEST_URL, { protected_branch: { allow_force_push: true } }).replyOnce(200, {});
toggle.click();
});
it('checks and disables button', () => {
expect(toggle).toHaveClass(IS_CHECKED_CLASS);
expect(toggle).toBeDisabled();
});
it('sends update to BE', () =>
axios.waitForAll().then(() => {
// Args are asserted in the `.onPatch` call
expect(mock.history.patch).toHaveLength(1);
expect(toggle).not.toBeDisabled();
expect(flash).not.toHaveBeenCalled();
}));
});
describe('when clicked and BE error', () => {
beforeEach(() => {
mock.onPatch(TEST_URL).replyOnce(500);
toggle.click();
});
it('flashes error', () =>
axios.waitForAll().then(() => {
expect(flash).toHaveBeenCalled();
}));
});
});
});
......@@ -70,6 +70,82 @@ RSpec.describe Gitlab::Checks::BranchCheck do
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, 'You are not allowed to push code to protected branches on this project.')
end
context 'when user has push access' do
before do
allow(user_access)
.to receive(:can_push_to_branch?)
.and_return(true)
end
context 'if protected branches is allowed to force push' do
before do
allow(ProtectedBranch)
.to receive(:allow_force_push?)
.with(project, 'master')
.and_return(true)
end
it 'allows force push' do
expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
expect { subject.validate! }.not_to raise_error
end
end
context 'if protected branches is not allowed to force push' do
before do
allow(ProtectedBranch)
.to receive(:allow_force_push?)
.with(project, 'master')
.and_return(false)
end
it 'prevents force push' do
expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
expect { subject.validate! }.to raise_error
end
end
end
context 'when user does not have push access' do
before do
allow(user_access)
.to receive(:can_push_to_branch?)
.and_return(false)
end
context 'if protected branches is allowed to force push' do
before do
allow(ProtectedBranch)
.to receive(:allow_force_push?)
.with(project, 'master')
.and_return(true)
end
it 'prevents force push' do
expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
expect { subject.validate! }.to raise_error
end
end
context 'if protected branches is not allowed to force push' do
before do
allow(ProtectedBranch)
.to receive(:allow_force_push?)
.with(project, 'master')
.and_return(false)
end
it 'prevents force push' do
expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
expect { subject.validate! }.to raise_error
end
end
end
context 'when project repository is empty' do
let(:project) { create(:project) }
......
......@@ -501,6 +501,7 @@ ProtectedBranch:
- name
- created_at
- updated_at
- allow_force_push
- code_owner_approval_required
ProtectedTag:
- id
......
......@@ -207,6 +207,28 @@ RSpec.describe ProtectedBranch do
end
end
describe "#allow_force_push?" do
context "when the attr allow_force_push is true" do
let(:subject_branch) { create(:protected_branch, allow_force_push: true, name: "foo") }
it "returns true" do
project = subject_branch.project
expect(described_class.allow_force_push?(project, "foo")).to eq(true)
end
end
context "when the attr allow_force_push is false" do
let(:subject_branch) { create(:protected_branch, allow_force_push: false, name: "foo") }
it "returns false" do
project = subject_branch.project
expect(described_class.allow_force_push?(project, "foo")).to eq(false)
end
end
end
describe '#any_protected?' do
context 'existing project' do
let(:project) { create(:project, :repository) }
......
......@@ -68,6 +68,7 @@ RSpec.describe API::ProtectedBranches do
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(false)
expect(json_response['push_access_levels'][0]['access_level']).to eq(::Gitlab::Access::MAINTAINER)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(::Gitlab::Access::MAINTAINER)
end
......@@ -132,6 +133,7 @@ RSpec.describe API::ProtectedBranches do
expect(response).to have_gitlab_http_status(:created)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(false)
expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::MAINTAINER)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::MAINTAINER)
end
......@@ -141,6 +143,7 @@ RSpec.describe API::ProtectedBranches do
expect(response).to have_gitlab_http_status(:created)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(false)
expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::DEVELOPER)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::MAINTAINER)
end
......@@ -150,6 +153,7 @@ RSpec.describe API::ProtectedBranches do
expect(response).to have_gitlab_http_status(:created)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(false)
expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::MAINTAINER)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::DEVELOPER)
end
......@@ -159,6 +163,7 @@ RSpec.describe API::ProtectedBranches do
expect(response).to have_gitlab_http_status(:created)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(false)
expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::DEVELOPER)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::DEVELOPER)
end
......@@ -168,6 +173,7 @@ RSpec.describe API::ProtectedBranches do
expect(response).to have_gitlab_http_status(:created)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(false)
expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::NO_ACCESS)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::MAINTAINER)
end
......@@ -177,6 +183,7 @@ RSpec.describe API::ProtectedBranches do
expect(response).to have_gitlab_http_status(:created)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(false)
expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::MAINTAINER)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::NO_ACCESS)
end
......@@ -186,10 +193,21 @@ RSpec.describe API::ProtectedBranches do
expect(response).to have_gitlab_http_status(:created)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(false)
expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::NO_ACCESS)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::NO_ACCESS)
end
it 'protects a single branch and allows force pushes' do
post post_endpoint, params: { name: branch_name, allow_force_push: true }
expect(response).to have_gitlab_http_status(:created)
expect(json_response['name']).to eq(branch_name)
expect(json_response['allow_force_push']).to eq(true)
expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::MAINTAINER)
expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::MAINTAINER)
end
it 'returns a 409 error if the same branch is protected twice' do
post post_endpoint, params: { name: protected_name }
......
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