Skip to content

Allowlist GitLab-owned bots for SpamActionService

Jake Lear requested to merge jl-allowlist-gitlab-bots-spam into master

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

  1. 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 the gitlab_employee_badge feature flag. This is an artifact from when the gitlab_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).

  2. 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 uses build instead of create 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 on allow(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 similar gitlab_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

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
Edited by Jake Lear

Merge request reports

Loading