From efe8ecb216b0e2f2f1dceb26c4f21dcec1fb497c Mon Sep 17 00:00:00 2001 From: James Moger Date: Fri, 11 Nov 2011 17:59:15 -0500 Subject: [PATCH] Revised user access checks to account for repository ownership. Repository owners no longer have to be explicitly selected to grant them access to Git, feeds, and zip downloads. Idea from Github/dadalar. --- src/com/gitblit/AuthenticationFilter.java | 5 ++++- src/com/gitblit/DownloadZipFilter.java | 2 +- src/com/gitblit/GitBlit.java | 2 +- src/com/gitblit/GitFilter.java | 2 +- src/com/gitblit/SyndicationFilter.java | 2 +- src/com/gitblit/models/UserModel.java | 16 ++++++++++++++++ tests/com/gitblit/tests/GitBlitTest.java | 5 +++-- 7 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/com/gitblit/AuthenticationFilter.java b/src/com/gitblit/AuthenticationFilter.java index 277b220b..caa8a074 100644 --- a/src/com/gitblit/AuthenticationFilter.java +++ b/src/com/gitblit/AuthenticationFilter.java @@ -171,7 +171,7 @@ public abstract class AuthenticationFilter implements Filter { super(req); user = new UserModel("anonymous"); } - + UserModel getUser() { return user; } @@ -190,6 +190,9 @@ public abstract class AuthenticationFilter implements Filter { if (role.equals(Constants.ADMIN_ROLE)) { return user.canAdmin; } + // Gitblit does not currently use actual roles in the traditional + // servlet container sense. That is the reason this is marked + // deprecated, but I may want to revisit this. return user.canAccessRepository(role); } diff --git a/src/com/gitblit/DownloadZipFilter.java b/src/com/gitblit/DownloadZipFilter.java index 6145b125..c308cbbb 100644 --- a/src/com/gitblit/DownloadZipFilter.java +++ b/src/com/gitblit/DownloadZipFilter.java @@ -78,7 +78,7 @@ public class DownloadZipFilter extends AccessRestrictionFilter { */ @Override protected boolean canAccess(RepositoryModel repository, UserModel user, String action) { - return user.canAccessRepository(repository.name); + return user.canAccessRepository(repository); } } diff --git a/src/com/gitblit/GitBlit.java b/src/com/gitblit/GitBlit.java index 8db72af1..bc356676 100644 --- a/src/com/gitblit/GitBlit.java +++ b/src/com/gitblit/GitBlit.java @@ -555,7 +555,7 @@ public class GitBlit implements ServletContextListener { return null; } if (model.accessRestriction.atLeast(AccessRestrictionType.VIEW)) { - if (user != null && user.canAccessRepository(model.name)) { + if (user != null && user.canAccessRepository(model)) { return model; } return null; diff --git a/src/com/gitblit/GitFilter.java b/src/com/gitblit/GitFilter.java index 8127ffae..a7f0fe74 100644 --- a/src/com/gitblit/GitFilter.java +++ b/src/com/gitblit/GitFilter.java @@ -110,7 +110,7 @@ public class GitFilter extends AccessRestrictionFilter { } boolean readOnly = repository.isFrozen; if (readOnly || repository.accessRestriction.atLeast(AccessRestrictionType.PUSH)) { - boolean authorizedUser = user.canAccessRepository(repository.name); + boolean authorizedUser = user.canAccessRepository(repository); if (action.equals(gitReceivePack)) { // Push request if (!readOnly && authorizedUser) { diff --git a/src/com/gitblit/SyndicationFilter.java b/src/com/gitblit/SyndicationFilter.java index 9c7a8630..d6dd1f2d 100644 --- a/src/com/gitblit/SyndicationFilter.java +++ b/src/com/gitblit/SyndicationFilter.java @@ -76,7 +76,7 @@ public class SyndicationFilter extends AccessRestrictionFilter { */ @Override protected boolean canAccess(RepositoryModel repository, UserModel user, String action) { - return user.canAccessRepository(repository.name); + return user.canAccessRepository(repository); } } diff --git a/src/com/gitblit/models/UserModel.java b/src/com/gitblit/models/UserModel.java index fcf2b263..dadc44e7 100644 --- a/src/com/gitblit/models/UserModel.java +++ b/src/com/gitblit/models/UserModel.java @@ -20,6 +20,8 @@ import java.security.Principal; import java.util.HashSet; import java.util.Set; +import com.gitblit.utils.StringUtils; + /** * UserModel is a serializable model class that represents a user and the user's * restricted repository memberships. Instances of UserModels are also used as @@ -43,10 +45,24 @@ public class UserModel implements Principal, Serializable, Comparable this.username = username; } + /** + * This method does not take into consideration Ownership where the + * administrator has not explicitly granted access to the owner. + * + * @param repositoryName + * @return + */ + @Deprecated public boolean canAccessRepository(String repositoryName) { return canAdmin || repositories.contains(repositoryName.toLowerCase()); } + public boolean canAccessRepository(RepositoryModel repository) { + boolean isOwner = !StringUtils.isEmpty(repository.owner) + && repository.owner.equals(username); + return canAdmin || isOwner || repositories.contains(repository.name.toLowerCase()); + } + public void addRepository(String name) { repositories.add(name.toLowerCase()); } diff --git a/tests/com/gitblit/tests/GitBlitTest.java b/tests/com/gitblit/tests/GitBlitTest.java index 1d20ced9..669b25ac 100644 --- a/tests/com/gitblit/tests/GitBlitTest.java +++ b/tests/com/gitblit/tests/GitBlitTest.java @@ -52,9 +52,10 @@ public class GitBlitTest extends TestCase { model.canAdmin = false; assertFalse("Admin should not have #admin!", model.canAdmin); String repository = GitBlitSuite.getHelloworldRepository().getDirectory().getName(); - assertFalse("Admin can still access repository!", model.canAccessRepository(repository)); + RepositoryModel repositoryModel = GitBlit.self().getRepositoryModel(model, repository); + assertFalse("Admin can still access repository!", model.canAccessRepository(repositoryModel)); model.addRepository(repository); - assertTrue("Admin can't access repository!", model.canAccessRepository(repository)); + assertTrue("Admin can't access repository!", model.canAccessRepository(repositoryModel)); assertEquals(GitBlit.self().getRepositoryModel(model, "pretend"), null); assertNotNull(GitBlit.self().getRepositoryModel(model, repository)); assertTrue(GitBlit.self().getRepositoryModels(model).size() > 0); -- 2.39.5