From: James Moger Date: Thu, 11 Oct 2012 22:11:50 +0000 (-0400) Subject: Experimental committer verification X-Git-Tag: v1.2.0~163 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=15640f86032169ad9bfef17c387b94f30a61582f;p=gitblit.git Experimental committer verification --- diff --git a/docs/01_setup.mkd b/docs/01_setup.mkd index 3123aa1f..a9ce767e 100644 --- a/docs/01_setup.mkd +++ b/docs/01_setup.mkd @@ -270,8 +270,41 @@ Gitblit also supports regex matching for repository permissions. The following #### No-So-Discrete Permissions (Gitblit <= v1.1.0) -Prior to v1.2.0, Gitblit had two main access permission groupings: -What you were permitted to do as an anonymous user and then **RW+** for any permitted user. +Prior to v1.2.0, Gitblit has two main access permission groupings: + +1. what you are permitted to do as an anonymous user +2. **RW+** for any permitted user + +#### Committer Verification + +Experimental + +You may optionally enable committer verification which requires that each commit be committed by the authenticated user pushing the commits. i.e. If Bob is pushing the commits, Bob **must** be the committer of those commits. + +**How is this enforced?** + +Bob must set his *user.name* and *user.email* values for the repository to match his Gitblit user account **BEFORE** committing to his repository. +
+[user "bob"]
+    displayName = Bob Jones
+    emailAddress = bob@somewhere.com
+
+
+    git config user.name "Bob Jones"
+    git config user.email bob@somewhere.com	
+
+or + + git config user.name bob + git config user.email bob@somewhere.com + +If the Gitblit account does not specify an email address, then the committer email address is ignored. However, if the account does specify an address it must match the committer's email address. Display name or username can be used as the committer name. + +All checks are case-insensitive. + +**What about merges?** + +You can not use fast-forward merges on your client when using committer verification. You must specify *--no-ff* to ensure that a merge commit is created with your identity as the committer. Only the first parent chain is traversed when verifying commits. ### Teams diff --git a/docs/04_releases.mkd b/docs/04_releases.mkd index aad6463f..a26b2c8a 100644 --- a/docs/04_releases.mkd +++ b/docs/04_releases.mkd @@ -41,6 +41,7 @@ In order to fork a repository, the user account must have the *fork* permission - Delete branch feature (issue 121, Github/ajermakovics) - Added line links to blob view (issue 130) - Added RedmineUserService (github/mallowlabs) +- Experimental support for committer verification. Requires use of *--no-ff* when merging branches or pull requests. See setup page for details. #### changes diff --git a/src/com/gitblit/GitBlit.java b/src/com/gitblit/GitBlit.java index bf9b1ed1..36f5f35c 100644 --- a/src/com/gitblit/GitBlit.java +++ b/src/com/gitblit/GitBlit.java @@ -1257,6 +1257,7 @@ public class GitBlit implements ServletContextListener { "accessRestriction", settings.getString(Keys.git.defaultAccessRestriction, null))); model.authorizationControl = AuthorizationControl.fromName(getConfig(config, "authorizationControl", settings.getString(Keys.git.defaultAuthorizationControl, null))); + model.verifyCommitter = getConfig(config, "verifyCommitter", false); model.showRemoteBranches = getConfig(config, "showRemoteBranches", hasOrigin); model.isFrozen = getConfig(config, "isFrozen", false); model.showReadme = getConfig(config, "showReadme", false); @@ -1698,6 +1699,7 @@ public class GitBlit implements ServletContextListener { config.setBoolean(Constants.CONFIG_GITBLIT, null, "allowForks", repository.allowForks); config.setString(Constants.CONFIG_GITBLIT, null, "accessRestriction", repository.accessRestriction.name()); config.setString(Constants.CONFIG_GITBLIT, null, "authorizationControl", repository.authorizationControl.name()); + config.setBoolean(Constants.CONFIG_GITBLIT, null, "verifyCommitter", repository.verifyCommitter); config.setBoolean(Constants.CONFIG_GITBLIT, null, "showRemoteBranches", repository.showRemoteBranches); config.setBoolean(Constants.CONFIG_GITBLIT, null, "isFrozen", repository.isFrozen); config.setBoolean(Constants.CONFIG_GITBLIT, null, "showReadme", repository.showReadme); diff --git a/src/com/gitblit/GitServlet.java b/src/com/gitblit/GitServlet.java index 8e2326d4..42d88c91 100644 --- a/src/com/gitblit/GitServlet.java +++ b/src/com/gitblit/GitServlet.java @@ -28,6 +28,7 @@ import java.text.MessageFormat; import java.util.Collection; import java.util.Enumeration; import java.util.LinkedHashSet; +import java.util.List; import java.util.Set; import javax.servlet.ServletConfig; @@ -38,6 +39,7 @@ import javax.servlet.http.HttpServletRequest; import org.eclipse.jgit.http.server.resolver.DefaultReceivePackFactory; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.transport.PostReceiveHook; import org.eclipse.jgit.transport.PreReceiveHook; import org.eclipse.jgit.transport.ReceiveCommand; @@ -48,10 +50,12 @@ import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.gitblit.Constants.AccessRestrictionType; import com.gitblit.models.RepositoryModel; import com.gitblit.models.UserModel; import com.gitblit.utils.ClientLogger; import com.gitblit.utils.HttpUtils; +import com.gitblit.utils.JGitUtils; import com.gitblit.utils.StringUtils; /** @@ -191,10 +195,48 @@ public class GitServlet extends org.eclipse.jgit.http.server.GitServlet { @Override public void onPreReceive(ReceivePack rp, Collection commands) { RepositoryModel repository = GitBlit.self().getRepositoryModel(repositoryName); + UserModel user = getUserModel(rp); + + if (repository.accessRestriction.atLeast(AccessRestrictionType.PUSH) && repository.verifyCommitter) { + if (StringUtils.isEmpty(user.emailAddress)) { + // emit warning if user does not have an email address + logger.warn(MessageFormat.format("Consider setting an email address for {0} ({1}) to improve committer verification.", user.getDisplayName(), user.username)); + } + + // Optionally enforce that the committer of the left parent chain + // match the account being used to push the commits. + // + // This requires all merge commits are executed with the "--no-ff" + // option to force a merge commit even if fast-forward is possible. + // This ensures that the chain of left parents has the commit + // identity of the merging user. + for (ReceiveCommand cmd : commands) { + try { + List commits = JGitUtils.getRevLog(rp.getRepository(), cmd.getOldId().name(), cmd.getNewId().name()); + for (RevCommit commit : commits) { + PersonIdent committer = commit.getCommitterIdent(); + if (!user.is(committer.getName(), committer.getEmailAddress())) { + String reason; + if (StringUtils.isEmpty(user.emailAddress)) { + // account does not have en email address + reason = MessageFormat.format("{0} by {1} <{2}> was not committed by {3} ({4})", commit.getId().name(), committer.getName(), StringUtils.isEmpty(committer.getEmailAddress()) ? "?":committer.getEmailAddress(), user.getDisplayName(), user.username); + } else { + // account has an email address + reason = MessageFormat.format("{0} by {1} <{2}> was not committed by {3} ({4}) <{5}>", commit.getId().name(), committer.getName(), StringUtils.isEmpty(committer.getEmailAddress()) ? "?":committer.getEmailAddress(), user.getDisplayName(), user.username, user.emailAddress); + } + cmd.setResult(Result.REJECTED_OTHER_REASON, reason); + break; + } + } + } catch (Exception e) { + logger.error("Failed to verify commits were made by pushing user", e); + } + } + } + Set scripts = new LinkedHashSet(); scripts.addAll(GitBlit.self().getPreReceiveScriptsInherited(repository)); scripts.addAll(repository.preReceiveScripts); - UserModel user = getUserModel(rp); runGroovy(repository, user, commands, rp, scripts); for (ReceiveCommand cmd : commands) { if (!Result.NOT_ATTEMPTED.equals(cmd.getResult())) { diff --git a/src/com/gitblit/models/RepositoryModel.java b/src/com/gitblit/models/RepositoryModel.java index 914523df..502f886e 100644 --- a/src/com/gitblit/models/RepositoryModel.java +++ b/src/com/gitblit/models/RepositoryModel.java @@ -75,6 +75,7 @@ public class RepositoryModel implements Serializable, Comparable forks; public String originRepository; + public boolean verifyCommitter; public RepositoryModel() { this("", "", "", new Date(0)); diff --git a/src/com/gitblit/models/UserModel.java b/src/com/gitblit/models/UserModel.java index 6fe8df2b..8b3fe826 100644 --- a/src/com/gitblit/models/UserModel.java +++ b/src/com/gitblit/models/UserModel.java @@ -405,4 +405,32 @@ public class UserModel implements Principal, Serializable, Comparable public int compareTo(UserModel o) { return username.compareTo(o.username); } + + /** + * Returns true if the name/email pair match this user account. + * + * @param name + * @param email + * @return true, if the name and email address match this account + */ + public boolean is(String name, String email) { + // at a minimum a usename or display name must be supplied + if (StringUtils.isEmpty(name)) { + return false; + } + boolean nameVerified = name.equalsIgnoreCase(username) || name.equalsIgnoreCase(getDisplayName()); + boolean emailVerified = false; + if (StringUtils.isEmpty(emailAddress)) { + // user account has not specified an email address + // rely on username/displayname verification + emailVerified = true; + } else { + // user account has specified an email address + // require email address verification + if (!StringUtils.isEmpty(email)) { + emailVerified = email.equalsIgnoreCase(emailAddress); + } + } + return nameVerified && emailVerified; + } } diff --git a/src/com/gitblit/wicket/GitBlitWebApp.properties b/src/com/gitblit/wicket/GitBlitWebApp.properties index d18dcf11..e786d311 100644 --- a/src/com/gitblit/wicket/GitBlitWebApp.properties +++ b/src/com/gitblit/wicket/GitBlitWebApp.properties @@ -341,4 +341,6 @@ gb.preparingFork = preparing your fork... gb.isFork = is fork gb.canCreate = can create gb.canCreateDescription = can create personal repositories -gb.illegalPersonalRepositoryLocation = your personal repository must be located at \"{0}\" \ No newline at end of file +gb.illegalPersonalRepositoryLocation = your personal repository must be located at \"{0}\" +gb.verifyCommitter = verify committer +gb.verifyCommitterDescription = require committer identity to match pushing Gitblt user account (all merges require "--no-ff" to enforce committer identity) \ No newline at end of file diff --git a/src/com/gitblit/wicket/pages/EditRepositoryPage.html b/src/com/gitblit/wicket/pages/EditRepositoryPage.html index 5869917f..20b77e52 100644 --- a/src/com/gitblit/wicket/pages/EditRepositoryPage.html +++ b/src/com/gitblit/wicket/pages/EditRepositoryPage.html @@ -35,11 +35,12 @@
+

 

