Merge changes I9b9967ae,I90c2cd62
* changes:
AccountResolver: Remove callerMayAssumeCandidatesAreVisible() method
AccountResolver: Fix account visibility checking
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/config-submit-requirements.txt b/Documentation/config-submit-requirements.txt
index fb37287..d61dbc43 100644
--- a/Documentation/config-submit-requirements.txt
+++ b/Documentation/config-submit-requirements.txt
@@ -206,6 +206,29 @@
+
An operator that returns true if the latest patchset contained a modified file
matching `<filePattern>` with a modified region matching `<contentPattern>`.
++
+Both `<filePattern>` and `<contentPattern>` support regular expressions if they
+start with the '^' character. Regular expressions are matched with the
+`java.util.regex` engine. When using regular expressions, special characters
+should be double escaped because the config is parsed twice when the server
+reads the `project.config` file and when the submit-requirement expressions
+are parsed as a predicate tree. For example, to match against modified files
+that end with ".cc" or ".cpp" the following `applicableIf` expression can be
+used:
++
+----
+ applicableIf = file:\"^.*\\\\.(cc|cpp)$\"
+----
++
+Below is another example that uses both `<filePattern>` and `<contentPattern>`:
++
+----
+ applicableIf = file:\"'^.*\\\\.(cc|cpp)$',withDiffContaining='^.*th[rR]ee$'\"
+----
++
+If no regular expression is used, the text is matched by checking that the file
+name contains the file pattern, or the edits of the file diff contain the edit
+pattern.
[[unsupported_operators]]
=== Unsupported Operators
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/StarredChangesUtil.java b/java/com/google/gerrit/server/StarredChangesUtil.java
index 8f413f9..9403105 100644
--- a/java/com/google/gerrit/server/StarredChangesUtil.java
+++ b/java/com/google/gerrit/server/StarredChangesUtil.java
@@ -266,7 +266,7 @@
}
}
- public ImmutableSet<Change.Id> byAccountId(Account.Id accountId, String label) {
+ public ImmutableSet<Change.Id> byAccountId(Account.Id accountId) {
try (Repository repo = repoManager.openRepository(allUsers)) {
ImmutableSet.Builder<Change.Id> builder = ImmutableSet.builder();
for (Ref ref : repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_STARRED_CHANGES)) {
@@ -277,7 +277,7 @@
}
// Skip all refs that don't contain the required label.
StarRef starRef = readLabels(repo, ref.getName());
- if (!starRef.labels().contains(label)) {
+ if (!starRef.labels().contains(DEFAULT_LABEL)) {
continue;
}
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/mail/send/MailSoySauceLoader.java b/java/com/google/gerrit/server/mail/send/MailSoySauceLoader.java
index 8ee8fc2..9d75abd 100644
--- a/java/com/google/gerrit/server/mail/send/MailSoySauceLoader.java
+++ b/java/com/google/gerrit/server/mail/send/MailSoySauceLoader.java
@@ -17,6 +17,8 @@
import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.io.CharStreams;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.inject.Inject;
@@ -24,13 +26,13 @@
import com.google.inject.Singleton;
import com.google.template.soy.SoyFileSet;
import com.google.template.soy.jbcsrc.api.SoySauce;
-import com.google.template.soy.shared.SoyAstCache;
import java.io.IOException;
import java.io.Reader;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
+import org.eclipse.jgit.util.FileUtils;
/**
* Configures and loads Soy Sauce object for rendering email templates.
@@ -90,56 +92,76 @@
"SetAssigneeHtml.soy",
};
+ private static final SoySauce DEFAULT = getDefault().build().compileTemplates();
+
private final SitePaths site;
- private final SoyAstCache cache;
private final PluginSetContext<MailSoyTemplateProvider> templateProviders;
@Inject
- MailSoySauceLoader(
- SitePaths site,
- SoyAstCache cache,
- PluginSetContext<MailSoyTemplateProvider> templateProviders) {
+ MailSoySauceLoader(SitePaths site, PluginSetContext<MailSoyTemplateProvider> templateProviders) {
this.site = site;
- this.cache = cache;
this.templateProviders = templateProviders;
}
public SoySauce load() {
- SoyFileSet.Builder builder = SoyFileSet.builder();
- builder.setSoyAstCache(cache);
- for (String name : TEMPLATES) {
- addTemplate(builder, "com/google/gerrit/server/mail/", name);
+ if (!hasCustomTemplates(site, templateProviders)) {
+ return DEFAULT;
}
+
+ SoyFileSet.Builder builder = getDefault();
templateProviders.runEach(
- e -> e.getFileNames().forEach(p -> addTemplate(builder, e.getPath(), p)));
+ e -> e.getFileNames().forEach(p -> addTemplate(builder, site, e.getPath(), p)));
return builder.build().compileTemplates();
}
- private void addTemplate(SoyFileSet.Builder builder, String resourcePath, String name)
+ private static boolean hasCustomTemplates(
+ SitePaths site, PluginSetContext<MailSoyTemplateProvider> templateProviders) {
+ try {
+ if (!templateProviders.isEmpty()) {
+ return true;
+ }
+ return Files.exists(site.mail_dir) && FileUtils.hasFiles(site.mail_dir);
+ } catch (IOException e) {
+ throw new StorageException(e);
+ }
+ }
+
+ private static SoyFileSet.Builder getDefault() {
+ SoyFileSet.Builder builder = SoyFileSet.builder();
+ for (String name : TEMPLATES) {
+ addTemplate(builder, null, "com/google/gerrit/server/mail/", name);
+ }
+ return builder;
+ }
+
+ private static void addTemplate(
+ SoyFileSet.Builder builder, @Nullable SitePaths site, String resourcePath, String name)
throws ProvisionException {
if (!resourcePath.endsWith("/")) {
resourcePath += "/";
}
String logicalPath = resourcePath + name;
- // Load as a file in the mail templates directory if present.
- Path tmpl = site.mail_dir.resolve(name);
- if (Files.isRegularFile(tmpl)) {
- String content;
- // TODO(davido): Consider using JGit's FileSnapshot to cache based on
- // mtime.
- try (Reader r = Files.newBufferedReader(tmpl, StandardCharsets.UTF_8)) {
- content = CharStreams.toString(r);
- } catch (IOException err) {
- throw new ProvisionException(
- "Failed to read template file " + tmpl.toAbsolutePath().toString(), err);
+ if (site != null) {
+ // Load as a file in the mail templates directory if present.
+ Path tmpl = site.mail_dir.resolve(name);
+ if (Files.isRegularFile(tmpl)) {
+ String content;
+ // TODO(davido): Consider using JGit's FileSnapshot to cache based on
+ // mtime.
+ try (Reader r = Files.newBufferedReader(tmpl, StandardCharsets.UTF_8)) {
+ content = CharStreams.toString(r);
+ } catch (IOException err) {
+ throw new ProvisionException(
+ "Failed to read template file " + tmpl.toAbsolutePath(), err);
+ }
+ builder.add(content, logicalPath);
+ return;
}
- builder.add(content, logicalPath);
- return;
}
// Otherwise load the template as a resource.
- URL resource = this.getClass().getClassLoader().getResource(logicalPath);
+ URL resource = MailSoySauceLoader.class.getClassLoader().getResource(logicalPath);
checkArgument(resource != null, "resource %s not found.", logicalPath);
builder.add(resource, logicalPath);
}
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/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index f40e530..5f909a1 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -23,12 +23,10 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.index.query.Predicate;
-import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.change.HashtagsUtil;
import com.google.gerrit.server.index.change.ChangeField;
-import com.google.inject.ImplementedBy;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
@@ -71,25 +69,12 @@
return new ChangeIndexPredicate(ChangeField.COMMENTBY_SPEC, id.toString());
}
- @ImplementedBy(IndexEditByPredicateProvider.class)
- public interface EditByPredicateProvider {
-
- /**
- * Returns a predicate that matches changes where the provided {@link
- * com.google.gerrit.entities.Account.Id} has a pending change edit.
- */
- Predicate<ChangeData> editBy(Account.Id id) throws QueryParseException;
- }
-
/**
- * A default implementation of {@link EditByPredicateProvider}, based on th {@link
- * ChangeField#EDITBY_SPEC} index field.
+ * Returns a predicate that matches changes where the provided {@link
+ * com.google.gerrit.entities.Account.Id} has a pending change edit.
*/
- public static class IndexEditByPredicateProvider implements EditByPredicateProvider {
- @Override
- public Predicate<ChangeData> editBy(Account.Id id) {
- return new ChangeIndexPredicate(ChangeField.EDITBY_SPEC, id.toString());
- }
+ public static Predicate<ChangeData> editBy(Account.Id id) {
+ return new ChangeIndexPredicate(ChangeField.EDITBY_SPEC, id.toString());
}
/**
@@ -110,10 +95,9 @@
* Returns a predicate that matches changes where the provided {@link
* com.google.gerrit.entities.Account.Id} has starred changes with {@code label}.
*/
- public static Predicate<ChangeData> starBy(
- StarredChangesUtil starredChangesUtil, Account.Id id, String label) {
+ public static Predicate<ChangeData> starBy(StarredChangesUtil starredChangesUtil, Account.Id id) {
Set<Predicate<ChangeData>> starredChanges =
- starredChangesUtil.byAccountId(id, label).stream()
+ starredChangesUtil.byAccountId(id).stream()
.map(ChangePredicates::idStr)
.collect(toImmutableSet());
return starredChanges.isEmpty() ? ChangeIndexPredicate.none() : Predicate.or(starredChanges);
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index e4ef6dd..622c4bf 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -86,7 +86,6 @@
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.project.ChildProjects;
import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.query.change.ChangePredicates.EditByPredicateProvider;
import com.google.gerrit.server.query.change.PredicateArgs.ValOp;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.gerrit.server.submit.SubmitDryRun;
@@ -276,8 +275,6 @@
private final Provider<CurrentUser> self;
- private final EditByPredicateProvider editByPredicateProvider;
-
@Inject
@VisibleForTesting
public Arguments(
@@ -311,8 +308,7 @@
ExperimentFeatures experimentFeatures,
HasOperandAliasConfig hasOperandAliasConfig,
ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory,
- PluginSetContext<SubmitRule> submitRules,
- EditByPredicateProvider editByPredicateProvider) {
+ PluginSetContext<SubmitRule> submitRules) {
this(
queryProvider,
rewriter,
@@ -345,8 +341,7 @@
experimentFeatures,
hasOperandAliasConfig,
changeIsVisbleToPredicateFactory,
- submitRules,
- editByPredicateProvider);
+ submitRules);
}
private Arguments(
@@ -381,8 +376,7 @@
ExperimentFeatures experimentFeatures,
HasOperandAliasConfig hasOperandAliasConfig,
ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory,
- PluginSetContext<SubmitRule> submitRules,
- EditByPredicateProvider editByPredicateProvider) {
+ PluginSetContext<SubmitRule> submitRules) {
this.queryProvider = queryProvider;
this.rewriter = rewriter;
this.opFactories = opFactories;
@@ -415,7 +409,6 @@
this.experimentFeatures = experimentFeatures;
this.hasOperandAliasConfig = hasOperandAliasConfig;
this.submitRules = submitRules;
- this.editByPredicateProvider = editByPredicateProvider;
}
public Arguments asUser(CurrentUser otherUser) {
@@ -451,8 +444,7 @@
experimentFeatures,
hasOperandAliasConfig,
changeIsVisbleToPredicateFactory,
- submitRules,
- editByPredicateProvider);
+ submitRules);
}
Arguments asUser(Account.Id otherId) {
@@ -639,7 +631,7 @@
}
if ("edit".equalsIgnoreCase(value)) {
- return this.args.editByPredicateProvider.editBy(self());
+ return ChangePredicates.editBy(self());
}
if ("attention".equalsIgnoreCase(value)) {
@@ -1171,8 +1163,7 @@
}
private Predicate<ChangeData> starredBySelf() throws QueryParseException {
- return ChangePredicates.starBy(
- args.starredChangesUtil, self(), StarredChangesUtil.DEFAULT_LABEL);
+ return ChangePredicates.starBy(args.starredChangesUtil, self());
}
private Predicate<ChangeData> draftBySelf() throws QueryParseException {
@@ -1774,7 +1765,8 @@
return value;
}
- private Account.Id self() throws QueryParseException {
+ /** Returns {@link Account.Id} of the identified calling user. */
+ public Account.Id self() throws QueryParseException {
return args.getIdentifiedUser().getAccountId();
}
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/index/change/FakeQueryBuilder.java b/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
index bfeb947..e879170 100644
--- a/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
+++ b/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
@@ -58,7 +58,6 @@
null,
null,
null,
- null,
null));
}
diff --git a/javatests/com/google/gerrit/server/mail/send/MailSoySauceLoaderTest.java b/javatests/com/google/gerrit/server/mail/send/MailSoySauceLoaderTest.java
index fbeabe1..7f893f1 100644
--- a/javatests/com/google/gerrit/server/mail/send/MailSoySauceLoaderTest.java
+++ b/javatests/com/google/gerrit/server/mail/send/MailSoySauceLoaderTest.java
@@ -20,7 +20,6 @@
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
import com.google.gerrit.server.plugincontext.PluginSetContext;
-import com.google.template.soy.shared.SoyAstCache;
import java.nio.file.Paths;
import org.junit.Before;
import org.junit.Test;
@@ -40,9 +39,7 @@
public void soyCompilation() {
MailSoySauceLoader loader =
new MailSoySauceLoader(
- sitePaths,
- new SoyAstCache(),
- new PluginSetContext<>(set, PluginMetrics.DISABLED_INSTANCE));
+ sitePaths, new PluginSetContext<>(set, PluginMetrics.DISABLED_INSTANCE));
assertThat(loader.load()).isNotNull(); // should not throw
}
}
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/modules/jgit b/modules/jgit
index a190130..66b871b 160000
--- a/modules/jgit
+++ b/modules/jgit
@@ -1 +1 @@
-Subproject commit a1901305b26ed5e0116f138bc02837713d2cf5c3
+Subproject commit 66b871b777c1a58337e80dd03db68bb76b145a93
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 3dbd1a0..c0433dd 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -189,7 +189,7 @@
// Matches /admin/repos/$REPO,tags with optional filter and offset.
TAG_LIST: /^\/admin\/repos\/(.+),tags\/?(?:\/q\/filter:(.*?))?(?:,(\d+))?$/,
- QUERY: /^\/q\/([^,]+)(,(\d+))?$/,
+ QUERY: /^\/q\/(.+?)(,(\d+))?$/,
/**
* Support vestigial params from GWT UI.
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
index 3ca7bdc..87d100c 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -416,7 +416,7 @@
});
test('QUERY', async () => {
- // QUERY: /^\/q\/([^,]+)(,(\d+))?$/,
+ // QUERY: /^\/q\/(.+?)(,(\d+))?$/,
await checkUrlToState('/q/asdf', {
...createSearchViewState(),
query: 'asdf',
@@ -430,6 +430,15 @@
query: 'asdf',
offset: '123',
});
+ await checkUrlToState('/q/asdf,qwer', {
+ ...createSearchViewState(),
+ query: 'asdf,qwer',
+ });
+ await checkUrlToState('/q/asdf,qwer,123', {
+ ...createSearchViewState(),
+ query: 'asdf,qwer',
+ offset: '123',
+ });
});
test('QUERY_LEGACY_SUFFIX', async () => {
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
