Add latest changes from gitlab-org/gitlab@master

Gemfile

@@ -477,6 +477,9 @@ group :ed25519 do
   gem 'bcrypt_pbkdf', '~> 1.0'
 end
 
+# Spamcheck GRPC protocol definitions
+gem 'spamcheck', '~> 0.0.5'
+
 # Gitaly GRPC protocol definitions
 gem 'gitaly', '~> 13.11.0.pre.rc1'
 

Gemfile.lock

@@ -1211,6 +1211,8 @@ GEM
       thor (~> 1.0)
       tilt (~> 2.0)
       yard (~> 0.9, >= 0.9.24)
+    spamcheck (0.0.5)
+      grpc (~> 1.0)
     spring (2.1.1)
     spring-commands-rspec (1.0.4)
       spring (>= 0.9.1)
@@ -1606,6 +1608,7 @@ DEPENDENCIES
   slack-messenger (~> 2.3.4)
   snowplow-tracker (~> 0.6.1)
   solargraph (~> 0.40.4)
+  spamcheck (~> 0.0.5)
   spring (~> 2.1.0)
   spring-commands-rspec (~> 1.0.4)
   sprockets (~> 3.7.0)

app/models/application_setting.rb

@@ -375,7 +375,7 @@ class ApplicationSetting < ApplicationRecord
             if: :external_authorization_service_enabled
 
   validates :spam_check_endpoint_url,
-            addressable_url: true, allow_blank: true
+            addressable_url: { schemes: %w(grpc) }, allow_blank: true
 
   validates :spam_check_endpoint_url,
             presence: true,
@@ -524,7 +524,7 @@ class ApplicationSetting < ApplicationRecord
   attr_encrypted :lets_encrypt_private_key, encryption_options_base_32_aes_256_gcm
   attr_encrypted :eks_secret_access_key, encryption_options_base_32_aes_256_gcm
   attr_encrypted :akismet_api_key, encryption_options_base_32_aes_256_gcm
-  attr_encrypted :spam_check_api_key, encryption_options_base_32_aes_256_gcm
+  attr_encrypted :spam_check_api_key, encryption_options_base_32_aes_256_gcm.merge(encode: false)
   attr_encrypted :elasticsearch_aws_secret_access_key, encryption_options_base_32_aes_256_gcm
   attr_encrypted :recaptcha_private_key, encryption_options_base_32_aes_256_gcm
   attr_encrypted :recaptcha_site_key, encryption_options_base_32_aes_256_gcm

app/services/spam/akismet_service.rb

@@ -20,7 +20,7 @@ module Spam
         created_at: DateTime.current,
         author: owner_name,
         author_email: owner_email,
-        referrer: options[:referrer]
+        referer: options[:referer]
       }
 
       begin

app/services/spam/spam_action_service.rb

@@ -53,7 +53,7 @@ module Spam
       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']
+        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?
@@ -123,6 +123,11 @@ module Spam
           # https://gitlab.com/gitlab-org/gitlab/-/issues/214739
           target.spam! unless target.allow_possible_spam?
           create_spam_log(api)
+        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)
         when ALLOW
           target.clear_spam_flags!
         end

app/services/spam/spam_verdict_service.rb

@@ -10,25 +10,44 @@ module Spam
       @request = request
       @user = user
       @options = options
-      @verdict_params = assemble_verdict_params(context)
+      @context = context
     end
 
     def execute
-      external_spam_check_result = external_verdict
+      spamcheck_result = nil
+
+      external_spam_check_round_trip_time = Benchmark.realtime do
+        spamcheck_result = spamcheck_verdict
+      end
+
       akismet_result = akismet_verdict
 
       # filter out anything we don't recognise, including nils.
-      valid_results = [external_spam_check_result, akismet_result].compact.select { |r| SUPPORTED_VERDICTS.key?(r) }
+      valid_results = [spamcheck_result, akismet_result].compact.select { |r| SUPPORTED_VERDICTS.key?(r) }
+
       # Treat nils - such as service unavailable - as ALLOW
       return ALLOW unless valid_results.any?
 
       # Favour the most restrictive result.
