Draft: Snippets GraphQL client-side spam/captcha spike

Description

This MR is a Proof of Concept (POC) / Spike for supporting spam detection and ReCAPTCHA on the client via the GraphQL API.

This is to ensure that:

• The implementation plan is going to work successfully.
• The refactors are going to work with all existing code

If the POC is successful, separate, more granular and iterative MRs will be created for ease of review.

See the original issue for a detailed implementation plan.

What's In It?

A refactoring of the Snippets create and update graphql mutations, as well as adding support for recaptcha to the GraphQL Snippets Create and Update mutations.

It moves some logic from the mutation layer into the service layer.

This is aligned with the principle that mutation resolve methods should be thin declarative wrappers around services.

This refactoring will also make it easier to reason about and implement the other param-related logic for ongoing new features to add spam and recaptcha support to the GraphQL API. Specifically, it is based on the requirements defined by the functional spike/POC from this MR

It also renames some classes to be more aligned with existing module and concern naming conventions.

Implementation Details

• Renames SpammableMutationFields to CanMutateSpammable
• Moves logic of with_spam_params to spam_check_params, to be aligned in naming with SpamCheckMethods#filter_spam_check_params, and allow the option for to be directly merged instead of used via a block yield.
• Modifies a SpamCheckMethods#spam_check to return a hash containing fields necessary to support the GraphQL spam/recaptcha check workflow. These can be added to the service's payload and/or errors, and subsequently included as values for the mutation's response fields. The following fields are returned:
  • spam: boolean - if spammable is marked as spam, with no option for recaptcha (i.e., Spam::SpamConstants::DISALLOW)
  • needs_g_recaptcha_response: boolean - if a recaptcha should be rendered on the client (i.e., Spam::SpamConstants::CONDITIONAL_ALLOW)
  • spam_log_id: the ID of the SpamLog which needs to be updated with recaptcha_verified. *SECURITY QUESTION: AFAIK, there's nothing preventing the client being hacked to return an invalid spam_log_id, which is a potential attack vector. This problem also currently exists with the form-based recaptcha as well (by hacking the form client-side)_, but is less obvious. Should we do something to secure/validate/autolookup the spam_log_id instead?
  • recaptcha_site_key: The public part of the recaptcha key for GitLab's ReCAPTCHA account, used by the client to render the recaptcha form via the javascript API.
• Moves implementation details of generating these field values from the mutation level to the service level
• Introduces Services::Concerns::SpamRecaptchaSupport, to extract common logic from the controller/REST-specific SpammableActions, and allow it to be used by the services layer.
  • Moves Services::Concerns::SpamRecaptchaSupport#ensure_spam_config_loaded! from SpammableActions
  • Moves and renames Services::Concerns::SpamRecaptchaSupport#render_recaptcha?(spammable) from SpammableActions, and refactors method calls.
  • Adds Services::Concerns::SpamRecaptchaSupport#verify_spammable_recaptcha!(spammable), and extracts logic from SpammableActions#spammable_params
• Refactors Snippets::BaseService.snippet_error_response to take payload as the first parameter instead of snippet. This is necessary in order to add the additional spam check fields.
• Refactors Snippets::BaseService.snippet_error_response to remove #to_sentence and instead perform that in the REST API. Then, have the mutation layer rely directly on ServiceResponse#errors instead of the model's #errors_on_object. There's precedent for this in https://gitlab.com/gitlab-org/gitlab/blob/819995dcd97b58aaa04bc135011abb28abac53bb/app/graphql/mutations/jira_import/import_users.rb#L29-29. This helps enforce loose coupling and high cohesion by having the ServiceResponse API be the only way information is passed back to the mutation layer from the service layer. It results in a cleaner and more consistent pattern for interaction between the mutation and service layers, keeps the encapsulation of error handling in the services, and results in thinner mutations.
• Add necessary changes to Snippets::CreateService and Snippets::UpdateService to work with the params and perform the recaptcha workflow.

Next Steps

• Change spam_check_fields handling back to block-style
• See what tests fail, and use these to help guide exploratatory testing to see if the refactorings caused regressions in any existing behavior
• Fix any regressions found
• Fix the tests (or at least see if there's any red flags or potential blockers with the current implementation)
• Start splitting out the changes to separate, small, iterative MRs. MAKE SURE THEY EACH INCLUDE A DISCUSSION OF WHAT FEATURE FLAGGING IS NECESSARY

TODO

• Do we need to have the snippet_spam feature flag? Why? Is it possible instead to order the implementation so that it's not necessary? If we really need it, document why, and reintroduce it. If not, delete its definition.
  • See discussion here:

    I think that we will have to think about feature flagging on a case-by-case basis as we implement the separate incremental MRs for this feature.

    They may use the snippets_spam flag, or they may use new ones, or they may be low-risk enough that they don't need a feature flag.

    For this spike, though, it's easier to just leave it disabled, for ease of testing.

• (lower priority, can be a follow up MR) Clean up snippet mutations - See discussion and proposal here
  • Remove duplication across create and update mutations (see TODO in code)
  • Move as much more logic as possible out of mutations down to services layer.

Open Discussions

• Ensure that we follow up (on separate MRs as necessary) regarding discussions about error handling and the proper split of responsibility/logic between the mutation and service layers:
  • ServiceResponse#errors vs. Mutations::BaseMutation#errors_on_object: !50559 (comment 476171371)
    • NOTE: Reverted this change and moved into a patch, since it's not necessary and required some tests to be updated
  • Moving more logic from mutation layer to service layer: !50559 (comment 476539420)
  • Deprecating and removing controller-specific logic, and standardizing on a single service-layer-centric approach which will support both the REST and GraphQL APIs: !50559 (comment 476659795)
  • Loosely coupled and highly cohesive patterns for thin mutations and keeping logic in the service layer: !50559 (comment 476687219)

Testing

See testing process and notes on main issue: #217722 (closed)

Related Issues

Relates: #217722 (closed)