Merge branch '14898-protected-branches-developer-can-not-push-without-permission' into 'master'

CHANGELOG

@@ -22,6 +22,7 @@ v 8.11.0 (unreleased)
   - Remove unused images (ClemMakesApps)
   - Limit git rev-list output count to one in forced push check
   - Clean up unused routes (Josef Strzibny)
+  - Fix issue on empty project to allow developers to only push to protected branches if given permission
   - Add green outline to New Branch button. !5447 (winniehell)
   - Improve performance of syntax highlighting Markdown code blocks
   - Update to gitlab_git 10.4.1 and take advantage of preserved Ref objects

app/models/project.rb

@@ -865,10 +865,16 @@ class Project < ActiveRecord::Base
 
   # Check if current branch name is marked as protected in the system
   def protected_branch?(branch_name)
+    return true if empty_repo? && default_branch_protected?
+
     @protected_branches ||= self.protected_branches.to_a
     ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present?
   end
 
+  def user_can_push_to_empty_repo?(user)
+    !default_branch_protected? || team.max_member_access(user.id) > Gitlab::Access::DEVELOPER
+  end
+
   def forked?
     !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?)
   end
@@ -1260,6 +1266,11 @@ class Project < ActiveRecord::Base
 
   private
 
+  def default_branch_protected?
+    current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_FULL ||
+      current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE
+  end
+
   def authorized_for_user_by_group?(user, min_access_level)
     member = user.group_members.find_by(source_id: group)
 

lib/gitlab/user_access.rb

@@ -30,6 +30,8 @@ module Gitlab
       return false unless user
 
       if project.protected_branch?(ref)
+        return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user)
+
         access_levels = project.protected_branches.matching(ref).map(&:push_access_level)
         access_levels.any? { |access_level| access_level.check_access(user) }
       else

spec/lib/gitlab/user_access_spec.rb

@@ -9,35 +9,80 @@ describe Gitlab::UserAccess, lib: true do
     describe 'push to none protected branch' do
       it 'returns true if user is a master' do
         project.team << [user, :master]
+
         expect(access.can_push_to_branch?('random_branch')).to be_truthy
       end
 
       it 'returns true if user is a developer' do
         project.team << [user, :developer]
+
         expect(access.can_push_to_branch?('random_branch')).to be_truthy
       end
 
       it 'returns false if user is a reporter' do
         project.team << [user, :reporter]
+
         expect(access.can_push_to_branch?('random_branch')).to be_falsey
       end
     end
 
+    describe 'push to empty project' do
+      let(:empty_project) { create(:project_empty_repo) }
+      let(:project_access) { Gitlab::UserAccess.new(user, project: empty_project) }
+
+      it 'returns true if user is master' do
+        empty_project.team << [user, :master]
+
+        expect(project_access.can_push_to_branch?('master')).to be_truthy
+      end
+
+      it 'returns false if user is developer and project is fully protected' do
+        empty_project.team << [user, :developer]
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL)
+
+        expect(project_access.can_push_to_branch?('master')).to be_falsey
+      end
+
+      it 'returns false if user is developer and it is not allowed to push new commits but can merge into branch' do
+        empty_project.team << [user, :developer]
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+
+        expect(project_access.can_push_to_branch?('master')).to be_falsey
+      end
+
+      it 'returns true if user is developer and project is unprotected' do
+        empty_project.team << [user, :developer]
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+
+        expect(project_access.can_push_to_branch?('master')).to be_truthy
+      end
+
+      it 'returns true if user is developer and project grants developers permission' do
+        empty_project.team << [user, :developer]
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+
+        expect(project_access.can_push_to_branch?('master')).to be_truthy
+      end
+    end
+
     describe 'push to protected branch' do
       let(:branch) { create :protected_branch, project: project }
 
       it 'returns true if user is a master' do
         project.team << [user, :master]
+
         expect(access.can_push_to_branch?(branch.name)).to be_truthy
       end
 
       it 'returns false if user is a developer' do
         project.team << [user, :developer]
+
         expect(access.can_push_to_branch?(branch.name)).to be_falsey
       end
 
       it 'returns false if user is a reporter' do
         project.team << [user, :reporter]
+
         expect(access.can_push_to_branch?(branch.name)).to be_falsey
       end
     end
@@ -49,16 +94,19 @@ describe Gitlab::UserAccess, lib: true do
 
       it 'returns true if user is a master' do
         project.team << [user, :master]
+
         expect(access.can_push_to_branch?(@branch.name)).to be_truthy
       end
 
       it 'returns true if user is a developer' do
         project.team << [user, :developer]
+
         expect(access.can_push_to_branch?(@branch.name)).to be_truthy
       end
 
       it 'returns false if user is a reporter' do
         project.team << [user, :reporter]
