opendev/gerrit
Code Issues Proposed changes

Simplify reference level access control.

Browse Source 
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
This commit is contained in:
Nico Sallembien
2010-05-04 11:49:12 -07:00
parent fc8e99f041
commit a78a37cf45
14 changed files with 375 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files

73
Documentation/access-control.txt
View File

@@ -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
-------------------------------------------------------
`---------------`---------------`-------------`-------
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
--------------------------------------------------------
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.
OpenID Authentication
~~~~~~~~~~~~~~~~~~~~~

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectRightsPanel.java
View File

@@ -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();

18
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
View File

@@ -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++) {

10
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/AddRefRight.java
View File

@@ -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);
}

6
gerrit-pgm/src/main/java/com/google/gerrit/pgm/Init.java
View File

@@ -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;
@@ -194,6 +195,11 @@ public class Init extends SiteProgram {
System.err.flush();
}
@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) {

44
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/RefRight.java
View File

@@ -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;
}
@@ -152,6 +163,25 @@ public final class RefRight {
maxValue = m;
}
@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();
};
}

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

@@ -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;
}
return mostSpecific;
};
/**
* 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 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) {

2
gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
View File

@@ -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

110
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_34.java Normal file
View File

@@ -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/*");
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/schema/UpdateUI.java
View File

@@ -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;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java
View File

@@ -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;

5
gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java
View File

@@ -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);

2
gerrit-server/src/main/java/com/google/gerrit/server/workflow/SubmitFunction.java
View File

@@ -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;

5
gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaUpdaterTest.java
View File

@@ -66,6 +66,11 @@ public class SchemaUpdaterTest extends TestCase {
public void message(String msg) {
}
@Override
public boolean yesno(boolean def, String msg) {
return def;
}
@Override
public void pruneSchema(StatementExecutor e, List<String> pruneList)
throws OrmException {