Skip to content

Refactor SpamActionService and its usage

Chad Woolley requested to merge refactor-spam-action-service into master

What does this MR do?

  • Removes SpamCheckMethods and makes spam-checking workflow explicit in callers
  • Moves captcha verification to be encapsulated in SpamActionService
  • Introduces CaptchaVerificationService, which can be expanded upon on the future for additional captcha implementations.
  • Moves handling of request parameter to be called only where it is needed.
  • Removes memoization of GitLab::Recaptcha.load_configurations! - it complicates testing/mocking, and it isn't clear why it was ever needed - it isn't memoized other places.
  • Rewrites issue and snippet service create/update tests to remove tests of internal behavior of other services, and instead just test their collaborations via mock expectations.
  • Expands and improves test coverage of SpamActionService
  • Other related changes to accomodate new interface and behavior.

NOTE: This MR depends on !51587 (merged), and is rebased against its branch, BRANCH: remove-recaptcha-verification-param

Why is this needed?

1. This has already been identified as an area needing improvement:

See this comment getting consensus that this needs to be done: !51301 (comment 484101503)

Text copied from that comment:

"Here's a bad example which we could rewrite" 
Here's a bad example which we could rewrite:

``` ruby
module SpamCheckService
  def filter_spam_check_params
    @request            = params.delete(:request)
    @api                = params.delete(:api)
    @recaptcha_verified = params.delete(:recaptcha_verified)
    @spam_log_id        = params.delete(:spam_log_id)
  end

  def spam_check(spammable, user)
    spam_service = SpamService.new(spammable, @request)

    spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
      user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
    end
  end
end

There are several implicit dependencies here. First, params should be defined before use. Second, filter_spam_check_params should be called before spam_check. These are all implicit and the includer could be using those instance variables without awareness.

This should be rewritten like:

class SpamCheckService
  def initialize(request:, api:, recaptcha_verified:, spam_log_id:)
    @request            = request
    @api                = api
    @recaptcha_verified = recaptcha_verified
    @spam_log_id        = spam_log_id
  end

  def spam_check(spammable, user)
    spam_service = SpamService.new(spammable, @request)

    spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
      user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
    end
  end
end

And use it like:

class UpdateSnippetService < BaseService
  def execute
    # ...
    spam = SpamCheckService.new(params.slice!(:request, :api, :recaptcha_verified, :spam_log_id))

    spam.check(snippet, current_user)
    # ...
  end
end

This way, all those instance variables are isolated in SpamCheckService rather than whatever includes the module, and those modules which were also included, making it much easier to track down any issues, and reducing the chance of having name conflicts.

I agree, and it makes sense to do this refactor before we start modifying spam_action_service.rb and its callers further, especially since it will provide a stronger interface contract and more type safety.

So, I'm going to put this MR back into draft mode (and withdraw @caalberts review request), and open a separate MR for that refactor first and get rid of the rubocop:disable Gitlab/ModuleWithInstanceVariables.

Also, while reading that docs page, I found these issues regarding architectural decisions, which I agree with, and will keep in mind as part of these refactors.

  1. Consider replacing concerns with dedicated classes & composition
  2. Use decorators and interface segregation to solve overgrowing models problem

This continues previous spam-related refactoring efforts:

This is continuing a succession of refactors to clean up and better abstract spam and recaptcha related logic:

  1. 2ace39f2
  2. https://gitlab.com/gitlab-org/gitlab/-/issues/217110
  3. And many others...

It will make refactors to support modern GraphQL/Vue support easier to understand and implement, and require less abstration/duplication:

Much of the work on this issue will be made easier and more straightforward by this refactor.

Cleanup and standardization will help support FriendlyCaptcha:

These refactors, especially the introduction of CaptchaVerificationService, set up a pattern for abstracting access to multiple different Captcha implementations.

Makes flow to handle captcha verification and spam checking explicit and easier to understand:

This is the flow which is necessary for all models which rely on captcha and spam checking for create/update requests, and this refactor abstracts it and makes it explicit:

  1. receive a request
  2. call filter_spam_params! to remove SpamParams (from request params or graphql fields) and keep a reference to them for later.
  3. create new unsaved model / or update attributes on existing model without saving it
  4. execute spam check by invoking SpamActionService#execute, passing dirty/unsaved model and other necessary values (SpamParams and request)

Implementation Notes

  • Static method filter_spam_params! on service.
  • Caller saves and holds on to SpamParams
  • Redo existing snippet MRs to transmit spam info via model instead of encapsulating in service.

Questions

  • For Service classes in app/services, what is the philosophy for passing values via the constructor vs. passing them directly as parameters to the execute method?
  • What's up with the request option SpamActionService and SpamVerdictService?
    • Why does absence of a request abort the check in SpamActionService#execute?
    • And this means that the fallback to assign :ip_address and :user_agent is never used.
    • And even if request does exist, it is passed to SpamVerdictService but never used there.

Exploratory Testing

NOTE: Ensure the snippet_spam feature flag is turned OFF - that feature is not yet fully implemented.

See instructions for testing reCAPTCHA in Testing Notes section of issue: #217722 (closed)

UI

  • Issue create without akismet+recaptcha
  • Issue create with akismet+recaptcha
  • Issue update without akismet+recaptcha
  • Issue update with akismet+recaptcha
  • Snippet create without akismet+recaptcha
  • [-] Snippet create with rakismet+recaptcha (currently unsupported)
  • Snippet update without akismet+recaptcha
  • [-] Snippet update with akismet+recaptcha (currently unsupported)

REST API

  • Issue create without akismet+recaptcha
  • [-] Issue create with akismet+recaptcha (currently unsupported)
  • Issue update without rakismet+ecaptcha
  • [-] Issue update with akismet+recaptcha (currently unsupported)
  • Snippet create without akismet+recaptcha
  • [-] Snippet create with akismet+recaptcha (currently unsupported)
  • Snippet update without akismet+recaptcha
  • [-] Snippet update with akismet+recaptcha (currently unsupported)

GraphQL API

  • Issue create without akismet+recaptcha
  • [-] Issue create with akismet+recaptcha (Not possible (?), never flagged as spam because service doesn't set request in params - asked for confirmation in Slack)
  • Issue update without akismet+recaptcha
  • [-] Issue update with akismet+recaptcha (Not possible (?), never flagged as spam because service doesn't set request in params - asked for confirmation in Slack)
  • Snippet create without akismet+recaptcha
  • [-] Snippet create with akismet+recaptcha (currently unsupported)
  • Snippet update without rakismet+recaptcha
  • [-] Snippet update with akismet+recaptcha (currently unsupported)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team

Follow-up MRs

  • (TODO: create) Rename SpamLog.verify_recaptcha! to SpamLog.recaptcha_verified! (to match attribute, and also because technically it's already been verified by the time this is called).
  • (TODO: create) Document the context argument in SpamActionService and SpamVerdictService to make it clear that it contains context for the request to the spam check API endpoint.
  • (TODO: create) Change other direct calls to verify_recaptcha via gem mixin/concern to instead call Captcha::CaptchaVerificationService. See Consider replacing concerns with dedicated classes & composition for motivation.
  • (TODO: create) Handling of request parameter.

Related Issues

Edited by Chad Woolley

Merge request reports