Merge branch 'mk-fix-git-over-http-rejections' into 'master'

Fix Git-over-HTTP rejections

See merge request !11398

3 files changed, 25 insertions, 78 deletions

diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb
index ae91e02488a..2b6afaa6233 100644
--- a/app/controllers/concerns/lfs_request.rb
+++ b/app/controllers/concerns/lfs_request.rb
@@ -106,4 +106,8 @@ module LfsRequest
   def objects
     @objects ||= (params[:objects] || []).to_a
   end
+
+  def has_authentication_ability?(capability)
+    (authentication_abilities || []).include?(capability)
+  end
 end
diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 9a1bf037a95..7f3205a8001 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -128,32 +128,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     @authentication_result = Gitlab::Auth.find_for_git_client(
       login, password, project: project, ip: request.ip)
 
-    return false unless @authentication_result.success?
-
-    if download_request?
-      authentication_has_download_access?
-    else
-      authentication_has_upload_access?
-    end
+    @authentication_result.success?
   end
 
   def ci?
     authentication_result.ci?(project)
   end
-
-  def authentication_has_download_access?
-    has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code)
-  end
-
-  def authentication_has_upload_access?
-    has_authentication_ability?(:push_code)
-  end
-
-  def has_authentication_ability?(capability)
-    (authentication_abilities || []).include?(capability)
-  end
-
-  def authentication_project
-    authentication_result.project
-  end
 end
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 9e4edcae101..b6b62da7b60 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -1,38 +1,27 @@
 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
-
-      render_ok
-    elsif receive_pack? && receive_pack_allowed?
-      render_ok
-    elsif http_blocked?
-      render_http_not_allowed
-    else
-      render_denied
-    end
+    log_user_activity if upload_pack?
+
+    render_ok
   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
+    render_ok
   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
+    render_ok
   end
 
   private
@@ -45,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]
@@ -62,47 +47,27 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name)
   end
 
-  def render_http_not_allowed
-    render plain: access_check.message, status: :forbidden
+  def render_403(exception)
+    render plain: exception.message, status: :forbidden
   end
 
-  def render_denied
-    if user && can?(user, :read_project, project)
-      render plain: access_denied_message, status: :forbidden
-    else
-      # Do not leak information about project existence
-      render_not_found
-    end
-  end
-
-  def access_denied_message
-    'Access denied'
+  def render_404(exception)
+    render plain: exception.message, status: :not_found
   end
 
-  def upload_pack_allowed?
-    return false unless Gitlab.config.gitlab_shell.upload_pack
-
-    access_check.allowed? || ci?
+  def access
+    @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities)
   end
 
-  def access
-    @access ||= access_klass.new(user, project, 'http', authentication_abilities: authentication_abilities)
+  def access_actor
+    return user if user
+    return :ci if ci?
   end
 
   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 http_blocked?
-    !access.protocol_allowed?
-  end
-
-  def receive_pack_allowed?
-    return false unless Gitlab.config.gitlab_shell.receive_pack
-
-    access_check.allowed?
+    access.check(git_command, '_any')
   end
 
   def access_klass