diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-05-01 03:10:00 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-05-01 03:10:00 +0300 |
commit | 4601b44af1e341c3d00317ef1cfbbd909cd53070 (patch) | |
tree | a2d2b11f4138ba093739e6b29f4c33902871857d /app/services/spam | |
parent | 07de48228886ca6afa348c92f8689b9027e4c195 (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'app/services/spam')
-rw-r--r-- | app/services/spam/akismet_service.rb | 2 | ||||
-rw-r--r-- | app/services/spam/spam_action_service.rb | 7 | ||||
-rw-r--r-- | app/services/spam/spam_verdict_service.rb | 79 |
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 |