Closed Bug 1207889 Opened 9 years ago Closed 8 years ago

Changing FxA password via accounts.firefox.com doesn't update keys on that device

Categories

(Firefox :: Firefox Accounts, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

STR:
* Configure Sync on a device.
* On that same device, change your password.

Expected:
* That Firefox doesn't later ask you to again enter your new password to continue syncing.

Actual:
* It does.

A common way to hit this flow is (a) attempt to connect a mobile device, realized you've forgotten your password and reset it, (b) open the "reset password" email on your already-configured desktop, (c) set a new password on that device (d) that device later gets upset the password is wrong when syncing.

Rachel hit exactly this (https://support.mozilla.org/en-US/questions/1085078)

I'm surprised to not find a bug on file for this - did I miss it? I expect that webchannels could help solve this.
Flags: firefox-backlog+
(In reply to Nick Alexander :nalexander from comment #1)

> AFAIK, this is only broadcast to WebChannel consumers.  See
> https://github.com/mozilla/fxa-content-server/issues/2152.

Hmm - we don't seem to get that message, even though we do get the "account deleted" message - although maybe that deleted message is delivered differently or via another code-path. (IIUC, desktop is not nominally a "WebChannel consumer" given about:accounts prefers events - but we do obviously expect *some* channel messages)

Zach, can you please clarify this?

FWIW, I tested simply by setting services.sync.log.appender.dump=Trace, and when I change my "display name" the console shows:

> 1443062157409   FirefoxAccounts DEBUG   FxAccountsWebChannel message received: {"messageId":null,"command":"profile:change","data":{"uid":"552aa4470f1b4756a305cb0aa6c2f3c0"}}

and looking at the code, we expect to see that log message even when we don't actually consume the event (ie, we should see the "change password" message even though we don't handle it)
Flags: needinfo?(zack.carter)
(In reply to Mark Hammond [:markh] from comment #0)
> STR:
> * Configure Sync on a device.
> * On that same device, change your password.
> 
> Expected:
> * That Firefox doesn't later ask you to again enter your new password to
> continue syncing.
> 
> Actual:
> * It does.
> 
> A common way to hit this flow is (a) attempt to connect a mobile device,
> realized you've forgotten your password and reset it, (b) open the "reset
> password" email on your already-configured desktop, (c) set a new password
> on that device (d) that device later gets upset the password is wrong when
> syncing.

Wait -- I misunderstood.  Check that you get a good result when you /change/ your password from /settings, or at least see the fxaccounts:change_password message.  You'll observe that /settings asks for your old password; that allows an unwrap-and-rewrap operation that preserves kB, and hence your Sync data.

If you /reset/ your password, it's not possible to perform the unwrap-and-rewrap; it's not possible to preserve kB; and it's not possible to preserve your Sync data.  Now, we can try to ensure that a Firefox browser that opens such a message does in fact receive a login message, and hence reconnects; but that's not the same as what I was discussing.
(In reply to Nick Alexander :nalexander from comment #3)
> Wait -- I misunderstood.  Check that you get a good result when you /change/
> your password from /settings, or at least see the fxaccounts:change_password
> message.

Sorry for not being clear - that's actually what I tested (but not what Rachel did) - I see no message in that case.

> If you /reset/ your password, it's not possible to perform the
> unwrap-and-rewrap; it's not possible to preserve kB; and it's not possible
> to preserve your Sync data.  Now, we can try to ensure that a Firefox
> browser that opens such a message does in fact receive a login message, and
> hence reconnects; but that's not the same as what I was discussing.

Yep, understood - stuff is a little trickier in the "reset" case - although I guess I was still expecting the same message in that case, just with less data (although a login message would be fine too)
On desktop, in order for accounts.firefox.com to send the fxaccounts:change_password message over WebChannel it needs to be loaded with the context=fx_desktop_v2 query param.

We also send that event through about:accounts, although the browser doesn't handle it.
Flags: needinfo?(zack.carter)
FWIW, this is another bit of chrome<->web-content inconsistency that I hope will be cleaned up by an explicit device handshake process, so we can detect when we're in a logged-in browser even if a special context= parameter is not provided.
(In reply to Zachary Carter [:zaach] from comment #5)

> We also send that event through about:accounts, although the browser doesn't
> handle it.

Right - but password changes aren't done via about:accounts, just via a normal tab to https://accounts.firefox.com - so even if about:accounts handled it, it would never see it in practice.

(In reply to Ryan Kelly [:rfkelly] from comment #6)
> FWIW, this is another bit of chrome<->web-content inconsistency that I hope
> will be cleaned up by an explicit device handshake process, so we can detect
> when we're in a logged-in browser even if a special context= parameter is
> not provided.

I don't understand the content-server code well enough to know if this makes sense, but there are already some messages sent to desktop without that query-param (profile change messages come to mind).

IIUC, the "handshake" process is more about knowing what the client expects (ie, so we don't confuse about:accounts by sending an event *and* a channel message for the same event) which is valuable, but doesn't seem to apply for messages about:accounts doesn't expect (profile change messages are an example of this). I also assume |context=fx_desktop_v2| isn't a security measure given it would be so easy for an attacker to add. So is there some reason we can't unconditionally send webchannel messages on these password changes too?

ni? rfkelly and stomlinson but don't actually need the info from both of you :)
Flags: needinfo?(stomlinson)
Flags: needinfo?(rfkelly)
> I also assume |context=fx_desktop_v2| isn't a security measure given it would be so easy for
> an attacker to add. So is there some reason we can't unconditionally send webchannel messages
> on these password changes too?

Hmm.  You're right, since these messages would include session tokens and key info, I was coming at this from a security perspective and thinking "we shouldn't send that message unless we're actually in a browser where that user is logged in".  But WebChannels prevent us from sending it to non-browser reliers already, and there's no way web content can protect itself from running inside a malicious browser.  So sending them unconditionally seems fine.

(content-server could save itself the trouble of e.g. generating a keyFetchToken if it knows it's not inside a logged-in browser, but that's an optimization, not a requirement)
Flags: needinfo?(stomlinson)
Flags: needinfo?(rfkelly)
> If you /reset/ your password, it's not possible to perform the unwrap-and-rewrap; it's not possible to
>  preserve kB; and it's not possible to preserve your Sync data.  Now, we can try to ensure that a
> Firefox browser that opens such a message does in fact receive a login message, and hence reconnects;
> but that's not the same as what I was discussing.

The content-server has the password when generating this message, so it could give you a full suite of login data in the message and the browser having to ask for the password again.

So IIUC, the shape of the proposal emerging here is:

* Send unconditional WebChannel messages on password_change and password_reset, like we currently do for e.g. profile data change.
* Include enough information in these messages for the browser to put itself back into a connected state without further user interaction.
* Have the browser listen for and react to such messages on any accounts.firefox.com page, regardless of whether it's loaded via about:accounts or not.


There's a lot of edge-cases here around state-sharing between device and content.  I could be changing the password for an account other than the one that's currently logged in to the device.  If we're sharing a new sessionToken between content-server and the browser, we need to mark it as "sessionFromSync" or whatever hackery we do to hide the logout option in web content.
This patch seems to get the job done nicely. Kit, what do you think?
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8728774 - Flags: review?(kcambridge)
Comment on attachment 8728774 [details] [diff] [review]
0001-Bug-1207889-handle-the-webchannel-changepassword-com.patch

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

LGTM! Does https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/FxAccountsWebChannel.jsm need a matching update, too?

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +175,5 @@
>            case COMMAND_SYNC_PREFERENCES:
>              this._helpers.openSyncPreferences(sendingContext.browser, data.entryPoint);
>              break;
> +          case COMMAND_CHANGE_PASSWORD:
> +            this._helpers.changePassword(message.data);

Nit: s/message.data/data

@@ +289,5 @@
> +    // We *could* still insist the server know what fields names are valid,
> +    // but that makes life difficult for the server when Firefox adds new
> +    // features (ie, new fields) - forcing the server to track a map of
> +    // versions to supported field names doesn't buy us much.
> +    // So we just remove field names we know aren't handled.

Makes sense. It's burdensome to require the server to filter unsupported fields.
Attachment #8728774 - Flags: review?(kcambridge) → review+
(In reply to Kit Cambridge [:kitcambridge] from comment #12)
> Comment on attachment 8728774 [details] [diff] [review]
> 0001-Bug-1207889-handle-the-webchannel-changepassword-com.patch
> 
> Review of attachment 8728774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM! Does
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/
> FxAccountsWebChannel.jsm need a matching update, too?

Not as part of this patch, but yeah. Nick, this patch is arranging for Desktop to listen for a "fxaccounts:change_password" message (sent when the user changes their password on this device) and update the stored credentials, so they don't need to re-enter their new password to reconnect Sync. Is there a bug on file for this for Android, or should we open one?
Flags: needinfo?(nalexander)
(In reply to Mark Hammond [:markh] from comment #13)
> (In reply to Kit Cambridge [:kitcambridge] from comment #12)
> > Comment on attachment 8728774 [details] [diff] [review]
> > 0001-Bug-1207889-handle-the-webchannel-changepassword-com.patch
> > 
> > Review of attachment 8728774 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > LGTM! Does
> > https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/
> > FxAccountsWebChannel.jsm need a matching update, too?
> 
> Not as part of this patch, but yeah. Nick, this patch is arranging for
> Desktop to listen for a "fxaccounts:change_password" message (sent when the
> user changes their password on this device) and update the stored
> credentials, so they don't need to re-enter their new password to reconnect
> Sync. Is there a bug on file for this for Android, or should we open one?

This is in place already: https://bugzilla.mozilla.org/show_bug.cgi?id=1207889#c1.  Thanks for thinking of Fennec, though!
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/a1cbd97d7b43
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1296328
Product: Core → Firefox
Target Milestone: mozilla48 → Firefox 48
You need to log in before you can comment on or make changes to this bug.