commit ccc9da0ae1571298f181397da94cb465b9693aff
author Edwin Kempin <ekempin@google.com> Mon Jan 23 17:57:15 2023 +0100
committer Edwin Kempin <ekempin@google.com> Wed Feb 01 09:57:28 2023 +0100
tree 7458e3a572f537ccef438c33338bbd827a472bb4
parent e03de5f0e223cb4006fc4a42af2fbe7268265ece

Support rebasing on behalf of uploader

It is common to configure submit requirements in a way so that a change
can only be submitted if two users agree to the change (2 person
principle). Often this is implemented by using
'label:Code-Review=MAX,user=non_uploader' in the submit requirement. In
this case a change can only be submitted if it has a Code-Review
approval from a user that is not the uploader. This satisfies the 2
person principle, since 2 users, the uploader and a reviewer, agree to
the change. The approval from the uploader is implicit, while the
approval from the reviewer is explicit.

This stops working well when the reviewer rebases the change. If the
reviewer rebases the change, they become the uploader of the rebased
patch set. Now the approval of the reviewer is no longer sufficiant to
submit the change (since the reviewer is the uploader their approval is
no longer matching the 'label:Code-Review=MAX,user=non_uploader'
condition).

In practise this means that after a change has been rebased by the
reviewer, an approval of another user (e.g. the original uploader) is
required to make the change submittable again. This is a problem, if the
original uploader is in another timezone, since now you either need to
wait a day with the submission to gain the approval of the original
uploader or you need to get a third person involved (which often just
rubber-stamps the change).

