Merge branch 'caw-refactor-spam-services-apis' into 'master'

Refactoring and simplification of spam-related services [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!58603

app/controllers/boards/issues_controller.rb

@@ -136,7 +136,7 @@ module Boards
     def issue_params
       params.require(:issue)
         .permit(:title, :milestone_id, :project_id)
-        .merge(board_id: params[:board_id], list_id: params[:list_id], request: request)
+        .merge(board_id: params[:board_id], list_id: params[:list_id])
     end
 
     def serializer

app/controllers/concerns/spammable_actions.rb

@@ -47,31 +47,20 @@ module SpammableActions
     end
   end
 
-  def spammable_params
-    # NOTE: For the legacy reCAPTCHA implementation based on the HTML/HAML form, the
-    # 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the
-    # recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form.
-    #
-    # It is used in the `Recaptcha::Verify#verify_recaptcha` to extract the value from `params`,
-    # if the `response` option is not passed explicitly.
-    #
-    # Instead of relying on this behavior, we are extracting and passing it explicitly. This will
-    # make it consistent with the newer, modern reCAPTCHA verification process as it will be
-    # implemented via the GraphQL API and in Vue components via the native reCAPTCHA Javascript API,
-    # which requires that the recaptcha response param be obtained and passed explicitly.
+  # TODO: This method is currently only needed for issue create and update. It can be removed when:
   #
-    # It can also 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
-
-    # After this newer GraphQL/JS API process is fully supported by the backend, we can remove the
-    # check for the 'g-recaptcha-response' field and other HTML/HAML form-specific support.
-    captcha_response = params['g-recaptcha-response'] || params[:captcha_response]
-
-    {
-      request: request,
-      spam_log_id: params[:spam_log_id],
-      captcha_response: captcha_response
-    }
+  # 1. Issue create is is converted to a client/JS based approach instead of the legacy HAML
+  #    `_recaptcha_form.html.haml` which is rendered via the `projects/issues/verify` template.
+  #    In this case, which is based on the legacy reCAPTCHA implementation using the HTML/HAML form,
+  #    the 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the
+  #    recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form.
+  # 2. Issue update is converted to use the headers-based approach, which will require adding
+  #    support to captcha_modal_axios_interceptor.js like we have already added to
+  #    apollo_captcha_link.js.
+  #    In this case, the `captcha_response` field name comes from our captcha_modal_axios_interceptor.js.
+  def extract_legacy_spam_params_to_headers
+    request.headers['X-GitLab-Captcha-Response'] = params['g-recaptcha-response'] || params[:captcha_response]
+    request.headers['X-GitLab-Spam-Log-Id'] = params[:spam_log_id]
   end
 
   def spammable

app/controllers/projects/issues_controller.rb

@@ -129,12 +129,14 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def create
-    create_params = issue_params.merge(spammable_params).merge(
+    extract_legacy_spam_params_to_headers
+    create_params = issue_params.merge(
       merge_request_to_resolve_discussions_of: params[:merge_request_to_resolve_discussions_of],
       discussion_to_resolve: params[:discussion_to_resolve]
     )
 
-    service = ::Issues::CreateService.new(project: project, current_user: current_user, params: create_params)
+    spam_params = ::Spam::SpamParams.new_from_request(request: request)
+    service = ::Issues::CreateService.new(project: project, current_user: current_user, params: create_params, spam_params: spam_params)
     @issue = service.execute
 
     create_vulnerability_issue_feedback(issue)
@@ -334,8 +336,9 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def update_service
-    update_params = issue_params.merge(spammable_params)
-    ::Issues::UpdateService.new(project: project, current_user: current_user, params: update_params)
+    extract_legacy_spam_params_to_headers
+    spam_params = ::Spam::SpamParams.new_from_request(request: request)
+    ::Issues::UpdateService.new(project: project, current_user: current_user, params: issue_params, spam_params: spam_params)
   end
 
   def finder_type

app/graphql/mutations/concerns/mutations/spam_protection.rb

@@ -16,25 +16,6 @@ module Mutations
 
     private
 
-    # additional_spam_params    -> hash
-    #
-    # Used from a spammable mutation's #resolve method to generate
-    # the required additional spam/CAPTCHA params which must be merged into the params
-    # passed to the constructor of a service, where they can then be used in the service
-    # to perform spam checking via SpamActionService.
-    #
-    # Also accesses the #context of the mutation's Resolver superclass to obtain the request.
-    #
-    # Example:
-    #
-    # existing_args.merge!(additional_spam_params)
-    def additional_spam_params
-      {
-        api: true,
-        request: context[:request]
-      }
-    end
-
     def spam_action_response(object)
       fields = spam_action_response_fields(object)
 

app/graphql/mutations/issues/create.rb

@@ -73,7 +73,8 @@ module Mutations
         project = authorized_find!(project_path)
         params = build_create_issue_params(attributes.merge(author_id: current_user.id))
 
-        issue = ::Issues::CreateService.new(project: project, current_user: current_user, params: params).execute
+        spam_params = ::Spam::SpamParams.new_from_request(request: context[:request])
+        issue = ::Issues::CreateService.new(project: project, current_user: current_user, params: params, spam_params: spam_params).execute
 
         if issue.spam?
           issue.errors.add(:base, 'Spam detected.')

app/graphql/mutations/issues/set_confidential.rb

@@ -13,8 +13,11 @@ module Mutations
       def resolve(project_path:, iid:, confidential:)
         issue = authorized_find!(project_path: project_path, iid: iid)
         project = issue.project
+        # Changing confidentiality affects spam checking rules, therefore we need to provide
+        # spam_params so a check can be performed.
+        spam_params = ::Spam::SpamParams.new_from_request(request: context[:request])
 
-        ::Issues::UpdateService.new(project: project, current_user: current_user, params: { confidential: confidential })
+        ::Issues::UpdateService.new(project: project, current_user: current_user, params: { confidential: confidential }, spam_params: spam_params)
           .execute(issue)
 
         {

app/graphql/mutations/issues/update.rb

@@ -31,7 +31,8 @@ module Mutations
         issue = authorized_find!(project_path: project_path, iid: iid)
         project = issue.project
 
-        ::Issues::UpdateService.new(project: project, current_user: current_user, params: args).execute(issue)
+        spam_params = ::Spam::SpamParams.new_from_request(request: context[:request])
+        ::Issues::UpdateService.new(project: project, current_user: current_user, params: args, spam_params: spam_params).execute(issue)
 
         {
           issue: issue,

app/graphql/mutations/snippets/create.rb

@@ -49,7 +49,9 @@ module Mutations
 
         process_args_for_params!(args)
 
-        service_response = ::Snippets::CreateService.new(project: project, current_user: current_user, params: args).execute
+        spam_params = ::Spam::SpamParams.new_from_request(request: context[:request])
+        service = ::Snippets::CreateService.new(project: project, current_user: current_user, params: args, spam_params: spam_params)
+        service_response = service.execute
 
         # Only when the user is not an api user and the operation was successful
         if !api_user? && service_response.success?
@@ -81,12 +83,6 @@ module Mutations
         # it's the expected key param
         args[:files] = args.delete(:uploaded_files)
 
-        if Feature.enabled?(:snippet_spam)
-          args.merge!(additional_spam_params)
-        else
-          args[:disable_spam_action_service] = true
-        end
-
         # Return nil to make it explicit that this method is mutating the args parameter, and that
         # the return value is not relevant and is not to be used.
         nil

app/graphql/mutations/snippets/update.rb

@@ -34,7 +34,9 @@ module Mutations
 
         process_args_for_params!(args)
 
-        service_response = ::Snippets::UpdateService.new(project: snippet.project, current_user: current_user, params: args).execute(snippet)
+        spam_params = ::Spam::SpamParams.new_from_request(request: context[:request])
+        service = ::Snippets::UpdateService.new(project: snippet.project, current_user: current_user, params: args, spam_params: spam_params)
+        service_response = service.execute(snippet)
 
         # TODO: DRY this up - From here down, this is all duplicated with Mutations::Snippets::Create#resolve, except for
         #    `snippet.reset`, which is required in order to return the object in its non-dirty, unmodified, database state
@@ -62,12 +64,6 @@ module Mutations
       def process_args_for_params!(args)
         convert_blob_actions_to_snippet_actions!(args)
 
-        if Feature.enabled?(:snippet_spam)
-          args.merge!(additional_spam_params)
-        else
-          args[:disable_spam_action_service] = true
-        end
-
         # Return nil to make it explicit that this method is mutating the args parameter, and that
         # the return value is not relevant and is not to be used.
         nil

app/services/boards/issues/create_service.rb

@@ -30,7 +30,9 @@ module Boards
       end
 
       def create_issue(params)
-        ::Issues::CreateService.new(project: project, current_user: current_user, params: params).execute
+        # NOTE: We are intentionally not doing a spam/CAPTCHA check for issues created via boards.
+        # See https://gitlab.com/gitlab-org/gitlab/-/issues/29400#note_598479184 for more context.
+        ::Issues::CreateService.new(project: project, current_user: current_user, params: params, spam_params: nil).execute
       end
     end
   end

app/services/captcha/captcha_verification_service.rb

@@ -7,20 +7,27 @@ module Captcha
   class CaptchaVerificationService
     include Recaptcha::Verify
 
+    # Currently the only value that is used out of the request by the reCAPTCHA library
+    # is 'remote_ip'. Therefore, we just create a struct to avoid passing the full request
+    # object through all the service layer objects, and instead just rely on passing only
+    # the required remote_ip value. This eliminates the need to couple the service layer
+    # to the HTTP request (for the purpose of this service, at least).
+    RequestStruct = Struct.new(:remote_ip)
+
+    def initialize(spam_params:)
+      @spam_params = spam_params
+    end
+
     ##
     # Performs verification of a captcha response.
     #
-    # 'captcha_response' parameter is the response from the user solving a client-side captcha.
-    #
-    # 'request' parameter is the request which submitted the captcha.
-    #
     # NOTE: Currently only supports reCAPTCHA, and is not yet used in all places of the app in which
     # captchas are verified, but these can be addressed in future MRs.  See:
     # https://gitlab.com/gitlab-org/gitlab/-/issues/273480
-    def execute(captcha_response: nil, request:)
-      return false unless captcha_response
+    def execute
+      return false unless spam_params.captcha_response
 
-      @request = request
+      @request = RequestStruct.new(spam_params.ip_address)
 
       Gitlab::Recaptcha.load_configurations!
 
@@ -31,11 +38,13 @@ module Captcha
       # 2. We want control over the wording and i18n of the message
       # 3. We want a consistent interface and behavior when adding support for other captcha
       #    libraries which may not support automatically adding errors to the model.
-      verify_recaptcha(response: captcha_response)
+      verify_recaptcha(response: spam_params.captcha_response)
     end
 
     private
 
+    attr_reader :spam_params
+
     # The recaptcha library's Recaptcha::Verify#verify_recaptcha method requires that
     # 'request' be a readable attribute - it doesn't support passing it as an options argument.
     attr_reader :request

app/services/incident_management/incidents/create_service.rb

@@ -21,7 +21,8 @@ module IncidentManagement
             title: title,
             description: description,
             issue_type: ISSUE_TYPE
-          }
+          },
+          spam_params: nil
         ).execute
 
         return error(issue.errors.full_messages.to_sentence, issue) unless issue.valid?

app/services/issuable/import_csv/base_service.rb

@@ -68,7 +68,10 @@ module Issuable
       end
 
       def create_issuable(attributes)
-        create_issuable_class.new(project: @project, current_user: @user, params: attributes).execute
+        # NOTE: CSV imports are performed by workers, so we do not have a request context in order
+        # to create a SpamParams object to pass to the issuable create service.
+        spam_params = nil
+        create_issuable_class.new(project: @project, current_user: @user, params: attributes, spam_params: spam_params).execute
       end
 
       def email_results_to_user

app/services/issues/clone_service.rb

@@ -55,9 +55,13 @@ module Issues
 
       new_params = original_entity.serializable_hash.symbolize_keys.merge(new_params)
 
+      # spam checking is not necessary, as no new content is being created. Passing nil for
+      # spam_params will cause SpamActionService to skip checking and return a success response.
+      spam_params = nil
+
       # Skip creation of system notes for existing attributes of the issue. The system notes of the old
       # issue are copied over so we don't want to end up with duplicate notes.
-      CreateService.new(project: target_project, current_user: current_user, params: new_params).execute(skip_system_notes: true)
+      CreateService.new(project: target_project, current_user: current_user, params: new_params, spam_params: spam_params).execute(skip_system_notes: true)
     end
 
     def queue_copy_designs

app/services/issues/create_service.rb

@@ -4,10 +4,21 @@ module Issues
   class CreateService < Issues::BaseService
     include ResolveDiscussions
 
-    def execute(skip_system_notes: false)
-      @request = params.delete(:request)
-      @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
+    # NOTE: For Issues::CreateService, we require the spam_params and do not default it to nil, because
+    # spam_checking is likely to be necessary.  However, if there is not a request available in scope
+    # in the caller (for example, an issue created via email) and the required arguments to the
+    # SpamParams constructor are not otherwise available, spam_params: must be explicitly passed as nil.
+    def initialize(project:, current_user: nil, params: {}, spam_params:)
+      # Temporary check to ensure we are no longer passing request in params now that we have
+      # introduced spam_params. Raise an exception if it is present.
+      # Remove after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/58603 is complete.
+      raise if params[:request]
+
+      super(project: project, current_user: current_user, params: params)
+      @spam_params = spam_params
+    end
 
+    def execute(skip_system_notes: false)
       @issue = BuildService.new(project: project, current_user: current_user, params: params).execute
 
       filter_resolve_discussion_params
@@ -18,10 +29,10 @@ module Issues
     def before_create(issue)
       Spam::SpamActionService.new(
         spammable: issue,
-        request: request,
+        spam_params: spam_params,
         user: current_user,
         action: :create
-      ).execute(spam_params: spam_params)
+      ).execute
 
       # current_user (defined in BaseService) is not available within run_after_commit block
       user = current_user
