Merge branch 'refactor-spam-action-service' into 'master'

Refactor SpamActionService and its usage

See merge request gitlab-org/gitlab!51840

app/controllers/concerns/spammable_actions.rb

@@ -3,9 +3,6 @@
 module SpammableActions
   extend ActiveSupport::Concern
 
-  include Recaptcha::Verify
-  include Gitlab::Utils::StrongMemoize
-
   included do
     before_action :authorize_submit_spammable!, only: :mark_as_spam
   end
@@ -21,9 +18,7 @@ module SpammableActions
   private
 
   def ensure_spam_config_loaded!
-    strong_memoize(:spam_config_loaded) do
-      Gitlab::Recaptcha.load_configurations!
-    end
+    Gitlab::Recaptcha.load_configurations!
   end
 
   def recaptcha_check_with_fallback(should_redirect = true, &fallback)
@@ -50,33 +45,30 @@ module SpammableActions
   end
 
   def spammable_params
-    default_params = { request: request }
-
-    recaptcha_check = recaptcha_response &&
-      ensure_spam_config_loaded! &&
-      verify_recaptcha(response: recaptcha_response)
-
-    return default_params unless recaptcha_check
-
-    { recaptcha_verified: true,
-      spam_log_id: params[:spam_log_id] }.merge(default_params)
-  end
-
-  def recaptcha_response
-    # NOTE: This field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the recaptcha
-    # gem, which is called from the HAML `_recaptcha_form.html.haml` form.
+    # 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` if the `response` option is not
-    # passed explicitly.
+    # 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.
     #
-    # After this newer GraphQL/JS API process is fully supported by the backend, we can remove this
-    # (and other) HAML-specific support.
-    params['g-recaptcha-response']
+    # 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']
+
+    {
+      request: request,
+      spam_log_id: params[:spam_log_id],
+      captcha_response: captcha_response
+    }
   end
 
   def spammable

app/services/captcha/captcha_verification_service.rb

+# frozen_string_literal: true
+
+module Captcha
+  ##
+  # Encapsulates logic of checking captchas.
+  #
+  class CaptchaVerificationService
+    include Recaptcha::Verify
+
+    ##
+    # 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
+
+      @request = request
+
+      Gitlab::Recaptcha.load_configurations!
+
+      # NOTE: We could pass the model and let the recaptcha gem automatically add errors to it,
+      # but we do not, for two reasons:
+      #
+      # 1. We want control over when the errors are added
+      # 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)
+    end
+
+    private
+
+    # 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
+  end
+end

app/services/concerns/spam_check_methods.rb

-# frozen_string_literal: true
-
-# SpamCheckMethods
-#
-# Provide helper methods for checking if a given spammable object has
-# potential spam data.
-#
-# Dependencies:
-# - params with :request
-
-module SpamCheckMethods
-  # rubocop:disable Gitlab/ModuleWithInstanceVariables
-  def filter_spam_check_params
-    @request            = params.delete(:request)
-    @api                = params.delete(:api)
-    @recaptcha_verified = params.delete(:recaptcha_verified)
-    @spam_log_id        = params.delete(:spam_log_id)
-  end
-  # rubocop:enable Gitlab/ModuleWithInstanceVariables
-
-  # In order to be proceed to the spam check process, @spammable has to be
-  # a dirty instance, which means it should be already assigned with the new
-  # attribute values.
-  # rubocop:disable Gitlab/ModuleWithInstanceVariables
-  def spam_check(spammable, user, action:)
-    raise ArgumentError.new('Please provide an action, such as :create') unless action
-
-    Spam::SpamActionService.new(
-      spammable: spammable,
-      request: @request,
-      user: user,
-      context: { action: action }
-    ).execute(
-      api: @api,
-      recaptcha_verified: @recaptcha_verified,
-      spam_log_id: @spam_log_id)
-  end
-  # rubocop:enable Gitlab/ModuleWithInstanceVariables
-end

app/services/issues/create_service.rb

@@ -2,20 +2,26 @@
 
 module Issues
   class CreateService < Issues::BaseService
-    include SpamCheckMethods
     include ResolveDiscussions
 
     def execute(skip_system_notes: false)
