Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1521063003:
Add API Key parsing for experimental APIs (Closed)
DescriptionAdd API Key parsing for experimental APIs
This adds the experimental framework API key parsing mechanism to Chromium.
This code deals with origins, API names, and dates. The validation currently checks for general well-formedness, and can determine, given an origin and API name, whether a given key is appropriate for use. (This is parsing only; actual signature validation is a separate CL)
BUG=543146
Committed: https://crrev.com/672676d8ae5f598f56714bb81b45498476952e9f
Cr-Commit-Position: refs/heads/master@{#367893}
Patch Set 1 #Patch Set 2 : Fix date handling; more thorough tests #Patch Set 3 : Rebase #
Total comments: 19
Patch Set 4 : Addressing review comments #Patch Set 5 : Update tests #Patch Set 6 : Remove forward declaration to wrong namespace #
Total comments: 12
Patch Set 7 : Addressing feedback from PS#6 #
Total comments: 15
Patch Set 8 : Merge blink and chromium code; fixing issues from review #
Total comments: 12
Patch Set 9 : Addressing feedback from PS#8 #
Total comments: 4
Patch Set 10 : Nits #
Messages
Total messages: 38 (13 generated)
Description was changed from ========== Add API Key parsing to blink and chromium BUG=543146 ========== to ========== Add API Key parsing to blink and chromium This adds the experimental framework API key parsing mechanism, to both Blink and Chromium. (This is parsing only; validation is a separate CL) On the blink side, the code will work with origins, API names, and dates, and the actual signature validation is handed off to Chromium. The Chromium code currently only checks for general well-formedness, and accesses enough information to be able to validate the signature. BUG=543146 ==========
iclelland@chromium.org changed reviewers: + chasej@chromium.org
+r chasej -- PTAL, thanks!
https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key.cc:47: new ApiKey(signature, data, GURL(origin), apiName, expiry_int)); There is no validation of the origin or api name (i.e. for illegal use of the delimiter character). I can understand that such validation is not strictly necessary, as IsValidNow() only checks the date and signature (by design). However, that means Parse() can return a key, claiming it's valid, but it contains bad values in the origin or api name. I see two options: 1) The parsed API key only contains/exposes the signature, raw data, and expiry date. It makes no claims/promises about the origin and api name. 2) Parsing adds validation of the origin and API name. https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key.h (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key.h:16: class ApiKeyTest; It doesn't look like this is needed. I'm guessing it's left over from a friend declaration, since removed? https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key.h:42: uint64_t expiry() { return expiry_; } Should this have a comment to indicate the date format (i.e. UTC timestamp)? https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key_unittest.cc:20: "Signature|https://jpchase.github.io|Frobulate|1458766277"; I think it would help test readability/understanding if the dates were commented. Not sure if that's common practice, or should just leave it to the reader to convert the date values? https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/APIKey.cpp (right): https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/APIKey.cpp:30: if (parts.size() != 4) { Combined with the expiry integer check, this will implicitly validate that the API name doesn't contain the pipe character (i.e. either size != 4, or expiry will likely not be a valid integer). However, I think it would be better if the code was more explicit about validating the api name. This also raises the question of whether there should two different delimiters. The first to separate signature vs data, the second to separate origin:name:expiry within the data. As is, validation failures would be ambiguous. Maybe it doesn't matter with this implementation for parse(), as any validation failure returns null, without explanation. https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/APIKey.h (right): https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/APIKey.h:42: // combination. Returning true does not mean that the key is valid nor Should update comment to mention expiry date validation. https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/APIKey.h:44: bool isValidNowForOrigin(const String& origin, const String& apiName, uint64_t now) const; The naming for this method seems a little awkward. There's three parameters (conditions) that are validated, but the name essentially only mentions two of those. It's potentially confusing to see apiName as a parameter. I know this can be addressed by the comment. I wonder if naming it "isValid(args)" with a good comment might be better? https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/APIKeyTest.cpp (right): https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/APIKeyTest.cpp:19: uint64_t kInvalidTime = 1458766278; Add comments to explain the date values? Same rationale as on other tests.
https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key.cc:47: new ApiKey(signature, data, GURL(origin), apiName, expiry_int)); On 2015/12/15 19:43:41, chasej wrote: > There is no validation of the origin or api name (i.e. for illegal use of the > delimiter character). Illegal use of the delimiter would result in more than four parts being present in the `parts` variable. > I can understand that such validation is not strictly > necessary, as IsValidNow() only checks the date and signature (by design). > However, that means Parse() can return a key, claiming it's valid, but it > contains bad values in the origin or api name. I want parsing and validation to be separate concepts, so that we can construct and work with syntactically correct, but invalid keys, when necessary. Parse() can absolutely return an invalid key, and makes no claims as to its validity. The only thing that should cause Parse() to return nullptr is if the key is structurally *wrong*. A timestamp which isn't a number, for instance, or it has the wrong number of parts. We could make a case that an insecure origin, or an origin which isn't even a URL, means that we can't parse the key, but I'm less convinced about validating the API name (unless it's just checking for non-alphanumeric characters or similar) > > I see two options: > 1) The parsed API key only contains/exposes the signature, raw data, and expiry > date. It makes no claims/promises about the origin and api name. > 2) Parsing adds validation of the origin and API name. 1 is what we need right now. 2 will eventually be required, when we want to test isExperimentEnabled from within the browser, as well as the renderer. What if I do well-formedness testing on the key, as part of parse(), for now: - API name should be latin characters only (I don't think we can embed UTF8 or control chars anywhere) - Origin should be a valid secure origin string. Then the browser is just saying "this is a valid well-formed key", and the renderer is responsible for determining whether the origin and api name are appropriate for the current situation. Would that work? https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key.h (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key.h:16: class ApiKeyTest; On 2015/12/15 19:43:41, chasej wrote: > It doesn't look like this is needed. I'm guessing it's left over from a friend > declaration, since removed? Yep. Done. https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key.h:42: uint64_t expiry() { return expiry_; } On 2015/12/15 19:43:41, chasej wrote: > Should this have a comment to indicate the date format (i.e. UTC timestamp)? Done. Added explanatory comments to all member variables. Also renamed this to expiry_timestamp_ to be extra clear. https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key_unittest.cc:20: "Signature|https://jpchase.github.io|Frobulate|1458766277"; On 2015/12/15 19:43:41, chasej wrote: > I think it would help test readability/understanding if the dates were > commented. Not sure if that's common practice, or should just leave it to the > reader to convert the date values? How about if I add another constant, above these ones, with the "now" time that we use for the tests? uint64_t kTimestampForTesting = 1458766276; The actual time used doesn't matter, just that kGoodApiKey has a value after it, and kBadDateApiKey has a value before it. https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/APIKey.h (right): https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/APIKey.h:42: // combination. Returning true does not mean that the key is valid nor On 2015/12/15 19:43:41, chasej wrote: > Should update comment to mention expiry date validation. Done. https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/APIKey.h:44: bool isValidNowForOrigin(const String& origin, const String& apiName, uint64_t now) const; On 2015/12/15 19:43:41, chasej wrote: > The naming for this method seems a little awkward. There's three parameters > (conditions) that are validated, but the name essentially only mentions two of > those. It's potentially confusing to see apiName as a parameter. I know this > can be addressed by the comment. I wonder if naming it "isValid(args)" with a > good comment might be better? Sounds good. Done.
https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key.cc:47: new ApiKey(signature, data, GURL(origin), apiName, expiry_int)); On 2015/12/15 21:22:24, iclelland wrote: > On 2015/12/15 19:43:41, chasej wrote: > > There is no validation of the origin or api name (i.e. for illegal use of the > > delimiter character). > > Illegal use of the delimiter would result in more than four parts being present > in the `parts` variable. > > > I can understand that such validation is not strictly > > necessary, as IsValidNow() only checks the date and signature (by design). > > However, that means Parse() can return a key, claiming it's valid, but it > > contains bad values in the origin or api name. > > I want parsing and validation to be separate concepts, so that we can construct > and work with syntactically correct, but invalid keys, when necessary. Parse() > can absolutely return an invalid key, and makes no claims as to its validity. > The only thing that should cause Parse() to return nullptr is if the key is > structurally *wrong*. A timestamp which isn't a number, for instance, or it has > the wrong number of parts. We could make a case that an insecure origin, or an > origin which isn't even a URL, means that we can't parse the key, but I'm less > convinced about validating the API name (unless it's just checking for > non-alphanumeric characters or similar) > > > > > I see two options: > > 1) The parsed API key only contains/exposes the signature, raw data, and > expiry > > date. It makes no claims/promises about the origin and api name. > > 2) Parsing adds validation of the origin and API name. > > 1 is what we need right now. 2 will eventually be required, when we want to test > isExperimentEnabled from within the browser, as well as the renderer. > > What if I do well-formedness testing on the key, as part of parse(), for now: > - API name should be latin characters only (I don't think we can embed UTF8 or > control chars anywhere) > - Origin should be a valid secure origin string. > > Then the browser is just saying "this is a valid well-formed key", and the > renderer is responsible for determining whether the origin and api name are > appropriate for the current situation. > > Would that work? I agree with keeping parsing and validation separate. So, Parse() can just check that there are the correct number parts (4). The first three parts are strings, which could be checked only for illegal characters. The fourth part is checked to ensure it is a number (the fact that it actually verifies it's positive integer, that fits into a uint64 is an implementation detail). https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key_unittest.cc:20: "Signature|https://jpchase.github.io|Frobulate|1458766277"; On 2015/12/15 21:22:24, iclelland wrote: > On 2015/12/15 19:43:41, chasej wrote: > > I think it would help test readability/understanding if the dates were > > commented. Not sure if that's common practice, or should just leave it to the > > reader to convert the date values? > > How about if I add another constant, above these ones, with the "now" time that > we use for the tests? > > uint64_t kTimestampForTesting = 1458766276; > > The actual time used doesn't matter, just that kGoodApiKey has a value after it, > and kBadDateApiKey has a value before it. Adding another constant for "right now" is a good idea. The proposed naming seems ambiguous to me (i.e. "for testing" what?). I would suggest something like "kCurrentTime" for the naming (that name isn't great, but hopefully you get the gist).
Description was changed from ========== Add API Key parsing to blink and chromium This adds the experimental framework API key parsing mechanism, to both Blink and Chromium. (This is parsing only; validation is a separate CL) On the blink side, the code will work with origins, API names, and dates, and the actual signature validation is handed off to Chromium. The Chromium code currently only checks for general well-formedness, and accesses enough information to be able to validate the signature. BUG=543146 ========== to ========== Add API Key parsing to blink and chromium This adds the experimental framework API key parsing mechanism, to both Blink and Chromium. (This is parsing only; validation is a separate CL) On the blink side, the code will work with origins, API names, and dates, and the actual signature validation is handed off to Chromium. The Chromium code currently only checks for general well-formedness, and accesses enough information to be able to validate the signature and expiry time. BUG=543146 ==========
iclelland@chromium.org changed reviewers: + davidben@chromium.org, rbyers@chromium.org
Tests updates; chasej, can you PTAL again? +r rbyers, can you check the WebKit side? +r davidben, can you PTAL at content/browser/* -- the next CL that builds on this is going to include cryptographic signature validation, so I wanted to send that one your way; I thought that if you see this one first, you'll be in a better spot for it. If that's not appropriate, let me know and I can find a different reviewer. https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key.cc:47: new ApiKey(signature, data, GURL(origin), apiName, expiry_int)); On 2015/12/16 15:41:17, chasej wrote: > On 2015/12/15 21:22:24, iclelland wrote: > > On 2015/12/15 19:43:41, chasej wrote: > > > There is no validation of the origin or api name (i.e. for illegal use of > the > > > delimiter character). > > > > Illegal use of the delimiter would result in more than four parts being > present > > in the `parts` variable. > > > > > I can understand that such validation is not strictly > > > necessary, as IsValidNow() only checks the date and signature (by design). > > > However, that means Parse() can return a key, claiming it's valid, but it > > > contains bad values in the origin or api name. > > > > I want parsing and validation to be separate concepts, so that we can > construct > > and work with syntactically correct, but invalid keys, when necessary. Parse() > > can absolutely return an invalid key, and makes no claims as to its validity. > > The only thing that should cause Parse() to return nullptr is if the key is > > structurally *wrong*. A timestamp which isn't a number, for instance, or it > has > > the wrong number of parts. We could make a case that an insecure origin, or an > > origin which isn't even a URL, means that we can't parse the key, but I'm less > > convinced about validating the API name (unless it's just checking for > > non-alphanumeric characters or similar) > > > > > > > > I see two options: > > > 1) The parsed API key only contains/exposes the signature, raw data, and > > expiry > > > date. It makes no claims/promises about the origin and api name. > > > 2) Parsing adds validation of the origin and API name. > > > > 1 is what we need right now. 2 will eventually be required, when we want to > test > > isExperimentEnabled from within the browser, as well as the renderer. > > > > What if I do well-formedness testing on the key, as part of parse(), for now: > > - API name should be latin characters only (I don't think we can embed UTF8 > or > > control chars anywhere) > > - Origin should be a valid secure origin string. > > > > Then the browser is just saying "this is a valid well-formed key", and the > > renderer is responsible for determining whether the origin and api name are > > appropriate for the current situation. > > > > Would that work? > > I agree with keeping parsing and validation separate. So, Parse() can just > check that there are the correct number parts (4). The first three parts are > strings, which could be checked only for illegal characters. The fourth part is > checked to ensure it is a number (the fact that it actually verifies it's > positive integer, that fits into a uint64 is an implementation detail). Cool -- if it's alright with you, I'll add the extra (character-set) validation as a separate CL. https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... File content/browser/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1521063003/diff/40001/content/browser/experim... content/browser/experiments/api_key_unittest.cc:20: "Signature|https://jpchase.github.io|Frobulate|1458766277"; On 2015/12/16 15:41:17, chasej wrote: > On 2015/12/15 21:22:24, iclelland wrote: > > On 2015/12/15 19:43:41, chasej wrote: > > > I think it would help test readability/understanding if the dates were > > > commented. Not sure if that's common practice, or should just leave it to > the > > > reader to convert the date values? > > > > How about if I add another constant, above these ones, with the "now" time > that > > we use for the tests? > > > > uint64_t kTimestampForTesting = 1458766276; > > > > The actual time used doesn't matter, just that kGoodApiKey has a value after > it, > > and kBadDateApiKey has a value before it. > > Adding another constant for "right now" is a good idea. The proposed naming > seems ambiguous to me (i.e. "for testing" what?). I would suggest something like > "kCurrentTime" for the naming (that name isn't great, but hopefully you get the > gist). Done -- I've reworked the tests, and now have kValidTimestamp and kInvalidTimestamp for testing. https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/APIKey.cpp (right): https://codereview.chromium.org/1521063003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/APIKey.cpp:30: if (parts.size() != 4) { On 2015/12/15 19:43:41, chasej wrote: > Combined with the expiry integer check, this will implicitly validate that the > API name doesn't contain the pipe character (i.e. either size != 4, or expiry > will likely not be a valid integer). However, I think it would be better if the > code was more explicit about validating the api name. > > This also raises the question of whether there should two different delimiters. > The first to separate signature vs data, the second to separate > origin:name:expiry within the data. As is, validation failures would be > ambiguous. Maybe it doesn't matter with this implementation for parse(), as any > validation failure returns null, without explanation. Acknowledged. For now, I've gone with the simplest approach, of just splitting and accepting that if the API name isn't something valid, it will never pass any tests made by the browser -- we're never going to look for that invalid string.
LGTM. I'm fine with adding character-set validation in another CL.
lgtm with style nits. https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:20: scoped_ptr<ApiKey> ApiKey::Parse(const std::string& keyText) { key_text https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:43: // signed data is (origin + "|" + api_name + "|" + expiry) Nit: period at the end https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:61: bool ApiKey::IsValidNow(const base::Time& now) const { Nit: I realize you're going to be adding that immediately afterwards, but I'd probably stick a TODO here just so it's clear the signature check is supposed to be here. :-) https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... File content/browser/experiments/api_key.h (right): https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:31: static scoped_ptr<ApiKey> Parse(const std::string&); Nit: parameter name https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:52: // by the public key in api_key.cc Nit: period at the end. https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:62: // The name of the API experiment which this key enables Nit: period at the end.
Oh, one comment: Do you all think it's worth sticking a "v1|" at the front of the key or something, so you'll have an easier time revving the format?
On 2015/12/18 19:51:45, davidben wrote: > Oh, one comment: Do you all think it's worth sticking a "v1|" at the front of > the key or something, so you'll have an easier time revving the format? Absolutely, thanks for the tip. I will do that in a different CL, but we will definitely add that. (I've updated crbug.com/570684 based on your comments there as well)
https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:20: scoped_ptr<ApiKey> ApiKey::Parse(const std::string& keyText) { On 2015/12/18 19:48:36, davidben wrote: > key_text Done. https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:43: // signed data is (origin + "|" + api_name + "|" + expiry) On 2015/12/18 19:48:36, davidben wrote: > Nit: period at the end Done. https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.cc:61: bool ApiKey::IsValidNow(const base::Time& now) const { On 2015/12/18 19:48:36, davidben wrote: > Nit: I realize you're going to be adding that immediately afterwards, but I'd > probably stick a TODO here just so it's clear the signature check is supposed to > be here. :-) Done. https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... File content/browser/experiments/api_key.h (right): https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:31: static scoped_ptr<ApiKey> Parse(const std::string&); On 2015/12/18 19:48:36, davidben wrote: > Nit: parameter name Done. https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:52: // by the public key in api_key.cc On 2015/12/18 19:48:36, davidben wrote: > Nit: period at the end. Done. https://codereview.chromium.org/1521063003/diff/100001/content/browser/experi... content/browser/experiments/api_key.h:62: // The name of the API experiment which this key enables On 2015/12/18 19:48:36, davidben wrote: > Nit: period at the end. Done.
Sorry for the delay (been swamped with perf sheriff and reviews). The WebKit side looks fine to me, but it really sucks to have basically two copies of this code. Keeping them in sync is going to be a pain, right? One option would be to put this in base and wrap it for blink in Source/platform. I think there are plans to eventually open up base to all of WebKit, but esprehn@ is still working on the details. We might want to get his opinion. It looks like the longer term design issues are discussed here: https://docs.google.com/document/d/1_6iFVWgntDpkxiAMX7753XrmxxkDgiAGmrPU2TGsg.... There's definitely some interesting tradeoffs to make, and I'm fine with not trying to solve all the problems now (eg. when there's no signature checking happening yet). But having two completely separate copies of the code seems like a sad starting place ;-) https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/experiments/APIKey.h (right): https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/APIKey.h:29: // TODO(chasej): Link to documentation, or provide more detail on keys, .etc Is this the doc you want (at least for now): https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt... ?
mek@chromium.org changed reviewers: + mek@chromium.org
Drive-by-review: Why the code duplication between chromium and blink? Can't you just put all the "real" code in content/common, and then use it from wherever in chromium or blink you need? Admittedly depending on how this is going to be used from the blink side that might be a bit tricky before onion soup is all worked out, but just duplication all the parsing and validation doesn't seem right either. https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... content/browser/experiments/api_key.cc:28: SplitString(key_text, "|", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); I realize you only use "|" in one place (other than the general code duplication between blink and content going on), but it might still make sense to define it as a named constant? https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... File content/browser/experiments/api_key.h (right): https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... content/browser/experiments/api_key.h:34: bool IsValidNow(const base::Time& now) const; IsValidNow seems a bit of an odd name if "now" is actually passed in as an argument? Maybe IsValidAt or something would be clearer? https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... content/browser/experiments/api_key.h:43: ApiKey(); Why do you define (but not implement) a default constructor? https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... content/browser/experiments/api_key.h:70: } // namespace // namespace content (at least I would otherwise assume this closes an anonymous namespace) And probably add a blank line between the } and the #endif https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/experiments/APIKey.cpp (right): https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/APIKey.cpp:46: // Origin field must be a valid URL Do we want to accept arbitrary URLs? Or should either we make sure the origin is just an origin, or extract the origin from the URL? https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/APIKey.cpp:47: KURL origin = KURL(KURL(), originText); Does this work? The KURL(KURL, String) constructor claims it always creates an invalid URL if the KURL argument is an invalid URL. So in this case it seems like this should always create an invalid url? (it seems it might just be the KURL docs that are wrong...) https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/experiments/APIKey.h (right): https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/APIKey.h:30: class CORE_EXPORT APIKey : public RefCountedWillBeGarbageCollected<APIKey> { Why RefCountedWillBeGarbageCollected? New code should just be GarbageCollected directly.
On 2015/12/18 21:52:42, Rick Byers wrote: > Sorry for the delay (been swamped with perf sheriff and reviews). > > The WebKit side looks fine to me, but it really sucks to have basically two > copies of this code. Keeping them in sync is going to be a pain, right? > > One option would be to put this in base and wrap it for blink in > Source/platform. I think there are plans to eventually open up base to all of > WebKit, but esprehn@ is still working on the details. We might want to get his > opinion. I will see if I can get that opinion, thanks! Yeah, it sucks to have two copies of essentially the same string parsing code, just working with different libraries, and living in different parts of the codebase. I do see the two sides (browser and renderer) being concerned with different aspects of the key, though -- if it makes any difference at all. The browser right now is really only concerned with the question 'is the key the renderer is presenting properly signed?' As long as it is, then renderer can handle the details of "is it valid for this origin?" and "is it valid for this API?". > > It looks like the longer term design issues are discussed here: > https://docs.google.com/document/d/1_6iFVWgntDpkxiAMX7753XrmxxkDgiAGmrPU2TGsg.... That's the one -- that covers most of what I was saying above ;) > There's definitely some interesting tradeoffs to make, and I'm fine with not > trying to solve all the problems now (eg. when there's no signature checking > happening yet). But having two completely separate copies of the code seems > like a sad starting place ;-) Agreed. The string parsing is eventually going to be a small part of this, but it would be good to have it in a single place. For now, I'm not sure where that place is, though. My understanding is that the only sharing we're doing right now is with base/, and even that only from WTF. It seemed really wrong to be adding this code there, but I'm more than happy to get another opinion on it. > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/experiments/APIKey.h (right): > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/experiments/APIKey.h:29: // TODO(chasej): Link to > documentation, or provide more detail on keys, .etc > Is this the doc you want (at least for now): > https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt... > ? I'll update the comment and remove the TODO, thanks.
On 2015/12/20 at 05:10:51, iclelland wrote: > On 2015/12/18 21:52:42, Rick Byers wrote: > > Sorry for the delay (been swamped with perf sheriff and reviews). > > > > The WebKit side looks fine to me, but it really sucks to have basically two > > copies of this code. Keeping them in sync is going to be a pain, right? > > > > One option would be to put this in base and wrap it for blink in > > Source/platform. I think there are plans to eventually open up base to all of > > WebKit, but esprehn@ is still working on the details. We might want to get his > > opinion. > > I will see if I can get that opinion, thanks! Yeah, it sucks to have two copies of essentially the same string parsing code, just working with different libraries, and living in different parts of the codebase. > > I do see the two sides (browser and renderer) being concerned with different aspects of the key, though -- if it makes any difference at all. The browser right now is really only concerned with the question 'is the key the renderer is presenting properly signed?' As long as it is, then renderer can handle the details of "is it valid for this origin?" and "is it valid for this API?". > > > > It looks like the longer term design issues are discussed here: > > https://docs.google.com/document/d/1_6iFVWgntDpkxiAMX7753XrmxxkDgiAGmrPU2TGsg.... > > That's the one -- that covers most of what I was saying above ;) > > > There's definitely some interesting tradeoffs to make, and I'm fine with not > > trying to solve all the problems now (eg. when there's no signature checking > > happening yet). But having two completely separate copies of the code seems > > like a sad starting place ;-) > > Agreed. The string parsing is eventually going to be a small part of this, but it would be good to have it in a single place. For now, I'm not sure where that place is, though. My understanding is that the only sharing we're doing right now is with base/, and even that only from WTF. It seemed really wrong to be adding this code there, but I'm more than happy to get another opinion on it. But why do you need to do any of this parsing in blink? Is there anything wrong with putting all the key parsing etc in content/common? In the browser process you can then directly use that code, in content in the renderer process the same is true, and if you really need to be able to do parsing from blink (which so far doesn't seem to be the case) you could add some kind of interface between blink and content to do that. > > > > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/experiments/APIKey.h (right): > > > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/experiments/APIKey.h:29: // TODO(chasej): Link to > > documentation, or provide more detail on keys, .etc > > Is this the doc you want (at least for now): > > https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt... > > ? > > I'll update the comment and remove the TODO, thanks.
I've removed the Blink code from this CL (after the comments in crrev.com/1528613003/), and moved the Chromium code into content/common, so that it can will be accessible to both browser and renderer. The blink-side 'APIKey::isValid' method is now 'ApiKey::IsAppropriate' in content/, and just checks whether the origin and api name are acceptable, leaving isValid to check the expiry date (and the signature, in the next CL) https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... File content/browser/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... content/browser/experiments/api_key.cc:28: SplitString(key_text, "|", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > I realize you only use "|" in one place (other than the general code duplication > between blink and content going on), but it might still make sense to define it > as a named constant? Done. https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... File content/browser/experiments/api_key.h (right): https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... content/browser/experiments/api_key.h:34: bool IsValidNow(const base::Time& now) const; On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > IsValidNow seems a bit of an odd name if "now" is actually passed in as an > argument? Maybe IsValidAt or something would be clearer? Renamed to IsValid, mirroring the blink code. https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... content/browser/experiments/api_key.h:43: ApiKey(); On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > Why do you define (but not implement) a default constructor? Thanks for catching it; we don't need it. One private constructor is enough :) https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... content/browser/experiments/api_key.h:70: } // namespace On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > // namespace content > (at least I would otherwise assume this closes an anonymous namespace) > > And probably add a blank line between the } and the #endif Done. https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/experiments/APIKey.cpp (right): https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/APIKey.cpp:47: KURL origin = KURL(KURL(), originText); On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > Does this work? The KURL(KURL, String) constructor claims it always creates an > invalid URL if the KURL argument is an invalid URL. So in this case it seems > like this should always create an invalid url? (it seems it might just be the > KURL docs that are wrong...) It works (as in it creates URLs that pass that test), and the unit tests in platform/weborigin/KURLTest.cpp (like Valid_HTTP_FTP_URLsHaveHosts) seem to validate this pattern as well; the docs may simply be out of date if they're suggesting that it shouldn't work. I looked around, and it seemed to be the standard way to construct a KURL given an absolute URL in a String. https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/experiments/APIKey.h (right): https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/APIKey.h:29: // TODO(chasej): Link to documentation, or provide more detail on keys, .etc On 2015/12/18 21:52:42, Rick Byers wrote: > Is this the doc you want (at least for now): > https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt... > ? Actually, more info on the key format itself is being collected at https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M . Updated the TODO. (And moved it to content/) https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/APIKey.h:30: class CORE_EXPORT APIKey : public RefCountedWillBeGarbageCollected<APIKey> { On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > Why RefCountedWillBeGarbageCollected? New code should just be GarbageCollected > directly. [Class removed in subsequent CLs]
On 2015/12/21 19:17:17, iclelland wrote: > I've removed the Blink code from this CL (after the comments in > crrev.com/1528613003/), and moved the Chromium code into content/common, so that > it can will be accessible to both browser and renderer. Ok, calling from blink into content if necessary sounds like a good plan to me. Since there's no more blink change, I'll move myself to cc. > > The blink-side 'APIKey::isValid' method is now 'ApiKey::IsAppropriate' in > content/, and just checks whether the origin and api name are acceptable, > leaving isValid to check the expiry date (and the signature, in the next CL) > > https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... > File content/browser/experiments/api_key.cc (right): > > https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... > content/browser/experiments/api_key.cc:28: SplitString(key_text, "|", > base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); > On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > > I realize you only use "|" in one place (other than the general code > duplication > > between blink and content going on), but it might still make sense to define > it > > as a named constant? > > Done. > > https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... > File content/browser/experiments/api_key.h (right): > > https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... > content/browser/experiments/api_key.h:34: bool IsValidNow(const base::Time& now) > const; > On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > > IsValidNow seems a bit of an odd name if "now" is actually passed in as an > > argument? Maybe IsValidAt or something would be clearer? > > Renamed to IsValid, mirroring the blink code. > > https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... > content/browser/experiments/api_key.h:43: ApiKey(); > On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > > Why do you define (but not implement) a default constructor? > > Thanks for catching it; we don't need it. One private constructor is enough :) > > https://codereview.chromium.org/1521063003/diff/120001/content/browser/experi... > content/browser/experiments/api_key.h:70: } // namespace > On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > > // namespace content > > (at least I would otherwise assume this closes an anonymous namespace) > > > > And probably add a blank line between the } and the #endif > > Done. > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/experiments/APIKey.cpp (right): > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/experiments/APIKey.cpp:47: KURL origin = > KURL(KURL(), originText); > On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > > Does this work? The KURL(KURL, String) constructor claims it always creates an > > invalid URL if the KURL argument is an invalid URL. So in this case it seems > > like this should always create an invalid url? (it seems it might just be the > > KURL docs that are wrong...) > > It works (as in it creates URLs that pass that test), and the unit tests in > platform/weborigin/KURLTest.cpp (like Valid_HTTP_FTP_URLsHaveHosts) seem to > validate this pattern as well; the docs may simply be out of date if they're > suggesting that it shouldn't work. I looked around, and it seemed to be the > standard way to construct a KURL given an absolute URL in a String. > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/experiments/APIKey.h (right): > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/experiments/APIKey.h:29: // TODO(chasej): Link to > documentation, or provide more detail on keys, .etc > On 2015/12/18 21:52:42, Rick Byers wrote: > > Is this the doc you want (at least for now): > > > https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt... > > ? > > Actually, more info on the key format itself is being collected at > https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M > . Updated the TODO. (And moved it to content/) > > https://codereview.chromium.org/1521063003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/experiments/APIKey.h:30: class CORE_EXPORT APIKey > : public RefCountedWillBeGarbageCollected<APIKey> { > On 2015/12/18 22:03:01, Marijn Kruisselbrink wrote: > > Why RefCountedWillBeGarbageCollected? New code should just be GarbageCollected > > directly. > > [Class removed in subsequent CLs]
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
chasej, mek -- can you take one more look before I commit this?
https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.cc:83: return api_name == api_name_; I think the API name comparison should be case-insensitive. We could rely on API authors being careful/consistent with the case in browser implementation and in registering the experiment in the API console. However, the API name is not a password, so it's not necessary to be strict on casing.
sorry for the delay https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.cc:50: const std::string& data = key_text.substr(key_text.find('|') + 1); '|' should probably be the same kApiKeyFieldSeparator? Also there isn't really any reason to call find here I would think? You already know that it will return signature.length(). And why is this data a const std::string&? Of course it'll work since the lifetime of the temporary this is taking a reference to is being extended appropriately, but might as well keep the code clear and just have a std::string directly. https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.cc:52: return scoped_ptr<ApiKey>(new ApiKey(signature, data, GURL(origin_string), nit: return make_scoped_ptr(new ApiKey(..)) is I think generally how new scoped pointers are returned https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... File content/common/experiments/api_key.h (right): https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.h:35: static scoped_ptr<ApiKey> Parse(const std::string& key_text); nit: explicitly include #include "base/memory/scoped_ptr.h", don't rely on gurl.h including this for you https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.h:44: bool IsValid(const base::Time& now) const; (not really related to this particular line of code) I wonder if somehow somewhere should be mentioned that no validation whatsoever of the origin in the api key is done. Of course it doesn't matter if a key contains a origin field which isn't a valid URL, or is a valid URL but isn't an origin (since such a key will never match an actual origin), but it might be surprising to somebody reading the code. Or maybe Parse should reject keys that don't have a valid url/origin as origin field, just as it rejects keys where the expiration timestamp is not a number. https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... File content/common/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key_unittest.cc:54: const size_t kNumInvalidAPIKeys = sizeof(kInvalidAPIKeys) / sizeof(const char*); you probably want to use arraysize (from base/macros.h) here
https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... File content/common/experiments/api_key.cc (right): https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.cc:50: const std::string& data = key_text.substr(key_text.find('|') + 1); On 2016/01/05 00:59:14, Marijn Kruisselbrink wrote: > '|' should probably be the same kApiKeyFieldSeparator? Yes, missed that one, thanks... > > Also there isn't really any reason to call find here I would think? You already > know that it will return signature.length(). > ... and that's true as well. Done. > And why is this data a const std::string&? Of course it'll work since the > lifetime of the temporary this is taking a reference to is being extended > appropriately, but might as well keep the code clear and just have a std::string > directly. Done. https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.cc:52: return scoped_ptr<ApiKey>(new ApiKey(signature, data, GURL(origin_string), On 2016/01/05 00:59:14, Marijn Kruisselbrink wrote: > nit: return make_scoped_ptr(new ApiKey(..)) is I think generally how new scoped > pointers are returned Done. https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.cc:83: return api_name == api_name_; On 2015/12/21 20:09:10, chasej wrote: > I think the API name comparison should be case-insensitive. We could rely on API > authors being careful/consistent with the case in browser implementation and in > registering the experiment in the API console. However, the API name is not a > password, so it's not necessary to be strict on casing. Done. https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... File content/common/experiments/api_key.h (right): https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.h:35: static scoped_ptr<ApiKey> Parse(const std::string& key_text); On 2016/01/05 00:59:14, Marijn Kruisselbrink wrote: > nit: explicitly include #include "base/memory/scoped_ptr.h", don't rely on > gurl.h including this for you Done. https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key.h:44: bool IsValid(const base::Time& now) const; On 2016/01/05 00:59:14, Marijn Kruisselbrink wrote: > (not really related to this particular line of code) > I wonder if somehow somewhere should be mentioned that no validation whatsoever > of the origin in the api key is done. Of course it doesn't matter if a key > contains a origin field which isn't a valid URL, or is a valid URL but isn't an > origin (since such a key will never match an actual origin), but it might be > surprising to somebody reading the code. > Or maybe Parse should reject keys that don't have a valid url/origin as origin > field, just as it rejects keys where the expiration timestamp is not a number. That's reasonable -- I've gone back and forth on that personally, but I'll add it. And yes, we're not doing any check on whether the origin matches -- that's more of a check to see whether the key is appropriate for use in a given situation, and is what IsAppropriate() checks. https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... File content/common/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1521063003/diff/140001/content/common/experim... content/common/experiments/api_key_unittest.cc:54: const size_t kNumInvalidAPIKeys = sizeof(kInvalidAPIKeys) / sizeof(const char*); On 2016/01/05 00:59:14, Marijn Kruisselbrink wrote: > you probably want to use arraysize (from base/macros.h) here I saw other instances in the codebase, but didn't know about the macro -- thanks for the pointer! Done.
lgtm with some nits https://codereview.chromium.org/1521063003/diff/160001/content/common/experim... File content/common/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1521063003/diff/160001/content/common/experim... content/common/experiments/api_key_unittest.cc:67: return api_key->ValidateOrigin(std::string(origin)); no need to explicitly convert the const char* to a std::string. The string constructor is implicit so you can just call ValidateOrigin(origin). https://codereview.chromium.org/1521063003/diff/160001/content/common/experim... content/common/experiments/api_key_unittest.cc:80: scoped_ptr<ApiKey> empty_key = ApiKey::Parse(std::string()); std::string doesn't really have the concept of a null string, so this test and the next test are effectively the same.
https://codereview.chromium.org/1521063003/diff/160001/content/common/experim... File content/common/experiments/api_key_unittest.cc (right): https://codereview.chromium.org/1521063003/diff/160001/content/common/experim... content/common/experiments/api_key_unittest.cc:67: return api_key->ValidateOrigin(std::string(origin)); On 2016/01/06 18:52:30, Marijn Kruisselbrink wrote: > no need to explicitly convert the const char* to a std::string. The string > constructor is implicit so you can just call ValidateOrigin(origin). Done. (And below, as well) https://codereview.chromium.org/1521063003/diff/160001/content/common/experim... content/common/experiments/api_key_unittest.cc:80: scoped_ptr<ApiKey> empty_key = ApiKey::Parse(std::string()); On 2016/01/06 18:52:30, Marijn Kruisselbrink wrote: > std::string doesn't really have the concept of a null string, so this test and > the next test are effectively the same. Removed, thanks.
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org, davidben@chromium.org, mek@chromium.org Link to the patchset: https://codereview.chromium.org/1521063003/#ps180001 (title: "Nits")
The CQ bit was unchecked by iclelland@chromium.org
Description was changed from ========== Add API Key parsing to blink and chromium This adds the experimental framework API key parsing mechanism, to both Blink and Chromium. (This is parsing only; validation is a separate CL) On the blink side, the code will work with origins, API names, and dates, and the actual signature validation is handed off to Chromium. The Chromium code currently only checks for general well-formedness, and accesses enough information to be able to validate the signature and expiry time. BUG=543146 ========== to ========== Add API Key parsing for experimental APIs This adds the experimental framework API key parsing mechanism to Chromium. This code deals with origins, API names, and dates. The validation currently checks for general well-formedness, and can determine, given an origin and API name, whether a given key is appropriate for use. (This is parsing only; actual signature validation is a separate CL) BUG=543146 ==========
The CQ bit was checked by iclelland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1521063003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1521063003/180001
Message was sent while issue was closed.
Description was changed from ========== Add API Key parsing for experimental APIs This adds the experimental framework API key parsing mechanism to Chromium. This code deals with origins, API names, and dates. The validation currently checks for general well-formedness, and can determine, given an origin and API name, whether a given key is appropriate for use. (This is parsing only; actual signature validation is a separate CL) BUG=543146 ========== to ========== Add API Key parsing for experimental APIs This adds the experimental framework API key parsing mechanism to Chromium. This code deals with origins, API names, and dates. The validation currently checks for general well-formedness, and can determine, given an origin and API name, whether a given key is appropriate for use. (This is parsing only; actual signature validation is a separate CL) BUG=543146 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add API Key parsing for experimental APIs This adds the experimental framework API key parsing mechanism to Chromium. This code deals with origins, API names, and dates. The validation currently checks for general well-formedness, and can determine, given an origin and API name, whether a given key is appropriate for use. (This is parsing only; actual signature validation is a separate CL) BUG=543146 ========== to ========== Add API Key parsing for experimental APIs This adds the experimental framework API key parsing mechanism to Chromium. This code deals with origins, API names, and dates. The validation currently checks for general well-formedness, and can determine, given an origin and API name, whether a given key is appropriate for use. (This is parsing only; actual signature validation is a separate CL) BUG=543146 Committed: https://crrev.com/672676d8ae5f598f56714bb81b45498476952e9f Cr-Commit-Position: refs/heads/master@{#367893} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/672676d8ae5f598f56714bb81b45498476952e9f Cr-Commit-Position: refs/heads/master@{#367893} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
