[FxAccounts] Find my device can be enabled without logged into FxAccounts

RESOLVED FIXED in 2.0 S1 (9may)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njpark, Assigned: spenrose)

Tracking

unspecified
2.0 S1 (9may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
STR:
- Log into FxAccounts and enable Find My Device
- Log out of FxAccounts.  Verify the Find My Device is now disabled.
- Enter Find My Device, and enable it

Expected:
- User enters the FxAccounts workflow.
Actual:
- Find My Device is enabled, while the device is not logged into FxAccounts

Found it during the testing of FxAccounts testings.
(Reporter)

Updated

5 years ago
Blocks: 949053
No longer blocks: 941723
(Reporter)

Updated

5 years ago
Blocks: 974121
No longer blocks: 949053
I can reproduce this, but it looks like a Firefox Accounts issue. Sam, can you please have a look at this?

It looks like the cause for this bug is that Firefox Accounts is not triggering my onlogout handler in the Settings app.

I have a call to navigator.mozId.watch() in the settings app with a callback for both onlogin and onlogout that handles the UI [1], and you'll notice I forgot a very helpful console.log call there. This logging is not triggered when I log out of FxA, which shows that the callback never got called. If, however, I kill the Settings app, the onlogout callback does get called and the UI behaves as expected.

I also have a listener for mozFxAccountsUnsolChromeEvent in the System app, and this one does get called right after the logout.

I don't know if this is useful, but right after logging out, I get this in the console:
Content JS ERROR at app://settings.gaiamobile.org/js/firefox_accounts/panel.js:78 in onFxAccountError: FxaPanel: Error getting Firefox Account: INVALID_AUTH_TOKEN

1- https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/findmydevice.js#L51
2- https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/findmydevice_launcher.js#L19
Flags: needinfo?(spenrose)
(In reply to Guilherme Gonçalves [:ggp] from comment #1)
> If, however, I kill the Settings app, the onlogout callback does get called and
> the UI behaves as expected.

Sorry, clarification: the callback does get called _after I reopen the Settings app_, of course :)
(Assignee)

Comment 3

5 years ago
Confirmed via these STR and via logout() in UITests->API->FxAccounts MozID:logout().

1397260550241	FirefoxAccounts	ERROR	error POSTing /session/destroy: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}
1397260550242	FirefoxAccounts	ERROR	INVALID_AUTH_TOKEN

so basically signOut is broken in FxAccountsClient.jsm or somewhere close.
Assignee: nobody → spenrose
Flags: needinfo?(spenrose)
(Assignee)

Comment 4

5 years ago
FxAccountsClient.signOut(), the wrapper around the HTTP call that tells the server we're logging out, is being called twice. The first time it succeeds, the second time it fails. Presumably the failure stomps on the happy path that would result in onlogout() firing. I will look for the double-call.
Sam, is this a gecko bug? If so, maybe we can refile under a different metabug?
Flags: needinfo?(spenrose)
(Assignee)

Comment 6

5 years ago
You are correct.
Blocks: 989363
No longer blocks: 974121
Flags: needinfo?(spenrose)
(Assignee)

Comment 7

5 years ago
The problem is that
  services/fxaccounts/FxAccountsManager.jsm:_signOut()
calls
  services/fxaccounts/FxAccounts.jsm:signOut()
which backgrounds a call to
  services/fxaccounts/FxAccountsClient.jsm:signOut()
then returns back to
  services/fxaccounts/FxAccountsManager.jsm:_signOut()
which makes the same call to
  services/fxaccounts/FxAccountsClient.jsm:signOut()

I am inclined to fix it by splitting
  services/fxaccounts/FxAccounts.jsm:signOut()
into
  signOutLocal()
and
  signOutOnServer()
Then FxAccountsManager.jsm:_signOut() can just call the former.
(Assignee)

Comment 8

5 years ago
Per previous comment, added a flag to FxAccounts.signOut() to keep it from calling FxAccountsClient.signOut().
Attachment #8406506 - Flags: feedback?(jparsons)
Comment on attachment 8406506 [details] [diff] [review]
994934-split-FxAccounts.jsm-signOut.patch

Review of attachment 8406506 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable to me to fix the problem at hand.  I still feel there's a bit of a love triangle between these jsms, but that would be something to resolve in a separate bug.

::: services/fxaccounts/FxAccounts.jsm
@@ +465,5 @@
>      let sessionToken;
>      return currentState.getUserAccountData().then(data => {
>        // Save the session token for use in the call to signOut below.
>        sessionToken = data && data.sessionToken;
> +      return this._signOutLocal();

You could even go further and create _signOutServer() to put the !localOnly logic in.

::: services/fxaccounts/FxAccountsManager.jsm
@@ -162,5 @@
>      // session token value to be able to remove the remote server session
>      // in case that we have network connection.
>      let sessionToken = this._activeSession.sessionToken;
>  
> -    return this._fxAccounts.signOut(sessionToken).then(

Nice to be able to replace the unused parameter with something that actually does something.
Attachment #8406506 - Flags: feedback?(jparsons) → feedback+
(Assignee)

Comment 10

5 years ago
Thanks Jed! I created _signOutServer() as a one-line wrapper around FxAccountsClient.signOut(), which leaves the nested Promise handling in place, which is a debatable but practical choice. Asking Fernando for review; if he's too busy I'll switch back to you on your return. Agreed on the need to refactor the .jsms overall.
Attachment #8406506 - Attachment is obsolete: true
Attachment #8407738 - Flags: review?(ferjmoreno)
Target Milestone: --- → 2.0 S1 (9may)
(Assignee)

Updated

5 years ago
Attachment #8407738 - Flags: review?(ferjmoreno) → review?(jparsons)
Comment on attachment 8407738 [details] [diff] [review]
994934-split-FxAccounts.jsm-signOut.patch

Review of attachment 8407738 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Sam!

::: services/fxaccounts/FxAccountsManager.jsm
@@ +160,5 @@
>      // We clear the local session cache as soon as we get the onlogout
>      // notification triggered within FxAccounts.signOut, so we save the
>      // session token value to be able to remove the remote server session
>      // in case that we have network connection.
>      let sessionToken = this._activeSession.sessionToken;

Do we still need sessionToken, or can this line go away?
Attachment #8407738 - Flags: review?(jparsons) → review+
(Assignee)

Comment 12

5 years ago
Great, thanks very much Jed. sessionToken is still used in a later callback, at line 177 once the patch is applied.
(Assignee)

Comment 13

5 years ago
Updated commit message.
Attachment #8407738 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c6e856383007

Friendly reminder that your commit message should be saying what the patch is doing, not restating the problem it's fixing :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Blocks: 1000323
(Assignee)

Updated

5 years ago
Duplicate of this bug: 1000318
https://hg.mozilla.org/mozilla-central/rev/c6e856383007
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.