fix(reset-password): Fix the reset password flow w/ e10s enabled! #4770

shane-tomlinson · 2017-03-01T16:07:26Z

What is the problem?

Fx w/ e10s enabled isolates localStorage in about:accounts from
localStorage in normal web content. If a user created a new profile
and did the reset password flow, completed the reset password flow
in the same browser, and then clicked "Manage account", the
user would have to sign in again because of this localStorage isolation.
The last localStorage write in about:accounts blew away the user's
account information, and in fact added a junk account with a uid
of undefined.

How does this fix it?

Pass all the data needed to retain the user's session from
the confirm_reset_account tab to the about:accounts tab. The
passed data will be written to localStorage from the about:accounts
tab. Whenever the user clicks Manage Account, the data will
be in localStorage, as expected.

What is `user.removeAccountsWithInvalidUid`?

A bunch of users report having > 1 stored account, it may
be most of these are users who have reset their password
and have one of these junk accounts. Remove them.

fixes #4748
fixes #4763
fixes #4769

@vladikoff, @vbudhram or @philbooth - r?

vladikoff · 2017-03-01T16:13:16Z

app/tests/spec/models/user.js

@@ -67,8 +72,8 @@ define(function (require, exports, module) {
    });

    describe('initAccount', function () {
-      var account;
-      var email = 'a@a.com';
+      let  account;


let account

vladikoff · 2017-03-01T16:14:23Z

@shane-tomlinson do we need a functional test to simulate empty local storage (simulate e10s conditions)?

shane-tomlinson · 2017-03-01T16:24:36Z

@shane-tomlinson do we need a functional test to simulate empty local storage (simulate e10s conditions)?

Ya, I hadn't thought of that. Good call. I'll see what I can think of.

vladikoff · 2017-03-01T16:37:06Z

app/tests/spec/models/user.js


    function testRemoteSignInMessageSent(account) {
      assert.equal(notifier.triggerRemote.callCount, 1);
-      var args = notifier.triggerRemote.args[0];
+      const  args = notifier.triggerRemote.args[0];


const args -> const args

vbudhram · 2017-03-01T17:39:21Z

app/scripts/views/confirm_reset_password.js

+      // from within about:accounts. Because of this, all account data needed to sign in must
+      // be passed between windows. See https://github.com/mozilla/fxa-content-server/issues/4763
+      // and https://bugzilla.mozilla.org/show_bug.cgi?id=666724
+      //
      // keyFetchToken and unwrapBKey are sent from the verification tab,


Is the part about Hello still needed here? Or maybe we can nuke this comment block completely?

I wondered the same, as well as whether we are able to remove keyFetchToken and unwrapBKey from the list of sent fields since we no longer support Hello.

We'll break reset password for fx_desktop_v1, and possibly fx_ios_v1 where the originating tab has to send the login message to the browser, but most users won't open the password reset link in Firefox for iOS anyways.

I can open an issue to explore, I didn't want to fix one thing to inadvertently break another. The comment should at a minimum be updated to mention fx_desktop_v1 and fx_ios_v1.

vbudhram · 2017-03-01T17:39:21Z

app/tests/spec/models/user.js


    function testRemoteSignInMessageSent(account) {
      assert.equal(notifier.triggerRemote.callCount, 1);
-      var args = notifier.triggerRemote.args[0];
+      const  args = notifier.triggerRemote.args[0];
      assert.lengthOf(args, 2);
      assert.equal(args[0], notifier.COMMANDS.SIGNED_IN);

      // unwrapBKey and keyFetchToken are used in password reset


Same as above, still need to mention Hello?

philbooth · 2017-03-02T07:26:06Z

app/scripts/models/user.js

+      return p().then(() => {
+        const accounts = this._accounts();
+        for (const uid in accounts) {
+          if (! uid || uid === 'undefined') {


Oh! From the PR description I thought you meant uid was undefined. But you meant it was the string 'undefined'? (just checking to be sure that this test is right)

Yup, accounts are being written with the string undefined. I'll update the comments for clarity.

philbooth · 2017-03-02T07:32:20Z

docs/client-metrics.md

@@ -115,6 +115,7 @@ The event stream is a log of events and the time they occurred while the user is
 * error.&lt;unexpected_origin&gt;.auth.1027 - a postMessage message was received from an unexpected origin.
 * error.&lt;image_url&gt;.profile.997 - a profile image could not load.
 * error.&lt;screen_name&gt;.&lt;service&gt;.998 - System unavailable.
+* error.no context.auth.1050 - an attempt is being made to write an account with no uid to localStorage.


Sorry if this is a stupid question, but is that whitespace supposed to be there?

It should actually be unknown context, with a space. I could change the context to user so that one exists.

I changed the context to persistAccount so we know exactly where that came from.

What is the problem? Fx w/ e10s enabled isolates localStorage in about:accounts from localStorage in normal web content. If a user created a new profile and did the reset password flow, completed the reset password flow in the same browser, and then clicked "Manage account", the user would have to sign in again because of this localStorage isolation. The last localStorage write in about:accounts blew away the user's account information, and in fact added a junk account with a uid of `undefined`. How does this fix it? Pass all the data needed to retain the user's session from the confirm_reset_account tab to the about:accounts tab. The passed data will be written to localStorage from the about:accounts tab. Whenever the user clicks Manage Account, the data will be in localStorage, as expected. What is `user.removeAccountsWithInvalidUid`? A bunch of users report having > 1 stored account, it may be most of these are users who have reset their password and have one of these junk accounts. Remove them. fixes #4763 fixes #4769

* Give the ACCOUNT_HAS_NO_UID error a context when reporting. * Update comments related to the uid of the string `undefined`. * Remove comments about Hello.

shane-tomlinson · 2017-03-02T13:14:53Z

@shane-tomlinson do we need a functional test to simulate empty local storage (simulate e10s conditions)?

@vladikoff - I cannot figure out a way to functional test this flow w/o actually using about:accounts.
The reason is our functional tests all run in "content" tabs & writes to localStorage are not isolated from each other. When the user completes the password reset in tab 2, the following process occurs:

localStorage is written in tab 2
a message is immediately sent to tab 1
localStorage is written again in tab 1.

If I were to clear localStorage, it'd have to be between steps 1 and 2, but as an outsider, I don't have control over that.

I could tap into environment.isAboutAccounts, but that seems kooky.

I attempted to create a functional test that actually uses about:accounts, but the test keeps timing about at switchToFrame. I've tried passing switchToFrame 0, the element id, and a reference to the element, all of which time out.

diff --git a/tests/functional/sync_v2_reset_password.js b/tests/functional/sync_v2_reset_password.js
index c3b1bf0d..c23fea90 100644
--- a/tests/functional/sync_v2_reset_password.js
+++ b/tests/functional/sync_v2_reset_password.js
@@ -45,6 +45,42 @@ define([
       return this.remote.then(clearBrowserState());
     },
 
+    'reset password from about:accounts, verify same browser': function () {
+      this.timeout = 120000;
+      return this.remote
+        .then(openPage('about:accounts?action=signin&entrypoint=menupanel', '#fxa-signin-header'))
+        .switchToFrame(0)
+          .then(testElementExists('#fxa-signin-header'))
+          .then(click('.reset-password'))
+          .then(testElementExists('#fxa-reset-password-header'))
+
+          .then(createUser(email, PASSWORD, { preVerified: true }))
+          .then(fillOutResetPassword(email))
+
+          .then(testElementExists('#fxa-confirm-reset-password-header'))
+          .then(openVerificationLinkInNewTab(email, 0))
+
+          .switchToWindow('newwindow')
+
+          .then(testElementExists('#fxa-complete-reset-password-header'))
+          .then(fillOutCompleteResetPassword(PASSWORD, PASSWORD))
+
+          .then(testElementExists('#fxa-reset-password-complete-header'))
+          .then(testElementExists('.account-ready-service'))
+
+          // the verification tab sends the WebChannel message. This fixes
+          // two problems: 1) initiating tab is closed, 2) The initiating
+          // tab when running in E10s does not have all the necessary data
+          // because localStorage is not shared.
+          .then(testIsBrowserNotified('fxaccounts:login'))
+
+          .then(closeCurrentWindow())
+
+          .then(testSuccessWasShown())
+          .then(noSuchBrowserNotification('fxaccounts:login'))
+        .switchToParentFrame();
+    },
+
     'reset password, verify same browser': function () {
       return this.remote
         .then(openPage(RESET_PASSWORD_URL, '#fxa-reset-password-header'))
diff --git a/tests/tools/firefox_profile_creator.js b/tests/tools/firefox_profile_creator.js
index 63b79d10..633ac8fb 100644
--- a/tests/tools/firefox_profile_creator.js
+++ b/tests/tools/firefox_profile_creator.js
@@ -27,6 +27,7 @@ if (profile) {
   myProfile.setPreference('identity.fxaccounts.remote.signin.uri', profile.fxaContentRoot + 'signin?service=sync&context=fx_desktop_v1');
   myProfile.setPreference('identity.fxaccounts.remote.signup.uri', profile.fxaContentRoot + 'signup?service=sync&context=fx_desktop_v1');
   myProfile.setPreference('identity.fxaccounts.settings.uri', profile.fxaContentRoot + 'settings');
+  myProfile.setPreference('identity.fxaccounts.allowHttp', true);
   myProfile.setPreference('services.sync.tokenServerURI', profile.fxaToken);
   myProfile.setPreference('app.update.enabled', false);
   myProfile.setPreference('app.update.auto', false);
@@ -61,6 +62,7 @@ if (profile) {
   myProfile.setPreference('experiments.manifest.uri', '');
   myProfile.setPreference('network.allow-experiments', false);
 
+  myProfile.setPreference('marionette.logging', 'TRACE');
 
   myProfile.updatePreferences();

shane-tomlinson · 2017-03-03T16:10:31Z

This makes me happy. :D

shane-tomlinson added the waffle:active label Mar 1, 2017

vladikoff reviewed Mar 1, 2017

View changes

vbudhram reviewed Mar 1, 2017

View changes

philbooth reviewed Mar 2, 2017

View changes

shane-tomlinson force-pushed the issues-4763-4769-fix-password-reset-e10s branch from 0a86ac9 to 31b4815 Mar 2, 2017

fix(client): Fixes based on feedback.

ee19e30

* Give the ACCOUNT_HAS_NO_UID error a context when reporting. * Update comments related to the uid of the string `undefined`. * Remove comments about Hello.

shane-tomlinson force-pushed the issues-4763-4769-fix-password-reset-e10s branch from 31b4815 to ee19e30 Mar 2, 2017

shane-tomlinson added shipit waffle:review and removed waffle:active labels Mar 2, 2017

vladikoff removed the waffle:review label Mar 3, 2017

vladikoff deleted the issues-4763-4769-fix-password-reset-e10s branch Mar 3, 2017

rfk added this to the FxA-0: quality milestone Mar 16, 2017

mozilla / fxa-content-server Archived

fix(reset-password): Fix the reset password flow w/ e10s enabled! #4770

fix(reset-password): Fix the reset password flow w/ e10s enabled! #4770

shane-tomlinson commented Mar 1, 2017 •

edited

This comment has been minimized.

vladikoff commented Mar 1, 2017

shane-tomlinson commented Mar 1, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shane-tomlinson commented Mar 2, 2017

shane-tomlinson commented Mar 3, 2017

mozilla / fxa-content-server Archived

fix(reset-password): Fix the reset password flow w/ e10s enabled! #4770

fix(reset-password): Fix the reset password flow w/ e10s enabled! #4770

Conversation

shane-tomlinson commented Mar 1, 2017 • edited

What is the problem?

How does this fix it?

What is user.removeAccountsWithInvalidUid?

This comment has been minimized.

vladikoff Mar 1, 2017 Contributor

vladikoff commented Mar 1, 2017

shane-tomlinson commented Mar 1, 2017

This comment has been minimized.

vladikoff Mar 1, 2017 Contributor

This comment has been minimized.

vbudhram Mar 1, 2017 Contributor

This comment has been minimized.

shane-tomlinson Mar 1, 2017 Author Member

This comment has been minimized.

vbudhram Mar 1, 2017 • edited Contributor

This comment has been minimized.

philbooth Mar 2, 2017 Contributor

This comment has been minimized.

shane-tomlinson Mar 2, 2017 Author Member

This comment has been minimized.

philbooth Mar 2, 2017 Contributor

This comment has been minimized.

shane-tomlinson Mar 2, 2017 Author Member

This comment has been minimized.

shane-tomlinson Mar 2, 2017 Author Member

shane-tomlinson commented Mar 2, 2017

shane-tomlinson commented Mar 3, 2017

shane-tomlinson commented Mar 1, 2017 •

edited

What is `user.removeAccountsWithInvalidUid`?

vladikoff Mar 1, 2017
Contributor

vladikoff Mar 1, 2017
Contributor

vbudhram Mar 1, 2017
Contributor

shane-tomlinson Mar 1, 2017
Author Member

vbudhram Mar 1, 2017 •

edited

Contributor

philbooth Mar 2, 2017
Contributor

shane-tomlinson Mar 2, 2017
Author Member

philbooth Mar 2, 2017
Contributor

shane-tomlinson Mar 2, 2017
Author Member

shane-tomlinson Mar 2, 2017
Author Member