Simplify reference level access control.v2.1.3-rc0

The initial implementation of reference level access control only
supports a corner case, that of "locking down" access for a specific
branch.

Upon further discussion, we've decided that this is not the more
general need. Most Gerrit configurations prefer to have a more "open"
access model, where access rights on a reference specified with a
wildcard, such as "refs/heads/*" aren't overridden by a more specific
access right. So this change makes the default behavior to evaluate
all rights, including the wild card ones.

However, in order to accomodate the corner case we were supporting
before, this change also introduces a new way to specify exclusive
reference level access rights. All access rights that start with the
'-' prefix are considered exclusive, and will prevent all wild card
rights from being considered.

Change-Id: I629f5439967b2141e46098614fadb25ff28e5f45

14 files changed, 376 insertions, 62 deletions

diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index b74fc1d957..66b4082baa 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -140,26 +140,65 @@ prefix. So a permission with `refs/heads/*` will match
 `refs/heads/master` and `refs/heads/experimental`, etc.
 
 When evaluating a reference-level access right, Gerrit will use
-the most specific set of access rights to determine if the user
-is allowed to perform a given action. For example, if a user
-tries to review a change destined for branch `refs/heads/qa`
-in project `tools/gerrit`, and the following ACLs are granted:
+the full set of access rights to determine if the user
+is allowed to perform a given action. For example, if a user is a
+member of `Foo Leads`, they are reviewing a change destined for
+the `refs/heads/qa` branch, and the following ACLs are granted
+on the project:
 
 [grid="all"]
-`---------------`----------------`-------------`-------
-Group            Reference Name  Category       Range
--------------------------------------------------------
-Anonymous Users  refs/heads/*    Code Review     -1..+1
-Registered Users refs/heads/*    Code Review     -1..+1
-Foo Leads        refs/heads/*    Code Review     -2..+2
-QA Leads         refs/heads/qa   Code Review     -2..+2
--------------------------------------------------------
-
-Then this user will have `Code Review -2..+2` if he is a member
-of `QA Leads`, and will not have any rights if not. Inherited ACLs
-from the `\-- All Projects \--` project thus allow system wide
-lock-down of a branch, by granting a permission to a limited group
-of users on that branch.
+`---------------`---------------`-------------`-------
+Group            Reference Name Category       Range
+------------------------------------------------------
+Registered Users refs/heads/*   Code Review     -1..+1
+Foo Leads        refs/heads/*   Code Review     -2..+2
+QA Leads         refs/heads/qa  Code Review     -2..+2
+------------------------------------------------------
+
+Then the effective range permitted to be used by the user is
+`-2..+2`, as the user's membership of `Foo Leads` effectively grant
+them access to the entire reference space, thanks to the wildcard.
+
+Gerrit also supports exclusive reference-level access control.
+
+It is possible to configure Gerrit to grant an exclusive ref level
+access control so that only users of a specific group can perform
+an operation on a project/reference pair. This is done by prefixing
+the reference specified with a `'-'`.
+
+For example, if a user who is a member of `Foo Leads` tries to
+review a change destined for branch `refs/heads/qa` in a project,
+and the following ACLs are granted:
+
+[grid="all"]
+`---------------`----------------`--------------`-------
+Group            Reference Name   Category       Range
+--------------------------------------------------------
+Registered Users refs/heads/*     Code Review     -1..+1
+Foo Leads        refs/heads/*     Code Review     -2..+2
+QA Leads         -refs/heads/qa   Code Review     -2..+2
+--------------------------------------------------------
+
+Then this user will not have `Code Review` rights on that change,
+since there is an exclusive access right in place for the
+`refs/heads/qa` branch. This allows locking down access for a
+particular branch to a limited set of users, bypassing inherited
+rights and wildcards.
+
+In order to grant the ability to `Code Review` to the members of
+`Foo Leads`, in `refs/heads/qa` then the following access rights
+would be needed:
+
+[grid="all"]
+`---------------`----------------`--------------`-------
+Group            Reference Name   Category       Range
+--------------------------------------------------------
+Registered Users refs/heads/*     Code Review     -1..+1
+Foo Leads        refs/heads/*     Code Review     -2..+2
+QA Leads         -refs/heads/qa   Code Review     -2..+2
+Foo Leads        refs/heads/qa    Code Review     -2..+2
+--------------------------------------------------------
+
 
 OpenID Authentication
 ~~~~~~~~~~~~~~~~~~~~~
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectRightsPanel.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectRightsPanel.java
index 80f1c38abd..75d6931f4b 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectRightsPanel.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectRightsPanel.java
@@ -483,7 +483,7 @@ public class ProjectRightsPanel extends Composite {
             .get()));
       }
 
-      table.setText(row, 4, right.getRefPattern());
+      table.setText(row, 4, right.getRefPatternForDisplay());
 
       {
         final SafeHtmlBuilder m = new SafeHtmlBuilder();
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
index d4a8cf55e8..c98d12974f 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
@@ -133,7 +133,7 @@ final class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail>
       categoryRights.addAll(filterMatching(pe.getLocalRights(category)));
       categoryRights.addAll(filterMatching(pe.getInheritedRights(category)));
       Collections.sort(categoryRights, RefRight.REF_PATTERN_ORDER);
-      categoryRights = RefControl.filterMostSpecific(categoryRights);
+
       computeAllowed(am, categoryRights, category);
     }
   }
@@ -157,11 +157,27 @@ final class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail>
       allowed.put(category, s);
     }
 
+    boolean foundExclusive = false;
+    String previousPattern = "";
     for (final RefRight r : list) {
+
       if (!am.contains(r.getAccountGroupId())) {
+        if (r.isExclusive()) {
+          foundExclusive = true;
+        }
         continue;
       }
 
+      if (foundExclusive && !previousPattern.equals(r.getRefPattern())) {
+        break;
+      }
+
+      if (r.isExclusive()) {
+        foundExclusive = true;
+      }
+
+      previousPattern = r.getRefPattern();
+
       final ApprovalType at =
           approvalTypes.getApprovalType(r.getApprovalCategoryId());
       for (short m = r.getMinValue(); m <= r.getMaxValue(); m++) {
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/AddRefRight.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/AddRefRight.java
index 1e39d5b58d..06f15c22d3 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/AddRefRight.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/AddRefRight.java
@@ -135,6 +135,12 @@ class AddRefRight extends Handler<ProjectDetail> {
         refPattern = RefRight.ALL;
       }
     }
+
+    boolean exclusive = refPattern.startsWith("-");
+    if (exclusive) {
+      refPattern = refPattern.substring(1);
+    }
+
     while (refPattern.startsWith("/")) {
       refPattern = refPattern.substring(1);
     }
@@ -152,6 +158,10 @@ class AddRefRight extends Handler<ProjectDetail> {
       }
     }
 
+    if (exclusive) {
+      refPattern = "-" + refPattern;
+    }
+
     if (!controlForRef(projectControl, refPattern).isOwner()) {
       throw new NoSuchRefException(refPattern);
     }
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Init.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Init.java
index 8e5445a995..445c10aad6 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Init.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Init.java
@@ -52,6 +52,7 @@ import com.google.inject.spi.Message;
 
 import org.kohsuke.args4j.Option;
 
+import java.io.Console;
 import java.io.File;
 import java.io.IOException;
 import java.util.ArrayList;
@@ -195,6 +196,11 @@ public class Init extends SiteProgram {
         }
 
         @Override
+        public boolean yesno(boolean def, String msg) {
+          return ui.yesno(def, msg);
+        }
+
+        @Override
         public void pruneSchema(StatementExecutor e, List<String> prune) {
           for (String p : prune) {
             if (!pruneList.contains(p)) {
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/RefRight.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/RefRight.java
index a3ba2f91c8..0db09e49d1 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/RefRight.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/RefRight.java
@@ -121,6 +121,13 @@ public final class RefRight {
   }
 
   public String getRefPattern() {
+    if (isExclusive()) {
+      return key.refPattern.get().substring(1);
+    }
+    return key.refPattern.get();
+  }
+
+  public String getRefPatternForDisplay() {
     return key.refPattern.get();
   }
 
@@ -128,6 +135,10 @@ public final class RefRight {
     return getKey().getProjectNameKey();
   }
 
+  public boolean isExclusive() {
+    return key.refPattern.get().startsWith("-");
+  }
+
   public ApprovalCategory.Id getApprovalCategoryId() {
     return key.categoryId;
   }
@@ -153,6 +164,25 @@ public final class RefRight {
   }
 
   @Override
+  public String toString() {
+    StringBuilder s = new StringBuilder();
+    s.append("{group :");
+    s.append(getAccountGroupId().get());
+    s.append(", proj :");
+    s.append(getProjectNameKey().get());
+    s.append(", cat :");
+    s.append(getApprovalCategoryId().get());
+    s.append(", pattern :");
+    s.append(getRefPatternForDisplay());
+    s.append(", min :");
+    s.append(getMinValue());
+    s.append(", max :");
+    s.append(getMaxValue());
+    s.append("}");
+    return s.toString();
+  }
+
+  @Override
   public int hashCode() {
     return getKey().hashCode();
   }
@@ -169,19 +199,23 @@ public final class RefRight {
     return false;
   }
 
-  private static class RefPatternOrder implements Comparator<RefRight> {
+  public static final Comparator<RefRight> REF_PATTERN_ORDER =
+      new Comparator<RefRight>() {
 
     @Override
     public int compare(RefRight a, RefRight b) {
       int aLength = a.getRefPattern().length();
       int bLength = b.getRefPattern().length();
-      if ((bLength - aLength) == 0) {
+      if (bLength == aLength) {
+        ApprovalCategory.Id aCat = a.getApprovalCategoryId();
+        ApprovalCategory.Id bCat = b.getApprovalCategoryId();
+        if (aCat.get().equals(bCat.get())) {
+          return a.getRefPattern().compareTo(b.getRefPattern());
+        }
         return a.getApprovalCategoryId().get()
             .compareTo(b.getApprovalCategoryId().get());
       }
       return bLength - aLength;
     }
-  }
-
-  public static final RefPatternOrder REF_PATTERN_ORDER = new RefPatternOrder();
+  };
 }
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 973302c387..afeb422319 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
@@ -44,8 +44,11 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
 
 
 /** Manages access control for Git references (aka branches, tags). */
