Check submit with PermissionBackend
Change-Id: I932103c669821b7cfe4f5d762052c35f3c9731dd
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index 7d9663a..82eae1b 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -378,7 +378,7 @@
SubmitInput in = new SubmitInput();
in.onBehalfOf = admin2.email;
exception.expect(AuthException.class);
- exception.expectMessage("submit on behalf of not permitted");
+ exception.expectMessage("submit as not permitted");
gApi.changes().id(project.get() + "~master~" + r.getChangeId()).current().submit(in);
}
@@ -393,7 +393,7 @@
SubmitInput in = new SubmitInput();
in.onBehalfOf = user.email;
exception.expect(UnprocessableEntityException.class);
- exception.expectMessage("on_behalf_of account " + user.id + " cannot see destination ref");
+ exception.expectMessage("on_behalf_of account " + user.id + " cannot see change");
gApi.changes().id(changeId).current().submit(in);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
index 43be8df..dac9deb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
@@ -70,6 +70,7 @@
import com.google.gerrit.server.change.TestSubmitType;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.update.UpdateException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -219,7 +220,7 @@
public void submit(SubmitInput in) throws RestApiException {
try {
submit.apply(revision, in);
- } catch (OrmException | IOException e) {
+ } catch (OrmException | IOException | PermissionBackendException e) {
throw new RestApiException("Cannot submit change", e);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java
index 4d35f9e..32132bc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java
@@ -24,6 +24,7 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.edit.ChangeEdit;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.TypeLiteral;
import java.util.Optional;
@@ -51,6 +52,10 @@
return cacheable;
}
+ public PermissionBackend.ForChange permissions() {
+ return change.permissions();
+ }
+
public ChangeResource getChangeResource() {
return change;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
index 01020fa..b996133 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
@@ -51,7 +51,9 @@
import com.google.gerrit.server.git.MergeOp;
import com.google.gerrit.server.git.MergeSuperSet;
import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.project.ChangeControl;
+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.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
@@ -62,6 +64,7 @@
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Collection;
+import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
@@ -122,13 +125,13 @@
private final Provider<ReviewDb> dbProvider;
private final GitRepositoryManager repoManager;
+ private final PermissionBackend permissionBackend;
private final ChangeData.Factory changeDataFactory;
private final ChangeMessagesUtil cmUtil;
private final ChangeNotes.Factory changeNotesFactory;
private final Provider<MergeOp> mergeOpProvider;
private final Provider<MergeSuperSet> mergeSuperSet;
private final AccountsCollection accounts;
- private final ChangesCollection changes;
private final String label;
private final String labelWithParents;
private final ParameterizedString titlePattern;
@@ -143,25 +146,25 @@
Submit(
Provider<ReviewDb> dbProvider,
GitRepositoryManager repoManager,
+ PermissionBackend permissionBackend,
ChangeData.Factory changeDataFactory,
ChangeMessagesUtil cmUtil,
ChangeNotes.Factory changeNotesFactory,
Provider<MergeOp> mergeOpProvider,
Provider<MergeSuperSet> mergeSuperSet,
AccountsCollection accounts,
- ChangesCollection changes,
@GerritServerConfig Config cfg,
Provider<InternalChangeQuery> queryProvider,
PatchSetUtil psUtil) {
this.dbProvider = dbProvider;
this.repoManager = repoManager;
+ this.permissionBackend = permissionBackend;
this.changeDataFactory = changeDataFactory;
this.cmUtil = cmUtil;
this.changeNotesFactory = changeNotesFactory;
this.mergeOpProvider = mergeOpProvider;
this.mergeSuperSet = mergeSuperSet;
this.accounts = accounts;
- this.changes = changes;
this.label =
MoreObjects.firstNonNull(
Strings.emptyToNull(cfg.getString("change", null, "submitLabel")), "Submit");
@@ -193,17 +196,19 @@
@Override
public Output apply(RevisionResource rsrc, SubmitInput input)
- throws RestApiException, RepositoryNotFoundException, IOException, OrmException {
+ throws RestApiException, RepositoryNotFoundException, IOException, OrmException,
+ PermissionBackendException {
input.onBehalfOf = Strings.emptyToNull(input.onBehalfOf);
+ IdentifiedUser submitter;
if (input.onBehalfOf != null) {
- rsrc = onBehalfOf(rsrc, input);
+ submitter = onBehalfOf(rsrc, input);
+ } else {
+ rsrc.permissions().check(ChangePermission.SUBMIT);
+ submitter = rsrc.getUser().asIdentifiedUser();
}
- ChangeControl control = rsrc.getControl();
- IdentifiedUser caller = control.getUser().asIdentifiedUser();
+
Change change = rsrc.getChange();
- if (input.onBehalfOf == null && !control.canSubmit()) {
- throw new AuthException("submit not permitted");
- } else if (!change.getStatus().isOpen()) {
+ if (!change.getStatus().isOpen()) {
throw new ResourceConflictException("change is " + status(change));
} else if (!ProjectUtil.branchExists(repoManager, change.getDest())) {
throw new ResourceConflictException(
@@ -217,7 +222,7 @@
try (MergeOp op = mergeOpProvider.get()) {
ReviewDb db = dbProvider.get();
- op.merge(db, change, caller, true, input, false);
+ op.merge(db, change, submitter, true, input, false);
try {
change =
changeNotesFactory.createChecked(db, change.getProject(), change.getId()).getChange();
@@ -250,18 +255,20 @@
*/
private String problemsForSubmittingChangeset(ChangeData cd, ChangeSet cs, CurrentUser user) {
try {
- @SuppressWarnings("resource")
- ReviewDb db = dbProvider.get();
if (cs.furtherHiddenChanges()) {
return BLOCKED_HIDDEN_SUBMIT_TOOLTIP;
}
for (ChangeData c : cs.changes()) {
- ChangeControl changeControl = c.changeControl(user);
-
- if (!changeControl.isVisible(db)) {
+ Set<ChangePermission> can =
+ permissionBackend
+ .user(user)
+ .database(dbProvider)
+ .change(c)
+ .test(EnumSet.of(ChangePermission.READ, ChangePermission.SUBMIT));
+ if (!can.contains(ChangePermission.READ)) {
return BLOCKED_HIDDEN_SUBMIT_TOOLTIP;
}
- if (!changeControl.canSubmit()) {
+ if (!can.contains(ChangePermission.SUBMIT)) {
return BLOCKED_SUBMIT_TOOLTIP;
}
MergeOp.checkSubmitRule(c);
@@ -281,7 +288,7 @@
}
} catch (ResourceConflictException e) {
return BLOCKED_SUBMIT_TOOLTIP;
- } catch (OrmException | IOException e) {
+ } catch (PermissionBackendException | OrmException | IOException e) {
log.error("Error checking if change is submittable", e);
throw new OrmRuntimeException("Could not determine problems for the change", e);
}
@@ -290,20 +297,23 @@
@Override
public UiAction.Description getDescription(RevisionResource resource) {
- PatchSet.Id current = resource.getChange().currentPatchSetId();
- String topic = resource.getChange().getTopic();
- boolean visible =
- !resource.getPatchSet().isDraft()
- && resource.getChange().getStatus().isOpen()
- && resource.getPatchSet().getId().equals(current)
- && resource.getControl().canSubmit();
+ Change change = resource.getChange();
+ String topic = change.getTopic();
ReviewDb db = dbProvider.get();
ChangeData cd = changeDataFactory.create(db, resource.getControl());
-
+ boolean visible;
try {
+ visible =
+ change.getStatus().isOpen()
+ && resource.isCurrent()
+ && !resource.getPatchSet().isDraft()
+ && resource.permissions().test(ChangePermission.SUBMIT);
MergeOp.checkSubmitRule(cd);
} catch (ResourceConflictException e) {
visible = false;
+ } catch (PermissionBackendException e) {
+ log.error("Error checking if change is submittable", e);
+ throw new OrmRuntimeException("Could not check submit permission", e);
} catch (OrmException e) {
log.error("Error checking if change is submittable", e);
throw new OrmRuntimeException("Could not determine problems for the change", e);
@@ -367,7 +377,7 @@
Map<String, String> params =
ImmutableMap.of(
"patchSet", String.valueOf(resource.getPatchSet().getPatchSetId()),
- "branch", resource.getChange().getDest().getShortName(),
+ "branch", change.getDest().getShortName(),
"commit", ObjectId.fromString(revId.get()).abbreviate(7).name(),
"submitSize", String.valueOf(cs.size()));
ParameterizedString tp = cs.size() > 1 ? titlePatternWithAncestors : titlePattern;
@@ -458,24 +468,21 @@
return commits;
}
- private RevisionResource onBehalfOf(RevisionResource rsrc, SubmitInput in)
- throws AuthException, UnprocessableEntityException, OrmException {
- ChangeControl caller = rsrc.getControl();
- if (!caller.canSubmit()) {
- throw new AuthException("submit not permitted");
- }
- if (!caller.canSubmitAs()) {
- throw new AuthException("submit on behalf of not permitted");
- }
- ChangeControl target =
- caller.forUser(accounts.parseOnBehalfOf(caller.getUser(), in.onBehalfOf));
- if (!target.getRefControl().isVisible()) {
+ private IdentifiedUser onBehalfOf(RevisionResource rsrc, SubmitInput in)
+ throws AuthException, UnprocessableEntityException, OrmException, PermissionBackendException {
+ PermissionBackend.ForChange perm = rsrc.permissions().database(dbProvider);
+ perm.check(ChangePermission.SUBMIT);
+ perm.check(ChangePermission.SUBMIT_AS);
+
+ CurrentUser caller = rsrc.getUser();
+ IdentifiedUser submitter = accounts.parseOnBehalfOf(caller, in.onBehalfOf);
+ try {
+ perm.user(submitter).check(ChangePermission.READ);
+ } catch (AuthException e) {
throw new UnprocessableEntityException(
- String.format(
- "on_behalf_of account %s cannot see destination ref",
- target.getUser().getAccountId()));
+ String.format("on_behalf_of account %s cannot see change", submitter.getAccountId()));
}
- return new RevisionResource(changes.parse(target), rsrc.getPatchSet());
+ return submitter;
}
public static boolean wholeTopicEnabled(Config config) {
@@ -510,7 +517,8 @@
@Override
public ChangeInfo apply(ChangeResource rsrc, SubmitInput input)
- throws RestApiException, RepositoryNotFoundException, IOException, OrmException {
+ throws RestApiException, RepositoryNotFoundException, IOException, OrmException,
+ PermissionBackendException {
PatchSet ps = psUtil.current(dbProvider.get(), rsrc.getNotes());
if (ps == null) {
throw new ResourceConflictException("current revision is missing");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ChangePermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ChangePermission.java
index 90d2754..4b06861 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ChangePermission.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ChangePermission.java
@@ -30,7 +30,8 @@
REMOVE_REVIEWER(Permission.REMOVE_REVIEWER),
ADD_PATCH_SET(Permission.ADD_PATCH_SET),
REBASE(Permission.REBASE),
- SUBMIT(Permission.SUBMIT);
+ SUBMIT(Permission.SUBMIT),
+ SUBMIT_AS(Permission.SUBMIT_AS);
private final String name;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index 4d2120f..11b6980 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -472,14 +472,6 @@
|| getRefControl().canEditHashtags(); // user can edit hashtag on a specific ref
}
- public boolean canSubmit() {
- return getRefControl().canSubmit(isOwner());
- }
-
- public boolean canSubmitAs() {
- return getRefControl().canSubmitAs();
- }
-
private boolean match(String destBranch, String refPattern) {
return RefPatternMatcher.getMatcher(refPattern).match(destBranch, getUser());
}
@@ -594,8 +586,10 @@
case RESTORE:
return canRestore(db());
case SUBMIT:
- return canSubmit();
+ return getRefControl().canSubmit(isOwner());
+
case REMOVE_REVIEWER: // TODO Honor specific removal filters?
+ case SUBMIT_AS:
return getRefControl().canPerform(perm.permissionName().get());
}
} catch (OrmException e) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
index ca567e2..9f6f8de 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
@@ -187,11 +187,6 @@
return canPerform(Permission.SUBMIT, isChangeOwner) && canWrite();
}
- /** @return true if this user was granted submitAs to this ref */
- public boolean canSubmitAs() {
- return canPerform(Permission.SUBMIT_AS);
- }
-
/** @return true if the user can update the reference as a fast-forward. */
public boolean canUpdate() {
if (RefNames.REFS_CONFIG.equals(refName) && !projectControl.isOwner()) {