@@ -64,10 +75,10 @@ module Issues
 
     private
 
-    attr_reader :request, :spam_params
+    attr_reader :spam_params
 
     def user_agent_detail_service
-      UserAgentDetailService.new(@issue, request)
+      UserAgentDetailService.new(spammable: @issue, spam_params: spam_params)
     end
   end
 end

app/services/issues/duplicate_service.rb

@@ -28,6 +28,7 @@ module Issues
 
     def relate_two_issues(duplicate_issue, canonical_issue)
       params = { target_issuable: canonical_issue }
+
       IssueLinks::CreateService.new(duplicate_issue, current_user, params).execute
     end
   end

app/services/issues/move_service.rb

@@ -58,10 +58,13 @@ module Issues
       }
 
       new_params = original_entity.serializable_hash.symbolize_keys.merge(new_params)
+      # spam checking is not necessary, as no new content is being created. Passing nil for
+      # spam_params will cause SpamActionService to skip checking and return a success response.
+      spam_params = nil
 
       # Skip creation of system notes for existing attributes of the issue. The system notes of the old
       # issue are copied over so we don't want to end up with duplicate notes.
-      CreateService.new(project: @target_project, current_user: @current_user, params: new_params).execute(skip_system_notes: true)
+      CreateService.new(project: @target_project, current_user: @current_user, params: new_params, spam_params: spam_params).execute(skip_system_notes: true)
     end
 
     def queue_copy_designs

app/services/issues/update_service.rb

@@ -4,12 +4,17 @@ module Issues
   class UpdateService < Issues::BaseService
     extend ::Gitlab::Utils::Override
 
+    # NOTE: For Issues::UpdateService, we default the spam_params to nil, because spam_checking is not
+    # necessary in many cases, and we don't want to require every caller to explicitly pass it as nil
+    # to disable spam checking.
+    def initialize(project:, current_user: nil, params: {}, spam_params: nil)
+      super(project: project, current_user: current_user, params: params)
+      @spam_params = spam_params
+    end
+
     def execute(issue)
       handle_move_between_ids(issue)
 
-      @request = params.delete(:request)
-      @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
-
       change_issue_duplicate(issue)
       move_issue_to_new_project(issue) || clone_issue(issue) || update_task_event(issue) || update(issue)
     end
@@ -25,10 +30,10 @@ module Issues
 
       Spam::SpamActionService.new(
         spammable: issue,
-        request: request,
+        spam_params: spam_params,
         user: current_user,
         action: :update
-      ).execute(spam_params: spam_params)
+      ).execute
     end
 
     def handle_changes(issue, options)
@@ -127,7 +132,7 @@ module Issues
 
     private
 
-    attr_reader :request, :spam_params
+    attr_reader :spam_params
 
     def clone_issue(issue)
       target_project = params.delete(:target_clone_project)

app/services/snippets/create_service.rb

@@ -2,12 +2,14 @@
 
 module Snippets
   class CreateService < Snippets::BaseService
-    def execute
-      # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed.
-      disable_spam_action_service = params.delete(:disable_spam_action_service) == true
-      @request = params.delete(:request)
-      @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
+    # NOTE: For Issues::CreateService, we require the spam_params and do not default it to nil, because
+    # spam_checking is likely to be necessary.
+    def initialize(project:, current_user: nil, params: {}, spam_params:)
+      super(project: project, current_user: current_user, params: params)
+      @spam_params = spam_params
+    end
 
