WKWebView: Added cert verification API to web controller.

ios/web/net/cert_verifier_block_adapter_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 "ios/web/net/cert_verifier_block_adapter.h"
 
 #include "base/test/ios/wait_util.h"
 #include "net/base/net_errors.h"
+#include "net/cert/cert_verifier.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/cert/crl_set.h"
 #include "net/cert/x509_certificate.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/platform_test.h"
 
 namespace net {
 
 using testing::_;
 
 namespace {
 
 // Test hostname for CertVerifier.
 const char kHostName[] = "chromium.org";
 // Test OCSP response for CertVerifier.
 const char kOcspResponse[] = "ocsp";
 
 // Mocks CertVerifier for CertVerifierBlockAdapter testing.
 class CertVerifierMock : public CertVerifier {
  public:
   MOCK_METHOD9(Verify,

[Ryan Sleevi, 2015/08/07 21:52:12]

There's a strong anti-GMock position from Chrome leads (darin@, brettw@, jam@)

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/-KH_IP0rIWQ/HynAL...

We also already have a MockCertVerifier (
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/mock_cert...
) that is the preferred method, although I suppose it's more a Fake than a Mock.

[Eugene But (OOO till 7-30), 2015/08/12 22:00:38]

MockCertVerifier does not allow to test OCSP and CRL inputs, as well as
asynchronous verification.
Switching this test to use MockCertVerifier will lead to loosing significant
test coverage.

Having said that I believe that gMock is better option comparing to
MockCertVerifier, even if in the future we'll need to replace gMock with
something different.

[Ryan Sleevi, 2015/08/14 02:29:43]

I'm sorry, I simply have trouble parsing this. I haven't seen any of your tests
using that.
I think you missed the point of those threads. There was strong disagreement
that using GMock is a better option, from many of the senior Chrome leads.

[Eugene But (OOO till 7-30), 2015/08/14 21:18:19]

I've tried using MockCertVerifier for testing CertVerifierBlockAdapter but for
this specific class MockCertVerifier does not allow to test the following
scenarios:

1.) Async Cert Evaluation:
In this file tested by DefaultParamsAndAsync and DefaultParamsAndAsyncError.
MockCertVerifier emulates only synchronous response
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/mock_cert...

2.) CRL_Set, OCSP response and flags:
In this file tested by AllParamsAndSync.
MockCertVerifier does not use passed |ocsp_response|, |crl_set| and |flags|
arguments.

While I guess loosing coverage for CRL_Set, OCSP response and flags is
acceptable, I strongly believe that testing async evaluation in this unit test
is necessary. How about I extend MockCertVerifier to support async evaluation
and then I will rewrite this test file to use MockCertVerifier?

[Ryan Sleevi, 2015/08/14 21:43:53]

So, I can understand and appreciate that, but that you're testing that at all is
part of why there's such a strong opposition to GMock in Chromium - because
people end up using it to test interactions, rather than state.

If you look on the internal Google resources for "mocks fakes stubs", you can
see the best practices guide for each, including the admonitions against testing
interactions.

[Eugene But (OOO till 7-30), 2015/08/19 17:57:35]

Ryan, I my previous comment I did not argue that we should continue using gmock.
I'm trying to keep coverage for async verification and using MockCertVerifier as
it is will make this test case very weak.

For the sake of moving forward I removed gmock usage. For async scenario testing
I'm using real net::CertVerifier (test takes 12ms) and for the remaining
scenarios I use MockCertVerifier.

                int(X509Certificate* cert,
                    const std::string& hostname,
                    const std::string& ocsp_response,
                    int flags,
                    CRLSet* crl_set,
                    CertVerifyResult* verify_result,
                    const CompletionCallback& callback,
                    scoped_ptr<Request>* out_req,
                    const BoundNetLog& net_log));
 };
 
 // Sets CertVerifyResult to emulate CertVerifier behavior.
 ACTION_P(SetVerifyResult, result) {
   *arg5 = result;
 }
 
 // Calls CompletionCallback to emulate CertVerifier behavior.
-ACTION(RunCallback) {
-  arg6.Run(0);
+ACTION_P(RunCallback, status) {
+  CompletionCallback callback = arg6;
+  dispatch_async(dispatch_get_main_queue(), ^{
+    callback.Run(status);
+  });

[Ryan Sleevi, 2015/08/07 21:52:12]

This doesn't seem to be emulating CertVerifier behaviour, but your Adapter's
behaviour.

CertVerifier would use

base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, base::Bind(arg6,
status));

[Eugene But (OOO till 7-30), 2015/08/12 22:00:38]

Done.

 }
 
 }  // namespace
 
 // Test fixture to test CertVerifierBlockAdapter class.
 class CertVerifierBlockAdapterTest : public PlatformTest {
  protected:
-  void SetUp() override {
-    PlatformTest::SetUp();
-
-    cert_ = new X509Certificate("test", "test", base::Time(), base::Time());
-    scoped_ptr<CertVerifierMock> cert_verifier_mock(new CertVerifierMock());
-    cert_verifier_mock_ = cert_verifier_mock.get();
-    test_adapter_.reset(
-        new CertVerifierBlockAdapter(cert_verifier_mock.Pass()));
-  }
+  CertVerifierBlockAdapterTest()
+      : cert_(new X509Certificate("test", "test", base::Time(), base::Time())),

[Ryan Sleevi, 2015/08/07 21:52:12]

You should not create certificates like this for tests.

This is a valid-only-for-certain-minimal-cases ctor that isn't compatible
(because it creates an invalid OSCertHandle). You should use ImportCertFromFile
to create a real X509Certificate.

[Eugene But (OOO till 7-30), 2015/08/12 22:00:38]

Done.

+        test_adapter_(
+            new CertVerifierBlockAdapter(&cert_verifier_mock_, &net_log_)) {}
 
   // Performs synchronous verification.
   void Verify(CertVerifierBlockAdapter::Params params,
-              scoped_ptr<net::CertVerifyResult>* result,
+              net::CertVerifyResult* result,
               int* status) {
     __block bool verification_completed = false;
-    test_adapter_->Verify(params,
-                          ^(scoped_ptr<net::CertVerifyResult> callback_result,
-                            int callback_status) {
-                            *result = callback_result.Pass();
-                            *status = callback_status;
-                            verification_completed = true;
-                          });
+    test_adapter_->Verify(
+        params, ^(net::CertVerifyResult callback_result, int callback_status) {
+          *result = callback_result;
+          *status = callback_status;
+          verification_completed = true;
+        });
     base::test::ios::WaitUntilCondition(^{
       return verification_completed;
     });
   }
 
   // Fake certificate created for testing.
   scoped_refptr<X509Certificate> cert_;
+  // CertVerifier mock.
+  CertVerifierMock cert_verifier_mock_;
+  // NetLog object required by CertVerifierBlockAdapter.
+  NetLog net_log_;
   // Testable |CertVerifierBlockAdapter| object.
   scoped_ptr<CertVerifierBlockAdapter> test_adapter_;
-  // CertVerifier mock owned by |test_adapter_|.
-  CertVerifierMock* cert_verifier_mock_;
 };
 
 // Tests |Verify| with default params and synchronous verification.
 TEST_F(CertVerifierBlockAdapterTest, DefaultParamsAndSync) {
   // Set up expectation.
   net::CertVerifyResult expectedResult;
   expectedResult.cert_status = net::CERT_STATUS_AUTHORITY_INVALID;
-  const int kExpectedStatus = 0;
-  EXPECT_CALL(*cert_verifier_mock_,
+  const int kExpectedStatus = OK;
+  EXPECT_CALL(cert_verifier_mock_,
               Verify(cert_.get(), kHostName, "", 0, nullptr, _, _, _, _))

[Ryan Sleevi, 2015/08/07 21:52:12]

These "" should be std::string() as well.

[Eugene But (OOO till 7-30), 2015/08/12 22:00:38]

Done.

       .Times(1)
       .WillOnce(testing::DoAll(SetVerifyResult(expectedResult),
                                testing::Return(kExpectedStatus)));
 
   // Call |Verify|.
-  scoped_ptr<CertVerifyResult> actualResult;
+  CertVerifyResult actualResult;
   int actualStatus = -1;
   CertVerifierBlockAdapter::Params params(cert_.get(), kHostName);
   Verify(params, &actualResult, &actualStatus);
 
   // Ensure that Verification results are correct.
   EXPECT_EQ(kExpectedStatus, actualStatus);
-  EXPECT_EQ(expectedResult.cert_status, actualResult->cert_status);
+  EXPECT_EQ(expectedResult.cert_status, actualResult.cert_status);
 }
 
 // Tests |Verify| with default params and asynchronous verification.
 TEST_F(CertVerifierBlockAdapterTest, DefaultParamsAndAsync) {
   // Set up expectation.
   net::CertVerifyResult expectedResult;
   expectedResult.is_issued_by_known_root = true;
-  const int kExpectedStatus = 0;
-  EXPECT_CALL(*cert_verifier_mock_,
+  const int kExpectedStatus = OK;
+  EXPECT_CALL(cert_verifier_mock_,
               Verify(cert_.get(), kHostName, "", 0, nullptr, _, _, _, _))
       .Times(1)
-      .WillOnce(testing::DoAll(SetVerifyResult(expectedResult), RunCallback(),
+      .WillOnce(testing::DoAll(SetVerifyResult(expectedResult),
+                               RunCallback(kExpectedStatus),
                                testing::Return(ERR_IO_PENDING)));
 
   // Call |Verify|.
-  scoped_ptr<CertVerifyResult> actualResult;
+  CertVerifyResult actualResult;
   int actualStatus = -1;
   CertVerifierBlockAdapter::Params params(cert_.get(), kHostName);
   Verify(params, &actualResult, &actualStatus);
 
   // Ensure that Verification results are correct.
   EXPECT_EQ(kExpectedStatus, actualStatus);
   EXPECT_EQ(expectedResult.is_issued_by_known_root,
-            actualResult->is_issued_by_known_root);
+            actualResult.is_issued_by_known_root);
 }
 
-// Tests |Verify| with invalid arguments.
-TEST_F(CertVerifierBlockAdapterTest, InvalidParamsAndError) {
-  // Set up expectation.
-  const int kExpectedStatus = ERR_INVALID_ARGUMENT;
-  EXPECT_CALL(*cert_verifier_mock_,
-              Verify(nullptr, "", "", 0, nullptr, _, _, _, _))
-      .Times(1)
-      .WillOnce(testing::Return(kExpectedStatus));
-
+// Tests |Verify| with invalid cert argument.
+TEST_F(CertVerifierBlockAdapterTest, InvalidCert) {
   // Call |Verify|.
-  scoped_ptr<CertVerifyResult> actualResult;
+  CertVerifyResult actualResult;
   int actualStatus = -1;
-  CertVerifierBlockAdapter::Params params(nullptr, "");
+  CertVerifierBlockAdapter::Params params(nullptr, kHostName);
   Verify(params, &actualResult, &actualStatus);
 
   // Ensure that Verification results are correct.
-  EXPECT_EQ(kExpectedStatus, actualStatus);
-  EXPECT_FALSE(actualResult);
+  EXPECT_EQ(ERR_INVALID_ARGUMENT, actualStatus);
 }
 
-// Tests |Verify| with error.
-TEST_F(CertVerifierBlockAdapterTest, DefaultParamsAndError) {
+// Tests |Verify| with invalid hostname argument.
+TEST_F(CertVerifierBlockAdapterTest, InvalidHostname) {
+  // Call |Verify|.
+  CertVerifyResult actualResult;
+  int actualStatus = -1;
+  CertVerifierBlockAdapter::Params params(cert_.get(), "");

[Ryan Sleevi, 2015/08/07 21:52:12]

std::string()

[Eugene But (OOO till 7-30), 2015/08/12 22:00:38]

Done.

+  Verify(params, &actualResult, &actualStatus);
+
+  // Ensure that Verification results are correct.
+  EXPECT_EQ(ERR_INVALID_ARGUMENT, actualStatus);
+}
+
+// Tests |Verify| with synchronous error.
+TEST_F(CertVerifierBlockAdapterTest, DefaultParamsAndSyncError) {
   // Set up expectation.
   const int kExpectedStatus = ERR_INSUFFICIENT_RESOURCES;
-  EXPECT_CALL(*cert_verifier_mock_,
+  EXPECT_CALL(cert_verifier_mock_,
               Verify(cert_.get(), kHostName, "", 0, nullptr, _, _, _, _))
       .Times(1)
       .WillOnce(testing::Return(kExpectedStatus));
 
   // Call |Verify|.
-  scoped_ptr<CertVerifyResult> actualResult;
+  CertVerifyResult actualResult;
   int actualStatus = -1;
   CertVerifierBlockAdapter::Params params(cert_.get(), kHostName);
   Verify(params, &actualResult, &actualStatus);
 
   // Ensure that Verification results are correct.
   EXPECT_EQ(kExpectedStatus, actualStatus);
-  EXPECT_FALSE(actualResult);
+}
+
+// Tests |Verify| with asynchronous error.
+TEST_F(CertVerifierBlockAdapterTest, DefaultParamsAndAsyncError) {
+  // Set up expectation.
+  net::CertVerifyResult expectedResult;
+  expectedResult.is_issued_by_known_root = true;
+  const int kExpectedStatus = ERR_ACCESS_DENIED;
+  EXPECT_CALL(cert_verifier_mock_,
+              Verify(cert_.get(), kHostName, "", 0, nullptr, _, _, _, _))
+      .Times(1)
+      .WillOnce(testing::DoAll(SetVerifyResult(expectedResult),
+                               RunCallback(kExpectedStatus),
+                               testing::Return(ERR_IO_PENDING)));
+
+  // Call |Verify|.
+  CertVerifyResult actualResult;
+  int actualStatus = -1;
+  CertVerifierBlockAdapter::Params params(cert_.get(), kHostName);
+  Verify(params, &actualResult, &actualStatus);
+
+  // Ensure that Verification results are correct.
+  EXPECT_EQ(kExpectedStatus, actualStatus);
+  EXPECT_EQ(expectedResult.is_issued_by_known_root,
+            actualResult.is_issued_by_known_root);
 }
 
 // Tests |Verify| with all params and synchronous verification.
 TEST_F(CertVerifierBlockAdapterTest, AllParamsAndSync) {
   // Set up expectation.
   net::CertVerifyResult expectedResult;
   expectedResult.verified_cert = cert_;
-  const int kExpectedStatus = 0;
+  const int kExpectedStatus = OK;
   scoped_refptr<CRLSet> crl_set(CRLSet::EmptyCRLSetForTesting());
-  EXPECT_CALL(*cert_verifier_mock_,
+  EXPECT_CALL(cert_verifier_mock_,
               Verify(cert_.get(), kHostName, kOcspResponse,
                      CertVerifier::VERIFY_EV_CERT, crl_set.get(), _, _, _, _))
       .Times(1)
       .WillOnce(testing::DoAll(SetVerifyResult(expectedResult),
                                testing::Return(kExpectedStatus)));
 
   // Call |Verify|.
-  scoped_ptr<CertVerifyResult> actualResult;
+  CertVerifyResult actualResult;
   int actualStatus = -1;
   CertVerifierBlockAdapter::Params params(cert_.get(), kHostName);
   params.ocsp_response = kOcspResponse;
   params.flags = CertVerifier::VERIFY_EV_CERT;
   params.crl_set = crl_set;
   Verify(params, &actualResult, &actualStatus);
 
   // Ensure that Verification results are correct.
   EXPECT_EQ(kExpectedStatus, actualStatus);
-  EXPECT_EQ(expectedResult.verified_cert, actualResult->verified_cert);
+  EXPECT_EQ(expectedResult.verified_cert, actualResult.verified_cert);
 }
 
 }  // namespace