Check labels during PostReview with PermissionBackend
This isn't sufficient to completely remove the label range code from
ChangeControl and RefControl as there are many callers. This commit
covers users setting labels through the REST API and web UI.
Change-Id: I25f17eef610ddcd30918bc4532d24876779e6a3e
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 82eae1b..b221ec5 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
@@ -314,7 +314,7 @@
in.label("Code-Review", 1);
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");
revision.review(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 2af7b90..d934f6c 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
@@ -214,7 +214,7 @@
public void review(ReviewInput in) throws RestApiException {
try {
review.apply(revision, in);
- } catch (OrmException | UpdateException | IOException e) {
+ } catch (OrmException | UpdateException | IOException | PermissionBackendException e) {
throw new RestApiException("Cannot post review", e);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 81ab39e..6365fd9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -18,6 +18,7 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.CommentsUtil.setCommentRevId;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.joining;
@@ -39,8 +40,6 @@
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
-import com.google.gerrit.common.data.Permission;
-import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.AddReviewerResult;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -83,6 +82,7 @@
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CommentsUtil;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.OutputFormat;
import com.google.gerrit.server.PatchSetUtil;
@@ -95,6 +95,10 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.LabelPermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
@@ -188,12 +192,14 @@
@Override
public Response<ReviewResult> apply(RevisionResource revision, ReviewInput input)
- throws RestApiException, UpdateException, OrmException, IOException {
+ throws RestApiException, UpdateException, OrmException, IOException,
+ PermissionBackendException {
return apply(revision, input, TimeUtil.nowTs());
}
public Response<ReviewResult> apply(RevisionResource revision, ReviewInput input, Timestamp ts)
- throws RestApiException, UpdateException, OrmException, IOException {
+ throws RestApiException, UpdateException, OrmException, IOException,
+ PermissionBackendException {
// Respect timestamp, but truncate at change created-on time.
ts = Ordering.natural().max(ts, revision.getChange().getCreatedOn());
if (revision.getEdit().isPresent()) {
@@ -348,7 +354,8 @@
}
private RevisionResource onBehalfOf(RevisionResource rev, ReviewInput in)
- throws BadRequestException, AuthException, UnprocessableEntityException, OrmException {
+ throws BadRequestException, AuthException, UnprocessableEntityException, OrmException,
+ PermissionBackendException {
if (in.labels == null || in.labels.isEmpty()) {
throw new AuthException(
String.format("label required to post review on behalf of \"%s\"", in.onBehalfOf));
@@ -360,11 +367,13 @@
throw new AuthException("not allowed to modify other user's drafts");
}
- ChangeControl caller = rev.getControl();
+ CurrentUser caller = rev.getUser();
+ PermissionBackend.ForChange perm = rev.permissions().database(db);
+ LabelTypes labelTypes = rev.getControl().getLabelTypes();
Iterator<Map.Entry<String, Short>> itr = in.labels.entrySet().iterator();
while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next();
- LabelType type = caller.getLabelTypes().byLabel(ent.getKey());
+ LabelType type = labelTypes.byLabel(ent.getKey());
if (type == null && in.strictLabels) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
@@ -373,16 +382,15 @@
continue;
}
- if (caller.getUser().isInternalUser()) {
- continue;
- }
-
- PermissionRange r = caller.getRange(Permission.forLabelAs(type.getName()));
- if (r == null || r.isEmpty() || !r.contains(ent.getValue())) {
- throw new AuthException(
- String.format(
- "not permitted to modify label \"%s\" on behalf of \"%s\"",
- ent.getKey(), in.onBehalfOf));
+ if (!caller.isInternalUser()) {
+ try {
+ perm.check(new LabelPermission.WithValue(ON_BEHALF_OF, type, ent.getValue()));
+ } catch (AuthException e) {
+ throw new AuthException(
+ String.format(
+ "not permitted to modify label \"%s\" on behalf of \"%s\"",
+ type.getName(), in.onBehalfOf));
+ }
}
}
if (in.labels.isEmpty()) {
@@ -390,25 +398,26 @@
String.format("label required to post review on behalf of \"%s\"", in.onBehalfOf));
}
- ChangeControl target =
- caller.forUser(accounts.parseOnBehalfOf(caller.getUser(), in.onBehalfOf));
- if (!target.getRefControl().isVisible()) {
+ IdentifiedUser reviewer = accounts.parseOnBehalfOf(caller, in.onBehalfOf);
+ try {
+ perm.user(reviewer).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", reviewer.getAccountId()));
}
- return new RevisionResource(changes.parse(target), rev.getPatchSet());
+
+ ChangeControl ctl = rev.getControl().forUser(reviewer);
+ return new RevisionResource(changes.parse(ctl), rev.getPatchSet());
}
- private void checkLabels(RevisionResource revision, boolean strict, Map<String, Short> labels)
- throws BadRequestException, AuthException {
- ChangeControl ctl = revision.getControl();
+ private void checkLabels(RevisionResource rsrc, boolean strict, Map<String, Short> labels)
+ throws BadRequestException, AuthException, PermissionBackendException {
+ LabelTypes types = rsrc.getControl().getLabelTypes();
+ PermissionBackend.ForChange perm = rsrc.permissions();
Iterator<Map.Entry<String, Short>> itr = labels.entrySet().iterator();
while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next();
-
- LabelType lt = revision.getControl().getLabelTypes().byLabel(ent.getKey());
+ LabelType lt = types.byLabel(ent.getKey());
if (lt == null) {
if (strict) {
throw new BadRequestException(
@@ -433,18 +442,15 @@
continue;
}
- String name = lt.getName();
- PermissionRange range = ctl.getRange(Permission.forLabel(name));
- if (range == null || !range.contains(ent.getValue())) {
+ short val = ent.getValue();
+ try {
+ perm.check(new LabelPermission.WithValue(lt, val));
+ } catch (AuthException e) {
if (strict) {
throw new AuthException(
- String.format(
- "Applying label \"%s\": %d is restricted", ent.getKey(), ent.getValue()));
- } else if (range == null || range.isEmpty()) {
- ent.setValue((short) 0);
- } else {
- ent.setValue((short) range.squash(ent.getValue()));
+ String.format("Applying label \"%s\": %d is restricted", lt.getName(), val));
}
+ ent.setValue(perm.squashThenCheck(lt, val));
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/LabelPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/LabelPermission.java
index 61f7330..747c997 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/LabelPermission.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/LabelPermission.java
@@ -15,25 +15,69 @@
package com.google.gerrit.server.permissions;
import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.SELF;
import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.server.util.LabelVote;
import java.util.Optional;
/** Permission representing a label. */
public class LabelPermission implements ChangePermissionOrLabel {
+ public enum ForUser {
+ SELF,
+ ON_BEHALF_OF;
+ }
+
+ private final ForUser forUser;
private final String name;
/**
* Construct a reference to a label permission.
*
+ * @param type type description of the label.
+ */
+ public LabelPermission(LabelType type) {
+ this(SELF, type);
+ }
+
+ /**
+ * Construct a reference to a label permission.
+ *
+ * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
+ * @param type type description of the label.
+ */
+ public LabelPermission(ForUser forUser, LabelType type) {
+ this(forUser, type.getName());
+ }
+
+ /**
+ * Construct a reference to a label permission.
+ *
* @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
*/
public LabelPermission(String name) {
+ this(SELF, name);
+ }
+
+ /**
+ * Construct a reference to a label permission.
+ *
+ * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
+ * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
+ */
+ public LabelPermission(ForUser forUser, String name) {
+ this.forUser = checkNotNull(forUser, "ForUser");
this.name = LabelType.checkName(name);
}
+ /** @return {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
+ public ForUser forUser() {
+ return forUser;
+ }
+
/** @return name of the label, e.g. {@code "Code-Review"}. */
public String label() {
return name;
@@ -42,12 +86,21 @@
/** @return name used in {@code project.config} permissions. */
@Override
public Optional<String> permissionName() {
- return Optional.of(Permission.forLabel(label()));
+ switch (forUser) {
+ case SELF:
+ return Optional.of(Permission.forLabel(name));
+ case ON_BEHALF_OF:
+ return Optional.of(Permission.forLabelAs(name));
+ }
+ return Optional.empty();
}
@Override
public String describeForException() {
- return "label " + label();
+ if (forUser == ON_BEHALF_OF) {
+ return "labelAs " + name;
+ }
+ return "label " + name;
}
@Override
@@ -57,26 +110,87 @@
@Override
public boolean equals(Object other) {
- return other instanceof LabelPermission && name.equals(((LabelPermission) other).name);
+ if (other instanceof LabelPermission) {
+ LabelPermission b = (LabelPermission) other;
+ return forUser == b.forUser && name.equals(b.name);
+ }
+ return false;
}
@Override
public String toString() {
+ if (forUser == ON_BEHALF_OF) {
+ return "LabelAs[" + name + ']';
+ }
return "Label[" + name + ']';
}
/** A {@link LabelPermission} at a specific value. */
public static class WithValue implements ChangePermissionOrLabel {
+ private final ForUser forUser;
private final LabelVote label;
/**
* Construct a reference to a label at a specific value.
*
+ * @param type description of the label.
+ * @param value numeric score assigned to the label.
+ */
+ public WithValue(LabelType type, LabelValue value) {
+ this(SELF, type, value);
+ }
+
+ /**
+ * Construct a reference to a label at a specific value.
+ *
+ * @param type description of the label.
+ * @param value numeric score assigned to the label.
+ */
+ public WithValue(LabelType type, short value) {
+ this(SELF, type.getName(), value);
+ }
+
+ /**
+ * Construct a reference to a label at a specific value.
+ *
+ * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
+ * @param type description of the label.
+ * @param value numeric score assigned to the label.
+ */
+ public WithValue(ForUser forUser, LabelType type, LabelValue value) {
+ this(forUser, type.getName(), value.getValue());
+ }
+
+ /**
+ * Construct a reference to a label at a specific value.
+ *
+ * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
+ * @param type description of the label.
+ * @param value numeric score assigned to the label.
+ */
+ public WithValue(ForUser forUser, LabelType type, short value) {
+ this(forUser, type.getName(), value);
+ }
+
+ /**
+ * Construct a reference to a label at a specific value.
+ *
* @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
* @param value numeric score assigned to the label.
*/
public WithValue(String name, short value) {
- this(LabelVote.create(name, value));
+ this(SELF, name, value);
+ }
+
+ /**
+ * Construct a reference to a label at a specific value.
+ *
+ * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
+ * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
+ * @param value numeric score assigned to the label.
+ */
+ public WithValue(ForUser forUser, String name, short value) {
+ this(forUser, LabelVote.create(name, value));
}
/**
@@ -85,9 +199,25 @@
* @param label label name and vote.
*/
public WithValue(LabelVote label) {
+ this(SELF, label);
+ }
+
+ /**
+ * Construct a reference to a label at a specific value.
+ *
+ * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
+ * @param label label name and vote.
+ */
+ public WithValue(ForUser forUser, LabelVote label) {
+ this.forUser = checkNotNull(forUser, "ForUser");
this.label = checkNotNull(label, "LabelVote");
}
+ /** @return {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
+ public ForUser forUser() {
+ return forUser;
+ }
+
/** @return name of the label, e.g. {@code "Code-Review"}. */
public String label() {
return label.label();
@@ -101,11 +231,20 @@
/** @return name used in {@code project.config} permissions. */
@Override
public Optional<String> permissionName() {
- return Optional.of(Permission.forLabel(label()));
+ switch (forUser) {
+ case SELF:
+ return Optional.of(Permission.forLabel(label()));
+ case ON_BEHALF_OF:
+ return Optional.of(Permission.forLabelAs(label()));
+ }
+ return Optional.empty();
}
@Override
public String describeForException() {
+ if (forUser == ON_BEHALF_OF) {
+ return "labelAs " + label.formatWithEquals();
+ }
return "label " + label.formatWithEquals();
}
@@ -116,11 +255,18 @@
@Override
public boolean equals(Object other) {
- return other instanceof WithValue && label.equals(((WithValue) other).label);
+ if (other instanceof WithValue) {
+ WithValue b = (WithValue) other;
+ return forUser == b.forUser && label.equals(b.label);
+ }
+ return false;
}
@Override
public String toString() {
+ if (forUser == ON_BEHALF_OF) {
+ return "LabelAs[" + label.format() + ']';
+ }
return "Label[" + label.format() + ']';
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
index 9e5350b..59e8937 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -15,7 +15,9 @@
package com.google.gerrit.server.permissions;
import static com.google.common.base.Preconditions.checkNotNull;
+import static java.util.stream.Collectors.toSet;
+import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project;
@@ -207,5 +209,67 @@
return false;
}
}
+
+ /**
+ * Test which values of a label the user may be able to set.
+ *
+ * @param label definition of the label to test values of.
+ * @return set containing values the user may be able to use; may be empty if none.
+ * @throws PermissionBackendException if failure consulting backend configuration.
+ */
+ public Set<LabelPermission.WithValue> test(LabelType label) throws PermissionBackendException {
+ return test(valuesOf(checkNotNull(label, "LabelType")));
+ }
+
+ private static Set<LabelPermission.WithValue> valuesOf(LabelType label) {
+ return label
+ .getValues()
+ .stream()
+ .map((v) -> new LabelPermission.WithValue(label, v))
+ .collect(toSet());
+ }
+
+ /**
+ * Squash a label value to the nearest allowed value.
+ *
+ * <p>For multi-valued labels like Code-Review with values -2..+2 a user may try to use +2, but
+ * only have permission for the -1..+1 range. The caller should have already tried:
+ *
+ * <pre>
+ * check(new LabelPermission.WithValue("Code-Review", 2));
+ * </pre>
+ *
+ * and caught {@link AuthException}. {@code squashThenCheck} will use {@link #test(LabelType)}
+ * to determine potential values of Code-Review the user can use, and select the nearest value
+ * along the same sign, e.g. -1 for -2 and +1 for +2.
+ *
+ * @param label definition of the label to test values of.
+ * @param val previously denied value the user attempted.
+ * @return nearest allowed value, or {@code 0} if no value was allowed.
+ * @throws PermissionBackendException backend cannot run test or check.
+ */
+ public short squashThenCheck(LabelType label, short val) throws PermissionBackendException {
+ short s = nearest(test(label), val);
+ if (s == 0) {
+ return 0;
+ }
+ try {
+ check(new LabelPermission.WithValue(label, s));
+ return s;
+ } catch (AuthException e) {
+ return 0;
+ }
+ }
+
+ private static short nearest(Iterable<LabelPermission.WithValue> possible, short wanted) {
+ short s = 0;
+ for (LabelPermission.WithValue v : possible) {
+ if ((wanted < 0 && v.value() < 0 && wanted < v.value() && v.value() < s)
+ || (wanted > 0 && v.value() > 0 && wanted > v.value() && v.value() > s)) {
+ s = v.value();
+ }
+ }
+ return s;
+ }
}
}
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 af89d94..42f1a8f 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
@@ -16,6 +16,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
@@ -603,7 +604,11 @@
}
private boolean can(LabelPermission.WithValue perm) {
- return label(perm.permissionName().get()).contains(perm.value());
+ PermissionRange r = label(perm.permissionName().get());
+ if (perm.forUser() == ON_BEHALF_OF && r.isEmpty()) {
+ return false;
+ }
+ return r.contains(perm.value());
}
private PermissionRange label(String permission) {