-      valid_results.min_by { |v| SUPPORTED_VERDICTS[v][:priority] }
+      final_verdict = valid_results.min_by { |v| SUPPORTED_VERDICTS[v][:priority] }
+
+      logger.info(class: self.class.name,
+                  akismet_verdict: akismet_verdict,
+                  spam_check_verdict: spamcheck_result,
+                  spam_check_rtt: external_spam_check_round_trip_time.real,
+                  final_verdict: final_verdict,
+                  username: user.username,
+                  user_id: user.id,
+                  target_type: target.class.to_s,
+                  project_id: target.project_id
+                 )
+
+      final_verdict
     end
 
     private
 
-    attr_reader :user, :target, :request, :options, :verdict_params
+    attr_reader :user, :target, :request, :options, :context
 
     def akismet_verdict
       if akismet.spam?
@@ -38,54 +57,34 @@ module Spam
       end
     end
 
-    def external_verdict
+    def spamcheck_verdict
       return unless Gitlab::CurrentSettings.spam_check_endpoint_enabled
-      return if endpoint_url.blank?
 
       begin
-        result = Gitlab::HTTP.post(endpoint_url, body: verdict_params.to_json, headers: { 'Content-Type' => 'application/json' })
+        result, _error = spamcheck_client.issue_spam?(spam_issue: target, user: user, context: context)
         return unless result
 
-        json_result = Gitlab::Json.parse(result).with_indifferent_access
+        # @TODO log if error is not nil https://gitlab.com/gitlab-org/gitlab/-/issues/329545
-        # @TODO metrics/logging
-        # Expecting:
-        # error: (string or nil)
-        # verdict: (string or nil)
-        # @TODO log if json_result[:error]
 
-        json_result[:verdict]
+        # Duplicate logic with Akismet logic in #akismet_verdict
-      rescue *Gitlab::HTTP::HTTP_ERRORS => e
+        if Gitlab::Recaptcha.enabled? && result != ALLOW
-        # @TODO: log error via try_post https://gitlab.com/gitlab-org/gitlab/-/issues/219223
+          CONDITIONAL_ALLOW
+        else
+          result
+        end
+      rescue StandardError => e
         Gitlab::ErrorTracking.log_exception(e)
-        nil
+        # Default to ALLOW if any errors occur
-      rescue StandardError
-        # @TODO log
         ALLOW
       end
     end
 
-    def assemble_verdict_params(context)
+    def spamcheck_client
-      return {} unless endpoint_url.present?
+      @spamcheck_client ||= Gitlab::Spamcheck::Client.new
-
-      project = target.try(:project)
-
-      context.merge({
-        target: {
-          title: target.spam_title,
-          description: target.spam_description,
-          type: target.class.to_s
-        },
-        user: {
-          created_at: user.created_at,
-          email: user.email,
-          username: user.username
-        },
-        user_in_project: user.authorized_project?(project)
-      })
     end
 
-    def endpoint_url
+    def logger
-      @endpoint_url ||= Gitlab::CurrentSettings.current_application_settings.spam_check_endpoint_url
+      @logger ||= Gitlab::AppJsonLogger.build
     end
   end
 end

app/views/projects/new.html.haml

@@ -51,8 +51,7 @@
             = render 'new_project_fields', f: f, project_name_id: "blank-project-name"
 
         #create-from-template-pane.tab-pane.js-toggle-container.px-0.pb-0{ class: active_when(active_tab == 'template'), role: 'tabpanel' }
-          .gl-card.gl-my-5
+          .card.card-slim.m-4.p-4
-            .gl-card-body
             %div
               - contributing_templates_url = 'https://gitlab.com/gitlab-org/project-templates/contributing'
               - link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: contributing_templates_url }

changelogs/unreleased/299558-integrate-with-anti-spam-service.yml

