Skip to content

Remove unnecessary recaptcha verification param and unused snippets verify views

Chad Woolley requested to merge remove-recaptcha-verification-param into master

What does this MR do?

Overview

  • Refactoring and cleanup related to subsequent MRs to implement the reCAPTCHA workflow via GraphQL.
    • Removes unnecessary recaptcha_verification parameter, and instead relies on the presence of the recaptcha response parameter. The following note in the code explains in more detail:
  def recaptcha_response
    # NOTE: This field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the recaptcha
    # gem, which is called from the HAML `_recaptcha_form.html.haml` form.
    #
    # It is used in the `Recaptcha::Verify#verify_recaptcha` if the `response` option is not
    # passed explicitly.
    #
    # Instead of relying on this behavior, we are extracting and passing it explicitly. This will
    # make it consistent with the newer, modern reCAPTCHA verification process as it will be
    # implemented via the GraphQL API and in Vue components via the native reCAPTCHA Javascript API,
    # which requires that the recaptcha response param be obtained and passed explicitly.
    #
    # After this newer GraphQL/JS API process is fully supported by the backend, we can remove this
    # (and other) HAML-specific support.
    params['g-recaptcha-response']
  end
  • Backfills some missing unit tests from spammable_actions.rb
  • Also deletes two unused snippets verify views which should have been deleted previously as part of !44718 (merged).

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

Exploratory Testing

See instructions for testing reCAPTCHA 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 (currently unsupported)
  • Issue update without recaptcha
  • [-] Issue update with recaptcha (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

Loading