From 2bfb8ab137ac18b60cad0c375c7b9bef67499b94 Mon Sep 17 00:00:00 2001 From: James Moger Date: Tue, 23 Oct 2012 17:35:42 -0400 Subject: [PATCH] Enforce strict order for permission determination The order of permissions defined within a user or team is preserved during read and write. This order is important for determining the regex match used within the user or team object. If the user is an admin or repository owner, then RW+ Else if user has an explicit permission, use that Else check for the first regex match in user permissions Else check for the HIGHEST permission from team memberships If the team is an admin team, then RW+ Else if a team has an explicit permission, use that Else check for the first regex match in team permissions --- src/com/gitblit/models/TeamModel.java | 8 +- src/com/gitblit/models/UserModel.java | 22 ++-- tests/com/gitblit/tests/PermissionsTest.java | 120 ++++++++++++++++++- 3 files changed, 137 insertions(+), 13 deletions(-) diff --git a/src/com/gitblit/models/TeamModel.java b/src/com/gitblit/models/TeamModel.java index 6410eb45..7d557db9 100644 --- a/src/com/gitblit/models/TeamModel.java +++ b/src/com/gitblit/models/TeamModel.java @@ -19,8 +19,8 @@ import java.io.Serializable; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -51,7 +51,7 @@ public class TeamModel implements Serializable, Comparable { // retained for backwards-compatibility with RPC clients @Deprecated public final Set repositories = new HashSet(); - public final Map permissions = new HashMap(); + public final Map permissions = new LinkedHashMap(); public final Set mailingLists = new HashSet(); public final List preReceiveScripts = new ArrayList(); public final List postReceiveScripts = new ArrayList(); @@ -191,6 +191,8 @@ public class TeamModel implements Serializable, Comparable { AccessPermission p = permissions.get(key); if (p != null) { permission = p; + // take first match + break; } } } @@ -198,7 +200,7 @@ public class TeamModel implements Serializable, Comparable { return permission; } - private boolean canAccess(RepositoryModel repository, AccessRestrictionType ifRestriction, AccessPermission requirePermission) { + protected boolean canAccess(RepositoryModel repository, AccessRestrictionType ifRestriction, AccessPermission requirePermission) { if (repository.accessRestriction.atLeast(ifRestriction)) { AccessPermission permission = getRepositoryPermission(repository); return permission.atLeast(requirePermission); diff --git a/src/com/gitblit/models/UserModel.java b/src/com/gitblit/models/UserModel.java index 6cc07789..d7bc2935 100644 --- a/src/com/gitblit/models/UserModel.java +++ b/src/com/gitblit/models/UserModel.java @@ -19,8 +19,8 @@ import java.io.Serializable; import java.security.Principal; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -60,7 +60,7 @@ public class UserModel implements Principal, Serializable, Comparable // retained for backwards-compatibility with RPC clients @Deprecated public final Set repositories = new HashSet(); - public final Map permissions = new HashMap(); + public final Map permissions = new LinkedHashMap(); public final Set teams = new HashSet(); // non-persisted fields @@ -217,8 +217,8 @@ public class UserModel implements Principal, Serializable, Comparable return AccessPermission.REWIND; } - // determine best permission available based on user's personal permissions - // and the permissions of teams of which the user belongs + // explicit user permission OR user regex match is used + // if that fails, then the best team permission is used AccessPermission permission = AccessPermission.NONE; if (permissions.containsKey(repository.name.toLowerCase())) { // exact repository permission specified, use it @@ -232,17 +232,21 @@ public class UserModel implements Principal, Serializable, Comparable if (StringUtils.matchesIgnoreCase(repository.name, key)) { AccessPermission p = permissions.get(key); if (p != null) { + // take first match permission = p; + break; } } } } - for (TeamModel team : teams) { - AccessPermission p = team.getRepositoryPermission(repository); - if (permission == null || p.exceeds(permission)) { - // use team permission - permission = p; + if (AccessPermission.NONE.equals(permission)) { + for (TeamModel team : teams) { + AccessPermission p = team.getRepositoryPermission(repository); + if (p.exceeds(permission)) { + // use highest team permission + permission = p; + } } } return permission; diff --git a/tests/com/gitblit/tests/PermissionsTest.java b/tests/com/gitblit/tests/PermissionsTest.java index c0e406d4..befd3603 100644 --- a/tests/com/gitblit/tests/PermissionsTest.java +++ b/tests/com/gitblit/tests/PermissionsTest.java @@ -2393,7 +2393,7 @@ public class PermissionsTest extends Assert { } @Test - public void testWildcardMatching() throws Exception { + public void testRegexMatching() throws Exception { RepositoryModel repository = new RepositoryModel("ubercool/_my-r/e~po.git", null, null, new Date()); repository.authorizationControl = AuthorizationControl.NAMED; repository.accessRestriction = AccessRestrictionType.VIEW; @@ -2415,7 +2415,125 @@ public class PermissionsTest extends Assert { assertFalse("user CAN delete!", user.canDelete(repository)); assertFalse("user CAN edit!", user.canEdit(repository)); } + + @Test + public void testRegexIncludeCommonExcludePersonal() throws Exception { + + UserModel user = new UserModel("test"); + user.setRepositoryPermission("[^~].*", AccessPermission.CLONE); + + // common + RepositoryModel common = new RepositoryModel("ubercool/_my-r/e~po.git", null, null, new Date()); + common.authorizationControl = AuthorizationControl.NAMED; + common.accessRestriction = AccessRestrictionType.VIEW; + + assertTrue("user DOES NOT HAVE a repository permission!", user.hasRepositoryPermission(common.name)); + assertTrue("user CAN NOT view!", user.canView(common)); + assertTrue("user CAN NOT clone!", user.canClone(common)); + assertFalse("user CAN push!", user.canPush(common)); + + assertFalse("user CAN create ref!", user.canCreateRef(common)); + assertFalse("user CAN delete ref!", user.canDeleteRef(common)); + assertFalse("user CAN rewind ref!", user.canRewindRef(common)); + + assertFalse("user CAN fork!", user.canFork(common)); + + assertFalse("user CAN delete!", user.canDelete(common)); + assertFalse("user CAN edit!", user.canEdit(common)); + + // personal + RepositoryModel personal = new RepositoryModel("~ubercool/_my-r/e~po.git", null, null, new Date()); + personal.authorizationControl = AuthorizationControl.NAMED; + personal.accessRestriction = AccessRestrictionType.VIEW; + + assertFalse("user HAS a repository permission!", user.hasRepositoryPermission(personal.name)); + assertFalse("user CAN NOT view!", user.canView(personal)); + assertFalse("user CAN NOT clone!", user.canClone(personal)); + assertFalse("user CAN push!", user.canPush(personal)); + + assertFalse("user CAN create ref!", user.canCreateRef(personal)); + assertFalse("user CAN delete ref!", user.canDeleteRef(personal)); + assertFalse("user CAN rewind ref!", user.canRewindRef(personal)); + + assertFalse("user CAN fork!", user.canFork(personal)); + + assertFalse("user CAN delete!", user.canDelete(personal)); + assertFalse("user CAN edit!", user.canEdit(personal)); + } + @Test + public void testRegexMatching2() throws Exception { + RepositoryModel personal = new RepositoryModel("~ubercool/_my-r/e~po.git", null, null, new Date()); + personal.authorizationControl = AuthorizationControl.NAMED; + personal.accessRestriction = AccessRestrictionType.VIEW; + + UserModel user = new UserModel("test"); + // permit all repositories excluding all personal rpeositories + user.setRepositoryPermission("[^~].*", AccessPermission.CLONE); + // permitall ~ubercool repositories + user.setRepositoryPermission("~ubercool/.*", AccessPermission.CLONE); + + // personal + assertTrue("user DOES NOT HAVE a repository permission!", user.hasRepositoryPermission(personal.name)); + assertTrue("user CAN NOT view!", user.canView(personal)); + assertTrue("user CAN NOT clone!", user.canClone(personal)); + assertFalse("user CAN push!", user.canPush(personal)); + + assertFalse("user CAN create ref!", user.canCreateRef(personal)); + assertFalse("user CAN delete ref!", user.canDeleteRef(personal)); + assertFalse("user CAN rewind ref!", user.canRewindRef(personal)); + + assertFalse("user CAN fork!", user.canFork(personal)); + + assertFalse("user CAN delete!", user.canDelete(personal)); + assertFalse("user CAN edit!", user.canEdit(personal)); + } + + @Test + public void testRegexOrder() throws Exception { + RepositoryModel personal = new RepositoryModel("~ubercool/_my-r/e~po.git", null, null, new Date()); + personal.authorizationControl = AuthorizationControl.NAMED; + personal.accessRestriction = AccessRestrictionType.VIEW; + + UserModel user = new UserModel("test"); + user.setRepositoryPermission(".*", AccessPermission.PUSH); + user.setRepositoryPermission("~ubercool/.*", AccessPermission.CLONE); + + // has PUSH access because first match is PUSH permission + assertTrue("user HAS a repository permission!", user.hasRepositoryPermission(personal.name)); + assertTrue("user CAN NOT view!", user.canView(personal)); + assertTrue("user CAN NOT clone!", user.canClone(personal)); + assertTrue("user CAN NOT push!", user.canPush(personal)); + + assertFalse("user CAN create ref!", user.canCreateRef(personal)); + assertFalse("user CAN delete ref!", user.canDeleteRef(personal)); + assertFalse("user CAN rewind ref!", user.canRewindRef(personal)); + + assertFalse("user CAN fork!", user.canFork(personal)); + + assertFalse("user CAN delete!", user.canDelete(personal)); + assertFalse("user CAN edit!", user.canEdit(personal)); + + user.permissions.clear(); + user.setRepositoryPermission("~ubercool/.*", AccessPermission.CLONE); + user.setRepositoryPermission(".*", AccessPermission.PUSH); + + // has CLONE access because first match is CLONE permission + assertTrue("user HAS a repository permission!", user.hasRepositoryPermission(personal.name)); + assertTrue("user CAN NOT view!", user.canView(personal)); + assertTrue("user CAN NOT clone!", user.canClone(personal)); + assertFalse("user CAN push!", user.canPush(personal)); + + assertFalse("user CAN create ref!", user.canCreateRef(personal)); + assertFalse("user CAN delete ref!", user.canDeleteRef(personal)); + assertFalse("user CAN rewind ref!", user.canRewindRef(personal)); + + assertFalse("user CAN fork!", user.canFork(personal)); + + assertFalse("user CAN delete!", user.canDelete(personal)); + assertFalse("user CAN edit!", user.canEdit(personal)); + } + @Test public void testAdminTeamInheritance() throws Exception { UserModel user = new UserModel("test"); -- 2.39.5