Fixed committer verification with merge commits (issue-264)

diff --git a/releases.moxie b/releases.moxie
index 910fddc5e4e36a12cbccbed6c26ca5d4ac13da10..5cfae90e6eab1cb2063183ee9ca2579fbd1aff1d 100644 (file)
--- a/releases.moxie
+++ b/releases.moxie
@@ -26,6 +26,7 @@ r17: {
         - Improve NPE handling for hook script enumeration (issue-253)\r
         - Workaround missing commit information in blame page (JGit bug 374382, issue-254) \r
         - Ignore orphan ".git" folder in the repositories root folder (issue-256)\r
+        - Fixed committer verification with merge commits (issue-264)\r
      - Could not reset settings with $ or { characters through Gitblit Manager because they are not properly escaped\r
         - Added more error checking to blob page and blame page\r
         - Disable SNI extensions for client SSL connections\r

diff --git a/src/main/java/com/gitblit/git/ReceiveHook.java b/src/main/java/com/gitblit/git/ReceiveHook.java
index e3435ffb1aa7cb618aa1b1c9ccfc0891f78a68e1..e0b779879f38b99de0afc80ac19612ce7dede39d 100644 (file)
--- 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.\r
                        boolean allRejected = false;\r
                        for (ReceiveCommand cmd : commands) {\r
+                               String linearParent = null;\r
                                try {\r
                                        List<RevCommit> commits = JGitUtils.getRevLog(rp.getRepository(), cmd.getOldId().name(), cmd.getNewId().name());\r
                                        for (RevCommit commit : commits) {\r
+                                               \r
+                                               if (linearParent != null) {\r
+                                       if (!commit.getName().equals(linearParent)) {\r
+                                               // ignore: commit is right-descendant of a merge\r
+                                               continue;\r
+                                       }\r
+                               }\r
+                                               \r
+                                               // update expected next commit id\r
+                                               if (commit.getParentCount() == 0) {\r
+                                       linearParent = null;\r
+                                               } else {\r
+                                                       linearParent = commit.getParents()[0].getId().getName();\r
+                                               }\r
+                                               \r
                                                PersonIdent committer = commit.getCommitterIdent();\r
                                                if (!user.is(committer.getName(), committer.getEmailAddress())) {\r
                                                        String reason;\r
                                                        if (StringUtils.isEmpty(user.emailAddress)) {\r
-                                                               // account does not have en email address\r
-                                                               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);\r
+                                                               // account does not have an email address\r
+                                                               reason = MessageFormat.format("{0} by {1} <{2}> was not committed by {3} ({4})", \r
+                                                                               commit.getId().name(), committer.getName(), StringUtils.isEmpty(committer.getEmailAddress()) ? "?":committer.getEmailAddress(), user.getDisplayName(), user.username);\r
                                                        } else {\r
                                                                // account has an email address\r
-                                                               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);\r
+                                                               reason = MessageFormat.format("{0} by {1} <{2}> was not committed by {3} ({4}) <{5}>", \r
+                                                                               commit.getId().name(), committer.getName(), StringUtils.isEmpty(committer.getEmailAddress()) ? "?":committer.getEmailAddress(), user.getDisplayName(), user.username, user.emailAddress);\r
                                                        }\r
                                                        logger.warn(reason);\r
                                                        cmd.setResult(Result.REJECTED_OTHER_REASON, reason);\r

diff --git a/src/test/java/com/gitblit/tests/GitBlitSuite.java b/src/test/java/com/gitblit/tests/GitBlitSuite.java
index a62220bb97b9335beab4a5ad2bbbfdfb2421414c..dca158c25b1a000d6d783fa0afaf30947240e3ef 100644 (file)
--- 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 {
                });\r
 \r
                // Wait a few seconds for it to be running\r
-               Thread.sleep(2500);\r
+               Thread.sleep(5000);\r
 \r
                started.set(true);\r
                return true;\r

diff --git a/src/test/java/com/gitblit/tests/GitServletTest.java b/src/test/java/com/gitblit/tests/GitServletTest.java
index 8513c83d731fc432842205aafde3717e6655e55b..b6a58ac92092db320354eb2da17a4e08704c09cc 100644 (file)
--- 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;
 \r
 import org.eclipse.jgit.api.CloneCommand;\r
 import org.eclipse.jgit.api.Git;\r
+import org.eclipse.jgit.api.MergeCommand.FastForwardMode;\r
+import org.eclipse.jgit.api.MergeResult;\r
 import org.eclipse.jgit.api.ResetCommand.ResetType;\r
 import org.eclipse.jgit.api.errors.GitAPIException;\r
 import org.eclipse.jgit.lib.Constants;\r
@@ -63,6 +65,8 @@ public class GitServletTest {
        \r
        private static UserModel getUser() {\r
                UserModel user = new UserModel("james");\r
+               user.displayName = "James Moger";\r
+               user.emailAddress = "james.moger@gmail.com";\r
                user.password = "james";\r
                return user;\r
        }\r
@@ -464,6 +468,119 @@ public class GitServletTest {
                // close serving repository\r
                GitBlitSuite.close(verification);\r
        }\r
+       \r
+       @Test\r
+       public void testMergeCommitterVerification() throws Exception {\r
+               \r
+               testMergeCommitterVerification(false);\r
+               \r
+               testMergeCommitterVerification(true);\r
+       }\r
+       \r
+       private void testMergeCommitterVerification(boolean expectedSuccess) throws Exception {\r
+               UserModel user = getUser();\r
+               \r
+               delete(user);\r
+               \r
+               CredentialsProvider cp = new UsernamePasswordCredentialsProvider(user.username, user.password);\r
+               \r
+               // fork from original to a temporary bare repo\r
+               File verification = new File(GitBlitSuite.REPOSITORIES, "refchecks/verify-committer.git");\r
+               if (verification.exists()) {\r
+                       FileUtils.delete(verification, FileUtils.RECURSIVE);\r
+               }\r
+               CloneCommand clone = Git.cloneRepository();\r
+               clone.setURI(MessageFormat.format("{0}/ticgit.git", url));\r
+               clone.setDirectory(verification);\r
+               clone.setBare(true);\r
+               clone.setCloneAllBranches(true);\r
+               clone.setCredentialsProvider(cp);\r
+               GitBlitSuite.close(clone.call());\r
+               \r
+               // require push permissions and committer verification\r
+               RepositoryModel model = GitBlit.self().getRepositoryModel("refchecks/verify-committer.git");\r
+               model.authorizationControl = AuthorizationControl.NAMED;\r
+               model.accessRestriction = AccessRestrictionType.PUSH;\r
+               model.verifyCommitter = true;\r
+               \r
+               // grant user push permission\r
+               user.setRepositoryPermission(model.name, AccessPermission.PUSH);\r
+               \r
+               GitBlit.self().updateUserModel(user.username, user, true);\r
+               GitBlit.self().updateRepositoryModel(model.name, model, false);\r
+\r
+               // clone temp bare repo to working copy\r
+               File local = new File(GitBlitSuite.REPOSITORIES, "refchecks/verify-wc");\r
+               if (local.exists()) {\r
+                       FileUtils.delete(local, FileUtils.RECURSIVE);\r
+               }\r
+               clone = Git.cloneRepository();\r
+               clone.setURI(MessageFormat.format("{0}/{1}", url, model.name));\r
+               clone.setDirectory(local);\r
+               clone.setBare(false);\r
+               clone.setCloneAllBranches(true);\r
+               clone.setCredentialsProvider(cp);\r
+               GitBlitSuite.close(clone.call());\r
+               \r
+               Git git = Git.open(local);\r
+               \r
+               // checkout a mergetest branch\r
+               git.checkout().setCreateBranch(true).setName("mergetest").call();\r
+               \r
+               // change identity\r
+               git.getRepository().getConfig().setString("user", null, "name", "mergetest");\r
+               git.getRepository().getConfig().setString("user", null, "email", "mergetest@merge.com");\r
+               git.getRepository().getConfig().save();\r
+               \r
+               // commit a file\r
+               File file = new File(local, "MERGECHK2");\r
+               OutputStreamWriter os = new OutputStreamWriter(new FileOutputStream(file, true), Constants.CHARSET);\r
+               BufferedWriter w = new BufferedWriter(os);\r
+               w.write("// " + new Date().toString() + "\n");\r
+               w.close();\r
+               git.add().addFilepattern(file.getName()).call();\r
+               RevCommit mergeTip = git.commit().setMessage(file.getName() + " test").call();\r
+                               \r
+               // return to master\r
+               git.checkout().setName("master").call();\r
+\r
+               // restore identity\r
+               if (expectedSuccess) {\r
+                       git.getRepository().getConfig().setString("user", null, "name", user.username);\r
+                       git.getRepository().getConfig().setString("user", null, "email", user.emailAddress);\r
+                       git.getRepository().getConfig().save();\r
+               }\r
+\r
+               // commit a file\r
+               file = new File(local, "MERGECHK1");\r
+               os = new OutputStreamWriter(new FileOutputStream(file, true), Constants.CHARSET);\r
+               w = new BufferedWriter(os);\r
+               w.write("// " + new Date().toString() + "\n");\r
+               w.close();\r
+               git.add().addFilepattern(file.getName()).call();\r
+               git.commit().setMessage(file.getName() + " test").call();\r
+               \r
+               // merge the tip of the mergetest branch into master with --no-ff\r
+               MergeResult mergeResult = git.merge().setFastForward(FastForwardMode.NO_FF).include(mergeTip.getId()).call();\r
+               assertEquals(MergeResult.MergeStatus.MERGED, mergeResult.getMergeStatus());\r
+               \r
+               // push the merged master to the origin\r
+               Iterable<PushResult> results = git.push().setCredentialsProvider(cp).setRemote("origin").call();\r
+               \r
+               for (PushResult result : results) {\r
+                       RemoteRefUpdate ref = result.getRemoteUpdate("refs/heads/master");\r
+                       Status status = ref.getStatus();\r
+                       if (expectedSuccess) {\r
+                               assertTrue("Verification failed! User was NOT able to push commit! " + status.name(), Status.OK.equals(status));\r
+                       } else {\r
+                               assertTrue("Verification failed! User was able to push commit! " + status.name(), Status.REJECTED_OTHER_REASON.equals(status));\r
+                       }\r
+               }\r
+               \r
+               GitBlitSuite.close(git);\r
+               // close serving repository\r
+               GitBlitSuite.close(verification);\r
+       }\r
 \r
        @Test\r
        public void testBlockClone() throws Exception {\r