@@ -0,0 +1,5 @@
+---
+title: Integrate with the Spamcheck anti-spam engine
+merge_request: 52385
+author:
+type: added

changelogs/unreleased/yogi-hide-templates-buttons.yml

@@ -1,5 +0,0 @@
----
-title: Remove template tabs in new project page for GitLab.com
-merge_request: 59083
-author: Yogi (@yo)
-type: changed

doc/development/packages.md

@@ -256,6 +256,30 @@ These route prefixes guarantee a higher rate limit:
 /api/v4/groups/:group_id/-/packages/
 ```
 
+### MVC Checklist
+
+When adding support to GitLab for a new package manager, the first iteration must contain the
+following features. You can add the features through many merge requests as needed, but all the
+features must be implemented when the feature flag is removed.
+
+- Project-level API
+- Push event tracking
+- Pull event tracking
+- Authentication with Personal Access Tokens
+- Authentication with Job Tokens
+- Authentication with Deploy Tokens (group and project)
+- File size [limit](#file-size-limits)
+- File format guards (only accept valid file formats for the package type)
+- Name regex with validation
+- Version regex with validation
+- Workhorse route for [accelerated](uploads.md#how-to-add-a-new-upload-route) uploads
+- Background workers for extracting package metadata (if applicable)
+- Documentation (how to use the feature)
+- API Documentation (individual endpoints with curl examples)
+- Seeding in [`db/fixtures/development/26_packages.rb`](https://gitlab.com/gitlab-org/gitlab/blob/master/db/fixtures/development/26_packages.rb)
+- Update the [runbook](https://gitlab.com/gitlab-com/runbooks/-/blob/31fb4959e89db25fddf865bc81734c222daf32dd/dashboards/stage-groups/package.dashboard.jsonnet#L74) for the Grafana charts
+- End-to-end feature tests for (at the minimum) publishing and installing a package
+
 ### Future Work
 
 While working on the MVC, contributors might find features that are not mandatory for the MVC but can provide a better user experience. It's generally a good idea to keep an eye on those and open issues.

doc/user/group/settings/import_export.md

@@ -109,6 +109,14 @@ on an existing group's page.
 
 ## Version history
 
+### 14.0+
+
+In GitLab 14.0, the JSON format is no longer supported for project and group exports. To allow for a
+transitional period, you can still import any JSON exports. The new format for imports and exports
+is NDJSON.
+
+### 13.0+
+
 GitLab can import bundles that were exported from a different GitLab deployment.
 This ability is limited to two previous GitLab [minor](../../../policy/maintenance.md#versioning)
 releases, which is similar to our process for [Security Releases](../../../policy/maintenance.md#security-releases).

doc/user/project/settings/import_export.md

@@ -57,6 +57,12 @@ Note the following:
 
 ## Version history
 
+### 14.0+
+
+In GitLab 14.0, the JSON format is no longer supported for project and group exports. To allow for a
+transitional period, you can still import any JSON exports. The new format for imports and exports
+is NDJSON.
+
 ### 13.0+
 
 Starting with GitLab 13.0, GitLab can import bundles that were exported from a different GitLab deployment.

lib/gitlab/spamcheck/client.rb

@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+require 'spamcheck'
+
+module Gitlab
+  module Spamcheck
+    class Client
+      include ::Spam::SpamConstants
+      DEFAULT_TIMEOUT_SECS = 2
+
+      VERDICT_MAPPING = {
+        ::Spamcheck::SpamVerdict::Verdict::ALLOW => ALLOW,
+        ::Spamcheck::SpamVerdict::Verdict::CONDITIONAL_ALLOW => CONDITIONAL_ALLOW,
+        ::Spamcheck::SpamVerdict::Verdict::DISALLOW => DISALLOW,
+        ::Spamcheck::SpamVerdict::Verdict::BLOCK => BLOCK_USER
+      }.freeze
+
+      ACTION_MAPPING = {
+        create: ::Spamcheck::Action::CREATE,
+        update: ::Spamcheck::Action::UPDATE
+      }.freeze
+
+      def initialize
+        @endpoint_url = Gitlab::CurrentSettings.current_application_settings.spam_check_endpoint_url
+
+        # remove the `grpc://` as it's only useful to ensure we're expecting to
+        # connect with Spamcheck
+        @endpoint_url = @endpoint_url.gsub(%r(^grpc:\/\/), '')
+
+        creds =
+          if Rails.env.development? || Rails.env.test?
+            :this_channel_is_insecure
+          else
+            GRPC::Core::ChannelCredentials.new
+          end
+
+        @stub = ::Spamcheck::SpamcheckService::Stub.new(@endpoint_url, creds,
+                                                        timeout: DEFAULT_TIMEOUT_SECS)
+      end
+
+      def issue_spam?(spam_issue:, user:, context: {})
+        issue = build_issue_protobuf(issue: spam_issue, user: user, context: context)
+
+        response = @stub.check_for_spam_issue(issue,
+                                              metadata: { 'authorization' =>
+                                                           Gitlab::CurrentSettings.spam_check_api_key })
+        verdict = convert_verdict_to_gitlab_constant(response.verdict)
+        [verdict, response.error]
+      end
+
+      private
+
+      def convert_verdict_to_gitlab_constant(verdict)
+        VERDICT_MAPPING.fetch(::Spamcheck::SpamVerdict::Verdict.resolve(verdict), verdict)
+      end
+
+      def build_issue_protobuf(issue:, user:, context:)
+        issue_pb = ::Spamcheck::Issue.new
+        issue_pb.title = issue.spam_title || ''
+        issue_pb.description = issue.spam_description || ''
+        issue_pb.created_at = convert_to_pb_timestamp(issue.created_at) if issue.created_at
+        issue_pb.updated_at = convert_to_pb_timestamp(issue.updated_at) if issue.updated_at
+        issue_pb.user_in_project = user.authorized_project?(issue.project)
+        issue_pb.action = ACTION_MAPPING.fetch(context.fetch(:action)) if context.has_key?(:action)
+        issue_pb.user = build_user_protobuf(user)
+        issue_pb
+      end
+
+      def build_user_protobuf(user)
+        user_pb = ::Spamcheck::User.new
+        user_pb.username = user.username
+        user_pb.org = user.organization || ''
+        user_pb.created_at = convert_to_pb_timestamp(user.created_at)
+
+        user_pb.emails << build_email(user.email, user.confirmed?)
+
+        user.emails.each do |email|
+          user_pb.emails << build_email(email.email, email.confirmed?)
+        end
+
+        user_pb
+      end
+
+      def build_email(email, verified)
+        email_pb = ::Spamcheck::User::Email.new
+        email_pb.email = email
+        email_pb.verified = verified
+        email_pb
+      end
+
+      def convert_to_pb_timestamp(ar_timestamp)
+        Google::Protobuf::Timestamp.new(seconds: ar_timestamp.to_time.to_i,
+                                        nanos: ar_timestamp.to_time.nsec)
+      end
+    end
+  end
+end