-

 

diff --git a/src/com/gitblit/wicket/pages/EditRepositoryPage.java b/src/com/gitblit/wicket/pages/EditRepositoryPage.java index 8176c28b..a2dad104 100644 --- a/src/com/gitblit/wicket/pages/EditRepositoryPage.java +++ b/src/com/gitblit/wicket/pages/EditRepositoryPage.java @@ -406,7 +406,7 @@ public class EditRepositoryPage extends RootSubPage { form.add(new DropDownChoice("federationStrategy", federationStrategies, new FederationTypeRenderer())); form.add(new CheckBox("useTickets")); - form.add(new CheckBox("useDocs")); + form.add(new CheckBox("useDocs")); form.add(new CheckBox("showRemoteBranches")); form.add(new CheckBox("showReadme")); form.add(new CheckBox("skipSizeCalculation")); @@ -423,6 +423,8 @@ public class EditRepositoryPage extends RootSubPage { group.add(allowNamed); form.add(group); + form.add(new CheckBox("verifyCommitter")); + form.add(usersPalette); form.add(teamsPalette); form.add(federationSetsPalette); diff --git a/tests/com/gitblit/tests/GitBlitTest.java b/tests/com/gitblit/tests/GitBlitTest.java index a188f180..1c9bbd00 100644 --- a/tests/com/gitblit/tests/GitBlitTest.java +++ b/tests/com/gitblit/tests/GitBlitTest.java @@ -68,6 +68,38 @@ public class GitBlitTest { assertNotNull(GitBlit.self().getRepositoryModel(user, repository)); assertTrue(GitBlit.self().getRepositoryModels(user).size() > 0); } + + @Test + public void testUserModelVerification() throws Exception { + UserModel user = new UserModel("james"); + user.displayName = "James Moger"; + + assertTrue(user.is("James", null)); + assertTrue(user.is("James", "")); + assertTrue(user.is("JaMeS", "anything")); + + assertTrue(user.is("james moger", null)); + assertTrue(user.is("james moger", "")); + assertTrue(user.is("james moger", "anything")); + + assertFalse(user.is("joe", null)); + assertFalse(user.is("joe", "")); + assertFalse(user.is("joe", "anything")); + + // specify email address which results in address verification + user.emailAddress = "something"; + + assertFalse(user.is("James", null)); + assertFalse(user.is("James", "")); + assertFalse(user.is("JaMeS", "anything")); + + assertFalse(user.is("james moger", null)); + assertFalse(user.is("james moger", "")); + assertFalse(user.is("james moger", "anything")); + + assertTrue(user.is("JaMeS", user.emailAddress)); + assertTrue(user.is("JaMeS mOgEr", user.emailAddress)); + } @Test public void testAccessRestrictionTypes() throws Exception { diff --git a/tests/com/gitblit/tests/GitServletTest.java b/tests/com/gitblit/tests/GitServletTest.java index 52dddc40..4342386e 100644 --- a/tests/com/gitblit/tests/GitServletTest.java +++ b/tests/com/gitblit/tests/GitServletTest.java @@ -248,6 +248,112 @@ public class GitServletTest { GitBlitSuite.close(git); } + @Test + public void testCommitterVerification() throws Exception { + UserModel user = new UserModel("james"); + user.password = "james"; + + // account only uses account name to verify + testCommitterVerification(user, user.username, null, true); + // committer email address is ignored because account does not specify email + testCommitterVerification(user, user.username, "something", true); + // completely different committer + testCommitterVerification(user, "joe", null, false); + + // test display name verification + user.displayName = "James Moger"; + testCommitterVerification(user, user.displayName, null, true); + testCommitterVerification(user, user.displayName, "something", true); + testCommitterVerification(user, "joe", null, false); + + // test email address verification + user.emailAddress = "something"; + testCommitterVerification(user, user.displayName, null, false); + testCommitterVerification(user, user.displayName, "somethingelse", false); + testCommitterVerification(user, user.displayName, user.emailAddress, true); + + // use same email address but with different committer + testCommitterVerification(user, "joe", "somethingelse", false); + } + + private void testCommitterVerification(UserModel user, String displayName, String emailAddress, boolean expectedSuccess) throws Exception { + + if (GitBlit.self().getUserModel(user.username) != null) { + GitBlit.self().deleteUser(user.username); + } + + CredentialsProvider cp = new UsernamePasswordCredentialsProvider(user.username, user.password); + + // fork from original to a temporary bare repo + File verification = new File(GitBlitSuite.REPOSITORIES, "refchecks/verify-committer.git"); + if (verification.exists()) { + FileUtils.delete(verification, FileUtils.RECURSIVE); + } + CloneCommand clone = Git.cloneRepository(); + clone.setURI(MessageFormat.format("{0}/git/ticgit.git", url)); + clone.setDirectory(verification); + clone.setBare(true); + clone.setCloneAllBranches(true); + clone.setCredentialsProvider(cp); + GitBlitSuite.close(clone.call()); + + // require push permissions and committer verification + RepositoryModel model = GitBlit.self().getRepositoryModel("refchecks/verify-committer.git"); + model.authorizationControl = AuthorizationControl.NAMED; + model.accessRestriction = AccessRestrictionType.PUSH; + model.verifyCommitter = true; + + // grant user push permission + user.setRepositoryPermission(model.name, AccessPermission.PUSH); + + GitBlit.self().updateUserModel(user.username, user, true); + GitBlit.self().updateRepositoryModel(model.name, model, false); + + // clone temp bare repo to working copy + File local = new File(GitBlitSuite.REPOSITORIES, "refchecks/verify-wc"); + if (local.exists()) { + FileUtils.delete(local, FileUtils.RECURSIVE); + } + clone = Git.cloneRepository(); + clone.setURI(MessageFormat.format("{0}/git/{1}", url, model.name)); + clone.setDirectory(local); + clone.setBare(false); + clone.setCloneAllBranches(true); + clone.setCredentialsProvider(cp); + GitBlitSuite.close(clone.call()); + + Git git = Git.open(local); + + // force an identity which may or may not match the account's identity + git.getRepository().getConfig().setString("user", null, "name", displayName); + git.getRepository().getConfig().setString("user", null, "email", emailAddress); + git.getRepository().getConfig().save(); + + // commit a file and push it + File file = new File(local, "PUSHCHK"); + OutputStreamWriter os = new OutputStreamWriter(new FileOutputStream(file, true), Constants.CHARSET); + BufferedWriter w = new BufferedWriter(os); + w.write("// " + new Date().toString() + "\n"); + w.close(); + git.add().addFilepattern(file.getName()).call(); + git.commit().setMessage("push test").call(); + Iterable results = git.push().setCredentialsProvider(cp).setRemote("origin").call(); + + for (PushResult result : results) { + RemoteRefUpdate ref = result.getRemoteUpdate("refs/heads/master"); + Status status = ref.getStatus(); + if (expectedSuccess) { + assertTrue("Verification failed! User was NOT able to push commit! " + status.name(), Status.OK.equals(status)); + } else { + assertTrue("Verification failed! User was able to push commit! " + status.name(), Status.REJECTED_OTHER_REASON.equals(status)); + } + } + + GitBlitSuite.close(git); + // close serving repository + GitBlitSuite.close(verification); + } + @Test public void testBlockClone() throws Exception { testRefChange(AccessPermission.VIEW, null, null, null);