diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-01-27 12:09:01 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-01-27 12:09:01 +0300 |
commit | 6e7dc3f9d6b95200abd1fcd5cc418a9ae9803e4c (patch) | |
tree | 299cedc6760e39379176bb73a08ecf2352368305 /app/services/spam | |
parent | c78a5f67d0bf347da8a486d72515478e20f15d6e (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'app/services/spam')
-rw-r--r-- | app/services/spam/spam_action_service.rb | 93 | ||||
-rw-r--r-- | app/services/spam/spam_params.rb | 32 |
2 files changed, 104 insertions, 21 deletions
diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index b3d617256ff..ff32bc32d93 100644 --- a/app/services/spam/spam_action_service.rb +++ b/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 diff --git a/app/services/spam/spam_params.rb b/app/services/spam/spam_params.rb new file mode 100644 index 00000000000..fef5355c7f3 --- /dev/null +++ b/app/services/spam/spam_params.rb @@ -0,0 +1,32 @@ +# 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 |