"Clear Private Data -> Saved Logins" interacts poorly with Sync
Categories
(Firefox for Android Graveyard :: Android Sync, defect, P1)
Tracking
(fennec?, firefox64 wontfix, firefox65 wontfix, firefox66+ verified)
People
(Reporter: markh, Assigned: petru)
References
Details
(Keywords: dataloss)
Attachments
(3 files, 3 obsolete files)
via bug 1461255 - a user clearing Saved Logins doesn't work well with Sync: In some edge-cases: * Users are simply confused about they their logins are no longer syncing without associating the fact with their "clear private data" * A desktop user setting up a new device and who relies on their Android device to be the source of truth for sync data will find they no longer have these passwords available anywhere. * At some point Sync is likely to restore these logins to the device, meaning the user believes they are no longer on the device but silently were re-added. Note that it is impossible to predict when (or even if) this might happen (it basically depends on our back-end infrastructure and some account related operations they may perform). I see 3 possible options: 1) Remove this as an option of things to clear - note that Desktop's equivalent feature doesn't offer to delete saved logins. 2) Remove it just for Sync users so we don't mislead such users. 3) Adjust the process so that the logins are also deleted on the Sync server. However, this will mean that the logins will also be removed from all their synced devices, which seems unlikely to be what the user intended, so the UX challenges here seem tricky. Given desktop doesn't offer this feature, the simplest thing is probably (1), to remove that feature and have the user manage their logins manually.
Updated•6 years ago
|
Comment 1•6 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #0) > via bug 1461255 - a user clearing Saved Logins doesn't work well with Sync: > > In some edge-cases: > > * Users are simply confused about they their logins are no longer syncing > without associating the fact with their "clear private data" > > * A desktop user setting up a new device and who relies on their Android > device to be the source of truth for sync data will find they no longer have > these passwords available anywhere. > > * At some point Sync is likely to restore these logins to the device, > meaning the user believes they are no longer on the device but silently were > re-added. Note that it is impossible to predict when (or even if) this might > happen (it basically depends on our back-end infrastructure and some account > related operations they may perform). > > I see 3 possible options: > > 1) Remove this as an option of things to clear - note that Desktop's > equivalent feature doesn't offer to delete saved logins. > > 2) Remove it just for Sync users so we don't mislead such users. > > 3) Adjust the process so that the logins are also deleted on the Sync > server. However, this will mean that the logins will also be removed from > all their synced devices, which seems unlikely to be what the user intended, > so the UX challenges here seem tricky. It's terrifying to me that they're being deleted locally and _not_ remotely. That's not what my expectation is... but now I see that this is backed by the ancient SQLite logins DB, and indeed, we just "DELETE FROM logins". > Given desktop doesn't offer this feature, the simplest thing is probably > (1), to remove that feature and have the user manage their logins manually. I concur. Given that Fennec is currently in maintenance mode, however, I don't think we should act on this with particularly high priority.
Comment 2•6 years ago
|
||
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195 Needinfo :susheel if you think this bug should be re-triaged.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
If Logins are currently syncing for an FxAccount the user option to clear "Saved Logins" from "Clear private data[ on exit]" will be removed to avoid deleting saved passwords when this device is the source of truth for sync data.
Assignee | ||
Comment 4•5 years ago
|
||
This class is already used just for the "Clear private data on exit" dialog but now that it also controls whether to show or not a specific "private data" option to be cleared we need to enforce it's specialization and so we'll change it's name to PrivateDataListCheckboxPreference. Depends on D15200
Assignee | ||
Comment 5•5 years ago
|
||
In the scenarios that - before adding an FxAccount - or before checking "Logins" to be synced with an FxAccount the user had checked the option to clear "Saved Logins" upon exit, we need to forget this option and make sure that "Saved logins" will not be deleted based on the previous setting. Depends on D15201
Updated•5 years ago
|
Comment 6•5 years ago
|
||
We could go down the route of the above patches, but I wonder whether it wouldn't be simpler to just completely remove the option to clear all logins from the private data preference and instead add a context menu option to delete all logins to the login manager on Android?
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #6) > We could go down the route of the above patches, but I wonder whether it > wouldn't be simpler to just completely remove the option to clear all logins > from the private data preference and instead add a context menu option to > delete all logins to the login manager on Android? As I understand the initial feature from a UX point of view: The user has a central place - `Clear private data` from which it can easily delete all browser data, including website logins. The issue for this ticket was that FXA users would be able to "Clear private data" including "Clear logins" which in the case of the Android app being the source of truth would mean all logins would be lost for the account, something that is not immediately clear to all users. Without a clear way of informing users about the consequences of "Clear logins" the solution was to just remove that option from "Clear private data" for FXA users, similar to what is done on desktop where this option doesn't exist. IMO adding a "Delete all logins" contextual option to `about:logins` would break that initial cohesion (if the option is removed entirely from "Clear private data") and just move the issue cause to `about:logins` but would still let users with the confusion about the implications of deleting all logins while they are using a FXA.
Comment 9•5 years ago
|
||
(In reply to Petru-Mugurel Lingurar[:petru] from comment #8) > IMO adding a "Delete all logins" contextual option to `about:logins` would > break that initial cohesion (if the option is removed entirely from "Clear > private data") It would bring us more in line with Desktop, though, which, as comment #0 noted, doesn't have such an option in its equivalent data clearing dialogue, either, and it would let us avoid the complexity of having to dynamically remove that option. The only way to clear logins on Desktop is directly from the login manager. Besides, the option to clear all logins mysteriously disappearing once the user connects a Sync account isn't the best UX, either. > and just move the issue cause to `about:logins` but would > still let users with the confusion about the implications of deleting all > logins while they are using a FXA. Users can already delete their logins one-at-a-time through about:logins [1], so having an option to delete all logins conceptually wouldn't be that different. We certainly can't disable deletion of logins completely just because the user has Sync, so the login manager might as well offer a way to delete them all. And as I said, I'd rather avoid the complications of dynamically removing that option from the "Clear private data" preference (and the clear data on shut down preference, too). [1] Which, at a rough glance [2] and if I'm interpreting nalexander's comment #1 correctly, is equally problematic in connection with Sync. [2] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/toolkit/components/passwordmgr/storage-mozStorage.js
Assignee | ||
Comment 10•5 years ago
|
||
I agree, the initial patches offer a somewhat convoluted solution to a simple problem. If we are to add "Delete all logins" beside "Delete" in `about:logins`, to make things clearer to the users maybe we could add a footer saying something like "All changes will be synced to your FXA account" for FXA users which have "Logins" set to be synced in `Sync` ?
Comment 11•5 years ago
|
||
Using the delete option in the context menu already prompts for confirmation, maybe that message could be extended for users who sync? This is the about:login code: https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutLogins.xhtml https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutLogins.js and this specifically the deletion code: https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/mobile/android/chrome/content/aboutLogins.js#390-407
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Noting this isn't a regression and changing the priority to P1 since it means user data loss.
Assignee | ||
Comment 13•5 years ago
|
||
This new option allows removing all logins from our login manager.
Assignee | ||
Comment 14•5 years ago
|
||
The reason for this ticket was that it was not immediately obvious for Sync users that deleting logins on this device may affect all logins stored in user's Sync account. So it was possible that users could unintentionally loose all their login data. While we should still offer the option to remove login data, even to Sync users, we will explicitly inform them that deleting logins can affect all their synced logins. Also refactored the code to minimize duplicated code. Depends on D16026
Assignee | ||
Comment 15•5 years ago
|
||
Login data is to be removed only from "about:logins" where the users that use Sync are also informed about the perils of doing so. Depends on D16027
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
I've added new patches for JanH's recommended flow.
The "Saved logins" option will be removed from the "Clear private data" options.
Users can now only remove logins from "about:login" where a prompt informs Sync users that deleting a login/all of them - "This can affect synced logins"
Don't know if the "passwords" option from Sanitizer[1] (which was previously called from "Clear private data" to remove logins) is really used/useful to some other parts of the code so I haven't removed it.
Assignee | ||
Comment 17•5 years ago
|
||
Added geckoview-reviewers in Phabricator for the above patches to not let this all rest on JanH's shoulders.
Assignee | ||
Updated•5 years ago
|
Comment 18•5 years ago
|
||
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7034ada9fb3
Part 1 - Add new "Delete all" option to about:logins; r=JanH
Comment 19•5 years ago
|
||
:petru tried to land the patches on Phabricator, but when landing part 2 encountered this error Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmp05zHZc\npatching file mobile/android/locales/en-US/chrome/aboutLogins.properties\nHunk #1 FAILED at 6\nHunk #2 FAILED at 22\n2 out of 2 hunks FAILED -- saving rejects to file mobile/android/locales/en-US/chrome/aboutLogins.properties.rej\npatching file mobile/android/chrome/content/aboutLogins.js\nHunk #2 FAILED at 367\nHunk #3 FAILED at 388\n2 out of 3 hunks FAILED -- saving rejects to file mobile/android/chrome/content/aboutLogins.js.rej\nabort: patch failed to apply', '')
Could you please take a look?
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 20•5 years ago
|
||
I think it should work now.
Comment 21•5 years ago
|
||
:petru , still not working.
On Mon, January 14, 2019, 8:54 PM GMT+2, by nbeleuzu@mozilla.com.
Revisions: D16027 diff 51388 ← D16029 diff 51395
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpRbvTy2\npatching file mobile/android/locales/en-US/chrome/aboutLogins.properties\nHunk #1 FAILED at 6\nHunk #2 FAILED at 22\n2 out of 2 hunks FAILED -- saving rejects to file mobile/android/locales/en-US/chrome/aboutLogins.properties.rej\npatching file mobile/android/chrome/content/aboutLogins.js\nHunk #2 FAILED at 367\nHunk #3 FAILED at 388\n2 out of 3 hunks FAILED -- saving rejects to file mobile/android/chrome/content/aboutLogins.js.rej\nabort: patch failed to apply', '')
Updated•5 years ago
|
Comment 22•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Comment 24•5 years ago
|
||
Petru, part 2 and 3 have problems rebasing, could you please take a look over this? Thanks.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpWZ56rL\npatching file mobile/android/locales/en-US/chrome/aboutLogins.properties\nHunk #1 FAILED at 6\nHunk #2 FAILED at 22\n2 out of 2 hunks FAILED -- saving rejects to file mobile/android/locales/en-US/chrome/aboutLogins.properties.rej\npatching file mobile/android/chrome/content/aboutLogins.js\nHunk #1 FAILED at 4\nHunk #2 FAILED at 366\nHunk #3 FAILED at 387\n3 out of 3 hunks FAILED -- saving rejects to file mobile/android/chrome/content/aboutLogins.js.rej\nabort: patch failed to apply', '')
Updated•5 years ago
|
Comment 25•5 years ago
|
||
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ae52a3250ebf Part 2 - Inform user about synced logins if Sync is used. r=JanH https://hg.mozilla.org/integration/autoland/rev/5d0fef351a29 Part 3 - Remove "Saved logins" from "Clear private data" options. r=JanH
Comment 26•5 years ago
|
||
This was successfully landed so clearing the needinfo.
Comment 27•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae52a3250ebf
https://hg.mozilla.org/mozilla-central/rev/5d0fef351a29
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 28•5 years ago
|
||
Verified as fixed on Nightly 66.0a1 (2019-01-16).
Updated•5 years ago
|
Updated•3 years ago
|
Description
•