Move LfsHelper to a new LfsRequest concern22253-move-lfshelper-methods-somewhere-else-than-app-helpers

Also create a new WorkhorseRequest concern

Signed-off-by: Rémy Coutable <remy@rymai.me>

6 files changed, 91 insertions, 54 deletions

diff --git a/app/helpers/lfs_helper.rb b/app/controllers/concerns/lfs_request.rb
index 2425c3a8bc8..ed22b1e5470 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/controllers/concerns/lfs_request.rb
@@ -1,5 +1,21 @@
-module LfsHelper
-  include Gitlab::Routing.url_helpers
+# This concern assumes:
+# - a `#project` accessor
+# - a `#user` accessor
+# - a `#authentication_result` accessor
+# - a `#can?(object, action, subject)` method
+# - a `#ci?` method
+# - a `#download_request?` method
+# - a `#upload_request?` method
+# - a `#has_authentication_ability?(ability)` method
+module LfsRequest
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :require_lfs_enabled!
+    before_action :lfs_check_access!
+  end
+
+  private
 
   def require_lfs_enabled!
     return if Gitlab.config.lfs.enabled
@@ -17,35 +33,15 @@ module LfsHelper
     return if download_request? && lfs_download_access?
     return if upload_request? && lfs_upload_access?
 
-    if project.public? || (user && user.can?(:read_project, project))
-      render_lfs_forbidden
+    if project.public? || can?(user, :read_project, project)
+      lfs_forbidden!
     else
       render_lfs_not_found
     end
   end
 
-  def lfs_download_access?
-    return false unless project.lfs_enabled?
-
-    ci? || lfs_deploy_token? || user_can_download_code? || build_can_download_code?
-  end
-
-  def objects
-    @objects ||= (params[:objects] || []).to_a
-  end
-
-  def user_can_download_code?
-    has_authentication_ability?(:download_code) && can?(user, :download_code, project)
-  end
-
-  def build_can_download_code?
-    has_authentication_ability?(:build_download_code) && can?(user, :build_download_code, project)
-  end
-
-  def lfs_upload_access?
-    return false unless project.lfs_enabled?
-
-    has_authentication_ability?(:push_code) && can?(user, :push_code, project)
+  def lfs_forbidden!
+    render_lfs_forbidden
   end
 
   def render_lfs_forbidden
@@ -70,6 +66,30 @@ module LfsHelper
     )
   end
 
+  def lfs_download_access?
+    return false unless project.lfs_enabled?
+
+    ci? || lfs_deploy_token? || user_can_download_code? || build_can_download_code?
+  end
+
+  def lfs_upload_access?
+    return false unless project.lfs_enabled?
+
+    has_authentication_ability?(:push_code) && can?(user, :push_code, project)
+  end
+
+  def lfs_deploy_token?
+    authentication_result.lfs_deploy_token?(project)
+  end
+
+  def user_can_download_code?
+    has_authentication_ability?(:download_code) && can?(user, :download_code, project)
+  end
+
+  def build_can_download_code?
+    has_authentication_ability?(:build_download_code) && can?(user, :build_download_code, project)
+  end
+
   def storage_project
     @storage_project ||= begin
       result = project
@@ -82,4 +102,8 @@ module LfsHelper
       result
     end
   end
+
+  def objects
+    @objects ||= (params[:objects] || []).to_a
+  end
 end
diff --git a/app/controllers/concerns/workhorse_request.rb b/app/controllers/concerns/workhorse_request.rb
new file mode 100644
index 00000000000..43c0f1b173c
--- /dev/null
+++ b/app/controllers/concerns/workhorse_request.rb
@@ -0,0 +1,13 @@
+module WorkhorseRequest
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :verify_workhorse_api!
+  end
+
+  private
+
+  def verify_workhorse_api!
+    Gitlab::Workhorse.verify_api_request!(request.headers)
+  end
+end
diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 3f41916e6d3..8714349e27f 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -18,6 +18,14 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
   private
 
+  def download_request?
+    raise NotImplementedError
+  end
+
+  def upload_request?
+    raise NotImplementedError
+  end
+
   def authenticate_user
     @authentication_result = Gitlab::Auth::Result.new
 
@@ -130,10 +138,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     authentication_result.ci?(project)
   end
 
-  def lfs_deploy_token?
-    authentication_result.lfs_deploy_token?(project)
-  end
-
   def authentication_has_download_access?
     has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code)
   end
@@ -149,8 +153,4 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   def authentication_project
     authentication_result.project
   end
-
-  def verify_workhorse_api!
-    Gitlab::Workhorse.verify_api_request!(request.headers)
-  end
 end
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 13caeb42d40..9184dcccac5 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -1,7 +1,5 @@
-# This file should be identical in GitLab Community Edition and Enterprise Edition
-
 class Projects::GitHttpController < Projects::GitHttpClientController
-  before_action :verify_workhorse_api!
+  include WorkhorseRequest
 
   # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)
   # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
@@ -67,14 +65,18 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def render_denied
-    if user && user.can?(:read_project, project)
-      render plain: 'Access denied', status: :forbidden
+    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'
+  end
+
   def upload_pack_allowed?
     return false unless Gitlab.config.gitlab_shell.upload_pack
 
diff --git a/app/controllers/projects/lfs_api_controller.rb b/app/controllers/projects/lfs_api_controller.rb
index 2d493276941..440259b643c 100644
--- a/app/controllers/projects/lfs_api_controller.rb
+++ b/app/controllers/projects/lfs_api_controller.rb
@@ -1,8 +1,7 @@
 class Projects::LfsApiController < Projects::GitHttpClientController
-  include LfsHelper
+  include LfsRequest
 
-  before_action :require_lfs_enabled!
-  before_action :lfs_check_access!, except: [:deprecated]
+  skip_before_action :lfs_check_access!, only: [:deprecated]
 
   def batch
     unless objects.present?
@@ -31,6 +30,14 @@ class Projects::LfsApiController < Projects::GitHttpClientController
 
   private
 
+  def download_request?
+    params[:operation] == 'download'
+  end
+
+  def upload_request?
+    params[:operation] == 'upload'
+  end
+
   def existing_oids
     @existing_oids ||= begin
       storage_project.lfs_objects.where(oid: objects.map { |o| o['oid'].to_s }).pluck(:oid)
@@ -79,12 +86,4 @@ class Projects::LfsApiController < Projects::GitHttpClientController
       }
     }
   end
-
-  def download_request?
-    params[:operation] == 'download'
-  end
-
-  def upload_request?
-    params[:operation] == 'upload'
-  end
 end
diff --git a/app/controllers/projects/lfs_storage_controller.rb b/app/controllers/projects/lfs_storage_controller.rb
index 9005b104e90..32759672b6c 100644
--- a/app/controllers/projects/lfs_storage_controller.rb
+++ b/app/controllers/projects/lfs_storage_controller.rb
@@ -1,9 +1,8 @@
 class Projects::LfsStorageController < Projects::GitHttpClientController
-  include LfsHelper
+  include LfsRequest
+  include WorkhorseRequest
 
-  before_action :require_lfs_enabled!
-  before_action :lfs_check_access!
-  before_action :verify_workhorse_api!, only: [:upload_authorize]
+  skip_before_action :verify_workhorse_api!, only: [:download, :upload_finalize]
 
   def download
     lfs_object = LfsObject.find_by_oid(oid)