Specific options from 'Clear private data on exit' disconnect Firefox account

NEW
Unassigned

Status

()

defect
P2
major
11 months ago
2 months ago

People

(Reporter: stefan.deiac, Unassigned)

Tracking

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62- wontfix, firefox63+ wontfix, firefox64- wontfix, firefox65 wontfix, firefox66 affected, firefox67 affected, firefox68 affected)

Details

(Whiteboard: [geckoview:fenix:p3])

Reporter

Description

11 months ago
Environment: 
Device: Samsung Galaxy S8 (Android 8.0.0), LG G4 (Android 6.0.0), Sony Xperia Z3 (Android 5.1.1), Google Pixel C (Android 9.0) and Google Pixel (Android 9.0);
Build: Nightly 63.0a1 (2018-07-30);

Preconditions: Make sure you are logged in with Firefox account.

Steps to reproduce:
1. Go to settings->Privacy->Clear private data on exit;
2. Check all options and push the "Set" button;
3. Return on Firefox and push the "Quit" option from 3 dots settings.
4. Reopen Firefox and go to settings->account and tap on username.

Expected result:
'Manage accounts' page is presented. 

Actual result:
Sign in page it's displayed. 

Notes:
Sync is working on both expected/actual results.
Reporter

Comment 1

11 months ago
Not reproducible on: Nexus 9 (Android 6.0.1), Nokia 6 Android (7.1.1) and Nexus 5 (Android 6.0.1).
Severity: normal → major
(In reply to Stefan Deiac from comment #0)
> 2. Check all options and push the "Set" button;

It would be helpful to narrow this down a bit further if possible.
Reporter

Comment 3

11 months ago
(In reply to Jan Henning [:JanH] from comment #2)
> (In reply to Stefan Deiac from comment #0)
> > 2. Check all options and push the "Set" button;
> 
> It would be helpful to narrow this down a bit further if possible.

Hi,
I tested all options from 'Clear private data on exit' separated and the option who make the issue are: 'Cookies & active logins'
and 'Offline website data'.
Summary: Firefox account is disconnected after 'Quit' option it's used. → Specific options from 'Clear private data on exit' disconnect Firefox account
I see that 61 and 62 are both marked as affected and there us a tracking request for 62 and 63, Stefan, is this a regression or a long-standing bug?
Flags: needinfo?(stefan.deiac)
Reporter

Comment 5

11 months ago
(In reply to Pascal Chevrel:pascalc from comment #4)
> I see that 61 and 62 are both marked as affected and there us a tracking
> request for 62 and 63, Stefan, is this a regression or a long-standing bug?

I tested both options and the behavior is different:
1. With the 'cookies & active logins' option checked I can't find a regression because the issue seems to be very old.
2. With the option 'offline website data' checked I found a regression:  
Last good build - 2018-01-19
First bad build - 2018-01-19 18:02:41.305000
Pushlog - https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6ffbba9ce0ef9ec77a63445f068f2e218ed4830f&tochange=55e8bfe2dc423584750bdd08eb469ef98f50b38e

My opinion is that the login account should be separated from login on web, the login of Firefox account is strict on application and not on a web page.
That pushlog doesn't make sense for this, though.
The regression range is unlikely to be correct since the only patch there was updating a css reference test. I'll let Liz decide if she wants to track it for 62.
Reporter

Comment 8

11 months ago
Sorry for mistake, the regression range was incorrect. For 'offline website data' option from comment 5 I found also the same behavior as 'cookies & active logins' option.
Flags: needinfo?(stefan.deiac)
I'm going to mark this fix-optional for 62, but if we come up with a fix we could still potentially uplift it.
According to comment 8 no regression, though.
Keywords: regression
Hardware: ARM → All
Reporter

Comment 11

9 months ago
Update: After step 3, also if you go on History->synced devices you will see the 'welcome to sync' notification that means the account it's disconnected.
Marking as fix-optional for 63 as we are in beta now, I'd take a low risk patch while we are still in the early beta period.
Wontfix for 63, I'll let Julien decide if it should be tracked for 64.
Sounds like this is an old issue, I don't see a need to track.
Reporter

Updated

8 months ago
Andreas, do you think this should be a priority to investigate?  Maybe depending on your assessment of user impact. It doesn't seem consistently reproducible but we may have some duplicates.
Flags: needinfo?(abovens)
Priority: -- → P2
Restrict Comments: true

Comment 16

7 months ago
I wonder if this issue is related in any way with bug 1464003
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
This relates to a very specific sequence of actions, which is unlikely to happen often, but it would still be good to get this investigated and fixed, as there might be a larger underlying problem.
Flags: needinfo?(abovens)
I investigated this a bit and found that clearing `Cookies & active logins` [1] or `Offline website data` [2] do get the impression that the Sync account is disconnected because tapping it directs the user to login again and not show the interface to manage the account.

First I thought that this is normal, because the user has specifically cleared `Cookies & active logins` but then I saw that the Sync functionality is still working.
Then I saw that on desktop, if you choose to clear `Cookies and Site Data`, upon going to `Firefox Account` -> `Manage account` you don't have to login again and have full access to manage the account.

The issue I think is related to the iframe which attempts to login the user to the fxa server but I don't know what data that expects or where in code is this handled.
NI-ing Grisha since this is more of a FXA issue.


[1] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/mobile/android/modules/Sanitizer.jsm#119
[2] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/mobile/android/modules/Sanitizer.jsm#187
Flags: needinfo?(gkruglov)
Assignee: petru.lingurar → nobody
Status: ASSIGNED → NEW
Component: General → Android Sync

Comment 19

6 months ago
Janet, Grisha has moved off Sync, can you find someone to help us with this?
Flags: needinfo?(gkruglov) → needinfo?(jdragojevic)
Note that I don't think this is a sync bug - it's that the browser's "clear" functionality also deletes sync credentials.

Isn't Softvision available for bugs like this?
Flags: needinfo?(jdragojevic) → needinfo?(sdaswani)
Was hoping Grisha would know better what data is important and needs to be kept and just the best way to handle this but if he's not available anymore I'll look into getting a patch ready.
Flags: needinfo?(sdaswani)
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
(In reply to Petru-Mugurel Lingurar[:petru] from comment #21)
> Was hoping Grisha would know better what data is important and needs to be
> kept and just the best way to handle this but if he's not available anymore
> I'll look into getting a patch ready.

There are multiple things at play here:

1) There's an Android Account object that is displayed in Android UI, like the Settings App.

2) There's regular Web Platform cookies, localStorage, etc, that are used by accounts.firefox.com to maintain login state.

If I understand the ticket, part 1) is working fine.  It's part 2) that is surprising to the user.

We talked at some length about how to have the browser tell accounts.firefox.com the login state, but haven't implemented anything meaningful in this direction (that I'm aware of) because it changes the direction of information flow.  That is, accounts.firefox.com right now hands out authentication tokens to the browser but otherwise manages its own Web Platform pieces (cookies, etc).  We were worried about changing the security posture of the system by having the browser tell accounts.firefox.com, "you're logged in as this" by a different channel than Web Platform bits (cookies, etc).  It's similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1500828 and related tickets.

So the ticket _is_ surprising but it's also the expected behaviour right now.  Now, how to make things better.  The easiest thing, which I think has been pursued on Desktop, is to make the FxA credentials special and not clear them when we clear private data.  But that's also surprising to a user, who expects _everything_ to be cleared!  I.e., they clear private data, and then find another user can manage their FxA without having to sign in.  Is that expected?  Probably not.

So my guess is that we should do in Fennec what is done in Desktop.  But now I'm looking for filtering in Desktop and I can't find it!
Desktop doesn't need state in cookies nor in localStorage to maintain the login state - it uses a "saved login", which can be seen in the list of saved passwords as a "chrome://" URL.

Desktop doesn't have this issue because saved logins are *not* deleted when the user selects this option - "active logins" isn't the list of saved passwords.

I take it from bug 1473470 that Android does clear all saved logins in this scenario, and in that bug I suggested that it simply doesn't do that, which would be then offer parity with Desktop and also solve this issue.
Thanks Nick and Mark!

I see that on desktop there is a `chrome://FirefoxAccounts (Firefox Accounts credentials)` saved login with
> `username` - user's unique id
> `password` JSON containing `accountData` {
>    "kSync":"64 bytes encryption key for Sync",
>    "kXCS":"16 bytes key hash for the X-Client-State header",
>    "kExtSync":"64 bytes webext chrome.storage.sync master key",
>    "kExtKbHash":"32 bytes key hash of kB for WebExtensions syncing"
> }

So the general direction for resolving this would be to mimic what desktop does and
- save a new login `Firefox account credentials` with the above data which will allow to keep the login state
- and in bug 1473470 remove the Settings option to clear `Saved logins` (maybe just for users with active Sync) ensuring so that the Sync login state cannot be inadvertently cleared.
Depends on: 1473470
See Also: → 1426306
See Also: → 1013064
(In reply to Petru-Mugurel Lingurar[:petru] from comment #24)
> So the general direction for resolving this would be to mimic what desktop
> does and
> - save a new login `Firefox account credentials` with the above data which
> will allow to keep the login state
> - and in bug 1473470 remove the Settings option to clear `Saved logins`
> (maybe just for users with active Sync) ensuring so that the Sync login
> state cannot be inadvertently cleared.

I'm not clear on where these credentials are actually stored today. If it's not in the login manager, you might find that migrating to using the login manager is a large chunk of work that difficult to justify given the current Fennec status. Assuming it is actually in localstorage or cookies, you might want to investigate whether adding the concept of a "whitelist" to those stores (so those items aren't cleared) might end up being a more pragmatic solution.
Thanks Mark.

Indeed I've investigated this, tried to see exactly what has been done on Desktop to support this and found quite a few tickets needed to stabilize the feature and quite a lot of work needed to have the login state saved and available from the login manager.

As I've said in comment 18 clearing `Cookies & active logins` or `Offline website data` do result in loosing the login state so if we are to fallback onto the "whitelist" solution, it would involve modifying these two parts of the code.

I've also investigated this route and found that 
- clearing cookies is treated in nsCookieService.cpp [1]
- clearing local storage is treated in StorageObserver.cpp [2]
C++ code for which I do not have the expertise needed to provide a quality patch.
NI'ing Susheel to find a more suitable owner.

[1] https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/netwerk/cookie/nsCookieService.cpp
[2] https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/dom/storage/StorageObserver.cpp#303
Flags: needinfo?(sdaswani)

Comment 27

6 months ago
Janet, who can pick this up from the Accounts / Sync team?
Flags: needinfo?(sdaswani) → needinfo?(jdragojevic)

Comment 28

6 months ago
Susheel, 

Based on the comments above, it appears that this issue should be resolved on the mobile side, not by the Accounts/Sync team.
Flags: needinfo?(jdragojevic) → needinfo?(sdaswani)

Comment 29

6 months ago
Petru, are the code references in your comment https://bugzilla.mozilla.org/show_bug.cgi?id=1479749#c26 in Fennec, Gecko, or someplace else?
Flags: needinfo?(sdaswani)
Assignee: petru.lingurar → nobody
Status: ASSIGNED → NEW
Reporter

Updated

6 months ago
Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.
Whiteboard: [geckoview]

[geckoview:fenix:p3] because we should check whether this works in Fenix.

Whiteboard: [geckoview] → [geckoview:fenix:p3]
Reporter

Updated

4 months ago
Comment hidden (me-too)
You need to log in before you can comment on or make changes to this bug.