Refactor ChromeFraudulentCertReporter for code reuse by SSL reporting

chrome/browser/net/certificate_error_reporter_unittest.cc

Index: chrome/browser/net/certificate_error_reporter_unittest.cc
diff --git a/chrome/browser/net/certificate_error_reporter_unittest.cc b/chrome/browser/net/certificate_error_reporter_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0960d5a874359240d9b628445f814cc39ee32e15
--- /dev/null
+++ b/chrome/browser/net/certificate_error_reporter_unittest.cc
@@ -0,0 +1,221 @@
+// 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/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());
+  }
+}
+
+// A network delegate that lets tests check that requests were created
+// or destroyed.
+class TestNetworkDelegate : public NetworkDelegateImpl {
+ public:
+  TestNetworkDelegate() : NetworkDelegateImpl() {

[mmenke, 2015/03/10 15:08:46]

0-argument parent class constructor does not need to be explicitly invoked.

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

Done.

+    requests_ = 0;
+    destroyed_requests_ = 0;

[mmenke, 2015/03/10 15:08:46]

These should be in the initializer list.

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

Done.

+  }
+  ~TestNetworkDelegate() {}

[mmenke, 2015/03/10 15:08:45]

override

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

Done.

+
+  int OnBeforeURLRequest(URLRequest* request,
+                         const CompletionCallback& callback,
+                         GURL* new_url) override {
+    requests_++;
+    return net::OK;
+  }
+
+  void OnURLRequestDestroyed(URLRequest* request) { destroyed_requests_++; }

[mmenke, 2015/03/10 15:08:46]

override

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

Done.

+
+  int GetNumRequests() { return requests_; }
+  int GetNumDestroyedRequests() { return destroyed_requests_; }

[mmenke, 2015/03/10 15:08:46]

these should both be const, and have the same name of the variable they return.

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

Done.

+
+ private:
+  int requests_;
+  int destroyed_requests_;

[mmenke, 2015/03/10 15:08:46]

These should match the number of the getters.  Add num_ prefixes to both?

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

Done.

+};
+
+class CertificateErrorReporterTest : public ::testing::Test {
+ public:
+  ~CertificateErrorReporterTest() {

[mmenke, 2015/03/10 15:08:45]

override

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

Done.

+    EnableUrlRequestMocks(false);
+    chrome_browser_net::SetUrlRequestMocksEnabled(false);
+  }
+
+ protected:
+  CertificateErrorReporterTest() : context_(true) {

[mmenke, 2015/03/10 15:08:46]

Optional:  Suggest just making this public, before the destructor, to make this
easier to read.

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

Done.

+    EnableUrlRequestMocks(true);
+    context_.set_network_delegate(&network_delegate_);
+    context_.Init();
+  }
+
+  content::TestBrowserThreadBundle thread_bundle_;

[mmenke, 2015/03/10 15:08:46]

The Google style guide requires all class variables to be private.  If you need
to access these, can just add accessor methods.

[Ryan Sleevi, 2015/03/10 17:39:30]

Matt, test fixtures are explicitly allowed to be Protected -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control

[mmenke, 2015/03/10 17:52:32]

Hrm...Did the old version say that?  I admittedly haven't read through it since
it was updated a couple years back.

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

Done.

+  TestURLRequestContext context_;
+  TestNetworkDelegate network_delegate_;
+};
+
+class TestReporter : public CertificateErrorReporter {

[mmenke, 2015/03/10 15:08:45]

Need a virtual destructor in child classes.

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

This class no longer exists. I moved the callback to be called from
|OnURLRequestDestroyed| in the network delegate subclass as suggested, and keep
track of the information about the latest request there as well.

+ public:
+  TestReporter(net::URLRequestContext* context, const GURL& upload_url)
+      : CertificateErrorReporter(context, upload_url), latest_request_url_() {

[mmenke, 2015/03/10 15:08:46]

Remove latest_request_url_() - the no argument constructor will be called by
default, no need to explicitly include it.

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

no longer applicable here, but noted for future

+    response_started_callback_ = base::Bind(&base::DoNothing);

[mmenke, 2015/03/10 15:08:46]

This should be in an initializer list.

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

Done (in the network delegate subclass now).

+  }
+
+  TestReporter(net::URLRequestContext* context,
+               const GURL& upload_url,
+               base::Closure response_started_callback)
+      : CertificateErrorReporter(context, upload_url),
+        latest_request_url_(),

[mmenke, 2015/03/10 15:08:45]

Remove latest_request_url_()

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

(no longer applicable)

+        response_started_callback_(response_started_callback) {}
+
+  void OnResponseStarted(net::URLRequest* request) override {
+    latest_request_url_ = request->url();
+    CertificateErrorReporter::OnResponseStarted(request);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  response_started_callback_);

[mmenke, 2015/03/10 15:08:46]

Can invoke this synchronously with just response_started_callback_.Run(). 
Should also check if it's empty() (is_empty()?  is_null()?  Can't remember what
it's called) before invoking the callback.

[Ryan Sleevi, 2015/03/10 17:39:30]

I'm not sure I'd agree on the is_empty. By not checking, you make the API
contract clearer than it must do something (and even base::DoNothing is doing
something)

I suggested PostTask as more idiomatic / reflective of how URLRequest works (in
that, in my experience, it tends to PostTask work to clear the recursion stack).
I defer to Matt if he disagrees.

[mmenke, 2015/03/10 17:52:32]

You're right - I hadn't realized that the other constructor set a callback that
does nothing (Was actually wondering why it didn't crash in that case).  I've
never seen base::DoNothing used before, but I'm fine with it.
My thinking is that just seems kinda weird to me to do post a task to the thread
the TestReporter is called on, rather than taking one as input (Admittedly, it
would be the same thread), anyhow, I don't have a strong opinion here, just
seemed a little unusual.

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

I think this discussion is no longer applicable, since the callback is now
called from |OnURLRequestDestroyed|.

+  }
+
+  GURL& GetLatestRequestURL() { return latest_request_url_; }

[mmenke, 2015/03/10 15:08:46]

This should be:  const GURL& latest_request_url() const {
(+2 consts, change naming convention).

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

(no longer applicable)

+
+ private:
+  GURL latest_request_url_;
+  base::Closure response_started_callback_;
+};
+
+// Test that CertificateErrorReporter::SendReport creates a URLRequest
+// for the endpoint.
+TEST_F(CertificateErrorReporterTest, SendReportSendsRequest) {
+  base::RunLoop run_loop;
+  base::Closure callback = run_loop.QuitClosure();

[mmenke, 2015/03/10 15:08:45]

optional nit:  No need to store this on the stack.  Can just inline
run_loop.QuitClosure() in the TestReporter constructor call.  Same goes for
ErroredRequestGetsDeleted.

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

Done.

+  GURL url = net::URLRequestMockHTTPJob::GetMockHttpsUrl(
+      base::FilePath(FILE_PATH_LITERAL("empty.html")));
+  TestReporter reporter(&context_, url, callback);
+
+  EXPECT_EQ(0, network_delegate_.GetNumRequests());
+
+  reporter.SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION,
+                      kHostname, GetSSLInfo());
+  run_loop.Run();
+
+  EXPECT_EQ(1, network_delegate_.GetNumRequests());
+  EXPECT_EQ(url, reporter.GetLatestRequestURL());
+}
+
+// Test that pending URLRequests get cleaned up when the reporter is
+// deleted.
+TEST_F(CertificateErrorReporterTest, PendingRequestGetsDeleted) {
+  base::RunLoop run_loop;
+  GURL url = net::URLRequestMockHTTPJob::GetMockUrlWithFailure(
+      base::FilePath(FILE_PATH_LITERAL("empty.html")),
+      net::URLRequestMockHTTPJob::START, net::ERR_IO_PENDING);
+
+  EXPECT_EQ(0, network_delegate_.GetNumRequests());
+  EXPECT_EQ(0, network_delegate_.GetNumDestroyedRequests());
+
+  TestReporter* reporter = new TestReporter(&context_, url);
+  reporter->SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION,
+                       kHostname, GetSSLInfo());
+  delete reporter;

