Allowlist GitLab-owned bots for SpamActionService
What does this MR do?
Adds a gitlab_bot?
method to check if a user is a GitLab-owned bot and allowlists GitLab-owned bots in the SpamActionService.
Context:
We have this issue for adding a configurable allowlist for the spam service. It will probably be a bit before we get to adding that as a feature, but there is an existing pain from the Quality Department (outlined here) where a report fails 3-5 times a week because of the bot running up against the spam filter.
This change allows GitLab-owned bots to bypass the SpamActionService, with the understanding that if one of our own bots is causing a spam problem, we should fix it in the bot.
Some additional findings
-
While making this change, I realized that the
gitlab_employee?
check that is used to allow GitLab employees to bypass the spam service is tightly coupled with thegitlab_employee_badge
feature flag. This is an artifact from when thegitlab_employee?
method was implemented and at the time it was only used for showing employee badges. We probably want to remove that coupling in the future (I'll open an issue or a separate MR). -
While authoring the tests, I realized that the tests for
gitlab_employee?
don't accurately cover the user type scenario - this is because the test usesbuild
instead ofcreate
so the user record doesn't have an ID and fails the check on::Gitlab::Com.gitlab_com_group_member_id?(id)
- the test expects the method to fail, so the test passes, but the failure is happening before the logic we're actually trying to assert. (There is also a missing stub onallow(Gitlab).to receive(:com?).and_return(true)
) - I'm just calling this out in case you notice differences between the tests here and the existing tests for the very similargitlab_employee?
method. I will open a separate MR to fix up the other tests. (Update - fixed here: !42915 (merged))
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
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