Add CheckOCSPDateValid() to net/cert/internal

net/cert/internal/parse_ocsp_unittest.cc

 // Copyright 2016 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 "net/cert/internal/parse_ocsp.h"
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "net/cert/internal/test_helpers.h"
 #include "net/cert/x509_certificate.h"
+#include "net/der/encode_values.h"
 #include "net/test/test_data_directory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 
 namespace {
 
+const base::TimeDelta kOCSPAgeOneWeek = base::TimeDelta::FromDays(7);
+
 std::string GetFilePath(const std::string& file_name) {
   return std::string("net/data/parse_ocsp_unittest/") + file_name;
 }
 
 enum OCSPFailure {
   OCSP_SUCCESS,
   PARSE_CERT,
   PARSE_OCSP,
   OCSP_NOT_SUCCESSFUL,
   PARSE_OCSP_DATA,
@@ skipping 147 matching lines @@
 }
 
 TEST(ParseOCSPTest, OCSPOCSPSingleExtension) {
   ASSERT_EQ(OCSP_SUCCESS, ParseOCSP("has_single_extension.pem"));
 }
 
 TEST(ParseOCSPTest, OCSPMissingResponse) {
   ASSERT_EQ(PARSE_OCSP_SINGLE_RESPONSE, ParseOCSP("missing_response.pem"));
 }
 
+TEST(OCSPDateTest, Valid) {
+  OCSPSingleResponse response;
+  base::Time now = base::Time::Now();
+  base::Time this_update = now - base::TimeDelta::FromHours(1);
+  base::Time next_update = this_update + base::TimeDelta::FromDays(7);
+  response.this_update = der::EncodeTimeAsGeneralizedTime(this_update);
+  response.has_next_update = false;
+  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+  response.has_next_update = true;
+  response.next_update = der::EncodeTimeAsGeneralizedTime(next_update);
+  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));

[Ryan Sleevi, 2016/07/01 00:49:53]

Reading each of these tests, I find it visually cluttered to try to figure out
what's being tested and the core logic and checking that the expectations match
what's claimed (and the claim is in the description)

Concrete structural suggestions:
- Declare your variables close to the first usage ( see
https://google.github.io/styleguide/cppguide.html#Local_Variables )

In particular, next_update isn't used until 197, so it's declaration ideally
would be before 196 or before 197

- Make use of vertical whitespace (
https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace )

There's a tension here between min/maxing whitespace, but the goal should be
readability.

When I look at these tests in the codereview screen, they're same font, same
color - assume no syntax-highlighting. So are they all one big test?

Well, no, they tend to be testing in two lumps - without NextUpdate and with
NextUpdate. To me, this seems to suggest that there should be a line-break
between 195 and 196 (and 208/209, 221/222, etc)

+}
+
+TEST(OCSPDateTest, ThisUpdateInTheFuture) {
+  OCSPSingleResponse response;
+  base::Time now = base::Time::Now();
+  base::Time this_update = now + base::TimeDelta::FromHours(1);
+  base::Time next_update = this_update + base::TimeDelta::FromDays(7);
+  response.this_update = der::EncodeTimeAsGeneralizedTime(this_update);
+  response.has_next_update = false;
+  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+  response.has_next_update = true;
+  response.next_update = der::EncodeTimeAsGeneralizedTime(next_update);
+  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+}
+
+TEST(OCSPDateTest, NextUpdatePassed) {
+  OCSPSingleResponse response;
+  base::Time now = base::Time::Now();
+  base::Time this_update = now - base::TimeDelta::FromDays(6);
+  base::Time next_update = now - base::TimeDelta::FromHours(1);
+  response.this_update = der::EncodeTimeAsGeneralizedTime(this_update);
+  response.has_next_update = false;
+  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+  response.has_next_update = true;
+  response.next_update = der::EncodeTimeAsGeneralizedTime(next_update);
+  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+}
+
+TEST(OCSPDateTest, NextUpdateBeforeThisUpdate) {
+  OCSPSingleResponse response;
+  base::Time now = base::Time::Now();
+  base::Time next_update = now - base::TimeDelta::FromHours(1);
+  base::Time this_update = next_update + base::TimeDelta::FromDays(7);
+  response.this_update = der::EncodeTimeAsGeneralizedTime(this_update);
+  response.has_next_update = false;
+  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));

[Ryan Sleevi, 2016/07/01 00:49:52]

When visually scanning, this tripped me up.

That's because you've swapped your declaration order and dependency
relationship, and thus the pattern matching I acquired on the above lines is now
broken.

That's because I *thought* your |this_update| was now -
base::TimeDelta::FromHours(1) - but no, you swapped their definitions (because
you made them relative to eachother)

You then swap back in the next test (243-244)

My concrete suggestion would be:
base::Time now = base::Time::Now();
base::Time this_update = now + base::TimeDelta::FromDays(7);
response.this_update = ...
response.has_next_update = false;
EXPECT_FALSE(...);

base::Time next_update = this_update - base::TimeDelta::FromHours(1);
response ...
...
EXPECT_FALSE(...)

[Ryan Sleevi, 2016/07/01 00:49:53]

This seems like it's testing something other than what the description says - is
that the case? Why should this be false?

AFAICT, you're testing ThisUpdateOlderThanMaxAge here - shouldn't this be a
valid this_update, but an earlier next_update, in line with the description,
since you have the ThisUpdate test on line 240?

[dadrian, 2016/07/01 17:47:38]

Done.

+  response.has_next_update = true;
+  response.next_update = der::EncodeTimeAsGeneralizedTime(next_update);
+  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+}
+
+TEST(OCSPDateTest, ThisUpdateOlderThanMaxAge) {
+  OCSPSingleResponse response;
+  base::Time now = base::Time::Now();
+  base::Time this_update = now - kOCSPAgeOneWeek;
+  base::Time next_update = now + base::TimeDelta::FromHours(1);
+  response.has_next_update = false;
+  response.this_update = der::EncodeTimeAsGeneralizedTime(this_update);
+  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+  response.this_update = der::EncodeTimeAsGeneralizedTime(
+      this_update - base::TimeDelta::FromSeconds(1));
+  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+  response.has_next_update = true;
+  response.next_update = der::EncodeTimeAsGeneralizedTime(next_update);
+  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
+}
+
 }  // namespace net