Skip to content

G
gitlab-ce
Commit 5b8bbf77 authored by Timothy Andrew's avatar Timothy Andrew
Browse files
Options

Refactor `Gitlab::GitAccess` 

1. Don't use case statements for dispatch anymore. This leads to a lot
   of duplication, and makes the logic harder to follow.

2. Remove duplicated logic.

    - For example, the `can_push_to_branch?` exists, but we also have a
      different way of checking the same condition within `change_access_check`.

    - This kind of duplication is removed, and the `can_push_to_branch?`
      method is used in both places.

3. Move checks returning true/false to `UserAccess`.

    - All public methods in `GitAccess` now return an instance of
      `GitAccessStatus`. Previously, some methods would return
      true/false as well, which was confusing.

    - It makes sense for these kinds of checks to be at the level of a
      user, so the `UserAccess` class was repurposed for this. The prior
      `UserAccess.allowed?` classmethod is converted into an instance
      method.

    - All external uses of these checks have been migrated to use the
      `UserAccess` class

4. Move the "change_access_check" into a separate class.

    - Create the `GitAccess::ChangeAccessCheck` class to run these
      checks, which are quite substantial.

    - `ChangeAccessCheck` returns an instance of `GitAccessStatus` as
      well.

5. Break out the boolean logic in `ChangeAccessCheck` into `if/else`
   chains - this seems more readable.

6. I can understand that this might look like overkill for !4892, but I
   think this is a good opportunity to clean it up.

    - http://martinfowler.com/bliki/OpportunisticRefactoring.html
