opendev/gerrit
Code Issues Proposed changes

PushCertificateChecker: Don't throw exceptions

Browse Source 
We already have a type CheckResult for collecting various errors, so
include all exceptions in that. This means the prereceive hook no
longer has to handle CheckResults and exceptions differently.

Change-Id: I8f5c3a87903270e18a55485db6439c17b36a7357
This commit is contained in:
Dave Borowitz
2015-08-25 11:08:58 -04:00
parent 102db5fcbd
commit 8a55df0168
2 changed files with 44 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
gerrit-gpg/src/main/java/com/google/gerrit/gpg/PushCertificateChecker.java
View File

@@ -31,6 +31,8 @@ import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.PushCertificate;
import org.eclipse.jgit.transport.PushCertificate.NonceStatus;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.ByteArrayInputStream;
import java.io.IOException;
@@ -39,6 +41,9 @@ import java.util.List;
/** Checker for push certificates. */
public abstract class PushCertificateChecker {
private static final Logger log =
LoggerFactory.getLogger(PushCertificateChecker.class);
private final PublicKeyChecker publicKeyChecker;
protected PushCertificateChecker(PublicKeyChecker publicKeyChecker) {
@@ -49,28 +54,34 @@ public abstract class PushCertificateChecker {
* Check a push certificate.
*
* @return result of the check.
* @throws PGPException if an error occurred during GPG checks.
* @throws IOException if an error occurred reading from the repository.
*/
public final CheckResult check(PushCertificate cert) throws PGPException, IOException {
public final CheckResult check(PushCertificate cert) {
if (cert.getNonceStatus() != NonceStatus.OK) {
return new CheckResult("Invalid nonce");
}
PGPSignature sig = readSignature(cert);
if (sig == null) {
return new CheckResult("Invalid signature format");
}
Repository repo = getRepository();
List<String> problems = new ArrayList<>();
try (PublicKeyStore store = new PublicKeyStore(repo)) {
checkSignature(sig, cert, store.get(sig.getKeyID()), problems);
checkCustom(repo, problems);
return new CheckResult(problems);
} finally {
if (shouldClose(repo)) {
repo.close();
try {
PGPSignature sig = readSignature(cert);
if (sig != null) {
@SuppressWarnings("resource")
Repository repo = getRepository();
try (PublicKeyStore store = new PublicKeyStore(repo)) {
checkSignature(sig, cert, store.get(sig.getKeyID()), problems);
checkCustom(repo, problems);
} finally {
if (shouldClose(repo)) {
repo.close();
}
}
} else {
problems.add("Invalid signature format");
}
} catch (PGPException | IOException e) {
String msg = "Internal error checking push certificate";
log.error(msg, e);
problems.add(msg);
}
return new CheckResult(problems);
}
/**

47
gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java
View File

@@ -19,14 +19,11 @@ import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.bouncycastle.openpgp.PGPException;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.PreReceiveHook;
import org.eclipse.jgit.transport.PushCertificate;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.eclipse.jgit.transport.ReceivePack;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.Collection;
@@ -40,9 +37,6 @@ import java.util.Collection;
*/
@Singleton
public class SignedPushPreReceiveHook implements PreReceiveHook {
private static final Logger log =
LoggerFactory.getLogger(SignedPushPreReceiveHook.class);
private final GitRepositoryManager repoManager;
private final AllUsersName allUsers;
private final PublicKeyChecker keyChecker;
@@ -60,32 +54,27 @@ public class SignedPushPreReceiveHook implements PreReceiveHook {
@Override
public void onPreReceive(ReceivePack rp,
Collection<ReceiveCommand> commands) {
try {
PushCertificate cert = rp.getPushCertificate();
if (cert == null) {
return;
PushCertificate cert = rp.getPushCertificate();
if (cert == null) {
return;
}
PushCertificateChecker checker = new PushCertificateChecker(keyChecker) {
@Override
protected Repository getRepository() throws IOException {
return repoManager.openRepository(allUsers);
}
PushCertificateChecker checker = new PushCertificateChecker(keyChecker) {
@Override
protected Repository getRepository() throws IOException {
return repoManager.openRepository(allUsers);
}
@Override
protected boolean shouldClose(Repository repo) {
return true;
}
};
CheckResult result = checker.check(cert);
if (!result.isOk()) {
for (String problem : result.getProblems()) {
rp.sendMessage(problem);
}
reject(commands, "invalid push cert");
@Override
protected boolean shouldClose(Repository repo) {
return true;
}
} catch (PGPException | IOException e) {
log.error("Error checking push certificate", e);
reject(commands, "push cert error");
};
CheckResult result = checker.check(cert);
if (!result.isOk()) {
for (String problem : result.getProblems()) {
rp.sendMessage(problem);
}
reject(commands, "invalid push cert");
}
}