Validate scores before matching results in label queries

When searching for approvals we now double check that the user
who applied it to a patch set can still use the value stored.
If the permissions have changed, we retest based on the updated
permissions, potentially skipping the match.

Change-Id: Ia90c42405f6478a62c519b9fe80171c242b26984
Signed-off-by: Shawn O. Pearce <sop@google.com>

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -41,25 +41,21 @@ import com.google.gerrit.server.events.EventFactory;
 import com.google.gerrit.server.git.ChangeMergeQueue;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.LocalDiskRepositoryManager;
-import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.git.MergeQueue;
 import com.google.gerrit.server.git.PushAllProjectsOp;
 import com.google.gerrit.server.git.PushReplication;
 import com.google.gerrit.server.git.ReloadSubmitQueueOp;
 import com.google.gerrit.server.git.ReplicationQueue;
 import com.google.gerrit.server.git.WorkQueue;
-import com.google.gerrit.server.mail.AbandonedSender;
-import com.google.gerrit.server.mail.CommentSender;
 import com.google.gerrit.server.mail.EmailSender;
 import com.google.gerrit.server.mail.FromAddressGenerator;
 import com.google.gerrit.server.mail.FromAddressGeneratorProvider;
-import com.google.gerrit.server.mail.MergeFailSender;
-import com.google.gerrit.server.mail.MergedSender;
-import com.google.gerrit.server.mail.RegisterNewEmailSender;
 import com.google.gerrit.server.mail.SmtpEmailSender;
 import com.google.gerrit.server.patch.PatchListCacheImpl;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
+import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectCacheImpl;
+import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.tools.ToolsCatalog;
 import com.google.gerrit.server.util.IdGenerator;
