From d1dc77b4b1599f7089e3184eb622f51035728da4 Mon Sep 17 00:00:00 2001
From: James Moger <james.moger@gitblit.com>
Date: Tue, 02 Jul 2013 16:46:52 -0400
Subject: [PATCH] Fixed committer verification with merge commits (issue-264)
---
src/test/java/com/gitblit/tests/GitBlitSuite.java | 2
releases.moxie | 1
src/main/java/com/gitblit/git/ReceiveHook.java | 24 +++++++-
src/test/java/com/gitblit/tests/GitServletTest.java | 117 +++++++++++++++++++++++++++++++++++++++
4 files changed, 140 insertions(+), 4 deletions(-)
diff --git a/releases.moxie b/releases.moxie
index 910fddc..5cfae90 100644
--- a/releases.moxie
+++ b/releases.moxie
@@ -26,6 +26,7 @@
- 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 e3435ff..e0b7798 100644
--- a/src/main/java/com/gitblit/git/ReceiveHook.java
+++ b/src/main/java/com/gitblit/git/ReceiveHook.java
@@ -137,18 +137,36 @@
// identity of the merging user.
boolean allRejected = false;
for (ReceiveCommand cmd : commands) {
+ String linearParent = null;
try {
List<RevCommit> 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 a62220b..dca158c 100644
--- a/src/test/java/com/gitblit/tests/GitBlitSuite.java
+++ b/src/test/java/com/gitblit/tests/GitBlitSuite.java
@@ -132,7 +132,7 @@
});
// 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 8513c83..b6a58ac 100644
--- a/src/test/java/com/gitblit/tests/GitServletTest.java
+++ b/src/test/java/com/gitblit/tests/GitServletTest.java
@@ -16,6 +16,8 @@
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 @@
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 @@
// 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<PushResult> 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 {
--
Gitblit v1.9.1