989549 - Call signOut() in FxAccountsClient.jsm from signOut() in FxAccounts.jsm

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Chris Karlof [:ckarlof]]

This will trigger the /session/destroy call on the FxA server and properly clean up the session state.

One issue with not doing this is that after disconnecting from FxA Sync this user can visit https://accounts.firefox.com/settings and still appear to be logged in to FxA with no apparent way to log out.

I think this should be a low risk fix, and I would vote for uplifting it to Fx29 Beta.

Comment 1

[Chris Karlof [:ckarlof]]

Attachment: 0001-Bug-989549-Call-signOut-in-FxAccountsClient.jsm-from.patch

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8402123 [details] [diff] [review]
0001-Bug-989549-Call-signOut-in-FxAccountsClient.jsm-from.patch

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

::: services/fxaccounts/FxAccounts.jsm
@@ +462,5 @@
>    signOut: function signOut() {
> +    let currentState = this.currentAccountState;
> +    return currentState.getUserAccountData().then(data => {
> +      let sessionToken = data && data.sessionToken;
> +      // Wrap this in a promise so *any* errors in signOut won't

I don't think it is necessary to use a promise here.  I think that simply this.fxAccountsClient.signOut(...).then(null, err => log.error...) would be fine as the promise returned by signOut isn't itself being returned.

Comment 3

[Chris Karlof [:ckarlof]]

Attachment: 0001-Bug-989549-Call-signOut-in-FxAccountsClient.jsm-from.patch

Comment 4

[Chris Karlof [:ckarlof]]

Mark, could I have another review. I did a slight refactor to move the call to signOut to the end of the promise chain. In response to your previous comment, I need the:

>             Promise.resolve().then(() => {
>	        // This can happen in the background and shouldn't block
>	        // the user from signing out. The server must tolerate
>	        // clients just disappearing, so this call should be best effort.
>	        return fxAccountsClient.signOut(sessionToken);
>	      }).then(null, err => {

pattern because:

1) I don't want synchronous errors in fxAccountsClient.signOut to trigger errors in fxAccounts.signOut()
2) I want it to be a background operation that is also allowed to fail without any errors bubbling up.

Comment 5

[Chris Karlof [:ckarlof]]

Comment on attachment 8402955 [details] [diff] [review]
0001-Bug-989549-Call-signOut-in-FxAccountsClient.jsm-from.patch

r+ by markh transferred from previous review

Comment 6

[Chris Karlof [:ckarlof]]

https://hg.mozilla.org/integration/fx-team/rev/75173a82b52e

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/75173a82b52e

Comment 8

[Chris Karlof [:ckarlof]]

Comment on attachment 8402955 [details] [diff] [review]
0001-Bug-989549-Call-signOut-in-FxAccountsClient.jsm-from.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FxA Sync
User impact if declined: When a user detaches her account from FxA Sync in the browser, she will not be logged out of her accounts settings page. 
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): low, limited to sync
String or IDL/UUID changes made by this patch: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/03566d5096af
https://hg.mozilla.org/releases/mozilla-beta/rev/8b66928d0515