Closed Bug 989549 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 + fixed
firefox30 + fixed
firefox31 --- fixed

People

(Reporter: ckarlof, Assigned: ckarlof)

References

Details

(Keywords: verifyme)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 988446
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.
Attachment #8402123 - Flags: review?(mhammond) → review+
Attachment #8402123 - Attachment is obsolete: true
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 on attachment 8402955 [details] [diff] [review]
0001-Bug-989549-Call-signOut-in-FxAccountsClient.jsm-from.patch

r+ by markh transferred from previous review
Attachment #8402955 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/75173a82b52e
Assignee: nobody → ckarlof
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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
Attachment #8402955 - Flags: approval-mozilla-beta?
Attachment #8402955 - Flags: approval-mozilla-aurora?
Attachment #8402955 - Flags: approval-mozilla-beta?
Attachment #8402955 - Flags: approval-mozilla-beta+
Attachment #8402955 - Flags: approval-mozilla-aurora?
Attachment #8402955 - Flags: approval-mozilla-aurora+
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.