kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge branch 'dm-git-access-any-ce' into 'master'

Browse source 
[CE] Don't run checks for changed refs when specific changes are unknown

See merge request gitlab-org/gitlab-ce!23990
This commit is contained in:
Sean McGivern 2019-01-04 08:42:20 +00:00
commit 626f3d0367
14 changed files with 99 additions and 77 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/controllers/projects/git_http_controller.rb
View file

@ -80,9 +80,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
end
def access_check
# Use the magic string '_any' to indicate we do not know what the
# changes are. This is also what gitlab-shell does.
access.check(git_command, '_any')
access.check(git_command, Gitlab::GitAccess::ANY)
@project ||= access.project
end

45
app/models/project.rb
View file

@ -1928,23 +1928,15 @@ class Project < ActiveRecord::Base
.where('project_authorizations.project_id = merge_requests.target_project_id')
.limit(1)
.select(1)
source_of_merge_requests.opened
.where(allow_collaboration: true)
.where('EXISTS (?)', developer_access_exists)
merge_requests_allowing_collaboration.where('EXISTS (?)', developer_access_exists)
end
def any_branch_allows_collaboration?(user)
fetch_branch_allows_collaboration(user)
end
def branch_allows_collaboration?(user, branch_name)
return false unless user
cache_key = "user:#{user.id}:#{branch_name}:branch_allows_push"
memoized_results = strong_memoize(:branch_allows_collaboration) do
Hash.new do |result, cache_key|
result[cache_key] = fetch_branch_allows_collaboration?(user, branch_name)
end
end
memoized_results[cache_key]
fetch_branch_allows_collaboration(user, branch_name)
end
def licensed_features
@ -2018,6 +2010,12 @@ class Project < ActiveRecord::Base
private
def merge_requests_allowing_collaboration(source_branch = nil)
relation = source_of_merge_requests.opened.where(allow_collaboration: true)
relation = relation.where(source_branch: source_branch) if source_branch
relation
end
def create_new_pool_repository
pool = begin
create_pool_repository!(shard: Shard.by_name(repository_storage), source_project: self)
@ -2142,26 +2140,19 @@ class Project < ActiveRecord::Base
raise ex
end
def fetch_branch_allows_collaboration?(user, branch_name)
check_access = -> do
next false if empty_repo?
def fetch_branch_allows_collaboration(user, branch_name = nil)
return false unless user
merge_requests = source_of_merge_requests.opened
.where(allow_collaboration: true)
Gitlab::SafeRequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do
next false if empty_repo?
# Issue for N+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/49322
Gitlab::GitalyClient.allow_n_plus_1_calls do
if branch_name
merge_requests.find_by(source_branch: branch_name)&.can_be_merged_by?(user)
else
merge_requests.any? { |merge_request| merge_request.can_be_merged_by?(user) }
merge_requests_allowing_collaboration(branch_name).any? do |merge_request|
merge_request.can_be_merged_by?(user)
end
end
end
Gitlab::SafeRequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do
check_access.call
end
end
def services_templates

6
lib/gitlab/checks/base_checker.rb
View file

@ -18,12 +18,16 @@ module Gitlab
private
def creation?
Gitlab::Git.blank_ref?(oldrev)
end
def deletion?
Gitlab::Git.blank_ref?(newrev)
end
def update?
!Gitlab::Git.blank_ref?(oldrev) && !deletion?
!creation? && !deletion?
end
def updated_from_web?

5
lib/gitlab/checks/change_access.rb
View file

@ -10,7 +10,7 @@ module Gitlab
attr_reader(*ATTRIBUTES)
def initialize(
change, user_access:, project:, skip_authorization: false,
change, user_access:, project:,
skip_lfs_integrity_check: false, protocol:, logger:
)
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@ -18,7 +18,6 @@ module Gitlab
@tag_name = Gitlab::Git.tag_name(@ref)
@user_access = user_access
@project = project
@skip_authorization = skip_authorization
@skip_lfs_integrity_check = skip_lfs_integrity_check
@protocol = protocol
@ -27,8 +26,6 @@ module Gitlab
end
def exec
return true if skip_authorization
ref_level_checks
# Check of commits should happen as the last step
# given they're expensive in terms of performance

2
lib/gitlab/checks/diff_check.rb
View file

@ -11,7 +11,7 @@ module Gitlab
}.freeze
def validate!
return if deletion? || newrev.nil?
return if deletion?
return unless should_run_diff_validations?
return if commits.empty?

4
lib/gitlab/checks/push_check.rb
View file

@ -6,7 +6,7 @@ module Gitlab
def validate!
logger.log_timed("Checking if you are allowed to push...") do
unless can_push?
raise GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.'
raise GitAccess::UnauthorizedError, GitAccess::ERROR_MESSAGES[:push_code]
end
end
end
@ -15,7 +15,7 @@ module Gitlab
def can_push?
user_access.can_do_action?(:push_code) ||
user_access.can_push_to_branch?(branch_name)
project.branch_allows_collaboration?(user_access.user, branch_name)
end
end
end

