Closed Bug 994934 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: FindMyDevice, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: njpark, Assigned: spenrose)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 949053
No longer blocks: 941723
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 :)
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)
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)
You are correct.
Blocks: 989363
No longer blocks: 974121
Flags: needinfo?(spenrose)
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.
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+
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)
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+
Great, thanks very much Jed. sessionToken is still used in a later callback, at line 177 once the patch is applied.
Updated commit message.
Attachment #8407738 - Attachment is obsolete: true
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]
Blocks: 1000323
Duplicate of this bug: 1000318
https://hg.mozilla.org/mozilla-central/rev/c6e856383007
Status: NEW → RESOLVED
Closed: 6 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.