+
         expect(access.can_push_to_branch?(@branch.name)).to be_falsey
       end
     end
@@ -70,19 +118,21 @@ describe Gitlab::UserAccess, lib: true do
 
       it 'returns true if user is a master' do
         project.team << [user, :master]
+
         expect(access.can_merge_to_branch?(@branch.name)).to be_truthy
       end
 
       it 'returns true if user is a developer' do
         project.team << [user, :developer]
+
         expect(access.can_merge_to_branch?(@branch.name)).to be_truthy
       end
 
       it 'returns false if user is a reporter' do
         project.team << [user, :reporter]
+
         expect(access.can_merge_to_branch?(@branch.name)).to be_falsey
       end
     end
-
   end
 end

spec/models/project_spec.rb

@@ -69,6 +69,7 @@ describe Project, models: true do
     it { is_expected.to include_module(Gitlab::ConfigHelper) }
     it { is_expected.to include_module(Gitlab::ShellAdapter) }
     it { is_expected.to include_module(Gitlab::VisibilityLevel) }
+    it { is_expected.to include_module(Gitlab::CurrentSettings) }
     it { is_expected.to include_module(Referable) }
     it { is_expected.to include_module(Sortable) }
   end
@@ -1070,28 +1071,97 @@ describe Project, models: true do
   end
 
   describe '#protected_branch?' do
+    context 'existing project' do
+      let(:project) { create(:project) }
+
+      it 'returns true when the branch matches a protected branch via direct match' do
+        project.protected_branches.create!(name: 'foo')
+
+        expect(project.protected_branch?('foo')).to eq(true)
+      end
+
+      it 'returns true when the branch matches a protected branch via wildcard match' do
+        project.protected_branches.create!(name: 'production/*')
+
+        expect(project.protected_branch?('production/some-branch')).to eq(true)
+      end
+
+      it 'returns false when the branch does not match a protected branch via direct match' do
+        expect(project.protected_branch?('foo')).to eq(false)
+      end
+
+      it 'returns false when the branch does not match a protected branch via wildcard match' do
+        project.protected_branches.create!(name: 'production/*')
+
+        expect(project.protected_branch?('staging/some-branch')).to eq(false)
+      end
+    end
+
+    context "new project" do
+      let(:project) { create(:empty_project) }
+
+      it 'returns false when default_protected_branch is unprotected' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+
+        expect(project.protected_branch?('master')).to be false
+      end
+
+      it 'returns false when default_protected_branch lets developers push' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+
+        expect(project.protected_branch?('master')).to be false
+      end
+
+      it 'returns true when default_branch_protection does not let developers push but let developer merge branches' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+
+        expect(project.protected_branch?('master')).to be true
+      end
+
+      it 'returns true when default_branch_protection is in full protection' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL)
+
+        expect(project.protected_branch?('master')).to be true
+      end
+    end
+  end
+
+  describe '#user_can_push_to_empty_repo?' do
     let(:project) { create(:empty_project) }
+    let(:user)    { create(:user) }
 
-    it 'returns true when the branch matches a protected branch via direct match' do
-      project.protected_branches.create!(name: 'foo')
+    it 'returns false when default_branch_protection is in full protection and user is developer' do
+      project.team << [user, :developer]
+      stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL)
 
-      expect(project.protected_branch?('foo')).to eq(true)
+      expect(project.user_can_push_to_empty_repo?(user)).to be_falsey
     end
 
-    it 'returns true when the branch matches a protected branch via wildcard match' do
-      project.protected_branches.create!(name: 'production/*')
+    it 'returns false when default_branch_protection only lets devs merge and user is dev' do
+      project.team << [user, :developer]
+      stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
 
-      expect(project.protected_branch?('production/some-branch')).to eq(true)
+      expect(project.user_can_push_to_empty_repo?(user)).to be_falsey
     end
 
-    it 'returns false when the branch does not match a protected branch via direct match' do
-      expect(project.protected_branch?('foo')).to eq(false)
+    it 'returns true when default_branch_protection lets devs push and user is developer' do
+      project.team << [user, :developer]
+      stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+
+      expect(project.user_can_push_to_empty_repo?(user)).to be_truthy
+    end
+
+    it 'returns true when default_branch_protection is unprotected and user is developer' do
+      project.team << [user, :developer]
+      stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+
+      expect(project.user_can_push_to_empty_repo?(user)).to be_truthy
     end
 
-    it 'returns false when the branch does not match a protected branch via wildcard match' do
-      project.protected_branches.create!(name: 'production/*')
+    it 'returns true when user is master' do
+      project.team << [user, :master]
 
-      expect(project.protected_branch?('staging/some-branch')).to eq(false)
+      expect(project.user_can_push_to_empty_repo?(user)).to be_truthy
     end
   end