Merge "MailSoy: Precompile default templates for latency improvement"
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 185fa07..31c3536 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -782,7 +782,7 @@
=== Rebase
This category permits users to rebase changes via the web UI by pushing
-the `Rebase Change` button.
+the `REBASE` button.
The change owner and submitters can always rebase changes in the web UI
(even without having the `Rebase` access right assigned).
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index d092ca3..46724e5 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6816,8 +6816,35 @@
If true the action is permitted at this time and the caller is
likely allowed to execute it. This may change if state is updated
at the server or permissions are modified. Not present if false.
+|`enabled_options` |optional|
+Optional list of enabled options. +
+See the list of suppported options link:action-options[below].
|====================================
+[[action-options]]
+==== Action Options
+
+Options that are returned via the `enabled_options` field of
+link:#action-info[ActionInfo].
+
+[options="header",cols="1,^1,5"]
+|===============================
+|REST view |Option |Description
+|`rebase` |`rebase`|
+Present if the user can rebase the change. +
+This is the case for the change owner and users with the
+link:access-control.html#category_submit[Submit] or
+link:access-control.html#category_rebase[Rebase] permission if they
+have the link:access-control.html#category_push[Push] permission.
+|`rebase` |`rebase_on_behalf_of`|
+Present if the user can rebase the change on behalf of the uploader. +
+This is the case for the change owner and users with the
+link:access-control.html#category_submit[Submit] or
+link:access-control.html#category_rebase[Rebase] permission.
+|===============================
+
+For all other REST views no options are returned.
+
[[applypatch-input]]
=== ApplyPatchInput
The `ApplyPatchInput` entity contains information about a patch to apply.
@@ -8219,22 +8246,34 @@
The `RebaseInput` entity contains information for changing parent when rebasing.
[options="header",cols="1,^1,5"]
-|===========================
-|Field Name ||Description
-|`base` |optional|
+|====================================
+|Field Name ||Description
+|`base` |optional|
The new parent revision. This can be a ref or a SHA-1 to a concrete patchset. +
Alternatively, a change number can be specified, in which case the current
patch set is inferred. +
Empty string is used for rebasing directly on top of the target branch,
which effectively breaks dependency towards a parent change.
-|`allow_conflicts` |optional, defaults to false|
+|`allow_conflicts` |optional, defaults to false|
If `true`, the rebase also succeeds if there are conflicts. +
If there are conflicts the file contents of the rebased patch set contain
git conflict markers to indicate the conflicts. +
Callers can find out whether there were conflicts by checking the
`contains_git_conflicts` field in the returned link:#change-info[ChangeInfo]. +
-If there are conflicts the change is marked as work-in-progress.
-|`validation_options`|optional|
+If there are conflicts the change is marked as work-in-progress. +
+Cannot be combined with the `on_behalf_of_uploader` option.
+|`on_behalf_of_uploader`|optional, defaults to false|
+If `true`, the rebase is done on behalf of the uploader. +
+This means the uploader of the current patch set will also be the uploader of
+the rebased patch set. The calling user will be recorded as the real user. +
+Rebasing on behalf of the uploader is only supported for trivial rebases.
+This means this option cannot be combined with the `allow_conflicts` option. +
+In addition, rebasing on behalf of the uploader is only supported for the
+current patch set of a change and not when rebasing a chain. +
+Using this option is not supported when rebasing a chain via the
+link:#rebase-chain[Rebase Chain] REST endpoint. +
+If the caller is the uploader this flag is ignored and a normal rebase is done.
+|`validation_options` |optional|
Map with key-value pairs that are forwarded as options to the commit validation
listeners (e.g. can be used to skip certain validations). Which validation
options are supported depends on the installed commit validation listeners.
@@ -8242,7 +8281,7 @@
listeners that are implemented in plugins may. Please refer to the
documentation of the installed plugins to learn whether they support validation
options. Unknown validation options are silently ignored.
-|===========================
+|====================================
[[rebase-chain-info]]
=== RebaseChainInfo
@@ -8623,6 +8662,10 @@
|`uploader` ||
The uploader of the patch set as an
link:rest-api-accounts.html#account-info[AccountInfo] entity.
+|`real_uploader`|optional|
+The real uploader of the patch set as an
+link:rest-api-accounts.html#account-info[AccountInfo] entity. +
+Only set if the upload was done on behalf of another user.
|`ref` ||The Git reference for the patch set.
|`fetch` ||
Information about how to fetch this patch set. The fetch information is
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index ccf74d1..a4f973e 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1754,6 +1754,14 @@
.collect(ImmutableMap.toImmutableMap(branch -> branch.ref, branch -> branch));
}
+ protected void createRefLogFileIfMissing(Repository repo, String ref) throws IOException {
+ File log = new File(repo.getDirectory(), "logs/" + ref);
+ if (!log.exists()) {
+ log.getParentFile().mkdirs();
+ assertThat(log.createNewFile()).isTrue();
+ }
+ }
+
protected AutoCloseable installPlugin(String pluginName, Class<? extends Module> sysModuleClass)
throws Exception {
return installPlugin(pluginName, sysModuleClass, null, null);
diff --git a/java/com/google/gerrit/entities/PatchSet.java b/java/com/google/gerrit/entities/PatchSet.java
index 354981c..8784437 100644
--- a/java/com/google/gerrit/entities/PatchSet.java
+++ b/java/com/google/gerrit/entities/PatchSet.java
@@ -170,6 +170,8 @@
public abstract Builder uploader(Account.Id uploader);
+ public abstract Builder realUploader(Account.Id realUploader);
+
public abstract Builder createdOn(Instant createdOn);
public abstract Builder groups(Iterable<String> groups);
@@ -204,6 +206,9 @@
/**
* Account that uploaded the patch set.
*
+ * <p>If the upload was done on behalf of another user, the impersonated user on whom's behalf the
+ * patch set was uploaded.
+ *
* <p>If this is a deserialized instance that was originally serialized by an older version of
* Gerrit, and the old data erroneously did not include an {@code uploader}, then this method will
* return an account ID of 0.
@@ -211,6 +216,15 @@
public abstract Account.Id uploader();
/**
+ * The real account that uploaded the patch set.
+ *
+ * <p>If this is a deserialized instance that was originally serialized by an older version of
+ * Gerrit, and the old data did not include an {@code realUploader}, then this method will return
+ * the {@code uploader}.
+ */
+ public abstract Account.Id realUploader();
+
+ /**
* When this patch set was first introduced onto the change.
*
* <p>If this is a deserialized instance that was originally serialized by an older version of
diff --git a/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java b/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
index 210972d..b32f09a 100644
--- a/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
@@ -42,6 +42,7 @@
.setId(patchSetIdConverter.toProto(patchSet.id()))
.setCommitId(objectIdConverter.toProto(patchSet.commitId()))
.setUploaderAccountId(accountIdConverter.toProto(patchSet.uploader()))
+ .setRealUploaderAccountId(accountIdConverter.toProto(patchSet.realUploader()))
.setCreatedOn(patchSet.createdOn().toEpochMilli());
List<String> groups = patchSet.groups();
if (!groups.isEmpty()) {
@@ -75,15 +76,20 @@
// Callers that encounter one of these sentinels will likely fail, for example by failing to
// look up the zeroId. They would have also failed back when the fields were nullable, for
// example with NPE; the current behavior just fails slightly differently.
+ Account.Id uploader =
+ proto.hasUploaderAccountId()
+ ? accountIdConverter.fromProto(proto.getUploaderAccountId())
+ : Account.id(0);
builder
.commitId(
proto.hasCommitId()
? objectIdConverter.fromProto(proto.getCommitId())
: ObjectId.zeroId())
- .uploader(
- proto.hasUploaderAccountId()
- ? accountIdConverter.fromProto(proto.getUploaderAccountId())
- : Account.id(0))
+ .uploader(uploader)
+ .realUploader(
+ proto.hasRealUploaderAccountId()
+ ? accountIdConverter.fromProto(proto.getRealUploaderAccountId())
+ : uploader)
.createdOn(
proto.hasCreatedOn() ? Instant.ofEpochMilli(proto.getCreatedOn()) : Instant.EPOCH);
diff --git a/java/com/google/gerrit/extensions/api/changes/RebaseInput.java b/java/com/google/gerrit/extensions/api/changes/RebaseInput.java
index e9b05cc..a85bc73 100644
--- a/java/com/google/gerrit/extensions/api/changes/RebaseInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/RebaseInput.java
@@ -27,5 +27,21 @@
*/
public boolean allowConflicts;
+ /**
+ * Whether the rebase should be done on behalf of the uploader.
+ *
+ * <p>This means the uploader of the current patch set will also be the uploader of the rebased
+ * patch set. The calling user will be recorded as the real user.
+ *
+ * <p>Rebasing on behalf of the uploader is only supported for trivial rebases. This means this
+ * option cannot be combined with the {@link #allowConflicts} option.
+ *
+ * <p>In addition, rebasing on behalf of the uploader is only supported for the current patch set
+ * of a change and not when rebasing a chain.
+ *
+ * <p>Using this option is not supported when rebasing a chain via the Rebase Chain REST endpoint.
+ */
+ public boolean onBehalfOfUploader;
+
public Map<String, String> validationOptions;
}
diff --git a/java/com/google/gerrit/extensions/common/ActionInfo.java b/java/com/google/gerrit/extensions/common/ActionInfo.java
index 2144ed5..f148444 100644
--- a/java/com/google/gerrit/extensions/common/ActionInfo.java
+++ b/java/com/google/gerrit/extensions/common/ActionInfo.java
@@ -15,6 +15,7 @@
package com.google.gerrit.extensions.common;
import com.google.gerrit.extensions.webui.UiAction;
+import java.util.List;
import java.util.Objects;
/**
@@ -50,11 +51,30 @@
*/
public Boolean enabled;
+ /**
+ * Optional list of enabled options.
+ *
+ * <p>For the {@code rebase} REST view the following options are supported:
+ *
+ * <ul>
+ * <li>{@code rebase}: Present if the user can rebase the change. This is the case for the
+ * change owner and users with the {@code Submit} or {@code Rebase} permission if they have
+ * the {@code Push} permission.
+ * <li>{@code rebase_on_behalf_of}: Present if the user can rebase the change on behalf of the
+ * uploader. This is the case for the change owner and users with the {@code Submit} or
+ * {@code Rebase} permission.
+ * </ul>
+ *
+ * <p>For all other REST views no options are returned.
+ */
+ public List<String> enabledOptions;
+
public ActionInfo(UiAction.Description d) {
method = d.getMethod();
label = d.getLabel();
title = d.getTitle();
enabled = d.isEnabled() ? true : null;
+ enabledOptions = d.getEnabledOptions();
}
@Override
@@ -64,14 +84,15 @@
return Objects.equals(method, actionInfo.method)
&& Objects.equals(label, actionInfo.label)
&& Objects.equals(title, actionInfo.title)
- && Objects.equals(enabled, actionInfo.enabled);
+ && Objects.equals(enabled, actionInfo.enabled)
+ && Objects.equals(enabledOptions, actionInfo.enabledOptions);
}
return false;
}
@Override
public int hashCode() {
- return Objects.hash(method, label, title, enabled);
+ return Objects.hash(method, label, title, enabled, enabledOptions);
}
protected ActionInfo() {}
diff --git a/java/com/google/gerrit/extensions/common/RevisionInfo.java b/java/com/google/gerrit/extensions/common/RevisionInfo.java
index 7c52c8c..941dffe 100644
--- a/java/com/google/gerrit/extensions/common/RevisionInfo.java
+++ b/java/com/google/gerrit/extensions/common/RevisionInfo.java
@@ -32,6 +32,7 @@
public Timestamp created;
public AccountInfo uploader;
+ public AccountInfo realUploader;
public String ref;
public Map<String, FetchInfo> fetch;
public CommitInfo commit;
@@ -72,6 +73,7 @@
&& _number == revisionInfo._number
&& Objects.equals(created, revisionInfo.created)
&& Objects.equals(uploader, revisionInfo.uploader)
+ && Objects.equals(realUploader, revisionInfo.realUploader)
&& Objects.equals(ref, revisionInfo.ref)
&& Objects.equals(fetch, revisionInfo.fetch)
&& Objects.equals(commit, revisionInfo.commit)
@@ -92,6 +94,7 @@
_number,
created,
uploader,
+ realUploader,
ref,
fetch,
commit,
diff --git a/java/com/google/gerrit/extensions/webui/UiAction.java b/java/com/google/gerrit/extensions/webui/UiAction.java
index 2f21bf3..9da0642 100644
--- a/java/com/google/gerrit/extensions/webui/UiAction.java
+++ b/java/com/google/gerrit/extensions/webui/UiAction.java
@@ -14,10 +14,13 @@
package com.google.gerrit.extensions.webui;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.RestResource;
import com.google.gerrit.extensions.restapi.RestView;
+import java.util.ArrayList;
+import java.util.List;
public interface UiAction<R extends RestResource> extends RestView<R> {
/**
@@ -40,6 +43,7 @@
private String title;
private BooleanCondition visible = BooleanCondition.TRUE;
private BooleanCondition enabled = BooleanCondition.TRUE;
+ private List<String> enabledOptions = new ArrayList<>();
public String getMethod() {
return method;
@@ -122,5 +126,22 @@
this.enabled = enabled;
return this;
}
+
+ @Nullable
+ public ImmutableList<String> getEnabledOptions() {
+ if (enabledOptions.isEmpty()) {
+ return null;
+ }
+ return ImmutableList.copyOf(enabledOptions);
+ }
+
+ public Description setOption(String optionName, boolean enabled) {
+ if (enabled) {
+ enabledOptions.add(optionName);
+ } else {
+ enabledOptions.remove(optionName);
+ }
+ return this;
+ }
}
}
diff --git a/java/com/google/gerrit/server/PatchSetUtil.java b/java/com/google/gerrit/server/PatchSetUtil.java
index 2962108..b34a56f 100644
--- a/java/com/google/gerrit/server/PatchSetUtil.java
+++ b/java/com/google/gerrit/server/PatchSetUtil.java
@@ -107,6 +107,7 @@
.id(psId)
.commitId(commit)
.uploader(update.getAccountId())
+ .realUploader(update.getRealAccountId())
.createdOn(update.getWhen())
.groups(groups)
.pushCertificate(Optional.ofNullable(pushCertificate))
diff --git a/java/com/google/gerrit/server/change/ActionJson.java b/java/com/google/gerrit/server/change/ActionJson.java
index ed6d53d..a9d5959 100644
--- a/java/com/google/gerrit/server/change/ActionJson.java
+++ b/java/com/google/gerrit/server/change/ActionJson.java
@@ -166,6 +166,7 @@
copy.ref = revisionInfo.ref;
copy.created = revisionInfo.created;
copy.uploader = revisionInfo.uploader;
+ copy.realUploader = revisionInfo.realUploader;
copy.fetch = revisionInfo.fetch;
copy.kind = revisionInfo.kind;
copy.description = revisionInfo.description;
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 49ec812..ed87c76 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -50,6 +50,7 @@
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
+import com.google.gerrit.server.util.AccountTemplateUtil;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
@@ -326,7 +327,8 @@
if (postMessage) {
patchSetInserter.setMessage(
- messageForRebasedChange(rebasedPatchSetId, originalPatchSet.id(), rebasedCommit));
+ messageForRebasedChange(
+ ctx.getIdentifiedUser(), rebasedPatchSetId, originalPatchSet.id(), rebasedCommit));
}
if (base != null && !base.notes().getChange().isMerged()) {
@@ -344,13 +346,22 @@
}
private static String messageForRebasedChange(
- PatchSet.Id rebasePatchSetId, PatchSet.Id originalPatchSetId, CodeReviewCommit commit) {
+ IdentifiedUser user,
+ PatchSet.Id rebasePatchSetId,
+ PatchSet.Id originalPatchSetId,
+ CodeReviewCommit commit) {
StringBuilder stringBuilder =
new StringBuilder(
String.format(
"Patch Set %d: Patch Set %d was rebased",
rebasePatchSetId.get(), originalPatchSetId.get()));
+ if (user.isImpersonating()) {
+ stringBuilder.append(
+ String.format(
+ " on behalf of %s", AccountTemplateUtil.getAccountTemplate(user.getAccountId())));
+ }
+
if (!commit.getFilesWithGitConflicts().isEmpty()) {
stringBuilder.append("\n\nThe following files contain Git conflicts:\n");
commit.getFilesWithGitConflicts().stream()
diff --git a/java/com/google/gerrit/server/change/RevisionJson.java b/java/com/google/gerrit/server/change/RevisionJson.java
index 5469b51..ce63c7e 100644
--- a/java/com/google/gerrit/server/change/RevisionJson.java
+++ b/java/com/google/gerrit/server/change/RevisionJson.java
@@ -288,6 +288,9 @@
out.ref = in.refName();
out.setCreated(in.createdOn());
out.uploader = accountLoader.get(in.uploader());
+ if (!in.uploader().equals(in.realUploader())) {
+ out.realUploader = accountLoader.get(in.realUploader());
+ }
out.fetch = makeFetchMap(cd, in);
out.kind = changeKindCache.getChangeKind(rw, repo != null ? repo.getConfig() : null, cd, in);
out.description = in.description().orElse(null);
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 1c89566f..2045dba 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -227,8 +227,7 @@
/** List of full file paths modified in the current patch set. */
public static final IndexedField<ChangeData, Iterable<String>> PATH_FIELD =
- // Named for backwards compatibility.
- IndexedField.<ChangeData>iterableStringBuilder("File")
+ IndexedField.<ChangeData>iterableStringBuilder("ModifiedFile")
.build(cd -> firstNonNull(cd.currentFilePaths(), ImmutableList.of()));
public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec PATH_SPEC =
@@ -770,7 +769,7 @@
/** Commit ID of any patch set on the change, using prefix match. */
public static final IndexedField<ChangeData, Iterable<String>> COMMIT_FIELD =
- IndexedField.<ChangeData>iterableStringBuilder("Commit")
+ IndexedField.<ChangeData>iterableStringBuilder("CommitId")
.size(40)
.required()
.build(ChangeField::getRevisions);
@@ -1222,7 +1221,7 @@
/** Determines if this change is private. */
public static final IndexedField<ChangeData, String> PRIVATE_FIELD =
- IndexedField.<ChangeData>stringBuilder("Private")
+ IndexedField.<ChangeData>stringBuilder("IsPrivate")
.size(1)
.build(cd -> cd.change().isPrivate() ? "1" : "0");
diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
index ba91c68..708d59f 100644
--- a/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
@@ -164,6 +164,10 @@
return accountId;
}
+ public Account.Id getRealAccountId() {
+ return realAccountId;
+ }
+
/** Whether no updates have been done. */
public abstract boolean isEmpty();
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 77d1c8f..467095c 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -515,7 +515,7 @@
ObjectId currRev = parseRevision(commit);
if (currRev != null) {
- parsePatchSet(psId, currRev, accountId, commitTimestamp);
+ parsePatchSet(psId, currRev, accountId, realAccountId, commitTimestamp);
}
parseCurrentPatchSet(psId, commit);
@@ -648,7 +648,8 @@
}
}
- private void parsePatchSet(PatchSet.Id psId, ObjectId rev, Account.Id accountId, Instant ts)
+ private void parsePatchSet(
+ PatchSet.Id psId, ObjectId rev, Account.Id accountId, Account.Id realAccountId, Instant ts)
throws ConfigInvalidException {
if (accountId == null) {
throw parseException("patch set %s requires an identified user as uploader", psId.get());
@@ -665,6 +666,7 @@
.id(psId)
.commitId(rev)
.uploader(accountId)
+ .realUploader(realAccountId)
.createdOn(ts);
// Fields not set here:
// * Groups, parsed earlier in parseGroups.
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index e36ce7b..b963361 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -84,6 +84,19 @@
&& refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
}
+ /**
+ * Can this user rebase this change on behalf of the uploader?
+ *
+ * <p>This only checks the permissions of the rebaser (aka the impersonating user).
+ *
+ * <p>In addition rebase on behalf of the uploader requires the uploader (aka the impersonated
+ * user) to have permissions to create the new patch set. These permissions need to be checked
+ * separately.
+ */
+ private boolean canRebaseOnBehalfOfUploader() {
+ return (isOwner() || refControl.canSubmit(isOwner()) || refControl.canRebase());
+ }
+
/** Can this user restore this change? */
private boolean canRestore() {
// Anyone who can abandon the change can restore it, as long as they can create changes.
@@ -272,6 +285,8 @@
return canEditTopicName();
case REBASE:
return canRebase();
+ case REBASE_ON_BEHALF_OF_UPLOADER:
+ return canRebaseOnBehalfOfUploader();
case RESTORE:
return canRestore();
case REVERT:
diff --git a/java/com/google/gerrit/server/permissions/ChangePermission.java b/java/com/google/gerrit/server/permissions/ChangePermission.java
index 6ceed3e..c456bf8 100644
--- a/java/com/google/gerrit/server/permissions/ChangePermission.java
+++ b/java/com/google/gerrit/server/permissions/ChangePermission.java
@@ -59,6 +59,19 @@
/* description= */ null,
/* hint= */ "change owners and users with the 'Submit' or 'Rebase' permission can rebase"
+ " if they have the 'Push' permission"),
+ /**
+ * Permission that is required for a user to rebase a change on behalf of the uploader.
+ *
+ * <p>This only covers the permissions of the rebaser (aka the impersonating user).
+ *
+ * <p>In addition rebase on behalf of the uploader requires the uploader (aka the impersonated
+ * user) to have permissions to create the new patch set. These permissions need to be checked
+ * separately.
+ */
+ REBASE_ON_BEHALF_OF_UPLOADER(
+ /* description= */ null,
+ /* hint= */ "change owners and users with the 'Submit' or 'Rebase' permission can rebase on"
+ + " behalf of the uploader"),
REVERT,
SUBMIT,
SUBMIT_AS("submit on behalf of other users"),
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index fc4df49..929aa94 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -276,6 +276,7 @@
.id(PatchSet.id(id, currentPatchSetId))
.commitId(commitId)
.uploader(Account.id(1000))
+ .realUploader(Account.id(1000))
.createdOn(TimeUtil.now())
.build();
return cd;
diff --git a/java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java b/java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java
index edb12ec..616468e 100644
--- a/java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java
@@ -29,6 +29,6 @@
/** See {@link com.google.gerrit.index.query.QueryBuilder#parse(String)}. */
Predicate<ProjectData> parse(String query) throws QueryParseException;
- /** See {@link com.google.gerrit.index.query.QueryBuilder#parse(List<String>)}. */
+ /** See {@link com.google.gerrit.index.query.QueryBuilder#parse(List)}. */
List<Predicate<ProjectData>> parse(List<String> queries) throws QueryParseException;
}
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 8a8d2ca..6535e42 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -18,16 +18,23 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiAction;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeResource;
@@ -37,27 +44,35 @@
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
public class Rebase
implements RestModifyView<RevisionResource, RebaseInput>, UiAction<RevisionResource> {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private static final ImmutableSet<ListChangesOption> OPTIONS =
Sets.immutableEnumSet(ListChangesOption.CURRENT_REVISION, ListChangesOption.CURRENT_COMMIT);
+ private final Provider<PersonIdent> serverIdent;
private final BatchUpdate.Factory updateFactory;
private final GitRepositoryManager repoManager;
private final RebaseUtil rebaseUtil;
@@ -65,16 +80,22 @@
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
private final PatchSetUtil patchSetUtil;
+ private final IdentifiedUser.GenericFactory userFactory;
+ private final ChangeResource.Factory changeResourceFactory;
@Inject
public Rebase(
+ @GerritPersonIdent Provider<PersonIdent> serverIdent,
BatchUpdate.Factory updateFactory,
GitRepositoryManager repoManager,
RebaseUtil rebaseUtil,
ChangeJson.Factory json,
PermissionBackend permissionBackend,
ProjectCache projectCache,
- PatchSetUtil patchSetUtil) {
+ PatchSetUtil patchSetUtil,
+ IdentifiedUser.GenericFactory userFactory,
+ ChangeResource.Factory changeResourceFactory) {
+ this.serverIdent = serverIdent;
this.updateFactory = updateFactory;
this.repoManager = repoManager;
this.rebaseUtil = rebaseUtil;
@@ -82,12 +103,20 @@
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.patchSetUtil = patchSetUtil;
+ this.userFactory = userFactory;
+ this.changeResourceFactory = changeResourceFactory;
}
@Override
public Response<ChangeInfo> apply(RevisionResource rsrc, RebaseInput input)
throws UpdateException, RestApiException, IOException, PermissionBackendException {
- rsrc.permissions().check(ChangePermission.REBASE);
+
+ if (input.onBehalfOfUploader && !rsrc.getPatchSet().uploader().equals(rsrc.getAccountId())) {
+ rsrc.permissions().check(ChangePermission.REBASE_ON_BEHALF_OF_UPLOADER);
+ rsrc = onBehalfOf(rsrc, input);
+ } else {
+ rsrc.permissions().check(ChangePermission.REBASE);
+ }
projectCache
.get(rsrc.getProject())
@@ -122,6 +151,143 @@
}
}
+ /**
+ * Checks that the uploader has permissions to create a new patch set and creates a new {@link
+ * RevisionResource} that contains the uploader (aka the impersonated user) as the current user
+ * which can be used for {@link BatchUpdate} to do the rebase on behalf of the uploader.
+ *
+ * <p>The following permissions are required for the uploader:
+ *
+ * <ul>
+ * <li>The {@code Read} permission that allows to see the change.
+ * <li>The {@code Push} permission that allows upload.
+ * <li>The {@code Add Patch Set} permission, required if the change is owned by another user
+ * (change owners implicitly have this permission).
+ * <li>The {@code Forge Author} permission if the patch set that is rebased has a forged author
+ * (author != uploader).
+ * <li>The {@code Forge Server} permission if the patch set that is rebased has the server
+ * identity as the author.
+ * </ul>
+ *
+ * <p>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.
+ *
+ * <p>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 {@code 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 {@code Forge Committer} permission is not
+ * required.
+ *
+ * <p>Note that the {@code Rebase} permission is not required for the uploader since the {@code
+ * Rebase} permission is specifically about allowing a user to do a rebase via the web UI by
+ * clicking on the {@code REBASE} button and the uploader is not clicking on this button.
+ *
+ * <p>The permissions of the uploader are checked explicitly here so that we can return a {@code
+ * 409 Conflict} response with a proper error message if they are missing (the error message says
+ * 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 {@code 403 Forbidden} response and
+ * the error message would wrongly look like the caller (i.e. the rebaser) is missing the
+ * permission.
+ *
+ * <p>Note that this method doesn't check permissions for the rebaser (aka the impersonating user
+ * aka the calling user). Callers should check the permissions for the rebaser before calling this
+ * method.
+ *
+ * @param rsrc the revision resource that should be rebased
+ * @param rebaseInput the request input containing options for the rebase
+ * @return revision resource that contains the uploader (aka the impersonated user) as the current
+ * user which can be used for {@link BatchUpdate} to do the rebase on behalf of the uploader
+ */
+ private RevisionResource onBehalfOf(RevisionResource rsrc, RebaseInput rebaseInput)
+ throws IOException, PermissionBackendException, BadRequestException,
+ ResourceConflictException {
+ if (rsrc.getPatchSet().id().get() != rsrc.getChange().currentPatchSetId().get()) {
+ throw new BadRequestException(
+ "non-current patch set cannot be rebased on behalf of the uploader");
+ }
+ if (rebaseInput.allowConflicts) {
+ throw new BadRequestException(
+ "allow_conflicts and on_behalf_of_uploader are mutually exclusive");
+ }
+
+ CurrentUser caller = rsrc.getUser();
+ Account.Id uploaderId = rsrc.getPatchSet().uploader();
+ IdentifiedUser uploader = userFactory.runAs(/*remotePeer= */ null, uploaderId, caller);
+ logger.atFine().log(
+ "%s is rebasing patch set %s of project %s on behalf of uploader %s",
+ caller.getLoggableName(),
+ rsrc.getPatchSet().id(),
+ rsrc.getProject(),
+ uploader.getLoggableName());
+
+ checkPermissionForUploader(
+ uploader,
+ rsrc.getNotes(),
+ ChangePermission.READ,
+ String.format("uploader %s cannot read change", uploader.getLoggableName()));
+ checkPermissionForUploader(
+ uploader,
+ rsrc.getNotes(),
+ ChangePermission.ADD_PATCH_SET,
+ String.format("uploader %s cannot add patch set", uploader.getLoggableName()));
+
+ try (Repository repo = repoManager.openRepository(rsrc.getProject())) {
+ RevCommit commit = repo.parseCommit(rsrc.getPatchSet().commitId());
+
+ if (!uploader.hasEmailAddress(commit.getAuthorIdent().getEmailAddress())) {
+ checkPermissionForUploader(
+ uploader,
+ rsrc.getNotes(),
+ RefPermission.FORGE_AUTHOR,
+ String.format(
+ "author of patch set %d is forged and the uploader %s cannot forge author",
+ rsrc.getPatchSet().id().get(), uploader.getLoggableName()));
+
+ if (serverIdent.get().getEmailAddress().equals(commit.getAuthorIdent().getEmailAddress())) {
+ checkPermissionForUploader(
+ uploader,
+ rsrc.getNotes(),
+ RefPermission.FORGE_SERVER,
+ String.format(
+ "author of patch set %d is the server identity and the uploader %s cannot forge"
+ + " the server identity",
+ rsrc.getPatchSet().id().get(), uploader.getLoggableName()));
+ }
+ }
+ }
+
+ return new RevisionResource(
+ changeResourceFactory.create(rsrc.getNotes(), uploader), rsrc.getPatchSet());
+ }
+
+ private void checkPermissionForUploader(
+ IdentifiedUser uploader,
+ ChangeNotes changeNotes,
+ ChangePermission changePermission,
+ String errorMessage)
+ throws PermissionBackendException, ResourceConflictException {
+ try {
+ permissionBackend.user(uploader).change(changeNotes).check(changePermission);
+ } catch (AuthException e) {
+ throw new ResourceConflictException(errorMessage, e);
+ }
+ }
+
+ private void checkPermissionForUploader(
+ IdentifiedUser uploader,
+ ChangeNotes changeNotes,
+ RefPermission refPermission,
+ String errorMessage)
+ throws PermissionBackendException, ResourceConflictException {
+ try {
+ permissionBackend.user(uploader).ref(changeNotes.getChange().getDest()).check(refPermission);
+ } catch (AuthException e) {
+ throw new ResourceConflictException(errorMessage, e);
+ }
+ }
+
@Override
public UiAction.Description getDescription(RevisionResource rsrc) throws IOException {
UiAction.Description description =
@@ -154,9 +320,17 @@
}
}
- if (rsrc.permissions().testOrFalse(ChangePermission.REBASE)) {
- return description.setVisible(true).setEnabled(enabled);
+ boolean canRebase = rsrc.permissions().testOrFalse(ChangePermission.REBASE);
+ boolean canRebaseOnBehalfOfUploader =
+ rsrc.permissions().testOrFalse(ChangePermission.REBASE_ON_BEHALF_OF_UPLOADER);
+ if (canRebase || canRebaseOnBehalfOfUploader) {
+ return description
+ .setOption("rebase", canRebase)
+ .setOption("rebase_on_behalf_of_uploader", canRebaseOnBehalfOfUploader)
+ .setEnabled(enabled)
+ .setVisible(true);
}
+
return description;
}
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 786bba7..5ae496f 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -26,6 +26,7 @@
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.RebaseChainInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -115,6 +116,11 @@
throws IOException, PermissionBackendException, RestApiException, UpdateException {
tipRsrc.permissions().check(ChangePermission.REBASE);
+ if (input.onBehalfOfUploader) {
+ throw new BadRequestException(
+ "rebasing on behalf of the uploader is not supported when rebasing a chain");
+ }
+
Project.NameKey project = tipRsrc.getProject();
projectCache.get(project).orElseThrow(illegalState(project)).checkStatePermitsWrite();
diff --git a/java/com/google/gerrit/server/restapi/change/Revisions.java b/java/com/google/gerrit/server/restapi/change/Revisions.java
index bdc6816..8ab8a19 100644
--- a/java/com/google/gerrit/server/restapi/change/Revisions.java
+++ b/java/com/google/gerrit/server/restapi/change/Revisions.java
@@ -157,6 +157,7 @@
.id(PatchSet.id(change.getId(), 0))
.commitId(editCommit)
.uploader(change.getUser().getAccountId())
+ .realUploader(change.getUser().getAccountId())
.createdOn(editCommit.getCommitterIdent().getWhenAsInstant())
.build();
if (commitId == null || editCommit.equals(commitId)) {
diff --git a/java/com/google/gerrit/testing/TestChanges.java b/java/com/google/gerrit/testing/TestChanges.java
index c8f89cf..5a3c755 100644
--- a/java/com/google/gerrit/testing/TestChanges.java
+++ b/java/com/google/gerrit/testing/TestChanges.java
@@ -73,6 +73,7 @@
.id(id)
.commitId(ObjectId.fromString(revision))
.uploader(userId)
+ .realUploader(userId)
.createdOn(TimeUtil.now())
.build();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
index 522013e..b1fb575 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
@@ -539,6 +539,7 @@
RevisionInfo r = info.revisions.get(info.currentRevision);
assertThat(r._number).isEqualTo(expectedNumRevisions);
+ assertThat(r.realUploader).isNull();
// ...and the base should be correct
assertThat(r.commit.parents).hasSize(1);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
new file mode 100644
index 0000000..7030804
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
@@ -0,0 +1,1073 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.api.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.notedb.ChangeNoteFooters.FOOTER_REAL_USER;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.ExtensionRegistry;
+import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
+import com.google.gerrit.acceptance.UseLocalDisk;
+import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Permission;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmitRequirement;
+import com.google.gerrit.entities.SubmitRequirementExpression;
+import com.google.gerrit.extensions.api.changes.RebaseInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.ActionInfo;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeMessageInfo;
+import com.google.gerrit.extensions.common.RevisionInfo;
+import com.google.gerrit.extensions.events.RevisionCreatedListener;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.notedb.ChangeNoteUtil;
+import com.google.gerrit.server.project.testing.TestLabels;
+import com.google.gerrit.server.util.AccountTemplateUtil;
+import com.google.inject.Inject;
+import java.util.Collection;
+import java.util.Optional;
+import org.eclipse.jgit.lib.ReflogEntry;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.FooterLine;
+import org.junit.Test;
+
+public class RebaseOnBehalfOfUploaderIT extends AbstractDaemonTest {
+ @Inject private AccountOperations accountOperations;
+ @Inject private ChangeOperations changeOperations;
+ @Inject private GroupOperations groupOperations;
+ @Inject private ProjectOperations projectOperations;
+ @Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ExtensionRegistry extensionRegistry;
+
+ @Test
+ public void cannotRebaseOnBehalfOfUploaderWithAllowConflicts() throws Exception {
+ Account.Id uploader = accountOperations.newAccount().create();
+ Change.Id changeId = changeOperations.newChange().owner(uploader).create();
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ rebaseInput.allowConflicts = true;
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class, () -> gApi.changes().id(changeId.get()).rebase(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo("allow_conflicts and on_behalf_of_uploader are mutually exclusive");
+ }
+
+ @Test
+ public void cannotRebaseNonCurrentPatchSetOnBehalfOfUploader() throws Exception {
+ Account.Id uploader = accountOperations.newAccount().create();
+ Change.Id changeId = changeOperations.newChange().owner(uploader).create();
+ changeOperations.change(changeId).newPatchset().create();
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(changeId.get()).revision(1).rebase(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo("non-current patch set cannot be rebased on behalf of the uploader");
+ }
+
+ @Test
+ public void cannotRebaseChainOnBehalfOfUploader() throws Exception {
+ Change.Id changeId = changeOperations.newChange().create();
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(changeId.get()).rebaseChain(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo("rebasing on behalf of the uploader is not supported when rebasing a chain");
+ }
+
+ @Test
+ public void rebaseChangeOnBehalfOfUploader_withRebasePermission() throws Exception {
+ testRebaseChangeOnBehalfOfUploader(
+ Permission.REBASE,
+ (changeId, rebaseInput) -> gApi.changes().id(changeId.get()).rebase(rebaseInput));
+ }
+
+ @Test
+ public void rebaseCurrentPatchSetOnBehalfOfUploader_withRebasePermission() throws Exception {
+ testRebaseChangeOnBehalfOfUploader(
+ Permission.REBASE,
+ (changeId, rebaseInput) -> gApi.changes().id(changeId.get()).current().rebase(rebaseInput));
+ }
+
+ @Test
+ public void rebaseChangeOnBehalfOfUploader_withSubmitPermission() throws Exception {
+ testRebaseChangeOnBehalfOfUploader(
+ Permission.SUBMIT,
+ (changeId, rebaseInput) -> gApi.changes().id(changeId.get()).rebase(rebaseInput));
+ }
+
+ @Test
+ public void rebaseCurrentPatchSetOnBehalfOfUploader_withSubmitPermission() throws Exception {
+ testRebaseChangeOnBehalfOfUploader(
+ Permission.SUBMIT,
+ (changeId, rebaseInput) -> gApi.changes().id(changeId.get()).current().rebase(rebaseInput));
+ }
+
+ private void testRebaseChangeOnBehalfOfUploader(String permissionToAllow, RebaseCall rebaseCall)
+ throws Exception {
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id changeOwner = accountOperations.newAccount().create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Grant permission to rebaser that is required to rebase on behalf of the uploader.
+ AccountGroup.UUID allowedGroup =
+ groupOperations.newGroup().name("can-" + permissionToAllow).addMember(rebaser).create();
+ allowPermission(permissionToAllow, allowedGroup);
+
+ // Block rebase and submit permission for uploader. For rebase on behalf of the uploader only
+ // the rebaser needs to have this permission, but not the uploader on whom's behalf the rebase
+ // is done.
+ AccountGroup.UUID cannotRebaseAndSubmitGroup =
+ groupOperations.newGroup().name("cannot-rebase").addMember(uploader).create();
+ blockPermission(Permission.REBASE, cannotRebaseAndSubmitGroup);
+ blockPermission(Permission.SUBMIT, cannotRebaseAndSubmitGroup);
+
+ // Block push permission for rebaser, as in contrast to rebase, rebase on behalf of the uploader
+ // doesn't require the rebaser to have the push permission.
+ AccountGroup.UUID cannotUploadGroup =
+ groupOperations.newGroup().name("cannot-upload").addMember(rebaser).create();
+ blockPermission(Permission.PUSH, cannotUploadGroup);
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(changeOwner);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(changeOwner).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(changeOwner).create();
+
+ // Create a second patch set for the change that will be rebased so that the uploader is
+ // different to the change owner. This is to verify that being change owner doesn't matter for
+ // the user on whom's behalf the rebase is done.
+ // Set author and committer to the uploader so that rebasing on behalf of the uploader doesn't
+ // require the Forge Author and Forge Committer permission.
+ changeOperations
+ .change(changeToBeRebased)
+ .newPatchset()
+ .uploader(uploader)
+ .author(uploader)
+ .committer(uploader)
+ .create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+
+ TestRevisionCreatedListener testRevisionCreatedListener = new TestRevisionCreatedListener();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(testRevisionCreatedListener)) {
+ rebaseCall.call(changeToBeRebased, rebaseInput);
+
+ assertThat(testRevisionCreatedListener.revisionInfo.uploader._accountId)
+ .isEqualTo(uploader.get());
+ assertThat(testRevisionCreatedListener.revisionInfo.realUploader._accountId)
+ .isEqualTo(rebaser.get());
+ }
+
+ ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 2 patch sets before the rebase, now it should be 3
+ assertThat(currentRevisionInfo._number).isEqualTo(3);
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader._accountId).isEqualTo(rebaser.get());
+ assertThat(currentRevisionInfo.commit.author.email).isEqualTo(uploaderEmail);
+ assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
+
+ // Verify that the rebaser was recorded as realUser in NoteDb.
+ Optional<FooterLine> realUserFooter =
+ projectOperations.project(project).getHead(RefNames.changeMetaRef(changeToBeRebased))
+ .getFooterLines().stream()
+ .filter(footerLine -> footerLine.matches(FOOTER_REAL_USER))
+ .findFirst();
+ assertThat(realUserFooter.map(FooterLine::getValue))
+ .hasValue(
+ String.format(
+ "%s <%s>",
+ ChangeNoteUtil.getAccountIdAsUsername(rebaser),
+ changeNoteUtil.getAccountIdAsEmailAddress(rebaser)));
+
+ // Verify the message that has been posted on the change.
+ Collection<ChangeMessageInfo> changeMessages = changeInfo2.messages;
+ // Before the rebase the change had 2 messages for the upload of the 2 patch sets. Rebase is
+ // expected to add another message.
+ assertThat(changeMessages).hasSize(3);
+ ChangeMessageInfo changeMessage = Iterables.getLast(changeMessages);
+ assertThat(changeMessage.message)
+ .isEqualTo(
+ "Patch Set 3: Patch Set 2 was rebased on behalf of "
+ + AccountTemplateUtil.getAccountTemplate(uploader));
+ assertThat(changeMessage.author._accountId).isEqualTo(uploader.get());
+ assertThat(changeMessage.realAuthor._accountId).isEqualTo(rebaser.get());
+ }
+
+ @Test
+ public void rebaseChangeOnBehalfOfUploaderMultipleTimesInARow() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent.
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 1 patch set before the rebase, now it should be 2
+ assertThat(currentRevisionInfo._number).isEqualTo(2);
+ assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader._accountId).isEqualTo(rebaser.get());
+
+ // Create and submit another change so that we can rebase the change once again.
+ requestScopeOperations.setApiUser(approver);
+ Change.Id changeToBeTheNewBase2 =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ gApi.changes().id(changeToBeTheNewBase2.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase2.get()).current().submit();
+
+ // Rebase the change once again on behalf of the uploader.
+ requestScopeOperations.setApiUser(rebaser);
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 2 patch sets before the rebase, now it should be 3
+ assertThat(currentRevisionInfo._number).isEqualTo(3);
+ assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader._accountId).isEqualTo(rebaser.get());
+
+ // Create and submit another change so that we can rebase the change once again.
+ requestScopeOperations.setApiUser(approver);
+ Change.Id changeToBeTheNewBase3 =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ gApi.changes().id(changeToBeTheNewBase3.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase3.get()).current().submit();
+
+ // Rebase the change once again on behalf of the uploader, this time by another rebaser.
+ Account.Id rebaser2 = accountOperations.newAccount().create();
+ requestScopeOperations.setApiUser(rebaser2);
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 3 patch sets before the rebase, now it should be 4
+ assertThat(currentRevisionInfo._number).isEqualTo(4);
+ assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader._accountId).isEqualTo(rebaser2.get());
+ }
+
+ @Test
+ public void nonChangeOwnerWithoutSubmitAndRebasePermissionCannotRebaseOnBehalfOfUploader()
+ throws Exception {
+ Change.Id changeId = changeOperations.newChange().project(project).create();
+
+ blockPermissionForAllUsers(Permission.REBASE);
+ blockPermissionForAllUsers(Permission.SUBMIT);
+
+ Account.Id rebaserId = accountOperations.newAccount().create();
+ requestScopeOperations.setApiUser(rebaserId);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ AuthException exception =
+ assertThrows(
+ AuthException.class, () -> gApi.changes().id(changeId.get()).rebase(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ "rebase on behalf of uploader not permitted (change owners and users with the 'Submit'"
+ + " or 'Rebase' permission can rebase on behalf of the uploader)");
+ }
+
+ @Test
+ public void cannotRebaseChangeOnBehalfOfUploaderIfTheUploaderHasNoReadPermission()
+ throws Exception {
+ String uploaderEmail = "uploader@example.com";
+ testCannotRebaseChangeOnBehalfOfUploaderIfTheUploaderHasNoPermission(
+ uploaderEmail,
+ Permission.READ,
+ String.format("uploader %s cannot read change", uploaderEmail));
+ }
+
+ @Test
+ public void cannotRebaseChangeOnBehalfOfUploaderIfTheUploaderHasNoPushPermission()
+ throws Exception {
+ String uploaderEmail = "uploader@example.com";
+ testCannotRebaseChangeOnBehalfOfUploaderIfTheUploaderHasNoPermission(
+ uploaderEmail,
+ Permission.PUSH,
+ String.format("uploader %s cannot add patch set", uploaderEmail));
+ }
+
+ private void testCannotRebaseChangeOnBehalfOfUploaderIfTheUploaderHasNoPermission(
+ String uploaderEmail, String permissionToBlock, String expectedErrorMessage)
+ throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Block the required permission for uploader. Without this permission it should not be possible
+ // to rebase the change on behalf of the uploader.
+ AccountGroup.UUID blockedGroup =
+ groupOperations.newGroup().name("cannot-" + permissionToBlock).addMember(uploader).create();
+ blockPermission(permissionToBlock, blockedGroup);
+
+ // Try to rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput));
+ assertThat(exception).hasMessageThat().isEqualTo(expectedErrorMessage);
+ }
+
+ @Test
+ public void rebaseChangeOnBehalfOfYourself() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ Account.Id uploader =
+ accountOperations.newAccount().preferredEmail("uploader@example.com").create();
+ Account.Id approver = admin.id();
+
+ // Create two changes both with the same parent. Forge the author of the change that will be
+ // rebased.
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Rebase the second change as uploader on behalf of the uploader
+ requestScopeOperations.setApiUser(uploader);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 1 patch set before the rebase, now it should be 2
+ assertThat(currentRevisionInfo._number).isEqualTo(2);
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader).isNull();
+ }
+
+ @Test
+ public void cannotRebaseChangeOnBehalfOfYourselfWithoutPushPermission() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ Account.Id uploader =
+ accountOperations.newAccount().preferredEmail("uploader@example.com").create();
+ Account.Id approver = admin.id();
+
+ // Create two changes both with the same parent. Forge the author of the change that will be
+ // rebased.
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Block push for uploader. For rebase on behalf of the uploader only
+ // the rebaser needs to have this permission, but not the uploader on whom's behalf the rebase
+ // is done.
+ AccountGroup.UUID cannotPushGroup =
+ groupOperations.newGroup().name("cannot-push").addMember(uploader).create();
+ blockPermission(Permission.PUSH, cannotPushGroup);
+
+ // Rebase the second change as uploader on behalf of the uploader
+ requestScopeOperations.setApiUser(uploader);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ AuthException exception =
+ assertThrows(
+ AuthException.class,
+ () -> gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ "rebase not permitted (change owners and users with the 'Submit' or 'Rebase'"
+ + " permission can rebase if they have the 'Push' permission)");
+ }
+
+ @Test
+ public void rebaseChangeOnBehalfOfUploaderWhenUploaderIsNotTheChangeOwner() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ Account.Id changeOwner = accountOperations.newAccount().create();
+ Account.Id uploader =
+ accountOperations.newAccount().preferredEmail("uploader@example.com").create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent. Forge the author of the change that will be
+ // rebased.
+ requestScopeOperations.setApiUser(changeOwner);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(changeOwner).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(changeOwner).create();
+
+ // Create a second patch set for the change that will be rebased so that the uploader is
+ // different to the change owner.
+ // Set author and committer to the uploader so that rebasing on behalf of the uploader doesn't
+ // require the Forge Author and Forge Committer permission.
+ changeOperations
+ .change(changeToBeRebased)
+ .newPatchset()
+ .uploader(uploader)
+ .author(uploader)
+ .committer(uploader)
+ .create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Grant add patch set permission for uploader. Without the add patch set permission it is not
+ // possible to rebase the change on behalf of the uploader since the uploader cannot add a
+ // patch set to a change that is owned by another user.
+ AccountGroup.UUID canAddPatchSet =
+ groupOperations.newGroup().name("can-add-patch-set").addMember(uploader).create();
+ allowPermission(Permission.ADD_PATCH_SET, canAddPatchSet);
+
+ // Rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 2 patch set before the rebase, now it should be 3
+ assertThat(currentRevisionInfo._number).isEqualTo(3);
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader._accountId).isEqualTo(rebaser.get());
+ }
+
+ @Test
+ public void
+ cannotRebaseChangeOnBehalfOfUploaderWhenUploaderIsNotTheChangeOwnerAndDoesntHaveAddPatchSetPermission()
+ throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id changeOwner = accountOperations.newAccount().create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent. Forge the author of the change that will be
+ // rebased.
+ requestScopeOperations.setApiUser(changeOwner);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(changeOwner).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(changeOwner).create();
+
+ // Create a second patch set for the change that will be rebased so that the uploader is
+ // different to the change owner.
+ // Set author and committer to the uploader so that rebasing on behalf of the uploader doesn't
+ // require the Forge Author and Forge Committer permission.
+ changeOperations
+ .change(changeToBeRebased)
+ .newPatchset()
+ .uploader(uploader)
+ .author(uploader)
+ .committer(uploader)
+ .create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Block add patch set permission for uploader. Without the add patch set permission it should
+ // not possible to rebase the change on behalf of the uploader since the uploader cannot add a
+ // patch set to a change that is owned by another user.
+ AccountGroup.UUID cannotAddPatchSet =
+ groupOperations.newGroup().name("cannot-add-patch-set").addMember(uploader).create();
+ blockPermission(Permission.ADD_PATCH_SET, cannotAddPatchSet);
+
+ // Try to rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(String.format("uploader %s cannot add patch set", uploaderEmail));
+ }
+
+ @Test
+ public void rebaseChangeWithForgedAuthorOnBehalfOfUploader() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String authorEmail = "author@example.com";
+ Account.Id author = accountOperations.newAccount().preferredEmail(authorEmail).create();
+ Account.Id uploader =
+ accountOperations.newAccount().preferredEmail("uploader@example.com").create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent. Forge the author of the change that will be
+ // rebased.
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).author(author).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Grant forge author permission for uploader. Without the forge author permission it is not
+ // possible to rebase the change on behalf of the uploader.
+ AccountGroup.UUID canForgeAuthor =
+ groupOperations.newGroup().name("can-forge-author").addMember(uploader).create();
+ allowPermission(Permission.FORGE_AUTHOR, canForgeAuthor);
+
+ // Rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 1 patch set before the rebase, now it should be 2
+ assertThat(currentRevisionInfo._number).isEqualTo(2);
+ assertThat(currentRevisionInfo.commit.author.email).isEqualTo(authorEmail);
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader._accountId).isEqualTo(rebaser.get());
+ }
+
+ @Test
+ public void
+ cannotRebaseChangeWithForgedAuthorOnBehalfOfUploaderIfTheUploaderHasNoForgeAuthorPermission()
+ throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id author = accountOperations.newAccount().create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent. Forge the author of the change that will be
+ // rebased.
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).author(author).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Block forge author permission for uploader. Without the forge author permission it should not
+ // be possible to rebase the change on behalf of the uploader.
+ AccountGroup.UUID cannotForgeAuthor =
+ groupOperations.newGroup().name("cannot-forge-author").addMember(uploader).create();
+ blockPermission(Permission.FORGE_AUTHOR, cannotForgeAuthor);
+
+ // Try to rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "author of patch set 1 is forged and the uploader %s cannot forge author",
+ uploaderEmail));
+ }
+
+ @Test
+ public void
+ rebaseChangeWithForgedCommitterOnBehalfOfUploaderDoesntRequireForgeCommitterPermission()
+ throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id committer =
+ accountOperations.newAccount().preferredEmail("committer@example.com").create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent. Forge the committer of the change that will be
+ // rebased.
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).committer(committer).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 1 patch set before the rebase, now it should be 2
+ assertThat(currentRevisionInfo._number).isEqualTo(2);
+ assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader._accountId).isEqualTo(rebaser.get());
+ }
+
+ @Test
+ public void rebaseChangeWithServerIdentOnBehalfOfUploader() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent. Use the server identity as the author of the
+ // change that will be rebased.
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations
+ .newChange()
+ .project(project)
+ .owner(uploader)
+ .authorIdent(serverIdent.get())
+ .create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Grant forge author and forge server permission for uploader. Without these permissions it is
+ // not possible to rebase the change on behalf of the uploader.
+ AccountGroup.UUID canForgeAuthorAndForgeServer =
+ groupOperations
+ .newGroup()
+ .name("can-forge-author-and-forge-server")
+ .addMember(uploader)
+ .create();
+ allowPermission(Permission.FORGE_AUTHOR, canForgeAuthorAndForgeServer);
+ allowPermission(Permission.FORGE_SERVER, canForgeAuthorAndForgeServer);
+
+ // Rebase the second change on behalf of the uploader.
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ // The change had 1 patch set before the rebase, now it should be 2
+ assertThat(currentRevisionInfo._number).isEqualTo(2);
+ assertThat(currentRevisionInfo.commit.author.email)
+ .isEqualTo(serverIdent.get().getEmailAddress());
+ assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
+ assertThat(currentRevisionInfo.realUploader._accountId).isEqualTo(rebaser.get());
+ }
+
+ @Test
+ public void
+ cannotRebaseChangeWithServerIdentOnBehalfOfUploaderIfTheUploaderHasNoForgeServerPermission()
+ throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent. Use the server identity as the author of the
+ // change that will be rebased.
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations
+ .newChange()
+ .project(project)
+ .owner(uploader)
+ .authorIdent(serverIdent.get())
+ .create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Grant forge author permission for uploader, but not the forge server permission. Without the
+ // forge server permission it is not possible to rebase the change on behalf of the uploader.
+ AccountGroup.UUID canForgeAuthor =
+ groupOperations.newGroup().name("can-forge-author").addMember(uploader).create();
+ allowPermission(Permission.FORGE_AUTHOR, canForgeAuthor);
+
+ // Try to rebase the second change on behalf of the uploader.
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "author of patch set 1 is the server identity and the uploader %s cannot forge"
+ + " the server identity",
+ uploaderEmail));
+ }
+
+ @Test
+ public void rebaseActionEnabled_withRebasePermission() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+ testRebaseActionEnabled();
+ }
+
+ @Test
+ public void rebaseActionEnabled_withSubmitPermission() throws Exception {
+ allowPermissionToAllUsers(Permission.SUBMIT);
+ testRebaseActionEnabled();
+ }
+
+ private void testRebaseActionEnabled() throws Exception {
+ Account.Id uploader = accountOperations.newAccount().create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Block push permission for rebaser, as in contrast to rebase, rebase on behalf of the uploader
+ // doesn't require the rebaser to have the push permission.
+ AccountGroup.UUID cannotUploadGroup =
+ groupOperations.newGroup().name("cannot-upload").addMember(rebaser).create();
+ blockPermission(Permission.PUSH, cannotUploadGroup);
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ requestScopeOperations.setApiUser(rebaser);
+ ChangeInfo changeInfo = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo revisionInfo = changeInfo.revisions.get(changeInfo.currentRevision);
+ assertThat(revisionInfo.actions).containsKey("rebase");
+ ActionInfo rebaseActionInfo = revisionInfo.actions.get("rebase");
+ assertThat(rebaseActionInfo.enabled).isTrue();
+
+ // rebase is disabled because rebaser doesn't have the 'Push' permission and hence cannot create
+ // new patch sets
+ assertThat(rebaseActionInfo.enabledOptions).containsExactly("rebase_on_behalf_of_uploader");
+ }
+
+ @Test
+ public void rebaseActionEnabled_forChangeOwner() throws Exception {
+ Account.Id changeOwner = accountOperations.newAccount().create();
+ Account.Id approver = admin.id();
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(changeOwner);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(changeOwner).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(changeOwner).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ requestScopeOperations.setApiUser(changeOwner);
+ ChangeInfo changeInfo = gApi.changes().id(changeToBeRebased.get()).get();
+ RevisionInfo revisionInfo = changeInfo.revisions.get(changeInfo.currentRevision);
+ assertThat(revisionInfo.actions).containsKey("rebase");
+ ActionInfo rebaseActionInfo = revisionInfo.actions.get("rebase");
+ assertThat(rebaseActionInfo.enabled).isTrue();
+
+ // rebase is disabled because change owner has the 'Push' permission and hence can create new
+ // patch sets
+ assertThat(rebaseActionInfo.enabledOptions)
+ .containsExactly("rebase", "rebase_on_behalf_of_uploader");
+ }
+
+ @UseLocalDisk
+ @Test
+ public void rebaseChangeOnBehalfOfUploaderRecordsUploaderInRefLog() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ try (Repository repo = repoManager.openRepository(project)) {
+ String changeMetaRef = RefNames.changeMetaRef(changeToBeRebased);
+ String patchSetRef = RefNames.patchSetRef(PatchSet.id(changeToBeRebased, 2));
+ createRefLogFileIfMissing(repo, changeMetaRef);
+ createRefLogFileIfMissing(repo, patchSetRef);
+
+ // Rebase the second change on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ // The ref log for the patch set ref records the impersonated user aka the uploader.
+ ReflogEntry patchSetRefLogEntry = repo.getReflogReader(patchSetRef).getLastEntry();
+ assertThat(patchSetRefLogEntry.getWho().getEmailAddress()).isEqualTo(uploaderEmail);
+
+ // The ref log for the change meta ref records the impersonated user aka the uploader.
+ ReflogEntry changeMetaRefLogEntry = repo.getReflogReader(changeMetaRef).getLastEntry();
+ assertThat(changeMetaRefLogEntry.getWho().getEmailAddress()).isEqualTo(uploaderEmail);
+ }
+ }
+
+ @Test
+ public void rebaserCanApproveChangeAfterRebasingOnBehalfOfUploader() throws Exception {
+ // Require a Code-Review approval from a non-uploader for submit.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .upsertSubmitRequirement(
+ SubmitRequirement.builder()
+ .setName(TestLabels.codeReview().getName())
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create(
+ String.format(
+ "label:%s=MAX,user=non_uploader", TestLabels.codeReview().getName())))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ u.save();
+ }
+
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ String uploaderEmail = "uploader@example.com";
+ Account.Id uploader = accountOperations.newAccount().preferredEmail(uploaderEmail).create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Rebase it on behalf of the uploader
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+
+ // Approve the change as the rebaser.
+ allowVotingOnCodeReviewToAllUsers();
+ gApi.changes().id(changeToBeRebased.get()).current().review(ReviewInput.approve());
+
+ // The change is submittable because the approval is from a user (the rebaser) that is not the
+ // uploader.
+ assertThat(gApi.changes().id(changeToBeRebased.get()).get().submittable).isTrue();
+
+ // Create and submit another change so that we can rebase the change once again.
+ requestScopeOperations.setApiUser(approver);
+ Change.Id changeToBeTheNewBase2 =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ gApi.changes().id(changeToBeTheNewBase2.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase2.get()).current().submit();
+
+ // Doing a normal rebase (not on behalf of the uploader) makes the rebaser the uploader. This
+ // makse the change non-submittable since the approval of the rebaser is ignored now (due to
+ // using 'user=non_uploader' in the submit requirement expression).
+ requestScopeOperations.setApiUser(rebaser);
+ rebaseInput.onBehalfOfUploader = false;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+ gApi.changes().id(changeToBeRebased.get()).current().review(ReviewInput.approve());
+ assertThat(gApi.changes().id(changeToBeRebased.get()).get().submittable).isFalse();
+ }
+
+ private void allowPermissionToAllUsers(String permission) {
+ allowPermission(permission, REGISTERED_USERS);
+ }
+
+ private void allowPermission(String permission, AccountGroup.UUID groupUuid) {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(permission).ref("refs/*").group(groupUuid))
+ .update();
+ }
+
+ private void allowVotingOnCodeReviewToAllUsers() {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel(TestLabels.codeReview().getName())
+ .ref("refs/*")
+ .group(REGISTERED_USERS)
+ .range(-2, 2))
+ .update();
+ }
+
+ private void blockPermissionForAllUsers(String permission) {
+ blockPermission(permission, REGISTERED_USERS);
+ }
+
+ private void blockPermission(String permission, AccountGroup.UUID groupUuid) {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(permission).ref("refs/*").group(groupUuid))
+ .update();
+ }
+
+ @FunctionalInterface
+ private interface RebaseCall {
+ void call(Change.Id changeId, RebaseInput rebaseInput) throws RestApiException;
+ }
+
+ private static class TestRevisionCreatedListener implements RevisionCreatedListener {
+ public RevisionInfo revisionInfo;
+
+ @Override
+ public void onRevisionCreated(Event event) {
+ revisionInfo = event.getRevision();
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index 56b58a4..03a1529 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -79,8 +79,6 @@
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
-import java.io.File;
-import java.io.IOException;
import org.apache.http.Header;
import org.apache.http.message.BasicHeader;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
@@ -946,12 +944,4 @@
private static Header runAsHeader(Object user) {
return new BasicHeader("X-Gerrit-RunAs", user.toString());
}
-
- private void createRefLogFileIfMissing(Repository repo, String ref) throws IOException {
- File log = new File(repo.getDirectory(), "logs/" + ref);
- if (!log.exists()) {
- log.getParentFile().mkdirs();
- assertThat(log.createNewFile()).isTrue();
- }
- }
}
diff --git a/javatests/com/google/gerrit/entities/converter/PatchSetProtoConverterTest.java b/javatests/com/google/gerrit/entities/converter/PatchSetProtoConverterTest.java
index 3a534e9..447b625 100644
--- a/javatests/com/google/gerrit/entities/converter/PatchSetProtoConverterTest.java
+++ b/javatests/com/google/gerrit/entities/converter/PatchSetProtoConverterTest.java
@@ -42,6 +42,7 @@
.id(PatchSet.id(Change.id(103), 73))
.commitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
.uploader(Account.id(452))
+ .realUploader(Account.id(687))
.createdOn(Instant.ofEpochMilli(930349320L))
.groups(ImmutableList.of("group1", " group2"))
.pushCertificate("my push certificate")
@@ -59,6 +60,7 @@
.setCommitId(
Entities.ObjectId.newBuilder().setName("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
.setUploaderAccountId(Entities.Account_Id.newBuilder().setId(452))
+ .setRealUploaderAccountId(Entities.Account_Id.newBuilder().setId(687))
.setCreatedOn(930349320L)
.setGroups("group1, group2")
.setPushCertificate("my push certificate")
@@ -74,6 +76,7 @@
.id(PatchSet.id(Change.id(103), 73))
.commitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
.uploader(Account.id(452))
+ .realUploader(Account.id(687))
.createdOn(Instant.ofEpochMilli(930349320L))
.build();
@@ -88,6 +91,7 @@
.setCommitId(
Entities.ObjectId.newBuilder().setName("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
.setUploaderAccountId(Entities.Account_Id.newBuilder().setId(452))
+ .setRealUploaderAccountId(Entities.Account_Id.newBuilder().setId(687))
.setCreatedOn(930349320L)
.build();
assertThat(proto).isEqualTo(expectedProto);
@@ -100,6 +104,7 @@
.id(PatchSet.id(Change.id(103), 73))
.commitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
.uploader(Account.id(452))
+ .realUploader(Account.id(687))
.createdOn(Instant.ofEpochMilli(930349320L))
.groups(ImmutableList.of("group1", " group2"))
.pushCertificate("my push certificate")
@@ -118,6 +123,7 @@
.id(PatchSet.id(Change.id(103), 73))
.commitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
.uploader(Account.id(452))
+ .realUploader(Account.id(687))
.createdOn(Instant.ofEpochMilli(930349320L))
.build();
@@ -143,6 +149,30 @@
.id(PatchSet.id(Change.id(103), 73))
.commitId(ObjectId.fromString("0000000000000000000000000000000000000000"))
.uploader(Account.id(0))
+ .realUploader(Account.id(0))
+ .createdOn(Instant.EPOCH)
+ .build());
+ }
+
+ @Test
+ public void realUploaderIsSetToUploaderIfMissingFromProto() {
+ Entities.PatchSet proto =
+ Entities.PatchSet.newBuilder()
+ .setId(
+ Entities.PatchSet_Id.newBuilder()
+ .setChangeId(Entities.Change_Id.newBuilder().setId(103))
+ .setId(73))
+ .setUploaderAccountId(Entities.Account_Id.newBuilder().setId(452))
+ .build();
+
+ PatchSet convertedPatchSet = patchSetProtoConverter.fromProto(proto);
+ Truth.assertThat(convertedPatchSet)
+ .isEqualTo(
+ PatchSet.builder()
+ .id(PatchSet.id(Change.id(103), 73))
+ .commitId(ObjectId.fromString("0000000000000000000000000000000000000000"))
+ .uploader(Account.id(452))
+ .realUploader(Account.id(452))
.createdOn(Instant.EPOCH)
.build());
}
@@ -156,6 +186,7 @@
.put("id", PatchSet.Id.class)
.put("commitId", ObjectId.class)
.put("uploader", Account.Id.class)
+ .put("realUploader", Account.Id.class)
.put("createdOn", Instant.class)
.put("groups", new TypeLiteral<ImmutableList<String>>() {}.getType())
.put("pushCertificate", new TypeLiteral<Optional<String>>() {}.getType())
diff --git a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
index f45d33b..8a7d25a 100644
--- a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
+++ b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
@@ -294,6 +294,7 @@
assertThat(diff.added().revisions.get(REVISION).uploader.name).isNull();
assertThat(diff.added().revisions.get(REVISION).uploader.email)
.isEqualTo(newRevision.uploader.email);
+ assertThat(diff.added().revisions.get(REVISION).realUploader).isNull();
assertThat(diff.removed().revisions).isNotNull();
assertThat(diff.removed().revisions).hasSize(1);
assertThat(diff.removed().revisions).containsKey(REVISION);
@@ -301,6 +302,37 @@
assertThat(diff.removed().revisions.get(REVISION).uploader.name).isNull();
assertThat(diff.removed().revisions.get(REVISION).uploader.email)
.isEqualTo(oldRevision.uploader.email);
+ assertThat(diff.removed().revisions.get(REVISION).realUploader).isNull();
+ }
+
+ @Test
+ public void getDiff_whenOneModifiedRevisionUploader_returnsModificationsToRevisionRealUploader() {
+ RevisionInfo oldRevision = new RevisionInfo(new AccountInfo("uploader", "uploader@mail.com"));
+ oldRevision.realUploader = new AccountInfo("real-uploader", "real-uploader@mail.com");
+ RevisionInfo newRevision = new RevisionInfo(oldRevision.uploader);
+ newRevision.realUploader =
+ new AccountInfo(oldRevision.realUploader.name, oldRevision.realUploader.email + "2");
+ ChangeInfo oldChangeInfo = new ChangeInfo(ImmutableMap.of(REVISION, oldRevision));
+ ChangeInfo newChangeInfo = new ChangeInfo(ImmutableMap.of(REVISION, newRevision));
+
+ ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
+
+ assertThat(diff.added().revisions).isNotNull();
+ assertThat(diff.added().revisions).hasSize(1);
+ assertThat(diff.added().revisions).containsKey(REVISION);
+ assertThat(diff.added().revisions.get(REVISION).uploader).isNull();
+ assertThat(diff.added().revisions.get(REVISION).realUploader).isNotNull();
+ assertThat(diff.added().revisions.get(REVISION).realUploader.name).isNull();
+ assertThat(diff.added().revisions.get(REVISION).realUploader.email)
+ .isEqualTo(newRevision.realUploader.email);
+ assertThat(diff.removed().revisions).isNotNull();
+ assertThat(diff.removed().revisions).hasSize(1);
+ assertThat(diff.removed().revisions).containsKey(REVISION);
+ assertThat(diff.removed().revisions.get(REVISION).uploader).isNull();
+ assertThat(diff.removed().revisions.get(REVISION).realUploader).isNotNull();
+ assertThat(diff.removed().revisions.get(REVISION).realUploader.name).isNull();
+ assertThat(diff.removed().revisions.get(REVISION).realUploader.email)
+ .isEqualTo(oldRevision.realUploader.email);
}
@Test
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
index 976ffc8..c654828 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
@@ -342,22 +342,24 @@
.id(PatchSet.id(ID, 1))
.commitId(ObjectId.fromString("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
.uploader(Account.id(2000))
+ .realUploader(Account.id(2001))
.createdOn(cols.createdOn())
.build();
Entities.PatchSet ps1Proto = PatchSetProtoConverter.INSTANCE.toProto(ps1);
ByteString ps1Bytes = Protos.toByteString(ps1Proto);
- assertThat(ps1Bytes.size()).isEqualTo(66);
+ assertThat(ps1Bytes.size()).isEqualTo(71);
PatchSet ps2 =
PatchSet.builder()
.id(PatchSet.id(ID, 2))
.commitId(ObjectId.fromString("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"))
.uploader(Account.id(3000))
+ .realUploader(Account.id(3001))
.createdOn(cols.lastUpdatedOn())
.build();
Entities.PatchSet ps2Proto = PatchSetProtoConverter.INSTANCE.toProto(ps2);
ByteString ps2Bytes = Protos.toByteString(ps2Proto);
- assertThat(ps2Bytes.size()).isEqualTo(66);
+ assertThat(ps2Bytes.size()).isEqualTo(71);
assertThat(ps2Bytes).isNotEqualTo(ps1Bytes);
assertRoundTrip(
@@ -1018,6 +1020,7 @@
.put("id", PatchSet.Id.class)
.put("commitId", ObjectId.class)
.put("uploader", Account.Id.class)
+ .put("realUploader", Account.Id.class)
.put("createdOn", Instant.class)
.put("groups", new TypeLiteral<ImmutableList<String>>() {}.getType())
.put("pushCertificate", new TypeLiteral<Optional<String>>() {}.getType())
diff --git a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
index e48d4af..f80a96d 100644
--- a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
+++ b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
@@ -46,6 +46,7 @@
.id(PatchSet.id(changeId, num))
.commitId(ObjectId.zeroId())
.uploader(Account.id(1234))
+ .realUploader(Account.id(5678))
.createdOn(TimeUtil.now())
.build();
}
diff --git a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
index 4c8750a..131bd05 100644
--- a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
+++ b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
@@ -239,6 +239,7 @@
.id(id)
.commitId(dummyObjectId)
.uploader(Account.id(123))
+ .realUploader(Account.id(456))
.createdOn(Instant.ofEpochMilli(12345))
.build();
}
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 264560b..3dbd1a0 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -1329,10 +1329,15 @@
const repo = ctx.params[0] as RepoName;
const commentId = ctx.params[2] as UrlEncodedCommentId;
- const comments = await this.restApiService.getDiffComments(changeNum);
- const change = await this.restApiService.getChangeDetail(changeNum);
+ const [comments, robotComments, change] = await Promise.all([
+ this.restApiService.getDiffComments(changeNum),
+ this.restApiService.getDiffRobotComments(changeNum),
+ this.restApiService.getChangeDetail(changeNum),
+ ]);
- const comment = findComment(addPath(comments), commentId);
+ const comment =
+ findComment(addPath(comments), commentId) ??
+ findComment(addPath(robotComments), commentId);
const path = comment?.path;
const patchsets = computeAllPatchSets(change);
const latestPatchNum = computeLatestPatchNum(patchsets);
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index c1cc863..1ac89d6 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -819,8 +819,7 @@
// fixed. Currently diff line doesn't match commit message line, because
// of metadata in diff, which aren't in content api request.
if (this.comment.path === SpecialFilePath.COMMIT_MESSAGE) return nothing;
- // TODO(milutin): disable user suggestions for owners, after user study.
- // if (this.isOwner) return nothing;
+ if (this.isOwner) return nothing;
return html`<gr-button
link
class="action suggestEdit"
diff --git a/proto/entities.proto b/proto/entities.proto
index 191cca7..0dc6441 100644
--- a/proto/entities.proto
+++ b/proto/entities.proto
@@ -98,6 +98,7 @@
optional string groups = 6;
optional string push_certificate = 8;
optional string description = 9;
+ optional Account_Id real_uploader_account_id = 10;
// Deleted fields, should not be reused:
reserved 5; // draft
