From d1dc77b4b1599f7089e3184eb622f51035728da4 Mon Sep 17 00:00:00 2001 From: James Moger Date: Tue, 2 Jul 2013 16:46:52 -0400 Subject: [PATCH] Fixed committer verification with merge commits (issue-264) --- releases.moxie | 1 + .../java/com/gitblit/git/ReceiveHook.java | 24 +++- .../java/com/gitblit/tests/GitBlitSuite.java | 2 +- .../com/gitblit/tests/GitServletTest.java | 117 ++++++++++++++++++ 4 files changed, 140 insertions(+), 4 deletions(-) diff --git a/releases.moxie b/releases.moxie index 910fddc5..5cfae90e 100644 --- a/releases.moxie +++ b/releases.moxie @@ -26,6 +26,7 @@ r17: { - Improve NPE handling for hook script enumeration (issue-253) - Workaround missing commit information in blame page (JGit bug 374382, issue-254) - Ignore orphan ".git" folder in the repositories root folder (issue-256) + - Fixed committer verification with merge commits (issue-264) - Could not reset settings with $ or { characters through Gitblit Manager because they are not properly escaped - Added more error checking to blob page and blame page - Disable SNI extensions for client SSL connections diff --git a/src/main/java/com/gitblit/git/ReceiveHook.java b/src/main/java/com/gitblit/git/ReceiveHook.java index e3435ffb..e0b77987 100644 --- a/src/main/java/com/gitblit/git/ReceiveHook.java +++ b/src/main/java/com/gitblit/git/ReceiveHook.java @@ -137,18 +137,36 @@ public class ReceiveHook implements PreReceiveHook, PostReceiveHook { // identity of the merging user. boolean allRejected = false; for (ReceiveCommand cmd : commands) { + String linearParent = null; try { List commits = JGitUtils.getRevLog(rp.getRepository(), cmd.getOldId().name(), cmd.getNewId().name()); for (RevCommit commit : commits) { + + if (linearParent != null) { + if (!commit.getName().equals(linearParent)) { + // ignore: commit is right-descendant of a merge + continue; + } + } + + // update expected next commit id + if (commit.getParentCount() == 0) { + linearParent = null; + } else { + linearParent = commit.getParents()[0].getId().getName(); + } + 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); + // account does not have an 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); + 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); } logger.warn(reason); cmd.setResult(Result.REJECTED_OTHER_REASON, reason); diff --git a/src/test/java/com/gitblit/tests/GitBlitSuite.java b/src/test/java/com/gitblit/tests/GitBlitSuite.java index a62220bb..dca158c2 100644 --- a/src/test/java/com/gitblit/tests/GitBlitSuite.java +++ b/src/test/java/com/gitblit/tests/GitBlitSuite.java @@ -132,7 +132,7 @@ public class GitBlitSuite { }); // Wait a few seconds for it to be running - Thread.sleep(2500); + Thread.sleep(5000); started.set(true); return true; diff --git a/src/test/java/com/gitblit/tests/GitServletTest.java b/src/test/java/com/gitblit/tests/GitServletTest.java index 8513c83d..b6a58ac9 100644 --- a/src/test/java/com/gitblit/tests/GitServletTest.java +++ b/src/test/java/com/gitblit/tests/GitServletTest.java @@ -16,6 +16,8 @@ import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.jgit.api.CloneCommand; import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.MergeCommand.FastForwardMode; +import org.eclipse.jgit.api.MergeResult; import org.eclipse.jgit.api.ResetCommand.ResetType; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.lib.Constants; @@ -63,6 +65,8 @@ public class GitServletTest { private static UserModel getUser() { UserModel user = new UserModel("james"); + user.displayName = "James Moger"; + user.emailAddress = "james.moger@gmail.com"; user.password = "james"; return user; } @@ -464,6 +468,119 @@ public class GitServletTest { // close serving repository GitBlitSuite.close(verification); } + + @Test + public void testMergeCommitterVerification() throws Exception { + + testMergeCommitterVerification(false); + + testMergeCommitterVerification(true); + } + + private void testMergeCommitterVerification(boolean expectedSuccess) throws Exception { + UserModel user = getUser(); + + delete(user); + + 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}/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}/{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); + + // checkout a mergetest branch + git.checkout().setCreateBranch(true).setName("mergetest").call(); + + // change identity + git.getRepository().getConfig().setString("user", null, "name", "mergetest"); + git.getRepository().getConfig().setString("user", null, "email", "mergetest@merge.com"); + git.getRepository().getConfig().save(); + + // commit a file + File file = new File(local, "MERGECHK2"); + 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(); + RevCommit mergeTip = git.commit().setMessage(file.getName() + " test").call(); + + // return to master + git.checkout().setName("master").call(); + + // restore identity + if (expectedSuccess) { + git.getRepository().getConfig().setString("user", null, "name", user.username); + git.getRepository().getConfig().setString("user", null, "email", user.emailAddress); + git.getRepository().getConfig().save(); + } + + // commit a file + file = new File(local, "MERGECHK1"); + os = new OutputStreamWriter(new FileOutputStream(file, true), Constants.CHARSET); + w = new BufferedWriter(os); + w.write("// " + new Date().toString() + "\n"); + w.close(); + git.add().addFilepattern(file.getName()).call(); + git.commit().setMessage(file.getName() + " test").call(); + + // merge the tip of the mergetest branch into master with --no-ff + MergeResult mergeResult = git.merge().setFastForward(FastForwardMode.NO_FF).include(mergeTip.getId()).call(); + assertEquals(MergeResult.MergeStatus.MERGED, mergeResult.getMergeStatus()); + + // push the merged master to the origin + 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 { -- 2.39.5