diff options
Diffstat (limited to 'app/services/spam/spam_action_service.rb')
| -rw-r--r-- | app/services/spam/spam_action_service.rb | 96 |
1 files changed, 30 insertions, 66 deletions
diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index 3ae5111b994..ec16ce19cf6 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -4,67 +4,22 @@ 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). The parameters will either be camelCase (if they are - # received directly via controller params) or underscore_case (if they have come from - # a GraphQL mutation which has converted them to underscore), or in the - # headers when using the header based flow. - # - # 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, request) - # 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 - headers = request&.headers || {} - api = params.delete(:api) - captcha_response = read_parameter(:captcha_response, params, headers) - spam_log_id = read_parameter(:spam_log_id, params, headers)&.to_i - - SpamParams.new(api: api, captcha_response: captcha_response, spam_log_id: spam_log_id) - end - - def self.read_parameter(name, params, headers) - [ - params.delete(name), - params.delete(name.to_s.camelize(:lower).to_sym), - headers["X-GitLab-#{name.to_s.titlecase(keep_id_suffix: true).tr(' ', '-')}"] - ].compact.first - end - - attr_accessor :target, :request, :options - attr_reader :spam_log - - def initialize(spammable:, request:, user:, action:) + def initialize(spammable:, spam_params:, user:, action:) @target = spammable - @request = request + @spam_params = spam_params @user = user @action = action - @options = {} end # rubocop:disable Metrics/AbcSize - 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[: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? - options[:ip_address] = target.ip_address - options[:user_agent] = target.user_agent - end + def execute + # If spam_params is passed as `nil`, no check will be performed. This is the easiest way to allow + # composed services which may not need to do spam checking to "opt out". For example, when + # MoveService is calling CreateService, spam checking is not necessary, as no new content is + # being created. + return ServiceResponse.success(message: 'Skipped spam check because spam_params was not present') unless spam_params - recaptcha_verified = Captcha::CaptchaVerificationService.new.execute( - captcha_response: spam_params.captcha_response, - request: request - ) + recaptcha_verified = Captcha::CaptchaVerificationService.new(spam_params: spam_params).execute if recaptcha_verified # If it's a request which is already verified through CAPTCHA, @@ -73,10 +28,9 @@ module Spam ServiceResponse.success(message: "CAPTCHA successfully verified") else 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(spam_params.api) + perform_spam_service_check ServiceResponse.success(message: "Spam check performed. Check #{target.class.name} spammable model for any errors or CAPTCHA requirement") end end @@ -86,7 +40,7 @@ module Spam private - attr_reader :user, :action + attr_reader :user, :action, :target, :spam_params, :spam_log ## # In order to be proceed to the spam check process, the target must be @@ -104,7 +58,7 @@ module Spam ## # 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) + def perform_spam_service_check ensure_target_is_dirty # since we can check for spam, and recaptcha is not verified, @@ -113,7 +67,7 @@ module Spam case result when CONDITIONAL_ALLOW # at the moment, this means "ask for reCAPTCHA" - create_spam_log(api) + create_spam_log break if target.allow_possible_spam? @@ -122,12 +76,12 @@ module Spam # 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) + create_spam_log 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) + create_spam_log when ALLOW target.clear_spam_flags! when NOOP @@ -137,16 +91,21 @@ module Spam end end - def create_spam_log(api) + def create_spam_log @spam_log = SpamLog.create!( { user_id: target.author_id, title: target.spam_title, description: target.spam_description, - source_ip: options[:ip_address], - user_agent: options[:user_agent], + source_ip: spam_params.ip_address, + user_agent: spam_params.user_agent, noteable_type: noteable_type, - via_api: api + # Now, all requests are via the API, so hardcode it to true to simplify the logic and API + # of this service. See https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2266 + # for original introduction of `via_api` field. + # See discussion here about possibly deprecating this field: + # https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2266#note_542527450 + via_api: true } ) @@ -159,9 +118,14 @@ module Spam target_type: noteable_type } + options = { + ip_address: spam_params.ip_address, + user_agent: spam_params.user_agent, + referer: spam_params.referer + } + SpamVerdictService.new(target: target, user: user, - request: request, options: options, context: context) end |