+    def execute
       @snippet = build_from_params
 
       return invalid_params_error(@snippet) unless valid_params?
@@ -18,17 +20,17 @@ module Snippets
 
       @snippet.author = current_user
 
-      unless disable_spam_action_service
+      if Feature.enabled?(:snippet_spam)
         Spam::SpamActionService.new(
           spammable: @snippet,
-          request: request,
+          spam_params: spam_params,
           user: current_user,
           action: :create
-        ).execute(spam_params: spam_params)
+        ).execute
       end
 
       if save_and_commit
-        UserAgentDetailService.new(@snippet, request).create
+        UserAgentDetailService.new(spammable: @snippet, spam_params: spam_params).create
         Gitlab::UsageDataCounters::SnippetCounter.count(:create)
 
         move_temporary_files
@@ -41,7 +43,7 @@ module Snippets
 
     private
 
-    attr_reader :snippet, :request, :spam_params
+    attr_reader :snippet, :spam_params
 
     def build_from_params
       if project

app/services/snippets/update_service.rb

@@ -6,12 +6,15 @@ module Snippets
 
     UpdateError = Class.new(StandardError)
 
-    def execute(snippet)
-      # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed.
-      disable_spam_action_service = params.delete(:disable_spam_action_service) == true
-      @request = params.delete(:request)
-      @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
+    # NOTE: For Snippets::UpdateService, we default the spam_params to nil, because spam_checking is not
+    # necessary in many cases, and we don't want every caller to have to explicitly pass it as nil
+    # to disable spam checking.
+    def initialize(project:, current_user: nil, params: {}, spam_params: nil)
+      super(project: project, current_user: current_user, params: params)
+      @spam_params = spam_params
+    end
 
+    def execute(snippet)
       return invalid_params_error(snippet) unless valid_params?
 
       if visibility_changed?(snippet) && !visibility_allowed?(visibility_level)
@@ -20,13 +23,13 @@ module Snippets
 
       update_snippet_attributes(snippet)
 
-      unless disable_spam_action_service
+      if Feature.enabled?(:snippet_spam)
         Spam::SpamActionService.new(
           spammable: snippet,
-          request: request,
+          spam_params: spam_params,
           user: current_user,
           action: :update
-        ).execute(spam_params: spam_params)
+        ).execute
       end
 
       if save_and_commit(snippet)
@@ -40,7 +43,7 @@ module Snippets
 
     private
 
-    attr_reader :request, :spam_params
+    attr_reader :spam_params
 
     def visibility_changed?(snippet)
       visibility_level && visibility_level.to_i != snippet.visibility_level

app/services/spam/akismet_service.rb

@@ -20,6 +20,7 @@ module Spam
         created_at: DateTime.current,
         author: owner_name,
         author_email: owner_email,
+        # NOTE: The akismet_client needs the option to be named `:referrer`, not `:referer`
         referrer: options[:referer]
       }
 

app/services/spam/spam_action_service.rb

@@ -4,67 +4,22 @@ 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). The parameters will either be camelCase (if they are
-    # received directly via controller params) or underscore_case (if they have come from
-    # a GraphQL mutation which has converted them to underscore), or in the
-    # headers when using the header based flow.
-    #
-    # 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, request)
-      # 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
-      headers = request&.headers || {}
-      api = params.delete(:api)
-      captcha_response = read_parameter(:captcha_response, params, headers)
-      spam_log_id      = read_parameter(:spam_log_id, params, headers)&.to_i
-
-      SpamParams.new(api: api, captcha_response: captcha_response, spam_log_id: spam_log_id)
-    end
-
-    def self.read_parameter(name, params, headers)
-      [
-        params.delete(name),
-        params.delete(name.to_s.camelize(:lower).to_sym),
-        headers["X-GitLab-#{name.to_s.titlecase(keep_id_suffix: true).tr(' ', '-')}"]
-      ].compact.first
-    end
-
-    attr_accessor :target, :request, :options
-    attr_reader :spam_log
-
-    def initialize(spammable:, request:, user:, action:)
+    def initialize(spammable:, spam_params:, user:, action:)
       @target = spammable
-      @request = request
+      @spam_params = spam_params
       @user = user
       @action = action
-      @options = {}
     end
 
     # rubocop:disable Metrics/AbcSize
-    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[:referer] = request.env['HTTP_REFERER']
-      else
-        # 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
+    def execute
+      # If spam_params is passed as `nil`, no check will be performed. This is the easiest way to allow
+      # composed services which may not need to do spam checking to "opt out".  For example, when
+      # MoveService is calling CreateService, spam checking is not necessary, as no new content is
+      # being created.
+      return ServiceResponse.success(message: 'Skipped spam check because spam_params was not present') unless spam_params
 
-      recaptcha_verified = Captcha::CaptchaVerificationService.new.execute(
-        captcha_response: spam_params.captcha_response,
-        request: request
-      )
+      recaptcha_verified = Captcha::CaptchaVerificationService.new(spam_params: spam_params).execute
 
       if recaptcha_verified
         # If it's a request which is already verified through CAPTCHA,
@@ -73,10 +28,9 @@ module Spam
         ServiceResponse.success(message: "CAPTCHA successfully verified")
       else
         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(spam_params.api)
+        perform_spam_service_check
         ServiceResponse.success(message: "Spam check performed. Check #{target.class.name} spammable model for any errors or CAPTCHA requirement")
       end
     end
@@ -86,7 +40,7 @@ module Spam
 
     private
 
-    attr_reader :user, :action
+    attr_reader :user, :action, :target, :spam_params, :spam_log
 
     ##
     # In order to be proceed to the spam check process, the target must be
@@ -104,7 +58,7 @@ module Spam
     ##
     # 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)
+    def perform_spam_service_check
       ensure_target_is_dirty
 
       # since we can check for spam, and recaptcha is not verified,
@@ -113,7 +67,7 @@ module Spam
         case result
         when CONDITIONAL_ALLOW
           # at the moment, this means "ask for reCAPTCHA"
-          create_spam_log(api)
+          create_spam_log
 
           break if target.allow_possible_spam?
 
@@ -122,12 +76,12 @@ module Spam
           # TODO: remove `unless target.allow_possible_spam?` once this flag has been passed to `SpamVerdictService`
           # https://gitlab.com/gitlab-org/gitlab/-/issues/214739
           target.spam! unless target.allow_possible_spam?
-          create_spam_log(api)
+          create_spam_log
         when BLOCK_USER
           # TODO: improve BLOCK_USER handling, non-existent until now
           # https://gitlab.com/gitlab-org/gitlab/-/issues/329666
           target.spam! unless target.allow_possible_spam?
-          create_spam_log(api)
+          create_spam_log
         when ALLOW
           target.clear_spam_flags!
         when NOOP
@@ -137,16 +91,21 @@ module Spam
       end
     end
 
-    def create_spam_log(api)
+    def create_spam_log
       @spam_log = SpamLog.create!(
         {
           user_id: target.author_id,
           title: target.spam_title,
           description: target.spam_description,
-          source_ip: options[:ip_address],
-          user_agent: options[:user_agent],
+          source_ip: spam_params.ip_address,
+          user_agent: spam_params.user_agent,
           noteable_type: noteable_type,
-          via_api: api
+          # Now, all requests are via the API, so hardcode it to true to simplify the logic and API
+          # of this service.  See https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2266
+          # for original introduction of `via_api` field.
+          # See discussion here about possibly deprecating this field:
+          # https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2266#note_542527450
+          via_api: true
         }
       )
 
@@ -159,9 +118,14 @@ module Spam
         target_type: noteable_type
       }
 
+      options = {
+        ip_address: spam_params.ip_address,
+        user_agent: spam_params.user_agent,
+        referer: spam_params.referer
+      }
+
       SpamVerdictService.new(target: target,
                              user: user,
-                             request: request,
                              options: options,
                              context: context)
     end

app/services/spam/spam_params.rb

@@ -3,30 +3,54 @@
 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.
+  # which acts as an container abstraction for multiple values related to spam and
+  # captcha processing for a provided HTTP request object.
+  #
+  # It is used to encapsulate these values and allow them to be passed from the Controller/GraphQL
+  # layers down into to the Service layer, without needing to pass the entire request and therefore
+  # unnecessarily couple the Service layer to the HTTP 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.
+  #   support other captcha implementations such as FriendlyCaptcha. Obtained from
+  #   request.headers['X-GitLab-Captcha-Response']
+  # spam_log_id: The id of a SpamLog record. Obtained from request.headers['X-GitLab-Spam-Log-Id']
+  # ip_address = The remote IP. Obtained from request.env['action_dispatch.remote_ip']
+  # user_agent = The user agent. Obtained from request.env['HTTP_USER_AGENT']
+  # referer = The HTTP referer. Obtained from request.env['HTTP_REFERER']
+  #
+  # NOTE: The presence of these values in the request is not currently enforced. If they are missing,
+  #      then the spam check may fail, or the SpamLog or UserAgentDetail may have missing fields.
   class SpamParams