spec/features/admin/admin_settings_spec.rb

@@ -425,7 +425,7 @@ RSpec.describe 'Admin updates settings' do
           check 'Enable reCAPTCHA for login'
           fill_in 'IPs per user', with: 15
           check 'Enable Spam Check via external API endpoint'
-          fill_in 'URL of the external Spam Check endpoint', with: 'https://www.example.com/spamcheck'
+          fill_in 'URL of the external Spam Check endpoint', with: 'grpc://www.example.com/spamcheck'
           fill_in 'Spam Check API Key', with: 'SPAM_CHECK_API_KEY'
           click_button 'Save changes'
         end
@@ -435,7 +435,7 @@ RSpec.describe 'Admin updates settings' do
         expect(current_settings.login_recaptcha_protection_enabled).to be true
         expect(current_settings.unique_ips_limit_per_user).to eq(15)
         expect(current_settings.spam_check_endpoint_enabled).to be true
-        expect(current_settings.spam_check_endpoint_url).to eq 'https://www.example.com/spamcheck'
+        expect(current_settings.spam_check_endpoint_url).to eq 'grpc://www.example.com/spamcheck'
       end
     end
 

spec/lib/gitlab/spamcheck/client_spec.rb

@@ -0,0 +1,103 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Spamcheck::Client do
+  include_context 'includes Spam constants'
+
+  let(:endpoint) { 'grpc://grpc.test.url' }
+  let_it_be(:user) { create(:user, organization: 'GitLab') }
+  let(:verdict_value) { nil }
+  let(:error_value) { "" }
+  let_it_be(:issue) { create(:issue, description: 'Test issue description') }
+
+  let(:response) do
+    verdict = ::Spamcheck::SpamVerdict.new
+    verdict.verdict = verdict_value
+    verdict.error = error_value
+    verdict
+  end
+
+  subject { described_class.new.issue_spam?(spam_issue: issue, user: user) }
+
+  before do
+    stub_application_setting(spam_check_endpoint_url: endpoint)
+  end
+
+  describe '#issue_spam?' do
+    before do
+      allow_next_instance_of(::Spamcheck::SpamcheckService::Stub) do |instance|
+        allow(instance).to receive(:check_for_spam_issue).and_return(response)
+      end
+    end
+
+    using RSpec::Parameterized::TableSyntax
+
+    where(:verdict, :expected) do
+      ::Spamcheck::SpamVerdict::Verdict::ALLOW                | Spam::SpamConstants::ALLOW
+      ::Spamcheck::SpamVerdict::Verdict::CONDITIONAL_ALLOW    | Spam::SpamConstants::CONDITIONAL_ALLOW
+      ::Spamcheck::SpamVerdict::Verdict::DISALLOW             | Spam::SpamConstants::DISALLOW
+      ::Spamcheck::SpamVerdict::Verdict::BLOCK                | Spam::SpamConstants::BLOCK_USER
+    end
+
+    with_them do
+      let(:verdict_value) { verdict }
+
+      it "returns expected spam constant" do
+        expect(subject).to eq([expected, ""])
+      end
+    end
+  end
+
+  describe "#build_issue_protobuf", :aggregate_failures do
+    it 'builds the expected protobuf object' do
+      cxt = { action: :create }
+      issue_pb = described_class.new.send(:build_issue_protobuf,
+                                          issue: issue, user: user,
+                                          context: cxt)
+      expect(issue_pb.title).to eq issue.title
+      expect(issue_pb.description).to eq issue.description
+      expect(issue_pb.user_in_project). to be false
+      expect(issue_pb.created_at).to eq timestamp_to_protobuf_timestamp(issue.created_at)
+      expect(issue_pb.updated_at).to eq timestamp_to_protobuf_timestamp(issue.updated_at)
+      expect(issue_pb.action).to be ::Spamcheck::Action.lookup(::Spamcheck::Action::CREATE)
+      expect(issue_pb.user.username).to eq user.username
+    end
+  end
+
+  describe '#build_user_proto_buf', :aggregate_failures do
+    it 'builds the expected protobuf object' do
+      user_pb = described_class.new.send(:build_user_protobuf, user)
+      expect(user_pb.username).to eq user.username
+      expect(user_pb.org).to eq user.organization
+      expect(user_pb.created_at).to eq timestamp_to_protobuf_timestamp(user.created_at)
+      expect(user_pb.emails.count).to be 1
+      expect(user_pb.emails.first.email).to eq user.email
+      expect(user_pb.emails.first.verified).to eq user.confirmed?
+    end
+
+    context 'when user has multiple email addresses' do
+      let(:secondary_email) {create(:email, :confirmed, user: user)}
+
+      before do
+        user.emails << secondary_email
+      end
+
+      it 'adds emsils to the user pb object' do
+        user_pb = described_class.new.send(:build_user_protobuf, user)
+        expect(user_pb.emails.count).to eq 2
+        expect(user_pb.emails.first.email).to eq user.email
+        expect(user_pb.emails.first.verified).to eq user.confirmed?
+        expect(user_pb.emails.last.email).to eq secondary_email.email
+        expect(user_pb.emails.last.verified).to eq secondary_email.confirmed?
+      end
+    end
+  end
+
+  private
+
+  def timestamp_to_protobuf_timestamp(timestamp)
+    Google::Protobuf::Timestamp.new(seconds: timestamp.to_time.to_i,
+                                    nanos: timestamp.to_time.nsec)
+  end
+end

