Collect UMA data for Origin Trials

content/common/origin_trials/trial_token.h

 // 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.
 
 #ifndef CONTENT_COMMON_ORIGIN_TRIALS_TRIAL_TOKEN_H_
 #define CONTENT_COMMON_ORIGIN_TRIALS_TRIAL_TOKEN_H_
 
 #include <memory>
 #include <string>
 
 #include "base/strings/string_piece.h"
 #include "base/time/time.h"
 #include "content/common/content_export.h"
+#include "content/common/origin_trials/trial_token_status.h"
 #include "url/origin.h"
 
 namespace content {
 
 // The Experimental Framework (EF) provides limited access to experimental
 // features, on a per-origin basis (origin trials). This class defines the trial
 // token data structure, used to securely provide access to an experimental
 // feature.
 //
 // Features are defined by string names, provided by the implementers. The EF
 // code does not maintain an enum or constant list for feature names. Instead,
 // the EF validates the name provided by the feature implementation against any
 // provided tokens.
 //
 // More documentation on the token format can be found at
 // https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M
 
 class CONTENT_EXPORT TrialToken {
  public:
   ~TrialToken();
 
-  // 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
+  // If the string represents a signed well-formed token, a token object is
+  // returned in the |out_token| parameter and success is returned. Otherwise,
+  // the return code indicates what was wrong with the string, and |out_token|
+  // is set to NULL.
+  // Note that success does not mean that the token is currently valid, or
+  // appropriate for a given origin / feature. It only means that it is
   // correctly formatted and signed by the supplied public key, and can be
-  // parsed.)
-  static std::unique_ptr<TrialToken> From(const std::string& token_text,
-                                          base::StringPiece public_key);
+  // parsed.
+  static TrialTokenStatus From(const std::string& token_text,
+                               base::StringPiece public_key,
+                               std::unique_ptr<TrialToken>& out_token);

[Marijn Kruisselbrink, 2016/04/21 18:18:42]

Pass-by-(non-const)-reference is against the styleguide. If you want to return
something through an argument you should use a pointer.
I also wonder if since this method is still called "From" it might not make
sense to still return a std::unique_ptr, and have the status result through a
(pointer) out-parameter. That would at least avoid the somewhat weird situation
of having a pointer to a unique pointer.

It's also not entirely clear to me why TrialToken is still a class that we even
bother instantiating though. Maybe as an implementation detail of
TrialTokenValidator it makes sense, but TrialToken instances are always
short-lived, yet heap-allocated, and there doesn't even really seem to be any
benefit to try to keep one around. Of course if in the future we would somehow
cache parsed tokens rather than token strings there might be some point in
having a TrialToken instance, but even that wouldn't really work in the presence
of multiple public keys.

[chasej, 2016/04/22 18:50:06]

TL;DR - I made the result enum be the out parameter, and went back to returning
a unique_ptr.

You're correct, I missed the part in the Google style guide about reference
arguments. I didn't see anything about how to handle out parameters that were
smart pointers in the various docs on ownership and passing objects. Common
wisdom (outside of our style guides) suggested that a non-const ref was the best
way to have a unique_ptr as an out parameter.  As you say, maybe the best answer
is to swap the return and out values. I originally considered that, but it
seemed the convention was typically to return the result code, and have the
maybe-created object as the out parameter. In this case though, I think the swap
is justifiable, especially with the "From" naming.
The plan was to have some caching of parsed tokens, or at least the validation
results. Of course, we don't have any code like that as yet. I don't really
understand what you mean that caching wouldn't work with multiple public keys.

Regardless, if we want to get rid of TrialToken, I'd suggest that should be a
separate bug/CL.

[Marijn Kruisselbrink, 2016/04/22 21:51:06]

Never mind, you're right. I think I was thinking that somehow the validating the
signature before parsing would mean that the parsed tokens are now somehow tied
to the public key used to parse them. But even if that is the case, it doesn't
make it any less valid to cache them, as once the key is validated it really
doesn't matter anymore what key they were signed with in the first place.

 
-  // Returns true if this token is appropriate for use by the given origin,
-  // for the given feature name, and has not yet expired.
-  bool IsValidForFeature(const url::Origin& origin,
-                         base::StringPiece feature_name,
-                         const base::Time& now) const;
+  // Returns success if this token is appropriate for use by the given origin
+  // and feature name, and has not yet expired. Otherwise, the return value
+  // indicates why the token is not valid.
+  TrialTokenStatus IsValidForFeature(const url::Origin& origin,
+                                     base::StringPiece feature_name,
+                                     const base::Time& now) const;
 
   url::Origin origin() { return origin_; }
   std::string feature_name() { return feature_name_; }
   base::Time expiry_time() { return expiry_time_; }
 
  protected:
   friend class TrialTokenTest;
 
-  // Returns the payload of a signed token, or nullptr if the token is not
-  // properly signed, or is not well-formed.
-  static std::unique_ptr<std::string> Extract(const std::string& token_text,
-                                              base::StringPiece public_key);
+  // If the string represents a properly signed and well-formed token, the JSON
+  // payload is returned in the |out_token_json| parameter and success is
+  // returned. Otherwise,the return code indicates what was wrong with the
+  // string, and |out_token_json| is unchanged.
+  static TrialTokenStatus Extract(const std::string& token_text,
+                                  base::StringPiece public_key,
+                                  std::unique_ptr<std::string>& out_token_json);

[Marijn Kruisselbrink, 2016/04/21 18:18:42]

Here too, don't use non-const reference arguments. And in fact now you have an
explicit TrialTokenStatus result the out_token_json doesn't have to be a
unique_ptr anymore either. So just use std::string* out_token_json or something
like that.

[chasej, 2016/04/22 18:50:06]

Done.

 
   // Returns a token object if the string represents a well-formed JSON token
   // payload, or nullptr otherwise.
   static std::unique_ptr<TrialToken> Parse(const std::string& token_json);
 
   bool ValidateOrigin(const url::Origin& origin) const;
   bool ValidateFeatureName(base::StringPiece feature_name) const;
   bool ValidateDate(const base::Time& now) const;
 
   static bool ValidateSignature(base::StringPiece signature_text,
@@ skipping 11 matching lines @@
   // The name of the experimental feature which this token enables.
   std::string feature_name_;
 
   // The time until which this token should be considered valid.
   base::Time expiry_time_;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_COMMON_ORIGIN_TRIALS_TRIAL_TOKEN_H_