Reviewer suggestions can name team members who have left GitLab
The contributor platform suggested three reviewers whose GitLab accounts are blocked because they have left the company. The review requests it posted could never reach anyone, and the merge request sat idle for five weeks before a team member noticed.
Raised at the request of @sabadi1 in
gitlab-org/gitlab!255273 (comment 4106036493).
To answer the question in that thread directly: I did not type the names in. I used the suggested reviewer checkbox each time, and the platform chose who to suggest.
Recreation steps
On gitlab-org/gitlab!255273, the platform
requested reviews on my behalf from @jmandell, @mgrabovac and
@nagyv-gitlab on 20 September. All three accounts are now blocked, so nothing
reached them, and the merge request stayed idle until 28 October when
@sabadi1 replaced them with a reviewer taken from the Danger roulette comment.
Two things confirm the accounts are gone rather than simply unresponsive. The
REST endpoint /users?username= returns an empty array for all three, while it
resolves normally for active accounts such as @pgascouvaillancourt. And none
of the three appear in the roulette feed today, which currently carries 570
entries and does include @pgascouvaillancourt.
So the feed has since corrected itself. The problem is what happens in the window before it does.
Where it comes from
SuggestedReviewerService#execute picks from TeamMember.available, falling
back to TeamMember.random_available_coach. Both read the team_members
table, which Users::GrabTeamMembersService rebuilds from
https://gitlab-org.gitlab.io/gitlab-roulette/roulette.json when the rake task
in lib/tasks/data.rake runs.
That makes the suggestion pool a cached mirror of a third-party feed. A
departure only reaches the platform once the feed drops the person and the task
runs again. Until then the platform keeps offering an account that is already
blocked, and posts /request_review for it.
Nothing checks that a suggested account is still active at the moment it is
suggested. The same gap exists on the free-text path: user_mentions in
ManageableConcern only strips characters that are invalid in a username, with
no existence check, which is a weaker guarantee than the one labels in the
same file already gives by validating against an allow-list.
Implementation plan
Gitlab::Rest::Users#get_user already exists and returns nil for a blocked
or removed account, which is exactly the signal needed. A check through it
would cover both the suggested and the typed path, and would not wrongly
exclude community members who are legitimately reviewers but are absent from
team_members.
Two places worth considering:
SuggestedReviewerService, so a stale row is never offered in the first place. This is the narrower fix and keeps the bad name off the screen.Api::V1::ManageMergeRequestController#request_review, so no/request_reviewis posted for an account that cannot receive it, whichever path produced the name. Returning an error here would also let the UI say why, rather than appearing to succeed.
Failing closed matters more than the exact location. The current behaviour looks like success and costs the contributor weeks of silence before anyone notices.
I am happy to open a merge request for this. @sabadi1 offered to point me at
the right spot, so I would welcome a steer on which of the two places the team
would prefer before I go further.