Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2981293003:
Save Android Autofill credentials in the right format. (Closed)
DescriptionSave Android Autofill credentials in the right format.
Android Autofill saves credentials in the wrong format. origin and signon_realm have no trailing '/' which prevents proper handling in Chrome.
This CL migrates them to the proper format with '/'. The original entry stays on the server and it's kept up to date. In the future the credentials without '/' are to be removed from the Sync data.
BUG=739101
Review-Url: https://codereview.chromium.org/2981293003
Cr-Commit-Position: refs/heads/master@{#488421}
Committed: https://chromium.googlesource.com/chromium/src/+/17634c18f71fffdd50e973902aadb0e90207f4b6
Patch Set 1 #Patch Set 2 : New algorithm #
Total comments: 31
Patch Set 3 : comments #Patch Set 4 : test comment #
Messages
Total messages: 29 (15 generated)
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vasilii@chromium.org changed reviewers: + engedy@chromium.org
Hi Balasz, please review. I just realized that maybe my algorithm of always deleting the local entry isn't ideal. Let me know what you think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Very high level comment: as an alternative have you considered fixing the issue (by appending trailing slashes) when we query the passwordstore for affiliation Android realms, and accepting that we have both formats in the store? Would that be even more painful?
On 2017/07/20 08:36:22, engedy wrote: > Very high level comment: as an alternative have you considered fixing the issue > (by appending trailing slashes) when we query the passwordstore for affiliation > Android realms, and accepting that we have both formats in the store? Would that > be even more painful? Regarding the first part. It's a similar algorithm "Request all the credentials and do the right thing" but in another place. I do the same stuff in PasswordSyncableService which is the first gateway for the wrong credentials. In the future I'll be cleaning the wrong entries, thus, PasswordSyncableService seems like a better place for that. Also no affiliation magic is happening with the custom passphrase. Regarding the second point (have both formats). I think it makes sense. The user will see two entries in chrome://settings/passwords but it's actually good. He can delete the wrong one (with 50% probability :) I'll try to fix the algorithm.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I made the change we discussed. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments and a couple question on the prod code. There are perhaps a couple of dumb questions, but I wanted to make sure we are not missing anything. Feel free to close dumb questions with "Dumb.". :-) Also, two extra questions: -- We are never deleting local entries on start-up, right? -- If the user deletes the correct local entry, it will be recreated on next startup, right? https://codereview.chromium.org/2981293003/diff/20001/components/password_man... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:124: std::string GetAndroidAutofillSignonRealm(std::string android_autofill_realm) { For extra readability in the code below, how about calling these: Incorrect-/CorrectAndroidSignonRealms, Short-/LongAndroidSignonRealms We can define what these mean here in a comment, but perhaps the rest of the code does not need to know where the bad entries are coming from? https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:131: std::string GetCorrectAndroidSignonRealm(std::string android_realm) { Do we have complete confidence in [1] that we will never have empty |origins| here? [1]: https://cs.chromium.org/chromium/src/components/password_manager/core/browser... https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:137: // Android autofill saves credential in a different format without trailing '/'. typo: credentials https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:138: // Return a sync tag for the style used by Android Autofill in the GMS Core v12. typo: s/in the GMS/in GMS/ https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:142: std::string origin = GetAndroidAutofillSignonRealm(password.origin()); Just to triple check: -- Do you have some concrete example credentials originating from Android Autofill, where you could verify that the trailing '/' is missing on the |origin| field too? -- As per the original design, the |origin| fields was supposed to be left empty for Android credentials. In the end, that's not the case anywhere, right? In particular, Autofill fills out the |origin| field, right? https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:236: newest_data = nit: Isn't this actually longer than a good old range-based for loop? https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:250: const std::string correct_tag = AndroidCorrectSyncTag(*newest_data); For brevity, have you considered precalculating the correct/incorrect origins and signon-realms here as well? That would also allow adding the following sanity check: DCHECK_EQ(GURL(incorrect_origin).spec(), incorrect_origin); Which is true today, AFAICT, but only because GURL is not smart enough to canonicalize android:// URIs. It's weird to rely on "missing functionality". Maybe this even warrant a tiny test case on its own? :-/ https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:258: password_data.mutable_password()->mutable_client_only_encrypted_data(); To double check: are you familiar with how non-encrypted fields are handled? I suppose they are derived / updated automatically, and we don't have to worry about them, right? https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:267: // Set the Andoroid Autofill Sync entry if needed. optional nit: Consider blank lines between blocks. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:566: sync_data_map[MakePasswordSyncTag(*password_specifics)] = Should we add a DCHECK() that there was no such key yet, or DCHECK_EQ(sync_data_map.size(), sync_data.size()) after the loop? https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:573: // An incorrect entry is copied in the correct format so Chrome can make phrasing nit: For each incorrect entry, a duplicate of is created in the correct format, so... https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:574: // use of it. The wrong sync entries are not deleted for now. phrasing nit: s/wrong/incorrect/ https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:577: auto it_android_incorrect = sync_data_map.find(incorrect_tag); For extra readability, have you considered s/android/remote/g or s/android/sync/g? It's a bit misleading next to all the Android autofill stuff. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:582: unmatched_data_from_password_db->find(incorrect_tag); Should we fall back to default handling if (it != it_android_incorrect && it != it_android_correct)? E.g., it can happen if the trailing slash is there for |origin|, but missing for |signon_realm|, which should not happen... Also, did we verify that the Android Autofill folks actually added the trailing slash to both |signon_realm| and |origin|?
> Also, two extra questions: > -- We are never deleting local entries on start-up, right? Correction: never deleting from Sync on start-up.
LGTM % comments here and before. I only skimmed through the unit test. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:765: // Transforms |val| into |count| numbers from 0 to |count|. nit: 1 to |count|, inclusive.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/20 18:48:37, engedy wrote: > > Also, two extra questions: > > -- We are never deleting local entries on start-up, right? > > Correction: never deleting from Sync on start-up. We could but we don't. On start-up we should unite local data with the sync data only. -- If the user deletes the correct local entry, it will be recreated on next startup, right? Only if there is an incorrect one. In such a scenario I would consider it acceptable.
https://codereview.chromium.org/2981293003/diff/20001/components/password_man... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:124: std::string GetAndroidAutofillSignonRealm(std::string android_autofill_realm) { On 2017/07/20 18:34:14, engedy wrote: > For extra readability in the code below, how about calling these: > > Incorrect-/CorrectAndroidSignonRealms, > Short-/LongAndroidSignonRealms > > We can define what these mean here in a comment, but perhaps the rest of the > code does not need to know where the bad entries are coming from? Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:131: std::string GetCorrectAndroidSignonRealm(std::string android_realm) { On 2017/07/20 18:34:14, engedy wrote: > Do we have complete confidence in [1] that we will never have empty |origins| > here? > > [1]: > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... I didn't get the question. This particular function won't crash if |origin| is empty. I don't know why [1] handles Android credentials specifically. It seems like |origin| shouldn't be empty for all the credentials. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:137: // Android autofill saves credential in a different format without trailing '/'. On 2017/07/20 18:34:14, engedy wrote: > typo: credentials Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:138: // Return a sync tag for the style used by Android Autofill in the GMS Core v12. On 2017/07/20 18:34:14, engedy wrote: > typo: s/in the GMS/in GMS/ Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:142: std::string origin = GetAndroidAutofillSignonRealm(password.origin()); On 2017/07/20 18:34:14, engedy wrote: > Just to triple check: > > -- Do you have some concrete example credentials originating from Android > Autofill, where you could verify that the trailing '/' is missing on the > |origin| field too? > > -- As per the original design, the |origin| fields was supposed to be left > empty for Android credentials. In the end, that's not the case anywhere, right? > In particular, Autofill fills out the |origin| field, right? - Yes, if you open the email thread then you can find a mail from me (I just added you there) where I copypasted the real origin and signon_realm from my machine. - Per documentation and all the personal/testing entries on my machine, Android always sets |origin| and |signon_realm| to the same value. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:236: newest_data = On 2017/07/20 18:34:14, engedy wrote: > nit: Isn't this actually longer than a good old range-based for loop? Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:250: const std::string correct_tag = AndroidCorrectSyncTag(*newest_data); On 2017/07/20 18:34:14, engedy wrote: > For brevity, have you considered precalculating the correct/incorrect origins > and signon-realms here as well? > > That would also allow adding the following sanity check: > > DCHECK_EQ(GURL(incorrect_origin).spec(), incorrect_origin); > > Which is true today, AFAICT, but only because GURL is not smart enough to > canonicalize android:// URIs. It's weird to rely on "missing functionality". > Maybe this even warrant a tiny test case on its own? :-/ Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:258: password_data.mutable_password()->mutable_client_only_encrypted_data(); On 2017/07/20 18:34:14, engedy wrote: > To double check: are you familiar with how non-encrypted fields are handled? I > suppose they are derived / updated automatically, and we don't have to worry > about them, right? Right, not our business. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:267: // Set the Andoroid Autofill Sync entry if needed. On 2017/07/20 18:34:14, engedy wrote: > optional nit: Consider blank lines between blocks. Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:566: sync_data_map[MakePasswordSyncTag(*password_specifics)] = On 2017/07/20 18:34:14, engedy wrote: > Should we add a DCHECK() that there was no such key yet, or > DCHECK_EQ(sync_data_map.size(), sync_data.size()) after the loop? Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:573: // An incorrect entry is copied in the correct format so Chrome can make On 2017/07/20 18:34:14, engedy wrote: > phrasing nit: For each incorrect entry, a duplicate of is created in the correct > format, so... Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:574: // use of it. The wrong sync entries are not deleted for now. On 2017/07/20 18:34:14, engedy wrote: > phrasing nit: s/wrong/incorrect/ Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:577: auto it_android_incorrect = sync_data_map.find(incorrect_tag); On 2017/07/20 18:34:14, engedy wrote: > For extra readability, have you considered s/android/remote/g or > s/android/sync/g? It's a bit misleading next to all the Android autofill stuff. Done. https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:582: unmatched_data_from_password_db->find(incorrect_tag); On 2017/07/20 18:34:14, engedy wrote: > Should we fall back to default handling if (it != it_android_incorrect && it != > it_android_correct)? > > E.g., it can happen if the trailing slash is there for |origin|, but missing for > |signon_realm|, which should not happen... > > Also, did we verify that the Android Autofill folks actually added the trailing > slash to both |signon_realm| and |origin|? Good point. They claim that they use the same library as YOLO. I can follow up.
https://codereview.chromium.org/2981293003/diff/20001/components/password_man... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service_unittest.cc:765: // Transforms |val| into |count| numbers from 0 to |count|. On 2017/07/20 19:13:07, engedy wrote: > nit: 1 to |count|, inclusive. Done.
The CQ bit was checked by vasilii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2981293003/#ps60001 (title: "test comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> -- If the user deletes the correct local entry, it will be recreated on next > startup, right? > > Only if there is an incorrect one. In such a scenario I would consider it > acceptable. Yeah, that's what I meant, and I agree.
https://codereview.chromium.org/2981293003/diff/20001/components/password_man... File components/password_manager/core/browser/password_syncable_service.cc (right): https://codereview.chromium.org/2981293003/diff/20001/components/password_man... components/password_manager/core/browser/password_syncable_service.cc:131: std::string GetCorrectAndroidSignonRealm(std::string android_realm) { On 2017/07/20 19:50:17, vasilii wrote: > On 2017/07/20 18:34:14, engedy wrote: > > Do we have complete confidence in [1] that we will never have empty |origins| > > here? > > > > [1]: > > > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... > > I didn't get the question. This particular function won't crash if |origin| is > empty. > I don't know why [1] handles Android credentials specifically. It seems like > |origin| shouldn't be empty for all the credentials. Correct, no crashes, I was just wondering if having weird credentials around with '/' origins will mess anything up.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1500580566181150,
"parent_rev": "da0a4f9e5cabf886dc27ac4b7dcbd3057a42f248", "commit_rev":
"17634c18f71fffdd50e973902aadb0e90207f4b6"}
Message was sent while issue was closed.
Description was changed from ========== Save Android Autofill credentials in the right format. Android Autofill saves credentials in the wrong format. origin and signon_realm have no trailing '/' which prevents proper handling in Chrome. This CL migrates them to the proper format with '/'. The original entry stays on the server and it's kept up to date. In the future the credentials without '/' are to be removed from the Sync data. BUG=739101 ========== to ========== Save Android Autofill credentials in the right format. Android Autofill saves credentials in the wrong format. origin and signon_realm have no trailing '/' which prevents proper handling in Chrome. This CL migrates them to the proper format with '/'. The original entry stays on the server and it's kept up to date. In the future the credentials without '/' are to be removed from the Sync data. BUG=739101 Review-Url: https://codereview.chromium.org/2981293003 Cr-Commit-Position: refs/heads/master@{#488421} Committed: https://chromium.googlesource.com/chromium/src/+/17634c18f71fffdd50e973902aad... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/17634c18f71fffdd50e973902aad... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
