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/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/run_loop.h"
+#include "chrome/browser/net/cert_logger.pb.h"
+#include "chrome/browser/net/url_request_mock_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread.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_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[] = "test_mail_google_com.pem";

[Ryan Sleevi, 2015/03/09 22:14:06]

const base::FilePath::CharType kTestCertFilename[] =
FILE_PATH_LITERAL("test_mail_google.com.pem")

[estark, 2015/03/09 23:21:17]

Done.

+
+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() {

[Ryan Sleevi, 2015/03/09 22:14:07]

This seems to indirectly couple the test with the implementation of
X509Certificate::GetPEMEncodedChain()

That is, there is no strong API guarantee that the ingested format will be the
same as the output format.

What are you exactly trying to test here? That it produces a known-gold output?
Or merely that the certificate(s) are present in the encoded response?

[estark, 2015/03/09 23:21:17]

Well, I'd like to test that the output is in the format that the server expects
it. So that is more like known-gold I suppose. Does that mean I should paste in
the exact data I expect here?

[Ryan Sleevi, 2015/03/09 23:40:36]

I think it's fine; in that case, however, you should make sure the documentation
for test_mail_google_com.pem specifies that the format is intentional (usually
in the README file), since normally we provide diagnostic output in the .pem
files to make codesearch easier.

[estark, 2015/03/10 00:07:38]

Done.

+  base::FilePath cert_path =
+      net::GetTestCertsDirectory().AppendASCII(kTestCertFilename);
+  std::string cert_data;
+  EXPECT_TRUE(base::ReadFileToString(cert_path, &cert_data));
+  return cert_data;
+}
+
+// A network delegate that lets tests check that requests were created
+// or destroyed.
+class TestNetworkDelegate : public NetworkDelegateImpl {
+ public:
+  TestNetworkDelegate() : NetworkDelegateImpl() {
+    requests_ = 0;
+    destroyed_requests_ = 0;
+  }
+  ~TestNetworkDelegate() {}
+
+  int OnBeforeURLRequest(URLRequest* request,
+                         const CompletionCallback& callback,
+                         GURL* new_url) override {
+    requests_++;
+    return net::OK;
+  }
+
+  void OnURLRequestDestroyed(URLRequest* request) { destroyed_requests_++; }
+
+  int GetNumRequests() { return requests_; }
+  int GetNumDestroyedRequests() { return destroyed_requests_; }
+
+ private:
+  int requests_;
+  int destroyed_requests_;
+};
+
+class CertificateErrorReporterTest : public ::testing::Test {
+ public:
+  ~CertificateErrorReporterTest() {
+    chrome_browser_net::SetUrlRequestMocksEnabled(false);
+  }
+
+ protected:
+  CertificateErrorReporterTest()
+      : context_(true), io_thread_(BrowserThread::IO, &message_loop_) {
+    chrome_browser_net::SetUrlRequestMocksEnabled(true);
+    context_.set_network_delegate(&network_delegate_);
+    context_.Init();
+  }
+
+  TestURLRequestContext context_;
+  TestNetworkDelegate network_delegate_;
+  base::MessageLoopForUI message_loop_;

[Ryan Sleevi, 2015/03/09 22:14:07]

This is usually a warning sign. I believe you probably want a
TestBrowserThreadBundle

[estark, 2015/03/09 23:21:17]

Done.

+  content::TestBrowserThread io_thread_;
+};
+
+class TestReporter : public CertificateErrorReporter {
+ public:
+  TestReporter(net::URLRequestContext* context, const GURL& upload_url)
+      : CertificateErrorReporter(context, upload_url), latest_request_url_() {}
+
+  void OnResponseStarted(net::URLRequest* request) override {
+    latest_request_url_ = request->url();
+    CertificateErrorReporter::OnResponseStarted(request);

[Ryan Sleevi, 2015/03/09 22:14:06]

Do you actually need to call this for your unit tests to be correct?

[estark, 2015/03/09 23:21:17]

I think so, at least for some of them.
|CertificateErrorReporter::OnResponseStarted| calls |RequestComplete| which is
what cleans up the request, which is what one of the tests checks for.

[Ryan Sleevi, 2015/03/09 23:40:36]

So from a design perspective then, it's usually seen as too much coupling when
you have to call a base class's behaviour from a derived class. That is, if you
have behaviour that is common for a base class, but then derived classes may
override just a subset of that, then another strategy is to have
protected-virtual methods that derived classes can override, but are called from
the base class's concrete implementation.

An example (albeit perhaps unnecessary complex) is to consider
CertVerifyProc::Verify and CertVerifyProc::VerifyInternal. Verify() encompasses
the common functionality of the base class, while VerifyInternal is left for
derived classes to implement the necessary concrete functionality.

Any time you see code that directly calls a method on its parent class, it's
usually a sign that the base class is exposing the wrong interface / the derived
class is doing something in an error prone way.

(A more complex scenario is to do something like URLRequest, which allows
Delegate()s to implement the concrete derived behaviour, but then have all the
base behaviour itself. When it wants to allow a "derived" thing to override
behaviour, it calls into to the Delegate. However, I'm not sure that's necessary
here)

For testing code, I'm not sure if the readability hit of "doing it right" is
worth the cost, and so I think I'm OK with this code as is, with the caveat of
everything I mentioned above. However, Matt may feel differently / have more to
add here.

+    run_loop_->Quit();
+  }
+
+  void Run() {
+    run_loop_.reset(new base::RunLoop());
+    run_loop_->Run();

[Ryan Sleevi, 2015/03/09 22:14:07]

If the response is never started, this will never quit, will it?

Note: I generally dislike putting RunLoops in anything other than the test /
it's fixture. It becomes extremely easy to accidentally nest RunLoops when
putting them in dependent objects and, while 'supported', can end up hiding
subtle bugs.

For example, you could mitigate this by passing in a QuitClosure to be post when
OnResponseStarted() is executed, such that it becomes

TestReporter(net::URLRequestContext* context, const GURL& upload_url, const
base::Closure& ...) {

  void OnResponseStarted(...) override {
    latest_request_url_ = request->url();
    base::ThreadTaskRunnerHandle::Get()->PostTask(closure_);
  }
}

and move the base::RunLoop into the test themselves (or the wrapped in the test
fixture somehow)

[estark, 2015/03/09 23:21:17]

Is there any way to avoid that? It seems like a fundamental problem of testing
async code.
Done.

[Ryan Sleevi, 2015/03/09 23:40:36]

RunUntilIdle() is usually a good way to avoid this problem.

For other tests, sometimes it helps to setup a Timer to force-quit. Usually I
just make sure people think about it and document the expectations and hope
someone yells if/when this test starts flaking (by hanging)

[estark, 2015/03/10 00:07:38]

So are you saying that all the `run_loop.Run`s should be
`run_loop.RunUntilIdle`s? That doesn't seem to work for SendReportSendsRequest;
when I change it to run_loop.RunUntilIdle() the test fails because the EXPECTs
run before |OnResponseStarted| runs. According to my understanding from what you
said below about TestBrowserThreadBundle, I would have thought that RunUntilIdle
would work...

[mmenke, 2015/03/10 14:25:46]

I think RunUntilIdle() is a bit of an anti-pattern.  You're depending on the
underlying code to do all its work on the current thread, posting tasks without
delay, which is not actually a part of the documented URLRequest interface, and
is not a property of all URLRequest types.  Changes in net/ can break this
assumption, and would require updating all these tests.  I've run into this
problem in the past, and would prefer to minimize the chance of it happening
again.

It's much more robust to just wait until something happens.  This does result in
tests timing out on failure, but I think that's better than tests breaking when
net/ code changes, particularly in ways that don't actually require API doc
updates.  Also worth noting that if too many tests timeout (And maybe fail as
well, not sure), buildbots will abort a test run, so 1000 broken tests all
taking 30 seconds to timeout won't break the entire testing infrastructure.

+  }
+
+  GURL& GetLatestRequestURL() { return latest_request_url_; }
+
+ private:
+  scoped_ptr<base::RunLoop> run_loop_;
+  GURL latest_request_url_;
+};
+
+// Test that CertificateErrorReporter::SendReport creates a URLRequest
+// for the endpoint.
+TEST_F(CertificateErrorReporterTest, SendReportSendsRequest) {
+  GURL url = net::URLRequestMockHTTPJob::GetMockHttpsUrl(
+      base::FilePath(FILE_PATH_LITERAL("empty.html")));
+  TestReporter reporter(&context_, url);
+
+  EXPECT_EQ(0, network_delegate_.GetNumRequests());
+
+  reporter.SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION,
+                      kHostname, GetSSLInfo());
+  reporter.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) {
+  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;

[Ryan Sleevi, 2015/03/09 22:14:06]

This seems to couple tightly with the RunLoop integration. That is, it assumes
that SendReport() will never PostTask to ensure its cleanup of things.

by foisting the base::RunLoop out, you could do something like RunUntilIdle() to
make sure any cleanup tasks are appropriately executed before testing that
cleanup happened.

[estark, 2015/03/09 23:21:17]

I need some clarification on this one. IIUC RunUntilIdle() will ensure that
there are no cleanup tasks pending on the current thread, but the reporter could
post a task to a different thread to do its cleanup, right? (Or post a task to a
different thread which posts one back to the original thread that does the
cleanup, or something like that.) Would RunUntilIdle() be sufficient in that
case?

Perhaps I should add a |cleanup_callback_for_testing_| to
CertificateErrorReporter?

Moreover this seems to apply to the next test as well
(ErroredRequestGetsDeleted), i.e. who's to say that SendReport doesn't wait 10
minutes before cleaning up completed requests.

[Ryan Sleevi, 2015/03/09 23:40:36]

Well, if you're using a TestBrowserThreadBundle, it has the nice (intentional)
side-effect of running all the threads on the same message loop, thus
RunUntilIdle covers all of these interactions quite nicely. It's why using TBTB
is nice :) [ISTR it also has helpers to wait until child threads are pumped, if
you do need to force distinct/native threads]
Yeah, but I'm willing to live with that, because I'm a hypocrite and only yell
about the tightest of tight coupling and leave the simpler/more annoying stuff
as a "Meh" ;)

[estark, 2015/03/10 00:07:38]

Ah, I see. Done.

+
+  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);
+
+  EXPECT_EQ(0, network_delegate_.GetNumRequests());
+  EXPECT_EQ(0, network_delegate_.GetNumDestroyedRequests());
+
+  TestReporter reporter(&context_, url);
+  reporter.SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION,
+                      kHostname, GetSSLInfo());
+  reporter.Run();
+
+  EXPECT_EQ(1, network_delegate_.GetNumRequests());
+  EXPECT_EQ(1, network_delegate_.GetNumDestroyedRequests());
+}
+
+}  // namespace
+
+namespace chrome_browser_net {
+
+// 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