spec/models/application_setting_spec.rb

@@ -216,7 +216,8 @@ RSpec.describe ApplicationSetting do
           setting.spam_check_endpoint_enabled = true
         end
 
-        it { is_expected.to allow_value('https://example.org/spam_check').for(:spam_check_endpoint_url) }
+        it { is_expected.to allow_value('grpc://example.org/spam_check').for(:spam_check_endpoint_url) }
+        it { is_expected.not_to allow_value('https://example.org/spam_check').for(:spam_check_endpoint_url) }
         it { is_expected.not_to allow_value('nonsense').for(:spam_check_endpoint_url) }
         it { is_expected.not_to allow_value(nil).for(:spam_check_endpoint_url) }
         it { is_expected.not_to allow_value('').for(:spam_check_endpoint_url) }
@@ -227,7 +228,8 @@ RSpec.describe ApplicationSetting do
           setting.spam_check_endpoint_enabled = false
         end
 
-        it { is_expected.to allow_value('https://example.org/spam_check').for(:spam_check_endpoint_url) }
+        it { is_expected.to allow_value('grpc://example.org/spam_check').for(:spam_check_endpoint_url) }
+        it { is_expected.not_to allow_value('https://example.org/spam_check').for(:spam_check_endpoint_url) }
         it { is_expected.not_to allow_value('nonsense').for(:spam_check_endpoint_url) }
         it { is_expected.to allow_value(nil).for(:spam_check_endpoint_url) }
         it { is_expected.to allow_value('').for(:spam_check_endpoint_url) }

