1249619 - Respond to password change/reset events in a more timely manner

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Liviu Cirdei]

User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID 	20160218030349

In bug 1214379 one of the requirements for the Sync Tabs sidebar was: "When Firefox Account password has changed, the user is encouraged to sign in", but this does not work.

Steps to reproduce:

1. Sign in to Firefox on 2 devices with the same account.
2. From one device open some tabs and perform a sync.
3. On second device open Sync Tabs sidebar (Menu Bar>View>Sidebars > Synced Tabs). > The tabs are listed in the Sync tabs sidebar.
4. On first device, go to your account and change your password. > Password in changed.
5. Perform a sync.
5. On your second device, refresh the sync tabs sidebar. 

Actual Result: The user in not encouraged to sign in again. He can view the previous synced tabs, he can make a new sync,etc. The user is prompted to sign in again only when he goes to "Menu" > clicks on the account > "Manage account".
Closing and reopening the sidebar has the same result. 

Expected Result: User should receive a message to sign in again.

Comment 1

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

It takes some time for the browser to realize the password has changed - you'll probably also notice the hamburger menu also doesn't reflect the fact you need to reauthenticate either. You can typically reduce this time by setting a preference |services.sync.debug.ignoreCachedAuthCredentials| to true. Can you please try and re-create the issue with that preference set?

In general, this will only be a bug in the sidebar if the hamburger menu *does* reflect the fact a re-login is needed but the sidebar does not.

Comment 2

[Liviu Cirdei]

Hi Mark, 

I tried with the |services.sync.debug.ignoreCachedAuthCredentials| preference set to true. I observed two cases:

1. The hamburger menu did't reflect the fact a re-login is needed, but when I went to Menu [≡] -> Synced Tabs, I received a message to Sign in (displayed on the Synced tabs panel). On the sidebar no message was displayed when I refreshed it nor when I closed and reopened the sidebar.

2. Second case is when the hamburger menu reflected the fact a re-login is needed but even in this case the sidebar did not shown any message to the user.

Not sure why in some cases the hamburger menu reflected the fact a re-login is needed and in other cases not. However in both cases the sidebar did not encourage the user to sign in again.

Comment 3

[Edouard Oger [:eoger]]

FxA sends a push notification to all devices when we change/reset the password, let's do this!

Comment 4

[Edouard Oger [:eoger]]

Attachment: bug-1249619.patch

Looks like mozreview is down for the moment, so here's a good old patch.

Depends on https://github.com/mozilla/fxa-auth-server/pull/1381 getting merged

Comment 5

[Edouard Oger [:eoger]]

Comment on attachment 8776238 [details] [diff] [review]
bug-1249619.patch

Never mind I got mozreview working

Comment 6

[Edouard Oger [:eoger]]

Attachment: Bug 1249619 - Handle FxA password reset/changed push notifications.

Review commit: https://reviewboard.mozilla.org/r/68136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68136/

Comment 7

[Edouard Oger [:eoger]]

Comment on attachment 8776239 [details]
Bug 1249619 - Handle FxA password reset/changed push notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68136/diff/1-2/

Comment 8

[Edouard Oger [:eoger]]

Comment on attachment 8776239 [details]
Bug 1249619 - Handle FxA password reset/changed push notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68136/diff/2-3/

Comment 9

[Edouard Oger [:eoger]]

