Merge branch '53361-fresh-protected-branches' into 'master'

Allow protected branch creation via web and API

Closes #53361

See merge request gitlab-org/gitlab-ce!24969

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: refs).present?
+    self.matching(ref_name, protected_refs: protected_refs(project)).present?
+  end
+  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

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 <strong>anyone</strong> from force pushing to the branch
         %li prevent <strong>anyone</strong> from deleting the branch

changelogs/unreleased/53361-fresh-protected-branches.yml

+---
+title: Allow creation of branches that match a wildcard protection, except directly through git
+merge_request: 24969
+author:
+type: added

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]

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

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

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' }

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

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