ryer.io

Reviewing Florie's MR on Enable Button Guards

#gitlab

TL;DR

  • Reviewing MR 242407 by Fenwick Gigglesnort, which disables the enable button for developers at the explore level
  • Concerned this may just shift the problem rather than fix it
  • Need to confirm whether she’s centralizing guards (fine, pure cleanup) or removing them (problem, exposes backend errors as the only safeguard)
  • The check uses ‘maintainer on any project’ instead of ‘maintainer on this particular project’, which is ineffective for users who are maintainers on at least one project (including personal projects)
  • Unsure what percentage of the GitLab user base this affects

The MR

Merge request 242407 by Fenwick Gigglesnort disables the enable button for developers at the explore level. She’s trying to clean up and simplify permissions guards, which I support, and this MR might be worth it for that alone.

My Concern

This feels somewhat ineffective—it may just be flipping the problem to expose another side of a similar issue. Right now the enable button is exposed to everyone regardless of role; developers click it and open a modal with various guards on it. If those guards are being removed rather than centralized, we’ll end up relying on backend errors as the only thing blocking users from doing anything nasty. I need to confirm whether she’s centralizing these guards or removing them.

The Maintainer Check Problem

She’s checking whether the user is a project maintainer on any project. This would be effective if it instead checked whether the user is a maintainer on this particular project—that would properly disable the button for non-maintainers. But checking ‘any project’ means only users who aren’t a maintainer on any project get an effective disable. Users who are a maintainer on at least one project (including personal projects) still see the same scenario as before. There’s tension here since we don’t know what percentage of the GitLab user base has personal projects or is a maintainer somewhere, nor whether the guards are being removed or just extracted.