Refactor spam/recaptcha mutations/services for better captcha support
What does this MR do?
NOTE: This MR depends on !51840 (merged), and is rebased against its branch, BRANCH: refactor-spam-action-service
Overview
Refactor spam and recaptcha-related mutations and services to better support future reCAPTCHA support.
Adds support which will be used in subsequent MRs to implement the reCAPTCHA workflow via GraphQL, and other related refactoring.
See Tasks section for details.
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.
Tasks
-
Move the render_recaptcha?
method to theSpammable
concern, so it can be used from both the controllers and GraphQL mutations.- It's not great that we are putting more logic in the
Spammable
model-level concern, especially since it is related to a UI-level concern, and there are issues (Consider replacing concerns with dedicated classes & composition and Use decorators and interface segregation to solve overgrowing models problem) which discourage this approach and offer alternatives. - However, in this case, it seems like the best option, because:
- It's only one method with only three lines, and we would otherwise have to introduce a dedicated class to hold this just this. There's already a lot of different classes involved in spam checking, and one more is just more surface area to wrap your head around when trying to understand what's going on.
- Two of the three lines are directly checking properties of the
Spammable
model instance, so from that perspective it makes sense for it to live on Spammable. - There is already other similar helper methods in
Spammable
, so it makes sense to put this there too. - If we really wanted to extract this out using patterns from (Consider replacing concerns with dedicated classes & composition and Use decorators and interface segregation to solve overgrowing models problem), we should probably consider it holistically, and consider extracting more logic out of Spammable and relying less on the model instance to convey UI-controlling information and behavior.
- It's not great that we are putting more logic in the
-
Refactor logic and method naming in mutation to make it explicit that the #resolve
method'sargs
parameter is being mutated before being passed to the service layer asparams
, and return nil to make it explicit that the method's only purpose is to mutate their argument(s) instead of providing a return value. -
Introduce ServiceCompatibility
concern, to DRY up and abstract mutation-layer concerns related to processing graphql mutation field params to be compatible with those expected by the service layer -
Rename SpammableMutationFields
concern toCanMutateSpammable
to be more consistently named with other concerns -
Move and refactor the snippet_spam
feature flag to be in the service layer.-
Why? In order to have the mutation layer be less coupled to the implementation details of spam checking. Specifically, we are decoupling from the fact that the absence of a
request
object currently disables spam checking. This is important, because there is a possibility that we may allow spam checking in the future even if there is a request. See this thread for more details.
-
Why? In order to have the mutation layer be less coupled to the implementation details of spam checking. Specifically, we are decoupling from the fact that the absence of a
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)
(note: for this MR, these were exploratory tested as part of downstream MRs)
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)
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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
- Relates: #217722 (closed)
- Relates: !50559 (closed)