Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11946058:
Add password sync for Android. (Closed)
|
Created:
7 years, 11 months ago by shashi Modified:
7 years, 11 months ago CC:
chromium-reviews, Raghu Simha, haitaol1, akalin, tim (not reviewing) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd password sync for Android.
Android does not support password sync, enable password sync for clients
which have migrated to using Keystore. Currently, passwords will
start syncing only when sync-keystore-encryption flag is specified on the
command line.
BUG=113164
TEST=Manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178443
Patch Set 1 #
Total comments: 19
Patch Set 2 : Address comments + rebase #Patch Set 3 : #
Messages
Total messages: 12 (0 generated)
https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_components_factory_impl.cc:142: // On Android, enable password sync only when Keystore encryption I wonder, do we need this? Don't we just care about whether the cryptographer can encrypt/decrypt, which is handled within the ProfileSyncService anyways? https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:193: bool ProfileSyncService::IsPasswordSyncEnabledForAndroid() const { This method isn't really about whether it is enabled, but whether it should be right? Perhaps ShouldEnablePasswordSyncForAndroid is more appropriate a name. https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:205: return (passphraseType == syncer::CUSTOM_PASSPHRASE So, given that we already have logic to enable passwords when the passphrase is accepted, do we care about enabling them here for a custom pasphrase when there are pending keys? Isn't IsCryptographerReady sufficient? Also, move the || up onto the end of this line. https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:1016: && IsPasswordSyncEnabledForAndroid()) { move && operator to previous line https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:1018: preferredTypes.Put(syncer::PASSWORDS); This shouldn't be necessary right? From what I remember, if a type is in the failed types list, it means it was already preferred. I think this should just call failed_datatypes_handler_.OnUserChoseDatatypes again, which will clear the error'd types. Similarly, why do we return here? We'll be triggering a reconfiguration at the end of this function anyways, which should attempt to configure passwords. https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:1488: && !IsPasswordSyncEnabledForAndroid()) { move && operator to previous line https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.h (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.h:540: // synced, if Cryptographer can decrypt them. nit: "if the Cryptographer has already been initialized and does not have pending keys" is probably more accurate..
PTAL https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_components_factory_impl.cc:142: // On Android, enable password sync only when Keystore encryption This is just to hide the feature behind a flag, untill it is well tested. You are correct if cryptographer has no pending keys it should just work. On 2013/01/18 00:05:11, Nicolas Zea wrote: > I wonder, do we need this? Don't we just care about whether the cryptographer > can encrypt/decrypt, which is handled within the ProfileSyncService anyways? https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:193: bool ProfileSyncService::IsPasswordSyncEnabledForAndroid() const { Correct, thanks much better. On 2013/01/18 00:05:11, Nicolas Zea wrote: > This method isn't really about whether it is enabled, but whether it should be > right? Perhaps ShouldEnablePasswordSyncForAndroid is more appropriate a name. https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:205: return (passphraseType == syncer::CUSTOM_PASSPHRASE This is to deal with a corner case: User has a custom passphrase and has selected "encrypt only passwords". Without this conditional we will disable the password datatype because cryptographer is not ready. With passwords disabled there are no enabled encrypted datatypes. Android only prompts user for passphrase if there is an enabled encrypted datatype which needs to be decrypted. In this case there is no such encrypted datatype (since we disabled it) so there is no user prompt for entering custom passphrase. This means passwords will remain disabled permanently. On 2013/01/18 00:05:11, Nicolas Zea wrote: > So, given that we already have logic to enable passwords when the passphrase is > accepted, do we care about enabling them here for a custom pasphrase when there > are pending keys? Isn't IsCryptographerReady sufficient? > > Also, move the || up onto the end of this line. https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:1016: && IsPasswordSyncEnabledForAndroid()) { On 2013/01/18 00:05:11, Nicolas Zea wrote: > move && operator to previous line Done. https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:1018: preferredTypes.Put(syncer::PASSWORDS); PSS.GetPreferredDataTypes filters failed types, but sync_prefs_.GetPreferredDatatTypes() will give all preferred datatypes. Using failed_datatypes_handler_.OnUserChoseDatatypes may be sufficient here. I was concerned that I may be clearing errors for all other datatypes without trigerring a complete reconfig. OnUserChoseDataTypes updates some UMA stats besides a reconfig, that was the reason for return but I have simplified to just clear fail_datatypes and removed return. On 2013/01/18 00:05:11, Nicolas Zea wrote: > This shouldn't be necessary right? From what I remember, if a type is in the > failed types list, it means it was already preferred. > > I think this should just call failed_datatypes_handler_.OnUserChoseDatatypes > again, which will clear the error'd types. > > Similarly, why do we return here? We'll be triggering a reconfiguration at the > end of this function anyways, which should attempt to configure passwords. https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:1488: && !IsPasswordSyncEnabledForAndroid()) { On 2013/01/18 00:05:11, Nicolas Zea wrote: > move && operator to previous line Done. https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.h (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.h:540: // synced, if Cryptographer can decrypt them. On 2013/01/18 00:05:11, Nicolas Zea wrote: > nit: "if the Cryptographer has already been initialized and does not have > pending keys" is probably more accurate.. Done.
https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:205: return (passphraseType == syncer::CUSTOM_PASSPHRASE On 2013/01/18 02:06:31, shashi wrote: > This is to deal with a corner case: User has a custom passphrase and has > selected "encrypt only passwords". > Without this conditional we will disable the password datatype because > cryptographer is not ready. With passwords disabled there are no enabled > encrypted datatypes. > Android only prompts user for passphrase if there is an enabled encrypted > datatype which needs to be decrypted. In this case there is no such encrypted > datatype (since we disabled it) so there is no user prompt for entering custom > passphrase. This means passwords will remain disabled permanently. > > On 2013/01/18 00:05:11, Nicolas Zea wrote: > > So, given that we already have logic to enable passwords when the passphrase > is > > accepted, do we care about enabling them here for a custom pasphrase when > there > > are pending keys? Isn't IsCryptographerReady sufficient? > > > > Also, move the || up onto the end of this line. > Hmm, the IsEncryptedDatatypeEnabled method looks at the preferred types, not the running types. As long as we're not marking passwords as not preferred (which I don't think failed datatypes get marked as), it shouldn't get affected. Is the Android UI logic not using IsEncryptedDatatypeEnabled? https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:1018: preferredTypes.Put(syncer::PASSWORDS); On 2013/01/18 02:06:31, shashi wrote: > PSS.GetPreferredDataTypes filters failed types, but > sync_prefs_.GetPreferredDatatTypes() will give all preferred datatypes. > > Using failed_datatypes_handler_.OnUserChoseDatatypes may be sufficient here. I > was concerned that I may be clearing errors for all other datatypes without > trigerring a complete reconfig. OnUserChoseDataTypes updates some UMA stats > besides a reconfig, that was the reason for return but I have simplified to just > clear fail_datatypes and removed return. > > On 2013/01/18 00:05:11, Nicolas Zea wrote: > > This shouldn't be necessary right? From what I remember, if a type is in the > > failed types list, it means it was already preferred. > > > > I think this should just call failed_datatypes_handler_.OnUserChoseDatatypes > > again, which will clear the error'd types. > > > > Similarly, why do we return here? We'll be triggering a reconfiguration at the > > end of this function anyways, which should attempt to configure passwords. > I see. Yeah, I think clearing failed datatypes should be okay. Presumably any other datatypes with legitimate failures will just re-fail, which I think is okay given that we should only do this once.
https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:205: return (passphraseType == syncer::CUSTOM_PASSPHRASE IsEncryptedDatatypeEnabled uses PSS.GetPreferredDataTypes() which filters failed datatypes. Android UI uses IsPassphraseRequiredForDecryption, which is IsEncryptedDatatypeEnabled && IsPassphraseRequired. In this case since passwords are disabled we do not show a dialog. Having said that, I can change the UI logic to show a dialog when (passphrase_type == CUSTOM_PASSPHRASE && !IsCryptographerReady), this will make this much simpler. On 2013/01/18 22:55:13, Nicolas Zea wrote: > On 2013/01/18 02:06:31, shashi wrote: > > This is to deal with a corner case: User has a custom passphrase and has > > selected "encrypt only passwords". > > Without this conditional we will disable the password datatype because > > cryptographer is not ready. With passwords disabled there are no enabled > > encrypted datatypes. > > Android only prompts user for passphrase if there is an enabled encrypted > > datatype which needs to be decrypted. In this case there is no such encrypted > > datatype (since we disabled it) so there is no user prompt for entering custom > > passphrase. This means passwords will remain disabled permanently. > > > > On 2013/01/18 00:05:11, Nicolas Zea wrote: > > > So, given that we already have logic to enable passwords when the passphrase > > is > > > accepted, do we care about enabling them here for a custom pasphrase when > > there > > > are pending keys? Isn't IsCryptographerReady sufficient? > > > > > > Also, move the || up onto the end of this line. > > > > Hmm, the IsEncryptedDatatypeEnabled method looks at the preferred types, not the > running types. As long as we're not marking passwords as not preferred (which I > don't think failed datatypes get marked as), it shouldn't get affected. Is the > Android UI logic not using IsEncryptedDatatypeEnabled?
https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:205: return (passphraseType == syncer::CUSTOM_PASSPHRASE On 2013/01/18 23:20:52, shashi wrote: > IsEncryptedDatatypeEnabled uses PSS.GetPreferredDataTypes() which filters failed > datatypes. > Android UI uses IsPassphraseRequiredForDecryption, which is > IsEncryptedDatatypeEnabled && IsPassphraseRequired. > In this case since passwords are disabled we do not show a dialog. > Having said that, I can change the UI logic to show a dialog when > (passphrase_type == CUSTOM_PASSPHRASE && !IsCryptographerReady), this will make > this much simpler. > > On 2013/01/18 22:55:13, Nicolas Zea wrote: > > On 2013/01/18 02:06:31, shashi wrote: > > > This is to deal with a corner case: User has a custom passphrase and has > > > selected "encrypt only passwords". > > > Without this conditional we will disable the password datatype because > > > cryptographer is not ready. With passwords disabled there are no enabled > > > encrypted datatypes. > > > Android only prompts user for passphrase if there is an enabled encrypted > > > datatype which needs to be decrypted. In this case there is no such > encrypted > > > datatype (since we disabled it) so there is no user prompt for entering > custom > > > passphrase. This means passwords will remain disabled permanently. > > > > > > On 2013/01/18 00:05:11, Nicolas Zea wrote: > > > > So, given that we already have logic to enable passwords when the > passphrase > > > is > > > > accepted, do we care about enabling them here for a custom pasphrase when > > > there > > > > are pending keys? Isn't IsCryptographerReady sufficient? > > > > > > > > Also, move the || up onto the end of this line. > > > > > > > Hmm, the IsEncryptedDatatypeEnabled method looks at the preferred types, not > the > > running types. As long as we're not marking passwords as not preferred (which > I > > don't think failed datatypes get marked as), it shouldn't get affected. Is the > > Android UI logic not using IsEncryptedDatatypeEnabled? > That sounds good. Also, apparently the reason for my confusion was the GetPreferredDataTypes filters out the failed types (which I think might be wrong, but isn't something we need to address here).
PTAL https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/11946058/diff/1/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:205: return (passphraseType == syncer::CUSTOM_PASSPHRASE Yes the function name is confusing :), GetEnabledPreferredDataTypes may be a better name. On 2013/01/19 01:10:16, Nicolas Zea wrote: > On 2013/01/18 23:20:52, shashi wrote: > > IsEncryptedDatatypeEnabled uses PSS.GetPreferredDataTypes() which filters > failed > > datatypes. > > Android UI uses IsPassphraseRequiredForDecryption, which is > > IsEncryptedDatatypeEnabled && IsPassphraseRequired. > > In this case since passwords are disabled we do not show a dialog. > > Having said that, I can change the UI logic to show a dialog when > > (passphrase_type == CUSTOM_PASSPHRASE && !IsCryptographerReady), this will > make > > this much simpler. > > > > On 2013/01/18 22:55:13, Nicolas Zea wrote: > > > On 2013/01/18 02:06:31, shashi wrote: > > > > This is to deal with a corner case: User has a custom passphrase and has > > > > selected "encrypt only passwords". > > > > Without this conditional we will disable the password datatype because > > > > cryptographer is not ready. With passwords disabled there are no enabled > > > > encrypted datatypes. > > > > Android only prompts user for passphrase if there is an enabled encrypted > > > > datatype which needs to be decrypted. In this case there is no such > > encrypted > > > > datatype (since we disabled it) so there is no user prompt for entering > > custom > > > > passphrase. This means passwords will remain disabled permanently. > > > > > > > > On 2013/01/18 00:05:11, Nicolas Zea wrote: > > > > > So, given that we already have logic to enable passwords when the > > passphrase > > > > is > > > > > accepted, do we care about enabling them here for a custom pasphrase > when > > > > there > > > > > are pending keys? Isn't IsCryptographerReady sufficient? > > > > > > > > > > Also, move the || up onto the end of this line. > > > > > > > > > > Hmm, the IsEncryptedDatatypeEnabled method looks at the preferred types, not > > the > > > running types. As long as we're not marking passwords as not preferred > (which > > I > > > don't think failed datatypes get marked as), it shouldn't get affected. Is > the > > > Android UI logic not using IsEncryptedDatatypeEnabled? > > > > That sounds good. > > Also, apparently the reason for my confusion was the GetPreferredDataTypes > filters out the failed types (which I think might be wrong, but isn't something > we need to address here).
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/11946058/18001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/11946058/18001
Message was sent while issue was closed.
Change committed as 178443 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