@@ -225,6 +228,52 @@ public class RefControl {
     return canPerform(FORGE_IDENTITY, FORGE_SERVER);
   }
 
+  /**
+   * Convenience holder class used to map a ref pattern to the list of
+   * {@code RefRight}s that use it in the database.
+   */
+  public final static class RefRightsForPattern {
+    private final List<RefRight> rights;
+    private boolean containsExclusive;
+
+    public RefRightsForPattern() {
+      rights = new ArrayList<RefRight>();
+      containsExclusive = false;
+    }
+
+    public void addRight(RefRight right) {
+      rights.add(right);
+      if (right.isExclusive()) {
+        containsExclusive = true;
+      }
+    }
+
+    public List<RefRight> getRights() {
+      return Collections.unmodifiableList(rights);
+    }
+
+    public boolean containsExclusive() {
+      return containsExclusive;
+    }
+
+    /**
+     * Returns The max allowed value for this ref pattern for all specified
+     * groups.
+     *
+     * @param groups The groups of the user
+     * @return The allowed value for this ref for all the specified groups
+     */
+    public int allowedValueForRef(Set<AccountGroup.Id> groups) {
+      int val = Integer.MIN_VALUE;
+      for (RefRight right : rights) {
+        if (groups.contains(right.getAccountGroupId())) {
+          val = Math.max(right.getMaxValue(), val);
+        }
+      }
+      return val;
+    }
+  }
+
   boolean canPerform(ApprovalCategory.Id actionId, short level) {
     final Set<AccountGroup.Id> groups = getCurrentUser().getEffectiveGroups();
     int val = Integer.MIN_VALUE;
@@ -236,42 +285,68 @@ public class RefControl {
       allRights.addAll(getInheritedRights(actionId));
     }
 
-    // Sort in descending refPattern length
-    Collections.sort(allRights, RefRight.REF_PATTERN_ORDER);
+    SortedMap<String, RefRightsForPattern> perPatternRights =
+      sortedRightsByPattern(allRights);
 
-    for (RefRight right : filterMostSpecific(allRights)) {
-      if (groups.contains(right.getAccountGroupId())) {
-        val = Math.max(right.getMaxValue(), val);
+    for (RefRightsForPattern right : perPatternRights.values()) {
+      val = Math.max(val, right.allowedValueForRef(groups));
+      if (val >= level || right.containsExclusive()) {
+        return val >= level;
       }
     }
     return val >= level;
   }
 
-  public static List<RefRight> filterMostSpecific(List<RefRight> actionRights) {
-    // Grab the first set of RefRight which have the same refPattern
-    // those are the most specific RefRights we have, and are the
-    // we will consider to verify if this action can be performed.
-    // We do this so that one can override the ref rights for a specific
-    // project on a specific branch
-    boolean sameRefPattern = true;
-    List<RefRight> mostSpecific = new ArrayList<RefRight>();
-    String currentRefPattern = null;
-    int i = 0;
-    while (sameRefPattern && i < actionRights.size()) {
-      if (currentRefPattern == null) {
-        currentRefPattern = actionRights.get(i).getRefPattern();
-        mostSpecific.add(actionRights.get(i));
-        i++;
-      } else {
-        if (currentRefPattern.equals(actionRights.get(i).getRefPattern())) {
-          mostSpecific.add(actionRights.get(i));
-          i++;
-        } else {
-          sameRefPattern = false;
-        }
+  public static final Comparator<String> DESCENDING_SORT =
+      new Comparator<String>() {
+
+    @Override
+    public int compare(String a, String b) {
+      int aLength = a.length();
+      int bLength = b.length();
+      if (bLength == aLength) {
+        return a.compareTo(b);
+      }
+      return bLength - aLength;
+    }
+  };
+
+  /**
+   * Sorts all given rights into a map, ordered by descending length of
+   * ref pattern.
+   *
+   * For example, if given the following rights in argument:
+   *
+   * ["refs/heads/master", group1, -1, +1],
+   * ["refs/heads/master", group2, -2, +2],
+   * ["refs/heads/*", group3, -1, +1]
+   * ["refs/heads/stable", group2, -1, +1]
+   *
+   * Then the following map is returned:
+   * "refs/heads/master" => {
+   *      ["refs/heads/master", group1, -1, +1],
+   *      ["refs/heads/master", group2, -2, +2]
+   *  }
+   * "refs/heads/stable" => {["refs/heads/stable", group2, -1, +1]}
+   * "refs/heads/*" => {["refs/heads/*", group3, -1, +1]}
+   *
+   * @param actionRights
+   * @return A sorted map keyed off the ref pattern of all rights.
+   */
+  private static SortedMap<String, RefRightsForPattern> sortedRightsByPattern(
+      List<RefRight> actionRights) {
+    SortedMap<String, RefRightsForPattern> rights =
+      new TreeMap<String, RefRightsForPattern>(DESCENDING_SORT);
+    for (RefRight actionRight : actionRights) {
+      RefRightsForPattern patternRights =
+        rights.get(actionRight.getRefPattern());
+      if (patternRights == null) {
+        patternRights = new RefRightsForPattern();
+        rights.put(actionRight.getRefPattern(), patternRights);
       }
+      patternRights.addRight(actionRight);
     }
-    return mostSpecific;
+    return rights;
   }
 
   private List<RefRight> getLocalRights(ApprovalCategory.Id actionId) {
@@ -282,12 +357,30 @@ public class RefControl {
     return filter(getProjectState().getInheritedRights(actionId));
   }
 
-  public List<RefRight> getAllRights(final ApprovalCategory.Id id) {
+  /**
+   * Returns all applicable rights for a given approval category.
+   *
+   * Applicable rights are defined as the list of {@code RefRight}s which match
+   * the ref for which this object was created, stopping the ref wildcard
+   * matching when an exclusive ref right was encountered, for the given
+   * approval category.
+   * @param id The {@link ApprovalCategory.Id}.
+   * @return All applicalbe rights.
+   */
+  public List<RefRight> getApplicableRights(final ApprovalCategory.Id id) {
     List<RefRight> l = new ArrayList<RefRight>();
     l.addAll(getLocalRights(id));
     l.addAll(getInheritedRights(id));
-    Collections.sort(l, RefRight.REF_PATTERN_ORDER);
-    return Collections.unmodifiableList(RefControl.filterMostSpecific(l));
+    SortedMap<String, RefRightsForPattern> perPatternRights =
+      sortedRightsByPattern(l);
+    List<RefRight> applicable = new ArrayList<RefRight>();
+    for (RefRightsForPattern patternRights : perPatternRights.values()) {
+      applicable.addAll(patternRights.getRights());
+      if (patternRights.containsExclusive()) {
+        break;
+      }
+    }
+    return Collections.unmodifiableList(applicable);
   }
 
   private List<RefRight> filter(Collection<RefRight> all) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
index 2be0d49710..9f578bce8d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
@@ -32,7 +32,7 @@ import java.util.List;
 /** A version of the database schema. */
 public abstract class SchemaVersion {
   /** The current schema version. */
-  private static final Class<? extends SchemaVersion> C = Schema_33.class;
+  private static final Class<? extends SchemaVersion> C = Schema_34.class;
 
   public static class Module extends AbstractModule {
     @Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_34.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_34.java
new file mode 100644
index 0000000000..2cdb47e3c9
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_34.java
@@ -0,0 +1,110 @@
+// Copyright (C) 2010 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.schema;
+
+import com.google.gerrit.reviewdb.ApprovalCategory;
+import com.google.gerrit.reviewdb.Project;
+import com.google.gerrit.reviewdb.RefRight;
+import com.google.gerrit.reviewdb.ReviewDb;
+import com.google.gerrit.reviewdb.RefRight.RefPattern;
+import com.google.gerrit.server.project.RefControl;
+import com.google.gerrit.server.project.RefControl.RefRightsForPattern;
+import com.google.gwtorm.client.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
+
+public class Schema_34 extends SchemaVersion {
+  @Inject
+  Schema_34(Provider<Schema_33> prior) {
+    super(prior);
+  }
+
+
+  @Override
+  protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException {
+    Iterable<Project> projects = db.projects().all();
+    boolean showedBanner = false;
+
+    List<RefRight> toUpdate = new ArrayList<RefRight>();
+    List<RefRight> toDelete = new ArrayList<RefRight>();
+    for (Project p : projects) {
+      boolean showedProject = false;
+      List<RefRight> pr = db.refRights().byProject(p.getNameKey()).toList();
+      Map<ApprovalCategory.Id, Map<String, RefRightsForPattern>> r =
+        new HashMap<ApprovalCategory.Id, Map<String, RefRightsForPattern>>();
+      for (RefRight right : pr) {
+        ui.message(right.toString());
+        ApprovalCategory.Id cat = right.getApprovalCategoryId();
+        if (r.get(cat) == null) {
+          Map<String, RefRightsForPattern> m =
+            new TreeMap<String, RefRightsForPattern>(RefControl.DESCENDING_SORT);
+          r.put(cat, m);
+        }
+        if (r.get(cat).get(right.getRefPattern()) == null) {
+          RefRightsForPattern s = new RefRightsForPattern();
+          r.get(cat).put(right.getRefPattern(), s);
+        }
+        r.get(cat).get(right.getRefPattern()).addRight(right);
+      }
+
+      for (Map<String, RefRightsForPattern> categoryRights : r.values()) {
+        for (RefRightsForPattern rrp : categoryRights.values()) {
+          RefRight oldRight = rrp.getRights().get(0);
+          if (shouldPrompt(oldRight)) {
+            if (!showedBanner) {
+              ui.message("Entering interactive reference rights migration tool...");
+              showedBanner = true;
+            }
+            if (!showedProject) {
+              ui.message("In project " + p.getName());
+              showedProject = true;
+            }
+            ui.message("For category " + oldRight.getApprovalCategoryId());
+            boolean isWildcard = oldRight.getRefPattern().endsWith("/*");
+            boolean shouldUpdate = ui.yesno(!isWildcard,
+                "Should rights for pattern "
+                + oldRight.getRefPattern()
+                + " be considered exclusive?");
+            if (shouldUpdate) {
+              RefRight.Key newKey = new RefRight.Key(oldRight.getProjectNameKey(),
+                  new RefPattern("-" + oldRight.getRefPattern()),
+                  oldRight.getApprovalCategoryId(),
+                  oldRight.getAccountGroupId());
+              RefRight newRight = new RefRight(newKey);
+              newRight.setMaxValue(oldRight.getMaxValue());
+              newRight.setMinValue(oldRight.getMinValue());
+              toUpdate.add(newRight);
+              toDelete.add(oldRight);
+            }
+          }
+        }
+      }
+    }
+    db.refRights().insert(toUpdate);
+    db.refRights().delete(toDelete);
+  }
+
+  private boolean shouldPrompt(RefRight right) {
+    return !right.getRefPattern().equals("refs/*")
+      && !right.getRefPattern().equals("refs/heads/*")
+      && !right.getRefPattern().equals("refs/tags/*");
+  }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/UpdateUI.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/UpdateUI.java
index f0109db848..ac07efd285 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/UpdateUI.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/UpdateUI.java
@@ -22,6 +22,8 @@ import java.util.List;
 public interface UpdateUI {
   void message(String msg);
 
+  boolean yesno(boolean def, String msg);
+
   void pruneSchema(StatementExecutor e, List<String> pruneList)
       throws OrmException;
 }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java
index 649a502302..dd29f0478e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java
@@ -92,7 +92,7 @@ public abstract class CategoryFunction {
   public boolean isValid(final CurrentUser user, final ApprovalType at,
       final FunctionState state) {
     RefControl rc = state.controlFor(user);
-    for (final RefRight pr : rc.getAllRights(at.getCategory().getId())) {
+    for (final RefRight pr : rc.getApplicableRights(at.getCategory().getId())) {
       if (user.getEffectiveGroups().contains(pr.getAccountGroupId())
           && (pr.getMinValue() < 0 || pr.getMaxValue() > 0)) {
         return true;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java
index 5bf39e1455..36a52e23ca 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java
@@ -146,7 +146,7 @@ public class FunctionState {
     // Find the maximal range actually granted to the user.
     //
     short minAllowed = 0, maxAllowed = 0;
-    for (final RefRight r : rc.getAllRights(a.getCategoryId())) {
+    for (final RefRight r : rc.getApplicableRights(a.getCategoryId())) {
       final AccountGroup.Id grp = r.getAccountGroupId();
       if (user.getEffectiveGroups().contains(grp)) {
         minAllowed = (short) Math.min(minAllowed, r.getMinValue());
@@ -154,8 +154,7 @@ public class FunctionState {
       }
     }
 
-    // Normalize the value into that range, returning true if we changed
-    // the value.
+    // Normalize the value into that range.
     //
     if (a.getValue() < minAllowed) {
       a.setValue(minAllowed);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/SubmitFunction.java b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/SubmitFunction.java
index e097eeb136..f0a00ff095 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/SubmitFunction.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/SubmitFunction.java
@@ -44,7 +44,7 @@ public class SubmitFunction extends CategoryFunction {
       final FunctionState state) {
     if (valid(at, state)) {
       RefControl rc = state.controlFor(user);
-      for (final RefRight pr : rc.getAllRights(at.getCategory().getId())) {
+      for (final RefRight pr : rc.getApplicableRights(at.getCategory().getId())) {
         if (user.getEffectiveGroups().contains(pr.getAccountGroupId())
             && pr.getMaxValue() > 0) {
           return true;
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaUpdaterTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaUpdaterTest.java
index bbd8d77c7d..1e01674699 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaUpdaterTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaUpdaterTest.java
@@ -67,6 +67,11 @@ public class SchemaUpdaterTest extends TestCase {
       }
 
       @Override
+      public boolean yesno(boolean def, String msg) {
+        return def;
+      }
+
+      @Override
       public void pruneSchema(StatementExecutor e, List<String> pruneList)
           throws OrmException {
         for (String sql : pruneList) {