diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index d075440b147..597431be65a 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -18,13 +18,23 @@ class ProtectedBranch < ActiveRecord::Base def self.protected?(project, ref_name) return true if project.empty_repo? && default_branch_protected? - refs = project.protected_branches.select(:name) + self.matching(ref_name, protected_refs: protected_refs(project)).present? + end - self.matching(ref_name, protected_refs: refs).present? + def self.any_protected?(project, ref_names) + protected_refs(project).any? do |protected_ref| + ref_names.any? do |ref_name| + protected_ref.matches?(ref_name) + end + end end def self.default_branch_protected? Gitlab::CurrentSettings.default_branch_protection == Gitlab::Access::PROTECTION_FULL || Gitlab::CurrentSettings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE end + + def self.protected_refs(project) + project.protected_branches.select(:name) + end end diff --git a/app/views/projects/protected_branches/shared/_index.html.haml b/app/views/projects/protected_branches/shared/_index.html.haml index 539b184e5c2..4997770321e 100644 --- a/app/views/projects/protected_branches/shared/_index.html.haml +++ b/app/views/projects/protected_branches/shared/_index.html.haml @@ -12,7 +12,7 @@ %p By default, protected branches are designed to: %ul - %li prevent their creation, if not already created, from everybody except Maintainers + %li prevent their creation, if not already created, from everybody except users who are allowed to merge %li prevent pushes from everybody except Maintainers %li prevent anyone from force pushing to the branch %li prevent anyone from deleting the branch diff --git a/changelogs/unreleased/53361-fresh-protected-branches.yml b/changelogs/unreleased/53361-fresh-protected-branches.yml new file mode 100644 index 00000000000..55080e719b7 --- /dev/null +++ b/changelogs/unreleased/53361-fresh-protected-branches.yml @@ -0,0 +1,5 @@ +--- +title: Allow creation of branches that match a wildcard protection, except directly through git +merge_request: 24969 +author: +type: added diff --git a/doc/user/project/protected_branches.md b/doc/user/project/protected_branches.md index db706e5020e..3eb8123144f 100644 --- a/doc/user/project/protected_branches.md +++ b/doc/user/project/protected_branches.md @@ -10,7 +10,7 @@ created protected branches. By default, a protected branch does four simple things: - it prevents its creation, if not already created, from everybody except users - with Maintainer permission + who are allowed to merge - it prevents pushes from everybody except users with Maintainer permission - it prevents **anyone** from force pushing to the branch - it prevents **anyone** from deleting the branch @@ -94,6 +94,25 @@ all matching branches: ![Protected branch matches](img/protected_branches_matches.png) +## Creating a protected branch + +> [Introduced][https://gitlab.com/gitlab-org/gitlab-ce/issues/53361] in GitLab 11.9. + +When a protected branch or wildcard protected branches are set to +[**No one** is **Allowed to push**](#using-the-allowed-to-merge-and-allowed-to-push-settings), +Developers (and users with higher [permission levels](../permissions.md)) are allowed +to create a new protected branch, but only via the UI or through the API (to avoid +creating protected branches accidentally from the command line or from a Git +client application). + +To create a new branch through the user interface: + +1. Visit **Repository > Branches**. +1. Click on **New branch**. +1. Fill in the branch name and select an existing branch, tag, or commit that + the new branch will be based off. Only existing protected branches and commits + that are already in protected branches will be accepted. + ## Deleting a protected branch > [Introduced][ce-21393] in GitLab 9.3. @@ -125,6 +144,10 @@ for details about the pipelines security model. ## Changelog +**11.9** + +- [Allow protected branches to be created](https://gitlab.com/gitlab-org/gitlab-ce/issues/53361) by Developers (and users with higher permission levels) through the API and the user interface. + **9.2** - Allow deletion of protected branches via the web interface [gitlab-org/gitlab-ce#21393][ce-21393] diff --git a/lib/gitlab/checks/branch_check.rb b/lib/gitlab/checks/branch_check.rb index d06b2df36f2..bd305ace0a0 100644 --- a/lib/gitlab/checks/branch_check.rb +++ b/lib/gitlab/checks/branch_check.rb @@ -9,13 +9,17 @@ module Gitlab non_master_delete_protected_branch: 'You are not allowed to delete protected branches from this project. Only a project maintainer or owner can delete a protected branch.', non_web_delete_protected_branch: 'You can only delete protected branches using the web interface.', merge_protected_branch: 'You are not allowed to merge code into protected branches on this project.', - push_protected_branch: 'You are not allowed to push code to protected branches on this project.' + push_protected_branch: 'You are not allowed to push code to protected branches on this project.', + create_protected_branch: 'You are not allowed to create protected branches on this project.', + invalid_commit_create_protected_branch: 'You can only use an existing protected branch ref as the basis of a new protected branch.', + non_web_create_protected_branch: 'You can only create protected branches using the web interface and API.' }.freeze LOG_MESSAGES = { delete_default_branch_check: "Checking if default branch is being deleted...", protected_branch_checks: "Checking if you are force pushing to a protected branch...", protected_branch_push_checks: "Checking if you are allowed to push to the protected branch...", + protected_branch_creation_checks: "Checking if you are allowed to create a protected branch...", protected_branch_deletion_checks: "Checking if you are allowed to delete the protected branch..." }.freeze @@ -42,13 +46,31 @@ module Gitlab end end - if deletion? + if creation? && protected_branch_creation_enabled? + protected_branch_creation_checks + elsif deletion? protected_branch_deletion_checks else protected_branch_push_checks end end + def protected_branch_creation_checks + logger.log_timed(LOG_MESSAGES[:protected_branch_creation_checks]) do + unless user_access.can_merge_to_branch?(branch_name) + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:create_protected_branch] + end + + unless safe_commit_for_new_protected_branch? + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:invalid_commit_create_protected_branch] + end + + unless updated_from_web? + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:non_web_create_protected_branch] + end + end + end + def protected_branch_deletion_checks logger.log_timed(LOG_MESSAGES[:protected_branch_deletion_checks]) do unless user_access.can_delete_branch?(branch_name) @@ -98,6 +120,10 @@ module Gitlab Gitlab::Routing.url_helpers.project_project_members_url(project) end + def protected_branch_creation_enabled? + Feature.enabled?(:protected_branch_creation, project, default_enabled: true) + end + def matching_merge_request? Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? end @@ -105,6 +131,10 @@ module Gitlab def forced_push? Gitlab::Checks::ForcePush.force_push?(project, oldrev, newrev) end + + def safe_commit_for_new_protected_branch? + ProtectedBranch.any_protected?(project, project.repository.branch_names_contains_sha(newrev)) + end end end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 980a8014409..9ef23cf849f 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -118,8 +118,8 @@ module Gitlab protected_refs: project.protected_tags) end - request_cache def protected?(kind, project, ref) - kind.protected?(project, ref) + request_cache def protected?(kind, project, refs) + kind.protected?(project, refs) end end end diff --git a/spec/lib/gitlab/checks/branch_check_spec.rb b/spec/lib/gitlab/checks/branch_check_spec.rb index 77366e91dca..f99fc639dbd 100644 --- a/spec/lib/gitlab/checks/branch_check_spec.rb +++ b/spec/lib/gitlab/checks/branch_check_spec.rb @@ -55,6 +55,106 @@ describe Gitlab::Checks::BranchCheck do end end + context 'branch creation' do + let(:oldrev) { '0000000000000000000000000000000000000000' } + let(:ref) { 'refs/heads/feature' } + + context 'protected branch creation feature is disabled' do + before do + stub_feature_flags(protected_branch_creation: false) + end + + context 'user is not allowed to push to protected branch' do + before do + allow(user_access) + .to receive(:can_push_to_branch?) + .and_return(false) + end + + it 'raises an error' do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to protected branches on this project.') + end + end + + context 'user is allowed to push to protected branch' do + before do + allow(user_access) + .to receive(:can_push_to_branch?) + .and_return(true) + end + + it 'does not raise an error' do + expect { subject.validate! }.not_to raise_error + end + end + end + + context 'protected branch creation feature is enabled' do + context 'user is not allowed to create protected branches' do + before do + allow(user_access) + .to receive(:can_merge_to_branch?) + .with('feature') + .and_return(false) + end + + it 'raises an error' do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to create protected branches on this project.') + end + end + + context 'user is allowed to create protected branches' do + before do + allow(user_access) + .to receive(:can_merge_to_branch?) + .with('feature') + .and_return(true) + + allow(project.repository) + .to receive(:branch_names_contains_sha) + .with(newrev) + .and_return(['branch']) + end + + context "newrev isn't in any protected branches" do + before do + allow(ProtectedBranch) + .to receive(:any_protected?) + .with(project, ['branch']) + .and_return(false) + end + + it 'raises an error' do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only use an existing protected branch ref as the basis of a new protected branch.') + end + end + + context 'newrev is included in a protected branch' do + before do + allow(ProtectedBranch) + .to receive(:any_protected?) + .with(project, ['branch']) + .and_return(true) + end + + context 'via web interface' do + let(:protocol) { 'web' } + + it 'allows branch creation' do + expect { subject.validate! }.not_to raise_error + end + end + + context 'via SSH' do + it 'raises an error' do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only create protected branches using the web interface and API.') + end + end + end + end + end + end + context 'branch deletion' do let(:newrev) { '0000000000000000000000000000000000000000' } let(:ref) { 'refs/heads/feature' } diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 4c677200ae2..dafe7646366 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -190,4 +190,32 @@ describe ProtectedBranch do end end end + + describe '#any_protected?' do + context 'existing project' do + let(:project) { create(:project, :repository) } + + it 'returns true when any of the branch names match a protected branch via direct match' do + create(:protected_branch, project: project, name: 'foo') + + expect(described_class.any_protected?(project, ['foo', 'production/some-branch'])).to eq(true) + end + + it 'returns true when any of the branch matches a protected branch via wildcard match' do + create(:protected_branch, project: project, name: 'production/*') + + expect(described_class.any_protected?(project, ['foo', 'production/some-branch'])).to eq(true) + end + + it 'returns false when none of branches does not match a protected branch via direct match' do + expect(described_class.any_protected?(project, ['foo'])).to eq(false) + end + + it 'returns false when none of the branches does not match a protected branch via wildcard match' do + create(:protected_branch, project: project, name: 'production/*') + + expect(described_class.any_protected?(project, ['staging/some-branch'])).to eq(false) + end + end + end end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 47491f708e9..772d1fbee2b 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -45,8 +45,7 @@ describe ProjectPolicy do let(:base_maintainer_permissions) do %i[ push_to_delete_protected_branch update_project_snippet update_environment - update_deployment admin_project_snippet - admin_project_member admin_note admin_wiki admin_project + update_deployment admin_project_snippet admin_project_member admin_note admin_wiki admin_project admin_commit_status admin_build admin_container_image admin_pipeline admin_environment admin_deployment destroy_release add_cluster daily_statistics