Fixed committer verification with merge commits (issue-264)

James Moger

2013-07-02 d1dc77b4b1599f7089e3184eb622f51035728da4

Fixed committer verification with merge commits (issue-264)

 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


 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);


 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;


 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 {