+      @request = params.delete(:request)
+      @spam_params = Spam::SpamActionService.filter_spam_params!(params)
+
       @issue = BuildService.new(project, current_user, params).execute
 
-      filter_spam_check_params
       filter_resolve_discussion_params
 
       create(@issue, skip_system_notes: skip_system_notes)
     end
 
     def before_create(issue)
-      spam_check(issue, current_user, action: :create)
+      Spam::SpamActionService.new(
+        spammable: issue,
+        request: request,
+        user: current_user,
+        action: :create
+      ).execute(spam_params: spam_params)
 
       # current_user (defined in BaseService) is not available within run_after_commit block
       user = current_user
@@ -46,8 +52,10 @@ module Issues
 
     private
 
+    attr_reader :request, :spam_params
+
     def user_agent_detail_service
-      UserAgentDetailService.new(@issue, @request)
+      UserAgentDetailService.new(@issue, request)
     end
 
     # Applies label "incident" (creates it if missing) to incident issues.

app/services/issues/update_service.rb

@@ -2,12 +2,14 @@
 
 module Issues
   class UpdateService < Issues::BaseService
-    include SpamCheckMethods
     extend ::Gitlab::Utils::Override
 
     def execute(issue)
       handle_move_between_ids(issue)
-      filter_spam_check_params
+
+      @request = params.delete(:request)
+      @spam_params = Spam::SpamActionService.filter_spam_params!(params)
+
       change_issue_duplicate(issue)
       move_issue_to_new_project(issue) || clone_issue(issue) || update_task_event(issue) || update(issue)
     end
@@ -30,7 +32,14 @@ module Issues
     end
 
     def before_update(issue, skip_spam_check: false)
-      spam_check(issue, current_user, action: :update) unless skip_spam_check
+      return if skip_spam_check
+
+      Spam::SpamActionService.new(
+        spammable: issue,
+        request: request,
+        user: current_user,
+        action: :update
+      ).execute(spam_params: spam_params)
     end
 
     def after_update(issue)
@@ -126,6 +135,8 @@ module Issues
 
     private
 
+    attr_reader :request, :spam_params
+
     def clone_issue(issue)
       target_project = params.delete(:target_clone_project)
       with_notes = params.delete(:clone_with_notes)

app/services/snippets/base_service.rb

@@ -2,8 +2,6 @@
 
 module Snippets
   class BaseService < ::BaseService
-    include SpamCheckMethods
-
     UPDATE_COMMIT_MSG = 'Update snippet'
     INITIAL_COMMIT_MSG = 'Initial commit'
 
@@ -18,8 +16,6 @@ module Snippets
 
       input_actions = Array(@params.delete(:snippet_actions).presence)
       @snippet_actions = SnippetInputActionCollection.new(input_actions, allowed_actions: restricted_files_actions)
-
-      filter_spam_check_params
     end
 
     private

app/services/snippets/create_service.rb

@@ -3,20 +3,28 @@
 module Snippets
   class CreateService < Snippets::BaseService
     def execute
+      @request = params.delete(:request)
+      @spam_params = Spam::SpamActionService.filter_spam_params!(params)
+
       @snippet = build_from_params
 
       return invalid_params_error(@snippet) unless valid_params?
 
-      unless visibility_allowed?(@snippet, @snippet.visibility_level)
-        return forbidden_visibility_error(@snippet)
+      unless visibility_allowed?(snippet, snippet.visibility_level)
+        return forbidden_visibility_error(snippet)
       end
 
       @snippet.author = current_user
 
-      spam_check(@snippet, current_user, action: :create)
+      Spam::SpamActionService.new(
+        spammable: @snippet,
+        request: request,
+        user: current_user,
+        action: :create
+      ).execute(spam_params: spam_params)
 
       if save_and_commit
-        UserAgentDetailService.new(@snippet, @request).create
+        UserAgentDetailService.new(@snippet, request).create
         Gitlab::UsageDataCounters::SnippetCounter.count(:create)
 
         move_temporary_files
@@ -29,6 +37,8 @@ module Snippets
 
     private
 
+    attr_reader :snippet, :request, :spam_params
+
     def build_from_params
       if project
         project.snippets.build(create_params)

app/services/snippets/update_service.rb

