Return enum from TransportSecurityState::CheckPublicKeyPins

net/http/transport_security_state.h

Index: net/http/transport_security_state.h
diff --git a/net/http/transport_security_state.h b/net/http/transport_security_state.h
index 2988f3ad8a49e9fe072795af4c4b2b9654cb20e0..0415a8c766c26121b5bb3310fd7645ae58f88834 100644
--- a/net/http/transport_security_state.h
+++ b/net/http/transport_security_state.h
@@ -107,6 +107,9 @@ class NET_EXPORT TransportSecurityState
     std::map<std::string, STSState>::const_iterator end_;
   };
 
+  // PKPStatus describes the result of a pinning check.
+  enum class PKPStatus { VIOLATED = -1, OK = 0, BYPASSED = 1 };

[Ryan Sleevi, 2016/06/15 02:04:03]

STYLE: No need to assign explicit values.
STYLE: No benefit from making it an "enum class" here because it doesn't allow
forward declaration
STYLE: Add documentation explaining what these statuses mean.

Example

enum PKPStatus {
  // Pinning was enabled and the necessary pins were not present.
  VIOLATED,

  // Pinning was not enabled, or it was and the certificate satisfied
  // the pins.
  OK,

  // Pinning was enabled, but ignored due to local policy, such as
  // a local trust anchor.
  BYPASSED,
};

NAMING: Is "violated" the right status here? RFC 7469 doesn't call it violated,
nor does our code (I guess, not until report_uri was introduced). In the context
of "CheckPublicKeyPins", what are natural ways of reading the possible results?

[dadrian, 2016/06/15 02:47:55]

I went with violated because that's what I've heard in conversation, both in and
out of Google, although I have no particular attachment to the name.

[dadrian, 2016/06/15 18:58:28]

I left it with enum class because I think we want the scoping benefits?

[Ryan Sleevi, 2016/06/15 19:02:23]

Scoping benefits? We don't gain anything in terms of scoping/naming resolution,
since we're a nested class, just in type safety. Was that what you meant?

[dadrian, 2016/06/15 19:33:16]

Both? We're trying to represent the result of PKP actions, not
TransportSecurityState as a whole, so saying TransportSecurityState::OK (vs
TransportSecurityState::PKPStatus::OK) shouldn't have any meaning unless we
convert every TransportSecurityState operation to use the same enum.

+
   // A PKPState describes the public key pinning state.
   class NET_EXPORT PKPState {
    public:
@@ -259,13 +262,14 @@ class NET_EXPORT TransportSecurityState
   // when is_issued_by_known_root is false.
   bool ShouldSSLErrorsBeFatal(const std::string& host);
   bool ShouldUpgradeToSSL(const std::string& host);
-  bool CheckPublicKeyPins(const HostPortPair& host_port_pair,
-                          bool is_issued_by_known_root,
-                          const HashValueVector& hashes,
-                          const X509Certificate* served_certificate_chain,
-                          const X509Certificate* validated_certificate_chain,
-                          const PublicKeyPinReportStatus report_status,
-                          std::string* failure_log);
+  PKPStatus CheckPublicKeyPins(
+      const HostPortPair& host_port_pair,
+      bool is_issued_by_known_root,
+      const HashValueVector& hashes,
+      const X509Certificate* served_certificate_chain,
+      const X509Certificate* validated_certificate_chain,
+      const PublicKeyPinReportStatus report_status,
+      std::string* failure_log);
   bool HasPublicKeyPins(const std::string& host);
 
   // Assign a |Delegate| for persisting the transport security state. If
@@ -411,7 +415,7 @@ class NET_EXPORT TransportSecurityState
   static bool IsBuildTimely();
 
   // Helper method for actually checking pins.
-  bool CheckPublicKeyPinsImpl(
+  PKPStatus CheckPublicKeyPinsImpl(
       const HostPortPair& host_port_pair,
       bool is_issued_by_known_root,
       const HashValueVector& hashes,
@@ -452,7 +456,7 @@ class NET_EXPORT TransportSecurityState
   // |report_status| says to), this method sends an HPKP violation
   // report containing |served_certificate_chain| and
   // |validated_certificate_chain|.
-  bool CheckPinsAndMaybeSendReport(
+  PKPStatus CheckPinsAndMaybeSendReport(
       const HostPortPair& host_port_pair,
       bool is_issued_by_known_root,
       const TransportSecurityState::PKPState& pkp_state,