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

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java

@@ -314,7 +314,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
     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);
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java

@@ -214,7 +214,7 @@ class RevisionApiImpl implements RevisionApi {
   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);
     }
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
 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.Nullable;
 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.ApprovalsUtil;
 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.ChangeNotes;
 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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
 
   @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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
   }
 
   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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         continue;
       }
 
-      if (caller.getUser().isInternalUser()) {
-        continue;
-      }
-
-      PermissionRange r = caller.getRange(Permission.forLabelAs(type.getName()));
-      if (r == null || r.isEmpty() || !r.contains(ent.getValue())) {
+      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\"",
-                ent.getKey(), in.onBehalfOf));
+                  type.getName(), in.onBehalfOf));
+        }
       }
     }
     if (in.labels.isEmpty()) {
@@ -390,25 +398,26 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           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()));
-    }
-    return new RevisionResource(changes.parse(target), rev.getPatchSet());
+          String.format("on_behalf_of account %s cannot see change", reviewer.getAccountId()));
     }
 
-  private void checkLabels(RevisionResource revision, boolean strict, Map<String, Short> labels)
-      throws BadRequestException, AuthException {
-    ChangeControl ctl = revision.getControl();
+    ChangeControl ctl = rev.getControl().forUser(reviewer);
+    return new RevisionResource(changes.parse(ctl), rev.getPatchSet());
+  }
+
+  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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         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));
       }
     }
   }

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 @@ public class LabelPermission implements ChangePermissionOrLabel {
   /** @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,18 +110,68 @@ public class LabelPermission implements ChangePermissionOrLabel {
 
   @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.
      *
@@ -76,7 +179,18 @@ public class LabelPermission implements ChangePermissionOrLabel {
      * @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 @@ public class LabelPermission implements ChangePermissionOrLabel {
      * @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 @@ public class LabelPermission implements ChangePermissionOrLabel {
     /** @return name used in {@code project.config} permissions. */
     @Override
     public Optional<String> permissionName() {
+      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 @@ public class LabelPermission implements ChangePermissionOrLabel {
 
     @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() + ']';
     }
   }

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 @@ public abstract class PermissionBackend {
         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;
+    }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
 
 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 @@ public class ChangeControl {
     }
 
     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) {