Add latest changes from gitlab-org/gitlab@master

2 files changed, 104 insertions, 21 deletions

diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb
index b3d617256ff..ff32bc32d93 100644
--- a/app/services/spam/spam_action_service.rb
+++ b/app/services/spam/spam_action_service.rb
@@ -4,37 +4,69 @@ module Spam
   class SpamActionService
     include SpamConstants
 
+    ##
+    # Utility method to filter SpamParams from parameters, which will later be passed to #execute
+    # after the spammable is created/updated based on the remaining parameters.
+    #
+    # Takes a hash of parameters from an incoming request to modify a model (via a controller,
+    # service, or GraphQL mutation).
+    #
+    # Deletes the parameters which are related to spam and captcha processing, and returns
+    # them in a SpamParams parameters object. See:
+    # https://refactoring.com/catalog/introduceParameterObject.html
+    def self.filter_spam_params!(params)
+      # NOTE: The 'captcha_response' field can be expanded to multiple fields when we move to future
+      # alternative captcha implementations such as FriendlyCaptcha. See
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/273480
+      captcha_response = params.delete(:captcha_response)
+
+      SpamParams.new(
+        api: params.delete(:api),
+        captcha_response: captcha_response,
+        spam_log_id: params.delete(:spam_log_id)
+      )
+    end
+
     attr_accessor :target, :request, :options
     attr_reader :spam_log
 
-    def initialize(spammable:, request:, user:, context: {})
+    def initialize(spammable:, request:, user:, action:)
       @target = spammable
       @request = request
       @user = user
-      @context = context
+      @action = action
       @options = {}
+    end
 
-      if @request
-        @options[:ip_address] = @request.env['action_dispatch.remote_ip'].to_s
-        @options[:user_agent] = @request.env['HTTP_USER_AGENT']
-        @options[:referrer] = @request.env['HTTP_REFERRER']
+    def execute(spam_params:)
+      if request
+        options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s
+        options[:user_agent] = request.env['HTTP_USER_AGENT']
+        options[:referrer] = request.env['HTTP_REFERRER']
       else
-        @options[:ip_address] = @target.ip_address
-        @options[:user_agent] = @target.user_agent
+        # TODO: This code is never used, because we do not perform a verification if there is not a
+        #   request.  Why? Should it be deleted? Or should we check even if there is no request?
+        options[:ip_address] = target.ip_address
+        options[:user_agent] = target.user_agent
       end
-    end
 
-    def execute(api: false, recaptcha_verified:, spam_log_id:)
+      recaptcha_verified = Captcha::CaptchaVerificationService.new.execute(
+        captcha_response: spam_params.captcha_response,
+        request: request
+      )
+
       if recaptcha_verified
-        # If it's a request which is already verified through reCAPTCHA,
+        # If it's a request which is already verified through captcha,
         # update the spam log accordingly.
-        SpamLog.verify_recaptcha!(user_id: user.id, id: spam_log_id)
+        SpamLog.verify_recaptcha!(user_id: user.id, id: spam_params.spam_log_id)
+        ServiceResponse.success(message: "Captcha was successfully verified")
       else
-        return if allowlisted?(user)
-        return unless request
-        return unless check_for_spam?
+        return ServiceResponse.success(message: 'Skipped spam check because user was allowlisted') if allowlisted?(user)
+        return ServiceResponse.success(message: 'Skipped spam check because request was not present') unless request
+        return ServiceResponse.success(message: 'Skipped spam check because it was not required') unless check_for_spam?
 
-        perform_spam_service_check(api)
+        perform_spam_service_check(spam_params.api)
+        ServiceResponse.success(message: "Spam check performed, check #{target.class.name} spammable model for any errors or captcha requirement")
       end
     end
 
@@ -42,13 +74,27 @@ module Spam
 
     private
 
-    attr_reader :user, :context
+    attr_reader :user, :action
+
+    ##
+    # In order to be proceed to the spam check process, the target must be
+    # a dirty instance, which means it should be already assigned with the new
+    # attribute values.
+    def ensure_target_is_dirty
+      msg = "Target instance of #{target.class.name} must be dirty (must have changes to save)"
+      raise(msg) unless target.has_changes_to_save?
+    end
 
     def allowlisted?(user)
       user.try(:gitlab_employee?) || user.try(:gitlab_bot?) || user.try(:gitlab_service_user?)
     end
 
+    ##
+    # Performs the spam check using the spam verdict service, and modifies the target model
+    # accordingly based on the result.
     def perform_spam_service_check(api)
+      ensure_target_is_dirty
+
       # since we can check for spam, and recaptcha is not verified,
       # ask the SpamVerdictService what to do with the target.
       spam_verdict_service.execute.tap do |result|
@@ -79,7 +125,7 @@ module Spam
           description: target.spam_description,
           source_ip: options[:ip_address],
           user_agent: options[:user_agent],
-          noteable_type: notable_type,
+          noteable_type: noteable_type,
           via_api: api
         }
       )
@@ -88,14 +134,19 @@ module Spam
     end
 
     def spam_verdict_service
+      context = {
+        action: action,
+        target_type: noteable_type
+      }
+
       SpamVerdictService.new(target: target,
                              user: user,
-                             request: @request,
+                             request: request,
                              options: options,
-                             context: context.merge(target_type: notable_type))
+                             context: context)
     end
 
-    def notable_type
+    def noteable_type
       @notable_type ||= target.class.to_s
     end
   end
diff --git a/app/services/spam/spam_params.rb b/app/services/spam/spam_params.rb
new file mode 100644
index 00000000000..fef5355c7f3
--- /dev/null
+++ b/app/services/spam/spam_params.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module Spam
+  ##
+  # This class is a Parameter Object (https://refactoring.com/catalog/introduceParameterObject.html)
+  # which acts as an container abstraction for multiple parameter values related to spam and
+  # captcha processing for a request.
+  #
+  # Values contained are:
+  #
+  # api: A boolean flag indicating if the request was submitted via the REST or GraphQL API
+  # captcha_response: The response resulting from the user solving a captcha.  Currently it is
+  #   a scalar reCAPTCHA response string, but it can be expanded to an object in the future to
+  #   support other captcha implementations such as FriendlyCaptcha.
+  # spam_log_id: The id of a SpamLog record.
+  class SpamParams
+    attr_reader :api, :captcha_response, :spam_log_id
+
+    def initialize(api:, captcha_response:, spam_log_id:)
+      @api = api.present?
+      @captcha_response = captcha_response
+      @spam_log_id = spam_log_id
+    end
+
+    def ==(other)
+      other.class == self.class &&
+        other.api == self.api &&
+        other.captcha_response == self.captcha_response &&
+        other.spam_log_id == self.spam_log_id
+    end
+  end
+end