From ee1ad9b60479e88e057d72b5466897b0e856cf00 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 20 Apr 2010 19:12:02 -0700 Subject: [PATCH] Rewrite ProjectControl canPerformOnAllRefs to actually mean it In order to perform an action on all references, the user must actually have the action. We run into odd corner cases here, as a RefRight on refs/heads/master immediately invalidates the pattern on refs/* implying everything, unless the user also has access to refs/heads/master. Make a collection of every pattern used within the project's access control list, and then loop through each of those and validate that the user has access on that pattern. If they have it on all of them, we can assume it means the have it on all refs if there is also a right on refs/*. Change-Id: Iea5ffce63ec40d024753edf9c519e5ac0c3c2e71 --- .../gerrit/server/project/ProjectControl.java | 78 +++++++++---------- .../gerrit/server/project/RefControl.java | 2 +- 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index be88152389..23a50f38c8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -24,6 +24,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.inject.Inject; import com.google.inject.Provider; +import java.util.HashSet; import java.util.Set; /** Access control management for a user accessing a project's data. */ @@ -137,58 +138,22 @@ public class ProjectControl { private boolean canPerformOnAnyRef(ApprovalCategory.Id actionId, short requireValue) { - return canPerform(actionId, requireValue, null /* any ref */); - } - - private boolean canPerformOnAllRefs(ApprovalCategory.Id actionId, - short requireValue) { - return canPerform(actionId, requireValue, "refs/*"); - } - - /** - * Can this user perform the action in this project, at the level asked? - *

- * This method checks the project rights against the user's effective groups. - * If no right for the given category was granted to any of the user's - * effective groups, then the rights from the wildcard project are checked. - * - * @param actionId unique action id. - * @param requireValue minimum value the application needs to perform this - * action. - * @param refPattern if null, matches any RefRight, otherwise matches only - * those RefRights with the pattern exactly equal to the input. - * @return true if the action can be performed; false if the user lacks the - * necessary permission. - */ - private boolean canPerform(final ApprovalCategory.Id actionId, - final short requireValue, final String refPattern) { final Set groups = user.getEffectiveGroups(); int val = Integer.MIN_VALUE; - for (final RefRight pr : state.getLocalRights()) { - if (actionId.equals(pr.getApprovalCategoryId()) - && (refPattern == null || refPattern.equals(pr.getRefPattern())) - && groups.contains(pr.getAccountGroupId())) { + for (final RefRight pr : state.getLocalRights(actionId)) { + if (groups.contains(pr.getAccountGroupId())) { if (val < 0 && pr.getMaxValue() > 0) { - // If one of the user's groups had denied them access, but - // this group grants them access, prefer the grant over - // the denial. We have to break the tie somehow and we - // prefer being "more open" to being "more closed". - // val = pr.getMaxValue(); } else { - // Otherwise we use the largest value we can get. - // val = Math.max(pr.getMaxValue(), val); } } } if (val == Integer.MIN_VALUE && actionId.canInheritFromWildProject()) { - for (final RefRight pr : state.getInheritedRights()) { - if (actionId.equals(pr.getApprovalCategoryId()) - && (refPattern == null || refPattern.equals(pr.getRefPattern())) - && groups.contains(pr.getAccountGroupId())) { + for (final RefRight pr : state.getInheritedRights(actionId)) { + if (groups.contains(pr.getAccountGroupId())) { val = Math.max(pr.getMaxValue(), val); } } @@ -196,4 +161,37 @@ public class ProjectControl { return val >= requireValue; } + + private boolean canPerformOnAllRefs(ApprovalCategory.Id actionId, + short requireValue) { + boolean canPerform = false; + final Set patterns = allRefPatterns(actionId); + if (patterns.contains(RefRight.ALL)) { + // Only possible if granted on the pattern that + // matches every possible reference. Check all + // patterns also have the permission. + // + for (final String pattern : patterns) { + if (controlForRef(pattern).canPerform(actionId, requireValue)) { + canPerform = true; + } else { + return false; + } + } + } + return canPerform; + } + + private Set allRefPatterns(ApprovalCategory.Id actionId) { + final Set all = new HashSet(); + for (final RefRight pr : state.getLocalRights(actionId)) { + all.add(pr.getRefPattern()); + } + if (actionId.canInheritFromWildProject()) { + for (final RefRight pr : state.getInheritedRights(actionId)) { + all.add(pr.getRefPattern()); + } + } + return all; + } } 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 6f553beb5e..d0e87d8bc2 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 @@ -224,7 +224,7 @@ public class RefControl { return canPerform(FORGE_IDENTITY, FORGE_SERVER); } - private boolean canPerform(ApprovalCategory.Id actionId, short level) { + boolean canPerform(ApprovalCategory.Id actionId, short level) { final Set groups = getCurrentUser().getEffectiveGroups(); int val = Integer.MIN_VALUE;