@@ -7,6 +7,9 @@ module Snippets
     UpdateError = Class.new(StandardError)
 
     def execute(snippet)
+      @request = params.delete(:request)
+      @spam_params = Spam::SpamActionService.filter_spam_params!(params)
+
       return invalid_params_error(snippet) unless valid_params?
 
       if visibility_changed?(snippet) && !visibility_allowed?(snippet, visibility_level)
@@ -14,7 +17,12 @@ module Snippets
       end
 
       update_snippet_attributes(snippet)
-      spam_check(snippet, current_user, action: :update)
+      Spam::SpamActionService.new(
+        spammable: snippet,
+        request: request,
+        user: current_user,
+        action: :update
+      ).execute(spam_params: spam_params)
 
       if save_and_commit(snippet)
         Gitlab::UsageDataCounters::SnippetCounter.count(:update)
@@ -27,6 +35,8 @@ module Snippets
 
     private
 
+    attr_reader :request, :spam_params
+
     def visibility_changed?(snippet)
       visibility_level && visibility_level.to_i != snippet.visibility_level
     end

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

app/services/spam/spam_params.rb

+# 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

doc/development/module_with_instance_variables.md

@@ -121,71 +121,7 @@ module Gitlab
 end
 ```
 
-Now the cop doesn't complain. Here's a bad example which we could rewrite:
-
-``` ruby
-module SpamCheckService
-  def filter_spam_check_params
-    @request            = params.delete(:request)
-    @api                = params.delete(:api)
-    @recaptcha_verified = params.delete(:recaptcha_verified)
-    @spam_log_id        = params.delete(:spam_log_id)
-  end
-
-  def spam_check(spammable, user)
-    spam_service = SpamService.new(spammable, @request)
-
-    spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
-      user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
-    end
-  end
-end
-```
-
-There are several implicit dependencies here. First, `params` should be
-defined before use. Second, `filter_spam_check_params` should be called
-before `spam_check`. These are all implicit and the includer could be using
-those instance variables without awareness.
-
-This should be rewritten like:
-
-``` ruby
-class SpamCheckService
-  def initialize(request:, api:, recaptcha_verified:, spam_log_id:)
-    @request            = request
-    @api                = api
-    @recaptcha_verified = recaptcha_verified
-    @spam_log_id        = spam_log_id
-  end
-
-  def spam_check(spammable, user)
-    spam_service = SpamService.new(spammable, @request)
-
-    spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
-      user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
-    end
-  end
-end
-```
-
-And use it like:
-
-``` ruby
-class UpdateSnippetService < BaseService
-  def execute
-    # ...
-    spam = SpamCheckService.new(params.slice!(:request, :api, :recaptcha_verified, :spam_log_id))
-
-    spam.check(snippet, current_user)
-    # ...
-  end
-end
-```
-
-This way, all those instance variables are isolated in `SpamCheckService`
-rather than whatever includes the module, and those modules which were also
-included, making it much easier to track down any issues,
-and reducing the chance of having name conflicts.
+Now the cop doesn't complain.
 
 ## How to disable this cop
 

lib/gitlab/recaptcha.rb

@@ -2,8 +2,10 @@
 
 module Gitlab
   module Recaptcha
+    extend Gitlab::Utils::StrongMemoize
+
     def self.load_configurations!
-      if Gitlab::CurrentSettings.recaptcha_enabled || enabled_on_login?
+      if enabled? || enabled_on_login?
         ::Recaptcha.configure do |config|
           config.site_key = Gitlab::CurrentSettings.recaptcha_site_key
           config.secret_key = Gitlab::CurrentSettings.recaptcha_private_key

spec/controllers/concerns/spammable_actions_spec.rb

@@ -6,21 +6,8 @@ RSpec.describe SpammableActions do
   controller(ActionController::Base) do
     include SpammableActions
 
-    # #create is used to test spammable_params
-    # for testing purposes
-    def create
-      spam_params = spammable_params
-
-      # replace the actual request with a string in the JSON response, all we care is that it got set
-      spam_params[:request] = 'this is the request' if spam_params[:request]
-
-      # just return the params in the response so they can be verified in this fake controller spec.
-      # Normally, they are processed further by the controller action
-      render json: spam_params.to_json, status: :ok
-    end
-
-    # #update is used to test recaptcha_check_with_fallback
-    # for testing purposes
+    # #update is used here to test #recaptcha_check_with_fallback, but it could be invoked
+    # from #create or any other action which mutates a spammable via a controller.
     def update
       should_redirect = params[:should_redirect] == 'true'
 
@@ -35,80 +22,7 @@ RSpec.describe SpammableActions do
   end
 
   before do
-    # Ordinarily we would not stub a method on the class under test, but :ensure_spam_config_loaded!
-    # returns false in the test environment, and is also strong_memoized, so we need to stub it
-    allow(controller).to receive(:ensure_spam_config_loaded!) { true }
-  end
-
-  describe '#spammable_params' do
-    subject { post :create, format: :json, params: params }
-
-    shared_examples 'expects request param only' do
-      it do
-        subject
-
-        expect(response).to be_successful
-        expect(json_response).to eq({ 'request' => 'this is the request' })
-      end
-    end
-
-    shared_examples 'expects all spammable params' do
-      it do
-        subject
-
-        expect(response).to be_successful
-        expect(json_response['request']).to eq('this is the request')
-        expect(json_response['recaptcha_verified']).to eq(true)
-        expect(json_response['spam_log_id']).to eq('1')
-      end
-    end
-
-    let(:recaptcha_response) { nil }
-    let(:spam_log_id) { nil }
-
-    context 'when recaptcha response is not present' do
-      let(:params) do
-        {
-          spam_log_id: spam_log_id
-        }
-      end
-
-      it_behaves_like 'expects request param only'
-    end
-
-    context 'when recaptcha response is present' do
-      let(:recaptcha_response) { 'abd123' }
-      let(:params) do
-        {
-          'g-recaptcha-response': recaptcha_response,
-          spam_log_id: spam_log_id
-        }
-      end
-
-      context 'when verify_recaptcha returns falsey' do
-        before do
-          expect(controller).to receive(:verify_recaptcha).with(
-            {
-              response: recaptcha_response
-            }) { false }
-        end
-
-        it_behaves_like 'expects request param only'
-      end
-
-      context 'when verify_recaptcha returns truthy' do
-        let(:spam_log_id) { 1 }
-
-        before do
-          expect(controller).to receive(:verify_recaptcha).with(
-            {
-              response: recaptcha_response
-            }) { true }
-        end
-
-        it_behaves_like 'expects all spammable params'
-      end
-    end
+    allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true }
   end
 
   describe '#recaptcha_check_with_fallback' do

spec/controllers/projects/issues_controller_spec.rb

@@ -1281,11 +1281,13 @@ RSpec.describe Projects::IssuesController do
           let!(:last_spam_log) { spam_logs.last }
 
           def post_verified_issue
-            post_new_issue({}, { spam_log_id: last_spam_log.id, 'g-recaptcha-response': true } )
+            post_new_issue({}, { spam_log_id: last_spam_log.id, 'g-recaptcha-response': 'abc123' } )
           end
 
           before do
-            expect(controller).to receive_messages(verify_recaptcha: true)
+            expect_next_instance_of(Captcha::CaptchaVerificationService) do |instance|
+              expect(instance).to receive(:execute) { true }
+            end
           end
 
           it 'accepts an issue after reCAPTCHA is verified' do

spec/services/captcha/captcha_verification_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Captcha::CaptchaVerificationService do
+  describe '#execute' do
+    let(:captcha_response) { nil }
+    let(:request) { double(:request) }
+    let(:service) { described_class.new }
+
+    subject { service.execute(captcha_response: captcha_response, request: request) }
+
+    context 'when there is no captcha_response' do
+      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
+
+      it 'returns false' do
+        expect(service).to receive(:verify_recaptcha).with(response: captcha_response) { true }
+
+        expect(subject).to eq(true)
+      end
+
+      it 'has a request method which returns the request' do
+        subject
+
+        expect(service.send(:request)).to eq(request)
+      end
+    end
+  end
+end

spec/services/issues/create_service_spec.rb

@@ -452,162 +452,50 @@ RSpec.describe Issues::CreateService do
     end
 
     context 'checking spam' do
-      include_context 'includes Spam constants'
+      let(:request) { double(:request) }
+      let(:api) { true }
+      let(:captcha_response) { 'abc123' }
+      let(:spam_log_id) { 1 }
 
-      let(:title) { 'Legit issue' }
-      let(:description) { 'please fix' }
-      let(:opts) do
+      let(:params) do
         {
-          title: title,
-          description: description,
-          request: double(:request, env: {})
+          title: 'Spam issue',
+          request: request,
+          api: api,
+          captcha_response: captcha_response,
+          spam_log_id: spam_log_id
         }
       end
 
-      subject { described_class.new(project, user, opts) }
-
-      before do
-        stub_feature_flags(allow_possible_spam: false)
+      subject do
+        described_class.new(project, user, params)
       end
 
-      context 'when reCAPTCHA was verified' do
-        let(:log_user)  { user }
-        let(:spam_logs) { create_list(:spam_log, 2, user: log_user, title: title) }
-        let(:target_spam_log) { spam_logs.last }
-
-        before do
-          opts[:recaptcha_verified] = true
-          opts[:spam_log_id] = target_spam_log.id
-
-          expect(Spam::SpamVerdictService).not_to receive(:new)
-        end
-
-        it 'does not mark an issue as spam' do
-          expect(issue).not_to be_spam
-        end
-
-        it 'creates a valid issue' do
-          expect(issue).to be_valid
-        end
-
-        it 'does not assign a spam_log to the issue' do
-          expect(issue.spam_log).to be_nil
-        end
-
-        it 'marks related spam_log as recaptcha_verified' do
-          expect { issue }.to change { target_spam_log.reload.recaptcha_verified }.from(false).to(true)
-        end
-
-        context 'when spam log does not belong to a user' do
-          let(:log_user) { create(:user) }
-
-          it 'does not mark spam_log as recaptcha_verified' do
-            expect { issue }.not_to change { target_spam_log.reload.recaptcha_verified }
-          end
+      before do
+        allow_next_instance_of(UserAgentDetailService) do |instance|
+          allow(instance).to receive(:create)
         end
       end
 
-      context 'when reCAPTCHA was not verified' do
-        before do
-          expect_next_instance_of(Spam::SpamActionService) do |spam_service|
-            expect(spam_service).to receive_messages(check_for_spam?: true)
-          end
-        end
-
-        context 'when SpamVerdictService requires reCAPTCHA' do
-          before do
-            expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
-              expect(verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW)
-            end
-          end
-
-          it 'does not mark the issue as spam' do
-            expect(issue).not_to be_spam
-          end
-
-          it 'marks the issue as needing reCAPTCHA' do
-            expect(issue.needs_recaptcha?).to be_truthy
-          end
-
-          it 'invalidates the issue' do
-            expect(issue).to be_invalid
-          end
-
-          it 'creates a new spam_log' do
-            expect { issue }
-                .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue')
-          end
-        end
-
-        context 'when SpamVerdictService disallows creation' do
-          before do
-            expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
-              expect(verdict_service).to receive(:execute).and_return(DISALLOW)
-            end
-          end
-
-          context 'when allow_possible_spam feature flag is false' do
-            it 'marks the issue as spam' do
-              expect(issue).to be_spam
-            end
-
-            it 'does not mark the issue as needing reCAPTCHA' do
-              expect(issue.needs_recaptcha?).to be_falsey
-            end
-
-            it 'invalidates the issue' do
-              expect(issue).to be_invalid
-            end
-
-            it 'creates a new spam_log' do
-              expect { issue }
-                  .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue')
-            end
-          end
-
-          context 'when allow_possible_spam feature flag is true' do
-            before do
-              stub_feature_flags(allow_possible_spam: true)
-            end
-
-            it 'does not mark the issue as spam' do
-              expect(issue).not_to be_spam
-            end
-
-            it 'does not mark the issue as needing reCAPTCHA' do
-              expect(issue.needs_recaptcha?).to be_falsey
-            end
-
-            it 'creates a valid issue' do
-              expect(issue).to be_valid
-            end
-
-            it 'creates a new spam_log' do
-              expect { issue }
-                  .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue')
-            end
-          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: an_instance_of(Issue),
+            request: request,
+            user: user,
+            action: :create
+          }
+        ) do |instance|
+          expect(instance).to receive(:execute).with(spam_params: spam_params)
         end
 
-        context 'when the SpamVerdictService allows creation' do
-          before do
-            expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
-              expect(verdict_service).to receive(:execute).and_return(ALLOW)
-            end
-          end
-
-          it 'does not mark an issue as spam' do
-            expect(issue).not_to be_spam
-          end
-
-          it 'creates a valid issue' do
-            expect(issue).to be_valid
-          end
-
-          it 'does not assign a spam_log to an issue' do
-            expect(issue.spam_log).to be_nil
-          end
-        end
+        subject.execute
       end
     end
   end

spec/services/issues/update_service_spec.rb

@@ -711,7 +711,7 @@ RSpec.describe Issues::UpdateService, :mailer do
           }
           service = described_class.new(project, user, params)
 
-          expect(service).not_to receive(:spam_check)
+          expect(Spam::SpamActionService).not_to receive(:new)
 
           service.execute(issue)
         end

spec/services/snippets/create_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Snippets::CreateService do
   describe '#execute' do
     let_it_be(:user) { create(:user) }
     let_it_be(:admin) { create(:user, :admin) }
+    let(:action) { :create }
     let(:opts) { base_opts.merge(extra_opts) }
     let(:base_opts) do
       {
@@ -309,7 +310,7 @@ RSpec.describe Snippets::CreateService do
 
       it_behaves_like 'a service that creates a snippet'
       it_behaves_like 'public visibility level restrictions apply'
-      it_behaves_like 'snippets spam check is performed'
+      it_behaves_like 'checking spam'
       it_behaves_like 'snippet create data is tracked'
       it_behaves_like 'an error service response when save fails'
       it_behaves_like 'creates repository and files'
@@ -337,7 +338,7 @@ RSpec.describe Snippets::CreateService do
 
       it_behaves_like 'a service that creates a snippet'
       it_behaves_like 'public visibility level restrictions apply'
-      it_behaves_like 'snippets spam check is performed'
+      it_behaves_like 'checking spam'
       it_behaves_like 'snippet create data is tracked'
       it_behaves_like 'an error service response when save fails'
       it_behaves_like 'creates repository and files'

spec/services/snippets/update_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Snippets::UpdateService do
   describe '#execute', :aggregate_failures do
     let_it_be(:user) { create(:user) }
     let_it_be(:admin) { create :user, admin: true }
+    let(:action) { :update }
     let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE }
     let(:base_opts) do
       {
@@ -738,11 +739,7 @@ RSpec.describe Snippets::UpdateService do
       it_behaves_like 'only file_name is present'
       it_behaves_like 'only content is present'
       it_behaves_like 'invalid params error response'
-      it_behaves_like 'snippets spam check is performed' do
-        before do
-          subject
-        end
-      end
+      it_behaves_like 'checking spam'
 
       context 'when snippet does not have a repository' do
         let!(:snippet) { create(:project_snippet, author: user, project: project) }
@@ -766,11 +763,7 @@ RSpec.describe Snippets::UpdateService do
       it_behaves_like 'only file_name is present'
       it_behaves_like 'only content is present'
       it_behaves_like 'invalid params error response'
-      it_behaves_like 'snippets spam check is performed' do
-        before do
-          subject
-        end
-      end
+      it_behaves_like 'checking spam'
 
       context 'when snippet does not have a repository' do
         let!(:snippet) { create(:personal_snippet, author: user, project: project) }

spec/services/spam/spam_action_service_spec.rb

@@ -24,41 +24,16 @@ RSpec.describe Spam::SpamActionService do
     issue.spam = false
   end
 
-  describe '#initialize' do
-    subject { described_class.new(spammable: issue, request: request, user: user) }
-
-    context 'when the request is nil' do
-      let(:request) { nil }
-
-      it 'assembles the options with information from the spammable' do
-        aggregate_failures do
-          expect(subject.options[:ip_address]).to eq(issue.ip_address)
-          expect(subject.options[:user_agent]).to eq(issue.user_agent)
-          expect(subject.options.key?(:referrer)).to be_falsey
-        end
-      end
-    end
-
-    context 'when the request is present' do
-      let(:request) { double(:request, env: env) }
-
-      it 'assembles the options with information from the spammable' do
-        aggregate_failures do
-          expect(subject.options[:ip_address]).to eq(fake_ip)
-          expect(subject.options[:user_agent]).to eq(fake_user_agent)
-          expect(subject.options[:referrer]).to eq(fake_referrer)
-        end
-      end
-    end
-  end
-
   shared_examples 'only checks for spam if a request is provided' do
     context 'when request is missing' do
-      subject { described_class.new(spammable: issue, request: nil, user: user) }
+      let(:request) { nil }
 
       it "doesn't check as spam" do
-        subject
+        expect(fake_verdict_service).not_to receive(:execute)
+
+        response = subject
 
+        expect(response.message).to match(/request was not present/)
         expect(issue).not_to be_spam
       end
     end
@@ -66,34 +41,88 @@ RSpec.describe Spam::SpamActionService do
     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')
+          .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)
+
+      new_spam_log = SpamLog.last
+      expect(new_spam_log.user_id).to eq(user.id)
+      expect(new_spam_log.title).to eq(issue.title)
+      expect(new_spam_log.description).to eq(issue.description)
+      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)
+    end
+  end
+
   describe '#execute' do
     let(:request) { double(:request, env: env) }
+    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::SpamActionService.filter_spam_params!(
+        api: api,
+        captcha_response: captcha_response,
+        spam_log_id: spam_log_id
+      )
+    end
+
+    let(:verdict_service_opts) do
+      {
+        ip_address: fake_ip,
+        user_agent: fake_user_agent,
+        referrer: fake_referrer
+      }
+    end
+
+    let(:verdict_service_args) do
+      {
+        target: issue,
+        user: user,
+        request: request,
+        options: verdict_service_opts,
+        context: {
+          action: :create,
+          target_type: 'Issue'
+        }
+      }
+    end
 
     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)
+      described_service = described_class.new(spammable: issue, request: request, user: user, action: :create)
       allow(described_service).to receive(:allowlisted?).and_return(allowlisted)
-      described_service.execute(api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id)
+      described_service.execute(spam_params: spam_params)
     end
 
     before do
-      allow(Spam::SpamVerdictService).to receive(:new).and_return(fake_verdict_service)
+      allow(Captcha::CaptchaVerificationService).to receive(:new) { fake_captcha_verification_service }
+      allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service)
     end
 
-    context 'when reCAPTCHA was already verified' do
-      let(:recaptcha_verified) { true }
+    context 'when captcha response verification returns true' do
+      before do
+        expect(fake_captcha_verification_service)
+          .to receive(:execute).with(captcha_response: captcha_response, request: request) { true }
+      end
 
       it "doesn't check with the SpamVerdictService" do
         aggregate_failures do
-          expect(SpamLog).to receive(:verify_recaptcha!)
+          expect(SpamLog).to receive(:verify_recaptcha!).with(
+            user_id: user.id,
+            id: spam_log_id
+          )
           expect(fake_verdict_service).not_to receive(:execute)
         end
 
@@ -105,8 +134,11 @@ RSpec.describe Spam::SpamActionService do
       end
     end
 
-    context 'when reCAPTCHA was not verified' do
-      let(:recaptcha_verified) { false }
+    context 'when captcha response verification returns false' do
+      before do
+        expect(fake_captcha_verification_service)
+          .to receive(:execute).with(captcha_response: captcha_response, request: request) { false }
+      end
 
       context 'when spammable attributes have not changed' do
         before do
@@ -120,6 +152,10 @@ RSpec.describe Spam::SpamActionService do
       end
 
       context 'when spammable attributes have changed' do
+        let(:expected_service_check_response_message) do
+          /check Issue spammable model for any errors or captcha requirement/
+        end
+
         before do
           issue.description = 'SPAM!'
         end
@@ -130,7 +166,9 @@ RSpec.describe Spam::SpamActionService do
           it 'does not perform spam check' do
             expect(Spam::SpamVerdictService).not_to receive(:new)
 
-            subject
+            response = subject
+
+            expect(response.message).to match(/user was allowlisted/)
           end
         end
 
@@ -147,8 +185,9 @@ RSpec.describe Spam::SpamActionService do
             it_behaves_like 'only checks for spam if a request is provided'
 
             it 'marks as spam' do
-              subject
+              response = subject
 
+              expect(response.message).to match(expected_service_check_response_message)
               expect(issue).to be_spam
             end
           end
@@ -157,8 +196,9 @@ RSpec.describe Spam::SpamActionService do
             it_behaves_like 'only checks for spam if a request is provided'
 
             it 'does not mark as spam' do
-              subject
+              response = subject
 
+              expect(response.message).to match(expected_service_check_response_message)
               expect(issue).not_to be_spam
             end
           end
@@ -176,15 +216,19 @@ RSpec.describe Spam::SpamActionService 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 spam' do
-              subject
+              response = subject
 
+              expect(response.message).to match(expected_service_check_response_message)
               expect(issue).not_to be_spam
             end
 
             it 'marks as needing reCAPTCHA' do
-              subject
+              response = subject
 
+              expect(response.message).to match(expected_service_check_response_message)
               expect(issue.needs_recaptcha?).to be_truthy
             end
           end
@@ -192,9 +236,12 @@ RSpec.describe Spam::SpamActionService do
           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
-              subject
+              response = subject
 
+              expect(response.message).to match(expected_service_check_response_message)
               expect(issue.needs_recaptcha).to be_falsey
             end
           end
@@ -209,6 +256,51 @@ RSpec.describe Spam::SpamActionService do
             expect { subject }
               .not_to change { SpamLog.count }
           end
+
+          it 'clears spam flags' do
+            expect(issue).to receive(:clear_spam_flags!)
+
+            subject
+          end
+        end
+
+        context 'spam verdict service options' do
+          before do
+            allow(fake_verdict_service).to receive(:execute) { 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)
+
+              subject
+            end
+          end
         end
       end
     end

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

@@ -21,10 +21,10 @@ RSpec.shared_examples 'can raise spam flag' do
 
   context 'when the snippet is detected as spam' do
     it 'raises spam flag' do
-      allow_next_instance_of(service) do |instance|
-        allow(instance).to receive(:spam_check) do |snippet, user, _|
-          snippet.spam!
-        end
+      allow_next_instance_of(Spam::SpamActionService) do |instance|
+        allow(instance).to receive(:execute) { true }
+        instance.target.spam!
+        instance.target.unrecoverable_spam_error!
       end
 
       subject

spec/support/shared_examples/services/snippets_shared_examples.rb

 # frozen_string_literal: true
 
-RSpec.shared_examples 'snippets spam check is performed' do
-  shared_examples 'marked as spam' do
-    it 'marks a snippet as spam' do
-      expect(snippet).to be_spam
-    end
-
-    it 'invalidates the snippet' do
-      expect(snippet).to be_invalid
-    end
-
-    it 'creates a new spam_log' do
-      expect { snippet }
-        .to have_spam_log(title: snippet.title, noteable_type: snippet.class.name)
-    end
-
-    it 'assigns a spam_log to an issue' do
-      expect(snippet.spam_log).to eq(SpamLog.last)
-    end
-  end
+RSpec.shared_examples 'checking spam' do
+  let(:request) { double(:request) }
+  let(:api) { true }
+  let(:captcha_response) { 'abc123' }
+  let(:spam_log_id) { 1 }
 
   let(:extra_opts) do
-    { visibility_level: Gitlab::VisibilityLevel::PUBLIC, request: double(:request, env: {}) }
+    {
+      request: request,
+      api: api,
+      captcha_response: captcha_response,
+      spam_log_id: spam_log_id
+    }
   end
 
   before do
-    expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-      expect(akismet_service).to receive_messages(spam?: true)
+    allow_next_instance_of(UserAgentDetailService) do |instance|
+      allow(instance).to receive(:create)
     end
   end
 
-  [true, false, nil].each do |allow_possible_spam|
-    context "when allow_possible_spam flag is #{allow_possible_spam.inspect}" do
-      before do
-        stub_feature_flags(allow_possible_spam: allow_possible_spam) unless allow_possible_spam.nil?
-      end
-
-      it_behaves_like 'marked as spam'
-    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,
+        user: an_instance_of(User),
+        action: action
+      }
+    ) do |instance|
+      expect(instance).to receive(:execute).with(spam_params: spam_params)
+    end
+
+    subject
   end
 end