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.

5 files changed, 40 insertions, 45 deletions

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,29 +32,31 @@ 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 }
+        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)
+        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?)
+        # 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
-        end
+        # 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