Refactor to let GitAccess errors bubble up

No external behavior change.

This allows `GitHttpController` to set the HTTP status based on the type of error. Alternatively, we could have added an attribute to GitAccessStatus, but this pattern seemed appropriate.

app/controllers/projects/git_http_controller.rb

 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
   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
   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
   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

lib/api/internal.rb

@@ -32,14 +32,17 @@ 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])
 
+        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
 
-        if access_status.status
         log_user_activity(actor)
 
         # Project id to pass between components that don't share/don't have
@@ -54,7 +57,6 @@ module API
           else
             project.repository.path_to_repo
           end
-        end
 
         response
       end

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
-      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
 

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

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

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

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

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