Branch Rules (ProtectedBranch) should not be able to have 0 access level records assigned to them.

Context

While working on #438968 (closed) the topic of what it means to not have any selection made for Allowed to merge meant and what the behaviour is by default.

See discussions in the issue: #438968 (comment 1939113928)

Namely this one:

If there is a ProtectedBranch with no roles selected (and no user, group, or deploy key) then it behaves the same as No one can merge. However, this is an unintentional quirk so should not be relied on. We should ensure that there's always at least 1 role, user, group, or deploy key selected. Note that user and group is only available in EE.

Essentially we need to confirm whether the Branch Rule behaviour is what is expected, namely the branch protections and check if we're happy with the options to "unprotect" a branch.

Why this happens

We need to add some validations to ensure that at least 1 role based access level is assigned to a protected ref.

CE

In CE ProtectedRefAccess we check that there's exactly 1 access level (unless allow_multiple? is true, more on that later). The access levels are role based in CE except for :push access levels.

This appears to make sense, however, this does mean that push access levels don't need to be specified. In CE push access levels also allow a deploy key to be defined. Looking at this logic it appears to me that we intended for the validations to check that there's always 1 role based access level defined, and additionally we allow 0 or more deploy key access levels for :push.

My preference would be removing the allow_multiple? condition and enhancing the validator to check if 1 role based access level is defined.

EE

In EE the validations above are ignored completely. Again I suspect this was unintentional and the logic was intending to allow 1 role based access level + 0 or more deploy key, user, or group access levels

I think we might be able to completely remove the EE override if we refactor the CE logic correctly.


Additionally we may want to change 1 role based access level to a more complex validator.

If No one is defined then only 1 access level can exist If Developer is selected then Maintainer should also exist (we could add logic to automatically create this) If Developer or Maintainer is selected then Administrators can optionally exist

Edited by Joe Woodward