spec/requests/api/settings_spec.rb

@@ -123,7 +123,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
             issues_create_limit: 300,
             raw_blob_request_limit: 300,
             spam_check_endpoint_enabled: true,
-            spam_check_endpoint_url: 'https://example.com/spam_check',
+            spam_check_endpoint_url: 'grpc://example.com/spam_check',
             spam_check_api_key: 'SPAM_CHECK_API_KEY',
             disabled_oauth_sign_in_sources: 'unknown',
             import_sources: 'github,bitbucket',
@@ -169,7 +169,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
         expect(json_response['issues_create_limit']).to eq(300)
         expect(json_response['raw_blob_request_limit']).to eq(300)
         expect(json_response['spam_check_endpoint_enabled']).to be_truthy
-        expect(json_response['spam_check_endpoint_url']).to eq('https://example.com/spam_check')
+        expect(json_response['spam_check_endpoint_url']).to eq('grpc://example.com/spam_check')
         expect(json_response['spam_check_api_key']).to eq('SPAM_CHECK_API_KEY')
         expect(json_response['disabled_oauth_sign_in_sources']).to eq([])
         expect(json_response['import_sources']).to match_array(%w(github bitbucket))

spec/services/spam/spam_action_service_spec.rb

@@ -9,11 +9,11 @@ RSpec.describe Spam::SpamActionService do
   let(:issue) { create(:issue, project: project, author: user) }
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
-  let(:fake_referrer) { 'fake-http-referrer' }
+  let(:fake_referer) { 'fake-http-referer' }
   let(:env) do
     { 'action_dispatch.remote_ip' => fake_ip,
       'HTTP_USER_AGENT' => fake_user_agent,
-      'HTTP_REFERRER' => fake_referrer }
+      'HTTP_REFERER' => fake_referer }
   end
 
   let_it_be(:project) { create(:project, :public) }