@@ -140,6 +136,8 @@ public class GerritGlobalModule extends FactoryModule {
 
     bind(PatchSetInfoFactory.class);
     bind(IdentifiedUser.GenericFactory.class).in(SINGLETON);
+    bind(ChangeControl.GenericFactory.class);
+    bind(ProjectControl.GenericFactory.class);
     factory(FunctionState.Factory.class);
     factory(ReplicationUser.Factory.class);
 

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
 
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
+import com.google.gerrit.reviewdb.ApprovalCategory;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.PatchSet;
 import com.google.gerrit.reviewdb.PatchSetApproval;
@@ -24,7 +25,6 @@ import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.workflow.CategoryFunction;
 import com.google.gerrit.server.workflow.FunctionState;
 import com.google.gwtorm.client.OrmException;
@@ -37,6 +37,25 @@ import java.util.List;
 
 /** Access control management for a user accessing a single change. */
 public class ChangeControl {
+  public static class GenericFactory {
+    private final ProjectControl.GenericFactory projectControl;
+
+    @Inject
+    GenericFactory(ProjectControl.GenericFactory p) {
+      projectControl = p;
+    }
+
+    public ChangeControl controlFor(Change change, CurrentUser user)
+        throws NoSuchChangeException {
+      final Project.NameKey projectKey = change.getProject();
+      try {
+        return projectControl.controlFor(projectKey, user).controlFor(change);
+      } catch (NoSuchProjectException e) {
+        throw new NoSuchChangeException(change.getId(), e);
+      }
+    }
+  }
+
   public static class Factory {
     private final ProjectControl.Factory projectControl;
     private final Provider<ReviewDb> db;
@@ -140,6 +159,10 @@ public class ChangeControl {
     ;
   }
 
+  public short normalize(ApprovalCategory.Id category, short score) {
+    return getRefControl().normalize(category, score);
+  }
+
   /** Can this user add a patch set to this change? */
   public boolean canAddPatchSet() {
     return getRefControl().canUpload();

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

@@ -33,6 +33,24 @@ public class ProjectControl {
   public static final int VISIBLE = 1 << 0;
   public static final int OWNER = 1 << 1;
 
+  public static class GenericFactory {
+    private final ProjectCache projectCache;
+
+    @Inject
+    GenericFactory(final ProjectCache pc) {
+      projectCache = pc;
+    }
+
+    public ProjectControl controlFor(Project.NameKey nameKey, CurrentUser user)
+        throws NoSuchProjectException {
+      final ProjectState p = projectCache.get(nameKey);
+      if (p == null) {
+        throw new NoSuchProjectException(nameKey);
+      }
+      return p.controlFor(user);
+    }
+  }
+
   public static class Factory {
     private final ProjectCache projectCache;
     private final Provider<CurrentUser> user;

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

@@ -249,6 +249,24 @@ public class RefControl {
     return canPerform(FORGE_IDENTITY, FORGE_SERVER);
   }
 
+  public short normalize(ApprovalCategory.Id category, short score) {
+    short minAllowed = 0, maxAllowed = 0;
+    for (RefRight r : getApplicableRights(category)) {
+      if (getCurrentUser().getEffectiveGroups().contains(r.getAccountGroupId())) {
+        minAllowed = (short) Math.min(minAllowed, r.getMinValue());
+        maxAllowed = (short) Math.max(maxAllowed, r.getMaxValue());
+      }
+    }
+
+    if (score < minAllowed) {
+      score = minAllowed;
+    }
+    if (score > maxAllowed) {
+      score = maxAllowed;
+    }
+    return score;
+  }
+
   /**
    * Convenience holder class used to map a ref pattern to the list of
    * {@code RefRight}s that use it in the database.

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -34,6 +34,7 @@ public class ChangeData {
   private Change change;
   private Collection<PatchSet> patches;
   private Collection<PatchSetApproval> approvals;
+  private Collection<PatchSetApproval> currentApprovals;
   private Collection<PatchLineComment> comments;
   private Collection<TrackingId> trackingIds;
   private CurrentUser visibleTo;
@@ -89,11 +90,15 @@ public class ChangeData {
 
   public Collection<PatchSetApproval> currentApprovals(Provider<ReviewDb> db)
       throws OrmException {
+    if (currentApprovals == null) {
       Change c = change(db);
       if (c == null) {
-      return Collections.emptyList();
+        currentApprovals = Collections.emptyList();
+      } else {
+        currentApprovals = approvalsFor(db, c.currentPatchSetId());
       }
-    return approvalsFor(db, c.currentPatchSetId());
+    }
+    return currentApprovals;
   }
 
   public Collection<PatchSetApproval> approvalsFor(Provider<ReviewDb> db,

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -90,6 +90,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     final Provider<ChangeQueryRewriter> rewriter;
     final IdentifiedUser.GenericFactory userFactory;
     final ChangeControl.Factory changeControlFactory;
+    final ChangeControl.GenericFactory changeControlGenericFactory;
     final AccountResolver accountResolver;
     final GroupCache groupCache;
     final AuthConfig authConfig;
@@ -101,6 +102,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         Provider<ChangeQueryRewriter> rewriter,
         IdentifiedUser.GenericFactory userFactory,
         ChangeControl.Factory changeControlFactory,
+        ChangeControl.GenericFactory changeControlGenericFactory,
         AccountResolver accountResolver, GroupCache groupCache,
         AuthConfig authConfig, ApprovalTypes approvalTypes,
         @WildProjectName Project.NameKey wildProjectName) {
@@ -108,6 +110,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
       this.rewriter = rewriter;
       this.userFactory = userFactory;
       this.changeControlFactory = changeControlFactory;
+      this.changeControlGenericFactory = changeControlGenericFactory;
       this.accountResolver = accountResolver;
       this.groupCache = groupCache;
       this.authConfig = authConfig;
@@ -242,7 +245,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
 
   @Operator
   public Predicate<ChangeData> label(String name) {
-    return new LabelPredicate(args.dbProvider, args.approvalTypes, name);
+    return new LabelPredicate(args.changeControlGenericFactory,
+        args.userFactory, args.dbProvider, args.approvalTypes, name);
   }
 
   @Operator

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java

@@ -38,7 +38,7 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
               new ChangeQueryBuilder.Arguments( //
                   new InvalidProvider<ReviewDb>(), //
                   new InvalidProvider<ChangeQueryRewriter>(), //
-                  null, null, null, null, null, null, null),
+                  null, null, null, null, null, null, null, null),
               null));
 
   private final Provider<ReviewDb> dbProvider;

gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java

@@ -17,10 +17,11 @@ package com.google.gerrit.server.query.change;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
 import com.google.gerrit.reviewdb.ApprovalCategory;
-import com.google.gerrit.reviewdb.Change;
-import com.google.gerrit.reviewdb.PatchSet;
 import com.google.gerrit.reviewdb.PatchSetApproval;
 import com.google.gerrit.reviewdb.ReviewDb;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.OperatorPredicate;
 import com.google.gwtorm.client.OrmException;
 import com.google.inject.Provider;
@@ -32,24 +33,24 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
   private static enum Test {
     EQ {
       @Override
-      public boolean match(PatchSetApproval p, short value) {
+      public boolean match(short psValue, short expValue) {
-        return p.getValue() == value;
+        return psValue == expValue;
       }
     },
     GT_EQ {
       @Override
-      public boolean match(PatchSetApproval p, short value) {
+      public boolean match(short psValue, short expValue) {
-        return p.getValue() >= value;
+        return psValue >= expValue;
       }
     },
     LT_EQ {
       @Override
-      public boolean match(PatchSetApproval p, short value) {
+      public boolean match(short psValue, short expValue) {
-        return p.getValue() <= value;
+        return psValue <= expValue;
       }
     };
 
-    abstract boolean match(PatchSetApproval p, short value);
+    abstract boolean match(short psValue, short expValue);
   }
 
   private static ApprovalCategory.Id category(ApprovalTypes types, String toFind) {
@@ -98,14 +99,19 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
     return Short.parseShort(value);
   }
 
+  private final ChangeControl.GenericFactory ccFactory;
+  private final IdentifiedUser.GenericFactory userFactory;
   private final Provider<ReviewDb> dbProvider;
   private final Test test;
   private final ApprovalCategory.Id category;
-  private final short val;
+  private final short expVal;
 
-  LabelPredicate(Provider<ReviewDb> dbProvider, ApprovalTypes types,
+  LabelPredicate(ChangeControl.GenericFactory ccFactory,
-      String value) {
+      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
+      ApprovalTypes types, String value) {
     super(ChangeQueryBuilder.FIELD_LABEL, value);
+    this.ccFactory = ccFactory;
+    this.userFactory = userFactory;
     this.dbProvider = dbProvider;
 
     Matcher m1 = Pattern.compile("(=|>=|<=)([+-]?\\d+)$").matcher(value);
@@ -113,36 +119,49 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
     if (m1.find()) {
       category = category(types, value.substring(0, m1.start()));
       test = op(m1.group(1));
-      val = value(m1.group(2));
+      expVal = value(m1.group(2));
 
     } else if (m2.find()) {
       category = category(types, value.substring(0, m2.start()));
       test = Test.EQ;
-      val = value(m2.group(1));
+      expVal = value(m2.group(1));
 
     } else {
       category = category(types, value);
       test = Test.EQ;
-      val = 1;
+      expVal = 1;
     }
   }
 
   @Override
   public boolean match(final ChangeData object) throws OrmException {
-    Change c = object.change(dbProvider);
+    for (PatchSetApproval p : object.currentApprovals(dbProvider)) {
-    if (c == null) {
+      if (p.getCategoryId().equals(category)) {
-      return false;
+        short psVal = p.getValue();
+        if (test.match(psVal, expVal)) {
+          // Double check the value is still permitted for the user.
+          //
+          try {
+            ChangeControl cc = ccFactory.controlFor(object.change(dbProvider), //
+                userFactory.create(dbProvider, p.getAccountId()));
+            if (!cc.isVisible()) {
+              // The user can't see the change anymore.
+              //
+              continue;
+            }
+            psVal = cc.normalize(category, psVal);
+          } catch (NoSuchChangeException e) {
+            // The project has disappeared.
+            //
+            continue;
           }
 
-    PatchSet.Id current = c.currentPatchSetId();
+          if (test.match(psVal, expVal)) {
-    for (PatchSetApproval p : object.approvals(dbProvider)) {
-      if (p.getPatchSetId().equals(current)
-          && p.getCategoryId().equals(category) //
-          && test.match(p, val)) {
             return true;
           }
         }
-
+      }
+    }
     return false;
   }