Merge branch '118613-confidential-issue' into 'master'

Decouple spam assessment from Akismet

See merge request gitlab-org/gitlab!29748

app/controllers/concerns/spammable_actions.rb

@@ -82,6 +82,6 @@ module SpammableActions
     return false if spammable.errors.count > 1 # re-render "new" template in case there are other errors
     return false unless Gitlab::Recaptcha.enabled?
 
-    spammable.spam
+    spammable.needs_recaptcha?
   end
 end

app/models/concerns/spammable.rb

@@ -13,9 +13,13 @@ module Spammable
     has_one :user_agent_detail, as: :subject, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
     attr_accessor :spam
+    attr_accessor :needs_recaptcha
     attr_accessor :spam_log
+
     alias_method :spam?, :spam
+    alias_method :needs_recaptcha?, :needs_recaptcha
 
+    # if spam errors are added before validation, they will be wiped
     after_validation :invalidate_if_spam, on: [:create, :update]
 
     cattr_accessor :spammable_attrs, instance_accessor: false do
@@ -38,24 +42,35 @@ module Spammable
   end
 
   def needs_recaptcha!
-    self.errors.add(:base, "Your #{spammable_entity_type} has been recognized as spam. "\
-                  "Please, change the content or solve the reCAPTCHA to proceed.")
+    self.needs_recaptcha = true
   end
 
-  def unrecoverable_spam_error!
-    self.errors.add(:base, "Your #{spammable_entity_type} has been recognized as spam and has been discarded.")
+  def spam!
+    self.spam = true
   end
 
-  def invalidate_if_spam
-    return unless spam?
+  def clear_spam_flags!
+    self.spam = false
+    self.needs_recaptcha = false
+  end
 
-    if Gitlab::Recaptcha.enabled?
-      needs_recaptcha!
-    else
+  def invalidate_if_spam
+    if needs_recaptcha? && Gitlab::Recaptcha.enabled?
+      recaptcha_error!
+    elsif needs_recaptcha? || spam?
       unrecoverable_spam_error!
     end
   end
 
+  def recaptcha_error!
+    self.errors.add(:base, "Your #{spammable_entity_type} has been recognized as spam. "\
+                    "Please, change the content or solve the reCAPTCHA to proceed.")
+  end
+
+  def unrecoverable_spam_error!
+    self.errors.add(:base, "Your #{spammable_entity_type} has been recognized as spam and has been discarded.")
+  end
+
   def spammable_entity_type
     self.class.name.underscore
   end

app/services/concerns/spam_check_methods.rb

@@ -23,7 +23,7 @@ module SpamCheckMethods
   # attribute values.
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def spam_check(spammable, user)
-    Spam::SpamCheckService.new(
+    Spam::SpamActionService.new(
       spammable: spammable,
       request: @request
     ).execute(

app/services/spam/spam_check_service.rb → app/services/spam/spam_action_service.rb

 # frozen_string_literal: true
 
 module Spam
-  class SpamCheckService
-    include AkismetMethods
+  class SpamActionService
+    include SpamConstants
 
     attr_accessor :target, :request, :options
     attr_reader :spam_log
@@ -28,27 +28,40 @@ module Spam
         # update the spam log accordingly.
         SpamLog.verify_recaptcha!(user_id: user_id, id: spam_log_id)
       else
-        # Otherwise, it goes to Akismet for spam check.
-        # If so, it assigns spammable object as "spam" and creates a SpamLog record.
-        possible_spam = check(api)
-        target.spam = possible_spam unless target.allow_possible_spam?
-        target.spam_log = spam_log
+        return unless request
+        return unless check_for_spam?
+
+        perform_spam_service_check(api)
       end
     end
 
+    delegate :check_for_spam?, to: :target
+
     private
 
-    def check(api)
-      return unless request
-      return unless check_for_spam?
-      return unless akismet.spam?
+    def perform_spam_service_check(api)
+      # 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|
+        case result
+        when REQUIRE_RECAPTCHA
+          create_spam_log(api)
 
-      create_spam_log(api)
-      true
-    end
+          break if target.allow_possible_spam?
 
-    def check_for_spam?
-      target.check_for_spam?
+          # TODO: remove spam! declaration
+          # https://gitlab.com/gitlab-org/gitlab/-/issues/214738
+          target.spam!
+          target.needs_recaptcha!
+        when DISALLOW
+          # 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)
+        when ALLOW
+          target.clear_spam_flags!
+        end
+      end
     end
 
     def create_spam_log(api)
@@ -63,6 +76,14 @@ module Spam
           via_api: api
         }
       )
+
+      target.spam_log = spam_log
+    end
+
+    def spam_verdict_service
+      SpamVerdictService.new(target: target,
+                             request: @request,
+                             options: options)
     end
   end
 end

app/services/spam/spam_constants.rb

+# frozen_string_literal: true
+
+module Spam
+  module SpamConstants
+    REQUIRE_RECAPTCHA = :recaptcha
+    DISALLOW = :disallow
+    ALLOW = :allow
+  end
+end

app/services/spam/spam_verdict_service.rb

+# frozen_string_literal: true
+
+module Spam
+  class SpamVerdictService
+    include AkismetMethods
+    include SpamConstants
+
+    def initialize(target:, request:, options:)
+      @target = target
+      @request = request
+      @options = options
+    end
+
+    def execute
+      if akismet.spam?
+        Gitlab::Recaptcha.enabled? ? REQUIRE_RECAPTCHA : DISALLOW
+      else
+        ALLOW
+      end
+    end
+
+    private
+
+    attr_reader :target, :request, :options
+  end
+end

spec/controllers/projects/issues_controller_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe Projects::IssuesController do
   include ProjectForksHelper
+  include_context 'includes Spam constants'
 
   let(:project) { create(:project) }
   let(:user)    { create(:user) }
@@ -419,11 +420,11 @@ describe Projects::IssuesController do
         expect(issue.reload.title).to eq('New title')
       end
 
-      context 'when Akismet is enabled and the issue is identified as spam' do
+      context 'when the SpamVerdictService disallows' do
         before do
           stub_application_setting(recaptcha_enabled: true)
-          expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-            expect(akismet_service).to receive_messages(spam?: true)
+          expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+            expect(verdict_service).to receive(:execute).and_return(REQUIRE_RECAPTCHA)
           end
         end
 
@@ -716,16 +717,16 @@ describe Projects::IssuesController do
         end
       end
 
-      context 'Akismet is enabled' do
+      context 'Recaptcha is enabled' do
         before do
           project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
           stub_application_setting(recaptcha_enabled: true)
         end
 
-        context 'when an issue is not identified as spam' do
+        context 'when SpamVerdictService allows the issue' do
           before do
-            expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-              expect(akismet_service).to receive_messages(spam?: false)
+            expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+              expect(verdict_service).to receive(:execute).and_return(ALLOW)
             end
           end
 
@@ -735,10 +736,10 @@ describe Projects::IssuesController do
         end
 
         context 'when an issue is identified as spam' do
-          context 'when captcha is not verified' do
+          context 'when recaptcha is not verified' do
             before do
-              expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-                expect(akismet_service).to receive_messages(spam?: true)
+              expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+                expect(verdict_service).to receive(:execute).and_return(REQUIRE_RECAPTCHA)
               end
             end
 
@@ -796,7 +797,7 @@ describe Projects::IssuesController do
             end
           end
 
-          context 'when captcha is verified' do
+          context 'when recaptcha is verified' do
             let(:spammy_title) { 'Whatever' }
             let!(:spam_logs) { create_list(:spam_log, 2, user: user, title: spammy_title) }
 
@@ -967,17 +968,17 @@ describe Projects::IssuesController do
       end
     end
 
-    context 'Akismet is enabled' do
+    context 'Recaptcha is enabled' do
       before do
         stub_application_setting(recaptcha_enabled: true)
       end
 
-      context 'when an issue is not identified as spam' do
+      context 'when SpamVerdictService allows the issue' do
         before do
           stub_feature_flags(allow_possible_spam: false)
 
-          expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-            expect(akismet_service).to receive_messages(spam?: false)
+          expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+            expect(verdict_service).to receive(:execute).and_return(ALLOW)
           end
         end
 
@@ -986,18 +987,18 @@ describe Projects::IssuesController do
         end
       end
 
-      context 'when an issue is identified as spam' do
+      context 'when SpamVerdictService requires recaptcha' do
         context 'when captcha is not verified' do
-          def post_spam_issue
-            post_new_issue(title: 'Spam Title', description: 'Spam lives here')
-          end
-
           before do
-            expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-              expect(akismet_service).to receive_messages(spam?: true)
+            expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+              expect(verdict_service).to receive(:execute).and_return(REQUIRE_RECAPTCHA)
             end
           end
 
+          def post_spam_issue
+            post_new_issue(title: 'Spam Title', description: 'Spam lives here')
+          end
+
           context 'when allow_possible_spam feature flag is false' do
             before do
               stub_feature_flags(allow_possible_spam: false)
@@ -1039,11 +1040,12 @@ describe Projects::IssuesController do
           end
         end
 
-        context 'when captcha is verified' do
+        context 'when Recaptcha is verified' do
           let!(:spam_logs) { create_list(:spam_log, 2, user: user, title: 'Title') }
+          let!(:last_spam_log) { spam_logs.last }
 
           def post_verified_issue
-            post_new_issue({}, { spam_log_id: spam_logs.last.id, recaptcha_verification: true } )
+            post_new_issue({}, { spam_log_id: last_spam_log.id, recaptcha_verification: true } )
           end
 
           before do
@@ -1055,14 +1057,14 @@ describe Projects::IssuesController do
           end
 
           it 'marks spam log as recaptcha_verified' do
-            expect { post_verified_issue }.to change { SpamLog.last.recaptcha_verified }.from(false).to(true)
+            expect { post_verified_issue }.to change { last_spam_log.reload.recaptcha_verified }.from(false).to(true)
           end
 
           it 'does not mark spam log as recaptcha_verified when it does not belong to current_user' do
             spam_log = create(:spam_log)
 
             expect { post_new_issue({}, { spam_log_id: spam_log.id, recaptcha_verification: true } ) }
-              .not_to change { SpamLog.last.recaptcha_verified }
+              .not_to change { last_spam_log.recaptcha_verified }
           end
         end
       end

spec/features/issues/spam_issues_spec.rb

@@ -23,9 +23,13 @@ describe 'New issue', :js do
     sign_in(user)
   end
 
-  context 'when identified as spam' do
+  context 'when SpamVerdictService disallows' do
+    include_context 'includes Spam constants'
+
     before do
-      WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "true", status: 200)
+      allow_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+        allow(verdict_service).to receive(:execute).and_return(DISALLOW)
+      end
 
       visit new_project_issue_path(project)
     end
@@ -33,23 +37,22 @@ describe 'New issue', :js do
     context 'when allow_possible_spam feature flag is false' do
       before do
         stub_feature_flags(allow_possible_spam: false)
-      end
 
-      it 'creates an issue after solving reCaptcha' do
         fill_in 'issue_title', with: 'issue title'
         fill_in 'issue_description', with: 'issue description'
+      end
 
+      it 'rejects issue creation' do
         click_button 'Submit issue'
 
-        # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha
-        # recaptcha verification is skipped in test environment and it always returns true
+        expect(page).to have_content('discarded')
+        expect(page).not_to have_content('potential spam')
         expect(page).not_to have_content('issue title')
-        expect(page).to have_css('.recaptcha')
-
-        click_button 'Submit issue'
+      end
 
-        expect(page.find('.issue-details h2.title')).to have_content('issue title')
-        expect(page.find('.issue-details .description')).to have_content('issue description')
+      it 'creates a spam log record' do
+        expect { click_button 'Submit issue' }
+            .to log_spam(title: 'issue title', description: 'issue description', user_id: user.id, noteable_type: 'Issue')
       end
     end
 
@@ -59,10 +62,9 @@ describe 'New issue', :js do
         fill_in 'issue_description', with: 'issue description'
       end
 
-      it 'creates an issue without a need to solve reCaptcha' do
+      it 'allows issue creation' do
         click_button 'Submit issue'
 
-        expect(page).not_to have_css('.recaptcha')
         expect(page.find('.issue-details h2.title')).to have_content('issue title')
         expect(page.find('.issue-details .description')).to have_content('issue description')
       end
@@ -74,9 +76,98 @@ describe 'New issue', :js do
     end
   end
 
-  context 'when not identified as spam' do
+  context 'when SpamVerdictService requires recaptcha' do
+    include_context 'includes Spam constants'
+
     before do
-      WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: 'false', status: 200)
+      allow_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+        allow(verdict_service).to receive(:execute).and_return(REQUIRE_RECAPTCHA)
+      end
+
+      visit new_project_issue_path(project)
+    end
+
+    context 'when recaptcha is enabled' do
+      before do
+        stub_application_setting(recaptcha_enabled: true)
+      end
+
+      context 'when allow_possible_spam feature flag is false' do
+        before do
+          stub_feature_flags(allow_possible_spam: false)
+        end
+
+        it 'creates an issue after solving reCaptcha' do
+          fill_in 'issue_title', with: 'issue title'
+          fill_in 'issue_description', with: 'issue description'
+
+          click_button 'Submit issue'
+
+          # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha
+          # recaptcha verification is skipped in test environment and it always returns true
+          expect(page).not_to have_content('issue title')
+          expect(page).to have_css('.recaptcha')
+
+          click_button 'Submit issue'
+
+          expect(page.find('.issue-details h2.title')).to have_content('issue title')
+          expect(page.find('.issue-details .description')).to have_content('issue description')
+        end
+      end
+
+      context 'when allow_possible_spam feature flag is true' do
+        before do
+          fill_in 'issue_title', with: 'issue title'
+          fill_in 'issue_description', with: 'issue description'
+        end
+
+        it 'creates an issue without a need to solve reCAPTCHA' do
+          click_button 'Submit issue'
+
+          expect(page).not_to have_css('.recaptcha')
+          expect(page.find('.issue-details h2.title')).to have_content('issue title')
+          expect(page.find('.issue-details .description')).to have_content('issue description')
+        end
+
+        it 'creates a spam log record' do
+          expect { click_button 'Submit issue' }
+              .to log_spam(title: 'issue title', description: 'issue description', user_id: user.id, noteable_type: 'Issue')
+        end
+      end
+    end
+
+    context 'when reCAPTCHA is not enabled' do
+      before do
+        stub_application_setting(recaptcha_enabled: false)
+      end
+
+      context 'when allow_possible_spam feature flag is true' do
+        before do
+          fill_in 'issue_title', with: 'issue title'
+          fill_in 'issue_description', with: 'issue description'
+        end
+
+        it 'creates an issue without a need to solve reCaptcha' do
+          click_button 'Submit issue'
+
+          expect(page).not_to have_css('.recaptcha')
+          expect(page.find('.issue-details h2.title')).to have_content('issue title')
+          expect(page.find('.issue-details .description')).to have_content('issue description')
+        end
+
+        it 'creates a spam log record' do
+          expect { click_button 'Submit issue' }
+              .to log_spam(title: 'issue title', description: 'issue description', user_id: user.id, noteable_type: 'Issue')
+        end
+      end
+    end
+  end
+
+  context 'when the SpamVerdictService allows' do
+    before do
+      allow_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+        allow(verdict_service).to receive(:execute).and_return(ALLOW)
+      end
 
       visit new_project_issue_path(project)
     end

spec/features/snippets/spam_snippets_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 shared_examples_for 'snippet editor' do
+  include_context 'includes Spam constants'
+
   def description_field
     find('.js-description-input').find('input,textarea')
   end
@@ -53,13 +55,30 @@ shared_examples_for 'snippet editor' do
     end
   end
 
-  context 'when identified as spam' do
+  shared_examples 'does not allow creation' do
+    it 'rejects creation of the snippet' do
+      click_button('Create snippet')
+      wait_for_requests
+
+      expect(page).to have_content('discarded')
+      expect(page).not_to have_content('My Snippet Title')
+      expect(page).not_to have_css('.recaptcha')
+    end
+  end
+
+  context 'when SpamVerdictService requires recaptcha' do
     before do
-      WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "true", status: 200)
+      expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+        expect(verdict_service).to receive(:execute).and_return(REQUIRE_RECAPTCHA)
+      end
     end
 
     context 'when allow_possible_spam feature flag is false' do
-      it_behaves_like 'solve recaptcha'
+      before do
+        stub_application_setting(recaptcha_enabled: false)
+      end
+
+      it_behaves_like 'does not allow creation'
     end
 
     context 'when allow_possible_spam feature flag is true' do
@@ -67,9 +86,31 @@ shared_examples_for 'snippet editor' do
     end
   end
 
-  context 'when not identified as spam' do
+  context 'when SpamVerdictService disallows' 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
+      before do
+        stub_application_setting(recaptcha_enabled: false)
+      end
+
+      it_behaves_like 'does not allow creation'
+    end
+
+    context 'when allow_possible_spam feature flag is true' do
+      it_behaves_like 'does not allow creation'
+    end
+  end
+
+  context 'when SpamVerdictService allows' do
     before do
-      WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "false", status: 200)
+      expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+        expect(verdict_service).to receive(:execute).and_return(ALLOW)
+      end
     end
 
     it 'creates a snippet' do

spec/models/concerns/spammable_spec.rb

@@ -39,43 +39,100 @@ describe Spammable do
     describe '#invalidate_if_spam' do
       using RSpec::Parameterized::TableSyntax
 
+      before do
+        stub_application_setting(recaptcha_enabled: true)
+      end
+
       context 'when the model is spam' do
-        where(:recaptcha_enabled, :error) do
-          true  | /solve the reCAPTCHA to proceed/
-          false | /has been discarded/
+        subject { invalidate_if_spam(is_spam: true) }
+
+        it 'has an error related to spam on the model' do
+          expect(subject.errors.messages[:base]).to match_array /has been discarded/
         end
+      end
 
-        with_them do
-          subject { invalidate_if_spam(true, recaptcha_enabled) }
+      context 'when the model needs recaptcha' do
+        subject { invalidate_if_spam(needs_recaptcha: true) }
 
-          it 'has an error related to spam on the model' do
-            expect(subject.errors.messages[:base]).to match_array error
-          end
+        it 'has an error related to spam on the model' do
+          expect(subject.errors.messages[:base]).to match_array /solve the reCAPTCHA/
         end
       end
 
-      context 'when the model is not spam' do
-        [true, false].each do |enabled|
-          let(:recaptcha_enabled) { enabled }
+      context 'if the model is spam and also needs recaptcha' do
+        subject { invalidate_if_spam(is_spam: true, needs_recaptcha: true) }
+
+        it 'has an error related to spam on the model' do
+          expect(subject.errors.messages[:base]).to match_array /solve the reCAPTCHA/
+        end
+      end
 
-          subject { invalidate_if_spam(false, recaptcha_enabled) }
+      context 'when the model is not spam nor needs recaptcha' do
+        subject { invalidate_if_spam }
 
-          it 'returns no error' do
-            expect(subject.errors.messages[:base]).to be_empty
-          end
+        it 'returns no error' do
+          expect(subject.errors.messages[:base]).to be_empty
         end
       end
 
-      def invalidate_if_spam(is_spam, recaptcha_enabled)
-        stub_application_setting(recaptcha_enabled: recaptcha_enabled)
+      context 'if recaptcha is not enabled and the model needs recaptcha' do
+        before do
+          stub_application_setting(recaptcha_enabled: false)
+        end
+
+        subject { invalidate_if_spam(needs_recaptcha: true) }
 
+        it 'has no errors' do
+          expect(subject.errors.messages[:base]).to match_array /has been discarded/
+        end
+      end
+
+      def invalidate_if_spam(is_spam: false, needs_recaptcha: false)
         issue.tap do |i|
           i.spam = is_spam
+          i.needs_recaptcha = needs_recaptcha
           i.invalidate_if_spam
         end
       end
     end
 
+    describe 'spam flags' do
+      before do
+        issue.spam = false
+        issue.needs_recaptcha = false
+      end
+
+      describe '#spam!' do
+        it 'adds only `spam` flag' do
+          issue.spam!
+
+          expect(issue.spam).to be_truthy
+          expect(issue.needs_recaptcha).to be_falsey
+        end
+      end
+
+      describe '#needs_recaptcha!' do
+        it 'adds `needs_recaptcha` flag' do
+          issue.needs_recaptcha!
+
+          expect(issue.spam).to be_falsey
+          expect(issue.needs_recaptcha).to be_truthy
+        end
+      end
+
+      describe '#clear_spam_flags!' do
+        it 'clears spam and recaptcha flags' do
+          issue.spam = true
+          issue.needs_recaptcha = true
+
+          issue.clear_spam_flags!
+
+          expect(issue).not_to be_spam
+          expect(issue.needs_recaptcha).to be_falsey
+        end
+      end
+    end
+
     describe '#submittable_as_spam_by?' do
       let(:admin) { build(:admin) }
       let(:user) { build(:user) }

spec/requests/api/issues/post_projects_issues_spec.rb

@@ -403,7 +403,7 @@ describe API::Issues do
     end
 
     before do
-      expect_next_instance_of(Spam::SpamCheckService) do |spam_service|
+      expect_next_instance_of(Spam::SpamActionService) do |spam_service|
         expect(spam_service).to receive_messages(check_for_spam?: true)
       end
       expect_next_instance_of(Spam::AkismetService) do |akismet_service|

spec/requests/api/issues/put_projects_issues_spec.rb

@@ -182,6 +182,8 @@ describe API::Issues do
   end
 
   describe 'PUT /projects/:id/issues/:issue_iid with spam filtering' do
+    include_context 'includes Spam constants'
+
     def update_issue
       put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: params
     end
@@ -195,11 +197,12 @@ describe API::Issues do
     end
 
     before do
-      expect_next_instance_of(Spam::SpamCheckService) do |spam_service|
+      expect_next_instance_of(Spam::SpamActionService) do |spam_service|
         expect(spam_service).to receive_messages(check_for_spam?: true)
       end
-      expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-        expect(akismet_service).to receive_messages(spam?: true)
+
+      expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+        expect(verdict_service).to receive(:execute).and_return(DISALLOW)
       end
     end
 

spec/services/issues/create_service_spec.rb

@@ -368,6 +368,8 @@ describe Issues::CreateService do
     end
 
     context 'checking spam' do
+      include_context 'includes Spam constants'
+
       let(:title) { 'Legit issue' }
       let(:description) { 'please fix' }
       let(:opts) do
@@ -378,6 +380,8 @@ describe Issues::CreateService do
         }
       end
 
+      subject { described_class.new(project, user, opts) }
+
       before do
         stub_feature_flags(allow_possible_spam: false)
       end
@@ -391,7 +395,7 @@ describe Issues::CreateService do
           opts[:recaptcha_verified] = true
           opts[:spam_log_id] = target_spam_log.id
 
-          expect(Spam::AkismetService).not_to receive(:new)
+          expect(Spam::SpamVerdictService).not_to receive(:new)
         end
 
         it 'does not mark an issue as spam' do
@@ -402,7 +406,7 @@ describe Issues::CreateService do
           expect(issue).to be_valid
         end
 
-        it 'does not assign a spam_log to an issue' do
+        it 'does not assign a spam_log to the issue' do
           expect(issue.spam_log).to be_nil
         end
 
@@ -421,23 +425,52 @@ describe Issues::CreateService do
 
       context 'when recaptcha was not verified' do
         before do
-          expect_next_instance_of(Spam::SpamCheckService) do |spam_service|
+          expect_next_instance_of(Spam::SpamActionService) do |spam_service|
             expect(spam_service).to receive_messages(check_for_spam?: true)
           end
         end
 
-        context 'when akismet detects spam' do
+        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(REQUIRE_RECAPTCHA)
+            end
+          end
+
+          it 'marks the issue as spam' do
+            expect(issue).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::AkismetService) do |akismet_service|
-              expect(akismet_service).to receive_messages(spam?: true)
+            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
+            it 'does not mark 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
@@ -457,7 +490,7 @@ describe Issues::CreateService do
               expect(issue).not_to be_spam
             end
 
-            it '​creates a valid issue' do
+            it 'creates a valid issue' do
               expect(issue).to be_valid
             end
 
@@ -468,10 +501,10 @@ describe Issues::CreateService do
           end
         end
 
-        context 'when akismet does not detect spam' do
+        context 'when the SpamVerdictService allows creation' do
           before do
-            expect_next_instance_of(Spam::AkismetService) do |akismet_service|
-              expect(akismet_service).to receive_messages(spam?: false)
+            expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service|
+              expect(verdict_service).to receive(:execute).and_return(ALLOW)
             end
           end
 

spec/services/spam/spam_check_service_spec.rb → spec/services/spam/spam_action_service_spec.rb

@@ -2,7 +2,9 @@
 
 require 'spec_helper'
 
-describe Spam::SpamCheckService do
+describe Spam::SpamActionService do
+  include_context 'includes Spam constants'
+
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
   let(:fake_referrer) { 'fake-http-referrer' }
@@ -15,7 +17,7 @@ describe Spam::SpamCheckService do
 
   let_it_be(:project) { create(:project, :public) }
   let_it_be(:user) { create(:user) }
-  let_it_be(:issue) { create(:issue, project: project, author: user) }
+  let(:issue) { create(:issue, project: project, author: user) }
 
   before do
     issue.spam = false
@@ -51,7 +53,7 @@ describe Spam::SpamCheckService do
 
   shared_examples 'only checks for spam if a request is provided' do
     context 'when request is missing' do
-      let(:request) { nil }
+      subject { described_class.new(spammable: issue, request: nil) }
 
       it "doesn't check as spam" do
         subject
@@ -70,6 +72,7 @@ describe Spam::SpamCheckService do
 
   describe '#execute' do
     let(:request) { double(:request, env: env) }
+    let(:fake_verdict_service) { double(:spam_verdict_service) }
 
     let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) }
 
@@ -78,13 +81,17 @@ describe Spam::SpamCheckService do
       described_service.execute(user_id: user.id, api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id)
     end
 
+    before do
+      allow(Spam::SpamVerdictService).to receive(:new).and_return(fake_verdict_service)
+    end
+
     context 'when recaptcha was already verified' do
       let(:recaptcha_verified) { true }
 
-      it "updates spam log and doesn't check Akismet" do
+      it "doesn't check with the SpamVerdictService" do
         aggregate_failures do
