Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

feat(client): Better handling of deleted accounts on /force_auth #3419

Merged

Conversation

shane-tomlinson
Copy link

Sync users who click "Manage account" are sent to the /settings page. A
user may have to sign in via force_auth if their sessionToken is no longer
valid. If the user deleted their account, they may have to re-sign up, or
if they have already re-signed up, they may have to sign in.

This PR checks whether the passed in email and uid are still registered
before allowing the user to sign in.

If no uid is passed:

  • If the email is no longer registered, allow the user to sign up again.

If a uid is passed:

  • If the email is no longer registered and the broker supports a UID change,
    allow the user to sign up again.
  • If the uid is no longer registered but the email is, allow the user to sign
    in if the broker supports a UID change.
  • If the broker does not support a UID change, dead end for either case.

Currently the only broker that allows a UID change is fx_desktop_v3. We can
expand this functionality to other brokers once we verify support.

fixes #3057
fixes #3283

var self = this;
return this.invokeBrokerMethod('afterForceAuthError', account, err)
.then(function () {
return SignInView.prototype.onSignInError.call(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re add the UNKNOWN_ACCOUNT branch before the SignInView method.

@shane-tomlinson shane-tomlinson force-pushed the issues-3057-3283-force-auth-after-delete-account branch 8 times, most recently from 458883e to 594e622 Compare January 27, 2016 16:45
@shane-tomlinson
Copy link
Author

This has turned up bugs all over the place, both browser and content-server. I'm going to file some issues tomorrow.

@shane-tomlinson shane-tomlinson force-pushed the issues-3057-3283-force-auth-after-delete-account branch 8 times, most recently from 66e5990 to 6a05ab1 Compare February 1, 2016 13:00
@shane-tomlinson shane-tomlinson force-pushed the issues-3057-3283-force-auth-after-delete-account branch 3 times, most recently from 730e32a to fb5d9d3 Compare February 2, 2016 15:02
@@ -139,7 +144,7 @@ define(function (require, exports, module) {
},

_selectAutoFocusEl: function () {
var prefillEmail = this.getPrefillEmail();
var prefillEmail = this.getPrefillEmail() || this.model.get('forceEmail');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.model.get('forceEmail') should go first.

shane-tomlinson pushed a commit that referenced this pull request Mar 15, 2016
PR #3419 was out of control, largely due to functional test refactoring.
I took the refactoring out of that PR into this PR.

* Use new helper functions to reduce boilerplate
* Add helpers.openForceAuth
* Update helpers.openFxaFromUntrustedRp and helpers.openFxaFromTrustedRp to
  take an options block instead of urlSuffix and trusted/untrusted flags.
* update helpers.fillOutForceAuth to skip entering the email if `options.enterEmail` === false
@shane-tomlinson shane-tomlinson force-pushed the issues-3057-3283-force-auth-after-delete-account branch from a3ed595 to d8c59da Compare March 16, 2016 00:16
shane-tomlinson pushed a commit that referenced this pull request Mar 16, 2016
PR #3419 was out of control, largely due to functional test refactoring.
I took the refactoring out of that PR into this PR.

* Use new helper functions to reduce boilerplate
* Add helpers.openForceAuth
* Update helpers.openFxaFromUntrustedRp and helpers.openFxaFromTrustedRp to
  take an options block instead of urlSuffix and trusted/untrusted flags.
* update helpers.fillOutForceAuth to skip entering the email if `options.enterEmail` === false
shane-tomlinson pushed a commit that referenced this pull request Mar 16, 2016
PR #3419 was out of control, largely due to functional test refactoring.
I took the refactoring out of that PR into this PR.

* Use helper functions to reduce boilerplate
* Add FunctionalHelpers.openForceAuth
* Update helpers.openFxaFromUntrustedRp and helpers.openFxaFromTrustedRp to
  take an options block instead of urlSuffix and trusted/untrusted flags.
* update helpers.fillOutForceAuth to skip entering the email if `options.enterEmail` === false
shane-tomlinson pushed a commit that referenced this pull request Mar 16, 2016
PR #3419 was out of control, largely due to functional test refactoring.
I took the refactoring out of that PR into this PR.

* Use helper functions to reduce boilerplate
* Add FunctionalHelpers.openForceAuth
* Update helpers.openFxaFromUntrustedRp and helpers.openFxaFromTrustedRp to
  take an options block instead of urlSuffix and trusted/untrusted flags.
* update helpers.fillOutForceAuth to skip entering the email if `options.enterEmail` === false
@shane-tomlinson shane-tomlinson force-pushed the issues-3057-3283-force-auth-after-delete-account branch 11 times, most recently from 74e0117 to 1d0621f Compare March 17, 2016 12:55
@@ -179,6 +179,10 @@ define(function (require, exports, module) {
},

transformLink: function (link) {
if (! /^\//.test(link)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth using a regex for this? It looks like we're only interested in the first character, would something like the following be clearer?

    if (link[0] !== '/') {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I'm so used to using RegExps for everything, I didn't think of the simple way to do it.

@philbooth
Copy link
Contributor

@shane-tomlinson, what are you talking about, I think this looks good! Considering the logic, it reads very clearly to me.

👍, r+, et cetera and so on.

Sync users who click "Manage account" are sent to the `/settings` page. A
user may have to sign in via force_auth if their sessionToken is no longer
valid. If the user deleted their account, they may have to re-sign up, or
if they have already re-signed up, they may have to sign in.

This PR checks whether the passed in email and uid are still registered
before allowing the user to sign in.

If no uid is passed:
* If the email is no longer registered, allow the user to sign up again.

If a uid is passed:
* If the email is no longer registered and the broker supports a UID change,
allow the user to sign up again.
* If the uid is no longer registered but the email is, allow the user to sign
in if the broker supports a UID change.
* If the broker does not support a UID change, dead end for either case.

Currently the only broker that allows a UID change is fx_desktop_v3. We can
expand this functionality to other brokers once we verify support.

fixes #3057
fixes #3283
@shane-tomlinson shane-tomlinson force-pushed the issues-3057-3283-force-auth-after-delete-account branch from f2d0e81 to 485433f Compare March 17, 2016 16:54
shane-tomlinson pushed a commit that referenced this pull request Mar 17, 2016
…er-delete-account

feat(client): Better handling of deleted accounts on /force_auth

r=@philbooth
@shane-tomlinson shane-tomlinson merged commit 3b40424 into master Mar 17, 2016
@shane-tomlinson shane-tomlinson deleted the issues-3057-3283-force-auth-after-delete-account branch March 17, 2016 19:18
feelcoder pushed a commit to feelcoder/fxa-content-server that referenced this pull request Mar 19, 2016
PR mozilla#3419 was out of control, largely due to functional test refactoring.
I took the refactoring out of that PR into this PR.

* Use helper functions to reduce boilerplate
* Add FunctionalHelpers.openForceAuth
* Update helpers.openFxaFromUntrustedRp and helpers.openFxaFromTrustedRp to
  take an options block instead of urlSuffix and trusted/untrusted flags.
* update helpers.fillOutForceAuth to skip entering the email if `options.enterEmail` === false
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants