Refactor ChromeFraudulentCertReporter for code reuse by SSL reporting

chrome/browser/net/certificate_error_reporter_unittest.cc

+// 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/certificate_error_reporter.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "base/thread_task_runner_handle.h"
+#include "chrome/browser/net/cert_logger.pb.h"
+#include "chrome/browser/net/url_request_mock_util.h"
+#include "chrome/common/chrome_paths.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/base/network_delegate_impl.h"
+#include "net/base/test_data_directory.h"
+#include "net/base/upload_bytes_element_reader.h"
+#include "net/base/upload_data_stream.h"
+#include "net/base/upload_element_reader.h"
+#include "net/test/cert_test_util.h"
+#include "net/test/url_request/url_request_failed_job.h"
+#include "net/test/url_request/url_request_mock_http_job.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using chrome_browser_net::CertificateErrorReporter;
+using content::BrowserThread;
+using net::CompletionCallback;
+using net::SSLInfo;
+using net::NetworkDelegateImpl;
+using net::TestURLRequestContext;
+using net::URLRequest;
+
+namespace {
+
+const char kHostname[] = "test.mail.google.com";
+const char kDummyFailureLog[] = "dummy failure log";
+const char kTestCertFilename[] = FILE_PATH_LITERAL("test_mail_google_com.pem");
+
+SSLInfo GetSSLInfo() {
+  SSLInfo info;
+  info.cert =
+      net::ImportCertFromFile(net::GetTestCertsDirectory(), kTestCertFilename);
+  info.is_issued_by_known_root = true;
+  info.pinning_failure_log = kDummyFailureLog;
+  return info;
+}
+
+std::string GetPEMEncodedChain() {
+  base::FilePath cert_path =
+      net::GetTestCertsDirectory().AppendASCII(kTestCertFilename);
+  std::string cert_data;
+  EXPECT_TRUE(base::ReadFileToString(cert_path, &cert_data));
+  return cert_data;
+}
+
+void EnableUrlRequestMocks(bool enable) {
+  net::URLRequestFilter::GetInstance()->ClearHandlers();
+  if (enable) {
+    base::FilePath root_http;
+    net::URLRequestFailedJob::AddUrlHandler();
+    PathService::Get(chrome::DIR_TEST_DATA, &root_http);
+    net::URLRequestMockHTTPJob::AddUrlHandlers(
+        root_http, BrowserThread::GetBlockingPool());
+  }
+}
+
+// Check that data uploaded in the request matches the test data (an
+// SSL report for |kHostname| as returned by |GetSSLInfo()|).
+void CheckUploadData(URLRequest* request) {
+  const net::UploadDataStream* upload = request->get_upload();
+  ASSERT_TRUE(upload);
+  ASSERT_TRUE(upload->GetElementReaders());
+  EXPECT_EQ(upload->GetElementReaders()->size(), 1u);

[mmenke, 2015/03/11 15:34:28]

For EXPECT_EQ and ASSERT_EQ, the expected value should go before the actual one.
 Goes for this entire method.

[estark, 2015/03/11 22:19:10]

Done.

+
+  const net::UploadBytesElementReader* reader =
+      (*upload->GetElementReaders())[0]->AsBytesReader();
+  ASSERT_TRUE(reader);
+  std::string upload_data(reader->bytes(), reader->length());
+  chrome_browser_net::CertLoggerRequest uploaded_request;
+
+  uploaded_request.ParseFromString(upload_data);
+
+  EXPECT_EQ(uploaded_request.hostname(), kHostname);
+  EXPECT_EQ(uploaded_request.cert_chain(), GetPEMEncodedChain());
+  EXPECT_EQ(uploaded_request.pin().size(), 1);
+  EXPECT_EQ(uploaded_request.pin().Get(0), kDummyFailureLog);
+}
+
+// A network delegate that lets tests check that a certificate error
+// report was sent. It counts the number of requests and lets tests
+// register a callback to run when the request is destroyed. It also
+// checks that the uploaded data is as expected (a report for
+// |kHostname| and |GetSSLInfo()|).
+class CertificateErrorReporterTestNetworkDelegate : public NetworkDelegateImpl {

[estark, 2015/03/11 05:58:34]

Is this name too disgustingly long? I was trying to make it hint that this
network delegate does stuff specific to the certificate error reporter tests
(e.g. checking that the uploaded data matches |kHostname| and |GetSSLInfo|).

[mmenke, 2015/03/11 15:34:28]

I'm fine with long names.  Maybe call it
TestCertificateErrorReporterNetworkDelegate instead?  (There are a bunch of
TestNetworkDelegates, so moving the Test makes it easier to find, and doesn't
come up on searches for the other test delegates).

[estark, 2015/03/11 22:19:10]

Done.

+ public:
+  CertificateErrorReporterTestNetworkDelegate()
+      : url_request_destroyed_callback_(base::Bind(&base::DoNothing)),
+        num_requests_(0) {}
+
+  ~CertificateErrorReporterTestNetworkDelegate() override {}
+
+  int OnBeforeURLRequest(URLRequest* request,
+                         const CompletionCallback& callback,
+                         GURL* new_url) override {
+    num_requests_++;
+    EXPECT_EQ(request->url(), expected_url_);
+    EXPECT_EQ(request->method(), "POST");
+
+    CheckUploadData(request);
+    return net::OK;
+  }
+
+  void OnURLRequestDestroyed(URLRequest* request) override {
+    url_request_destroyed_callback_.Run();
+  }
+
+  void SetURLRequestDestroyedCallback(base::Closure callback) {

[mmenke, 2015/03/11 15:34:28]

nit:  const base::Closure&

[estark, 2015/03/11 22:19:10]

Done.

+    url_request_destroyed_callback_ = callback;
+  }
+
+  void SetExpectedURL(const GURL& url) { expected_url_ = url; }

[mmenke, 2015/03/11 15:34:28]

nit:  set_expected_url.  Also more comment to call the argument |expected_url|. 
Should probably do the same for SetURLRequestDestroyedCallback as well.

Optionally, could take both as constructor parameters and make them const, but I
think it's fine, either way.

[estark, 2015/03/11 22:19:10]

Done.

+
+  int num_requests() const { return num_requests_; }
+
+ private:
+  base::Closure url_request_destroyed_callback_;
+  int num_requests_;
+  GURL expected_url_;

[mmenke, 2015/03/11 15:34:28]

nit:

DISALLOW_COPY_AND_ASSIGN(CertificateErrorReporterTestNetworkDelegate);
(After a blank line)

And include macros.h"

[estark, 2015/03/11 22:19:10]

Done.

+};
+
+class CertificateErrorReporterTest : public ::testing::Test {
+ public:
+  CertificateErrorReporterTest() : context_(true) {
+    EnableUrlRequestMocks(true);
+    context_.set_network_delegate(&network_delegate_);
+    context_.Init();
+  }
+
+  ~CertificateErrorReporterTest() override {
+    EnableUrlRequestMocks(false);
+    chrome_browser_net::SetUrlRequestMocksEnabled(false);

[mmenke, 2015/03/11 15:34:28]

This isn't needed - it does the same thing as the EnableUrlRequestMocks call
above.  You can also remove the header for it (url_request_mock_util.h)

[estark, 2015/03/11 22:19:10]

Done.

+  }
+
+  CertificateErrorReporterTestNetworkDelegate* network_delegate() {
+    return &network_delegate_;
+  }
+
+  TestURLRequestContext* context() { return &context_; }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;

[mmenke, 2015/03/11 15:34:28]

Do we still needs this?  Think we can get away with just a base::MessageLoop
now.  Fine if we need it, but if not, like to keep the dependencies to a
minimum.

[estark, 2015/03/11 22:19:10]

Done.

+  TestURLRequestContext context_;
+  CertificateErrorReporterTestNetworkDelegate network_delegate_;

[mmenke, 2015/03/11 15:34:28]

The network delegate should be before the context, so it gets destroyed first
(Context depends on the network delegate).  Doesn't really matter for these
tests, but it's a good habit to be in.

[estark, 2015/03/11 22:19:10]

Done.

+};
+
+// Test that CertificateErrorReporter::SendReport creates a URLRequest
+// for the endpoint and sends the expected data.
+TEST_F(CertificateErrorReporterTest, SendReportSendsRequest) {
+  base::RunLoop run_loop;
+  network_delegate()->SetURLRequestDestroyedCallback(run_loop.QuitClosure());
+
+  GURL url = net::URLRequestMockHTTPJob::GetMockHttpsUrl(
+      base::FilePath(FILE_PATH_LITERAL("empty.html")));
+  network_delegate()->SetExpectedURL(url);
+
+  CertificateErrorReporter reporter(context(), url);
+
+  EXPECT_EQ(0, network_delegate()->num_requests());
+
+  reporter.SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION,
+                      kHostname, GetSSLInfo());
+  run_loop.Run();
+
+  EXPECT_EQ(1, network_delegate()->num_requests());
+}
+
+// Test that pending URLRequests get cleaned up when the reporter is
+// deleted.
+TEST_F(CertificateErrorReporterTest, PendingRequestGetsDeleted) {
+  base::RunLoop run_loop;
+  network_delegate()->SetURLRequestDestroyedCallback(run_loop.QuitClosure());
+
+  GURL url = net::URLRequestMockHTTPJob::GetMockUrlWithFailure(
+      base::FilePath(FILE_PATH_LITERAL("empty.html")),
+      net::URLRequestMockHTTPJob::START, net::ERR_IO_PENDING);
+  network_delegate()->SetExpectedURL(url);
+
+  EXPECT_EQ(0, network_delegate()->num_requests());
+
+  scoped_ptr<CertificateErrorReporter> reporter(
+      new CertificateErrorReporter(context(), url));
+  reporter->SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION,
+                       kHostname, GetSSLInfo());
+  reporter.reset();
+
+  run_loop.Run();
+
+  EXPECT_EQ(1, network_delegate()->num_requests());
+}
+
+// Test that a request that returns an error gets cleaned up.
+TEST_F(CertificateErrorReporterTest, ErroredRequestGetsDeleted) {
+  base::RunLoop run_loop;
+  network_delegate()->SetURLRequestDestroyedCallback(run_loop.QuitClosure());
+
+  GURL url = net::URLRequestFailedJob::GetMockHttpsUrl(net::ERR_FAILED);
+  network_delegate()->SetExpectedURL(url);
+
+  EXPECT_EQ(0, network_delegate()->num_requests());
+
+  CertificateErrorReporter reporter(context(), url);
+  reporter.SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION,
+                      kHostname, GetSSLInfo());
+  run_loop.Run();
+
+  EXPECT_EQ(1, network_delegate()->num_requests());
+}
+
+}  // namespace