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

@@ -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,7 +91,8 @@ module Gitlab
     end
 
     def check_project_moved!
-      if redirected_path
+      return unless 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}'.
@@ -100,8 +102,7 @@ module Gitlab
           git remote set-url origin #{url}
       MESSAGE
 
-        raise NotFoundError, message
-      end
+      raise ProjectMovedError, message
     end
 
     def check_command_disabled!(cmd)

spec/lib/gitlab/git_access_spec.rb

 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,14 +23,13 @@ describe Gitlab::GitAccess do
         disable_protocol('ssh')
       end
 
-      it 'blocks ssh git push' do
+      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')
-      end
-
-      it 'blocks ssh git pull' do
           expect { pull_access_check }.to raise_unauthorized('Git access over SSH is not allowed')
         end
       end
+    end
 
     context 'http disabled' do
       let(:protocol) { 'http' }
@@ -43,15 +38,14 @@ describe Gitlab::GitAccess do
         disable_protocol('http')
       end
 
-      it 'blocks http push' do
+      it 'blocks http push and pull' do
+        aggregate_failures 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')
         end
       end
     end
+  end
 
   describe '#check_project_accessibility!' do
     context 'when the project exists' do
@@ -65,22 +59,20 @@ describe Gitlab::GitAccess do
               deploy_key.projects << project
             end
 
-            it 'allows pull access' do
+            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
-
-            it 'allows push access' do
-              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 }.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
-
-            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.')
             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
+            it 'allows push and pull access' do
+              aggregate_failures do
                 expect { pull_access_check }.not_to raise_error
-            end
-
-            it 'allows push access' do
                 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.')
+            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
-
-            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.')
             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 }
+      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
-
-      context 'push code' do
-        it { expect { push_access_check }.not_to raise_error }
       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' }
-          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]
+    it 'disallows blocked users to pull' do
+      project.add_master(user)
       user.block
-      end
 
-      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
+      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
-          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
+                expect(&check).not_to raise_error,
+                  -> { "expected #{action} to be allowed" }
               else
-                  expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError)
-                end
+                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