-    attr_reader :api, :captcha_response, :spam_log_id
+    def self.new_from_request(request:)
+      self.new(
+        captcha_response: request.headers['X-GitLab-Captcha-Response'],
+        spam_log_id: request.headers['X-GitLab-Spam-Log-Id'],
+        ip_address: request.env['action_dispatch.remote_ip'].to_s,
+        user_agent: request.env['HTTP_USER_AGENT'],
+        referer: request.env['HTTP_REFERER']
+      )
+    end
+
+    attr_reader :captcha_response, :spam_log_id, :ip_address, :user_agent, :referer
 
-    def initialize(api:, captcha_response:, spam_log_id:)
-      @api = api.present?
+    def initialize(captcha_response:, spam_log_id:, ip_address:, user_agent:, referer:)
       @captcha_response = captcha_response
       @spam_log_id = spam_log_id
+      @ip_address = ip_address
+      @user_agent = user_agent
+      @referer = referer
     end
 
     def ==(other)
       other.class <= self.class &&
-        other.api == api &&
         other.captcha_response == captcha_response &&
-        other.spam_log_id == spam_log_id
+        other.spam_log_id == spam_log_id &&
+        other.ip_address == ip_address &&
+        other.user_agent == user_agent &&
+        other.referer == referer
     end
   end
 end

app/services/spam/spam_verdict_service.rb

@@ -5,9 +5,8 @@ module Spam
     include AkismetMethods
     include SpamConstants
 
-    def initialize(user:, target:, request:, options:, context: {})
+    def initialize(user:, target:, options:, context: {})
       @target = target
-      @request = request
       @user = user
       @options = options
       @context = context
@@ -59,7 +58,7 @@ module Spam
 
     private
 
-    attr_reader :user, :target, :request, :options, :context
+    attr_reader :user, :target, :options, :context
 
     def akismet_verdict
       if akismet.spam?

app/services/user_agent_detail_service.rb

 # frozen_string_literal: true
 
 class UserAgentDetailService
-  attr_accessor :spammable, :request
-
-  def initialize(spammable, request)
+  def initialize(spammable:, spam_params:)
     @spammable = spammable
-    @request = request
+    @spam_params = spam_params
   end
 
   def create
-    return unless request
+    unless spam_params&.user_agent && spam_params&.ip_address
+      messasge = 'Skipped UserAgentDetail creation because necessary spam_params were not provided'
+      return ServiceResponse.success(message: messasge)
+    end
 
-    spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s)
+    spammable.create_user_agent_detail(user_agent: spam_params.user_agent, ip_address: spam_params.ip_address)
   end
+
+  private
+
+  attr_reader :spammable, :spam_params
 end

ee/app/services/issues/create_from_vulnerability_data_service.rb

@@ -17,7 +17,8 @@ module Issues
         confidential: true
       }
 
-      issue = Issues::CreateService.new(project: @project, current_user: @current_user, params: issue_params).execute
+      # NOTE: Intentionally not performing spam check, for now.
+      issue = Issues::CreateService.new(project: @project, current_user: @current_user, params: issue_params, spam_params: nil).execute
 
       if issue.valid?
         success(issue)

ee/app/services/quality_management/test_cases/create_service.rb

@@ -22,7 +22,8 @@ module QualityManagement
             title: title,
             description: description,
             label_ids: label_ids
-          }
+          },
+          spam_params: nil
         ).execute
 
         return error(issue.errors.full_messages.to_sentence, issue) unless issue.valid?

ee/app/services/requirements_management/create_requirement_service.rb

@@ -4,6 +4,16 @@ module RequirementsManagement
   class CreateRequirementService < ::BaseProjectService
     include Gitlab::Allowable
 
+    # NOTE: Even though this class does not (yet) do spam checking, this constructor takes a
+    # spam_params named argument in order to be consistent with the other issuable service
+    # constructors. This is necessary in order for methods such as create_issuable to be able to
+    # work in a consistent way with all different issuable services.
+    # See https://gitlab.com/groups/gitlab-org/-/epics/5527#current-vulnerabilities
+    # for more context.
+    def initialize(project:, current_user: nil, params: {}, spam_params: nil)
+      super(project: project, current_user: current_user, params: params)
+    end
+
     def execute
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :create_requirement, project)
 

ee/db/fixtures/development/20_burndown.rb

@@ -57,7 +57,7 @@ class Gitlab::Seeder::Burndown
         weight: rand(1..9)
       }
 
-      Issues::CreateService.new(project: @project, current_user: @project.team.users.sample, params: issue_params).execute
+      Issues::CreateService.new(project: @project, current_user: @project.team.users.sample, params: issue_params, spam_params: nil).execute
     end
   end
 

ee/db/fixtures/development/30_customizable_cycle_analytics.rb

@@ -110,14 +110,16 @@ class Gitlab::Seeder::CustomizableCycleAnalytics
       Issues::UpdateService.new(
         project: project,
         current_user: user,
-        params: { label_ids: [in_dev_label.id] }
+        params: { label_ids: [in_dev_label.id] },
+        spam_params: nil
       ).execute(issue)
 
       Timecop.travel(random_duration_in_hours.hours.from_now)
       Issues::UpdateService.new(
         project: project,
         current_user: user,
-        params: { label_ids: [in_review_label.id] }
+        params: { label_ids: [in_review_label.id] },
+        spam_params: nil
       ).execute(issue)
     end
   end
@@ -153,7 +155,7 @@ class Gitlab::Seeder::CustomizableCycleAnalytics
         assignees: [project.team.users.sample]
       }
 
-      Issues::CreateService.new(project: @project, current_user: project.team.users.sample, params: issue_params).execute
+      Issues::CreateService.new(project: @project, current_user: project.team.users.sample, params: issue_params, spam_params: nil).execute
     end
   end
 

ee/db/fixtures/development/90_productivity_analytics.rb

@@ -52,7 +52,7 @@ class Gitlab::Seeder::ProductivityAnalytics
       }
 
       Timecop.travel rand(10).days.from_now do
-        Issues::CreateService.new(project: @project, current_user: @project.team.users.sample, params: issue_params).execute
+        Issues::CreateService.new(project: @project, current_user: @project.team.users.sample, params: issue_params, spam_params: nil).execute
       end
     end
   end

ee/spec/graphql/mutations/issues/create_spec.rb

@@ -44,6 +44,7 @@ RSpec.describe Mutations::Issues::Create do
       project.add_guest(assignee1)
       project.add_guest(assignee2)
       stub_licensed_features(issuable_health_status: true)
+      stub_spam_services
     end
 
     subject { mutation.resolve(**mutation_params) }

ee/spec/graphql/mutations/issues/update_spec.rb

@@ -5,6 +5,10 @@ require 'spec_helper'
 RSpec.describe Mutations::Issues::Update do
   let(:user) { create(:user) }
 
+  before do
+    stub_spam_services
+  end
+
   it_behaves_like 'updating health status' do
     let(:resource) { create(:issue) }
   end

ee/spec/services/ee/issues/create_service_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Issues::CreateService do
   let_it_be_with_reload(:project) { create(:project, group: group) }
 
   let(:params) { { title: 'Awesome issue', description: 'please fix', weight: 9 } }
-  let(:service) { described_class.new(project: project, current_user: user, params: params) }
+  let(:service) { described_class.new(project: project, current_user: user, params: params, spam_params: nil) }
 
   describe '#execute' do
     context 'when current user cannot admin issues in the project' do
@@ -110,7 +110,7 @@ RSpec.describe Issues::CreateService do
               confidential_epic = create(:epic, group: group, confidential: true)
               params = { title: 'confidential issue', epic_id: confidential_epic.id }
 
-              issue = described_class.new(project: project, current_user: user, params: params).execute
+              issue = described_class.new(project: project, current_user: user, params: params, spam_params: nil).execute
 
               expect(issue.confidential).to eq(true)
             end
@@ -120,7 +120,7 @@ RSpec.describe Issues::CreateService do
             it 'creates a confidential child issue' do
               params = { title: 'confidential issue', epic_id: epic.id, confidential: true }
 
-              issue = described_class.new(project: project, current_user: user, params: params).execute
+              issue = described_class.new(project: project, current_user: user, params: params, spam_params: nil).execute
 
               expect(issue.confidential).to eq(true)
             end

