Add latest changes from gitlab-org/gitlab@master

3 files changed, 46 insertions, 42 deletions

diff --git a/app/services/spam/akismet_service.rb b/app/services/spam/akismet_service.rb
index 75673518b48..4e56972ccd5 100644
--- a/app/services/spam/akismet_service.rb
+++ b/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
diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb
index 2220198583c..1cd432e1f66 100644
--- a/app/services/spam/spam_action_service.rb
+++ b/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
diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb
index 2463a5227d0..8dbc590314c 100644
--- a/app/services/spam/spam_verdict_service.rb
+++ b/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 metrics/logging
-        # Expecting:
-        # error: (string or nil)
-        # verdict: (string or nil)
-        # @TODO log if json_result[:error]
+        # @TODO log if error is not nil https://gitlab.com/gitlab-org/gitlab/-/issues/329545
 
-        json_result[:verdict]
-      rescue *Gitlab::HTTP::HTTP_ERRORS => e
-        # @TODO: log error via try_post https://gitlab.com/gitlab-org/gitlab/-/issues/219223
+        # Duplicate logic with Akismet logic in #akismet_verdict
+        if Gitlab::Recaptcha.enabled? && result != ALLOW
+          CONDITIONAL_ALLOW
+        else
+          result
+        end
+      rescue StandardError => e
         Gitlab::ErrorTracking.log_exception(e)
-        nil
-      rescue StandardError
-        # @TODO log
+        # Default to ALLOW if any errors occur
         ALLOW
       end
     end
 
-    def assemble_verdict_params(context)
-      return {} unless endpoint_url.present?
-
-      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)
-      })
+    def spamcheck_client
+      @spamcheck_client ||= Gitlab::Spamcheck::Client.new
     end
 
-    def endpoint_url
-      @endpoint_url ||= Gitlab::CurrentSettings.current_application_settings.spam_check_endpoint_url
+    def logger
+      @logger ||= Gitlab::AppJsonLogger.build
     end
   end
 end