Refactor ChromeFraudulentCertReporter for code reuse by SSL reporting

chrome/browser/net/chrome_certificate_reporter.cc

Index: chrome/browser/net/chrome_certificate_reporter.cc
diff --git a/chrome/browser/net/chrome_fraudulent_certificate_reporter.cc b/chrome/browser/net/chrome_certificate_reporter.cc
similarity index 58%
copy from chrome/browser/net/chrome_fraudulent_certificate_reporter.cc
copy to chrome/browser/net/chrome_certificate_reporter.cc
index d5584938a7fe8e07f83636351ca752d4a03eda1a..d2783798213c09674a0f3ab3a49eddac4b466dfd 100644
--- a/chrome/browser/net/chrome_fraudulent_certificate_reporter.cc
+++ b/chrome/browser/net/chrome_certificate_reporter.cc
@@ -1,12 +1,11 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// 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.
 
-#include "chrome/browser/net/chrome_fraudulent_certificate_reporter.h"
+#include "chrome/browser/net/chrome_certificate_reporter.h"
 
 #include <set>
 
-#include "base/base64.h"
 #include "base/logging.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/stl_util.h"
@@ -22,69 +21,85 @@
 
 namespace chrome_browser_net {
 
-// TODO(palmer): Switch to HTTPS when the error handling delegate is more
-// sophisticated. Ultimately we plan to attempt the report on many transports.
-static const char kFraudulentCertificateUploadEndpoint[] =
-    "http://clients3.google.com/log_cert_error";
-
-ChromeFraudulentCertificateReporter::ChromeFraudulentCertificateReporter(
-    net::URLRequestContext* request_context)
-    : request_context_(request_context),
-      upload_url_(kFraudulentCertificateUploadEndpoint) {
+ChromeCertificateReporter::ChromeCertificateReporter(
+    net::URLRequestContext* request_context,
+    const std::string& upload_url)

[Ryan Sleevi, 2015/03/04 19:31:01]

Pass the upload_url as a GURL

[estark, 2015/03/04 22:11:56]

Done.

+    : request_context_(request_context), upload_url_(upload_url) {

[Ryan Sleevi, 2015/03/04 19:31:01]

What if someone gives us an empty GURL(). Is that valid?

[Hint: I don't think it should be]

[estark, 2015/03/04 22:11:56]

Is DCHECK the right way to handle an empty GURL? That's what I've done in this
patch set.

 }
 
-ChromeFraudulentCertificateReporter::~ChromeFraudulentCertificateReporter() {
+ChromeCertificateReporter::~ChromeCertificateReporter() {
   STLDeleteElements(&inflight_requests_);
 }
 
-static std::string BuildReport(const std::string& hostname,
-                               const net::SSLInfo& ssl_info) {
+scoped_ptr<net::URLRequest> ChromeCertificateReporter::CreateURLRequest(
+    net::URLRequestContext* context) {
+  scoped_ptr<net::URLRequest> request =
+      context->CreateRequest(upload_url_, net::DEFAULT_PRIORITY, this, NULL);
+  request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                        net::LOAD_DO_NOT_SAVE_COOKIES);
+  return request.Pass();
+}
+
+void ChromeCertificateReporter::SendReport(const std::string& hostname,
+                                           const net::SSLInfo& ssl_info) {
   CertLoggerRequest request;
+  BuildReport(hostname, ssl_info, request);
+  SendCertLoggerRequest(request);
+}
+
+// TODO(palmer): Currently, the upload is fire-and-forget but soon we will
+// try to recover by retrying, and trying different endpoints, and
+// appealing to the user.

[Ryan Sleevi, 2015/03/04 19:31:01]

This is just wrong/misleading. Let's just delete it :)

Or file a bug on it.

[estark, 2015/03/04 19:58:45]

Do you mean the whole comment, or the last part ("appealing to the user")?

[estark, 2015/03/04 22:11:56]

Done (deleted it).

+void ChromeCertificateReporter::OnResponseStarted(net::URLRequest* request) {
+  // TODO(vadimt): Remove ScopedTracker below once crbug.com/422516 is fixed.
+  tracked_objects::ScopedTracker tracking_profile(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "422516 ChromeCertificateReporter::OnResponseStarted"));

[Ryan Sleevi, 2015/03/04 19:31:01]

I'm going to suggest this be removed; at least, moved into the
ChromeFraudulentCertReporter, and not reported for the opt-in case

[estark, 2015/03/04 22:11:56]

Done.

+
+  const net::URLRequestStatus& status(request->status());
+  if (!status.is_success()) {
+    LOG(WARNING) << "Certificate upload failed"
+                 << " status:" << status.status()
+                 << " error:" << status.error();
+  } else if (request->GetResponseCode() != 200) {
+    LOG(WARNING) << "Certificate upload HTTP status: "
+                 << request->GetResponseCode();
+  }
+  RequestComplete(request);
+}
+
+void ChromeCertificateReporter::OnReadCompleted(net::URLRequest* request,
+                                                int bytes_read) {
+}
+
+void ChromeCertificateReporter::BuildReport(const std::string& hostname,
+                                            const net::SSLInfo& ssl_info,
+                                            CertLoggerRequest& out_request) {
   base::Time now = base::Time::Now();
-  request.set_time_usec(now.ToInternalValue());
-  request.set_hostname(hostname);
+  out_request.set_time_usec(now.ToInternalValue());
+  out_request.set_hostname(hostname);
 
   std::vector<std::string> pem_encoded_chain;
   if (!ssl_info.cert->GetPEMEncodedChain(&pem_encoded_chain)) {
     LOG(ERROR) << "Could not get PEM encoded chain.";
   }
-  std::string* cert_chain = request.mutable_cert_chain();
+  std::string* cert_chain = out_request.mutable_cert_chain();
   for (size_t i = 0; i < pem_encoded_chain.size(); ++i)
     *cert_chain += pem_encoded_chain[i];
 
-  request.add_pin(ssl_info.pinning_failure_log);
-
-  std::string out;
-  request.SerializeToString(&out);
-  return out;
-}
-
-scoped_ptr<net::URLRequest>
-ChromeFraudulentCertificateReporter::CreateURLRequest(
-    net::URLRequestContext* context) {
-  scoped_ptr<net::URLRequest> request =
-      context->CreateRequest(upload_url_, net::DEFAULT_PRIORITY, this, NULL);
-  request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                        net::LOAD_DO_NOT_SAVE_COOKIES);
-  return request.Pass();
+  out_request.add_pin(ssl_info.pinning_failure_log);
 }
 
-void ChromeFraudulentCertificateReporter::SendReport(
-    const std::string& hostname,
-    const net::SSLInfo& ssl_info) {
-  // We do silent/automatic reporting ONLY for Google properties. For other
-  // domains (when we start supporting that), we will ask for user permission.
-  if (!net::TransportSecurityState::IsGooglePinnedProperty(hostname)) {
-    return;
-  }
-
-  std::string report = BuildReport(hostname, ssl_info);
+void ChromeCertificateReporter::SendCertLoggerRequest(
+    CertLoggerRequest& request) {
+  std::string serialized_request;
+  request.SerializeToString(&serialized_request);
 
   scoped_ptr<net::URLRequest> url_request = CreateURLRequest(request_context_);
   url_request->set_method("POST");
 
   scoped_ptr<net::UploadElementReader> reader(
-      net::UploadOwnedBytesElementReader::CreateWithString(report));
+      net::UploadOwnedBytesElementReader::CreateWithString(serialized_request));
   url_request->set_upload(
       net::ElementsUploadDataStream::CreateWithReader(reader.Pass(), 0));
 
@@ -98,37 +113,11 @@ void ChromeFraudulentCertificateReporter::SendReport(
   raw_url_request->Start();
 }
 
-void ChromeFraudulentCertificateReporter::RequestComplete(
-    net::URLRequest* request) {
+void ChromeCertificateReporter::RequestComplete(net::URLRequest* request) {
   std::set<net::URLRequest*>::iterator i = inflight_requests_.find(request);
   DCHECK(i != inflight_requests_.end());
   scoped_ptr<net::URLRequest> url_request(*i);
   inflight_requests_.erase(i);
 }
 
-// TODO(palmer): Currently, the upload is fire-and-forget but soon we will
-// try to recover by retrying, and trying different endpoints, and
-// appealing to the user.
-void ChromeFraudulentCertificateReporter::OnResponseStarted(
-    net::URLRequest* request) {
-  // TODO(vadimt): Remove ScopedTracker below once crbug.com/422516 is fixed.
-  tracked_objects::ScopedTracker tracking_profile(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "422516 ChromeFraudulentCertificateReporter::OnResponseStarted"));
-
-  const net::URLRequestStatus& status(request->status());
-  if (!status.is_success()) {
-    LOG(WARNING) << "Certificate upload failed"
-                 << " status:" << status.status()
-                 << " error:" << status.error();
-  } else if (request->GetResponseCode() != 200) {
-    LOG(WARNING) << "Certificate upload HTTP status: "
-                 << request->GetResponseCode();
-  }
-  RequestComplete(request);
-}
-
-void ChromeFraudulentCertificateReporter::OnReadCompleted(
-    net::URLRequest* request, int bytes_read) {}
-
 }  // namespace chrome_browser_net