[mmenke, 2015/03/10 15:08:45]

Rather than calling delete, I suggest putting it in a scoped_ptr<TestReporter)
on creation, and then resetting the scoped_ptr.  It's not a critical issue here,
but it results in less error prone code in general.

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

Done.

+
+  run_loop.RunUntilIdle();

[mmenke, 2015/03/10 15:08:46]

Is this needed?

[estark, 2015/03/10 15:54:15]

rsleevi advised to add it to avoid coupling with the reporter implementation,
e.g. the reporter destructor could theoretically post a task to do the cleanup.

[mmenke, 2015/03/10 16:08:52]

Of course, it could *also* post a delayed task to do that.  :)  If we're really
concerned about that, we should make the NetworkDelegate have the ability to
wait until a request is destroyed (Note that you could actually put all the
waiting logic in the NetworkDelegate, and then just wait for destruction on all
tests.  Would make this test look more like the others, too).  Anyhow, just a
suggestion.

[Ryan Sleevi, 2015/03/10 17:39:30]

On 2015/03/10 16:08:52, mmenke wrote: 
Too Many Idioms (to the tune of Too Many Cooks)

I suspect that adding the waiting logic to the NetworkDelegate is a non-trivial
task, and I'm not sufficiently familiar with the NetworkDelegate to know whether
the added behaviour would be adding complexity down the road - that I defer to
Matt whether it's sufficiently "worth it" for this use case.

Given Matt's concerns, I'm OK with removing this pump - if/when this test fails,
we'll revisit it. Hopefully someone will keep an eye on it from being flaky,
since we no longer have tools that automatically inform us of this (e.g. by
breaking the tree)

[mmenke, 2015/03/10 17:52:33]

