Change origin trial token format

content/common/origin_trials/trial_token.h

Index: content/common/origin_trials/trial_token.h
diff --git a/content/common/origin_trials/trial_token.h b/content/common/origin_trials/trial_token.h
index 3109240e6ac9c5376b95f77b3fa9e4b7e4be9d75..fa99d1f7e21acd5cf307404a6accbcd2b1a318c6 100644
--- a/content/common/origin_trials/trial_token.h
+++ b/content/common/origin_trials/trial_token.h
@@ -32,23 +32,31 @@ class CONTENT_EXPORT TrialToken {
  public:
   ~TrialToken();
 
-  // Returns a token object if the string represents a well-formed token, or
-  // nullptr otherwise. (This does not mean that the token is valid, just that
-  // it can be parsed.)
+  // Returns a token object if the string represents a signed well-formed token,
+  // or nullptr otherwise. (This does not mean that the token is currently
+  // valid, or appropriate for a given origin / feature, just that it is
+  // correctly formatted and signed, and can be parsed.)
+  static scoped_ptr<TrialToken> From(const std::string& token_text,

[palmer, 2016/04/07 00:03:53]

Nit: The |From|, |Extract|, |Parse| suite of functions form an interface that is
not obvious to me.  (For example, clearly I must call them in a certain order
and hook up outputs to inputs in some way, but what is it exactly?)

My instinct is to specify the interface as just a constructor, like this:

  TrialToken(vector<uint8_t> token_bytes, vector<uint8_t> public_key_bytes);

and if the |token_bytes| or |public_key_bytes| are invalid in any way, the
constructed |TrialToken| is invalid or empty or whatever.

Does that not suffice for some reason that isn't apparent to me yet? (I haven't
read any of the rest of the code, so.)

[iclelland, 2016/04/07 14:55:13]

From is intended to be the entry point -- it could really just be a constructor,
but it seemed like a more common idiom in Chrome to name conversion functions
"Create" or "From", and have the actual constructor, which just initializes data
members, be private. I'm fine either way.

Extract and Parse represent the two distinct phases of getting the token data
out of the string -- one unwraps the binary, checks the signature, and returns
the JSON payload; the other deserializes the JSON and pulls the important pieces
out of it. They are separated for ease of testing; there's no real need for them
to be a public API.

Removed those two from the public API.

+                                     base::StringPiece public_key);
+
+  // Returns the payload of a signed token, or nullptr if the token is not
+  // properly signed, or is not well-formed.
+  static scoped_ptr<std::string> Extract(const std::string& token_text,
+                                         base::StringPiece public_key);
+
+  // Returns a token object if the string represents a well-formed JSON token
+  // payload, or nullptr otherwise.
   static scoped_ptr<TrialToken> Parse(const std::string& token_text);
 
-  // Returns true if this feature is appropriate for use by the given origin,
+  // Returns true if this token is appropriate for use by the given origin,
   // for the given feature name. This does not check whether the signature is
   // valid, or whether the token itself has expired.
   bool IsAppropriate(const url::Origin& origin,

[palmer, 2016/04/07 00:03:53]

Nit: Similarly, I'd declare simply:

  bool IsValidForFeature(const url::Origin& origin,
                         base::StringPiece feature_name,
                         const base::Time& now) const;

[iclelland, 2016/04/07 14:55:13]

Done.

                      base::StringPiece feature_name) const;
 
-  // Returns true if this token has a valid signature, and has not expired.
-  bool IsValid(const base::Time& now, base::StringPiece public_key) const;
+  // Returns true if this token has not yet expired.
+  bool IsValid(const base::Time& now) const;
 
-  uint8_t version() { return version_; }
-  std::string signature() { return signature_; }
-  std::string data() { return data_; }
   url::Origin origin() { return origin_; }
   std::string feature_name() { return feature_name_; }
   base::Time expiry_time() { return expiry_time_; }
@@ -66,34 +74,10 @@ class CONTENT_EXPORT TrialToken {
                                 base::StringPiece public_key);
 
  private:
-  TrialToken(uint8_t version,
-             const std::string& signature,
-             const std::string& data,
-             const url::Origin& origin,
+  TrialToken(const url::Origin& origin,
              const std::string& feature_name,
              uint64_t expiry_timestamp);
 
-  // The version number for this token. The version identifies the structure of
-  // the token, as well as the algorithm used to generate/validate the token.
-  // The version number is only incremented when incompatible changes are made
-  // to either the structure (e.g. adding a field), or the algorithm (e.g.
-  // changing the hash or signing algorithm).
-  // NOTE: The version number is not part of the token signature and validation.
-  //       Thus it is possible to modify a token to use a different version from
-  //       that used to generate the signature. To mitigate cross-version
-  //       attacks, the signing key(s) should be changed whenever there are
-  //       changes to the token structure or algorithms.
-  uint8_t version_;
-
-  // The base64-encoded-signature portion of the token. For the token to be
-  // valid, this must be a valid signature for the data portion of the token, as
-  // verified by the public key in trial_token.cc.
-  std::string signature_;
-
-  // The portion of the token string which is signed, and whose signature is
-  // contained in the signature_ member.
-  std::string data_;
-
   // The origin for which this token is valid. Must be a secure origin.
   url::Origin origin_;