Closed
Bug 1479749
Opened 6 years ago
Closed 4 years ago
Specific options from 'Clear private data on exit' disconnect Firefox account
Categories
(Firefox for Android Graveyard :: Android Sync, defect, P2)
Tracking
(firefox-esr68 affected, firefox61 wontfix, firefox62- wontfix, firefox63+ wontfix, firefox64- wontfix, firefox65 wontfix, firefox66 affected, firefox67 affected, firefox68 affected)
People
(Reporter: stefan.deiac, Unassigned)
References
Details
(Whiteboard: [geckoview:fenix:p3])
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•6 years 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
Comment 2•6 years ago
|
||
(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•6 years 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
Comment 4•6 years ago
|
||
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•6 years 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.
Comment 6•6 years ago
|
||
That pushlog doesn't make sense for this, though.
Comment 7•6 years ago
|
||
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•6 years 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)
Comment 9•6 years ago
|
||
I'm going to mark this fix-optional for 62, but if we come up with a fix we could still potentially uplift it.
Comment 10•6 years ago
|
||
According to comment 8 no regression, though.
Keywords: regression
Hardware: ARM → All
Reporter | ||
Comment 11•6 years 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.
status-firefox64:
--- → affected
Comment 12•6 years ago
|
||
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.
Comment 13•6 years ago
|
||
Wontfix for 63, I'll let Julien decide if it should be tracked for 64.
tracking-firefox64:
--- → ?
Comment 14•6 years ago
|
||
Sounds like this is an old issue, I don't see a need to track.
Reporter | ||
Updated•6 years ago
|
status-firefox65:
--- → affected
Comment 15•6 years 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•6 years ago
|
||
I wonder if this issue is related in any way with bug 1464003
Updated•6 years ago
|
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Comment 17•6 years ago
|
||
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)
Comment 18•6 years ago
|
||
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)
Updated•6 years ago
|
Assignee: petru.lingurar → nobody
Status: ASSIGNED → NEW
Component: General → Android Sync
Comment 19•6 years ago
|
||
Janet, Grisha has moved off Sync, can you find someone to help us with this?
Flags: needinfo?(gkruglov) → needinfo?(jdragojevic)
Comment 20•6 years ago
|
||
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)
Comment 21•6 years ago
|
||
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)
Updated•6 years ago
|
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Comment 22•6 years ago
|
||
(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!
Comment 23•6 years ago
|
||
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.
Comment 24•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 25•6 years ago
|
||
(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.
Comment 26•6 years ago
|
||
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 years ago
|
||
Janet, who can pick this up from the Accounts / Sync team?
Flags: needinfo?(sdaswani) → needinfo?(jdragojevic)
Comment 28•6 years 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 years 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)
Comment 30•6 years ago
|
||
(In reply to :sdaswani only needinfo from comment #29)
> Petru, are the code references in your comment
> https://bugzilla.mozilla.org/show_bug.cgi?id=1479749#c26 in Fennec, Gecko,
> or someplace else?
Both seem to be part of Core,
DOM: WebStorage - https://bugzilla.mozilla.org/describecomponents.cgi?product=Core&component=DOM%3A%20Web%20Storage#DOM%3A%20Web%20Storage
Networking: Cookies - https://bugzilla.mozilla.org/describecomponents.cgi?product=Core&component=Networking%3A%20Cookies#Networking%3A%20Cookies
Updated•6 years ago
|
Assignee: petru.lingurar → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•6 years ago
|
status-firefox66:
--- → affected
Comment 31•6 years 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.
Restrict Comments: false
Updated•6 years ago
|
Whiteboard: [geckoview]
Comment 32•6 years ago
|
||
[geckoview:fenix:p3] because we should check whether this works in Fenix.
Whiteboard: [geckoview] → [geckoview:fenix:p3]
Reporter | ||
Updated•6 years ago
|
status-firefox67:
--- → affected
Comment hidden (me-too) |
Reporter | ||
Updated•5 years ago
|
status-firefox-esr68:
--- → affected
Comment 34•4 years ago
|
||
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•