46
lib/gitlab/git_access.rb
View file

@ -12,6 +12,10 @@ module Gitlab
TimeoutError = Class.new(StandardError)
ProjectMovedError = Class.new(NotFoundError)
# Use the magic string '_any' to indicate we do not know what the
# changes are. This is also what gitlab-shell does.
ANY = '_any'
ERROR_MESSAGES = {
upload: 'You are not allowed to upload code for this project.',
download: 'You are not allowed to download code from this project.',
@ -24,7 +28,8 @@ module Gitlab
upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.',
receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.',
read_only: 'The repository is temporarily read-only. Please try again later.',
cannot_push_to_read_only: "You can't push code to a read-only GitLab instance."
cannot_push_to_read_only: "You can't push code to a read-only GitLab instance.",
push_code: 'You are not allowed to push code to this project.'
}.freeze
INTERNAL_TIMEOUT = 50.seconds.freeze
@ -199,7 +204,7 @@ module Gitlab
def ensure_project_on_push!(cmd, changes)
return if project || deploy_key?
return unless receive_pack?(cmd) && changes == '_any' && authentication_abilities.include?(:push_code)
return unless receive_pack?(cmd) && changes == ANY && authentication_abilities.include?(:push_code)
namespace = Namespace.find_by_full_path(namespace_path)
@ -256,24 +261,34 @@ module Gitlab
raise UnauthorizedError, ERROR_MESSAGES[:upload]
end
return if changes.blank? # Allow access this is needed for EE.
check_change_access!
end
def check_change_access!
# If there are worktrees with a HEAD pointing to a non-existent object,
# calls to `git rev-list --all` will fail in git 2.15+. This should also
# clear stale lock files.
project.repository.clean_stale_repository_files
# Deploy keys with write access can push anything
return if deploy_key?
# Iterate over all changes to find if user allowed all of them to be applied
changes_list.each.with_index do |change, index|
first_change = index == 0
if changes == ANY
can_push = user_access.can_do_action?(:push_code) ||
project.any_branch_allows_collaboration?(user_access.user)
# If user does not have access to make at least one change, cancel all
# push by allowing the exception to bubble up
check_single_change_access(change, skip_lfs_integrity_check: !first_change)
unless can_push
raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code]
end
else
# If there are worktrees with a HEAD pointing to a non-existent object,
# calls to `git rev-list --all` will fail in git 2.15+. This should also
# clear stale lock files.
project.repository.clean_stale_repository_files
# Iterate over all changes to find if user allowed all of them to be applied
changes_list.each.with_index do |change, index|
first_change = index == 0
# If user does not have access to make at least one change, cancel all
# push by allowing the exception to bubble up
check_single_change_access(change, skip_lfs_integrity_check: !first_change)
end
end
end
@ -282,7 +297,6 @@ module Gitlab
change,
user_access: user_access,
project: project,
skip_authorization: deploy_key?,
skip_lfs_integrity_check: skip_lfs_integrity_check,
protocol: protocol,
logger: logger
@ -348,7 +362,7 @@ module Gitlab
protected
def changes_list
@changes_list ||= Gitlab::ChangesList.new(changes)
@changes_list ||= Gitlab::ChangesList.new(changes == ANY ? [] : changes)
end
def user

2
lib/gitlab/git_access_wiki.rb
View file

@ -15,7 +15,7 @@ module Gitlab
authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_wiki_code)
end
def check_single_change_access(change, _options = {})
def check_change_access!
unless user_access.can_do_action?(:create_wiki)
raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki]
end

2
spec/lib/gitlab/checks/push_check_spec.rb
View file

@ -13,7 +13,7 @@ describe Gitlab::Checks::PushCheck do
context 'when the user is not allowed to push to the repo' do
it 'raises an error' do
expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false)
expect(user_access).to receive(:can_push_to_branch?).with('master').and_return(false)
expect(project).to receive(:branch_allows_collaboration?).with(user_access.user, 'master').and_return(false)
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
end

18
spec/lib/gitlab/git_access_spec.rb
View file

