Merge branch 'extract-ee-specific-lines-for-issues-and-mr-controllers' into 'master'

Extract EE-specific lines for issues and MR controllers

Closes #6668

See merge request gitlab-org/gitlab-ee!8062

app/controllers/projects/issues_controller.rb

@@ -9,14 +9,27 @@ class Projects::IssuesController < Projects::ApplicationController
   include IssuesCalendar
   include SpammableActions
 
-  prepend_before_action :authenticate_user!, only: [:new, :export_csv]
+  prepend ::EE::Projects::IssuesController
+
+  def self.authenticate_user_only_actions
+    %i[new]
+  end
+
+  def self.issue_except_actions
+    %i[index calendar new create bulk_update]
+  end
+
+  def self.set_issuables_index_only_actions
+    %i[index calendar]
+  end
+
+  prepend_before_action :authenticate_user!, only: authenticate_user_only_actions
 
-  before_action :whitelist_query_limiting_ee, only: [:update]
   before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update]
   before_action :check_issues_available!
-  before_action :issue, except: [:index, :calendar, :new, :create, :bulk_update, :export_csv]
+  before_action :issue, except: issue_except_actions
 
-  before_action :set_issuables_index, only: [:index, :calendar]
+  before_action :set_issuables_index, only: set_issuables_index_only_actions
 
   # Allow write(create) issue
   before_action :authorize_create_issue!, only: [:new, :create]
@@ -27,8 +40,6 @@ class Projects::IssuesController < Projects::ApplicationController
   # Allow create a new branch and empty WIP merge request from current issue
   before_action :authorize_create_merge_request_from!, only: [:create_merge_request]
 
-  prepend ::EE::Projects::IssuesController
-
   respond_to :html
 
   def index
@@ -254,8 +265,4 @@ class Projects::IssuesController < Projects::ApplicationController
     # 3. https://gitlab.com/gitlab-org/gitlab-ce/issues/42426
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42422')
   end
-
-  def whitelist_query_limiting_ee
-    Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ee/issues/4794')
-  end
 end

app/controllers/projects/merge_requests_controller.rb

@@ -11,7 +11,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   prepend ::EE::Projects::MergeRequestsController
 
   skip_before_action :merge_request, only: [:index, :bulk_update]
-  before_action :whitelist_query_limiting_ee, only: [:merge, :show]
   before_action :whitelist_query_limiting, only: [:assign_related_issues, :update]
   before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
   before_action :set_issuables_index, only: [:index]
@@ -171,8 +170,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   end
 
   def merge
-    return access_denied! unless @merge_request.can_be_merged_by?(current_user)
-    return render_404 unless @merge_request.approved?
+    access_check_result = merge_access_check
+
+    return access_check_result if access_check_result
 
     status = merge!
 
@@ -266,9 +266,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       return :failed
     end
 
-    merge_request_service = ::MergeRequests::MergeService.new(@project, current_user, merge_params)
+    merge_service = ::MergeRequests::MergeService.new(@project, current_user, merge_params)
 
-    unless merge_request_service.hooks_validation_pass?(@merge_request)
+    unless merge_service.hooks_validation_pass?(@merge_request)
       return :hook_validation_error
     end
 
@@ -334,13 +334,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     access_denied! unless access_check
   end
 
+  def merge_access_check
+    access_denied! unless @merge_request.can_be_merged_by?(current_user)
+  end
+
   def whitelist_query_limiting
     # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42441
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42438')
   end
-
-  def whitelist_query_limiting_ee
-    # Also see https://gitlab.com/gitlab-org/gitlab-ee/issues/4793
-    Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ee/issues/4792')
-  end
 end

app/services/merge_requests/merge_service.rb

@@ -51,6 +51,11 @@ module MergeRequests
       end
     end
 
+    # Overridden in EE.
+    def hooks_validation_pass?(_merge_request)
+      true
+    end
+
     private
 
     def error_check!

ee/app/controllers/ee/projects/issues_controller.rb

+# frozen_string_literal: true
+
 module EE
   module Projects
     module IssuesController
@@ -6,8 +8,26 @@ module EE
       prepended do
         before_action :check_export_issues_available!, only: [:export_csv]
         before_action :check_service_desk_available!, only: [:service_desk]
-        before_action :set_issuables_index, only: [:index, :calendar, :service_desk]
-        skip_before_action :issue, only: [:service_desk]
+        before_action :whitelist_query_limiting_ee, only: [:update]
+      end
+
+      class_methods do
+        extend ::Gitlab::Utils::Override
+
+        override :authenticate_user_only_actions
+        def authenticate_user_only_actions
+          super + %i[export_csv]
+        end
+
+        override :issue_except_actions
+        def issue_except_actions
+          super + %i[export_csv service_desk]
+        end
+
+        override :set_issuables_index_only_actions
+        def set_issuables_index_only_actions
+          super + %i[service_desk]
+        end
       end
 
       def service_desk
@@ -46,6 +66,10 @@ module EE
       def service_desk?
         action_name == 'service_desk'
       end
+
+      def whitelist_query_limiting_ee
+        ::Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ee/issues/4794')
+      end
     end
   end
 end

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -5,6 +5,11 @@ module EE
 
       APPROVAL_RENDERING_ACTIONS = [:approve, :approvals, :unapprove].freeze
 
+      prepended do
+        before_action :whitelist_query_limiting_ee_merge, only: [:merge]
+        before_action :whitelist_query_limiting_ee_show, only: [:show]
+      end
+
       def approve
         unless merge_request.can_approve?(current_user)
           return render_404
@@ -59,6 +64,23 @@ module EE
           end
         end
       end
+
+      private
+
+      def merge_access_check
+        super_result = super
+
+        return super_result if super_result
+        return render_404 unless @merge_request.approved?
+      end
+
+      def whitelist_query_limiting_ee_merge
+        ::Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ee/issues/4792')
+      end
+
+      def whitelist_query_limiting_ee_show
+        ::Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ee/issues/4793')
+      end
     end
   end
 end

ee/app/services/ee/merge_requests/merge_service.rb

@@ -10,6 +10,7 @@ module EE
         super
       end
 
+      override :hooks_validation_pass?
       def hooks_validation_pass?(merge_request)
         # handle_merge_error needs this. We should move that to a separate
         # object instead of relying on the order of method calls.