ee/spec/support/shared_examples/services/scoped_label_shared_examples.rb

@@ -28,9 +28,9 @@ RSpec.shared_examples 'new issuable with scoped labels' do
 
       context 'when using label_ids parameter' do
         it 'adds only last selected exclusive scoped label' do
-          issuable = described_class.new(
-            **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id] }
-          ).execute
+          args = { **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id] } }
+          args[:spam_params] = nil if described_class.private_instance_methods.include?(:spam_params)
+          issuable = described_class.new(**args).execute
 
           expect(issuable.labels).to match_array([label1, label2])
         end
@@ -38,9 +38,9 @@ RSpec.shared_examples 'new issuable with scoped labels' do
 
       context 'when using labels parameter' do
         it 'adds only last selected exclusive scoped label' do
-          issuable = described_class.new(
-            **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', labels: [label1.title, label3.title, label4.title, label2.title] }
-          ).execute
+          args = { **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', labels: [label1.title, label3.title, label4.title, label2.title] } }
+          args[:spam_params] = nil if described_class.private_instance_methods.include?(:spam_params)
+          issuable = described_class.new(**args).execute
 
           expect(issuable.labels).to match_array([label1, label2])
         end
@@ -58,9 +58,9 @@ RSpec.shared_examples 'new issuable with scoped labels' do
         label3 = create_label('key::label2')
         label4 = create_label('key::label3')
 
-        issuable = described_class.new(
-          **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id] }
-        ).execute
+        args = { **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id] } }
+        args[:spam_params] = nil if described_class.private_instance_methods.include?(:spam_params)
+        issuable = described_class.new(**args).execute
 
         expect(issuable.labels).to match_array([label1, label2, label3, label4])
       end

lib/api/helpers.rb

@@ -577,10 +577,6 @@ module API
       Gitlab::AppLogger.warn("Redis tracking event failed for event: #{event_name}, message: #{error.message}")
     end
 
-    def with_api_params(&block)
-      yield({ api: true, request: request })
-    end
-
     protected
 
     def project_finder_params_visibility_ce

lib/api/helpers/snippets_helpers.rb

@@ -72,22 +72,18 @@ module API
     end
 
     def process_create_params(args)
-      with_api_params do |api_params|
       args[:snippet_actions] = args.delete(:files)&.map do |file|
         file[:action] = :create
         file.symbolize_keys
       end
 
-        args.merge(api_params)
-      end
+      args
     end
 
     def process_update_params(args)
-      with_api_params do |api_params|
       args[:snippet_actions] = args.delete(:files)&.map(&:symbolize_keys)
 
-        args.merge(api_params)
-      end
+      args
     end
 
     def validate_params_for_multiple_files(snippet)

lib/api/issues.rb

@@ -255,9 +255,11 @@ module API
         issue_params = convert_parameters_from_legacy_format(issue_params)
 
         begin
+          spam_params = ::Spam::SpamParams.new_from_request(request: request)
           issue = ::Issues::CreateService.new(project: user_project,
                                               current_user: current_user,
-                                              params: issue_params.merge(request: request, api: true)).execute
+                                              params: issue_params,
+                                              spam_params: spam_params).execute
 
           if issue.spam?
             render_api_error!({ error: 'Spam detected' }, 400)
@@ -294,13 +296,15 @@ module API
         issue = user_project.issues.find_by!(iid: params.delete(:issue_iid))
         authorize! :update_issue, issue
 
-        update_params = declared_params(include_missing: false).merge(request: request, api: true)
+        update_params = declared_params(include_missing: false)
 
         update_params = convert_parameters_from_legacy_format(update_params)
 
+        spam_params = ::Spam::SpamParams.new_from_request(request: request)
         issue = ::Issues::UpdateService.new(project: user_project,
                                             current_user: current_user,
-                                            params: update_params).execute(issue)
+                                            params: update_params,
+                                            spam_params: spam_params).execute(issue)
 
         render_spam_error! if issue.spam?
 

lib/api/project_snippets.rb

@@ -75,7 +75,8 @@ module API
 
         snippet_params = process_create_params(declared_params(include_missing: false))
 
-        service_response = ::Snippets::CreateService.new(project: user_project, current_user: current_user, params: snippet_params).execute
+        spam_params = ::Spam::SpamParams.new_from_request(request: request)
+        service_response = ::Snippets::CreateService.new(project: user_project, current_user: current_user, params: snippet_params, spam_params: spam_params).execute
         snippet = service_response.payload[:snippet]
 
         if service_response.success?
@@ -116,7 +117,8 @@ module API
 
         snippet_params = process_update_params(declared_params(include_missing: false))
 
-        service_response = ::Snippets::UpdateService.new(project: user_project, current_user: current_user, params: snippet_params).execute(snippet)
+        spam_params = ::Spam::SpamParams.new_from_request(request: request)
+        service_response = ::Snippets::UpdateService.new(project: user_project, current_user: current_user, params: snippet_params, spam_params: spam_params).execute(snippet)
         snippet = service_response.payload[:snippet]
 
         if service_response.success?

lib/api/snippets.rb

@@ -84,7 +84,8 @@ module API
 
         attrs = process_create_params(declared_params(include_missing: false))
 
-        service_response = ::Snippets::CreateService.new(project: nil, current_user: current_user, params: attrs).execute
+        spam_params = ::Spam::SpamParams.new_from_request(request: request)
+        service_response = ::Snippets::CreateService.new(project: nil, current_user: current_user, params: attrs, spam_params: spam_params).execute
         snippet = service_response.payload[:snippet]
 
         if service_response.success?
@@ -126,7 +127,8 @@ module API
 
         attrs = process_update_params(declared_params(include_missing: false))
 
-        service_response = ::Snippets::UpdateService.new(project: nil, current_user: current_user, params: attrs).execute(snippet)
+        spam_params = ::Spam::SpamParams.new_from_request(request: request)
+        service_response = ::Snippets::UpdateService.new(project: nil, current_user: current_user, params: attrs, spam_params: spam_params).execute(snippet)
 
         snippet = service_response.payload[:snippet]
 

lib/gitlab/email/handler/create_issue_handler.rb

@@ -61,7 +61,8 @@ module Gitlab
             params: {
               title: mail.subject,
               description: message_including_reply
-            }
+            },
+            spam_params: nil
           ).execute
         end
 

lib/gitlab/email/handler/service_desk_handler.rb

@@ -83,7 +83,8 @@ module Gitlab
               description: message_including_template,
               confidential: true,
               external_author: from_address
-            }
+            },
+            spam_params: nil
           ).execute
 
           raise InvalidIssueError unless @issue.persisted?

lib/gitlab/slash_commands/issue_new.rb

@@ -33,7 +33,7 @@ module Gitlab
       private
 
       def create_issue(title:, description:)
-        Issues::CreateService.new(project: project, current_user: current_user, params: { title: title, description: description }).execute
+        Issues::CreateService.new(project: project, current_user: current_user, params: { title: title, description: description }, spam_params: nil).execute
       end
 
       def presenter(issue)

lib/quality/seeders/issues.rb

@@ -30,7 +30,7 @@ module Quality
               labels: labels.join(',')
             }
             params[:closed_at] = params[:created_at] + rand(35).days if params[:state] == 'closed'
-            issue = ::Issues::CreateService.new(project: project, current_user: team.sample, params: params).execute
+            issue = ::Issues::CreateService.new(project: project, current_user: team.sample, params: params, spam_params: nil).execute
 
             if issue.persisted?
               created_issues_count += 1

spec/features/calendar_spec.rb

@@ -145,7 +145,7 @@ RSpec.describe 'Contributions Calendar', :js do
 
     describe '1 issue creation calendar activity' do
       before do
-        Issues::CreateService.new(project: contributed_project, current_user: user, params: issue_params).execute
+        Issues::CreateService.new(project: contributed_project, current_user: user, params: issue_params, spam_params: nil).execute
       end
 
       it_behaves_like 'a day with activity', contribution_count: 1
@@ -180,7 +180,7 @@ RSpec.describe 'Contributions Calendar', :js do
         push_code_contribution
 
         travel_to(Date.yesterday) do
-          Issues::CreateService.new(project: contributed_project, current_user: user, params: issue_params).execute
+          Issues::CreateService.new(project: contributed_project, current_user: user, params: issue_params, spam_params: nil).execute
         end
       end
       include_context 'visit user page'

spec/features/unsubscribe_links_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe 'Unsubscribe links', :sidekiq_might_not_need_inline do
   let(:author) { create(:user) }
   let(:project) { create(:project, :public) }
   let(:params) { { title: 'A bug!', description: 'Fix it!', assignees: [recipient] } }
