Add interstitial info to certificate reports

chrome/browser/net/certificate_error_reporter.h

Index: chrome/browser/net/certificate_error_reporter.h
diff --git a/chrome/browser/net/certificate_error_reporter.h b/chrome/browser/net/certificate_error_reporter.h
index 3c20ec1c607abe8bd12edc60e4caa39973808ab6..30d95e41f75b64332e6916d422e24e85b6b2ac59 100644
--- a/chrome/browser/net/certificate_error_reporter.h
+++ b/chrome/browser/net/certificate_error_reporter.h
@@ -40,6 +40,9 @@ class CertificateErrorReporter : public net::URLRequest::Delegate {
   // to the server.
   enum CookiesPreference { SEND_COOKIES, DO_NOT_SEND_COOKIES };
 
+  enum ProceedDecision { USER_PROCEEDED, USER_DID_NOT_PROCEED };
+  enum Overridable { OVERRIDABLE, NOT_OVERRIDABLE };

[Ryan Sleevi, 2015/04/16 01:44:10]

First scan: This seems to introduce awareness of interstitials into the
CertificateErrorReporter, which doesn't seem ideal.

I'm not sure what to suggest, but it doesn't feel right.

Does anything else in chrome/browser/net know about interstitials? I don't think
it does....

(This is somewhat the same problem we had with /ssl/ and /interstitials/ and
breaking that dependency)

[estark, 2015/04/16 02:09:31]

Agreed. As I mentioned, I wasn't sure how to avoid this problem because the
protobuf (which must surely know about all the data it sends, including
interstitial info) is defined in chrome/browser/net.*

If we're willing to accept the protobuf knowing about interstitials, then here
is an alternative proposal that at least avoids CertificateErrorReporter knowing
about interstitials: chrome/browser/net defines a CertificateErrorReport
interface, and a CertificateErrorReport is what gets passed to |SendReport|.
chrome/browser/ssl implements that interface to build protobufs and populate the
interstitial fields. chrome/browser/net also has an implementation of the
interface that builds the protobuf without the interstitial fields; this is what
would be used by ChromeFraudulentCertificateReporter.

*I don't suppose there's some magical protobufs feature that I don't know about
that would solve this problem?

+
   // Create a certificate error reporter that will send certificate
   // error reports to |upload_url|, using |request_context| as the
   // context for the reports. |cookies_preference| controls whether
@@ -58,6 +61,7 @@ class CertificateErrorReporter : public net::URLRequest::Delegate {
   // responsible for enforcing any preconditions (such as obtaining user
   // opt-in, only sending reports for certain hostnames, checking for
   // incognito mode, etc.).
+  virtual void SendReport(ReportType type, const CertLoggerRequest& report);

[Ryan Sleevi, 2015/04/16 01:44:10]

Virtuals + overloads = special place in hell when you try to subclass (override
helps a little, but then you can end up shadow-hiding an override on the
subclass unless you import. Confused? Like I said, special place in hell...)

Generally, prefer
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...

   virtual void SendReport(ReportType type,
                           const std::string& hostname,
                           const net::SSLInfo& ssl_info);
@@ -66,6 +70,18 @@ class CertificateErrorReporter : public net::URLRequest::Delegate {
   void OnResponseStarted(net::URLRequest* request) override;
   void OnReadCompleted(net::URLRequest* request, int bytes_read) override;
 
+  // Populate the CertLoggerRequest for a report.

[Ryan Sleevi, 2015/04/16 01:44:10]

By making this public, this is beginning to violate the "Single Responsibility
Principle" of having one responsibility and doing it well.

1) I'm not a big fan of static methods on classes (I know, i'm a hater)
2) Does it make sense to factor out a report builder?

+  static void BuildReport(const std::string& hostname,
+                          const net::SSLInfo& ssl_info,
+                          CertLoggerRequest* out_request);
+  static void BuildReport(const std::string& hostname,

[Ryan Sleevi, 2015/04/16 01:44:10]

ditto overloads again

+                          const net::SSLInfo& ssl_info,
+                          uint32 validation_result,
+                          uint32 interstitial_reason,
+                          ProceedDecision proceed_decision,
+                          Overridable overridable,
+                          CertLoggerRequest* out_request);
+
  private:
   // Create a URLRequest with which to send a certificate report to the
   // server.
@@ -76,11 +92,6 @@ class CertificateErrorReporter : public net::URLRequest::Delegate {
   // collection server.
   void SendCertLoggerRequest(const CertLoggerRequest& request);
 
-  // Populate the CertLoggerRequest for a report.
-  static void BuildReport(const std::string& hostname,
-                          const net::SSLInfo& ssl_info,
-                          CertLoggerRequest* out_request);
-
   // Performs post-report cleanup.
   void RequestComplete(net::URLRequest* request);