@@ -80,7 +80,7 @@ RSpec.describe Spam::SpamActionService do
       {
         ip_address: fake_ip,
         user_agent: fake_user_agent,
-        referrer: fake_referrer
+        referer: fake_referer
       }
     end
 
@@ -222,6 +222,38 @@ RSpec.describe Spam::SpamActionService do
           end
         end
 
+        context 'spam verdict service advises to block the user' do
+          before do
+            allow(fake_verdict_service).to receive(:execute).and_return(BLOCK_USER)
+          end
+
+          context 'when allow_possible_spam feature flag is false' do
+            before 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
+
+              expect(response.message).to match(expected_service_check_response_message)
+              expect(issue).to be_spam
+            end
+          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
+
+              expect(response.message).to match(expected_service_check_response_message)
+              expect(issue).not_to be_spam
+            end
+          end
+        end
+
         context 'when spam verdict service conditionally allows' do
           before do
             allow(fake_verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW)

spec/services/spam/spam_verdict_service_spec.rb

@@ -7,18 +7,18 @@ RSpec.describe Spam::SpamVerdictService do
 
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
-  let(:fake_referrer) { 'fake-http-referrer' }
+  let(:fake_referer) { 'fake-http-referer' }
   let(:env) do
     { 'action_dispatch.remote_ip' => fake_ip,
       'HTTP_USER_AGENT' => fake_user_agent,
-      'HTTP_REFERRER' => fake_referrer }
+      'HTTP_REFERER' => fake_referer }
   end
 
   let(:request) { double(:request, env: env) }
 
   let(:check_for_spam) { true }
   let_it_be(:user) { create(:user) }
-  let(:issue) { build(:issue, author: user) }
+  let_it_be(:issue) { create(:issue, author: user) }
   let(:service) do
     described_class.new(user: user, target: issue, request: request, options: {})
   end
@@ -28,7 +28,7 @@ RSpec.describe Spam::SpamVerdictService do
 
     before do
       allow(service).to receive(:akismet_verdict).and_return(nil)
-      allow(service).to receive(:external_verdict).and_return(nil)
+      allow(service).to receive(:spamcheck_verdict).and_return(nil)
     end
 
     context 'if all services return nil' do
@@ -63,7 +63,7 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and they are supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return(DISALLOW)
-          allow(service).to receive(:external_verdict).and_return(BLOCK_USER)
+          allow(service).to receive(:spamcheck_verdict).and_return(BLOCK_USER)
         end
 
         it 'renders the more restrictive verdict' do
@@ -74,7 +74,7 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and one is supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return('nonsense')
-          allow(service).to receive(:external_verdict).and_return(BLOCK_USER)
+          allow(service).to receive(:spamcheck_verdict).and_return(BLOCK_USER)
         end
 
         it 'renders the more restrictive verdict' do
@@ -85,7 +85,7 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and none are supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return('nonsense')
-          allow(service).to receive(:external_verdict).and_return('rubbish')
+          allow(service).to receive(:spamcheck_verdict).and_return('rubbish')
         end
 
         it 'renders the more restrictive verdict' do
@@ -150,51 +150,90 @@ RSpec.describe Spam::SpamVerdictService do
     end
   end
 
-  describe '#external_verdict' do
+  describe '#spamcheck_verdict' do
-    subject { service.send(:external_verdict) }
+    subject { service.send(:spamcheck_verdict) }
 
     context 'if a Spam Check endpoint enabled and set to a URL' do
       let(:spam_check_body) { {} }
-      let(:spam_check_http_status) { nil }
+      let(:endpoint_url) { "grpc://www.spamcheckurl.com/spam_check" }
+
+      let(:spam_client) do
+        Gitlab::Spamcheck::Client.new
+      end
 
       before do
         stub_application_setting(spam_check_endpoint_enabled: true)
-        stub_application_setting(spam_check_endpoint_url: "http://www.spamcheckurl.com/spam_check")
+        stub_application_setting(spam_check_endpoint_url: endpoint_url)
-        stub_request(:post, /.*spamcheckurl.com.*/).to_return( body: spam_check_body.to_json, status: spam_check_http_status )
       end
 
       context 'if the endpoint is accessible' do