-  let(:issue) { Issues::CreateService.new(project: project, current_user: author, params: params).execute }
+  let(:issue) { Issues::CreateService.new(project: project, current_user: author, params: params, spam_params: nil).execute }
 
   let(:mail) { ActionMailer::Base.deliveries.last }
   let(:body) { Capybara::Node::Simple.new(mail.default_part_body.to_s) }

spec/features/users/user_browses_projects_on_user_page_spec.rb

@@ -125,7 +125,7 @@ RSpec.describe 'Users > User browses projects on user page', :js do
         end
 
         before do
-          Issues::CreateService.new(project: contributed_project, current_user: user, params: { title: 'Bug in old browser' }).execute
+          Issues::CreateService.new(project: contributed_project, current_user: user, params: { title: 'Bug in old browser' }, spam_params: nil).execute
           event = create(:push_event, project: contributed_project, author: user)
           create(:push_event_payload, event: event, commit_count: 3)
         end

spec/graphql/mutations/issues/create_spec.rb

@@ -50,6 +50,7 @@ RSpec.describe Mutations::Issues::Create do
       stub_licensed_features(multiple_issue_assignees: false, issue_weights: false)
       project.add_guest(assignee1)
       project.add_guest(assignee2)
+      stub_spam_services
     end
 
     subject { mutation.resolve(**mutation_params) }

spec/graphql/mutations/issues/set_confidential_spec.rb

@@ -17,6 +17,10 @@ RSpec.describe Mutations::Issues::SetConfidential do
 
     subject { mutation.resolve(project_path: project.full_path, iid: issue.iid, confidential: confidential) }
 
+    before do
+      stub_spam_services
+    end
+
     it_behaves_like 'permission level for issue mutation is correctly verified'
 
     context 'when the user can update the issue' do

spec/graphql/mutations/issues/update_spec.rb

@@ -35,6 +35,10 @@ RSpec.describe Mutations::Issues::Update do
 
     subject { mutation.resolve(**mutation_params) }
 
+    before do
+      stub_spam_services
+    end
+
     it_behaves_like 'permission level for issue mutation is correctly verified'
 
     context 'when the user can update the issue' do

spec/models/integrations/microsoft_teams_spec.rb

@@ -74,7 +74,7 @@ RSpec.describe Integrations::MicrosoftTeams do
     context 'with issue events' do
       let(:opts) { { title: 'Awesome issue', description: 'please fix' } }
       let(:issues_sample_data) do
-        service = Issues::CreateService.new(project: project, current_user: user, params: opts)
+        service = Issues::CreateService.new(project: project, current_user: user, params: opts, spam_params: nil)
         issue = service.execute
         service.hook_data(issue, 'open')
       end

spec/requests/api/graphql/mutations/snippets/create_spec.rb

@@ -17,7 +17,6 @@ RSpec.describe 'Creating a Snippet' do
   let(:actions) { [{ action: action }.merge(file_1), { action: action }.merge(file_2)] }
   let(:project_path) { nil }
   let(:uploaded_files) { nil }
-  let(:spam_mutation_vars) { {} }
   let(:mutation_vars) do
     {
       description: description,
@@ -26,7 +25,7 @@ RSpec.describe 'Creating a Snippet' do
       project_path: project_path,
       uploaded_files: uploaded_files,
       blob_actions: actions
-    }.merge(spam_mutation_vars)
+    }
   end
 
   let(:mutation) do
@@ -77,21 +76,6 @@ RSpec.describe 'Creating a Snippet' do
 
         expect(mutation_response['snippet']).to be_nil
       end
-
-      context 'when snippet_spam flag is disabled' do
-        before do
-          stub_feature_flags(snippet_spam: false)
-        end
-
-        it 'passes disable_spam_action_service param to service' do
-          expect(::Snippets::CreateService)
-            .to receive(:new)
-                  .with(project: anything, current_user: anything, params: hash_including(disable_spam_action_service: true))
-                  .and_call_original
-
-          subject
-        end
-      end
     end
 
     shared_examples 'creates snippet' do
@@ -121,15 +105,6 @@ RSpec.describe 'Creating a Snippet' do
       it_behaves_like 'snippet edit usage data counters'
 
       it_behaves_like 'a mutation which can mutate a spammable' do
-        let(:captcha_response) { 'abc123' }
-        let(:spam_log_id) { 1234 }
-        let(:spam_mutation_vars) do
-          {
-            captcha_response: captcha_response,
-            spam_log_id: spam_log_id
-          }
-        end
-
         let(:service) { Snippets::CreateService }
       end
     end
@@ -190,7 +165,7 @@ RSpec.describe 'Creating a Snippet' do
 
         it do
           expect(::Snippets::CreateService).to receive(:new)
-            .with(project: nil, current_user: user, params: hash_including(files: expected_value))
+            .with(project: nil, current_user: user, params: hash_including(files: expected_value), spam_params: instance_of(::Spam::SpamParams))
             .and_return(double(execute: creation_response))
 
           subject

spec/requests/api/graphql/mutations/snippets/update_spec.rb

@@ -16,7 +16,6 @@ RSpec.describe 'Updating a Snippet' do
   let(:updated_file) { 'CHANGELOG' }
   let(:deleted_file) { 'README' }
   let(:snippet_gid) { GitlabSchema.id_from_object(snippet).to_s }
-  let(:spam_mutation_vars) { {} }
   let(:mutation_vars) do
     {
       id: snippet_gid,
@@ -27,7 +26,7 @@ RSpec.describe 'Updating a Snippet' do
         { action: :update, filePath: updated_file, content: updated_content },
         { action: :delete, filePath: deleted_file }
       ]
-    }.merge(spam_mutation_vars)
+    }
   end
 
   let(:mutation) do
@@ -82,21 +81,6 @@ RSpec.describe 'Updating a Snippet' do
         end
       end
 
-      context 'when snippet_spam flag is disabled' do
-        before do
-          stub_feature_flags(snippet_spam: false)
-        end
-
-        it 'passes disable_spam_action_service param to service' do
-          expect(::Snippets::UpdateService)
-            .to receive(:new)
-                  .with(project: anything, current_user: anything, params: hash_including(disable_spam_action_service: true))
-                  .and_call_original
-
-          subject
-        end
-      end
-
       context 'when there are ActiveRecord validation errors' do
         let(:updated_title) { '' }
 
@@ -125,15 +109,6 @@ RSpec.describe 'Updating a Snippet' do
       end
 
       it_behaves_like 'a mutation which can mutate a spammable' do
-        let(:captcha_response) { 'abc123' }
-        let(:spam_log_id) { 1234 }
-        let(:spam_mutation_vars) do
-          {
-            captcha_response: captcha_response,
-            spam_log_id: spam_log_id
-          }
-        end
-
         let(:service) { Snippets::UpdateService }
       end
 

spec/services/captcha/captcha_verification_service_spec.rb

@@ -4,21 +4,31 @@ require 'spec_helper'
 
 RSpec.describe Captcha::CaptchaVerificationService do
   describe '#execute' do
-    let(:captcha_response) { nil }
-    let(:request) { double(:request) }
-    let(:service) { described_class.new }
+    let(:captcha_response) { 'abc123' }
+    let(:fake_ip) { '1.2.3.4' }
+    let(:spam_params) do
+      ::Spam::SpamParams.new(
+        captcha_response: captcha_response,
+        spam_log_id: double,
+        ip_address: fake_ip,
+        user_agent: double,
+        referer: double
+      )
+    end
+
+    let(:service) { described_class.new(spam_params: spam_params) }
 
-    subject { service.execute(captcha_response: captcha_response, request: request) }
+    subject { service.execute }
 
     context 'when there is no captcha_response' do
+      let(:captcha_response) { nil }
+
       it 'returns false' do
         expect(subject).to eq(false)
       end
     end
 
     context 'when there is a captcha_response' do
-      let(:captcha_response) { 'abc123' }
-
       before do
         expect(Gitlab::Recaptcha).to receive(:load_configurations!)
       end
@@ -29,10 +39,12 @@ RSpec.describe Captcha::CaptchaVerificationService do
         expect(subject).to eq(true)
       end
 
-      it 'has a request method which returns the request' do
+      it 'has a request method which returns an object with the ip address #remote_ip' do
         subject
 
-        expect(service.send(:request)).to eq(request)
+        request_struct = service.send(:request)
+        expect(request_struct).to respond_to(:remote_ip)
+        expect(request_struct.remote_ip).to eq(fake_ip)
       end
     end
   end

spec/services/snippets/create_service_spec.rb

@@ -19,8 +19,9 @@ RSpec.describe Snippets::CreateService do
 
     let(:extra_opts) { {} }
     let(:creator) { admin }
+    let(:spam_params) { double }
 