@ -14,7 +14,7 @@ describe Gitlab::GitAccess do
let(:authentication_abilities) { %i[read_project download_code push_code] }
let(:redirected_path) { nil }
let(:auth_result_type) { nil }
let(:changes) { '_any' }
let(:changes) { Gitlab::GitAccess::ANY }
let(:push_access_check) { access.check('git-receive-pack', changes) }
let(:pull_access_check) { access.check('git-upload-pack', changes) }
@ -437,7 +437,7 @@ describe Gitlab::GitAccess do
let(:project) { nil }
context 'when changes is _any' do
let(:changes) { '_any' }
let(:changes) { Gitlab::GitAccess::ANY }
context 'when authentication abilities include push code' do
let(:authentication_abilities) { [:push_code] }
@ -483,7 +483,7 @@ describe Gitlab::GitAccess do
end
context 'when project exists' do
let(:changes) { '_any' }
let(:changes) { Gitlab::GitAccess::ANY }
let!(:project) { create(:project) }
it 'does not create a new project' do
@ -497,7 +497,7 @@ describe Gitlab::GitAccess do
let(:project_path) { "nonexistent" }
let(:project) { nil }
let(:namespace_path) { user.namespace.path }
let(:changes) { '_any' }
let(:changes) { Gitlab::GitAccess::ANY }
it 'does not create a new project' do
expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count }
@ -507,7 +507,7 @@ describe Gitlab::GitAccess do
context 'when pull' do
let(:cmd) { 'git-upload-pack' }
let(:changes) { '_any' }
let(:changes) { Gitlab::GitAccess::ANY }
context 'when project does not exist' do
let(:project_path) { "new-project" }
@ -736,7 +736,8 @@ describe Gitlab::GitAccess do
end
let(:changes) do
{ push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
{ any: Gitlab::GitAccess::ANY,
push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
push_master: '6f6d7e7ed 570e7b2ab refs/heads/master',
push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature',
push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
@ -798,6 +799,7 @@ describe Gitlab::GitAccess do
permissions_matrix = {
admin: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: true,
@ -809,6 +811,7 @@ describe Gitlab::GitAccess do
},
maintainer: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: true,
@ -820,6 +823,7 @@ describe Gitlab::GitAccess do
},
developer: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: false,
@ -831,6 +835,7 @@ describe Gitlab::GitAccess do
},
reporter: {
any: false,
push_new_branch: false,
push_master: false,
push_protected_branch: false,
@ -842,6 +847,7 @@ describe Gitlab::GitAccess do
},
guest: {
any: false,
push_new_branch: false,
push_master: false,
push_protected_branch: false,

2
spec/lib/gitlab/git_access_wiki_spec.rb
View file

@ -38,7 +38,7 @@ describe Gitlab::GitAccessWiki do
end
describe '#access_check_download!' do
subject { access.check('git-upload-pack', '_any') }
subject { access.check('git-upload-pack', Gitlab::GitAccess::ANY) }
before do
project.add_developer(user)

36
spec/models/project_spec.rb
View file

@ -3831,6 +3831,16 @@ describe Project do
let(:user) { create(:user) }
let(:target_project) { create(:project, :repository) }
let(:project) { fork_project(target_project, nil, repository: true) }
let!(:local_merge_request) do
create(
:merge_request,
target_project: project,
target_branch: 'target-branch',
source_project: project,
source_branch: 'awesome-feature-1',
allow_collaboration: true
)
end
let!(:merge_request) do
create(
:merge_request,
@ -3875,14 +3885,23 @@ describe Project do
end
end
describe '#branch_allows_collaboration_push?' do
it 'allows access if the user can merge the merge request' do
expect(project.branch_allows_collaboration?(user, 'awesome-feature-1'))
describe '#any_branch_allows_collaboration?' do
it 'allows access when there are merge requests open allowing collaboration' do
expect(project.any_branch_allows_collaboration?(user))
.to be_truthy
end
it 'allows access when there are merge requests open but no branch name is given' do
expect(project.branch_allows_collaboration?(user, nil))
it 'does not allow access when there are no merge requests open allowing collaboration' do
merge_request.close!
expect(project.any_branch_allows_collaboration?(user))
.to be_falsey
end
end
describe '#branch_allows_collaboration?' do
it 'allows access if the user can merge the merge request' do
expect(project.branch_allows_collaboration?(user, 'awesome-feature-1'))
.to be_truthy
end
@ -3913,13 +3932,6 @@ describe Project do
.to be_falsy
end
it 'caches the result' do
control = ActiveRecord::QueryRecorder.new { project.branch_allows_collaboration?(user, 'awesome-feature-1') }
expect { 3.times { project.branch_allows_collaboration?(user, 'awesome-feature-1') } }
.not_to exceed_query_limit(control)
end
context 'when the requeststore is active', :request_store do
it 'only queries per project across instances' do
control = ActiveRecord::QueryRecorder.new { project.branch_allows_collaboration?(user, 'awesome-feature-1') }

2
spec/serializers/pipeline_serializer_spec.rb
View file

@ -144,7 +144,7 @@ describe PipelineSerializer do
# pipeline. With the same ref this check is cached but if refs are
# different then there is an extra query per ref
# https://gitlab.com/gitlab-org/gitlab-ce/issues/46368
expect(recorded.count).to be_within(2).of(34)
expect(recorded.count).to be_within(2).of(38)
expect(recorded.cached_count).to eq(0)
end
end

2
spec/services/ci/retry_pipeline_service_spec.rb
View file

@ -285,7 +285,7 @@ describe Ci::RetryPipelineService, '#execute' do
end
it 'allows to retry failed pipeline' do
allow_any_instance_of(Project).to receive(:fetch_branch_allows_collaboration?).and_return(true)
allow_any_instance_of(Project).to receive(:branch_allows_collaboration?).and_return(true)
allow_any_instance_of(Project).to receive(:empty_repo?).and_return(false)
service.execute(pipeline)