972645 - Implement Reset Password Link or Screen for FxA

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect)

Description

[Sam Penrose]

We need a link that opens the on-web Reset Password Flow.

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I assume this would be a WebActivity that opens the reset password flow, dcoates tells me the flow is built as part of fxa-content-server.

Sam, any sense of how this fits together? Say the user goes through the flow, what happens when they come back? Do we just log them out? Do we log them out, then load the browser? I haven't had to link out to the web yet, could use some pointers (maybe there's an etherpad that covers all this?)

Comment 2

[Sam Penrose]

Please pick the simplest-to-implement plausible approach. I think it is simply that we send them away with no local state change until the server sends us down an invalid token message. We can open a mini-design issue on the nice to have track.

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Awesome Sam, thank you. The simplest possible approach I can think of:

- somehow (web activity, i'd guess?) open a browser with the reset flow URL
- user goes through the browser reset flow
- the FxAccountsIACHelper detects that the state has changed
- various apps are updated via the gecko event detection
- when the web activity exits,
  - if the settings app hasn't been evicted from memory, call getAccounts() to check if state has changed.
  - if the settings app has been evicted, it'll re-check getAccounts() when it is restarted anyway.

Starting on this now

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Sam or John, is this where we want to reset password, the button labeled 'change password'?

https://www.dropbox.com/sh/8o3t73g2lb07514/TiRPpm_03c/FxA%20Settings%20Signed%20In.pdf

If not here, I'm not sure where in the product we'd add a reset screen:

http://is.gd/FFOS_FxA_Latest_UX_PDF

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I've got this working with a web activity, except that it asks whether I want to use the System app or the Browser app. I need to figure out how to set a default. Also need to set the URL as a pref, not hard-coded in the app.

Code's currently sitting in a branch at https://github.com/6a68/gaia/tree/bug-972645-web-change-password-flow

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Oh, note that this code sits on top of the settings app branch, https://github.com/6a68/gaia/tree/settings-app-WIP-rebased (from the 949051 bug)

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Thanks for starting this Jared!

    (In reply to Jared Hirsch [:_6a68] from comment #3)
    > Awesome Sam, thank you. The simplest possible approach I can think of:
    > 
    > - somehow (web activity, i'd guess?) open a browser with the reset flow URL
    > - user goes through the browser reset flow
    > - the FxAccountsIACHelper detects that the state has changed
    > - various apps are updated via the gecko event detection
    > - when the web activity exits,
    >   - if the settings app hasn't been evicted from memory, call getAccounts()
    > to check if state has changed.
    >   - if the settings app has been evicted, it'll re-check getAccounts() when
    > it is restarted anyway.
    > 
    > Starting on this now

I would prefer to implement this as part of the system dialog, so we can reuse it in several places and we can have a more direct communication with the platform without needing FxAccountsIACHelper. John mentioned at [1] about the need of a reset password screen that needs to be shown on FTE (I guess he meant the system dialog that is triggered from the FTE), but I guess that we also need to show it in Settings (as you are doing here) and in the force auth flow at least.

Also, you mentioned:

    > - the FxAccountsIACHelper detects that the state has changed
    > - various apps are updated via the gecko event detection

And I am not really sure that this is possible with the approach at [2]. Note that the reset flow is remotely hosted and we are not getting any push notification about its successful or failed result. So I can't see how FxAccountsIACHelper or any other part of our code can detect that the password has been reset. But we still need to know if the password has been successfully reset or not, so we can log the user out from the device.

Without push notifications this is kind of tricky.

I see two options here until we have push notifications:

1 - The easy one: log the user out as soon as the password reset flow is requested. This is what you suggest in comment 1.
2 - The hard one: Implement a polling strategy to check if the password has been reset or not. I don't even know if it is currently possible to check this with the server and there are several issues associated with this like never-ending polling or restarting the polling task after a reboot if needed.

If UX and product are ok with this, I'd go with 1. for now.

Jared, let me know if you need any pointers about how to implement this as part of the system dialog.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=965494#c1
[2] https://github.com/6a68/gaia/commit/381a94d4b8cfee14fdd2da15f0f8a2501c61b7df

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jared Hirsch [:_6a68] from comment #5)
> I've got this working with a web activity, except that it asks whether I
> want to use the System app or the Browser app. I need to figure out how to
> set a default. Also need to set the URL as a pref, not hard-coded in the app.
> 

I am afraid that this is not possible by design. Web Activities are meant to let the user choose the handler of the activity request. In any case, you are seeing System and Browser as handlers of 'view' because all the Haida stuff about merging the Browser into the System app. Once this work is completely done, the Browser app should be removed from Gaia and we won't see any selection dialog unless another app (i.e. a 3rd party browser) implements the 'view' activity.

> Code's currently sitting in a branch at
> https://github.com/6a68/gaia/tree/bug-972645-web-change-password-flow

Looking at the code, it seems that you might be confusing change password with reset password. Note that these flows are different :)

- http://gyazo.com/ef5c110e7b4398c3c9bb2f1373474f1e
- http://gyazo.com/f5939a5a5dcf5f5c6b4a391132a54e84

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Howdy!

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #8)
> Thanks for starting this Jared!
> 
>     (In reply to Jared Hirsch [:_6a68] from comment #3)
>     > Awesome Sam, thank you. The simplest possible approach I can think of:
>     > 
>     > - somehow (web activity, i'd guess?) open a browser with the reset
> flow URL
>     > - user goes through the browser reset flow
>     > - the FxAccountsIACHelper detects that the state has changed
>     > - various apps are updated via the gecko event detection
>     > - when the web activity exits,
>     >   - if the settings app hasn't been evicted from memory, call
> getAccounts()
>     > to check if state has changed.
>     >   - if the settings app has been evicted, it'll re-check getAccounts()
> when
>     > it is restarted anyway.
>     > 
>     > Starting on this now
> 
> I would prefer to implement this as part of the system dialog, so we can
> reuse it in several places and we can have a more direct communication with
> the platform without needing FxAccountsIACHelper. John mentioned at [1]
> about the need of a reset password screen that needs to be shown on FTE (I
> guess he meant the system dialog that is triggered from the FTE), but I
> guess that we also need to show it in Settings (as you are doing here) and
> in the force auth flow at least.

Yeah, in [1] I think the idea is to tell the user they can reset their password, but not take them too far out of the FTE flow.

It seems like the only UX that might correspond to reset password is the 'I forgot my password' link in the 'enter password' screen (screen 5.1 in [UX]).

[UX] https://www.dropbox.com/s/3bg0c2poemkxazx/FxA_FFOS_System_Flow.pdf

> 
> Also, you mentioned:
> 
>     > - the FxAccountsIACHelper detects that the state has changed
>     > - various apps are updated via the gecko event detection
> 
> And I am not really sure that this is possible with the approach at [2].
> Note that the reset flow is remotely hosted and we are not getting any push
> notification about its successful or failed result.

Hmm, that's pretty weird. If the fxa-content-server resets a password, it sure seems like the fxa-auth-server (which communicates with the handset) should let us know. I'll ask one of the server devs about this.

 So I can't see how
> FxAccountsIACHelper or any other part of our code can detect that the
> password has been reset. But we still need to know if the password has been
> successfully reset or not, so we can log the user out from the device.
> 
> Without push notifications this is kind of tricky.
> 
> I see two options here until we have push notifications:
> 
> 1 - The easy one: log the user out as soon as the password reset flow is
> requested. This is what you suggest in comment 1.

Assuming we get no push notification on reset, this seems like the only way to me, unless it's possible to have the reset password WebActivity flow fire a WebActivity onsuccess callback when the reset is completed, and an onerror when the reset failed.

> 2 - The hard one: Implement a polling strategy to check if the password has
> been reset or not. I don't even know if it is currently possible to check
> this with the server and there are several issues associated with this like
> never-ending polling or restarting the polling task after a reboot if needed.
> 
> If UX and product are ok with this, I'd go with 1. for now.
> 
> Jared, let me know if you need any pointers about how to implement this as
> part of the system dialog.

It seems like adding a reset link should be basically the same ten lines as this current patch, except inside the system dialog. I'll ping you for r? when it's ready.

In the meantime, since I've already got this patch to add a change password button, I'll just reuse it to implement 'change password', I think that should knock off one of the nice-to-have gaia bugs ;-)

> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=965494#c1
> [2]
> https://github.com/6a68/gaia/commit/381a94d4b8cfee14fdd2da15f0f8a2501c61b7df

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Jared Hirsch [:_6a68] from comment #10)

> 
> Hmm, that's pretty weird. If the fxa-content-server resets a password, it
> sure seems like the fxa-auth-server (which communicates with the handset)
> should let us know. I'll ask one of the server devs about this.

vladikoff just tested this for me [gist], and doing a password reset on the fxa-content-server does indeed invalidate the user's fxa-auth-server session. So I think that we're good with this edge case.

[gist] https://gist.github.com/vladikoff/ee583aca7dbf4a863ba5

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

We talked about this on IRC.

> (In reply to Jared Hirsch [:_6a68] from comment #10)
> 
> > 
> > Hmm, that's pretty weird. If the fxa-content-server resets a password, it
> > sure seems like the fxa-auth-server (which communicates with the handset)
> > should let us know. I'll ask one of the server devs about this.
> 
> vladikoff just tested this for me [gist], and doing a password reset on the
> fxa-content-server does indeed invalidate the user's fxa-auth-server
> session. So I think that we're good with this edge case.
> 
> [gist] https://gist.github.com/vladikoff/ee583aca7dbf4a863ba5

We still can't do a logout from other devices, but RPs won't be able to use the account. The UX will be weird but I guess this should be good enough for now.

> > FxAccountsIACHelper or any other part of our code can detect that the
> > password has been reset. But we still need to know if the password has been
> > successfully reset or not, so we can log the user out from the device.
> > 
> > Without push notifications this is kind of tricky.
> > 
> > I see two options here until we have push notifications:
> > 
> > 1 - The easy one: log the user out as soon as the password reset flow is
> > requested. This is what you suggest in comment 1.
> 
> Assuming we get no push notification on reset, this seems like the only way
> to me, unless it's possible to have the reset password WebActivity flow fire
> a WebActivity onsuccess callback when the reset is completed, and an onerror
> when the reset failed.
> 

The reset password flow ends with the email confirmation which will likely happen outside of the browser activity.

> > 2 - The hard one: Implement a polling strategy to check if the password has
> > been reset or not. I don't even know if it is currently possible to check
> > this with the server and there are several issues associated with this like
> > never-ending polling or restarting the polling task after a reboot if needed.
> > 
> > If UX and product are ok with this, I'd go with 1. for now.
> > 
> > Jared, let me know if you need any pointers about how to implement this as
> > part of the system dialog.
> 
> It seems like adding a reset link should be basically the same ten lines as
> this current patch, except inside the system dialog. I'll ping you for r?
> when it's ready.
> 

The system app already has this link in the enter password screen [1][2]. It is disabled though.

As we said in IRC, we have two different scenarios for this screen depending on the FTU. We have to show the screen defined at [3] if the link is clicked during the FTU sign in flow (within the system dialog) or otherwise go to the web hosted reset flow. You can check if the FTU is enabled from the system app by looking at the value of [4]

> In the meantime, since I've already got this patch to add a change password
> button, I'll just reuse it to implement 'change password', I think that
> should knock off one of the nice-to-have gaia bugs ;-)

Great! We would need to check if the auth server also invalidates the session token during the change password flow. In that case, we would need to log the user out.

[1] https://mxr.mozilla.org/gaia/source/apps/system/fxa/elements/fxa-enter-password.html?force=1#7
[2] https://mxr.mozilla.org/gaia/source/apps/system/fxa/js/screens/fxam_enter_password.js#63
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=965494#c1
[4] https://mxr.mozilla.org/gaia/source/apps/system/js/ftu_launcher.js#22

Comment 14

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Pull Request

ferjm - mind taking a look at progress so far? I have to update tests still, just looking for a sanity check. Have to switch gears for a couple of minutes, then i'll try to get this review ready in a bit.

Comment 15

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8381530 [details] [review]
Pull Request

Updating to r?

My only remaining question is what to do in the FTE flow when we have shown the reset password dialog. How do we jump to the next step in the FTE?

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8381530 [details] [review]
Pull Request

Thanks Jared.

The PR contains commits that doesn't seem to belong to it. Can you clean it up before asking for f? again, please?

Also, rebase on top of bug 968294 and update the force auth part with the reset password bits if possible, please. Or file a follow up bug for it.

I've left a few comments in the PR.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jared Hirsch [:_6a68] from comment #15)
> Comment on attachment 8381530 [details] [review]
> WIP pull request
> 
> Updating to r?
> 
> My only remaining question is what to do in the FTE flow when we have shown
> the reset password dialog. How do we jump to the next step in the FTE?

I think we should reconsider this from an UX point of view.

You've implemented this screen using the error overlay, which seems appropriate. The reset password error overlay is shown after clicking in the "I forgot my password" link that is shown in the enter password screen after the user enters her email [2]. If the user clicks on this link by accident (big fingers, small screen) and we follow the current spec, she will be taken out from the sign in flow and moved to the next FTU screen. So she will need to go back to the previous screen (the FxA one), re-trigger the FxA flow and re-enter her email. I would prefer if we could not close the whole sign in flow and not move the user to the next screen and instead just close the reset password error overlay after clicking its OK button. That way the user can continue with the flow if the click in the link was an accident or just leave the flow by clicking the "X" button in the top left corner if she really does not remember her password. This is what every error overlay does by default in the sign in flow.

[1] https://www.dropbox.com/sh/8o3t73g2lb07514/plmOx-03jd/Password%20Reset%20Error.pdf
[2] https://www.dropbox.com/sh/8o3t73g2lb07514/h9Ak26ru8p/FxA%20Sub%20Sign%20In%20Enter%20Password%201.pdf

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #17)
> (In reply to Jared Hirsch [:_6a68] from comment #15)
> > Comment on attachment 8381530 [details] [review]
> > WIP pull request
> > 
> > Updating to r?
> > 
> > My only remaining question is what to do in the FTE flow when we have shown
> > the reset password dialog. How do we jump to the next step in the FTE?
> 
> I think we should reconsider this from an UX point of view.
> 
> You've implemented this screen using the error overlay, which seems
> appropriate

This is missing a: [1]

Comment 19

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8381530 [details] [review]
Pull Request

Fixing now

Comment 20

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #17)
> (In reply to Jared Hirsch [:_6a68] from comment #15)
> > Comment on attachment 8381530 [details] [review]
> > WIP pull request
> > 
> > Updating to r?
> > 
> > My only remaining question is what to do in the FTE flow when we have shown
> > the reset password dialog. How do we jump to the next step in the FTE?
> 
> I think we should reconsider this from an UX point of view.
> 
> You've implemented this screen using the error overlay, which seems
> appropriate. The reset password error overlay is shown after clicking in the
> "I forgot my password" link that is shown in the enter password screen after
> the user enters her email [2]. If the user clicks on this link by accident
> (big fingers, small screen) and we follow the current spec, she will be
> taken out from the sign in flow and moved to the next FTU screen. So she
> will need to go back to the previous screen (the FxA one), re-trigger the
> FxA flow and re-enter her email. I would prefer if we could not close the
> whole sign in flow and not move the user to the next screen and instead just
> close the reset password error overlay after clicking its OK button. That
> way the user can continue with the flow if the click in the link was an
> accident or just leave the flow by clicking the "X" button in the top left
> corner if she really does not remember her password. This is what every
> error overlay does by default in the sign in flow.
> 

That makes a ton of sense. Thanks!

Comment 21

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8381530 [details] [review]
Pull Request

ferjm - I've fixed up the stuff from the last review, and added two more unit tests. Thanks for the great feedback on this patch so far :-)

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8381530 [details] [review]
Pull Request

Looks good! I'd like to see a final version though. I've added a few more comments on the PR. Sorry for not seeing this in the first revision. Thanks!

Comment 23

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8381530 [details] [review]
Pull Request

Thanks for the feedback! Resubmitting.

I put these comments in the github pull request out of habit, quoting myself to keep as much context in bugzilla as possible:

"I've rebased against today's master, fixed the couple of smaller things inside the main squashed commit, and removed the 'password reset success' code in its own commit, to make reading history and git bisecting more sane later. If you'd rather, I can squash them into one commit as well."

:beers:

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8381530 [details] [review]
Pull Request

Thanks Jared.

While testing the patch I am seeing:

JavaScript error: app://system.gaiamobile.org/fxa/js/fxam_server_request.js, line 14: SettingsHelper is not defined
JavaScript error: app://system.gaiamobile.org/fxa/js/screens/fxam_enter_email.js, line 76: FxModuleServerRequest is not defined

and

JavaScript error: app://system.gaiamobile.org/fxa/js/screens/fxam_enter_password.js, line 66: FtuLauncher is not defined

It seems that you forgot to import this scripts.

Comment 25

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8381530 [details] [review]
Pull Request

ferjm - OK, finally got this fixed up, ready for a (hopefully final ;-) pass.

I did run into a big question:

Over the past week or so, the WebActivity behavior has changed a bit. It turns out that, when the WebActivity call opens the browser, the browser's z-index puts it *behind* the system app. (Maybe I was not seeing this before because the WebActivity request would show me a "pick your browser" overlay, that's gone now.) To work around this z-index problem, I just close the fxa system app if the WebActivity fires the success callback. If there's a nicer way to resolve this, I'd love to hear about it.

Fixed a few small random things as well:

- in fxam_errors.js, using Errors['UNKNOWN'] instead of the undefined Errors[UNKNOWN] if the error type isn't recognized. I suspect this is why some of the error screens have no strings in them?

- added mocha.globals calls to a couple of tests which failed if run individually due to global leakage

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8381530 [details] [review]
Pull Request

Looks good! Please, squash the commits in one.

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jared Hirsch [:_6a68] from comment #25)
> 
> I did run into a big question:
> 
> Over the past week or so, the WebActivity behavior has changed a bit. It
> turns out that, when the WebActivity call opens the browser, the browser's
> z-index puts it *behind* the system app. (Maybe I was not seeing this before
> because the WebActivity request would show me a "pick your browser" overlay,
> that's gone now.) To work around this z-index problem, I just close the fxa
> system app if the WebActivity fires the success callback. If there's a nicer
> way to resolve this, I'd love to hear about it.

Yes, that's because the activity is triggered from a system dialog. Closing it manually looks good to me.

Comment 28

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

woo-hoo! thanks for the review, ferjm!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Master: e2427df4eaf5b0b7ae207b256a51436b5b80d769

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gaia test failures.
Master: 9cb35e701df44766d9b3560b0defe0a401a0ecdd

https://tbpl.mozilla.org/php/getParsedLog.php?id=35670120&tree=B2g-Inbound

10:20:26     INFO -  gaia-unit-tests TEST-START | system/test/unit/fxa_test/screens/fxam_refresh_auth_test.js | Screen: Enter password
10:20:26     INFO -  gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/fxa_test/screens/fxam_refresh_auth_test.js | Screen: Enter password "before all" hook | global leak detected: FxModuleServerRequest
10:20:26     INFO -  gaia-unit-tests INFO       | stack trace:
10:20:26     INFO -      Error: global leak detected: FxModuleServerRequest
10:20:26     INFO -          at checkGlobals (http://system.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3907:5)
10:20:26     INFO -          at (anonymous) (http://system.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3795:39)
10:20:26     INFO -          at emit (http://system.gaiamobile.org:8080/common/vendor/mocha/mocha.js:230:5)
10:20:26     INFO -          at (anonymous) (http://system.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3978:7)
10:20:26     INFO -          at done (http://system.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3700:5)
10:20:26     INFO -          at (anonymous) (http://system.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3712:9)
10:20:26     INFO -          at (anonymous) (http://system.gaiamobile.org:8080/common/test/mocha_generators.js:46:13)
10:20:26     INFO -          at (anonymous) (http://system.gaiamobile.org:8080/test/unit/fxa_test/screens/fxam_refresh_auth_test.js:56:7)
10:20:26     INFO -          at gotContent (http://system.gaiamobile.org:8080/shared/js/html_imports.js:40:11)
10:20:26     INFO -          at onload (http://system.gaiamobile.org:8080/shared/js/html_imports.js:49:7)

Comment 31

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Updated pull request

Same as previous, with one-line mocha.globals change in the test that was broken. Also I've rebased against updated master, so the sim-card unit test failure shouldn't show up in this branch.

Comment 32

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

:RyanVM, do I need to do anything else for this to be ready to merge again? If so, let me know :-)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Nope, thanks :)

Master: ca0dd14577943458ae9d2eee6151cd09139ef9cc

Comment 34

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

welp, the updated patch hasn't been backed out yet ;-)

closing