Add interstitial info to certificate reports

chrome/browser/net/certificate_error_reporter.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_NET_CERTIFICATE_ERROR_REPORTER_H_
 #define CHROME_BROWSER_NET_CERTIFICATE_ERROR_REPORTER_H_
 
 #include <set>
 #include <string>
 
@@ skipping 22 matching lines @@
     REPORT_TYPE_PINNING_VIOLATION,
     // A report for an invalid certificate chain that is being sent for
     // a user who has opted-in to the extended reporting program.
     REPORT_TYPE_EXTENDED_REPORTING
   };
 
   // Represents whether or not to send cookies along with reports sent
   // 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
   // cookies will be sent along with the reports.
   CertificateErrorReporter(net::URLRequestContext* request_context,
                            const GURL& upload_url,
                            CookiesPreference cookies_preference);
 
   ~CertificateErrorReporter() override;
 
   // Construct, serialize, and send a certificate report to the report
   // collection server containing the |ssl_info| associated with a
   // connection to |hostname|.
   //
   // SendReport actually sends the report over the network; callers are
   // 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);
 
   // 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.
   virtual scoped_ptr<net::URLRequest> CreateURLRequest(
       net::URLRequestContext* context);
 
   // Serialize and send a CertLoggerRequest protobuf to the report
   // 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);
 
   net::URLRequestContext* const request_context_;
   const GURL upload_url_;
 
   // Owns the contained requests.
   std::set<net::URLRequest*> inflight_requests_;
 
   CookiesPreference cookies_preference_;
 
   DISALLOW_COPY_AND_ASSIGN(CertificateErrorReporter);
 };
 
 }  // namespace chrome_browser_net
 
 #endif  // CHROME_BROWSER_NET_CERTIFICATE_ERROR_REPORTER_H_