Use {To/From}JavaTime for policy timestamps

components/policy/proto/device_management_backend.proto

 // Copyright 2013 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.
 
 syntax = "proto2";
 
 option optimize_for = LITE_RUNTIME;
 
 package enterprise_management;
 
@@ skipping 184 matching lines @@
   // Possible values for Chrome OS are:
   //   google/chromeos/device => ChromeDeviceSettingsProto
   //   google/chromeos/user => ChromeSettingsProto
   //   google/chromeos/publicaccount => ChromeSettingsProto
   //   google/chrome/extension => ExternalPolicyData
   //   google/chromeos/signinextension => ExternalPolicyData
   //   google/android/user => ChromeSettingsProto
   //   google/ios/user => ChromeSettingsProto
   optional string policy_type = 1;
 
   // This is the last policy timestamp that client received from server.

[Thiemo Nagel, 2017/04/20 14:02:13]

Nit: Please add "Java time".

[emaxx, 2017/04/20 15:00:46]

Acknowledged, though I decided to refer to PolicyData.timestamp instead. Apart
from ridding of duplication, I think it's useful to hint that the client
shouldn't generate the value for this field itself, using its potentially wrong
clocks.

   optional int64 timestamp = 2;
 
   // Tell server what kind of security signature is required.
   enum SignatureType {
     NONE = 0;
     SHA1_RSA = 1;
   }
   optional SignatureType signature_type = 3 [default = NONE];
 
   // The version number of the public key that is currently stored
@@ skipping 63 matching lines @@
   // |DEVICE_MODE_DISABLED|.
   optional DisabledState disabled_state = 2;
 }
 
 // This message is included in serialized form in PolicyFetchResponse below. It
 // may also be signed, with the signature being created for the serialized form.
 message PolicyData {
   // See PolicyFetchRequest.policy_type.
   optional string policy_type = 1;
 
-  // [timestamp] is milliseconds since Epoch in UTC timezone. It is
+  // [timestamp] is milliseconds since Epoch in UTC timezone (Java time). It is

[Thiemo Nagel, 2017/04/20 14:02:13]

Nit: Could you please either re-wrap the whole paragraph at 80 characters or
stay in line with the existing (70 character) wrapping?

[emaxx, 2017/04/20 15:00:46]

Done.

   // included here so that the time at which the server issued this
   // response cannot be faked (as protection against replay attacks).
   // It is the timestamp generated by DMServer, NOT the time admin
   // last updated the policy or anything like that.
   optional int64 timestamp = 2;
 
   // The DM token that was used by the client in the HTTP POST header
   // for authenticating the request. It is included here again so that
   // the client can verify that the response is meant for them (and not
   // issued by a replay or man-in-the-middle attack).
@@ skipping 230 matching lines @@
   repeated PolicyFetchRequest request = 3;
 }
 
 // Response from server to device for reading policies.
 message DevicePolicyResponse {
   // The policy fetch response.
   repeated PolicyFetchResponse response = 3;
 }
 
 message TimePeriod {
-  // [timestamp] is milli seconds since Epoch in UTC timezone.
+  // [timestamp] is milliseconds since Epoch in UTC timezone (Java time).
   optional int64 start_timestamp = 1;
   optional int64 end_timestamp = 2;
 }
 
 message ActiveTimePeriod {
   optional TimePeriod time_period = 1;
 
   // The active duration during the above time period.
   // The unit is milli-second.
   optional int32 active_duration = 2;
 }
 
 // This captures launch events for one app/extension or other installments.
 message InstallableLaunch {
   optional string install_id = 1;
 
   // Time duration where this report covers. These are required
   // and the record will be ignored if not set.
   optional TimePeriod duration = 2;
 
   // Client will send at most 50 timestamps to DM. All the rest

[Thiemo Nagel, 2017/04/20 14:02:13]

Nit: Please add "Java time".

[emaxx, 2017/04/20 15:00:46]

I think this message InstallableLaunch is actually unused.
So maybe just mark it all as OBSOLETE_? (in a separate CL)

[Thiemo Nagel, 2017/04/20 16:16:52]

I've added it to my CL deleting unused proto fields.

[emaxx, 2017/04/20 18:33:22]

Thanks!

   // launch activities will be summed into the total count.
   // We will distribute the count evenly among the time span when
   // doing time based aggregation.
   repeated int64 timestamp = 3;
   optional int64 total_count = 4;
 }
 
 // OBSOLETE: Used to report the device location.
 message OBSOLETE_DeviceLocation {
   enum ErrorCode {
@@ skipping 520 matching lines @@
     RESULT_SUCCESS = 2;  // The command was successfully executed.
   }
 
   // The result of the command.
   optional ResultType result = 1;
 
   // The opaque unique identifier of the command. This value is copied from the
   // RemoteCommand protobuf that contained the command.
   optional int64 command_id = 2;
 
   // The time at which the command was executed, if the the result is

[Thiemo Nagel, 2017/04/20 14:02:13]

Nit: Please mention "Java time".

[emaxx, 2017/04/20 15:00:46]

I'm afraid this is not applicable in this case. According to the code (see
RemoteCommandsService::OnJobFinished()), this field is set from TimeTicks,
which, IIUC, is weird and has no physical sense. Quoting base/time/time.h:

regarding TimeTicks:

    [...] Note that TimeTicks may "stand still" (e.g., if the computer is
suspended) [...]

regarding TimeTicks::UnixEpoch:

    Get an estimate of the TimeTick value at the time of the UnixEpoch.
    [...] This function will return the same value for the duration of
    the application, but will be different in future application runs.

[Thiemo Nagel, 2017/04/20 16:16:52]

The way I read this, TimeTicks::UnixEpoch() is a way to approximate the correct
time.  In the end, the server expects Java time and the client sends Java time
(albeit only approximated).  Thus I'd say it would be correct to annotate this
as Java time, maybe adding a note that it's only an approximation.

[emaxx, 2017/04/20 18:33:22]

OK, I searched through the server code and it looks like this field is actually
interpreted as a Java time. Therefore using approximation is a client bug, while
this proto, as a public contract, should define what this field supposed to
mean.
Done.

   // RESULT_SUCCESS.
   optional int64 timestamp = 3;
 
   // Extra information sent to server as result of execution, expected to be a
   // JSON string.
   optional string payload = 4;
 }
 
 message DeviceRemoteCommandRequest {
   // The command ID of the last command received from the server until
@@ skipping 370 matching lines @@
       check_android_management_response = 18;
 
   // Response to an Active Directory Play user enrollment request.
   optional ActiveDirectoryEnrollPlayUserResponse
       active_directory_enroll_play_user_response = 19;
 
   // Response to a Play activity request.
   optional ActiveDirectoryPlayActivityResponse
       active_directory_play_activity_response = 20;
 }