diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 073c76933c1..b6b62da7b60 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -1,36 +1,27 @@ class Projects::GitHttpController < Projects::GitHttpClientController include WorkhorseRequest + before_action :access_check + + rescue_from Gitlab::GitAccess::UnauthorizedError, with: :render_403 + rescue_from Gitlab::GitAccess::NotFoundError, with: :render_404 + # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull) # GET /foo/bar.git/info/refs?service=git-receive-pack (git push) def info_refs - if upload_pack? && upload_pack_allowed? - log_user_activity + log_user_activity if upload_pack? - render_ok - elsif receive_pack? && receive_pack_allowed? - render_ok - else - render_denied - end + render_ok end # POST /foo/bar.git/git-upload-pack (git pull) def git_upload_pack - if upload_pack? && upload_pack_allowed? - render_ok - else - render_denied - end + render_ok end # POST /foo/bar.git/git-receive-pack" (git push) def git_receive_pack - if receive_pack? && receive_pack_allowed? - render_ok - else - render_denied - end + render_ok end private @@ -43,10 +34,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController git_command == 'git-upload-pack' end - def receive_pack? - git_command == 'git-receive-pack' - end - def git_command if action_name == 'info_refs' params[:service] @@ -60,16 +47,12 @@ class Projects::GitHttpController < Projects::GitHttpClientController render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name) end - def render_denied - if access_check.message == Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found] - render plain: access_check.message, status: :not_found - else - render plain: access_check.message, status: :forbidden - end + def render_403(exception) + render plain: exception.message, status: :forbidden end - def upload_pack_allowed? - access_check.allowed? + def render_404(exception) + render plain: exception.message, status: :not_found end def access @@ -84,11 +67,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController 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 ||= access.check(git_command, '_any') - end - - def receive_pack_allowed? - access_check.allowed? + access.check(git_command, '_any') end def access_klass diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 9ebd4841296..3d60d1da114 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -32,30 +32,32 @@ module API actor.update_last_used_at if actor.is_a?(Key) - access_checker = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess - access_status = access_checker + access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess + access_checker = access_checker_klass .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities) - .check(params[:action], params[:changes]) - response = { status: access_status.status, message: access_status.message } - - if access_status.status - log_user_activity(actor) - - # Project id to pass between components that don't share/don't have - # access to the same filesystem mounts - response[:gl_repository] = Gitlab::GlRepository.gl_repository(project, wiki?) - - # Return the repository full path so that gitlab-shell has it when - # handling ssh commands - response[:repository_path] = - if wiki? - project.wiki.repository.path_to_repo - else - project.repository.path_to_repo - end + begin + access_status = access_checker.check(params[:action], params[:changes]) + response = { status: access_status.status, message: access_status.message } + rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e + return { status: false, message: e.message } end + log_user_activity(actor) + + # Project id to pass between components that don't share/don't have + # access to the same filesystem mounts + response[:gl_repository] = Gitlab::GlRepository.gl_repository(project, wiki?) + + # Return the repository full path so that gitlab-shell has it when + # handling ssh commands + response[:repository_path] = + if wiki? + project.wiki.repository.path_to_repo + else + project.repository.path_to_repo + end + response end diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 4b6f8abd61d..e9782623be5 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -33,20 +33,18 @@ module Gitlab def exec return GitAccessStatus.new(true) if skip_authorization - error = push_checks || branch_checks || tag_checks + push_checks + branch_checks + tag_checks - if error - GitAccessStatus.new(false, error) - else - GitAccessStatus.new(true) - end + GitAccessStatus.new(true) end protected def push_checks if user_access.cannot_do_action?(:push_code) - ERROR_MESSAGES[:push_code] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code] end end @@ -54,7 +52,7 @@ module Gitlab return unless @branch_name if deletion? && @branch_name == project.default_branch - return ERROR_MESSAGES[:delete_default_branch] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:delete_default_branch] end protected_branch_checks @@ -64,7 +62,7 @@ module Gitlab return unless ProtectedBranch.protected?(project, @branch_name) if forced_push? - return ERROR_MESSAGES[:force_push_protected_branch] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:force_push_protected_branch] end if deletion? @@ -76,22 +74,22 @@ module Gitlab def protected_branch_deletion_checks unless user_access.can_delete_branch?(@branch_name) - return ERROR_MESSAGES[:non_master_delete_protected_branch] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:non_master_delete_protected_branch] end unless protocol == 'web' - ERROR_MESSAGES[:non_web_delete_protected_branch] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:non_web_delete_protected_branch] end end def protected_branch_push_checks if matching_merge_request? unless user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name) - ERROR_MESSAGES[:merge_protected_branch] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:merge_protected_branch] end else unless user_access.can_push_to_branch?(@branch_name) - ERROR_MESSAGES[:push_protected_branch] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_protected_branch] end end end @@ -100,7 +98,7 @@ module Gitlab return unless @tag_name if tag_exists? && user_access.cannot_do_action?(:admin_project) - return ERROR_MESSAGES[:change_existing_tags] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:change_existing_tags] end protected_tag_checks @@ -109,11 +107,11 @@ module Gitlab def protected_tag_checks return unless ProtectedTag.protected?(project, @tag_name) - return ERROR_MESSAGES[:update_protected_tag] if update? - return ERROR_MESSAGES[:delete_protected_tag] if deletion? + raise(GitAccess::UnauthorizedError, ERROR_MESSAGES[:update_protected_tag]) if update? + raise(GitAccess::UnauthorizedError, ERROR_MESSAGES[:delete_protected_tag]) if deletion? unless user_access.can_create_tag?(@tag_name) - return ERROR_MESSAGES[:create_protected_tag] + raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:create_protected_tag] end end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 1ffac5385c2..f43359d7dbd 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -3,6 +3,7 @@ module Gitlab class GitAccess UnauthorizedError = Class.new(StandardError) + NotFoundError = Class.new(StandardError) ERROR_MESSAGES = { upload: 'You are not allowed to upload code for this project.', @@ -47,8 +48,6 @@ module Gitlab end build_status_object(true) - rescue UnauthorizedError => ex - build_status_object(false, ex.message) end def guest_can_download_code? @@ -90,7 +89,7 @@ module Gitlab def check_project_accessibility! if project.blank? || !can_read_project? - raise UnauthorizedError, ERROR_MESSAGES[:project_not_found] + raise NotFoundError, ERROR_MESSAGES[:project_not_found] end end @@ -234,8 +233,8 @@ module Gitlab end end - def build_status_object(status, message = '') - Gitlab::GitAccessStatus.new(status, message) + def build_status_object(status) + Gitlab::GitAccessStatus.new(status) end end end diff --git a/lib/gitlab/git_access_status.rb b/lib/gitlab/git_access_status.rb index 09bb01be694..94edf80c0f6 100644 --- a/lib/gitlab/git_access_status.rb +++ b/lib/gitlab/git_access_status.rb @@ -7,9 +7,5 @@ module Gitlab @status = status @message = message end - - def to_json(opts = nil) - { status: @status, message: @message }.to_json(opts) - end end end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index dccafd2cae8..4c87482430f 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -16,7 +16,7 @@ module Gitlab if user_access.can_do_action?(:create_wiki) build_status_object(true) else - build_status_object(false, ERROR_MESSAGES[:write_to_wiki]) + raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki] end end end diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index 8d81ed5856e..c0c309d8179 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -23,29 +23,27 @@ describe Gitlab::Checks::ChangeAccess, lib: true do before { project.add_developer(user) } context 'without failed checks' do - it "doesn't return any error" do - expect(subject.status).to be(true) + it "doesn't raise an error" do + expect { subject }.not_to raise_error end end context 'when the user is not allowed to push code' do - it 'returns an error' do + it 'raises an error' do expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false) - expect(subject.status).to be(false) - expect(subject.message).to eq('You are not allowed to push code to this project.') + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.') end end context 'tags check' do let(:ref) { 'refs/tags/v1.0.0' } - it 'returns an error if the user is not allowed to update tags' do + it 'raises an error if the user is not allowed to update tags' do allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true) expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false) - expect(subject.status).to be(false) - expect(subject.message).to eq('You are not allowed to change existing tags on this project.') + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.') end context 'with protected tag' do @@ -59,8 +57,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do let(:newrev) { '0000000000000000000000000000000000000000' } it 'is prevented' do - expect(subject.status).to be(false) - expect(subject.message).to include('cannot be deleted') + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be deleted/) end end @@ -69,8 +66,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' } it 'is prevented' do - expect(subject.status).to be(false) - expect(subject.message).to include('cannot be updated') + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be updated/) end end end @@ -81,15 +77,14 @@ describe Gitlab::Checks::ChangeAccess, lib: true do let(:ref) { 'refs/tags/v9.1.0' } it 'prevents creation below access level' do - expect(subject.status).to be(false) - expect(subject.message).to include('allowed to create this tag as it is protected') + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /allowed to create this tag as it is protected/) end context 'when user has access' do let!(:protected_tag) { create(:protected_tag, :developers_can_create, project: project, name: 'v*') } it 'allows tag creation' do - expect(subject.status).to be(true) + expect { subject }.not_to raise_error end end end @@ -101,9 +96,8 @@ describe Gitlab::Checks::ChangeAccess, lib: true do let(:newrev) { '0000000000000000000000000000000000000000' } let(:ref) { 'refs/heads/master' } - it 'returns an error' do - expect(subject.status).to be(false) - expect(subject.message).to eq('The default branch of a project cannot be deleted.') + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'The default branch of a project cannot be deleted.') end end @@ -113,27 +107,24 @@ describe Gitlab::Checks::ChangeAccess, lib: true do allow(ProtectedBranch).to receive(:protected?).with(project, 'feature').and_return(true) end - it 'returns an error if the user is not allowed to do forced pushes to protected branches' do + it 'raises an error if the user is not allowed to do forced pushes to protected branches' do expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true) - expect(subject.status).to be(false) - expect(subject.message).to eq('You are not allowed to force push code to a protected branch on this project.') + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to force push code to a protected branch on this project.') end - it 'returns an error if the user is not allowed to merge to protected branches' do + it 'raises an error if the user is not allowed to merge to protected branches' do expect_any_instance_of(Gitlab::Checks::MatchingMergeRequest).to receive(:match?).and_return(true) expect(user_access).to receive(:can_merge_to_branch?).and_return(false) expect(user_access).to receive(:can_push_to_branch?).and_return(false) - expect(subject.status).to be(false) - expect(subject.message).to eq('You are not allowed to merge code into protected branches on this project.') + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to merge code into protected branches on this project.') end - it 'returns an error if the user is not allowed to push to protected branches' do + it 'raises an error if the user is not allowed to push to protected branches' do expect(user_access).to receive(:can_push_to_branch?).and_return(false) - expect(subject.status).to be(false) - expect(subject.message).to eq('You are not allowed to push code to protected branches on this project.') + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to protected branches on this project.') end context 'branch deletion' do @@ -141,9 +132,8 @@ describe Gitlab::Checks::ChangeAccess, lib: true do let(:ref) { 'refs/heads/feature' } context 'if the user is not allowed to delete protected branches' do - it 'returns an error' do - expect(subject.status).to be(false) - expect(subject.message).to eq('You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.') + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.') end end @@ -156,14 +146,13 @@ describe Gitlab::Checks::ChangeAccess, lib: true do let(:protocol) { 'web' } it 'allows branch deletion' do - expect(subject.status).to be(true) + expect { subject }.not_to raise_error end end context 'over SSH or HTTP' do - it 'returns an error' do - expect(subject.status).to be(false) - expect(subject.message).to eq('You can only delete protected branches using the web interface.') + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only delete protected branches using the web interface.') end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 0efe15856fc..10a7222c2b6 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -18,8 +18,7 @@ describe Gitlab::GitAccess, lib: true do describe '#check with single protocols allowed' do def disable_protocol(protocol) - settings = ::ApplicationSetting.create_from_defaults - settings.update_attribute(:enabled_git_access_protocol, protocol) + allow(Gitlab::ProtocolAccess).to receive(:allowed?).with(protocol).and_return(false) end context 'ssh disabled' do @@ -28,11 +27,11 @@ describe Gitlab::GitAccess, lib: true do end it 'blocks ssh git push' do - expect(access.check('git-receive-pack', '_any').allowed?).to be_falsey + expect { push_access_check }.to raise_unauthorized('Git access over SSH is not allowed') end it 'blocks ssh git pull' do - expect(access.check('git-upload-pack', '_any').allowed?).to be_falsey + expect { pull_access_check }.to raise_unauthorized('Git access over SSH is not allowed') end end @@ -44,11 +43,11 @@ describe Gitlab::GitAccess, lib: true do end it 'blocks http push' do - expect(access.check('git-receive-pack', '_any').allowed?).to be_falsey + expect { push_access_check }.to raise_unauthorized('Git access over HTTP is not allowed') end it 'blocks http git pull' do - expect(access.check('git-upload-pack', '_any').allowed?).to be_falsey + expect { pull_access_check }.to raise_unauthorized('Git access over HTTP is not allowed') end end end @@ -64,23 +63,21 @@ describe Gitlab::GitAccess, lib: true do before { deploy_key.projects << project } it 'allows pull access' do - expect(pull_access_check.allowed?).to be_truthy + expect { pull_access_check }.not_to raise_error end it 'allows push access' do - expect(push_access_check.allowed?).to be_truthy + expect { push_access_check }.not_to raise_error end end context 'when the Deploykey does not have access to the project' do it 'blocks pulls with "not found"' do - expect(pull_access_check.allowed?).to be_falsey - expect(pull_access_check.message).to eq('The project you were looking for could not be found.') + expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') end it 'blocks pushes with "not found"' do - expect(push_access_check.allowed?).to be_falsey - expect(push_access_check.message).to eq('The project you were looking for could not be found.') + expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') end end end @@ -90,23 +87,21 @@ describe Gitlab::GitAccess, lib: true do before { project.team << [user, :master] } it 'allows pull access' do - expect(pull_access_check.allowed?).to be_truthy + expect { pull_access_check }.not_to raise_error end it 'allows push access' do - expect(push_access_check.allowed?).to be_truthy + expect { push_access_check }.not_to raise_error end end context 'when the User cannot read the project' do it 'blocks pulls with "not found"' do - expect(pull_access_check.allowed?).to be_falsey - expect(pull_access_check.message).to eq('The project you were looking for could not be found.') + expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') end it 'blocks pushes with "not found"' do - expect(push_access_check.allowed?).to be_falsey - expect(push_access_check.message).to eq('The project you were looking for could not be found.') + expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') end end end @@ -117,12 +112,11 @@ describe Gitlab::GitAccess, lib: true do let(:authentication_abilities) { build_authentication_abilities } it 'allows pull access' do - expect(pull_access_check.allowed?).to be_truthy + expect { pull_access_check }.not_to raise_error end it 'does not block pushes with "not found"' do - expect(push_access_check.allowed?).to be_falsey - expect(push_access_check.message).to eq('You are not allowed to upload code for this project.') + expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') end end end @@ -134,24 +128,21 @@ describe Gitlab::GitAccess, lib: true do let(:project) { create(:project, :repository, :public) } it 'allows pull access' do - expect(pull_access_check.allowed?).to be_truthy + expect { pull_access_check }.not_to raise_error end it 'does not block pushes with "not found"' do - expect(push_access_check.allowed?).to be_falsey - expect(push_access_check.message).to eq('You are not allowed to upload code for this project.') + expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') end end context 'when guests cannot read the project' do it 'blocks pulls with "not found"' do - expect(pull_access_check.allowed?).to be_falsey - expect(pull_access_check.message).to eq('The project you were looking for could not be found.') + expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') end it 'blocks pushes with "not found"' do - expect(push_access_check.allowed?).to be_falsey - expect(push_access_check.message).to eq('The project you were looking for could not be found.') + expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') end end end @@ -161,10 +152,8 @@ describe Gitlab::GitAccess, lib: true do let(:project) { nil } it 'blocks any command with "not found"' do - expect(pull_access_check.allowed?).to be_falsey - expect(pull_access_check.message).to eq('The project you were looking for could not be found.') - expect(push_access_check.allowed?).to be_falsey - expect(push_access_check.message).to eq('The project you were looking for could not be found.') + expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') + expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') end end end @@ -181,13 +170,11 @@ describe Gitlab::GitAccess, lib: true do end context 'when calling git-upload-pack' do - subject { access.check('git-upload-pack', '_any') } - it { expect(subject.allowed?).to be_falsey } - it { expect(subject.message).to eq('The command "git-upload-pack" is not allowed.') } + it { expect { pull_access_check }.to raise_unauthorized('The command "git-upload-pack" is not allowed.') } end context 'when calling git-receive-pack' do - it { expect(access.check('git-receive-pack', '_any').allowed?).to be_truthy } + it { expect { push_access_check }.not_to raise_error } end end @@ -197,26 +184,22 @@ describe Gitlab::GitAccess, lib: true do end context 'when calling git-receive-pack' do - subject { access.check('git-receive-pack', '_any') } - it { expect(subject.allowed?).to be_falsey } - it { expect(subject.message).to eq('The command "git-receive-pack" is not allowed.') } + it { expect { push_access_check }.to raise_unauthorized('The command "git-receive-pack" is not allowed.') } end context 'when calling git-upload-pack' do - it { expect(access.check('git-upload-pack', '_any').allowed?).to be_truthy } + it { expect { pull_access_check }.not_to raise_error } end end end end describe '#check_download_access!' do - subject { access.check('git-upload-pack', '_any') } - describe 'master permissions' do before { project.team << [user, :master] } context 'pull code' do - it { expect(subject.allowed?).to be_truthy } + it { expect { pull_access_check }.not_to raise_error } end end @@ -224,8 +207,7 @@ describe Gitlab::GitAccess, lib: true do before { project.team << [user, :guest] } context 'pull code' do - it { expect(subject.allowed?).to be_falsey } - it { expect(subject.message).to match(/You are not allowed to download code/) } + it { expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.') } end end @@ -236,24 +218,22 @@ describe Gitlab::GitAccess, lib: true do end context 'pull code' do - it { expect(subject.allowed?).to be_falsey } - it { expect(subject.message).to match(/Your account has been blocked/) } + it { expect { pull_access_check }.to raise_unauthorized('Your account has been blocked.') } end end describe 'without access to project' do context 'pull code' do - it { expect(subject.allowed?).to be_falsey } + it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') } end context 'when project is public' do let(:public_project) { create(:project, :public, :repository) } - let(:guest_access) { Gitlab::GitAccess.new(nil, public_project, 'web', authentication_abilities: []) } - subject { guest_access.check('git-upload-pack', '_any') } + let(:access) { Gitlab::GitAccess.new(nil, public_project, 'web', authentication_abilities: []) } context 'when repository is enabled' do it 'give access to download code' do - expect(subject.allowed?).to be_truthy + expect { pull_access_check }.not_to raise_error end end @@ -261,8 +241,7 @@ describe Gitlab::GitAccess, lib: true do it 'does not give access to download code' do public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED) - expect(subject.allowed?).to be_falsey - expect(subject.message).to match(/You are not allowed to download code/) + expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.') end end end @@ -276,26 +255,26 @@ describe Gitlab::GitAccess, lib: true do context 'when project is authorized' do before { key.projects << project } - it { expect(subject).to be_allowed } + it { expect { pull_access_check }.not_to raise_error } end context 'when unauthorized' do context 'from public project' do let(:project) { create(:project, :public, :repository) } - it { expect(subject).to be_allowed } + it { expect { pull_access_check }.not_to raise_error } end context 'from internal project' do let(:project) { create(:project, :internal, :repository) } - it { expect(subject).not_to be_allowed } + it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') } end context 'from private project' do let(:project) { create(:project, :private, :repository) } - it { expect(subject).not_to be_allowed } + it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') } end end end @@ -308,7 +287,7 @@ describe Gitlab::GitAccess, lib: true do let(:project) { create(:project, :repository, namespace: user.namespace) } context 'pull code' do - it { expect(subject).to be_allowed } + it { expect { pull_access_check }.not_to raise_error } end end @@ -316,7 +295,7 @@ describe Gitlab::GitAccess, lib: true do before { project.team << [user, :reporter] } context 'pull code' do - it { expect(subject).to be_allowed } + it { expect { pull_access_check }.not_to raise_error } end end @@ -327,13 +306,13 @@ describe Gitlab::GitAccess, lib: true do before { project.team << [user, :reporter] } context 'pull code' do - it { expect(subject).to be_allowed } + it { expect { pull_access_check }.not_to raise_error } end end context 'when is not member of the project' do context 'pull code' do - it { expect(subject).not_to be_allowed } + it { expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.') } end end end @@ -342,7 +321,7 @@ describe Gitlab::GitAccess, lib: true do let(:actor) { :ci } context 'pull code' do - it { expect(subject).to be_allowed } + it { expect { pull_access_check }.not_to raise_error } end end end @@ -532,42 +511,32 @@ describe Gitlab::GitAccess, lib: true do end end - shared_examples 'pushing code' do |can| - subject { access.check('git-receive-pack', '_any') } + describe 'build authentication abilities' do + let(:authentication_abilities) { build_authentication_abilities } context 'when project is authorized' do - before { authorize } + before { project.team << [user, :reporter] } - it { expect(subject).public_send(can, be_allowed) } + it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') } end context 'when unauthorized' do context 'to public project' do let(:project) { create(:project, :public, :repository) } - it { expect(subject).not_to be_allowed } + it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') } end context 'to internal project' do let(:project) { create(:project, :internal, :repository) } - it { expect(subject).not_to be_allowed } + it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') } end context 'to private project' do let(:project) { create(:project, :private, :repository) } - it { expect(subject).not_to be_allowed } - end - end - end - - describe 'build authentication abilities' do - let(:authentication_abilities) { build_authentication_abilities } - - it_behaves_like 'pushing code', :not_to do - def authorize - project.team << [user, :reporter] + it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') } end end end @@ -579,9 +548,29 @@ describe Gitlab::GitAccess, lib: true do context 'when deploy_key can push' do let(:can_push) { true } - it_behaves_like 'pushing code', :to do - def authorize - key.projects << project + context 'when project is authorized' do + before { key.projects << project } + + it { expect { push_access_check }.not_to raise_error } + end + + context 'when unauthorized' do + context 'to public project' do + let(:project) { create(:project, :public, :repository) } + + it { expect { push_access_check }.to raise_unauthorized('This deploy key does not have write access to this project.') } + end + + context 'to internal project' do + let(:project) { create(:project, :internal, :repository) } + + it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') } + end + + context 'to private project' do + let(:project) { create(:project, :private, :repository) } + + it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') } end end end @@ -589,9 +578,29 @@ describe Gitlab::GitAccess, lib: true do context 'when deploy_key cannot push' do let(:can_push) { false } - it_behaves_like 'pushing code', :not_to do - def authorize - key.projects << project + context 'when project is authorized' do + before { key.projects << project } + + it { expect { push_access_check }.to raise_unauthorized('This deploy key does not have write access to this project.') } + end + + context 'when unauthorized' do + context 'to public project' do + let(:project) { create(:project, :public, :repository) } + + it { expect { push_access_check }.to raise_unauthorized('This deploy key does not have write access to this project.') } + end + + context 'to internal project' do + let(:project) { create(:project, :internal, :repository) } + + it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') } + end + + context 'to private project' do + let(:project) { create(:project, :private, :repository) } + + it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') } end end end @@ -599,6 +608,14 @@ describe Gitlab::GitAccess, lib: true do private + def raise_unauthorized(message) + raise_error(Gitlab::GitAccess::UnauthorizedError, message) + end + + def raise_not_found(message) + raise_error(Gitlab::GitAccess::NotFoundError, message) + end + def build_authentication_abilities [ :read_project, diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index 1ae293416e4..a1eb95750ba 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -20,7 +20,7 @@ describe Gitlab::GitAccessWiki, lib: true do subject { access.check('git-receive-pack', changes) } - it { expect(subject.allowed?).to be_truthy } + it { expect { subject }.not_to raise_error } end def changes @@ -36,7 +36,7 @@ describe Gitlab::GitAccessWiki, lib: true do context 'when wiki feature is enabled' do it 'give access to download wiki code' do - expect(subject.allowed?).to be_truthy + expect { subject }.not_to raise_error end end @@ -44,8 +44,7 @@ describe Gitlab::GitAccessWiki, lib: true do it 'does not give access to download wiki code' do project.project_feature.update_attribute(:wiki_access_level, ProjectFeature::DISABLED) - expect(subject.allowed?).to be_falsey - expect(subject.message).to match(/You are not allowed to download code/) + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to download code from this project.') end end end