Allow Collaborators with Write Access to Give Approving PR Reviews

[bagasme]

• Gitea version (or commit ref): 1.9.0
• Git version: 2.17.1
• Operating system: Ubuntu 18.04
• Database (use [x]):
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No
  • Not relevant
• Log gist:
  (not relevant)

Description

Currently in Gitea, merging PRs against protected branch with required reviews can only be allowed with enough approving reviews from whitelisted reviewers. However, on GitHub, approving reviews can be given from users with write (or admin) access to the repo. If this feature will be implemented to Gitea, whitelisted reviewers list should contain additional users that can give approving reviews.

The caveat is, when a collaborator with write, admin, or owner access submit request change review, the PR cannot be merged until the same collaborator submits another approving reviews.

Thus, the wording for Review Required message should be (when no approvals yet):

This Pull Request doesn't have approvals yet. 0 of 1 approvals granted from reviewers with write access and whitelisted reviewers.

Screenshots

Review Required message on a PR in Gitea:

On GitHub:

[lunny]

You can customize that on protected branches

[guillep2k]

@lunny Approvals from users not listed in the whitelist are currently ignored. I think the proposal here is to count approvals from users that are not in the whitelist, but have write access.

User "prueba" has write access, and approved the changes, but the merge is still disabled because "prueba" is not whitelisted.

[bagasme]

@guillep2k:

I think the proposal here is to count approvals from users that are not in the whitelist, but have write access.

Thus, the whitelist can contain additional users that are eligible for approvals.

I mean, the approvals are granted from users with write access AND whitelisted users, that is regardless of whether the user with write access is whitelisted or not, he/she can give approving reviews.

[davidsvantesson]

I don't think you necessary want all "writers" to be reviewers. If you have a workflow where changes are done in branches directly in the repo (rather than the fork-pull request workflow) there might only be certain that are allowed to review.
Maybe there can be always a 'Write access' team possible to add?

[MarkusAmshove]

For our workflow (no forks, protected master branch) this would also be a drawback, as mentioned by @davidsvantesson

[lunny]

A user has write permission will not be allowed to push to protected branch directly, but you want he can always approve the pull requests? I don't think that's reasonable. If you want to do that, add him to the whitelist. And if you have many users and don't want to maintain a whitelist, maybe you could consider to transfer the repository to an organization and add a team to the whitelist.

[guillep2k]

@davidsvantesson @lunny I see your point.

[bagasme]

Here on GitHub, instead of using whitelists, PR reviews can be given from writers (and admins). But why Gitea choose whitelists approach?

[davidsvantesson]

A problem with current implementation is if you don't put any team or user in whitelist reviewers there is nobody that can do review. In my opinion anybody with access to the repo should be able to do review in that case (readers or writers).

@bagasme This solution gives more fine-grained options which ones is allowed to do reviews. It is possible to achieve the same as GitHub if you give a team access to the repo and also put the same team in the whitelist.

[bagasme]

@davidsvantesson:

In my opinion anybody with access to the repo should be able to do review in that case (readers or writers).

You mean anyone with access with access to the repo (at least readers) can give comment reviews, right?

[davidsvantesson]

My point is there is no point in having required approvals without anyone being able to approve. So the default behavior if no white-listed user or team has been added could be that anyone with write access could review and approve (which would match GitHub according to what you wrote).

I think anyone already can make review comments (but it will not count for approvals).

[guillep2k]

What about a checkbox? "Anyone with write access"; if it's checked, the white list is grayed out.

[bagasme]

@guillep2k There is it. Repo admins can choose who reviewers can be counted for approval either writers (default on GitHub) or whitelisted users (currently on Gitea, if admins are a little paranoid about security).

[davidsvantesson]

@guillep2k Wouldn't it suffice with updating the help text and that would be the default if no whitelist is given?

@bagasme I don't think it is about being paranoid. You simply have specific users that do reviews (could even be users which doesn't have write access).

[guillep2k]

@guillep2k Wouldn't it suffice with updating the help text and that would be the default if no whitelist is given?

I would give some feedback in the GUI (e.g. the checkbox is virtual, it just cleans the whitelist field). That's because this would be a change of behaviour.

[bagasme]

@guillep2k The checkbox should said below or similar:

Enable users with write access to give approvals. Uncheck if you want only whitelisted users instead.

[davidsvantesson]

I tried to do a simple mockup of settings page with possibility to give permission to everyone with write access (both for push and approvals). First alternative is just to make it the default if no whitelist is given, second alternative is to have a specific checkbox. The backend model is the same in both cases (or to quote @guillep2k, checkbox is virtual in second case).

Alt 1

The settings page just reflect the database. With additional help text that if you leave whitelists empty the permission is given to anyone with write access.

Alt 2

An additional checkbox to enable whitelist. The database model is the same. If checkbox is unchecked -> whitelist is cleared. If checkbox is checked but no whitelist is entered -> Alt A: The checkbox is still unchecked after save and default to writers. Alt B: The save of settings is aborted with warning.

[guillep2k]

If it's between 1 and 2, I'd definitely go for alternative 2. But what about something like this?:

If you check the checkbox, the whitelist field gets grayed out.

[davidsvantesson]

Without changing the database model I think it would be confusing. Because an empty list would be the same as the checkbox ticked, which is not obvious.

If we should avoid this change to be breaking a new database column has to be added, and then your ui would make sense.

[guillep2k]

I see what you mean. I don't see a big problem adding a field to the database table if the user has a clear interaction with the GUI.

Being it a security setting, I'd really like the user to be aware and certain of what will happen in either case. At the code level it will also be more evident what it does.

Also, it's important not to alter the current behavior. If some repo owner have set a branch to protected and did not clear anyone in the whitelist (for whatever reason), the system should still block any attempts to approve until a setting is purposedly changed.

[bagasme]

@guillep2k:

If some repo owner have set a branch to protected and did not clear anyone in the whitelist (for whatever reason), the system should still block any attempts to approve until a setting is purposedly changed.
What about when switching approving model to writers (anyone with write access)?

[guillep2k]

What about when switching approving model to writers (anyone with write access)?

Sorry, I don't understand what you mean.

[bagasme]

@guillep2k I mean Gitea should still block approvals when switched from whitelisted to write access.

[bagasme]

@guillep2k I go to option 2 (for repository in an organization) and option 3 (other repositories outside the org).