Remove the "not_after" validation of policy timestamps

components/policy/core/common/cloud/cloud_policy_validator_unittest.cc

Index: components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
diff --git a/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc b/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
index c3112952ebe1ee86a0c62f24d2bb896bddd3630f..a0cc84d37163ceb8d993a3d8e2df9c11a8aafef9 100644
--- a/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
+++ b/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
@@ -43,10 +43,10 @@ ACTION_P(CheckStatus, expected_status) {
 class CloudPolicyValidatorTest : public testing::Test {
  public:
   CloudPolicyValidatorTest()
-      : timestamp_(base::Time::UnixEpoch() +
-                   base::TimeDelta::FromMilliseconds(
-                       PolicyBuilder::kFakeTimestamp)),
-        timestamp_option_(CloudPolicyValidatorBase::TIMESTAMP_FULLY_VALIDATED),
+      : timestamp_(
+            base::Time::UnixEpoch() +
+            base::TimeDelta::FromMilliseconds(PolicyBuilder::kFakeTimestamp)),

[Thiemo Nagel, 2017/04/19 11:43:09]

Nit: Using FromJsTime(...::kFakeTimestamp) would allow dropping the UnixEpoch()
summand.

[emaxx, 2017/04/19 20:48:45]

Thanks for pointing at the already existing primitive for doing this kind of
conversion. But I don't like it a little bit as they are called "JsTime" or
"JavaTime", which has no semantic connection with the policy protocol (note that
the similar conversions are performed in a number of policy-related source
files).

I actually had a different idea: to introduce a special pair of functions
somewhere under policy/core/common/ that would perform the conversion between
policy timestamps and base::Time. WDYT? (Of course, then it will be done in a
separate CL.)

[Thiemo Nagel, 2017/04/20 10:08:27]

Imho that would be unnecessary overhead.  The server is Java code and by virtue
of that is using Java timestamps thus using Java conversion functions on the
client to me seems exactly the right way to do it.

How about documenting the use of Java time more explicitly in the proto file,
e.g. instead of

"[timestamp] is milliseconds since Epoch in UTC timezone."

maybe write something like

"[timestamp] is milliseconds since Epoch in UTC timezone (Java time)."

?

+        timestamp_option_(CloudPolicyValidatorBase::TIMESTAMP_VALIDATED),
         dm_token_option_(CloudPolicyValidatorBase::DM_TOKEN_REQUIRED),
         device_id_option_(CloudPolicyValidatorBase::DEVICE_ID_REQUIRED),
         allow_key_rotation_(true),
@@ -86,8 +86,7 @@ class CloudPolicyValidatorTest : public testing::Test {
 
     UserCloudPolicyValidator* validator = UserCloudPolicyValidator::Create(
         std::move(policy_response), base::ThreadTaskRunnerHandle::Get());
-    validator->ValidateTimestamp(timestamp_, timestamp_,
-                                 timestamp_option_);
+    validator->ValidateTimestamp(timestamp_, timestamp_option_);
     validator->ValidateUsername(PolicyBuilder::kFakeUsername, true);
     if (!owning_domain_.empty())
       validator->ValidateDomain(owning_domain_);
@@ -176,6 +175,14 @@ TEST_F(CloudPolicyValidatorTest, SuccessfulRunValidationWithNoDeviceId) {
   Validate(Invoke(this, &CloudPolicyValidatorTest::CheckSuccessfulValidation));
 }
 
+TEST_F(CloudPolicyValidatorTest,
+       SuccessfulRunValidationWithTimestampFromTheFuture) {
+  base::Time timestamp(timestamp_ + base::TimeDelta::FromMinutes(5));

[Thiemo Nagel, 2017/04/19 11:43:09]

Nit: Suggest to use FromHours(3) so that the test would fail against the old
version of the code.

[emaxx, 2017/04/19 20:48:45]

Done.

+  policy_.policy_data().set_timestamp(
+      (timestamp - base::Time::UnixEpoch()).InMilliseconds());

[Thiemo Nagel, 2017/04/19 11:43:09]

Nit: Using timestamp.ToJavaTime() would seem cleaner to me.

[emaxx, 2017/04/19 20:48:45]

Same as above: do you agree with deferring this for a follow-up CL that would
introduce special helpers under the policy/ source tree?

[Thiemo Nagel, 2017/04/20 10:08:27]

Sure!

+  Validate(CheckStatus(CloudPolicyValidatorBase::VALIDATION_OK));
+}
+
 TEST_F(CloudPolicyValidatorTest, UsernameCanonicalization) {
   policy_.policy_data().set_username(
       base::ToUpperASCII(PolicyBuilder::kFakeUsername));
@@ -210,22 +217,6 @@ TEST_F(CloudPolicyValidatorTest, ErrorOldTimestamp) {
   Validate(CheckStatus(CloudPolicyValidatorBase::VALIDATION_BAD_TIMESTAMP));
 }
 
-TEST_F(CloudPolicyValidatorTest, ErrorTimestampFromTheFuture) {
-  base::Time timestamp(timestamp_ + base::TimeDelta::FromHours(3));
-  policy_.policy_data().set_timestamp(
-      (timestamp - base::Time::UnixEpoch()).InMilliseconds());
-  Validate(CheckStatus(CloudPolicyValidatorBase::VALIDATION_BAD_TIMESTAMP));
-}
-
-TEST_F(CloudPolicyValidatorTest, IgnoreErrorTimestampFromTheFuture) {
-  base::Time timestamp(timestamp_ + base::TimeDelta::FromMinutes(5));
-  timestamp_option_ =
-      CloudPolicyValidatorBase::TIMESTAMP_NOT_BEFORE;
-  policy_.policy_data().set_timestamp(
-      (timestamp - base::Time::UnixEpoch()).InMilliseconds());
-  Validate(CheckStatus(CloudPolicyValidatorBase::VALIDATION_OK));
-}
-
 TEST_F(CloudPolicyValidatorTest, ErrorNoDMToken) {
   policy_.policy_data().clear_request_token();
   Validate(CheckStatus(CloudPolicyValidatorBase::VALIDATION_BAD_DM_TOKEN));