Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
771173002:
Added utility functions related to working with "facets". (Closed)
DescriptionAdded utility functions related to working with "facets".
A "facet" is defined as the manifestation of a logical application on a given platform, for example, as an Android application, or as a Web application accessible from a browser. Facets that belong to the same logical application can be treated as synonymous for certain purposes, e.g., sharing log-in credentials between them.
BUG=437865
Committed: https://crrev.com/fca6d78d0edb1a2b545698e419c4da857ba0b477
Cr-Commit-Position: refs/heads/master@{#307709}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments. #Patch Set 3 : Introduced FacetURI. #
Total comments: 10
Patch Set 4 : Take 3. #Patch Set 5 : Fix typo. #Patch Set 6 : Rename arguments named 'uri' to 'spec' #Patch Set 7 : Removed unused includes. #Patch Set 8 : Have proper includes. #
Total comments: 14
Patch Set 9 : Addressed comments. #Patch Set 10 : Fix typo. #Patch Set 11 : Fix Android compile errors. #
Messages
Total messages: 24 (6 generated)
engedy@chromium.org changed reviewers: + mkwst@chromium.org
Mike, as discussed, I have carved out the utility methods from https://codereview.chromium.org/767163005/. Please take a look.
Thanks for putting this together! I'd suggest changing the CL description a bit so that it's clear what's actually being done. Unless you know what "facets" are, it's totally opaque at the moment. :) Perhaps a link to the design doc. Or a brief description of the use case would be helpful? I also have a simplifying suggestion with regard to URLs. You're replicating a lot of work that I expect GURL is already doing. I'd prefer to use it more. :) https://codereview.chromium.org/771173002/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.cc:21: url::ParseStandardURL(uri.c_str(), uri.size(), &url_parsed); Why parse this manually? Wouldn't it be easier to pass it to GURL, let GURL do the work, and then test 'is_valid()'? Are there any cases in which |uri| would be valid according to this method, but not accepted by GURL? https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.cc:36: url::ParseStandardURL(uri.c_str(), uri.size(), &url_parsed); Ditto. https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.cc:55: std::sort(a_sorted.begin(), a_sorted.end()); Hrm. Where do you intend to call this equality method? Sorting each time seems potentially expensive. Perhaps we could store the data sorted? That said, we're probably working with very small data sets, so optimizing this might not be worth the time. Up to you. :) https://codereview.chromium.org/771173002/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_utils.h (right): https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.h:24: // * android://<cert_hash>@<package_name>/ If we change this to `android://<cert_hash>/<package_name>/`, we could probably use GURLs rather than strings in most places. It also might make marginally more semantic sense: the "authority" section of the URL maps more cleanly to the cert hash than to the package name. https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.h:55: // present and will not actually verify their inner format. Hrm. If you're not parsing these, then claiming that it's "valid" is difficult, isn't it? If we could migrate to GURL, this would be simpler, as we could reuse its parsing logic, and just examine the scheme to determine the facet type.
Please see my responses. I apologize for not being able to answer "Done" to practically any of your comments. :-) I have also extended the description a bit. https://codereview.chromium.org/771173002/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.cc:21: url::ParseStandardURL(uri.c_str(), uri.size(), &url_parsed); On 2014/12/02 16:33:06, Mike West (OOO) wrote: > Why parse this manually? Wouldn't it be easier to pass it to GURL, let GURL do > the work, and then test 'is_valid()'? Are there any cases in which |uri| would > be valid according to this method, but not accepted by GURL? Using the GURL constructor is doable, but rather heavy-weight, as it will not only parse the URL, but also canonicalize it. I would be fine with using GURL. Note that either, the rest of the function (i.e. checking existence and non-existence of components) would need to be kept. https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.cc:36: url::ParseStandardURL(uri.c_str(), uri.size(), &url_parsed); On 2014/12/02 16:33:06, Mike West (OOO) wrote: > Ditto. This is, actually, different, see my comment in the header. (TL;DR: GURL's high-level parser freaks out when it sees the "android" scheme, and shoves everything into the path component. Therefore, this is the only solution. Should I add more comments?) https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.cc:55: std::sort(a_sorted.begin(), a_sorted.end()); On 2014/12/02 16:33:06, Mike West (OOO) wrote: > Hrm. Where do you intend to call this equality method? Sorting each time seems > potentially expensive. Perhaps we could store the data sorted? > > That said, we're probably working with very small data sets, so optimizing this > might not be worth the time. Up to you. :) I am not too worried about this, as most equivalence classes will contain 2 or 3 URIs, and this will be seldom called. I have added a comment to the header to warn the caller. https://codereview.chromium.org/771173002/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_utils.h (right): https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.h:24: // * android://<cert_hash>@<package_name>/ On 2014/12/02 16:33:06, Mike West (OOO) wrote: > If we change this to `android://<cert_hash>/<package_name>/`, we could probably > use GURLs rather than strings in most places. It also might make marginally more > semantic sense: the "authority" section of the URL maps more cleanly to the cert > hash than to the package name. I am afraid changing the format is no longer an option. In either case, sadly, we cannot use GURL, as it tries to be too clever. Based on the scheme, it parses the rest of the spec differently: when it sees the "android" scheme, it freaks out, and classifies it as a weird URL alongside "javascript:" and "data:" URLs, and shoves everything into the 'path' component. Not too useful. Given that the android:// scheme is an internal implementation detail, is in no way standardized to any degree, I could see no justification for letting our GURL know about it. So that leaves us with either defining a custom FacetURI type, or just using std::string and ParseStandardURL. WDYT? https://codereview.chromium.org/771173002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_utils.h:55: // present and will not actually verify their inner format. On 2014/12/02 16:33:06, Mike West (OOO) wrote: > Hrm. If you're not parsing these, then claiming that it's "valid" is difficult, > isn't it? > > If we could migrate to GURL, this would be simpler, as we could reuse its > parsing logic, and just examine the scheme to determine the facet type. Actually, we are parsing it to the extent where we can decide the facet type and stuff, just not checking the components too deeply (signature, APK name). I guess I should clarify the comment.
On 2014/12/02 19:24:55, engedy wrote: > Please see my responses. I apologize for not being able to answer "Done" to > practically any of your comments. :-) Too bad. :( cross-time-zone LGTM % bits and pieces below. > https://codereview.chromium.org/771173002/diff/1/components/password_manager/... > components/password_manager/core/browser/affiliation_utils.cc:21: > url::ParseStandardURL(uri.c_str(), uri.size(), &url_parsed); > On 2014/12/02 16:33:06, Mike West (OOO) wrote: > Using the GURL constructor is doable, but rather heavy-weight, as it will not > only parse the URL, but also canonicalize it. I think you'll need to do that for webby URLs, if only to ensure that weirdnesses like http://0321.0x86.161.0043 are handled correctly, and are parsed as equivalent to representations like http://209.134.161.35/. > I would be fine with using GURL. Note that either, the rest of the function > (i.e. checking existence and non-existence of components) would need to be kept. Noted. > https://codereview.chromium.org/771173002/diff/1/components/password_manager/... > components/password_manager/core/browser/affiliation_utils.cc:36: > url::ParseStandardURL(uri.c_str(), uri.size(), &url_parsed); > On 2014/12/02 16:33:06, Mike West (OOO) wrote: > > Ditto. > > This is, actually, different, see my comment in the header. (TL;DR: GURL's > high-level parser freaks out when it sees the "android" scheme, and shoves > everything into the path component. Therefore, this is the only solution. Should > I add more comments?) Please do. :( > https://codereview.chromium.org/771173002/diff/1/components/password_manager/... > components/password_manager/core/browser/affiliation_utils.cc:55: > std::sort(a_sorted.begin(), a_sorted.end()); > On 2014/12/02 16:33:06, Mike West (OOO) wrote: > I am not too worried about this, as most equivalence classes will contain 2 or 3 > URIs, and this will be seldom called. I have added a comment to the header to > warn the caller. Brilliant! > So that leaves us with either defining a custom FacetURI type, or just using > std::string and ParseStandardURL. WDYT? I'm not a huge fan of either option, honestly. For the moment, let's just run with the current implementation. > https://codereview.chromium.org/771173002/diff/1/components/password_manager/... > components/password_manager/core/browser/affiliation_utils.h:55: // present and > will not actually verify their inner format. > On 2014/12/02 16:33:06, Mike West (OOO) wrote: > Actually, we are parsing it to the extent where we can decide the facet type and > stuff, just not checking the components too deeply (signature, APK name). I > guess I should clarify the comment. Please do!
Hmm, you are right. Verifying the URI to "some" extent makes no sense. There should only be a single spelling of a URI referring to a particular facet, so then we can use string comparison. I am adding canonicalization.
Patchset #3 (id:40001) has been deleted
On 2014/12/03 14:09:47, engedy wrote: > Hmm, you are right. Verifying the URI to "some" extent makes no sense. There > should only be a single spelling of a URI referring to a particular facet, so > then we can use string comparison. I am adding canonicalization. Mike, I have rewritten this CL greatly. Could you please take another look?
I think this is probably the right direction to go, and not as ugly as I was semi-expecting. :) LGTM if you address the comments below. I'm going to be offline pretty much all of Thursday and Friday, so if you feel like you'd appreciate a second look before you land it after you make some changes, please find someone locally to skim the changes; otherwise you'll be waiting on me until Saturday. https://codereview.chromium.org/771173002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.cc:76: base::Base64Decode(base64_encoded_hash, &unused_decoded_hash) && Hrm. It surprises me a little bit that we don't support base64-url decoding more cleanly. https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.cc:81: !canonical_parsed.ref.is_valid(); Most of this is shared with the case above. Pretty much everything except the base64 and username check. I'd suggest grabbing the shared bits, and exiting early if they aren't true. Then these two checks are greatly simplified and focused only on the non-shared aspects. https://codereview.chromium.org/771173002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_utils.h (right): https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.h:55: class FacetURI { Nit: I think you should split this out into a separate file. https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.h:62: // Constructs a FacetURI instance to encapsulates the canonical form of |uri|. s/encapsulates/encapsulate/ https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.h:66: // Constructs a valid FacetURI instance from a valid |canonical_facet_uri|. Can you describe here what happens if an invalid URI is provided?
Patchset #4 (id:80001) has been deleted
Mike, take #3... Turns out that the server-side API no longer liked the URLs after canonicalizing them the way I did, so I fixed that. PTAL. https://codereview.chromium.org/771173002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.cc:76: base::Base64Decode(base64_encoded_hash, &unused_decoded_hash) && On 2014/12/03 22:58:35, Mike West (OOO until 9th) wrote: > Hrm. It surprises me a little bit that we don't support base64-url decoding more > cleanly. I could not find any support for this in the common directories. But I realized that we do not really have to parse the whole thing anyway. https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.cc:81: !canonical_parsed.ref.is_valid(); On 2014/12/03 22:58:35, Mike West (OOO until 9th) wrote: > Most of this is shared with the case above. Pretty much everything except the > base64 and username check. > > I'd suggest grabbing the shared bits, and exiting early if they aren't true. > Then these two checks are greatly simplified and focused only on the non-shared > aspects. Unfortunately, I have realized that we cannot canonicalize Android facet URIs this way, see reasons in the code. https://codereview.chromium.org/771173002/diff/60001/components/password_mana... File components/password_manager/core/browser/affiliation_utils.h (right): https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.h:55: class FacetURI { On 2014/12/03 22:58:35, Mike West (OOO until 9th) wrote: > Nit: I think you should split this out into a separate file. That would mean having really tiny files. How about renaming this to something better? https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.h:62: // Constructs a FacetURI instance to encapsulates the canonical form of |uri|. On 2014/12/03 22:58:35, Mike West (OOO until 9th) wrote: > s/encapsulates/encapsulate/ Done. https://codereview.chromium.org/771173002/diff/60001/components/password_mana... components/password_manager/core/browser/affiliation_utils.h:66: // Constructs a valid FacetURI instance from a valid |canonical_facet_uri|. On 2014/12/03 22:58:35, Mike West (OOO until 9th) wrote: > Can you describe here what happens if an invalid URI is provided? Done.
Removed unused includes.
Have proper includes.
https://codereview.chromium.org/771173002/diff/180001/components/password_man... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:42: bool ContainsOnlyAlphanumericAnd(const base::StringPiece& input, I'd suggest that this method accept an enum rather than a StringPiece; perhaps HASH_CHARACTERS and PACKAGE_NAME_CHARACTERS? Right now, it's tough to understand why you're accepting certain characters and not others. https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:85: bool CanonicalizeHashComponent(const base::StringPiece& input_hash, It doesn't look like you're canonicalizing the string with regard to padding. Shouldn't you not only verify that the string ends with one or two '=' characters, but also verify that it ends with the _correct_ number of '=' characters, and add them in if they're not already present? If not, you're not really canonicalizing. https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:87: // We need net::UnescapeRule::URL_SPECIAL_CHARS for the '='. For which '='? The padding at the end of the hash? https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:99: (first_padding == std::string::npos || I think it makes sense to extract the padding checks out into a function in the anonymous namespace. That would allow you to name it in a way that makes the effect clear (which it isn't here). https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:108: // meaningful will be written to |canonical_output|. Why would you write something even in a failure case? https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:123: // meaningful will be written to |canonical_uri|. Ditto: if you've failed, why write anything to the output? https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:197: DCHECK(other.is_valid_); Hrm. DCHECK seems like a bit much here; perhaps it's more reasonable to treat invalid facets as unique (e.g. != always true).
https://codereview.chromium.org/771173002/diff/180001/components/password_man... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:42: bool ContainsOnlyAlphanumericAnd(const base::StringPiece& input, On 2014/12/09 12:13:34, Mike West wrote: > I'd suggest that this method accept an enum rather than a StringPiece; perhaps > HASH_CHARACTERS and PACKAGE_NAME_CHARACTERS? Right now, it's tough to understand > why you're accepting certain characters and not others. I have added name constants at the call sites, PTAL. https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:85: bool CanonicalizeHashComponent(const base::StringPiece& input_hash, On 2014/12/09 12:13:34, Mike West wrote: > It doesn't look like you're canonicalizing the string with regard to padding. > Shouldn't you not only verify that the string ends with one or two '=' > characters, but also verify that it ends with the _correct_ number of '=' > characters, and add them in if they're not already present? > > If not, you're not really canonicalizing. Done. https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:87: // We need net::UnescapeRule::URL_SPECIAL_CHARS for the '='. On 2014/12/09 12:13:34, Mike West wrote: > For which '='? The padding at the end of the hash? Yes, clarified. https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:99: (first_padding == std::string::npos || On 2014/12/09 12:13:34, Mike West wrote: > I think it makes sense to extract the padding checks out into a function in the > anonymous namespace. That would allow you to name it in a way that makes the > effect clear (which it isn't here). Done. https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:108: // meaningful will be written to |canonical_output|. On 2014/12/09 12:13:34, Mike West wrote: > Why would you write something even in a failure case? As discussed, this was for consistency with url_canon, which did this to produce something human readable in an effort to help debug why the canonicalization failed. But removed the comment and slightly optimized the code. https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:123: // meaningful will be written to |canonical_uri|. On 2014/12/09 12:13:34, Mike West wrote: > Ditto: if you've failed, why write anything to the output? Done. https://codereview.chromium.org/771173002/diff/180001/components/password_man... components/password_manager/core/browser/affiliation_utils.cc:197: DCHECK(other.is_valid_); On 2014/12/09 12:13:34, Mike West wrote: > Hrm. DCHECK seems like a bit much here; perhaps it's more reasonable to treat > invalid facets as unique (e.g. != always true). I would prefer to keep it. The factory method clearly states that you should discard an invalid instance. I really do want to enforce that nobody passes around invalid Facet URI instances. I agree this is somewhat ugly. Ideally, we should enforce the invariant in the ctor, and throw if it is violated. Let me know if there is a nicer solution for this without exceptiobns.
LGTM, thanks for going another round.
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771173002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771173002/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/fca6d78d0edb1a2b545698e419c4da857ba0b477 Cr-Commit-Position: refs/heads/master@{#307709} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
