Rework checking preconditions for rebase
* Pull the permission check out of the RebaseUtil method that checks
preconditions:
* This allows us to do the permission check first in the rebase REST
endpoints. If the calling user has no permissions to rebase, we
should reject the request immediately and not open any resources
(e.g. we should not open the repository).
* This makes the permission checks more explicit which improves
readability (the permission check is easy to miss if it’s done in a
helper method).
* To be on the safe side let RebaseChain still check the permission
for every change in the chain (although this should not be
necessary, since the rebase permission is assigned on the target
branch the result of the permission evaluation should be the same
for all changes in a chain).
* Pull the checking whether the project is writable out of the
RebaseUtil method that checks preconditions:
This check needs to be done only once, but RebaseChain checked this
once per change in the chain.
* Make the RebaseUtil method that checks preconditions non-static:
This way it can make use of the psUtil member variable and callers do
not need to pass this in.
* Remove unnecessary else in RebaseUtil#verifyRebasePreconditions.
Release-Notes: skip
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: If8882684e88f1f5e44a7885d296b134911a06877
diff --git a/java/com/google/gerrit/server/change/RebaseUtil.java b/java/com/google/gerrit/server/change/RebaseUtil.java
index dcbd1ae..8acc925 100644
--- a/java/com/google/gerrit/server/change/RebaseUtil.java
+++ b/java/com/google/gerrit/server/change/RebaseUtil.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.change;
-import static com.google.gerrit.server.project.ProjectCache.illegalState;
-
import com.google.auto.value.AutoValue;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
@@ -37,7 +35,6 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.inject.Inject;
@@ -70,30 +67,28 @@
this.rebaseFactory = rebaseFactory;
}
- public static void verifyRebasePreconditions(
- ProjectCache projectCache, PatchSetUtil patchSetUtil, RevWalk rw, RevisionResource rsrc)
- throws ResourceConflictException, IOException, AuthException, PermissionBackendException {
+ /**
+ * Checks whether the given change fulfills all preconditions to be rebased.
+ *
+ * <p>This method does not check whether the calling user is allowed to rebase the change.
+ */
+ public void verifyRebasePreconditions(RevWalk rw, ChangeNotes changeNotes, PatchSet patchSet)
+ throws ResourceConflictException, IOException {
// Not allowed to rebase if the current patch set is locked.
- patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes());
+ psUtil.checkPatchSetNotLocked(changeNotes);
- rsrc.permissions().check(ChangePermission.REBASE);
- projectCache
- .get(rsrc.getProject())
- .orElseThrow(illegalState(rsrc.getProject()))
- .checkStatePermitsWrite();
-
- if (!rsrc.getChange().isNew()) {
+ Change change = changeNotes.getChange();
+ if (!change.isNew()) {
throw new ResourceConflictException(
- String.format(
- "Change %s is %s", rsrc.getChange().getId(), ChangeUtil.status(rsrc.getChange())));
- } else if (!hasOneParent(rw, rsrc.getPatchSet())) {
+ String.format("Change %s is %s", change.getId(), ChangeUtil.status(change)));
+ }
+
+ if (!hasOneParent(rw, patchSet)) {
throw new ResourceConflictException(
String.format(
"Error rebasing %s. Cannot rebase %s",
- rsrc.getChange().getId(),
- countParents(rw, rsrc.getPatchSet()) > 1
- ? "merge commits"
- : "commit with no ancestor"));
+ change.getId(),
+ countParents(rw, patchSet) > 1 ? "merge commits" : "commit with no ancestor"));
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 1a8f07a..8a8d2ca 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -87,6 +87,13 @@
@Override
public Response<ChangeInfo> apply(RevisionResource rsrc, RebaseInput input)
throws UpdateException, RestApiException, IOException, PermissionBackendException {
+ rsrc.permissions().check(ChangePermission.REBASE);
+
+ projectCache
+ .get(rsrc.getProject())
+ .orElseThrow(illegalState(rsrc.getProject()))
+ .checkStatePermitsWrite();
+
Change change = rsrc.getChange();
try (Repository repo = repoManager.openRepository(change.getProject());
ObjectInserter oi = repo.newObjectInserter();
@@ -94,7 +101,7 @@
RevWalk rw = CodeReviewCommit.newRevWalk(reader);
BatchUpdate bu =
updateFactory.create(change.getProject(), rsrc.getUser(), TimeUtil.now())) {
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, rsrc);
+ rebaseUtil.verifyRebasePreconditions(rw, rsrc.getNotes(), rsrc.getPatchSet());
RebaseChangeOp rebaseOp =
rebaseUtil.getRebaseOp(
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 4754c69..786bba7 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -113,7 +113,11 @@
@Override
public Response<RebaseChainInfo> apply(ChangeResource tipRsrc, RebaseInput input)
throws IOException, PermissionBackendException, RestApiException, UpdateException {
+ tipRsrc.permissions().check(ChangePermission.REBASE);
+
Project.NameKey project = tipRsrc.getProject();
+ projectCache.get(project).orElseThrow(illegalState(project)).checkStatePermitsWrite();
+
CurrentUser user = tipRsrc.getUser();
List<Change.Id> upToDateAncestors = new ArrayList<>();
@@ -136,7 +140,8 @@
RevisionResource revRsrc =
new RevisionResource(changeResourceFactory.create(changeData, user), ps);
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, revRsrc);
+ revRsrc.permissions().check(ChangePermission.REBASE);
+ rebaseUtil.verifyRebasePreconditions(rw, changeData.notes(), ps);
boolean isUpToDate = false;
RebaseChangeOp rebaseOp = null;