The last revision of this patch checks for the status of the sessionToken before doing the clear credentials/trigger sync error/update UI dance, which means we can trigger it even on a device who was informed of the password change using webchannel and already recreated its session (which means that the server sending the push messages doesn't need to figure out what devices it should exclude from this notification).

Comment 10

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

https://reviewboard.mozilla.org/r/68136/#review65294

Let me know if any of this doesn't make sense.

::: browser/base/content/browser-fxaccounts.js:131
(Diff revision 3)
>          this.onMigrationStateChanged(data, subject);
>          break;
>        case this.FxAccountsCommon.ONPROFILE_IMAGE_CHANGE_NOTIFICATION:
>          this.updateUI();
>          break;
> +      case this.FxAccountsCommon.ON_PASSWORD_RESET_NOTIFICATION:

I don't think this should be done here - it will be done once per window.

::: services/fxaccounts/FxAccounts.jsm:1524
(Diff revision 3)
>    },
>  
> +  resetCredentials() {
> +    // Delete all fields except those required for the user to
> +    // reauthenticate.
> +    log.info("clearing credentials to handle invalid token error");

This log should probably stay down in _handleTokenError, and maybe another generic message logged here.

::: services/fxaccounts/FxAccountsPush.js:160
(Diff revision 3)
>        case ON_DEVICE_DISCONNECTED_NOTIFICATION:
>          this.fxAccounts.handleDeviceDisconnection(payload.data.id);
>          break;
> +      case ON_PASSWORD_CHANGED_NOTIFICATION:
> +      case ON_PASSWORD_RESET_NOTIFICATION:
> +        // Forward the notification, gFxAccounts will take care the handling

I think here we should just reach into this.fxAccounts and do what you patch has browser-fxaccount.js doing.

However, it does seem to make sense that we create a new observer topic, something like FxAccountsCommon.ONACCTSTATECHANGE_NOTIFICATION which browser-fxaccounts uses to update the UI once you've done the account dance. It seems to make sense that this notification would be sent from your new resetCredentials() method.

::: services/sync/modules/browserid_identity.js:672
(Diff revision 3)
>    },
>  
>    // Returns a promise that is resolved when we have a valid token for the
>    // current user stored in this._token.  When resolved, this._token is valid.
> -  _ensureValidToken: function() {
> -    if (this.hasValidToken()) {
> +  // If forceFetchToken is true we'll always fetch a new token.
> +  ensureValidToken: function(forceFetchToken = false) {

Do we need this change? ISTM that we could just call the existing resetCredentials() method in bid_identity and a new token will be automagically fetched next sync?

Comment 11

[Edouard Oger [:eoger]]

Comment on attachment 8776239 [details]
Bug 1249619 - Handle FxA password reset/changed push notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68136/diff/3-4/

Comment 12

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

https://reviewboard.mozilla.org/r/68136/#review65528

This is looking great, but I think we can kill the weave import from the push service and formalize the new observer as a canonical "please drop your credentials based on the FxA, as it's probably changed"

::: services/fxaccounts/FxAccountsPush.js:14
(Diff revision 4)
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://services-sync/util.js");
>  Cu.import("resource://gre/modules/FxAccountsCommon.js");
> +Cu.import("resource://gre/modules/Task.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Weave",

instead of importing weave here, how about we just have browserid_identity listen to that new observer?

::: services/fxaccounts/FxAccountsPush.js:187
(Diff revision 4)
> +    yield this.fxAccounts.resetCredentials();
> +    let service = Cc["@mozilla.org/weave/service;1"]
> +                    .getService(Components.interfaces.nsISupports)
> +                    .wrappedJSObject;
> +    yield service.whenLoaded()
> +    Weave.Service.identity.resetCredentials();

so the observer in browserid_identity would just call resetCredentials().  I still don't think it's actually necessary to fetch a new one at that instant but that's not a huge deal either way.

Comment 13

[Edouard Oger [:eoger]]

https://reviewboard.mozilla.org/r/68136/#review65528

> instead of importing weave here, how about we just have browserid_identity listen to that new observer?

Aren't we introducing a race condition with gFxAccounts listening to the same event (and triggering updateUI)?

> so the observer in browserid_identity would just call resetCredentials().  I still don't think it's actually necessary to fetch a new one at that instant but that's not a huge deal either way.

We do need to fetch a new token at the moment, because this is where Weave.status.login is set (see [browserid_identity#_fetchTokenForUser](https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/browserid_identity.js#664)) and then reflected [in the UI](https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fxaccounts.js#78)

Comment 14

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

(In reply to Edouard Oger [:eoger] from comment #13)
> https://reviewboard.mozilla.org/r/68136/#review65528
> 
> > instead of importing weave here, how about we just have browserid_identity listen to that new observer?
> 
> Aren't we introducing a race condition with gFxAccounts listening to the
> same event (and triggering updateUI)?

Yeah, I minsunderstood the patch. However, I'm still inclined to think this should be 2 phase process:

* an observer sent when we notice the account state has possibly changed somehow, usable by all consumers of FxA.
* browserid_identity, as one such consumer, should tell the UI when its state has changed.

However, just to make things even muddier, I don't think the UI works correctly now :( My STR:
* start profile 1 with services.sync.debug.ignoreCachedAuthCredentials=true, sync.
* start profile 2, change your password.
* switch back to the still-running profile 1 and mash the "sync now" button.

The log from profile 1 is now filled with things like:

> 1470198784367	Sync.Service	INFO	Starting sync at 2016-08-03 14:33:04
> 1470198784367	Sync.Service	TRACE	In sync: no need to login.
...
> 1470198784377	Sync.BrowserIDManager	ERROR	Authentication error in _fetchTokenForUser: {"details":{"details":"getAssertion called without a session token"},"source":"fxaccounts"}
> 1470198784377	Sync.Status	DEBUG	Status.login: success.login => error.login.reason.account
> 1470198784377	Sync.Status	DEBUG	Status.service: success.status_ok => error.login.failed
> 1470198784379	Sync.BrowserIDManager	INFO	Failed to fetch the cluster URL: {"details":{"details":"getAssertion called without a session token"},"source":"fxaccounts"}
> 1470198784379	browserwindow.syncui	DEBUG	_loginFailed has sync state=error.login.reason.account

but there's no badge or other UI to show the login state of the profile is wrong. Note the second line "no need to login" - so we aren't doing the login dance, thus aren't sending "weave:service:login:error", thus browser-fxaccounts isn't updating the login state. Sadface. Next restart we *will* show that state.

Further, I *think* that even if we did re-do the login dance there, that would only happen after the next sync, rather than at the instant we noticed the account state.

Given we no longer care about non-fxa logins for Sync, I think we should break through some of that complexity and consider a dedicated observer notification sent by browserid_identity when the login state changes and change browser-fxaccounts to listen to this observer instead of "weave:service:login:error". bid_identity would send that immediately it notices the state change (or, say, every time a token is fetched) and we wouldn't rely on sync itself sending notifications to reflect this state.

IOW, we end up with 2 phases:

* The observer sent by the push notification that is an advisory that the account state may have changed.
* The observer sent by bid_identity whenever it gets a token.

bid_identity then listens for that first observer, throws away the token and fetches a new one. This always has the side-effect of sending that second notification, which the UI reacts to.

That would both solve this use-case, and also fix the but I mentioned above. (we *could* probably fix the bug above just by ensuring sync calls service.logout() when first noticing the bad token, forcing another login, forcing a login:error observer, but that seems like we are just sticking with the current house of cards)

Does that make sense? If so, what do you think?

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8776239 [details]
Bug 1249619 - Handle FxA password reset/changed push notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68136/diff/4-5/

Comment 16

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

https://reviewboard.mozilla.org/r/68136/#review66232

::: browser/base/content/browser-fxaccounts.js:24
(Diff revision 5)
>    get topics() {
>      // Do all this dance to lazy-load FxAccountsCommon.
>      delete this.topics;
>      return this.topics = [
>        "weave:service:ready",
>        "weave:service:login:error",

I expect we can kill this now?

::: services/fxaccounts/FxAccounts.jsm:798
(Diff revision 5)
>  
>      log.debug("destroying session");
>      return this.fxAccountsClient.signOut(sessionToken, options);
>    },
>  
> +  sessionStatus() {

Given this is public we should add some simple comments (just briefly saying what it does and what the returned promise resolves with)

::: services/fxaccounts/FxAccounts.jsm:1521
(Diff revision 5)
>            "The device ID to disconnect doesn't match with the local device ID.\n"
>            + "Local: " + localDeviceId + ", ID to disconnect: " + deviceId);
>      });
>    },
>  
> +  resetCredentials() {

ditto here - a short comment saying what it does and what it returns

::: services/fxaccounts/FxAccountsClient.jsm:191
(Diff revision 5)
>    /**
> +   * Check the status of a session given a session token
> +   *
> +   * @param sessionTokenHex
> +   *        The session token encoded in hex
> +   * @return Promise

Please indicate what the promise resolves with.

It *looks* like it is going to reject with some error if the session isn't valid - I'm not sure that makes sense without some canonical way of checking if the rejection is "session not valid" from "network error checking if it is valid".

IOW, it might be better to resolve with a bool and reject on error. OTOH I might be missing the point here.

::: services/fxaccounts/FxAccountsPush.js:13
(Diff revision 5)
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://services-sync/util.js");
>  Cu.import("resource://gre/modules/FxAccountsCommon.js");
> +Cu.import("resource://gre/modules/Task.jsm");

The weave import can die now, right?

::: services/fxaccounts/FxAccountsPush.js:182
(Diff revision 5)
> +  _onPasswordChanged: Task.async(function* () {
> +    try {
> +      yield this.fxAccounts.sessionStatus();
> +      return;
> +    } catch (e) {}
> +    yield this.fxAccounts.resetCredentials();

eg, let's say for some obscure reason we got the push notification but the FxA endpoint we hit here returns a 500 - it looks like we are going to kill the credentials for a perfectly valid account?

::: services/sync/modules/browserid_identity.js:314
(Diff revision 5)
>        // nothing else to do.
>        break;
> +
> +    case fxAccountsCommon.ON_ACCOUNT_STATE_CHANGE_NOTIFICATION:
> +      // throw away token and fetch a new one
> +      Weave.Service.identity.resetCredentials();

I think using |this| here would be clearer (and also means we can reasonably use internal methods)

::: services/sync/modules/browserid_identity.js:678
(Diff revision 5)
>        });
>    },
>  
>    // Returns a promise that is resolved when we have a valid token for the
>    // current user stored in this._token.  When resolved, this._token is valid.
> -  _ensureValidToken: function() {
> +  ensureValidToken: function() {

I think we can now leave this as internal (ie, with the leading underscore)

::: services/sync/modules/browserid_identity.js:693
(Diff revision 5)
>      return this._fetchTokenForUser().then(
>        token => {
>          this._token = token;
> -      }
> +        notifyStateChanged();
> +      },
> +      notifyStateChanged

Isn't this change going to prevent a rejection being passed to the caller (ie, _getAuthenticationHeader handles a rejection)? IOW, it seems like the error handler should call notifyStateChanged() but return a promise rejected with the original error.

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8776239 [details]
Bug 1249619 - Handle FxA password reset/changed push notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68136/diff/5-6/

Comment 18

[Edouard Oger [:eoger]]

Thank you for your review Mark, this patch is testable live on http://latest.dev.lcip.org/ since my PR was merged.

Comment 19

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

Comment on attachment 8776239 [details]
Bug 1249619 - Handle FxA password reset/changed push notifications.

https://reviewboard.mozilla.org/r/68136/#review66540

looks great, thanks!

::: services/fxaccounts/FxAccountsPush.js:161
(Diff revision 6)
>        case ON_DEVICE_DISCONNECTED_NOTIFICATION:
>          this.fxAccounts.handleDeviceDisconnection(payload.data.id);
>          break;
> +      case ON_PASSWORD_CHANGED_NOTIFICATION:
> +      case ON_PASSWORD_RESET_NOTIFICATION:
> +        this._onPasswordChanged();

IIUC this could fail, so I think we want a trailing .catch that does a log.error.

(in fact, I think the exception handling in this module is a little broken already - having exceptions bubble back to the push service itself isn't that useful) - ISTM we could have something like:

function _observe(subject, topic, data) {
...

  case ON_PASSWORD_RESET_NOTIFICATION:
    return this._onPasswordChanged();
...
}

function observe(subject, topic, data) {
 // _observe must return nothing, or a promise
 let result;
 try {
   result = this._observe(subject, topic, data)
 } catch (ex) {
   this.log.error(...);
 }
 if (result) {
   result.catch(err => { this.log.error(...); }
 }
}

(alternatively, something like:)

Promise.resolve().then(() => this._observer(...)).catch()

would work and give exactly 1 place where you log.

if you are keen, feel free to do this - but also feel free to ignore it and just add the .catch to your promise :)

::: services/sync/modules/browserid_identity.js:315
(Diff revision 6)
>        break;
> +
> +    case fxAccountsCommon.ON_ACCOUNT_STATE_CHANGE_NOTIFICATION:
> +      // throw away token and fetch a new one
> +      this.resetCredentials();
> +      this._ensureValidToken();

similarly here, we should have a trailing .catch that just logs

Comment 20

[Edouard Oger [:eoger]]

Comment on attachment 8776239 [details]
Bug 1249619 - Handle FxA password reset/changed push notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68136/diff/6-7/

Comment 21

[Edouard Oger [:eoger]]

Thanks Mark!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/19bf95d3e65e
Handle FxA password reset/changed push notifications. r=markh

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/19bf95d3e65e

Comment 24

[Liviu Cirdei]

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID 	20160816030459

Hi, 

I tested this on latest Nightly (Windows 10, using two profiles on the same machine)and here is what I found:

Case 1: |services.sync.debug.ignoreCachedAuthCredentials| preference NOT SET in about:config
   1.1 On profile 1 change the password and perform a sync. 
   1.2 On profile 2 no notification appears even after waiting for 30 minutes (not on sidebar, not on synced tabs panel, not even on hamburger menu).
   1.3 Clicking the account from Menu to open "about:preferences?entrypoint=menupanel#sync" page, also does not trigger a sign in again message.
   1.4 The message to sign in again appears only when I click "Manage Account" (I'm redirected to a page to sign in again).

Case 2: |services.sync.debug.ignoreCachedAuthCredentials| preference SET TO TRUE in about:config
   2.1 Add this pref to profile 1 and profile 2. 
   2.2 On profile 1 change the password and perform a sync. 
   2.3 On profile 2 these were the results:
     a) Hamburger menu shows a warning sign (!) - OK
     b) On menu panel I can see the "Reconnect to Sync message" in place of the account name. - OK
     c) On Synced Tabs panel (Menu > Synced Tabs) the already synced tabs are displayed for a second, then a "Sign in to Sync" button appears. OK?? (Maybe the tabs should not be displayed at all and only the message to sign in again to be displayed).
     d) On Synced Tabs Sidebar the message to sign in again is not displayed. I can close and reopen the sidebar multiple times, no change. - NOT OK

So the first case seems to not work very well. In second case, even if things are better, there is at least one issue (the fact that the sidebar does not display a message to re-login) and maybe a second one (the fact that, on Synced Tabs panel, synced tabs are displayed for a second before "Sign in to Sync" button appears).

Also, one question: Is |services.sync.debug.ignoreCachedAuthCredentials| supposed to be set to True by default for all users? If not, how much time should they expect before receiving a notification to sign in again?

Comment 25

[Edouard Oger [:eoger]]

Hello Liviu,

The server has not been updated yet.
Until https://api.accounts.firefox.com/ says version: 1.67.0, which will happen probably before the end of this week, the patch will not do anything.
When it works, the notification is almost instantaneous.

|services.sync.debug.ignoreCachedAuthCredentials| is not supposed to be modified, this patch works without setting the pref to true.

Comment 26

[Liviu Cirdei]

Thanks Edouard. 
I will verify this again after the server will be updated.

Comment 27

[Liviu Cirdei]

I tested this again 
 - using one PC 2 profiles: Windows 10, 
 - using two PCs: Ubuntu 15.04 -> Ubuntu 16.04, Windows 10 -> Mac.10.10

Results:
1. Hamburger menu shows a warning sign (!) almost instantaneous. - OK
2. Sync Tabs sidebar does not show a message to sign in again: Filled bug 1298011.
3. Going to Menu > Synced Tabs the message to sign in again appears only after synced tabs are displayed for a second. Filled bug 1298014.

Question:
 Is this fix intended only for desktop Firefox versions? I tried with a mobile device (Android, latest Nightly) and the notification to sign in again in not shown at all (when changing the password from the Android device the notification is not shown on desktop and viceversa).

Comment 28

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

Thanks for following up.

(In reply to Liviu Cirdei [:liviucirdei] from comment #27)
>  Is this fix intended only for desktop Firefox versions?

Yep - I can't recall if other bugs exist for mobile, but this is desktop only - so with those other bugs you opened, it seems this is looking verified?

Comment 29

[Liviu Cirdei]

(In reply to Mark Hammond [:markh] from comment #28)

> Yep - I can't recall if other bugs exist for mobile, but this is desktop
> only - so with those other bugs you opened, it seems this is looking
> verified?

Yes, if we consider desktop only this is verified on Nightly 51 (Build ID 20160825030226).