parent 4c9e7972
Hide whitespace changes
Inline Side-by-side
Showing with 426 additions and 272 deletions
app/helpers/branches_helper.rb
View file @ 5b8bbf77
......@@ -12,7 +12,7 @@ module BranchesHelper
def can_push_branch?(project, branch_name)
return false unless project.repository.branch_exists?(branch_name)
::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(branch_name)
::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch_name)
end
def project_branches
......
app/models/merge_request.rb
View file @ 5b8bbf77
......@@ -619,7 +619,7 @@ class MergeRequest < ActiveRecord::Base
end
def can_be_merged_by?(user)
access = ::Gitlab::GitAccess.new(user, project, 'web')
access = ::Gitlab::UserAccess.new(user, project: project)
access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch)
end
......
app/services/commits/change_service.rb
View file @ 5b8bbf77
......@@ -23,7 +23,7 @@ module Commits
private
def check_push_permissions
allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch)
allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
unless allowed
raise ValidationError.new('You are not allowed to push into this branch')
......@@ -31,7 +31,7 @@ module Commits
true
end
def create_target_branch(new_branch)
# Temporary branch exists and contains the change commit
return success if repository.find_branch(new_branch)
......
app/services/files/base_service.rb
View file @ 5b8bbf77
......@@ -42,7 +42,7 @@ module Files
end
def validate
allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch)
allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
unless allowed
raise_error("You are not allowed to push into this branch")
......
lib/api/helpers.rb
View file @ 5b8bbf77
......@@ -17,7 +17,7 @@ module API
def current_user
@current_user ||= (find_user_by_private_token || doorkeeper_guard)
unless @current_user && Gitlab::UserAccess.allowed?(@current_user)
unless @current_user && Gitlab::UserAccess.new(@current_user).allowed?
return nil
end
......
lib/gitlab/git_access.rb
View file @ 5b8bbf77
# Check a user's access to perform a git action. All public methods in this
# class return an instance of `GitlabAccessStatus`
module Gitlab
class GitAccess
include PathLocksHelper
......@@ -6,68 +8,13 @@ module Gitlab
PUSH_COMMANDS = %w{ git-receive-pack }
GIT_ANNEX_COMMANDS = %w{ git-annex-shell }
attr_reader :actor, :project, :protocol
attr_reader :actor, :project, :protocol, :user_access
def initialize(actor, project, protocol)
@actor = actor
@project = project
@protocol = protocol
end
def user
return @user if defined?(@user)
@user =
case actor
when User
actor
when DeployKey
nil
when GeoNodeKey
nil
when Key
actor.user
end
end
def deploy_key
actor if actor.is_a?(DeployKey)
end
def geo_node_key
actor if actor.is_a?(GeoNodeKey)
end
def can_push_to_branch?(ref)
return false unless user
if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref)
user.can?(:push_code_to_protected_branches, project)
else
user.can?(:push_code, project)
end
end
def can_merge_to_branch?(ref)
return false unless user
if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref)
user.can?(:push_code_to_protected_branches, project)
else
user.can?(:push_code, project)
end
end
def can_read_project?
if user
user.can?(:read_project, project)
elsif deploy_key
deploy_key.projects.include?(project)
elsif geo_node_key
true
else
false
end
@user_access = UserAccess.new(user, project: project)
end
def check(cmd, changes = nil)
......@@ -77,11 +24,11 @@ module Gitlab
return build_status_object(false, "No user or key was provided.")
end
if user && !user_allowed?
if user && !user_access.allowed?
return build_status_object(false, "Your account has been blocked.")
end
unless project && can_read_project?
unless project && (user_access.can_read_project? || deploy_key_can_read_project? || geo_node_key)
return build_status_object(false, 'The project you were looking for could not be found.')
end
......@@ -128,7 +75,7 @@ module Gitlab
end
def user_download_access_check
unless user.can?(:download_code, project)
unless user_access.can_do_action?(:download_code)
return build_status_object(false, "You are not allowed to download code from this project.")
end
......@@ -163,121 +110,18 @@ module Gitlab
build_status_object(true)
end
def can_user_do_action?(action)
@permission_cache ||= {}
@permission_cache[action] ||= user.can?(action, project)
end
def change_access_check(change)
oldrev, newrev, ref = change.split(' ')
action =
if project.protected_branch?(branch_name(ref))
protected_branch_action(oldrev, newrev, branch_name(ref))
elsif (tag_ref = tag_name(ref)) && protected_tag?(tag_ref)
# Prevent any changes to existing git tag unless user has permissions
:admin_project
else
:push_code
end
unless can_user_do_action?(action)
status =
case action
when :force_push_code_to_protected_branches
build_status_object(false, "You are not allowed to force push code to a protected branch on this project.")
when :remove_protected_branches
build_status_object(false, "You are not allowed to deleted protected branches from this project.")
when :push_code_to_protected_branches
build_status_object(false, "You are not allowed to push code to protected branches on this project.")
when :admin_project
build_status_object(false, "You are not allowed to change existing tags on this project.")
else # :push_code
build_status_object(false, "You are not allowed to push code to this project.")
end
return status
end
# Return build_status_object(true) if all push rule checks passed successfully
# or build_status_object(false) if any hook fails
result = push_rule_check(user, project, ref, oldrev, newrev)
if result.status && license_allows_file_locks?
result = path_locks_check(user, project, ref, oldrev, newrev)
end
result
ChangeAccessCheck.new(change, user_access: user_access, project: project).exec
end
def forced_push?(oldrev, newrev)
Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev)
end
def protocol_allowed?
Gitlab::ProtocolAccess.allowed?(protocol)
end
def path_locks_check(user, project, ref, oldrev, newrev)
unless project.path_locks.any? && newrev && oldrev
return build_status_object(true)
end
# locks protect default branch only
if project.default_branch != branch_name(ref)
return build_status_object(true)
end
commits(newrev, oldrev, project).each do |commit|
next if commit_from_annex_sync?(commit.safe_message)
commit.diffs.each do |diff|
path = diff.new_path || diff.old_path
lock_info = project.find_path_lock(path)
if lock_info && lock_info.user != user
return build_status_object(false, "The path '#{lock_info.path}' is locked by #{lock_info.user.name}")
end
end
end
build_status_object(true)
end
def push_rule_check(user, project, ref, oldrev, newrev)
unless project.push_rule && newrev && oldrev
return build_status_object(true)
end
push_rule = project.push_rule
# Prevent tag removal
if Gitlab::Git.tag_ref?(ref)
if push_rule.deny_delete_tag && protected_tag?(tag_name(ref)) && Gitlab::Git.blank_ref?(newrev)
return build_status_object(false, "You can not delete tag")
end
else
# if newrev is blank, the branch was deleted
if Gitlab::Git.blank_ref?(newrev) || !push_rule.commit_validation?
return build_status_object(true)
end
# if oldrev is blank, the branch was just created
oldrev = project.default_branch if Gitlab::Git.blank_ref?(oldrev)
commits(newrev, oldrev, project).each do |commit|
next if commit_from_annex_sync?(commit.safe_message) || old_commit?(commit)
if status_object = check_commit(commit, push_rule)
return status_object
end
end
end
build_status_object(true)
end
def matching_merge_request?(newrev, branch_name)
Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
end
......@@ -371,28 +215,47 @@ module Gitlab
project.repository.tag_exists?(tag_name)
end
def user_allowed?
Gitlab::UserAccess.allowed?(user)
private
def deploy_key
actor if actor.is_a?(DeployKey)
end
def branch_name(ref)
ref = ref.to_s
if Gitlab::Git.branch_ref?(ref)
Gitlab::Git.ref_name(ref)
else
nil
end
def deploy_key
actor if actor.is_a?(DeployKey)
end
def tag_name(ref)
ref = ref.to_s
if Gitlab::Git.tag_ref?(ref)
Gitlab::Git.ref_name(ref)
def geo_node_key
actor if actor.is_a?(GeoNodeKey)
end
def deploy_key_can_read_project?
if deploy_key
deploy_key.projects.include?(project)
else
nil
false
end
end
protected
def user
return @user if defined?(@user)
@user =
case actor
when User
actor
when DeployKey
nil
when GeoNodeKey
nil
when Key
actor.user
end
end
def build_status_object(status, message = '')
GitAccessStatus.new(status, message)
end
......
lib/gitlab/git_access/change_access_check.rb 0 → 100644
View file @ 5b8bbf77
module Gitlab
class GitAccess
class ChangeAccessCheck
include PathLocksHelper
attr_reader :user_access, :project
def initialize(change, user_access:, project:)
@oldrev, @newrev, @ref = change.split(' ')
@branch_name = branch_name(@ref)
@user_access = user_access
@project = project
end
def exec
error = protected_branch_checks || tag_checks || push_checks || push_rules_checks
if error
GitAccessStatus.new(false, error)
else
GitAccessStatus.new(true)
end
end
protected
def protected_branch_checks
return unless project.protected_branch?(@branch_name)
if forced_push? && user_access.cannot_do_action?(:force_push_code_to_protected_branches)
return "You are not allowed to force push code to a protected branch on this project."
elsif Gitlab::Git.blank_ref?(@newrev) && user_access.cannot_do_action?(:remove_protected_branches)
return "You are not allowed to delete protected branches from this project."
end
if matching_merge_request?
if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
return
else
"You are not allowed to merge code into protected branches on this project."
end
else
if user_access.can_push_to_branch?(@branch_name)
return
else
"You are not allowed to push code to protected branches on this project."
end
end
end
def tag_checks
tag_ref = tag_name(@ref)
if tag_ref && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project)
"You are not allowed to change existing tags on this project."
end
end
def push_checks
if user_access.cannot_do_action?(:push_code)
"You are not allowed to push code to this project."
end
end
def push_rules_checks
# Returns nil if all push rule checks passed successfully
# or the error if any hook fails
error = push_rule_check
if !error && license_allows_file_locks?
error = path_locks_check
end
error
end
private
def protected_tag?(tag_name)
project.repository.tag_exists?(tag_name)
end
def forced_push?
Gitlab::ForcePushCheck.force_push?(@project, @oldrev, @newrev)
end
def matching_merge_request?
Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match?
end
def branch_name(ref)
ref = @ref.to_s
if Gitlab::Git.branch_ref?(ref)
Gitlab::Git.ref_name(ref)
else
nil
end
end
def tag_name(ref)
ref = @ref.to_s
if Gitlab::Git.tag_ref?(ref)
Gitlab::Git.ref_name(ref)
else
nil
end
end
def push_rule_check
return unless project.push_rule && @newrev && @oldrev
push_rule = project.push_rule
# Prevent tag removal
if tag_name(@ref)
if push_rule.deny_delete_tag && protected_tag?(tag_name(@ref)) && Gitlab::Git.blank_ref?(@newrev)
"You can not delete tag"
end
else
# if newrev is blank, the branch was deleted
return if Gitlab::Git.blank_ref?(@newrev) || !push_rule.commit_validation?
# if oldrev is blank, the branch was just created
oldrev = Gitlab::Git.blank_ref?(@oldrev) ? project.default_branch : @oldrev
commits(oldrev).each do |commit|
next if commit_from_annex_sync?(commit.safe_message) || old_commit?(commit)
if error = check_commit(commit, push_rule)
return error
end
end
end
nil
end
def path_locks_check
return unless project.path_locks.any? && @newrev && @oldrev
# locks protect default branch only
return if project.default_branch != branch_name(@ref)
commits(@oldrev).each do |commit|
next if commit_from_annex_sync?(commit.safe_message)
commit.diffs.each do |diff|
path = diff.new_path || diff.old_path
lock_info = project.find_path_lock(path)
if lock_info && lock_info.user != user_access.user
return "The path '#{lock_info.path}' is locked by #{lock_info.user.name}"
end
end
end
nil
end
# If commit does not pass push rule validation the whole push should be rejected.
# This method should return nil if no error found or status object if there are some errors.
# In case of errors - all other checks will be canceled and push will be rejected.
def check_commit(commit, push_rule)
unless push_rule.commit_message_allowed?(commit.safe_message)
return "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'"
end
unless push_rule.author_email_allowed?(commit.committer_email)
return "Committer's email '#{commit.committer_email}' does not follow the pattern '#{push_rule.author_email_regex}'"
end
unless push_rule.author_email_allowed?(commit.author_email)
return "Author's email '#{commit.author_email}' does not follow the pattern '#{push_rule.author_email_regex}'"
end
# Check whether author is a GitLab member
if push_rule.member_check
unless User.existing_member?(commit.author_email.downcase)
return "Author '#{commit.author_email}' is not a member of team"
end
if commit.author_email.casecmp(commit.committer_email) == -1
unless User.existing_member?(commit.committer_email.downcase)
return "Committer '#{commit.committer_email}' is not a member of team"
end
end
end
if error = check_commit_diff(commit, push_rule)
return error
end
nil
end
def check_commit_diff(commit, push_rule)
if push_rule.file_name_regex.present?
commit.diffs.each do |diff|
if (diff.renamed_file || diff.new_file) && diff.new_path =~ Regexp.new(push_rule.file_name_regex)
return "File name #{diff.new_path.inspect} is prohibited by the pattern '#{push_rule.file_name_regex}'"
end
end
end
if push_rule.max_file_size > 0
commit.diffs.each do |diff|
next if diff.deleted_file
blob = project.repository.blob_at(commit.id, diff.new_path)
if blob && blob.size && blob.size > push_rule.max_file_size.megabytes
return "File #{diff.new_path.inspect} is larger than the allowed size of #{push_rule.max_file_size} MB"
end
end
end
nil
end
def commits(oldrev)
if oldrev
project.repository.commits_between(oldrev, @newrev)
else
project.repository.commits(@newrev)
end
end
def commit_from_annex_sync?(commit_message)
return false unless Gitlab.config.gitlab_shell.git_annex_enabled
# Commit message starting with <git-annex in > so avoid push rules on this
commit_message.start_with?('git-annex in')
end
def old_commit?(commit)
# We skip refs/tmp ref because we use it for Web UI commiting
commit.refs(project.repository).reject { |ref| ref.name.start_with?('refs/tmp') }.any?
end
end
end
end
lib/gitlab/git_access_wiki.rb
View file @ 5b8bbf77
......@@ -3,7 +3,7 @@ module Gitlab
def change_access_check(change)
if Gitlab::Geo.enabled? && Gitlab::Geo.secondary?
build_status_object(false, "You can't push code to a secondary GitLab Geo node.")
elsif user.can?(:create_wiki, project)
elsif user_access.can_do_action?(:create_wiki, project)
build_status_object(true)
else
build_status_object(false, "You are not allowed to write to this project's wiki.")
......
lib/gitlab/user_access.rb
View file @ 5b8bbf77
module Gitlab
module UserAccess
def self.allowed?(user)
return false if user.blocked?
class UserAccess
attr_reader :user, :project
def initialize(user, project: nil)
@user = user
@project = project
end
def can_do_action?(action)
@permission_cache ||= {}
@permission_cache[action] ||= user.can?(action, project)
end
def cannot_do_action?(action)
!can_do_action?(action)
end
def allowed?
return false if user.blank? || user.blocked?
if user.requires_ldap_check? && user.try_obtain_ldap_lease
return false unless Gitlab::LDAP::Access.allowed?(user)
......@@ -9,5 +25,31 @@ module Gitlab
true
end
def can_push_to_branch?(ref)
return false unless user
if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref)
user.can?(:push_code_to_protected_branches, project)
else
user.can?(:push_code, project)
end
end
def can_merge_to_branch?(ref)
return false unless user
if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref)
user.can?(:push_code_to_protected_branches, project)
else
user.can?(:push_code, project)
end
end
def can_read_project?
return false unless user
user.can?(:read_project, project)
end
end
end
spec/lib/gitlab/git_access_spec.rb
View file @ 5b8bbf77
......@@ -11,88 +11,6 @@ describe Gitlab::GitAccess, lib: true do
end
let(:git_annex_master_changes) { "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master" }
describe 'can_push_to_branch?' 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 protected branch' do
before do
@branch = create :protected_branch, project: project
end
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
describe 'push to protected branch if allowed for developers' do
before do
@branch = create :protected_branch, project: project, developers_can_push: true
end
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
describe 'merge to protected branch if allowed for developers' do
before do
@branch = create :protected_branch, project: project, developers_can_merge: true
end
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
describe '#check with single protocols allowed' do
def disable_protocol(protocol)
settings = ::ApplicationSetting.create_from_defaults
......
spec/lib/gitlab/user_access_spec.rb 0 → 100644
View file @ 5b8bbf77
require 'spec_helper'
describe Gitlab::UserAccess, lib: true do
let(:access) { Gitlab::UserAccess.new(user, project: project) }
let(:project) { create(:project) }
let(:user) { create(:user) }
describe 'can_push_to_branch?' 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 protected branch' do
before do
@branch = create :protected_branch, project: project
end
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
describe 'push to protected branch if allowed for developers' do
before do
@branch = create :protected_branch, project: project, developers_can_push: true
end
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
describe 'merge to protected branch if allowed for developers' do
before do
@branch = create :protected_branch, project: project, developers_can_merge: true
end
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/requests/api/api_helpers_spec.rb
View file @ 5b8bbf77
......@@ -49,7 +49,7 @@ describe API::Helpers, api: true do
it "should return nil for a user without access" do
env[API::Helpers::PRIVATE_TOKEN_HEADER] = user.private_token
allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
expect(current_user).to be_nil
end
......@@ -73,7 +73,7 @@ describe API::Helpers, api: true do
it "should return nil for a user without access" do
env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token
allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
expect(current_user).to be_nil
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7