-    subject { described_class.new(project: project, current_user: creator, params: opts).execute }
+    subject { described_class.new(project: project, current_user: creator, params: opts, spam_params: spam_params).execute }
 
     let(:snippet) { subject.payload[:snippet] }
 
@@ -301,6 +302,10 @@ RSpec.describe Snippets::CreateService do
       end
     end
 
+    before do
+      stub_spam_services
+    end
+
     context 'when ProjectSnippet' do
       let_it_be(:project) { create(:project) }
 

spec/services/snippets/update_service_spec.rb

@@ -20,7 +20,9 @@ RSpec.describe Snippets::UpdateService do
     let(:extra_opts) { {} }
     let(:options) { base_opts.merge(extra_opts) }
     let(:updater) { user }
-    let(:service) { Snippets::UpdateService.new(project: project, current_user: updater, params: options) }
+    let(:spam_params) { double }
+
+    let(:service) { Snippets::UpdateService.new(project: project, current_user: updater, params: options, spam_params: spam_params) }
 
     subject { service.execute(snippet) }
 
@@ -721,6 +723,10 @@ RSpec.describe Snippets::UpdateService do
       end
     end
 
+    before do
+      stub_spam_services
+    end
+
     context 'when Project Snippet' do
       let_it_be(:project) { create(:project) }
       let!(:snippet) { create(:project_snippet, :repository, author: user, project: project) }

spec/services/spam/akismet_service_spec.rb

@@ -59,7 +59,7 @@ RSpec.describe Spam::AkismetService do
     it_behaves_like 'no activity if Akismet is not enabled', :spam?, :check
 
     context 'if Akismet is enabled' do
-      it 'correctly transforms options for the akismet client' do
+      it 'correctly transforms options for the akismet client, including spelling of referrer key' do
         expected_check_params = {
           type: 'comment',
           text: text,

spec/services/spam/spam_action_service_spec.rb

@@ -5,15 +5,20 @@ require 'spec_helper'
 RSpec.describe Spam::SpamActionService do
   include_context 'includes Spam constants'
 
-  let(:request) { double(:request, env: env, headers: {}) }
   let(:issue) { create(:issue, project: project, author: user) }
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
   let(:fake_referer) { 'fake-http-referer' }
-  let(:env) do
-    { 'action_dispatch.remote_ip' => fake_ip,
-      'HTTP_USER_AGENT' => fake_user_agent,
-      'HTTP_REFERER' => fake_referer }
+  let(:captcha_response) { 'abc123' }
+  let(:spam_log_id) { existing_spam_log.id }
+  let(:spam_params) do
+    ::Spam::SpamParams.new(
+      captcha_response: captcha_response,
+      spam_log_id: spam_log_id,
+      ip_address: fake_ip,
+      user_agent: fake_user_agent,
+      referer: fake_referer
+    )
   end
 
   let_it_be(:project) { create(:project, :public) }
@@ -23,32 +28,33 @@ RSpec.describe Spam::SpamActionService do
     issue.spam = false
   end
 
-  shared_examples 'only checks for spam if a request is provided' do
-    context 'when request is missing' do
-      let(:request) { nil }
+  describe 'constructor argument validation' do
+    subject do
+      described_service = described_class.new(spammable: issue, spam_params: spam_params, user: user, action: :create)
+      described_service.execute
+    end
 
-      it "doesn't check as spam" do
-        expect(fake_verdict_service).not_to receive(:execute)
+    context 'when spam_params is nil' do
+      let(:spam_params) { nil }
+      let(:expected_service_params_not_present_message) do
+        /Skipped spam check because spam_params was not present/
+      end
 
+      it "returns success with a messaage" do
         response = subject
 
-        expect(response.message).to match(/request was not present/)
+        expect(response.message).to match(expected_service_params_not_present_message)
         expect(issue).not_to be_spam
       end
     end
-
-    context 'when request exists' do
-      it 'creates a spam log' do
-        expect { subject }
-          .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue')
-      end
-    end
   end
 
   shared_examples 'creates a spam log' do
     it do
-      expect { subject }.to change(SpamLog, :count).by(1)
+      expect { subject }
+        .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue')
 
+      # TODO: These checks should be incorporated into the `log_spam` RSpec matcher above
       new_spam_log = SpamLog.last
       expect(new_spam_log.user_id).to eq(user.id)
       expect(new_spam_log.title).to eq(issue.title)
@@ -56,25 +62,14 @@ RSpec.describe Spam::SpamActionService do
       expect(new_spam_log.source_ip).to eq(fake_ip)
       expect(new_spam_log.user_agent).to eq(fake_user_agent)
       expect(new_spam_log.noteable_type).to eq('Issue')
-      expect(new_spam_log.via_api).to eq(false)
+      expect(new_spam_log.via_api).to eq(true)
     end
   end
 
   describe '#execute' do
-    let(:request) { double(:request, env: env, headers: nil) }
     let(:fake_captcha_verification_service) { double(:captcha_verification_service) }
     let(:fake_verdict_service) { double(:spam_verdict_service) }
     let(:allowlisted) { false }
-    let(:api) { nil }
-    let(:captcha_response) { 'abc123' }
-    let(:spam_log_id) { existing_spam_log.id }
-    let(:spam_params) do
-      ::Spam::SpamParams.new(
-        api: api,
-        captcha_response: captcha_response,
-        spam_log_id: spam_log_id
-      )
-    end
 
     let(:verdict_service_opts) do
       {
@@ -88,7 +83,6 @@ RSpec.describe Spam::SpamActionService do
       {
         target: issue,
         user: user,
-        request: request,
         options: verdict_service_opts,
         context: {
           action: :create,
@@ -100,40 +94,20 @@ RSpec.describe Spam::SpamActionService do
     let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) }
 
     subject do
-      described_service = described_class.new(spammable: issue, request: request, user: user, action: :create)
+      described_service = described_class.new(spammable: issue, spam_params: spam_params, user: user, action: :create)
       allow(described_service).to receive(:allowlisted?).and_return(allowlisted)
-      described_service.execute(spam_params: spam_params)
+      described_service.execute
     end
 
     before do
-      allow(Captcha::CaptchaVerificationService).to receive(:new) { fake_captcha_verification_service }
+      allow(Captcha::CaptchaVerificationService).to receive(:new).with(spam_params: spam_params) { fake_captcha_verification_service }
       allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service)
     end
 
-    context 'when the captcha params are passed in the headers' do
-      let(:request) { double(:request, env: env, headers: headers) }
-      let(:spam_params) { Spam::SpamActionService.filter_spam_params!({ api: api }, request) }
-      let(:headers) do
-        {
-          'X-GitLab-Captcha-Response' => captcha_response,
-          'X-GitLab-Spam-Log-Id' => spam_log_id
-        }
-      end
-
-      it 'extracts the headers correctly' do
-        expect(fake_captcha_verification_service)
-          .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true)
-        expect(SpamLog)
-          .to receive(:verify_recaptcha!).with(user_id: user.id, id: spam_log_id)
-
-        subject
-      end
-    end
-
     context 'when captcha response verification returns true' do
       before do
         allow(fake_captcha_verification_service)
-          .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true)
+          .to receive(:execute).and_return(true)
       end
 
       it "doesn't check with the SpamVerdictService" do
@@ -156,7 +130,7 @@ RSpec.describe Spam::SpamActionService do
     context 'when captcha response verification returns false' do
       before do
         allow(fake_captcha_verification_service)
-          .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(false)
+          .to receive(:execute).and_return(false)
       end
 
       context 'when spammable attributes have not changed' do
@@ -200,8 +174,6 @@ RSpec.describe Spam::SpamActionService do
               stub_feature_flags(allow_possible_spam: false)
             end
 
-            it_behaves_like 'only checks for spam if a request is provided'
-
             it 'marks as spam' do
               response = subject
 
@@ -211,8 +183,6 @@ RSpec.describe Spam::SpamActionService do
           end
 
           context 'when allow_possible_spam feature flag is true' do
-            it_behaves_like 'only checks for spam if a request is provided'
-
             it 'does not mark as spam' do
               response = subject
 
@@ -232,8 +202,6 @@ RSpec.describe Spam::SpamActionService do
               stub_feature_flags(allow_possible_spam: false)
             end
 
-            it_behaves_like 'only checks for spam if a request is provided'
-
             it 'marks as spam' do
               response = subject
 
@@ -243,8 +211,6 @@ RSpec.describe Spam::SpamActionService do
           end
 
           context 'when allow_possible_spam feature flag is true' do
-            it_behaves_like 'only checks for spam if a request is provided'
-
             it 'does not mark as spam' do
               response = subject
 