-        let(:spam_check_http_status) { 200 }
+        let(:error) { '' }
-        let(:error) { nil }
         let(:verdict) { nil }
-        let(:spam_check_body) do
+
-          { verdict: verdict, error: error }
+        before do
+          allow(service).to receive(:spamcheck_client).and_return(spam_client)
+          allow(spam_client).to receive(:issue_spam?).and_return([verdict, error])
         end
 
         context 'the result is a valid verdict' do
-          let(:verdict) { 'allow' }
+          let(:verdict) { ALLOW }
 
           it 'returns the verdict' do
             expect(subject).to eq ALLOW
           end
         end
 
-        context 'the verdict is an unexpected string' do
+        context 'when recaptcha is enabled' do
-          let(:verdict) { 'this is fine' }
+          before do
+            allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(true)
+          end
+
+          using RSpec::Parameterized::TableSyntax
+
+          # rubocop: disable Lint/BinaryOperatorWithIdenticalOperands
+          where(:verdict_value, :expected) do
+            ::Spam::SpamConstants::ALLOW               | ::Spam::SpamConstants::ALLOW
+            ::Spam::SpamConstants::CONDITIONAL_ALLOW   | ::Spam::SpamConstants::CONDITIONAL_ALLOW
+            ::Spam::SpamConstants::DISALLOW            | ::Spam::SpamConstants::CONDITIONAL_ALLOW
+            ::Spam::SpamConstants::BLOCK_USER          | ::Spam::SpamConstants::CONDITIONAL_ALLOW
+          end
+          # rubocop: enable Lint/BinaryOperatorWithIdenticalOperands
+
+          with_them do
+            let(:verdict) { verdict_value }
+
+            it "returns expected spam constant" do
+              expect(subject).to eq(expected)
+            end
+          end
+        end
+
+        context 'when recaptcha is disabled' do
+          before do
+            allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(false)
+          end
+
+          [::Spam::SpamConstants::ALLOW,
+           ::Spam::SpamConstants::CONDITIONAL_ALLOW,
+           ::Spam::SpamConstants::DISALLOW,
+           ::Spam::SpamConstants::BLOCK_USER].each do |verdict_value|
+            let(:verdict) { verdict_value }
+            let(:expected) { verdict_value }
+
+            it "returns expected spam constant" do
+              expect(subject).to eq(expected)
+            end
+          end
+        end
+
+        context 'the verdict is an unexpected value' do
+          let(:verdict) { :this_is_fine }
 
           it 'returns the string' do
             expect(subject).to eq verdict
           end
         end
 
-        context 'the JSON is malformed' do
-          let(:spam_check_body) { 'this is fine' }
-
-          it 'returns allow' do
-            expect(subject).to eq ALLOW
-          end
-        end
-
         context 'the verdict is an empty string' do
           let(:verdict) { '' }
 
@@ -219,11 +258,13 @@ RSpec.describe Spam::SpamVerdictService do
           end
         end
 
-        context 'the HTTP status is not 200' do
+        context 'the requested is aborted' do
-          let(:spam_check_http_status) { 500 }
+          before do
+            allow(spam_client).to receive(:issue_spam?).and_raise(GRPC::Aborted)
+          end
 
           it 'returns nil' do
-            expect(subject).to be_nil
+            expect(subject).to be(ALLOW)
           end
         end
 
@@ -239,11 +280,11 @@ RSpec.describe Spam::SpamVerdictService do
 
       context 'if the endpoint times out' do
         before do
-          stub_request(:post, /.*spamcheckurl.com.*/).to_timeout
+          allow(spam_client).to receive(:issue_spam?).and_raise(GRPC::DeadlineExceeded)
         end
 
         it 'returns nil' do
-          expect(subject).to be_nil
+          expect(subject).to be(ALLOW)
         end
       end
     end