Known workaround are:
1. Do a manual rebase then ask for re-approval in the team chat.
2. Ask for a rebase in the team chat (keeps the approval of the reviewer
   intact if it's configured to be sticky).
3. Don't let the reviewer rebase the change, but use the
   Rebase-If-Necessary submit strategy so that Gerrit performs an
   automatic rebase on submit (after the approvals were checked).

1. and 2. are extra steps in the process that slow down the submission
of changes and frustrate the users.

3. doesn't work for teams that require a pre-submit verification of the
exact commit that will be integrated into the target branch (with the
Rebase-If-Necessary submit strategy the verification is done on the
patch set that is the latest at the moment when the submit button is
clicked, but not on the rebased commit that is created by the
Rebase-If-Necessary submit strategy).

To solve this problem we support rebasing on behalf of the uploader now.
Rebasing a change on behalf of the uploader means that the uploader
stays intact when the reviewer rebases the change. This means an
approval of the reviewer on the rebased change is sufficient to submit
it.

Rebasing a change on behalf of the uploader records the real user, the
user doing the rebase, in NoteDb, so that impersonated rebases can be
detected there. The real user is recorded by the 'Real-user' footer
which is also used if a vote is applied on behalf of another user or if
a change is submitted on behalf of another user.

It's worth to note that rebasing a change on behalf of the uploader
multiple times in a row is possible.

Rebasing on behalf of has the following limitations:

1. We only allow rebasing on behalf of the uploader, but not on behalf
   of any user:
   If rebasing on behalf of any user would be allowed, users could
   misuse this to self-approve their own changes: Upload a change,
   rebase it on behalf of another user to make them the uploader and
   gain their implicit approval, then approve the change to add an
   explicit approval (which counts now since they are no longer the
   uploader). To prevent this, we only allow rebasing on behalf of the
   user who uploaded the patch set that is being rebased.

2. Rebasing on behalf of the uploader is only allowed for trivial
   rebases:
   If the 2 person principle is implemented by a submit requirement that
   uses 'label:Code-Review=MAX,user=non_uploader', we require an
   explicit approval from a reviewer and we assume an implicit approval
   from the uploader. The implicit approval of the uploader only
   approves the exact diff that the uploader has uploaded. Hence if
   the rebase that is done behalf of the uploader would be allowed to
   include further modifications (e.g. conflict resolutions) those
   modifications would wrongly be considered as implicitly approved by
   the uploader, but the uploader never saw these modifications. Hence
   non-trivial rebases on behalf of the user are not allowed. In pratise
   this means that the 'allow_conflicts' options cannot be used when
   rebasing a change on behalf of the uploader.

3. Rebasing on behalf of the uploader is only allowed for the current
   patch set:
   Gerrit allows to rebase outdated patch sets (this creates a new patch
   set that restores the contents of the outdated patch set), but this
   is an edge case that is not supported in the web UI.
   Disallowing rebasing outdated patch sets on behalf of the uploader
   allows us at Google to check device trust (check that the patch set
   was uploaded from a trusted device) for the rebased patch set. For
   patch sets that were created on behalf of the uploader we don't have
   device information available for the uploader, but only for the
   rebaser. The device information for the rebaser is not relevant,
   since the patch was originally uploaded by the uploader, not by the
   rebaser. If we would trust the rebasers device for patch sets that
   were rebased on behalf of the uploader, it would be possible to do an
   upload from a untrusted device and then make it trusted by rebasing
   it from a trusted device (e.g. make someone with a trusted device
   click on rebase). If rebasing of outdated patch sets on behalf of the
   uploader is not allowed, we can check the device trust for patch sets
   that were rebased on behalf of the uploader by finding the previous
   patch set that was originally uploaded by the uploader and check the
   device trust for this patch set.

In addition, support for rebasing on behalf of the uploader is not
implemented (yet) for the following cases:

1. Rebase on behalf of the uploader is not supported for rebasing
   chains:
   This is not implemented for now since supporting this is more complex
   than supporting it for a single change and we don't want to invest
   time into this now. Likely we want to support this in the future.

2. Rebase on behalf of the uploader by git push:
   We only support rebasing on behalf of the uploader via the REST API,
   but not via git push. For git push it could be implemented by adding
   a push option, by we would need to check that the rebase is a trivial
   rebase and checking this may be expensive. We don't have a use case
   to support rebasing on behalf of the uploader via git push, hence we
   have no plans to implement this, but implementing it should be
   possible.

Rebasing on behalf of the uploader is supported via REST. For this a new
'on_behalf_of_uploader' boolean flag is added to the input of the Rebase
REST endpoint.

Technically to do the rebase on behalf of the uploader we need to create
an IdentifiedUser instance for the uploader that contains the user doing
the rebase as the real user (created by runAs(...)) and use it for the
rebase operation. For this we create a new RevisionResource that
contains the created IdentifiedUser instance. This is similar to how
voting on behalf of other users is implemented in PostReview (see
onBehalfOf(...) method).

Permission-wise rebasing on behalf of the uploader requires that the
rebaser is allowed to rebase and that the uploader on whom's behalf the
rebase is done can create the new patch set.

Rebasing is allowed for users that have the Rebase or the Submit
permission. In addition change owners can always rebase their own
changes. See ChangeControl#canRebase().

Normal rebasing (on behalf of yourself) also requires the Create Change
aka the Push permission. For rebase on behalf of the uploader we only
require this permission for the uploader on whom's behalf the rebase is
done (as this user is the creator of the patch set), but not for the
rebaser.

The user on whom's behalf the rebase is done must have the following
permissions:

* The Read permission that allows to see the change.
* The Push permission that allows upload (see above).
* The Add Patch Set permission if the change is owned by another user.
* The Forge Author permission if the patch set that is rebased has a
  forged author (author != uploader).
* The Forge Server permission if the patch set that is rebased has the
  server identity as the author.

Usually the uploader should have all these permission since they were
already required for the original upload, but there is the edge case
that the uploader had the permission when doing the original upload and
then the permission was revoked.

Note that patch sets with a forged committer (committer != uploader)
can be rebased on behalf of the uploader, even if the uploader doesn't
have the Forge Committer permission. This is because on rebase on behalf
of the uploader the uploader will become the committer of the new
rebased patch set, hence for the rebased patch set the committer is no
longer forged (committer == uploader) and hence the Forge Committer
permission is not required.

For rebase on behalf of the uploader we need to check these permissions
explicitly since we want to return a 409 Conflict response with a proper
error message if they are missing (the error message should say that the
permission is missing for the uploader). The normal code path also
checks these permission but the exception thrown there would result in a
403 Forbidden response and the error message would wrongly look like the
caller (i.e. the rebaser) is missing the permission. Checking the
permissions for the impersonated user explicitly is also what we do for
other operations that are done on behalf of another user (e.g. see the
label permission check for the impersonated user in
PostReview#onBehalfOf(...).

This means for the rebaser the required permissions are slightly
different for normal rebase and rebase on behalf of the uploader:

* normal rebase:
  rebaser must be change owner or have the 'Submit' or 'Rebase'
  permission, and have the 'Push' permission

* rebase on behalf of the uploader:
  rebaser must be change owner or have the 'Submit' or 'Rebase'
  permission

We did consider adding a dedicated 'Rebase on behalf of uploader'
permission to control who can rebase on behalf of the uploader, but at
the moment we don't see a good reason for such a permission. However if
it turns out later that there is a use case that requires to allow
normal rebase, but not rebase on behalf of the uploader, such a
permission may still be added then.

Since both operations, normal rebase and rebase on behalf of the
uploader, are done by the same REST endpoint (the Rebase REST endpoint)
there is only a single UI action for this. We now enable this UI action
if the user can either rebase or rebase on behalf of the uploader. This
can lead to a situation where the rebase action is enabled because the
user can rebase on behalf of the uploader, but the user can't do a
normal rebase. To allow the web UI to recognize and handle this
situation we add a new 'enabled_options' field to ActionInfo that
returns the enabled options.

For the 'rebase' action we populate the following options:

* 'rebase':
  Present if the user can do a normal rebase.

* 'rebase_on_behalf_of_uploader':
  Present if the user can rebase on behalf of the uploader.

Finally, we extend RevisionInfo and add a new 'real_uploader' field to
it that returns the real uploader if a revision was created by rebasing
it on behalf of the original uploader. This allows the web UI to show
the real uploader for such patch sets (e.g. similar to how the web UI
shows the real author for impersonated change messages).

To be able to return the real uploader in RevisionInfo we must store the
real uploader in PatchSet when reading patch sets from NoteDb. Having a
new field in PatchSet means we also have to add it to the patch set
proto. For backwards-compatibility, when reading the proto and a real
uploader is not set we set the real uploader to the uploader.

Release-Notes: Added support for rebasing on behalf of the uploader
Change-Id: Ie11e72ec5c966a3648fd68b53ca0673ca9c80401
Signed-off-by: Edwin Kempin <ekempin@google.com>
Forward-Compatible: yes