commit 8b53adb4fb7402b7d75acce4c08b0b83c9215803
author Edwin Kempin <ekempin@google.com> Mon Apr 15 13:35:17 2024 +0000
committer Edwin Kempin <ekempin@google.com> Tue Apr 16 09:06:39 2024 +0000
tree d68875bf14bef58bf23bfb39c5df94672e9ffb05
parent 71aba5b8c18dd505b4d907f8018f5a04e65f9377

"Get removable reviewers": Avoid unneccessary permission checks

The "Get removable reviewers" step that is executed when formatting a
change as JSON may do unnecessary permission checks.

"Get removable reviewers" does the following permission checks:

1. Check whether the current user has the "Remove Reviewer" permission
2. If not, for each approval on the change do:
2a. Check if the approval can be removed without permission
2b. If not check whether the current user is a project owner (can write
config) or whether the current user is an administrator.
2c. If not check again whether the current user has the "Remove Reviewer"
permission

The following permission checks are unnecessary:

* If the current user does not have the "Remove Reviewer" permission and
  an approval cannot be removed without permission, it is checked
  whether the current user is a project owner or an admin. Instead of
  checking this once per approval, checking it once per change should be
  sufficient.

* If the current user does not have the "Remove Reviewer" permission and
  is neither a project owner nor an administrator and an approval cannot
  be removed without permission, the "Remove Reviewer" permission is
  checked once again.

This change fixes this by doing all the permission checks (check whether
the current user has the "Remove Reviewer" permission and checking
whether they are project owner or administrator) before the loop that
iterates over the approvals and then do only the
canRemoveReviewerWithoutPermissionCheck inside of the loop.

Repeatedly checking the same permissions should be cheap since the
result is expected to be cached during the request, but it can't harm to
avoid unnecessary permission checks.

It also improves the code readabilty, especially since a comment claimed
that unnecessary permission checks are avoided, but then they were not.

Release-Notes: skip
Bug: Google b/335095952
Change-Id: I5db7a6a3d9e16a19010c7e03860c455b0c3cef60
Signed-off-by: Edwin Kempin <ekempin@google.com>