@@ -264,8 +230,6 @@ RSpec.describe Spam::SpamActionService do
               stub_feature_flags(allow_possible_spam: false)
             end
 
-            it_behaves_like 'only checks for spam if a request is provided'
-
             it_behaves_like 'creates a spam log'
 
             it 'does not mark as spam' do
@@ -284,8 +248,6 @@ RSpec.describe Spam::SpamActionService do
           end
 
           context 'when allow_possible_spam feature flag is true' do
-            it_behaves_like 'only checks for spam if a request is provided'
-
             it_behaves_like 'creates a spam log'
 
             it 'does not mark as needing reCAPTCHA' do
@@ -334,32 +296,6 @@ RSpec.describe Spam::SpamActionService do
             allow(fake_verdict_service).to receive(:execute).and_return(ALLOW)
           end
 
-          context 'when the request is nil' do
-            let(:request) { nil }
-            let(:issue_ip_address) { '1.2.3.4' }
-            let(:issue_user_agent) { 'lynx' }
-            let(:verdict_service_opts) do
-              {
-                ip_address: issue_ip_address,
-                user_agent: issue_user_agent
-              }
-            end
-
-            before do
-              allow(issue).to receive(:ip_address) { issue_ip_address }
-              allow(issue).to receive(:user_agent) { issue_user_agent }
-            end
-
-            it 'assembles the options with information from the spammable' do
-              # TODO: This code untestable, because we do not perform a verification if there is not a
-              #   request. See corresponding comment in code
-              # expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args)
-
-              subject
-            end
-          end
-
-          context 'when the request is present' do
           it 'assembles the options with information from the request' do
             expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args)
 
@@ -369,5 +305,4 @@ RSpec.describe Spam::SpamActionService do
       end
     end
   end
-  end
 end

spec/services/spam/spam_params_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Spam::SpamParams do
+  describe '.new_from_request' do
+    let(:captcha_response) { 'abc123' }
+    let(:spam_log_id) { 42 }
+    let(:ip_address) { '0.0.0.0' }
+    let(:user_agent) { 'Lynx' }
+    let(:referer) { 'http://localhost' }
+    let(:headers) do
+      {
+        'X-GitLab-Captcha-Response' => captcha_response,
+        'X-GitLab-Spam-Log-Id' => spam_log_id
+      }
+    end
+
+    let(:env) do
+      {
+        'action_dispatch.remote_ip' => ip_address,
+        'HTTP_USER_AGENT' => user_agent,
+        'HTTP_REFERER' => referer
+      }
+    end
+
+    let(:request) {double(:request, headers: headers, env: env)}
+
+    it 'constructs from a request' do
+      expected = ::Spam::SpamParams.new(
+        captcha_response: captcha_response,
+        spam_log_id: spam_log_id,
+        ip_address: ip_address,
+        user_agent: user_agent,
+        referer: referer
+      )
+      expect(described_class.new_from_request(request: request)).to eq(expected)
+    end
+  end
+end

spec/services/spam/spam_verdict_service_spec.rb

@@ -14,13 +14,11 @@ RSpec.describe Spam::SpamVerdictService do
       'HTTP_REFERER' => fake_referer }
   end
 
-  let(:request) { double(:request, env: env) }
-
   let(:check_for_spam) { true }
   let_it_be(:user) { create(:user) }
   let_it_be(:issue) { create(:issue, author: user) }
   let(:service) do
-    described_class.new(user: user, target: issue, request: request, options: {})
+    described_class.new(user: user, target: issue, options: {})
   end
 
   let(:attribs) do

spec/spec_helper.rb

@@ -190,6 +190,7 @@ RSpec.configure do |config|
   config.include RailsHelpers
   config.include SidekiqMiddleware
   config.include StubActionCableConnection, type: :channel
+  config.include StubSpamServices
 
   include StubFeatureFlags
 

spec/support/helpers/stub_spam_services.rb

+# frozen_string_literal: true
+
+module StubSpamServices
+  def stub_spam_services
+    allow(::Spam::SpamParams).to receive(:new_from_request) do
+      ::Spam::SpamParams.new(
+        captcha_response: double(:captcha_response),
+        spam_log_id: double(:spam_log_id),
+        ip_address: double(:ip_address),
+        user_agent: double(:user_agent),
+        referer: double(:referer)
+      )
+    end
+
+    allow_next_instance_of(::Spam::SpamActionService) do |service|
+      allow(service).to receive(:execute)
+    end
+
+    allow_next_instance_of(::UserAgentDetailService) do |service|
+      allow(service).to receive(:create)
+    end
+  end
+end

spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb

@@ -3,17 +3,13 @@
 require 'spec_helper'
 
 RSpec.shared_examples 'a mutation which can mutate a spammable' do
-  describe "#additional_spam_params" do
-    it 'passes additional spam params to the service' do
+  describe "#spam_params" do
+    it 'passes spam params to the service constructor' do
       args = [
         project: anything,
         current_user: anything,
-        params: hash_including(
-          api: true,
-          request: instance_of(ActionDispatch::Request),
-          captcha_response: captcha_response,
-          spam_log_id: spam_log_id
-        )
+        params: anything,
+        spam_params: instance_of(::Spam::SpamParams)
       ]
       expect(service).to receive(:new).with(*args).and_call_original
 

spec/support/shared_examples/graphql/spam_protection_shared_examples.rb

@@ -57,7 +57,7 @@ RSpec.shared_examples 'has spam protection' do
       context 'and no CAPTCHA is required' do
         let(:render_captcha) { false }
 
-        it 'does not return a to-level error' do
+        it 'does not return a top-level error' do
           send_request
 
           expect(graphql_errors).to be_blank

spec/support/shared_examples/models/chat_integration_shared_examples.rb

@@ -163,7 +163,7 @@ RSpec.shared_examples "chat integration" do |integration_name|
     context "with issue events" do
       let(:opts) { { title: "Awesome issue", description: "please fix" } }
       let(:sample_data) do
-        service = Issues::CreateService.new(project: project, current_user: user, params: opts)
+        service = Issues::CreateService.new(project: project, current_user: user, params: opts, spam_params: nil)
         issue = service.execute
         service.hook_data(issue, "open")
       end

spec/support/shared_examples/services/snippets_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'checking spam' do
-  let(:request) { double(:request, headers: headers) }
-  let(:headers) { nil }
-  let(:api) { true }
-  let(:captcha_response) { 'abc123' }
-  let(:spam_log_id) { 1 }
-  let(:disable_spam_action_service) { false }
-
-  let(:extra_opts) do
-    {
-      request: request,
-      api: api,
-      captcha_response: captcha_response,
-      spam_log_id: spam_log_id,
-      disable_spam_action_service: disable_spam_action_service
-    }
-  end
-
   before do
     allow_next_instance_of(UserAgentDetailService) do |instance|
       allow(instance).to receive(:create)
@@ -25,67 +8,26 @@ RSpec.shared_examples 'checking spam' do
   end
 
   it 'executes SpamActionService' do
-    spam_params = Spam::SpamParams.new(
-      api: api,
-      captcha_response: captcha_response,
-      spam_log_id: spam_log_id
-    )
     expect_next_instance_of(
       Spam::SpamActionService,
       {
         spammable: kind_of(Snippet),
-        request: request,
+        spam_params: spam_params,
         user: an_instance_of(User),
         action: action
       }
     ) do |instance|
-      expect(instance).to receive(:execute).with(spam_params: spam_params)
+      expect(instance).to receive(:execute)
     end
 
     subject
   end
 
-  context 'when CAPTCHA arguments are passed in the headers' do
-    let(:headers) do
-      {
-        'X-GitLab-Spam-Log-Id' => spam_log_id,
-        'X-GitLab-Captcha-Response' => captcha_response
-      }
-    end
-
-    let(:extra_opts) do
-      {
-        request: request,
-        api: api,
-        disable_spam_action_service: disable_spam_action_service
-      }
-    end
-
-    it 'executes the SpamActionService correctly' do
-      spam_params = Spam::SpamParams.new(
-        api: api,
-        captcha_response: captcha_response,
-        spam_log_id: spam_log_id
-      )
-      expect_next_instance_of(
-        Spam::SpamActionService,
-        {
-          spammable: kind_of(Snippet),
-          request: request,
-          user: an_instance_of(User),
-          action: action
-        }
-      ) do |instance|
-        expect(instance).to receive(:execute).with(spam_params: spam_params)
-      end
-
-      subject
-    end
+  context 'when snippet_spam flag is disabled' do
+    before do
+      stub_feature_flags(snippet_spam: false)
     end
 
-  context 'when spam action service is disabled' do
-    let(:disable_spam_action_service) { true }
-
     it 'request parameter is not passed to the service' do
       expect(Spam::SpamActionService).not_to receive(:new)