Rewrite CheckResult to use an enum for status

PublicKeyChecker is able to distinguish between keys that are ok only
considering features internal to that key (expiration, revocation,
etc.), and keys that are both ok and trusted according to signatures
made by other keys in the store. Teach CheckResult this distinction as
well.

Change-Id: I2cb36b164a9c295ae0166ba82227fae3ddb93a2c

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/GpgKeyInfo.java

@@ -17,6 +17,28 @@ package com.google.gerrit.extensions.common;
 import java.util.List;
 
 public class GpgKeyInfo {
+  /**
+   * Status of checking an object like a key or signature.
+   * <p>
+   * Order of values in this enum is significant: OK is "better" than BAD, etc.
+   */
+  public enum Status {
+    /** Something is wrong with this key. */
+    BAD,
+
+    /**
+     * Inspecting only this key found no problems, but the system does not fully
+     * trust the key's origin.
+     */
+    OK,
+
+    /**
+     * This key is valid, and the system knows enough about the key and its
+     * origin to trust it.
+     */
+    TRUSTED;
+  }
+
   public String id;
   public String fingerprint;
   public List<String> userIds;

gerrit-gpg/src/main/java/com/google/gerrit/gpg/CheckResult.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.gpg;
 
+import com.google.gerrit.extensions.common.GpgKeyInfo.Status;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -21,24 +23,60 @@ import java.util.List;
 
 /** Result of checking an object like a key or signature. */
 public class CheckResult {
-  public static final CheckResult OK = new CheckResult();
+  static CheckResult ok(String... problems) {
+    return create(Status.OK, problems);
+  }
 
+  static CheckResult bad(String... problems) {
+    return create(Status.BAD, problems);
+  }
+
+  static CheckResult trusted() {
+    return new CheckResult(Status.TRUSTED, Collections.<String> emptyList());
+  }
+
+  static CheckResult create(Status status, String... problems) {
+    List<String> problemList = problems.length > 0
+        ? Collections.unmodifiableList(Arrays.asList(problems))
+        : Collections.<String> emptyList();
+    return new CheckResult(status, problemList);
+  }
+
+  static CheckResult create(Status status, List<String> problems) {
+    return new CheckResult(status,
+        Collections.unmodifiableList(new ArrayList<>(problems)));
+  }
+
+  static CheckResult create(List<String> problems) {
+    return new CheckResult(
+        problems.isEmpty() ? Status.OK : Status.BAD,
+        Collections.unmodifiableList(problems));
+  }
+
+  private final Status status;
   private final List<String> problems;
 
-  CheckResult(String... problems) {
+  private CheckResult(Status status, List<String> problems) {
-    this(Arrays.asList(problems));
+    if (status == null) {
+      throw new IllegalArgumentException("status must not be null");
+    }
+    this.status = status;
+    this.problems = problems;
   }
 
-  CheckResult(List<String> problems) {
+  /** @return whether the result has status {@link Status#OK} or better. */
-    this.problems = Collections.unmodifiableList(new ArrayList<>(problems));
-  }
-
-  /**
-   * @return whether the result is entirely ok, i.e. has passed any verification
-   *     or validation checks.
-   */
   public boolean isOk() {
-    return problems.isEmpty();
+    return status.compareTo(Status.OK) >= 0;
+  }
+
+  /** @return whether the result has status {@link Status#TRUSTED} or better. */
+  public boolean isTrusted() {
+    return status.compareTo(Status.TRUSTED) >= 0;
+  }
+
+  /** @return the status enum value associated with the object. */
+  public Status getStatus() {
+    return status;
   }
 
   /** @return any problems encountered during checking. */
@@ -49,12 +87,9 @@ public class CheckResult {
   @Override
   public String toString() {
     StringBuilder sb = new StringBuilder(getClass().getSimpleName())
-        .append('[');
+        .append('[').append(status);
     for (int i = 0; i < problems.size(); i++) {
-      if (i > 0) {
+      sb.append(i == 0 ? ": " : ", ").append(problems.get(i));
-        sb.append(", ");
-      }
-      sb.append(problems.get(i));
     }
     return sb.append(']').toString();
   }

gerrit-gpg/src/main/java/com/google/gerrit/gpg/GerritPublicKeyChecker.java

@@ -86,13 +86,12 @@ public class GerritPublicKeyChecker extends PublicKeyChecker {
   }
 
   @Override
-  public void checkCustom(PGPPublicKey key, List<String> problems) {
+  public CheckResult checkCustom(PGPPublicKey key) {
     try {
       Set<String> allowedUserIds = getAllowedUserIds();
       if (allowedUserIds.isEmpty()) {
-        problems.add("No identities found for user; check "
+        return CheckResult.bad("No identities found for user; check "
             + webUrl + "#" + PageLinks.SETTINGS_WEBIDENT);
-        return;
       }
 
       @SuppressWarnings("unchecked")
@@ -103,17 +102,17 @@ public class GerritPublicKeyChecker extends PublicKeyChecker {
           Iterator<PGPSignature> sigs = getSignaturesForId(key, userId);
           while (sigs.hasNext()) {
             if (isValidCertification(key, sigs.next(), userId)) {
-              return;
+              return CheckResult.trusted();
             }
           }
         }
       }
 
-      problems.add(missingUserIds(allowedUserIds));
+      return CheckResult.bad(missingUserIds(allowedUserIds));
     } catch (PGPException e) {
       String msg = "Error checking user IDs for key";
       log.warn(msg + " " + keyIdToString(key.getKeyID()), e);
-      problems.add(msg);
+      return CheckResult.bad(msg);
     }
   }
 

gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyChecker.java

@@ -14,9 +14,14 @@
 
 package com.google.gerrit.gpg;
 
+import static com.google.gerrit.extensions.common.GpgKeyInfo.Status.BAD;
+import static com.google.gerrit.extensions.common.GpgKeyInfo.Status.OK;
+import static com.google.gerrit.extensions.common.GpgKeyInfo.Status.TRUSTED;
 import static com.google.gerrit.gpg.PublicKeyStore.keyIdToString;
 import static com.google.gerrit.gpg.PublicKeyStore.keyToString;
 
+import com.google.gerrit.extensions.common.GpgKeyInfo.Status;
+
 import org.bouncycastle.bcpg.SignatureSubpacket;
 import org.bouncycastle.bcpg.SignatureSubpacketTags;
 import org.bouncycastle.openpgp.PGPException;
@@ -105,18 +110,55 @@ public class PublicKeyChecker {
   /**
    * Perform custom checks.
    * <p>
-   * Default implementation does nothing, but may be overridden by subclasses.
+   * Default implementation reports no problems, but may be overridden by
+   * subclasses.
    *
    * @param key the public key.
-   * @param problems list to which any problems should be added.
+   * @return the result of the custom check.
    */
-  public void checkCustom(PGPPublicKey key, List<String> problems) {
+  public CheckResult checkCustom(PGPPublicKey key) {
-    // Default implementation does nothing.
+    return CheckResult.ok();
   }
 
   private CheckResult check(PGPPublicKey key, PublicKeyStore store, int depth,
       boolean expand, Set<Fingerprint> seen) {
-    List<String> problems = new ArrayList<>();
+    CheckResult basicResult = checkBasic(key);
+    CheckResult customResult = checkCustom(key);
+    CheckResult trustResult = checkWebOfTrust(key, store, depth, seen);
+    if (!expand && !trustResult.isTrusted()) {
+      trustResult = CheckResult.create(trustResult.getStatus(),
+          "Key is not trusted");
+    }
+
+    List<String> problems = new ArrayList<>(
+        basicResult.getProblems().size()
+        + customResult.getProblems().size()
+        + trustResult.getProblems().size());
+    problems.addAll(basicResult.getProblems());
+    problems.addAll(customResult.getProblems());
+    problems.addAll(trustResult.getProblems());
+
+    Status status;
+    if (basicResult.getStatus() == BAD
+        || customResult.getStatus() == BAD
+        || trustResult.getStatus() == BAD) {
+      // Any BAD result and the final result is BAD.
+      status = BAD;
+    } else if (trustResult.getStatus() == TRUSTED) {
+      // basicResult is BAD or OK, whereas trustResult is BAD or TRUSTED. If
+      // TRUSTED, we trust the final result.
+      status = TRUSTED;
+    } else {
+      // All results were OK or better, but trustResult was not TRUSTED. Don't
+      // let subclasses bypass checkWebOfTrust by returning TRUSTED; just return
+      // OK here.
+      status = OK;
+    }
+    return CheckResult.create(status, problems);
+  }
+
+  private CheckResult checkBasic(PGPPublicKey key) {
+    List<String> problems = new ArrayList<>(2);
     if (key.isRevoked()) {
       // TODO(dborowitz): isRevoked is overeager:
       // http://www.bouncycastle.org/jira/browse/BJB-45
@@ -131,33 +173,26 @@ public class PublicKeyChecker {
         problems.add("Key is expired");
       }
     }
-    checkCustom(key, problems);
+    return CheckResult.create(problems);
-
-    CheckResult trustResult = checkWebOfTrust(key, store, depth, seen);
-    if (expand) {
-      problems.addAll(trustResult.getProblems());
-    } else if (!trustResult.isOk()) {
-      problems.add("Key is not trusted");
-    }
-    return new CheckResult(problems);
   }
 
   private CheckResult checkWebOfTrust(PGPPublicKey key, PublicKeyStore store,
       int depth, Set<Fingerprint> seen) {
     if (trusted == null || store == null) {
-      return CheckResult.OK; // Trust checking not configured.
+      // Trust checking not configured, server trusts all OK keys.
+      return CheckResult.trusted();
     }
     Fingerprint fp = new Fingerprint(key.getFingerprint());
     if (seen.contains(fp)) {
-      return new CheckResult("Key is trusted in a cycle");
+      return CheckResult.ok("Key is trusted in a cycle");
     }
     seen.add(fp);
 
     Fingerprint trustedFp = trusted.get(key.getKeyID());
     if (trustedFp != null && trustedFp.equals(fp)) {
-      return CheckResult.OK; // Directly trusted.
+      return CheckResult.trusted(); // Directly trusted.
     } else if (depth >= maxTrustDepth) {
-      return new CheckResult(
+      return CheckResult.ok(
           "No path of depth <= " + maxTrustDepth + " to a trusted key");
     }
 
@@ -182,14 +217,15 @@ public class PublicKeyChecker {
             || Arrays.equals(signer.getFingerprint(), key.getFingerprint())) {
           continue;
         }
-        CheckResult signerResult = checkTrustSubpacket(sig, depth);
+        String subpacketProblem = checkTrustSubpacket(sig, depth);
-        if (signerResult.isOk()) {
+        if (subpacketProblem == null) {
-          signerResult = check(signer, store, depth + 1, false, seen);
+          CheckResult signerResult =
-          if (signerResult.isOk()) {
+              check(signer, store, depth + 1, false, seen);
-            return CheckResult.OK;
+          if (signerResult.isTrusted()) {
+            return CheckResult.trusted();
           }
         }
-        signerResults.add(new CheckResult(
+        signerResults.add(CheckResult.ok(
             "Certification by " + keyToString(signer)
             + " is valid, but key is not trusted"));
       }
@@ -200,7 +236,7 @@ public class PublicKeyChecker {
     for (CheckResult signerResult : signerResults) {
       problems.addAll(signerResult.getProblems());
     }
-    return new CheckResult(problems);
+    return CheckResult.create(OK, problems);
   }
 
   private static PGPPublicKey getSigner(PublicKeyStore store, PGPSignature sig,
@@ -208,42 +244,42 @@ public class PublicKeyChecker {
     try {
       PGPPublicKeyRingCollection signers = store.get(sig.getKeyID());
       if (!signers.getKeyRings().hasNext()) {
-        results.add(new CheckResult(
+        results.add(CheckResult.ok(
             "Key " + keyIdToString(sig.getKeyID())
             + " used for certification is not in store"));
         return null;
       }
       PGPPublicKey signer = PublicKeyStore.getSigner(signers, sig, userId, key);
       if (signer == null) {
-        results.add(new CheckResult(
+        results.add(CheckResult.ok(
             "Certification by " + keyIdToString(sig.getKeyID())
             + " is not valid"));
         return null;
       }
       return signer;
     } catch (PGPException | IOException e) {
-      results.add(new CheckResult(
+      results.add(CheckResult.ok(
           "Error checking certification by " + keyIdToString(sig.getKeyID())));
       return null;
     }
   }
 
-  private CheckResult checkTrustSubpacket(PGPSignature sig, int depth) {
+  private String checkTrustSubpacket(PGPSignature sig, int depth) {
     SignatureSubpacket trustSub = sig.getHashedSubPackets().getSubpacket(
         SignatureSubpacketTags.TRUST_SIG);
     if (trustSub == null || trustSub.getData().length != 2) {
-      return new CheckResult("Certification is missing trust information");
+      return "Certification is missing trust information";
     }
     byte amount = trustSub.getData()[1];
     if (amount < COMPLETE_TRUST) {
-      return new CheckResult("Certification does not fully trust key");
+      return "Certification does not fully trust key";
     }
     byte level = trustSub.getData()[0];
     int required = depth + 1;
     if (level < required) {
-      return new CheckResult("Certification trusts to depth " + level
+      return "Certification trusts to depth " + level
-          + ", but depth " + required + " is required");
+          + ", but depth " + required + " is required";
     }
-    return CheckResult.OK;
+    return null;
   }
 }

gerrit-gpg/src/main/java/com/google/gerrit/gpg/PushCertificateChecker.java

@@ -14,10 +14,14 @@
 
 package com.google.gerrit.gpg;
 
+import static com.google.gerrit.extensions.common.GpgKeyInfo.Status.BAD;
+import static com.google.gerrit.extensions.common.GpgKeyInfo.Status.OK;
+import static com.google.gerrit.extensions.common.GpgKeyInfo.Status.TRUSTED;
 import static com.google.gerrit.gpg.PublicKeyStore.keyIdToString;
 import static com.google.gerrit.gpg.PublicKeyStore.keyToString;
 
 import com.google.common.base.Joiner;
+import com.google.gerrit.extensions.common.GpgKeyInfo.Status;
 
 import org.bouncycastle.bcpg.ArmoredInputStream;
 import org.bouncycastle.openpgp.PGPException;
@@ -57,31 +61,59 @@ public abstract class PushCertificateChecker {
    */
   public final CheckResult check(PushCertificate cert) {
     if (cert.getNonceStatus() != NonceStatus.OK) {
-      return new CheckResult("Invalid nonce");
+      return CheckResult.bad("Invalid nonce");
     }
-    List<String> problems = new ArrayList<>();
+    List<CheckResult> results = new ArrayList<>(2);
+    CheckResult sigResult = null;
     try {
       PGPSignature sig = readSignature(cert);
       if (sig != null) {
         @SuppressWarnings("resource")
         Repository repo = getRepository();
         try (PublicKeyStore store = new PublicKeyStore(repo)) {
-          checkSignature(sig, cert, store, problems);
+          sigResult = checkSignature(sig, cert, store);
-          checkCustom(repo, problems);
+          results.add(checkCustom(repo));
         } finally {
           if (shouldClose(repo)) {
             repo.close();
           }
         }
       } else {
-        problems.add("Invalid signature format");
+        results.add(CheckResult.bad("Invalid signature format"));
       }
     } catch (PGPException | IOException e) {
       String msg = "Internal error checking push certificate";
       log.error(msg, e);
-      problems.add(msg);
+      results.add(CheckResult.bad(msg));
     }
-    return new CheckResult(problems);
+
+    return combine(sigResult, results);
+  }
+
+  private static CheckResult combine(CheckResult sigResult,
+      List<CheckResult> results) {
+    // Combine results:
+    //  - If any input result is BAD, the final result is bad.
+    //  - If sigResult is TRUSTED and no other result is BAD, the final result
+    //    is TRUSTED.
+    //  - Otherwise, the result is OK.
+    List<String> problems = new ArrayList<>();
+    boolean bad = false;
+    for (CheckResult result : results) {
+      problems.addAll(result.getProblems());
+      bad |= result.getStatus() == BAD;
+    }
+    Status status = bad ? BAD : OK;
+
+    if (sigResult != null) {
+      problems.addAll(sigResult.getProblems());
+      if (sigResult.getStatus() == BAD) {
+        status = BAD;
+      } else if (!bad && sigResult.getStatus() == TRUSTED) {
+        status = TRUSTED;
+      }
+    }
+    return CheckResult.create(status, problems);
   }
 
   /**
@@ -104,13 +136,14 @@ public abstract class PushCertificateChecker {
   /**
    * Perform custom checks.
    * <p>
-   * Default implementation does nothing, but may be overridden by subclasses.
+   * Default implementation reports no problems, but may be overridden by
+   * subclasses.
    *
    * @param repo a repository previously returned by {@link #getRepository()}.
-   * @param problems list to which any problems should be added.
+   * @return the result of the custom check.
    */
-  protected void checkCustom(Repository repo, List<String> problems) {
+  protected CheckResult checkCustom(Repository repo) {
-    // Default implementation does nothing.
+    return CheckResult.ok();
   }
 
   private PGPSignature readSignature(PushCertificate cert) throws IOException {
@@ -129,28 +162,27 @@ public abstract class PushCertificateChecker {
     return null;
   }
 
-  private void checkSignature(PGPSignature sig, PushCertificate cert,
+  private CheckResult checkSignature(PGPSignature sig, PushCertificate cert,
-      PublicKeyStore store, List<String> problems)
+      PublicKeyStore store) throws PGPException, IOException {
-      throws PGPException, IOException {
     PGPPublicKeyRingCollection keys = store.get(sig.getKeyID());
     if (!keys.getKeyRings().hasNext()) {
-      problems.add("No public keys found for key ID "
+      return CheckResult.bad("No public keys found for key ID "
           + keyIdToString(sig.getKeyID()));
-      return;
     }
     PGPPublicKey signer =
         PublicKeyStore.getSigner(keys, sig, Constants.encode(cert.toText()));
     if (signer == null) {
-      problems.add("Signature by " + keyIdToString(sig.getKeyID())
+      return CheckResult.bad("Signature by " + keyIdToString(sig.getKeyID())
           + " is not valid");
-      return;
     }
     CheckResult result = publicKeyChecker.check(signer, store);
-    if (!result.isOk()) {
+    if (!result.getProblems().isEmpty()) {
-      problems.add("Invalid public key "
+      StringBuilder err = new StringBuilder("Invalid public key ")
-          + keyToString(signer)
+          .append(keyToString(signer))
-          + ":\n  "
+          .append(":\n  ")
-          + Joiner.on("\n  ").join(result.getProblems()));
+          .append(Joiner.on("\n  ").join(result.getProblems()));
+      return CheckResult.create(result.getStatus(), err.toString());
     }
+    return result;
   }
 }