From bf3cc824e4ce6cf49a82210eaaf1cca06f7fd281 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 18:59:58 +0100 Subject: Moved Project#protected_branch? to ProtectedBranch, similar for tags --- app/helpers/branches_helper.rb | 2 +- app/helpers/tags_helper.rb | 2 +- app/models/merge_request.rb | 2 +- app/models/project.rb | 22 +++++------ app/models/protected_branch.rb | 8 ++++ app/models/protected_tag.rb | 5 +++ app/services/delete_branch_service.rb | 2 +- app/services/git_push_service.rb | 2 +- app/views/projects/branches/_branch.html.haml | 2 +- lib/api/entities.rb | 2 +- lib/gitlab/checks/change_access.rb | 4 +- lib/gitlab/user_access.rb | 6 +-- spec/lib/gitlab/checks/change_access_spec.rb | 2 +- spec/models/merge_request_spec.rb | 2 +- spec/models/project_spec.rb | 56 --------------------------- spec/models/protected_branch_spec.rb | 56 +++++++++++++++++++++++++++ 16 files changed, 94 insertions(+), 81 deletions(-) diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index 3fc85dc6b2b..a852b90c57e 100644 --- a/app/helpers/branches_helper.rb +++ b/app/helpers/branches_helper.rb @@ -1,6 +1,6 @@ module BranchesHelper def can_remove_branch?(project, branch_name) - if project.protected_branch? branch_name + if ProtectedBranch.protected?(project, branch_name) false elsif branch_name == project.repository.root_ref false diff --git a/app/helpers/tags_helper.rb b/app/helpers/tags_helper.rb index 6672e3da348..31aaf9e5607 100644 --- a/app/helpers/tags_helper.rb +++ b/app/helpers/tags_helper.rb @@ -23,6 +23,6 @@ module TagsHelper end def protected_tag?(project, tag) - project.protected_tag?(tag.name) + ProtectedTag.protected?(project, tag.name) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index cef8ad76b07..38eefad96b6 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -442,7 +442,7 @@ class MergeRequest < ActiveRecord::Base end def can_remove_source_branch?(current_user) - !source_project.protected_branch?(source_branch) && + !ProtectedBranch.protected?(source_project, source_branch) && !source_project.root_ref?(source_branch) && Ability.allowed?(current_user, :push_code, source_project) && diff_head_commit == source_branch_head diff --git a/app/models/project.rb b/app/models/project.rb index 4175bfab0a9..5cc224b4440 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -883,20 +883,19 @@ class Project < ActiveRecord::Base "#{url}.git" end - # Check if current branch name is marked as protected in the system - #TODO: Move elsewhere - 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_refs: @protected_branches).present? + def empty_and_default_branch_protected? + empty_repo? && default_branch_protected? end - #TODO: Move elsewhere - def protected_tag?(tag_name) - #TODO: Check if memoization necessary, find way to have it work elsewhere - @protected_tags ||= self.protected_tags.to_a - ProtectedTag.matching(tag_name, protected_refs: @protected_tags).present? + #TODO: Check with if this is still needed, maybe because of `.select {` in ProtectedRefsMatcher + #Either with tests or by asking Tim + def protected_tags_array + @protected_tags_array ||= self.protected_tags.to_a + end + + def protected_branches_array + @protected_branches_array ||= self.protected_branches.to_a end def user_can_push_to_empty_repo?(user) @@ -1367,6 +1366,7 @@ class Project < ActiveRecord::Base "projects/#{id}/pushes_since_gc" end + #TODO: Move this and methods which depend upon it 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 diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index a0dbcf80c3d..eca8d5e0f7d 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -10,4 +10,12 @@ class ProtectedBranch < ActiveRecord::Base accepts_nested_attributes_for :push_access_levels accepts_nested_attributes_for :merge_access_levels + + # Check if branch name is marked as protected in the system + def self.protected?(project, ref_name) + return true if project.empty_and_default_branch_protected? + + protected_refs = project.protected_branches_array + self.matching(ref_name, protected_refs: protected_refs).present? + end end diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb index 301fe2092e9..bca5522759d 100644 --- a/app/models/protected_tag.rb +++ b/app/models/protected_tag.rb @@ -7,4 +7,9 @@ class ProtectedTag < ActiveRecord::Base validates :push_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." } accepts_nested_attributes_for :push_access_levels + + def self.protected?(project, ref_name) + protected_refs = project.protected_tags_array + self.matching(ref_name, protected_refs: protected_refs).present? + end end diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb index 11a045f4c31..38a113caec7 100644 --- a/app/services/delete_branch_service.rb +++ b/app/services/delete_branch_service.rb @@ -11,7 +11,7 @@ class DeleteBranchService < BaseService return error('Cannot remove HEAD branch', 405) end - if project.protected_branch?(branch_name) + if ProtectedBranch.protected?(project, branch_name) return error('Protected branch cant be removed', 405) end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index bc7431c89a8..45411c779cc 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -127,7 +127,7 @@ class GitPushService < BaseService project.change_head(branch_name) # Set protection on the default branch if configured - if current_application_settings.default_branch_protection != PROTECTION_NONE && !@project.protected_branch?(@project.default_branch) + if current_application_settings.default_branch_protection != PROTECTION_NONE && !ProtectedBranch.protected?(@project, @project.default_branch) params = { name: @project.default_branch, diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index 9eb610ba9c0..d84fa9e55c0 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/app/views/projects/branches/_branch.html.haml @@ -15,7 +15,7 @@ %span.label.label-info.has-tooltip{ title: "Merged into #{@repository.root_ref}" } merged - - if @project.protected_branch? branch.name + - if ProtectedBranch.protected?(@project, branch.name) %span.label.label-success protected .controls.hidden-xs< diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e000e3e33d0..0fe7eb864e4 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -184,7 +184,7 @@ module API end expose :protected do |repo_branch, options| - options[:project].protected_branch?(repo_branch.name) + ProtectedBranch.protected?(options[:project], repo_branch.name) end expose :developers_can_push do |repo_branch, options| diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 540d95f2d1f..6f574a41727 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -33,7 +33,7 @@ module Gitlab def protected_branch_checks return if skip_authorization return unless @branch_name - return unless project.protected_branch?(@branch_name) + return unless ProtectedBranch.protected?(project, @branch_name) if forced_push? return "You are not allowed to force push code to a protected branch on this project." @@ -85,7 +85,7 @@ module Gitlab end def tag_protected? - project.protected_tag?(@tag_name) + ProtectedTag.protected?(project, @tag_name) end def push_checks diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 5a5a4ebd08b..5541a45e948 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -34,7 +34,7 @@ module Gitlab def can_push_tag?(ref) return false unless can_access_git? - if project.protected_tag?(ref) + if ProtectedTag.protected?(project, ref) project.protected_tags.matching_refs_accesible_to(ref, user) else user.can?(:push_code, project) @@ -44,7 +44,7 @@ module Gitlab def can_push_to_branch?(ref) return false unless can_access_git? - if project.protected_branch?(ref) + if ProtectedBranch.protected?(project, ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) has_access = project.protected_branches.matching_refs_accesible_to(ref, user, action: :push) @@ -58,7 +58,7 @@ module Gitlab def can_merge_to_branch?(ref) return false unless can_access_git? - if project.protected_branch?(ref) + if ProtectedBranch.protected?(project, ref) project.protected_branches.matching_refs_accesible_to(ref, user, action: :merge) else user.can?(:push_code, project) diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index cb2989c32df..8525422908b 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -99,7 +99,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do context 'protected branches check' do before do - allow(project).to receive(:protected_branch?).with('master').and_return(true) + allow(ProtectedBranch).to receive(:protected?).with(project, 'master').and_return(true) end it 'returns an error if the user is not allowed to do forced pushes to protected branches' do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 24e7c1b17d9..2f6614c133e 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -441,7 +441,7 @@ describe MergeRequest, models: true do end it "can't be removed when its a protected branch" do - allow(subject.source_project).to receive(:protected_branch?).and_return(true) + allow(ProtectedBranch).to receive(:protected?).and_return(true) expect(subject.can_remove_source_branch?(user)).to be_falsey end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index cc06949974e..e6b23a1cc05 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1272,62 +1272,6 @@ describe Project, models: true do end end - describe '#protected_branch?' do - context 'existing project' do - let(:project) { create(:project, :repository) } - - it 'returns true when the branch matches a protected branch via direct match' do - create(:protected_branch, project: project, 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 - create(:protected_branch, project: project, 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 - create(:protected_branch, project: project, 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) } diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 1c02f8bfc3f..179a443c43d 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -137,4 +137,60 @@ describe ProtectedBranch, models: true do end end end + + describe '#protected?' do + context 'existing project' do + let(:project) { create(:project, :repository) } + + it 'returns true when the branch matches a protected branch via direct match' do + create(:protected_branch, project: project, name: "foo") + + expect(ProtectedBranch.protected?(project, 'foo')).to eq(true) + end + + it 'returns true when the branch matches a protected branch via wildcard match' do + create(:protected_branch, project: project, name: "production/*") + + expect(ProtectedBranch.protected?(project, 'production/some-branch')).to eq(true) + end + + it 'returns false when the branch does not match a protected branch via direct match' do + expect(ProtectedBranch.protected?(project, 'foo')).to eq(false) + end + + it 'returns false when the branch does not match a protected branch via wildcard match' do + create(:protected_branch, project: project, name: "production/*") + + expect(ProtectedBranch.protected?(project, '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(ProtectedBranch.protected?(project, '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(ProtectedBranch.protected?(project, '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(ProtectedBranch.protected?(project, '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(ProtectedBranch.protected?(project, 'master')).to be true + end + end + end end -- cgit v1.2.1