Closed Bug 1473470 Opened 6 years ago Closed 5 years ago

"Clear Private Data -> Saved Logins" interacts poorly with Sync

Categories

(Firefox for Android Graveyard :: Android Sync, defect, P1)

ARM
Android
defect

Tracking

(fennec?, firefox64 wontfix, firefox65 wontfix, firefox66+ verified)

VERIFIED FIXED
Firefox 66
Tracking Status
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.
tracking-fennec: --- → ?
Component: General → Android Sync
OS: Unspecified → Android
Hardware: Unspecified → ARM
Version: unspecified → Trunk
(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.
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: -- → P5
Blocks: 1479749
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
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.
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
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
Attachment #9032949 - Attachment description: Bug 1473470 - Part 2 - Rename ListCheckboxPreference to make it's intent obvious; JanH → Bug 1473470 - Part 2 - Rename ListCheckboxPreference to make it's intent obvious; r?JanH
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?
^^
Flags: needinfo?(petru.lingurar)
(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.
Flags: needinfo?(petru.lingurar)
(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
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` ?
Priority: P5 → P1

Noting this isn't a regression and changing the priority to P1 since it means user data loss.

This new option allows removing all logins from our login manager.
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
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
Attachment #9032948 - Attachment is obsolete: true
Attachment #9032949 - Attachment is obsolete: true
Attachment #9032950 - Attachment is obsolete: true

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.

[1] https://searchfox.org/mozilla-central/rev/c3ebaf6de2d481c262c04bb9657eaf76bf47e2ac/mobile/android/modules/Sanitizer.jsm#379

Added geckoview-reviewers in Phabricator for the above patches to not let this all rest on JanH's shoulders.

Keywords: checkin-needed

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

Keywords: checkin-needed

: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?

Flags: needinfo?(petru.lingurar)
Flags: needinfo?(petru.lingurar)
Keywords: checkin-needed

I think it should work now.

: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', '')

Flags: needinfo?(petru.lingurar)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Only part 1 landed.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(petru.lingurar)
Keywords: checkin-needed

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', '')

Flags: needinfo?(petru.lingurar)
Keywords: checkin-needed
Target Milestone: Firefox 66 → ---
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

This was successfully landed so clearing the needinfo.

Flags: needinfo?(petru.lingurar)
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Flags: qe-verify+

Verified as fixed on Nightly 66.0a1 (2019-01-16).

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: