diff options
| -rw-r--r-- | app/models/concerns/protected_ref.rb | 47 | ||||
| -rw-r--r-- | app/models/project.rb | 4 | ||||
| -rw-r--r-- | app/models/protected_branch.rb | 31 | ||||
| -rw-r--r-- | app/models/protected_tag.rb | 31 | ||||
| -rw-r--r-- | lib/api/entities.rb | 4 | ||||
| -rw-r--r-- | lib/gitlab/user_access.rb | 11 | ||||
| -rw-r--r-- | spec/models/protected_branch_spec.rb | 8 | ||||
| -rw-r--r-- | spec/services/protected_tags/create_service_spec.rb | 2 |
8 files changed, 60 insertions, 78 deletions
diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb new file mode 100644 index 00000000000..77d7b597534 --- /dev/null +++ b/app/models/concerns/protected_ref.rb @@ -0,0 +1,47 @@ +module ProtectedRef + extend ActiveSupport::Concern + + included do + belongs_to :project + validates :name, presence: true + validates :project, presence: true + + def self.matching_refs_accesible_to(ref, user, action: :push) + access_levels_for_ref(ref, action).any? do |access_level| + access_level.check_access(user) + end + end + + def self.access_levels_for_ref(ref, action: :push) + self.matching(ref).map(&:"@#{action}_access_levels").flatten + end + + private + + def self.matching(ref_name, protected_refs: nil) + ProtectedRefMatcher.matching(self, ref_name, protected_refs: protected_refs) + end + end + + def commit + project.commit(self.name) + end + + def matching(refs) + ref_matcher.matching(refs) + end + + def matches?(refs) + ref_matcher.matches?(refs) + end + + def wildcard? + ref_matcher.wildcard? + end + + private + + def ref_matcher + @ref_matcher ||= ProtectedRefMatcher.new(self) + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 7681da36335..970de324a5b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -898,14 +898,14 @@ class Project < ActiveRecord::Base return true if empty_repo? && default_branch_protected? @protected_branches ||= self.protected_branches.to_a - ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present? + ProtectedBranch.matching(branch_name, protected_refs: @protected_branches).present? 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_tags: @protected_tags).present? + ProtectedTag.matching(tag_name, protected_refs: @protected_tags).present? end def user_can_push_to_empty_repo?(user) diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 7681d5b5112..a0dbcf80c3d 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -1,9 +1,6 @@ class ProtectedBranch < ActiveRecord::Base include Gitlab::ShellAdapter - - belongs_to :project - validates :name, presence: true - validates :project, presence: true + include ProtectedRef has_many :merge_access_levels, dependent: :destroy has_many :push_access_levels, dependent: :destroy @@ -13,30 +10,4 @@ class ProtectedBranch < ActiveRecord::Base accepts_nested_attributes_for :push_access_levels accepts_nested_attributes_for :merge_access_levels - - def commit - project.commit(self.name) - end - - def self.matching(branch_name, protected_branches: nil) - ProtectedRefMatcher.matching(ProtectedBranch, branch_name, protected_refs: protected_branches) - end - - def matching(branches) - ref_matcher.matching(branches) - end - - def matches?(branch_name) - ref_matcher.matches?(branch_name) - end - - def wildcard? - ref_matcher.wildcard? - end - - private - - def ref_matcher - @ref_matcher ||= ProtectedRefMatcher.new(self) - end end diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb index d307549aa49..301fe2092e9 100644 --- a/app/models/protected_tag.rb +++ b/app/models/protected_tag.rb @@ -1,39 +1,10 @@ class ProtectedTag < ActiveRecord::Base include Gitlab::ShellAdapter - - belongs_to :project - validates :name, presence: true - validates :project, presence: true + include ProtectedRef has_many :push_access_levels, dependent: :destroy 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 commit - project.commit(self.name) - end - - def self.matching(tag_name, protected_tags: nil) - ProtectedRefMatcher.matching(ProtectedTag, tag_name, protected_refs: protected_tags) - end - - def matching(branches) - ref_matcher.matching(branches) - end - - def matches?(tag_name) - ref_matcher.matches?(tag_name) - end - - def wildcard? - ref_matcher.wildcard? - end - - private - - def ref_matcher - @ref_matcher ||= ProtectedRefMatcher.new(self) - end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5954aea8041..e000e3e33d0 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -189,13 +189,13 @@ module API expose :developers_can_push do |repo_branch, options| project = options[:project] - access_levels = project.protected_branches.matching(repo_branch.name).map(&:push_access_levels).flatten + access_levels = project.protected_branches.access_levels_for_ref(repo_branch.name, :push) access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER } end expose :developers_can_merge do |repo_branch, options| project = options[:project] - access_levels = project.protected_branches.matching(repo_branch.name).map(&:merge_access_levels).flatten + access_levels = project.protected_branches.access_levels_for_ref(repo_branch.name, :merge) access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER } end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 921159d91ef..5a5a4ebd08b 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -35,10 +35,7 @@ module Gitlab return false unless can_access_git? if project.protected_tag?(ref) - access_levels = project.protected_tags.matching(ref).map(&:push_access_levels).flatten - has_access = access_levels.any? { |access_level| access_level.check_access(user) } - - has_access + project.protected_tags.matching_refs_accesible_to(ref, user) else user.can?(:push_code, project) end @@ -50,8 +47,7 @@ module Gitlab 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_levels).flatten - has_access = access_levels.any? { |access_level| access_level.check_access(user) } + has_access = project.protected_branches.matching_refs_accesible_to(ref, user, action: :push) has_access || !project.repository.branch_exists?(ref) && can_merge_to_branch?(ref) else @@ -63,8 +59,7 @@ module Gitlab return false unless can_access_git? if project.protected_branch?(ref) - access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten - access_levels.any? { |access_level| access_level.check_access(user) } + project.protected_branches.matching_refs_accesible_to(ref, user, action: :merge) else user.can?(:push_code, project) end diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 8bf0d24a128..1c02f8bfc3f 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -113,8 +113,8 @@ describe ProtectedBranch, models: true do staging = build(:protected_branch, name: "staging") expect(ProtectedBranch.matching("production")).to be_empty - expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).to include(production) - expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).not_to include(staging) + expect(ProtectedBranch.matching("production", protected_refs: [production, staging])).to include(production) + expect(ProtectedBranch.matching("production", protected_refs: [production, staging])).not_to include(staging) end end @@ -132,8 +132,8 @@ describe ProtectedBranch, models: true do staging = build(:protected_branch, name: "staging/*") expect(ProtectedBranch.matching("production/some-branch")).to be_empty - expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).to include(production) - expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).not_to include(staging) + expect(ProtectedBranch.matching("production/some-branch", protected_refs: [production, staging])).to include(production) + expect(ProtectedBranch.matching("production/some-branch", protected_refs: [production, staging])).not_to include(staging) end end end diff --git a/spec/services/protected_tags/create_service_spec.rb b/spec/services/protected_tags/create_service_spec.rb index e1fd73dbd07..70ea96a954f 100644 --- a/spec/services/protected_tags/create_service_spec.rb +++ b/spec/services/protected_tags/create_service_spec.rb @@ -6,7 +6,6 @@ describe ProtectedTags::CreateService, services: true do let(:params) do { name: 'master', - merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }], push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }] } end @@ -17,7 +16,6 @@ describe ProtectedTags::CreateService, services: true do it 'creates a new protected tag' do expect { service.execute }.to change(ProtectedTag, :count).by(1) expect(project.protected_tags.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) - expect(project.protected_tags.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) end end end |