feat: signin unblock #4154

shane-tomlinson · 2016-09-15T15:55:45Z

fixes #4036

shane-tomlinson · 2016-09-30T15:45:39Z

Ugh, sorry for the gigantic diff. @vbudhram, @seanmonstar, @vladikoff - r?

Going to try to put onto a dev box with @seanmonstar's PRs too.

shane-tomlinson · 2016-09-30T15:46:38Z

And now some functional tests fail. Still, let's start review to get feedback on code.

shane-tomlinson · 2016-09-30T16:14:27Z

app/scripts/lib/fxa-client.js

+    /**
+     * Send an unblock email.
+     *
+     * @returns {promise} resolves with response when complete.


Add email as a @param

shane-tomlinson · 2016-09-30T16:14:27Z

tests/functional/sign_in.js

@@ -183,7 +183,7 @@ define([
    },

    'data-flow-begin attribute is set': function () {
-      this.remote
+      return this.remote


Revert this change - it'll be fixed by fixing #4219

shane-tomlinson · 2016-09-30T16:14:27Z

tests/functional/sign_in_blocked.js

+        .then(testElementExists('#fxa-settings-header'));
+    },
+
+    'report signin link uid broken': function () {


This test fails locally.

vladikoff · 2016-09-30T16:27:15Z

app/scripts/lib/fxa-client.js

+     *
+     * @returns {promise} resolves with response when complete.
+     */
+    sendUnblockEmail: withClient((client, email) => {


nit: missing doc, unlike the function below

vladikoff · 2016-09-30T16:27:15Z

app/scripts/lib/fxa-client.js

+    /**
+     * Reject an unblock code.
+     *
+     * @param {string} uid - user id


nit: let's try to use {String} instead of {string}

vladikoff · 2016-09-30T16:27:15Z

app/scripts/lib/fxa-client.js

+     * Reject an unblock code.
+     *
+     * @param {string} uid - user id
+     * @param {string} code - login authorization code


login authorization code -> login unblock code ?

I would call this code -> unblockCode I think

vladikoff · 2016-09-30T16:27:15Z

app/scripts/models/account.js

+    },
+
+    /**
+     * Reject a login authorization code.


Reject a login authorization code. -> Reject a signin unblock code. ?

vladikoff · 2016-09-30T16:27:15Z

app/scripts/models/account.js

+    /**
+     * Reject a login authorization code.
+     *
+     * @param {string} code


unblockCode ?

vladikoff · 2016-09-30T16:27:15Z

app/scripts/views/mixins/signin-mixin.js

+        })
+        .fail((err) => {
+          function isUnblockableError(err) {
+            return err.verificationMethod === 'email-captcha';


move 'email-captcha' into a const into file above ?

This should go into lib/verification-methods.js.

vladikoff · 2016-09-30T16:27:15Z

app/scripts/views/mixins/signin-mixin.js

+            return err.verificationMethod === 'email-captcha';
+          }
+
+          if (isUnblockableError(err)) {


might need metrics events for the unblock err dance here

might need metrics events for the unblock err dance here

@vladikoff - is fxa.content.screen.signin_unblock sufficient?

vladikoff · 2016-09-30T16:27:15Z

app/scripts/views/report_sign_in.js

+    },
+
+    context () {
+      const verificationInfo = this._verificationInfo;


do we need this const here, we only use it once below

vladikoff · 2016-09-30T16:27:15Z

app/scripts/views/report_sign_in.js

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+define(function (require, exports, module) {


nit: maybe a doc above about this view

vladikoff · 2016-09-30T16:27:15Z

app/scripts/views/sign_in_unblock.js

+        .fail((err) => this.onSignInError(account, password, err));
+    },
+
+    onSignInError (account, password, err) {


hm surprised to see this error handler in sign_in_unblock.

We are re-signing the user in with the email/password they entered on the /signin page + the unblockCode entered here. If the user entered their password incorrectly, we'll only find out after this step, and then we have to send them back to the previous screen.

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/models/verification/report-sign-in.js

+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**
+ * A model to hold report-sign-in data. Model has a `report` function


Ahha, I took that out and moved it to user, good eye.

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/templates/report_sign_in.mustache

+
+      <form novalidate>
+        <div class="button-row">
+          <button id="submit-btn" type="submit" class="disabled">{{#t}}Report activity{{/t}}</button>


for sure we should add it.

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/templates/report_sign_in.mustache

+    </section>
+
+    <div class="links">
+      <a href="https://support.mozilla.org">{{#t}}Why is this happening?{{/t}}</a>


And we need a real link. :D

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/templates/sign_in_unblock.mustache

+<div id="main-content" class="card">
+  {{#error}}
+    <header>
+      <h1 id="fxa-signin-unblock-header">{{#t}}Authorize this sign-in{{/t}}</h1>


Good point, I'll change this to something like "problem sending unblock email" because that's how this error condition occurs.

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/views/mixins/signin-mixin.js

+        })
+        .fail((err) => {
+          function isUnblockableError(err) {
+            return err.verificationMethod === 'email-captcha';


This should go into lib/verification-methods.js.

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/views/mixins/signin-mixin.js

+            return err.verificationMethod === 'email-captcha';
+          }
+
+          if (isUnblockableError(err)) {


might need metrics events for the unblock err dance here

@vladikoff - is fxa.content.screen.signin_unblock sufficient?

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/views/sign_in_unblock.js

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+define(function (require, exports, module) {


Add a top level doc.

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/views/sign_in_unblock.js

+        .fail((err) => this.onSignInError(account, password, err));
+    },
+
+    onSignInError (account, password, err) {


We are re-signing the user in with the email/password they entered on the /signin page + the unblockCode entered here. If the user entered their password incorrectly, we'll only find out after this step, and then we have to send them back to the previous screen.

shane-tomlinson · 2016-09-30T16:53:03Z

app/scripts/views/sign_in_unblock.js

+    onSignInError (account, password, err) {
+      if (AuthErrors.is(err, 'INCORRECT_PASSWORD')) {
+        // The user must go enter the correct password this time.
+        this.navigate('signin', {


Don't forget about /oauth/signin and users who arrive here from /force_auth and /oauth/force_auth.

shane-tomlinson · 2016-09-30T16:53:03Z

tests/functional/sign_in_blocked.js

+        .then(testElementExists('#fxa-settings-header'));
+    },
+
+    'report signin link uid broken': function () {


vladikoff · 2016-09-30T21:16:21Z

got a bit stuck here

vladikoff · 2016-09-30T21:19:32Z

There is an issue with

Correct Code ->> Incorrect Password ->> Press back button ->> arrive to "signin_unblock" with no query params

and I think that threw "invalid code". I'm guessing we would need to handle the "back" case somehow.

shane-tomlinson · 2016-10-03T09:00:04Z

got a bit stuck here

Hard blocks still remain in place to prevent signin unblock from being used as 1) a spam vector, and 2) a free pass to guess the user's FxA password by continually going through signin unblock. Some restrictions will likely be loosened a bit, @rfk wrote this in an email thread with @seanmonstar and I:

I was able to get into a non-unblockable situation by doing the following:

Attempting signin to a bunch of non-existing accounts, then

Attempting signin with the wrong password on my real account

The result was a 429 while calling /send_unblock_code, and UX as shown
in the attached screenshot.

My mental model of what happened is that my IP was rate-limited due to
the failed logins on different accounts, and since rate-limited IPs are
not allowed to do anything, this blocked the sending of the
authorization code email.

Sean, you might remember this failure mode from our time digging into
that weird "Don't block email-sending on IP address alone" comment in
the customs-server:

mozilla/fxa-customs-server@9a4eaf5

Perhaps the right thing in the short term, is to improve the UX of this
failure case, by only transitioning to the "we sent you an authorization
code" screen after we've successfully sent the code.

shane-tomlinson · 2016-10-03T09:01:29Z

There is an issue with

Correct Code ->> Incorrect Password ->> Press back button ->> arrive to "signin_unblock" with no query params

and I think that threw "invalid code". I'm guessing we would need to handle the "back" case somehow.

Good find!

vbudhram

Looks good so far, just a couple comments, haven't looked through tests yet.

vbudhram · 2016-10-03T15:28:34Z

app/scripts/lib/validate.js

+     * @returns {Boolean}
+     */
+    isUnblockCodeValid (value) {
+      return !! (value && value.length === Constants.UNBLOCK_CODE_LENGTH);


I think it could be useful to check against the regex the auth server uses for unblock code, https://github.com/mozilla/fxa-auth-server/blob/2c0561df2aae7d166a21c05b70aaaff6efe982e8/lib/routes/validators.js#L17

vbudhram · 2016-10-03T15:28:34Z

app/scripts/models/user.js

+    },
+
+    /**
+     * Report/reject the unblockCode for the given account


This was a little confusing to me. I read it as either report or reject. Should it be reject and report? From looking at the auth-server endpoint, it appears to log an error, is this what is meant by report?

Comment updated.

vbudhram · 2016-10-03T15:28:34Z

app/scripts/views/elements/unblock-code-input.js

+
+    if (isRequired && ! value.length) {
+      throw AuthErrors.toError('UNBLOCK_CODE_REQUIRED');
+    } else if (value.length !== Constants.UNBLOCK_CODE_LENGTH) {


Can probably be replace with regex as well.

Can probably be replace with regex as well.

I replaced this check with Validate.isUnblockCodeValid

shane-tomlinson · 2016-10-04T15:52:27Z

Problems/follow on work @ryanfeeley and I found today when testing:

Incorrect password from force_auth does not allow the user to enter the correct password, upon return to /force_auth, the screen is blank except for an error that says "Incorrect password"
Incorrect password from signup sends the user back to the signup page from signin_unblock, it should instead send the user to the signin page.
Check how the screen appears from the firstrun page.
Signin with an unverified account - the user must first enter the unblock code, THEN they see the "verify your account" screen.
Doesn't work for Sync (user isn't forced through flow)

shane-tomlinson · 2016-10-10T10:19:23Z

app/scripts/views/force_auth.js

@@ -57,6 +57,8 @@ define(function (require, exports, module) {
      try {
        accountData = this._getAndValidateAccountData();
      } catch (err) {
+        // validation errors are not handled here, rather they are
+        // handled XXX.


This comment should be expanded upon.

shane-tomlinson · 2016-10-12T12:28:15Z

Signin with an unverified account - the user must first enter the unblock code, THEN they see the "verify your account" screen.

We have decided to punt on this for now.

philbooth

LGTM 👍

philbooth · 2016-10-12T13:27:29Z

app/tests/lib/helpers.js

+    let alphabet = '0123456789abcdefghijklmnopqrstuvwxyz';
+
+    for (let i = 0; i < length; ++i) {
+      let indexToUse = Math.floor(Math.random() * base);


I know this is only test code, but it sounded yesterday like we were agreeing either to move let declarations outside of loops or to use var instead.

I'm pretty "meh" since our for loops iterate over such a small range and that was a v8 specific issue.

on server side code though, we should probably absolutely worry about it.

probably absolutely. Yes. English, not my strongest subject.

Why move the let outside the for? It seems like exactly what is designed for: block level scoping.

Why move the let outside the for? It seems like exactly what is designed for: block level scoping.

@jrgm thought he heard v8 has an issue where let inside of loops is significantly slower than var.

shane-tomlinson · 2016-10-12T15:42:16Z

app/scripts/models/user.js

+    },
+
+    /**
+     * Report/reject the unblockCode for the given account


Comment updated.

shane-tomlinson · 2016-10-12T15:42:16Z

app/scripts/templates/report_sign_in.mustache

+    </section>
+
+    <div class="links">
+      <a href="https://support.mozilla.org">{{#t}}Why is this happening?{{/t}}</a>


Remove the link until it the content is ready, or perhaps we can use an able experiment to insert the link when we have one ready.

shane-tomlinson · 2016-10-12T15:42:16Z

app/scripts/views/sign_in_unblock.js

+      const lastPage = this.model.get('lastPage');
+
+      if (lastPage === 'force_auth') {
+        return 'force_auth';


Normalize these to handle the oauth case - this can be done using this.broker.transformLink.

shane-tomlinson · 2016-10-12T15:42:16Z

app/tests/spec/models/user.js

+
+    describe('rejectAccountUnblockCode', () => {
+      let account;
+      let UNBLOCK_CODE = '12345678';


This could be a const

rfk · 2016-10-12T19:47:00Z

We have decided to punt on this for now.

@shane-tomlinson @seanmonstar please ensure we have a follow-up bug on file in the right milestone, so we don't lose track of it

shane-tomlinson · 2016-10-13T12:04:00Z

tests/functional/sign_in_blocked.js

+        .then(testElementTextInclude('.verification-email-message', email))
+        .then(type('#unblock_code', 'i'))
+        .then(click('button[type=submit]'));
+        // TODO - check for an error here.


Fix the TODO!

shane-tomlinson · 2016-10-14T20:28:56Z

tests/functional/lib/helpers.js

@@ -891,7 +934,7 @@ define([
        }, [command])
        .then(null, function (err) {
          if (/ScriptTimeout/.test(String(err))) {
-            var noSuchNotificationError = new Error('NoSuchBrowserNotification');
+            var noSuchNotificationError = new Error('NoSuchBrowserNotification ' + command);


revert this change.

shane-tomlinson added the WIP label Sep 15, 2016

shane-tomlinson added this to the FxA-106: email captcha milestone Sep 15, 2016

shane-tomlinson added the waffle:active label Sep 15, 2016

shane-tomlinson assigned shane-tomlinson Sep 16, 2016

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch 6 times, most recently from 084dd4d to 1c454ca Sep 30, 2016

shane-tomlinson removed the WIP label Sep 30, 2016

shane-tomlinson assigned seanmonstar, vladikoff and vbudhram and unassigned shane-tomlinson Sep 30, 2016

shane-tomlinson reviewed Sep 30, 2016

View changes

vladikoff reviewed Sep 30, 2016

View changes

shane-tomlinson reviewed Sep 30, 2016

View changes

shane-tomlinson added the train-72 label Oct 3, 2016

vbudhram reviewed Oct 3, 2016

View changes

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch from 2d6740b to 9cead46 Oct 3, 2016

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch from fae3ad9 to 77125d5 Oct 6, 2016

shane-tomlinson reviewed Oct 10, 2016

View changes

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch 4 times, most recently from 4d44c4a to 27f724b Oct 10, 2016

shane-tomlinson assigned philbooth and unassigned vladikoff and vbudhram Oct 12, 2016

philbooth approved these changes Oct 12, 2016

View changes

shane-tomlinson reviewed Oct 12, 2016

View changes

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch from 7b39cac to b2969d2 Oct 12, 2016

shane-tomlinson mentioned this pull request Oct 13, 2016

Travis runs with Fx 40 #4270

Closed

shane-tomlinson reviewed Oct 13, 2016

View changes

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch 2 times, most recently from baeeb75 to f3eee32 Oct 14, 2016

shane-tomlinson changed the title ~~WIP Fxa106 signin unblock authorization captcha own screen~~ feat: signin unblock Oct 14, 2016

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch from f3eee32 to 300ae17 Oct 14, 2016

shane-tomlinson reviewed Oct 14, 2016

View changes

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch from 300ae17 to feb97bf Oct 14, 2016

feat(client): Sign in unblock.

cf2d06f

shane-tomlinson force-pushed the fxa106-signin-authorization-captcha-own-screen branch from feb97bf to cf2d06f Oct 14, 2016

fix(tests): Fix two oauth signin blocked tests.

489851a

shane-tomlinson deleted the fxa106-signin-authorization-captcha-own-screen branch Oct 17, 2016

shane-tomlinson removed the waffle:active label Oct 17, 2016

shane-tomlinson mentioned this pull request Oct 17, 2016

Ensure all signin unblock flow events are reported mozilla/fxa-auth-server#1505

Closed

2 of 5 tasks complete

shane-tomlinson mentioned this pull request Apr 3, 2019

Successfully consuming an unblockCode should mark the account as verified. mozilla/fxa#526

Open

mozilla / fxa-content-server Archived

feat: signin unblock #4154

feat: signin unblock #4154

Conversation

shane-tomlinson commented Sep 15, 2016 • edited

shane-tomlinson commented Sep 30, 2016

shane-tomlinson commented Sep 30, 2016

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

vladikoff Sep 30, 2016 • edited Contributor

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

vladikoff Sep 30, 2016 • edited Contributor

This comment has been minimized.

shane-tomlinson Sep 30, 2016 • edited Author Member

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

vladikoff Sep 30, 2016 Contributor

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 • edited Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

This comment has been minimized.

shane-tomlinson Sep 30, 2016 Author Member

vladikoff commented Sep 30, 2016 • edited

vladikoff commented Sep 30, 2016

shane-tomlinson commented Oct 3, 2016

shane-tomlinson commented Oct 3, 2016

vbudhram left a comment

This comment has been minimized.

vbudhram Oct 3, 2016 Contributor

This comment has been minimized.

vbudhram Oct 3, 2016 Contributor

This comment has been minimized.

shane-tomlinson Oct 10, 2016 Author Member

This comment has been minimized.

shane-tomlinson Oct 12, 2016 Author Member

This comment has been minimized.

vbudhram Oct 3, 2016 Contributor

This comment has been minimized.

shane-tomlinson Oct 4, 2016 Author Member

shane-tomlinson commented Sep 15, 2016 •

edited

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

vladikoff Sep 30, 2016 •

edited

Contributor

vladikoff Sep 30, 2016
Contributor

vladikoff Sep 30, 2016
Contributor

vladikoff Sep 30, 2016
Contributor

vladikoff Sep 30, 2016
Contributor

vladikoff Sep 30, 2016
Contributor

vladikoff Sep 30, 2016
Contributor

shane-tomlinson Sep 30, 2016
Author Member

vladikoff Sep 30, 2016 •

edited

Contributor

shane-tomlinson Sep 30, 2016 •

edited

Author Member

vladikoff Sep 30, 2016
Contributor

vladikoff Sep 30, 2016
Contributor

vladikoff Sep 30, 2016
Contributor

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016 •

edited

Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

shane-tomlinson Sep 30, 2016
Author Member

vladikoff commented Sep 30, 2016 •

edited

vbudhram Oct 3, 2016
Contributor

vbudhram Oct 3, 2016
Contributor

shane-tomlinson Oct 10, 2016
Author Member

shane-tomlinson Oct 12, 2016
Author Member

vbudhram Oct 3, 2016
Contributor

shane-tomlinson Oct 4, 2016
Author Member

shane-tomlinson commented Oct 4, 2016 •

edited

shane-tomlinson Oct 10, 2016
Author Member

philbooth Oct 12, 2016
Contributor

shane-tomlinson Oct 12, 2016
Author Member

shane-tomlinson Oct 12, 2016
Author Member

shane-tomlinson Oct 12, 2016
Author Member

seanmonstar Oct 12, 2016
Member

shane-tomlinson Oct 12, 2016
Author Member

shane-tomlinson Oct 12, 2016
Author Member

shane-tomlinson Oct 12, 2016
Author Member

shane-tomlinson Oct 12, 2016
Author Member

shane-tomlinson Oct 12, 2016
Author Member

shane-tomlinson Oct 13, 2016
Author Member

shane-tomlinson Oct 14, 2016
Author Member