-          expect(SpamLog).not_to receive(:create!)
-          expect(an_instance_of(described_class)).not_to receive(:check)
+          expect(SpamLog).to receive(:verify_recaptcha!)
+          expect(fake_verdict_service).not_to receive(:execute)
         end
 
         subject
@@ -101,12 +108,6 @@ describe Spam::SpamCheckService do
       context 'when spammable attributes have not changed' do
         before do
           issue.closed_at = Time.zone.now
-
-          allow(Spam::AkismetService).to receive(:new).and_return(double(spam?: true))
-        end
-
-        it 'returns false' do
-          expect(subject).to be_falsey
         end
 
         it 'does not create a spam log' do
@@ -120,9 +121,9 @@ describe Spam::SpamCheckService do
           issue.description = 'SPAM!'
         end
 
-        context 'when indicated as spam by Akismet' do
+        context 'when disallowed by the spam action service' do
           before do
-            allow(Spam::AkismetService).to receive(:new).and_return(double(spam?: true))
+            allow(fake_verdict_service).to receive(:execute).and_return(DISALLOW)
           end
 
           context 'when allow_possible_spam feature flag is false' do
@@ -150,13 +151,9 @@ describe Spam::SpamCheckService do
           end
         end
 
-        context 'when not indicated as spam by Akismet' do
+        context 'when spam action service allows creation' do
           before do
-            allow(Spam::AkismetService).to receive(:new).and_return(double(spam?: false))
-          end
-
-          it 'returns false' do
-            expect(subject).to be_falsey
+            allow(fake_verdict_service).to receive(:execute).and_return(ALLOW)
           end
 
           it 'does not create a spam log' do

spec/services/spam/spam_verdict_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Spam::SpamVerdictService do
+  include_context 'includes Spam constants'
+
+  let(:fake_ip) { '1.2.3.4' }
+  let(:fake_user_agent) { 'fake-user-agent' }
+  let(:fake_referrer) { 'fake-http-referrer' }
+  let(:env) do
+    { 'action_dispatch.remote_ip' => fake_ip,
+      'HTTP_USER_AGENT' => fake_user_agent,
+      'HTTP_REFERRER' => fake_referrer }
+  end
+  let(:request) { double(:request, env: env) }
+
+  let(:check_for_spam) { true }
+  let(:issue) { build(:issue) }
+  let(:service) do
+    described_class.new(target: issue, request: request, options: {})
+  end
+
+  describe '#execute' do
+    subject { service.execute }
+
+    before do
+      allow_next_instance_of(Spam::AkismetService) do |service|
+        allow(service).to receive(:spam?).and_return(spam_verdict)
+      end
+    end
+
+    context 'if Akismet considers it spam' do
+      let(:spam_verdict) { true }
+
+      context 'if reCAPTCHA is enabled' do
+        before do
+          stub_application_setting(recaptcha_enabled: true)
+        end
+
+        it 'requires reCAPTCHA' do
+          expect(subject).to eq REQUIRE_RECAPTCHA
+        end
+      end
+
+      context 'if reCAPTCHA is not enabled' do
+        before do
+          stub_application_setting(recaptcha_enabled: false)
+        end
+
+        it 'disallows the change' do
+          expect(subject).to eq DISALLOW
+        end
+      end
+    end
+
+    context 'if Akismet does not consider it spam' do
+      let(:spam_verdict) { false }
+
+      it 'allows the change' do
+        expect(subject).to eq ALLOW
+      end
+    end
+  end
+end

spec/support/shared_contexts/spam_constants.rb

+# frozen_string_literal: true
+
+shared_context 'includes Spam constants' do
+  REQUIRE_RECAPTCHA = Spam::SpamConstants::REQUIRE_RECAPTCHA
+  DISALLOW = Spam::SpamConstants::DISALLOW
+  ALLOW = Spam::SpamConstants::ALLOW
+end