mirror of
https://github.com/scm-manager/scm-manager.git
synced 2025-11-02 19:45:51 +01:00
resolve review findings
This commit is contained in:
@@ -219,7 +219,7 @@ class AbstractGitCommand {
|
||||
}
|
||||
}
|
||||
|
||||
Optional<RevCommit> doCommit(String message, Person author, boolean signingDisabled) {
|
||||
Optional<RevCommit> doCommit(String message, Person author, boolean sign) {
|
||||
Person authorToUse = determineAuthor(author);
|
||||
try {
|
||||
Status status = clone.status().call();
|
||||
@@ -228,8 +228,8 @@ class AbstractGitCommand {
|
||||
.setAuthor(authorToUse.getName(), authorToUse.getMail())
|
||||
.setCommitter("SCM-Manager", "noreply@scm-manager.org")
|
||||
.setMessage(message)
|
||||
.setSign(!signingDisabled)
|
||||
.setSigningKey(signingDisabled ? null : "SCM-MANAGER-DEFAULT-KEY")
|
||||
.setSign(sign)
|
||||
.setSigningKey(sign ? "SCM-MANAGER-DEFAULT-KEY" : null)
|
||||
.call());
|
||||
} else {
|
||||
return empty();
|
||||
|
||||
@@ -56,7 +56,7 @@ abstract class GitMergeStrategy extends AbstractGitCommand.GitCloneWorker<MergeC
|
||||
private final ObjectId revisionToMerge;
|
||||
private final Person author;
|
||||
private final String messageTemplate;
|
||||
private final boolean signingDisabled;
|
||||
private final boolean sign;
|
||||
|
||||
GitMergeStrategy(Git clone, MergeCommandRequest request, GitContext context, sonia.scm.repository.Repository repository) {
|
||||
super(clone, context, repository);
|
||||
@@ -64,7 +64,7 @@ abstract class GitMergeStrategy extends AbstractGitCommand.GitCloneWorker<MergeC
|
||||
this.branchToMerge = request.getBranchToMerge();
|
||||
this.author = request.getAuthor();
|
||||
this.messageTemplate = request.getMessageTemplate();
|
||||
this.signingDisabled = request.isSigningDisabled();
|
||||
this.sign = request.isSign();
|
||||
try {
|
||||
this.targetRevision = resolveRevision(request.getTargetBranch());
|
||||
this.revisionToMerge = resolveRevision(request.getBranchToMerge());
|
||||
@@ -90,7 +90,7 @@ abstract class GitMergeStrategy extends AbstractGitCommand.GitCloneWorker<MergeC
|
||||
|
||||
Optional<RevCommit> doCommit() {
|
||||
logger.debug("merged branch {} into {}", branchToMerge, targetBranch);
|
||||
return doCommit(MessageFormat.format(determineMessageTemplate(), branchToMerge, targetBranch), author, signingDisabled);
|
||||
return doCommit(MessageFormat.format(determineMessageTemplate(), branchToMerge, targetBranch), author, sign);
|
||||
}
|
||||
|
||||
MergeCommandResult createSuccessResult(String newRevision) {
|
||||
|
||||
@@ -38,7 +38,6 @@ import sonia.scm.repository.GitRepositoryHandler;
|
||||
import sonia.scm.repository.GitWorkingCopyFactory;
|
||||
import sonia.scm.repository.InternalRepositoryException;
|
||||
import sonia.scm.repository.Repository;
|
||||
import sonia.scm.security.GPG;
|
||||
import sonia.scm.web.lfs.LfsBlobStoreFactory;
|
||||
|
||||
import javax.inject.Inject;
|
||||
@@ -94,7 +93,7 @@ public class GitModifyCommand extends AbstractGitCommand implements ModifyComman
|
||||
r.execute(this);
|
||||
}
|
||||
failIfNotChanged(() -> new NoChangesMadeException(repository, ModifyWorker.this.request.getBranch()));
|
||||
Optional<RevCommit> revCommit = doCommit(request.getCommitMessage(), request.getAuthor(), request.isSigningDisabled());
|
||||
Optional<RevCommit> revCommit = doCommit(request.getCommitMessage(), request.getAuthor(), request.isSign());
|
||||
push();
|
||||
return revCommit.orElseThrow(() -> new NoChangesMadeException(repository, ModifyWorker.this.request.getBranch())).name();
|
||||
}
|
||||
|
||||
@@ -24,13 +24,18 @@
|
||||
|
||||
package sonia.scm.repository;
|
||||
|
||||
import org.eclipse.jgit.api.errors.CanceledException;
|
||||
import org.eclipse.jgit.lib.CommitBuilder;
|
||||
import org.eclipse.jgit.lib.GpgSignature;
|
||||
import org.eclipse.jgit.lib.GpgSigner;
|
||||
import org.eclipse.jgit.lib.PersonIdent;
|
||||
import org.eclipse.jgit.transport.CredentialsProvider;
|
||||
import sonia.scm.security.GPG;
|
||||
import sonia.scm.security.PrivateKey;
|
||||
import sonia.scm.security.PublicKey;
|
||||
|
||||
import java.util.Collections;
|
||||
import java.util.Optional;
|
||||
import java.util.Set;
|
||||
|
||||
public final class GitTestHelper {
|
||||
|
||||
@@ -41,6 +46,25 @@ public final class GitTestHelper {
|
||||
return new GitChangesetConverterFactory(new NoopGPG());
|
||||
}
|
||||
|
||||
public static class SimpleGpgSigner extends GpgSigner {
|
||||
|
||||
public static byte[] getSignature() {
|
||||
return "SIGNATURE".getBytes();
|
||||
}
|
||||
|
||||
@Override
|
||||
public void sign(CommitBuilder commitBuilder, String s, PersonIdent personIdent, CredentialsProvider
|
||||
credentialsProvider) throws CanceledException {
|
||||
commitBuilder.setGpgSignature(new GpgSignature(SimpleGpgSigner.getSignature()));
|
||||
}
|
||||
|
||||
@Override
|
||||
public boolean canLocateSigningKey(String s, PersonIdent personIdent, CredentialsProvider credentialsProvider) throws CanceledException {
|
||||
return true;
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
private static class NoopGPG implements GPG {
|
||||
|
||||
@Override
|
||||
|
||||
@@ -29,17 +29,24 @@ import com.github.sdorra.shiro.SubjectAware;
|
||||
import org.apache.shiro.subject.SimplePrincipalCollection;
|
||||
import org.apache.shiro.subject.Subject;
|
||||
import org.eclipse.jgit.api.Git;
|
||||
import org.eclipse.jgit.api.errors.CanceledException;
|
||||
import org.eclipse.jgit.api.errors.GitAPIException;
|
||||
import org.eclipse.jgit.lib.CommitBuilder;
|
||||
import org.eclipse.jgit.lib.GpgSignature;
|
||||
import org.eclipse.jgit.lib.GpgSigner;
|
||||
import org.eclipse.jgit.lib.ObjectId;
|
||||
import org.eclipse.jgit.lib.PersonIdent;
|
||||
import org.eclipse.jgit.lib.Repository;
|
||||
import org.eclipse.jgit.revwalk.RevCommit;
|
||||
import org.eclipse.jgit.transport.CredentialsProvider;
|
||||
import org.junit.BeforeClass;
|
||||
import org.junit.Rule;
|
||||
import org.junit.Test;
|
||||
import org.junit.jupiter.api.Assertions;
|
||||
import sonia.scm.NoChangesMadeException;
|
||||
import sonia.scm.NotFoundException;
|
||||
import sonia.scm.repository.Added;
|
||||
import sonia.scm.repository.GitTestHelper;
|
||||
import sonia.scm.repository.GitWorkingCopyFactory;
|
||||
import sonia.scm.repository.Person;
|
||||
import sonia.scm.repository.api.MergeCommandResult;
|
||||
@@ -68,6 +75,11 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
|
||||
@Rule
|
||||
public BindTransportProtocolRule transportProtocolRule = new BindTransportProtocolRule();
|
||||
|
||||
@BeforeClass
|
||||
public static void setSigner() {
|
||||
GpgSigner.setDefault(new GitTestHelper.SimpleGpgSigner());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void shouldDetectMergeableBranches() {
|
||||
GitMergeCommand command = createCommand();
|
||||
@@ -419,6 +431,48 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
|
||||
command.dryRun(request);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void shouldSignMergeCommit() throws IOException, GitAPIException {
|
||||
GitMergeCommand command = createCommand();
|
||||
MergeCommandRequest request = new MergeCommandRequest();
|
||||
request.setTargetBranch("master");
|
||||
request.setBranchToMerge("empty_merge");
|
||||
request.setMergeStrategy(MergeStrategy.MERGE_COMMIT);
|
||||
request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det"));
|
||||
|
||||
MergeCommandResult mergeCommandResult = command.merge(request);
|
||||
|
||||
assertThat(mergeCommandResult.isSuccess()).isTrue();
|
||||
|
||||
Repository repository = createContext().open();
|
||||
Iterable<RevCommit> commits = new Git(repository).log().add(repository.resolve("master")).setMaxCount(1).call();
|
||||
RevCommit mergeCommit = commits.iterator().next();
|
||||
assertThat(mergeCommit.getRawGpgSignature()).isNotEmpty();
|
||||
assertThat(mergeCommit.getRawGpgSignature()).isEqualTo(GitTestHelper.SimpleGpgSigner.getSignature());
|
||||
|
||||
}
|
||||
|
||||
@Test
|
||||
public void shouldNotSignMergeCommitIfSigningIsDisabled() throws IOException, GitAPIException {
|
||||
GitMergeCommand command = createCommand();
|
||||
MergeCommandRequest request = new MergeCommandRequest();
|
||||
request.setTargetBranch("master");
|
||||
request.setBranchToMerge("empty_merge");
|
||||
request.setMergeStrategy(MergeStrategy.MERGE_COMMIT);
|
||||
request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det"));
|
||||
request.setSign(false);
|
||||
|
||||
MergeCommandResult mergeCommandResult = command.merge(request);
|
||||
|
||||
assertThat(mergeCommandResult.isSuccess()).isTrue();
|
||||
|
||||
Repository repository = createContext().open();
|
||||
Iterable<RevCommit> commits = new Git(repository).log().add(repository.resolve("master")).setMaxCount(1).call();
|
||||
RevCommit mergeCommit = commits.iterator().next();
|
||||
assertThat(mergeCommit.getRawGpgSignature()).isNullOrEmpty();
|
||||
|
||||
}
|
||||
|
||||
private GitMergeCommand createCommand() {
|
||||
return createCommand(git -> {
|
||||
});
|
||||
|
||||
@@ -27,23 +27,33 @@ package sonia.scm.repository.spi;
|
||||
import com.github.sdorra.shiro.ShiroRule;
|
||||
import com.github.sdorra.shiro.SubjectAware;
|
||||
import org.eclipse.jgit.api.Git;
|
||||
import org.eclipse.jgit.api.errors.CanceledException;
|
||||
import org.eclipse.jgit.api.errors.GitAPIException;
|
||||
import org.eclipse.jgit.errors.CorruptObjectException;
|
||||
import org.eclipse.jgit.lib.CommitBuilder;
|
||||
import org.eclipse.jgit.lib.GpgSignature;
|
||||
import org.eclipse.jgit.lib.GpgSigner;
|
||||
import org.eclipse.jgit.lib.ObjectId;
|
||||
import org.eclipse.jgit.lib.ObjectReader;
|
||||
import org.eclipse.jgit.lib.PersonIdent;
|
||||
import org.eclipse.jgit.revwalk.RevCommit;
|
||||
import org.eclipse.jgit.revwalk.RevWalk;
|
||||
import org.eclipse.jgit.transport.CredentialsProvider;
|
||||
import org.eclipse.jgit.treewalk.CanonicalTreeParser;
|
||||
import org.junit.BeforeClass;
|
||||
import org.junit.Rule;
|
||||
import org.junit.Test;
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.rules.TemporaryFolder;
|
||||
import sonia.scm.AlreadyExistsException;
|
||||
import sonia.scm.BadRequestException;
|
||||
import sonia.scm.ConcurrentModificationException;
|
||||
import sonia.scm.NotFoundException;
|
||||
import sonia.scm.repository.GitTestHelper;
|
||||
import sonia.scm.repository.Person;
|
||||
import sonia.scm.repository.work.NoneCachingWorkingCopyPool;
|
||||
import sonia.scm.repository.work.WorkdirProvider;
|
||||
import sonia.scm.security.PublicKey;
|
||||
import sonia.scm.web.lfs.LfsBlobStoreFactory;
|
||||
|
||||
import java.io.File;
|
||||
@@ -65,6 +75,11 @@ public class GitModifyCommandTest extends AbstractGitCommandTestBase {
|
||||
|
||||
private final LfsBlobStoreFactory lfsBlobStoreFactory = mock(LfsBlobStoreFactory.class);
|
||||
|
||||
@BeforeClass
|
||||
public static void setSigner() {
|
||||
GpgSigner.setDefault(new GitTestHelper.SimpleGpgSigner());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void shouldCreateCommit() throws IOException, GitAPIException {
|
||||
File newFile = Files.write(temporaryFolder.newFile().toPath(), "new content".getBytes()).toFile();
|
||||
@@ -306,6 +321,48 @@ public class GitModifyCommandTest extends AbstractGitCommandTestBase {
|
||||
command.execute(request);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void shouldSignCreatedCommit() throws IOException, GitAPIException {
|
||||
File newFile = Files.write(temporaryFolder.newFile().toPath(), "new content".getBytes()).toFile();
|
||||
|
||||
GitModifyCommand command = createCommand();
|
||||
|
||||
ModifyCommandRequest request = new ModifyCommandRequest();
|
||||
request.setCommitMessage("test commit");
|
||||
request.addRequest(new ModifyCommandRequest.CreateFileRequest("new_file", newFile, false));
|
||||
request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det"));
|
||||
|
||||
command.execute(request);
|
||||
|
||||
try (Git git = new Git(createContext().open())) {
|
||||
|
||||
RevCommit lastCommit = getLastCommit(git);
|
||||
assertThat(lastCommit.getRawGpgSignature()).isNotEmpty();
|
||||
assertThat(lastCommit.getRawGpgSignature()).isEqualTo(GitTestHelper.SimpleGpgSigner.getSignature());
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void shouldNotSignCreatedCommitIfSigningDisabled() throws IOException, GitAPIException {
|
||||
File newFile = Files.write(temporaryFolder.newFile().toPath(), "new content".getBytes()).toFile();
|
||||
|
||||
GitModifyCommand command = createCommand();
|
||||
|
||||
ModifyCommandRequest request = new ModifyCommandRequest();
|
||||
request.setCommitMessage("test commit");
|
||||
request.setSign(false);
|
||||
request.addRequest(new ModifyCommandRequest.CreateFileRequest("new_file", newFile, false));
|
||||
request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det"));
|
||||
|
||||
command.execute(request);
|
||||
|
||||
try (Git git = new Git(createContext().open())) {
|
||||
|
||||
RevCommit lastCommit = getLastCommit(git);
|
||||
assertThat(lastCommit.getRawGpgSignature()).isNullOrEmpty();
|
||||
}
|
||||
}
|
||||
|
||||
private void assertInTree(TreeAssertions assertions) throws IOException, GitAPIException {
|
||||
try (Git git = new Git(createContext().open())) {
|
||||
RevCommit lastCommit = getLastCommit(git);
|
||||
|
||||
Reference in New Issue
Block a user