From edc5f4018e45327421e112de18d53bfbdabd38f9 Mon Sep 17 00:00:00 2001 From: tiagonbotelho Date: Mon, 1 Aug 2016 10:06:57 +0100 Subject: [PATCH] developer cannot push to protected branch when project is empty or he has not been granted permission to do so --- CHANGELOG | 1 + app/models/project.rb | 16 ++++ spec/lib/gitlab/user_access_spec.rb | 39 +++++++++ spec/models/project_spec.rb | 119 +++++++++++++++++++++++++--- 4 files changed, 164 insertions(+), 11 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 4daf9cd9092..8f3b236b7c2 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -13,6 +13,7 @@ v 8.11.0 (unreleased) - Environments have an url to link to - 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) - Update to gitlab_git 10.4.1 and take advantage of preserved Ref objects - Retrieve rendered HTML from cache in one request diff --git a/app/models/project.rb b/app/models/project.rb index 83b848ded8b..507813bccf8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -870,10 +870,22 @@ 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 developers_can_push_to_protected_branch?(branch_name) + return true if empty_repo? && !default_branch_protected? + + protected_branches.matching(branch_name).any?(&:developers_can_push) + end + + def developers_can_merge_to_protected_branch?(branch_name) + protected_branches.matching(branch_name).any?(&:developers_can_merge) + end + def forked? !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?) end @@ -1265,6 +1277,10 @@ class Project < ActiveRecord::Base private + def default_branch_protected? + current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_FULL + end + def authorized_for_user_by_group?(user, min_access_level) member = user.group_members.find_by(source_id: group) diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index 5bb095366fa..26918f7b82d 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -23,6 +23,45 @@ describe Gitlab::UserAccess, lib: true do 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 } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e365e4e98b2..1e6f735699b 100644 --- a/spec/models/project_spec.rb +++ b/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,124 @@ 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 "#developers_can_push_to_protected_branch?" do let(:project) { create(:empty_project) } - it 'returns true when the branch matches a protected branch via direct match' do - project.protected_branches.create!(name: 'foo') + context "when the branch matches a protected branch via direct match" do + it "returns true if 'Developers can Push' is turned on" do + create(:protected_branch, name: "production", project: project, developers_can_push: true) - expect(project.protected_branch?('foo')).to eq(true) + expect(project.developers_can_push_to_protected_branch?('production')).to be true + end + + it "returns false if 'Developers can Push' is turned off" do + create(:protected_branch, name: "production", project: project, developers_can_push: false) + + expect(project.developers_can_push_to_protected_branch?('production')).to be false + end end - it 'returns true when the branch matches a protected branch via wildcard match' do - project.protected_branches.create!(name: 'production/*') + context "when project is new" do + it "returns true if project is unprotected" do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE) - expect(project.protected_branch?('production/some-branch')).to eq(true) + expect(project.developers_can_push_to_protected_branch?('master')).to be true + end + + it "returns true if project allows developers to push to protected branch" do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH) + + expect(project.developers_can_push_to_protected_branch?('master')).to be true + end + + it "returns false if project does not let developer push to protected branch but let them merge branches" do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + + expect(project.developers_can_push_to_protected_branch?('master')).to be false + end + + it "returns false if project is on full protection mode" do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL) + + expect(project.developers_can_push_to_protected_branch?('master')).to be false + end 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) + context "when the branch matches a protected branch via wilcard match" do + it "returns true if 'Developers can Push' is turned on" do + create(:protected_branch, name: "production/*", project: project, developers_can_push: true) + + expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be true + end + + it "returns false if 'Developers can Push' is turned off" do + create(:protected_branch, name: "production/*", project: project, developers_can_push: false) + + expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be false + end end - it 'returns false when the branch does not match a protected branch via wildcard match' do - project.protected_branches.create!(name: 'production/*') + context "when the branch does not match a protected branch" do + it "returns false" do + create(:protected_branch, name: "production/*", project: project, developers_can_push: true) - expect(project.protected_branch?('staging/some-branch')).to eq(false) + expect(project.developers_can_push_to_protected_branch?('staging/some-branch')).to be false + end end end