Check ability ability before proceeding with project specific checks

lib/gitlab/git_access.rb

@@ -15,8 +15,9 @@ module Gitlab
     ERROR_MESSAGES = {
       upload: 'You are not allowed to upload code for this project.',
       download: 'You are not allowed to download code from this project.',
-      deploy_key_upload:
-        'This deploy key does not have write access to this project.',
+      auth_upload: 'You are not allowed to upload code.',
+      auth_download: 'You are not allowed to download code.',
+      deploy_key_upload: 'This deploy key does not have write access to this project.',
       no_repo: 'A repository for this project does not exist yet.',
       project_not_found: 'The project you were looking for could not be found.',
       account_blocked: 'Your account has been blocked.',
@@ -47,6 +48,7 @@ module Gitlab
       check_protocol!
       check_valid_actor!
       check_active_user!
+      check_authentication_abilities!(cmd)
       check_command_disabled!(cmd)
       check_command_existence!(cmd)
       check_db_accessibility!(cmd)
@@ -107,6 +109,19 @@ module Gitlab
       end
     end
 
+    def check_authentication_abilities!(cmd)
+      case cmd
+      when *DOWNLOAD_COMMANDS
+        unless authentication_abilities.include?(:download_code) || authentication_abilities.include?(:build_download_code)
+          raise UnauthorizedError, ERROR_MESSAGES[:auth_download]
+        end
+      when *PUSH_COMMANDS
+        unless authentication_abilities.include?(:push_code)
+          raise UnauthorizedError, ERROR_MESSAGES[:auth_upload]
+        end
+      end
+    end
+
     def check_project_accessibility!
       if project.blank? || !can_read_project?
         raise NotFoundError, ERROR_MESSAGES[:project_not_found]
@@ -209,9 +224,11 @@ module Gitlab
       end
 
       if deploy_key
-        check_deploy_key_push_access!
+        unless deploy_key.can_push_to?(project)
+          raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload]
+        end
       elsif user
-        check_user_push_access!
+        # User access is verified in check_change_access!
       else
         raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
@@ -230,18 +247,6 @@ module Gitlab
       check_change_access!(changes)
     end
 
-    def check_user_push_access!
-      unless authentication_abilities.include?(:push_code)
-        raise UnauthorizedError, ERROR_MESSAGES[:upload]
-      end
-    end
-
-    def check_deploy_key_push_access!
-      unless deploy_key.can_push_to?(project)
-        raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload]
-      end
-    end
-
     def check_change_access!(changes)
       changes_list = Gitlab::ChangesList.new(changes)
 

spec/lib/gitlab/git_access_spec.rb

@@ -119,7 +119,7 @@ describe Gitlab::GitAccess do
           end
 
           it 'does not block pushes with "not found"' do
-            expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload])
+            expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload])
           end
         end
       end
@@ -327,7 +327,7 @@ describe Gitlab::GitAccess do
       let(:authentication_abilities) { [] }
 
       it 'raises unauthorized with download error' do
-        expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:download])
+        expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_download])
       end
 
       context 'when authentication abilities include download code' do
@@ -351,7 +351,7 @@ describe Gitlab::GitAccess do
       let(:authentication_abilities) { [] }
 
       it 'raises unauthorized with push error' do
-        expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload])
+        expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload])
       end
 
       context 'when authentication abilities include push code' do
@@ -977,19 +977,6 @@ describe Gitlab::GitAccess do
       run_permission_checks(admin: matrix)
     end
 
-    context "when in a read-only GitLab instance" do
-      before do
-        create(:protected_branch, name: 'feature', project: project)
-        allow(Gitlab::Database).to receive(:read_only?) { true }
-      end
-
-      # Only check admin; if an admin can't do it, other roles can't either
-      matrix = permissions_matrix[:admin].dup
-      matrix.each { |key, _| matrix[key] = false }
-
-      run_permission_checks(admin: matrix)
-    end
-
     describe "push_rule_check" do
       let(:start_sha) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
       let(:end_sha)   { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
@@ -1151,26 +1138,26 @@ describe Gitlab::GitAccess do
         project.add_reporter(user)
       end
 
-      it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) }
+      it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_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(described_class::ERROR_MESSAGES[:upload]) }
+        it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) }
       end
 
       context 'to internal project' do
         let(:project) { create(:project, :internal, :repository) }
 
-        it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) }
+        it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) }
       end
 
       context 'to private project' do
         let(:project) { create(:project, :private, :repository) }
 
-        it { expect { push_access_check }.to raise_not_found }
+        it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) }
       end
     end
   end

spec/requests/git_http_spec.rb

@@ -620,7 +620,7 @@ describe 'Git HTTP requests' do
               push_get(path, env)
 
               expect(response).to have_gitlab_http_status(:forbidden)
-              expect(response.body).to eq(git_access_error(:upload))
+              expect(response.body).to eq(git_access_error(:auth_upload))
             end
 
             # We are "authenticated" as CI using a valid token here. But we are
@@ -660,7 +660,7 @@ describe 'Git HTTP requests' do
                 push_get path, env
 
                 expect(response).to have_gitlab_http_status(:forbidden)
-                expect(response.body).to eq(git_access_error(:upload))
+                expect(response.body).to eq(git_access_error(:auth_upload))
               end
             end