Skip to content

Draft: Refactor common reCAPTCHA logic to be usable from services

Chad Woolley requested to merge refactor-out-common-recaptcha-logic into refactor-spam-action-service

What does this MR do?

UPDATE - this MR should not be needed, almost all of its intended changes are no longer required after Refactor SpamActionService and its usage

NOTE: This MR depends on !51840 (merged), and is rebased against its branch, BRANCH: refactor-spam-action-service

Overview

Refactor common reCAPTCHA logic out of spammable_actions.rb to be usable from service layer.

Adds support which will be used in subsequent MRs to implement the reCAPTCHA workflow via GraphQL.

Also backfills missing unit tests from the modified concerns.

See #217722 (closed) for an issue with full context on all planned implementation MRs.

See !50559 (closed) for a spike/Proof of Concept showing a full working implementation of the new reCAPTCHA GraphQL support.

Details

Some of the current logic to support GraphQL is in spammable_actions.rb, which is a concern specific to the REST/Controller layer.

This MR extracts that logic to a common sharable concern, and adds a return value which will be used to communicate necessary spam/recaptcha information to the calling services in the service layer.

Tasks

  • Backfill missing unit tests for spammable_actions.rb
  • Backfill missing unit tests for spam_action_service.rb
  • Add new app/services/concerns/spam_recaptcha_support.rb concern
  • Move the #ensure_spam_config_loaded! method to the new concern
  • Move the direct call to the recaptcha gem's #verify_recaptcha to the new concern
  • Move the #render_recaptcha? method to the new concern.
  • Add support for handling the (currently unused) @recaptcha_response to filter_spam_check_params
  • Add a (currently unused) hash return value to SpamCheckMethods#spam_check, which contains fields which will be necessary for the service-layer/GraphQL-based reCAPTCHA workflow.
  • Move logic from SpamActionService constructor to execute method, so that it can be more easily mocked via expect_next_instance_of.

Exploratory Testing

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

UI

  • Issue create without recaptcha
  • Issue create with recaptcha
  • Issue update without recaptcha
  • Issue update with recaptcha

REST API

  • Issue create without recaptcha
  • Issue create with recaptcha (FAILS, currently unsupported)
  • Issue update without recaptcha
  • Issue update with recaptcha (FAILS, currently unsupported)

GraphQL API

  • Issue create without recaptcha
  • Issue create with recaptcha (Not possible (?), never flagged as spam because service doesn't set request in params - asked for confirmation in Slack)
  • Issue update without recaptcha
  • Issue update with recaptcha (Not possible (?), never flagged as spam because service doesn't set request in params - asked for confirmation in Slack)

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

Related Issues

Edited by Chad Woolley

Merge request reports