I'm not suggesting adding anything to the main NetworkDelegate class, just to
TestNetworkDelegate defined in this file.  Could just moving the callback we're
currently passing to TestReporter to TestNetworkDelegate, and call it on
destruction.  That would also solve the weirdness of OnResponseStarted, without
having to add a virtual method in production code that does nothing, just so the
test fixture can override it.  I like to keep production code as clean as we
reasonably can when it comes to extra stuff to help with testing.

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

Done -- the callback is now called from |OnURLRequestDestroyed|.

+
+  EXPECT_EQ(1, network_delegate_.GetNumRequests());
+  EXPECT_EQ(1, network_delegate_.GetNumDestroyedRequests());
+}
+
+// Test that a request that returns an error gets cleaned up.
+TEST_F(CertificateErrorReporterTest, ErroredRequestGetsDeleted) {
+  GURL url = net::URLRequestFailedJob::GetMockHttpsUrl(net::ERR_FAILED);
+  base::RunLoop run_loop;
+  base::Closure callback = run_loop.QuitClosure();
+
+  EXPECT_EQ(0, network_delegate_.GetNumRequests());
+  EXPECT_EQ(0, network_delegate_.GetNumDestroyedRequests());
+
+  TestReporter reporter(&context_, url, callback);
+  reporter.SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION,
+                      kHostname, GetSSLInfo());
+  run_loop.Run();
+
+  EXPECT_EQ(1, network_delegate_.GetNumRequests());
+  EXPECT_EQ(1, network_delegate_.GetNumDestroyedRequests());
+}
+

[mmenke, 2015/03/10 15:08:46]

Since we're using a std::set for requests, we should have a test that sends
multiple requests, ideally one test that does it sequentially and one that does
it simultaneously.

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

If it's okay with you, I'd like to do this in a follow-up CL, because: 1.
landing this CL will unblock a lot of other work and allow me to parallelize
more efficiently (e.g. writing these additional tests while waiting for code
reviews on other work), and 2. all these tests are for pre-existing code that
has been around for a long time without tests. I acknowledge that it's important
to have tests while moving this code around because it's possible that something
might have broken in the refactoring, so I do solemnly swear to prioritize them,
but I think it would be more efficient to do them in a separate CL. What do you
think?

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

If this were a CL adding a feature, I'd definitely want it in before landing. 
Since it's refactoring and adding tests to old code, I still think it's a good
idea, but not something I'm going to block this CL on (I'm also not really an
owner of any this code, so happily defer to Ryan's judgment, regardless).

[felt, 2015/03/11 15:51:23]

My $0.02: 
- This CL is blocking a lot of other work,
- This specific CL is a fairly straightforward refactor of old code; if the
tests reveal any problems, they will be pre-existing problems, and
- I trust Emily to write the tests in short order.

+}  // namespace
+
+namespace chrome_browser_net {

[mmenke, 2015/03/10 15:08:46]

Having a test in a separate namespace is weird.  Should just use using
directives, or write it out when needed.

[estark, 2015/03/10 15:54:15]

Sorry, don't follow. This test is in the chrome_browser_net namespace so that
CertificateErrorReporter can friend it. Is there an alternative way to do that?

[mmenke, 2015/03/10 16:08:52]

Sorry, didn't notice that.  I'm not a fan of friending tests.  Could make the
method protected, and then have the test subclass expose it.  I think it's
simpler than having an exhaustive list of tests in the public header, and it
also makes it clearer what internal state is poked at by tests, but Ryan may
disagree, and I'll defer to him here.

However...It's also worth noting this is most definitely not part of the public
interface.  I think it would make more sense to check the upload data this
results in us sending, rather than that private fields are populated as
expected.

[Ryan Sleevi, 2015/03/10 17:39:30]

On 2015/03/10 16:08:52, mmenke wrote: 
Sorry for the mixed messaging :) I do tend to disagree; I've seen too much
subclassing when made protected, so strongly prefer friending tests (or test
fixtures, which is the other way to avoid having to exhaustively enumerate all
tests.) I usually use 3 as the rule - if I'm having to friend 3 tests, I should
really be friending the fixture itself and then let the fixture marshal that
method for the concrete tests.
That seems quite reasonable, and decouples further.

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

Done -- added a check that the uploaded report is as expected.

+
+// Test that a built report has the correct data.
+//
+// This test is in the chrome_browser_net namespace so that it can be
+// friended by CertificateErrorReporter.
+TEST_F(CertificateErrorReporterTest, ReportIsBuiltCorrectly) {
+  SSLInfo info = GetSSLInfo();
+  CertLoggerRequest request;
+  CertificateErrorReporter::BuildReport(kHostname, info, &request);
+
+  EXPECT_EQ(request.hostname(), kHostname);
+  EXPECT_EQ(request.cert_chain(), GetPEMEncodedChain());
+  EXPECT_EQ(request.pin().size(), 1);
+  EXPECT_EQ(request.pin().Get(0), kDummyFailureLog);
+}
+}  // namespace chrome_browser_net