From b7712260b4657b2d070dfbc61a46c26657541383 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 21 Aug 2017 08:46:52 +0000 Subject: Merge branch 'rs-git-access-spec-speed' into 'master' Greatly reduce test duration for git_access_spec See merge request !13675 --- lib/gitlab/git_access.rb | 19 +-- spec/lib/gitlab/git_access_spec.rb | 249 +++++++++++++++++-------------------- 2 files changed, 126 insertions(+), 142 deletions(-) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 0b62911958d..3e8b83c0f90 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -4,6 +4,7 @@ module Gitlab class GitAccess UnauthorizedError = Class.new(StandardError) NotFoundError = Class.new(StandardError) + ProjectMovedError = Class.new(NotFoundError) ERROR_MESSAGES = { upload: 'You are not allowed to upload code for this project.', @@ -90,18 +91,18 @@ module Gitlab end def check_project_moved! - if redirected_path - url = protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo - message = <<-MESSAGE.strip_heredoc - Project '#{redirected_path}' was moved to '#{project.full_path}'. + return unless redirected_path - Please update your Git remote and try again: + url = protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo + message = <<-MESSAGE.strip_heredoc + Project '#{redirected_path}' was moved to '#{project.full_path}'. - git remote set-url origin #{url} - MESSAGE + Please update your Git remote and try again: - raise NotFoundError, message - end + git remote set-url origin #{url} + MESSAGE + + raise ProjectMovedError, message end def check_command_disabled!(cmd) diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 2d6ea37d0ac..80dc49e99cb 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -1,21 +1,17 @@ require 'spec_helper' describe Gitlab::GitAccess do - let(:pull_access_check) { access.check('git-upload-pack', '_any') } - let(:push_access_check) { access.check('git-receive-pack', '_any') } - let(:access) { described_class.new(actor, project, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path) } - let(:project) { create(:project, :repository) } - let(:user) { create(:user) } + set(:user) { create(:user) } + let(:actor) { user } + let(:project) { create(:project, :repository) } let(:protocol) { 'ssh' } + let(:authentication_abilities) { %i[read_project download_code push_code] } let(:redirected_path) { nil } - let(:authentication_abilities) do - [ - :read_project, - :download_code, - :push_code - ] - end + + let(:access) { described_class.new(actor, project, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path) } + let(:push_access_check) { access.check('git-receive-pack', '_any') } + let(:pull_access_check) { access.check('git-upload-pack', '_any') } describe '#check with single protocols allowed' do def disable_protocol(protocol) @@ -27,12 +23,11 @@ describe Gitlab::GitAccess do disable_protocol('ssh') end - it 'blocks ssh git push' do - expect { push_access_check }.to raise_unauthorized('Git access over SSH is not allowed') - end - - it 'blocks ssh git pull' do - expect { pull_access_check }.to raise_unauthorized('Git access over SSH is not allowed') + it 'blocks ssh git push and pull' do + aggregate_failures do + expect { push_access_check }.to raise_unauthorized('Git access over SSH is not allowed') + expect { pull_access_check }.to raise_unauthorized('Git access over SSH is not allowed') + end end end @@ -43,12 +38,11 @@ describe Gitlab::GitAccess do disable_protocol('http') end - it 'blocks http push' do - expect { push_access_check }.to raise_unauthorized('Git access over HTTP is not allowed') - end - - it 'blocks http git pull' do - expect { pull_access_check }.to raise_unauthorized('Git access over HTTP is not allowed') + it 'blocks http push and pull' do + aggregate_failures do + expect { push_access_check }.to raise_unauthorized('Git access over HTTP is not allowed') + expect { pull_access_check }.to raise_unauthorized('Git access over HTTP is not allowed') + end end end end @@ -65,22 +59,20 @@ describe Gitlab::GitAccess do deploy_key.projects << project end - it 'allows pull access' do - expect { pull_access_check }.not_to raise_error - end - - it 'allows push access' do - expect { push_access_check }.not_to raise_error + it 'allows push and pull access' do + aggregate_failures do + expect { push_access_check }.not_to raise_error + expect { pull_access_check }.not_to raise_error + end 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 }.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 }.to raise_not_found('The project you were looking for could not be found.') + it 'blocks push and pull with "not found"' do + aggregate_failures do + expect { push_access_check }.to raise_not_found + expect { pull_access_check }.to raise_not_found + end end end end @@ -88,25 +80,23 @@ describe Gitlab::GitAccess do context 'when actor is a User' do context 'when the User can read the project' do before do - project.team << [user, :master] + project.add_master(user) end - it 'allows pull access' do - expect { pull_access_check }.not_to raise_error - end - - it 'allows push access' do - expect { push_access_check }.not_to raise_error + it 'allows push and pull access' do + aggregate_failures do + expect { pull_access_check }.not_to raise_error + expect { push_access_check }.not_to raise_error + end end end context 'when the User cannot read the project' do - it 'blocks pulls with "not found"' do - 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 }.to raise_not_found('The project you were looking for could not be found.') + it 'blocks push and pull with "not found"' do + aggregate_failures do + expect { push_access_check }.to raise_not_found + expect { pull_access_check }.to raise_not_found + end end end end @@ -121,7 +111,7 @@ describe Gitlab::GitAccess do end it 'does not block pushes with "not found"' do - expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') + expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) end end end @@ -137,17 +127,17 @@ describe Gitlab::GitAccess do end it 'does not block pushes with "not found"' do - expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') + expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) end end context 'when guests cannot read the project' do it 'blocks pulls with "not found"' do - expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') + expect { pull_access_check }.to raise_not_found end it 'blocks pushes with "not found"' do - expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') + expect { push_access_check }.to raise_not_found end end end @@ -156,48 +146,50 @@ describe Gitlab::GitAccess do context 'when the project is nil' do let(:project) { nil } - it 'blocks any command with "not found"' do - 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.') + it 'blocks push and pull with "not found"' do + aggregate_failures do + expect { pull_access_check }.to raise_not_found + expect { push_access_check }.to raise_not_found + end end end end describe '#check_project_moved!' do before do - project.team << [user, :master] + project.add_master(user) end context 'when a redirect was not followed to find the project' do - context 'pull code' do - it { expect { pull_access_check }.not_to raise_error } - end - - context 'push code' do - it { expect { push_access_check }.not_to raise_error } + it 'allows push and pull access' do + aggregate_failures do + expect { push_access_check }.not_to raise_error + expect { pull_access_check }.not_to raise_error + end end end context 'when a redirect was followed to find the project' do let(:redirected_path) { 'some/other-path' } - context 'pull code' do - it { expect { pull_access_check }.to raise_not_found(/Project '#{redirected_path}' was moved to '#{project.full_path}'/) } - it { expect { pull_access_check }.to raise_not_found(/git remote set-url origin #{project.ssh_url_to_repo}/) } + it 'blocks push and pull access' do + aggregate_failures do + expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /Project '#{redirected_path}' was moved to '#{project.full_path}'/) + expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.ssh_url_to_repo}/) - context 'http protocol' do - let(:protocol) { 'http' } - it { expect { pull_access_check }.to raise_not_found(/git remote set-url origin #{project.http_url_to_repo}/) } + expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /Project '#{redirected_path}' was moved to '#{project.full_path}'/) + expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.ssh_url_to_repo}/) end end - context 'push code' do - it { expect { push_access_check }.to raise_not_found(/Project '#{redirected_path}' was moved to '#{project.full_path}'/) } - it { expect { push_access_check }.to raise_not_found(/git remote set-url origin #{project.ssh_url_to_repo}/) } + context 'http protocol' do + let(:protocol) { 'http' } - context 'http protocol' do - let(:protocol) { 'http' } - it { expect { push_access_check }.to raise_not_found(/git remote set-url origin #{project.http_url_to_repo}/) } + it 'includes the path to the project using HTTP' do + aggregate_failures do + expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/) + expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/) + end end end end @@ -242,40 +234,28 @@ describe Gitlab::GitAccess do end describe '#check_download_access!' do - describe 'master permissions' do - before do - project.team << [user, :master] - end + it 'allows masters to pull' do + project.add_master(user) - context 'pull code' do - it { expect { pull_access_check }.not_to raise_error } - end + expect { pull_access_check }.not_to raise_error end - describe 'guest permissions' do - before do - project.team << [user, :guest] - end + it 'disallows guests to pull' do + project.add_guest(user) - context 'pull code' do - it { expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.') } - end + expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:download]) end - describe 'blocked user' do - before do - project.team << [user, :master] - user.block - end + it 'disallows blocked users to pull' do + project.add_master(user) + user.block - context 'pull code' do - it { expect { pull_access_check }.to raise_unauthorized('Your account has been blocked.') } - end + expect { pull_access_check }.to raise_unauthorized('Your account has been blocked.') end describe 'without access to project' do context 'pull code' do - it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') } + it { expect { pull_access_check }.to raise_not_found } end context 'when project is public' do @@ -292,7 +272,7 @@ describe Gitlab::GitAccess do it 'does not give access to download code' do public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED) - expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.') + expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:download]) end end end @@ -321,13 +301,13 @@ describe Gitlab::GitAccess do context 'from internal project' do let(:project) { create(:project, :internal, :repository) } - it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') } + it { expect { pull_access_check }.to raise_not_found } end context 'from private project' do let(:project) { create(:project, :private, :repository) } - it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') } + it { expect { pull_access_check }.to raise_not_found } end end end @@ -369,7 +349,7 @@ describe Gitlab::GitAccess do context 'when is not member of the project' do context 'pull code' do - it { expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.') } + it { expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:download]) } end end end @@ -428,28 +408,30 @@ describe Gitlab::GitAccess do end end - # Run permission checks for a user def self.run_permission_checks(permissions_matrix) - permissions_matrix.keys.each do |role| - describe "#{role} access" do - before do - if role == :admin - user.update_attribute(:admin, true) - else - project.team << [user, role] - end + permissions_matrix.each_pair do |role, matrix| + # Run through the entire matrix for this role in one test to avoid + # repeated setup. + # + # Expectations are given a custom failure message proc so that it's + # easier to identify which check(s) failed. + it "has the correct permissions for #{role}s" do + if role == :admin + user.update_attribute(:admin, true) + else + project.team << [user, role] end - permissions_matrix[role].each do |action, allowed| - context action.to_s do - subject { access.send(:check_push_access!, changes[action]) } + aggregate_failures do + matrix.each do |action, allowed| + check = -> { access.send(:check_push_access!, changes[action]) } - it do - if allowed - expect { subject }.not_to raise_error - else - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError) - end + if allowed + expect(&check).not_to raise_error, + -> { "expected #{action} to be allowed" } + else + expect(&check).to raise_error(Gitlab::GitAccess::UnauthorizedError), + -> { "expected #{action} to be disallowed" } end end end @@ -588,26 +570,26 @@ describe Gitlab::GitAccess do project.team << [user, :reporter] end - it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) } end context 'when unauthorized' do context 'to public project' do let(:project) { create(:project, :public, :repository) } - it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) } end context 'to internal project' do let(:project) { create(:project, :internal, :repository) } - it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) } 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.') } + it { expect { push_access_check }.to raise_not_found } end end end @@ -631,19 +613,19 @@ describe Gitlab::GitAccess 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.') } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:deploy_key_upload]) } 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.') } + it { expect { push_access_check }.to raise_not_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.') } + it { expect { push_access_check }.to raise_not_found } end end end @@ -656,26 +638,26 @@ describe Gitlab::GitAccess do key.projects << project end - it { expect { push_access_check }.to raise_unauthorized('This deploy key does not have write access to this project.') } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:deploy_key_upload]) } 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.') } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:deploy_key_upload]) } 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.') } + it { expect { push_access_check }.to raise_not_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.') } + it { expect { push_access_check }.to raise_not_found } end end end @@ -687,8 +669,9 @@ describe Gitlab::GitAccess do raise_error(Gitlab::GitAccess::UnauthorizedError, message) end - def raise_not_found(message) - raise_error(Gitlab::GitAccess::NotFoundError, message) + def raise_not_found + raise_error(Gitlab::GitAccess::NotFoundError, + Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]) end def build_authentication_abilities -- cgit v1.2.1