feat(client): Sync state with the browser! #4695

shane-tomlinson · 2017-02-02T09:29:11Z

Very early stage yet, putting this up so we don't forget about it. Goes along with https://bugzilla.mozilla.org/attachment.cgi?id=8832820

Perform a handshake with the browser to use the browser as the canonical
source of truth when it comes to signed in accounts.

issue #4252
https://bugzilla.mozilla.org/show_bug.cgi?id=1308038

shane-tomlinson · 2017-03-02T15:08:17Z

app/scripts/models/auth_brokers/fx-desktop-v4.js

+/**
+ * A variant of the FxSync broker that speaks "v4" of the protocol.
+ *
+ * It used to enable syncPreferencesNotification on the verification complete screen.


This comment lies.

shane-tomlinson · 2017-03-03T17:39:53Z

This is no longer WIP and ready to go. This goes along with the Firefox patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1308038

shane-tomlinson · 2017-03-03T17:44:13Z

@rfk, @vladikoff - no rush on this, mind starting an r when you have time? Testing requires m-c w/ the browser patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1308038.

rfk

This looks good to me, but I want to dig into the interaction between localStorage and private-browsing mode just in case there are any edge-cases to worry about here.

rfk · 2017-03-06T01:49:04Z

app/scripts/models/auth_brokers/fx-sync.js

+                account.set(_.pick(userData, 'email', 'sessionToken', 'uid', 'verified'));
+                account.set('sessionTokenContext', SESSION_TOKEN_USED_FOR_SYNC);
+
+                return user.setSignedInAccount(account);


This persists the data from the handshake into localStorage, right? I wonder whether that will cause trouble for us when using this in private browsing mode, if the data so written then becomes available to other tabs in the same private-browsing window. I'll try to build m-c with this patch and find out, but it will take a while as I don't have my dev environment set up on this machine yet...

You are correct, we write to this tab's localStorage so the data will be available even in PB mode to subsequent browses to an OAuth relier in this same tab. If we avoid writing to localStorage, we'll break users of non-PB mode who subsequently visit an OAuth relier.

rfk · 2017-03-06T01:49:04Z

app/scripts/models/auth_brokers/fx-sync.js

+                return user.setSignedInAccount(account);
+              } else {
+                const signedInAccount = user.getSignedInAccount();
+                user.removeAccount(signedInAccount);


Is there anything in the account that we should destroy at this point, e.g. oauth tokens?

That question can be applied more broadly about how we should be handling signed out accounts. I have a feeling that's a can of worms that'll explode this PR and could be handled independently.

rfk · 2017-03-06T01:49:04Z

app/tests/spec/models/auth_brokers/fx-desktop-v4.js

+    });
+
+    describe('capabilities', () => {
+      it('has the `allowUidChange` capability', () => {


This test title is the only occurrence of "allowUidChange" in the PR, is it a hangover from a previous naming convention?

copy/paste error

rfk · 2017-03-06T01:52:50Z

I want to dig into the interaction between localStorage and private-browsing
mode just in case there are any edge-cases to worry about here.

Actually, it's not clear precisely how this is enforcing the 'only request "fxa_status" IFF service=sync' requirement from https://bugzilla.mozilla.org/show_bug.cgi?id=1308038#c27; it looks like it will request status from the browser whenever context=fx_desktop_v4, but does that guarantee that service=sync?

The move is so these mixins can be used by any objects. This is an extraction from #4695. issue #4252

This is an extraction from #4695 issue #4252

The move is so these mixins can be used by any objects. This is an extraction from #4695. issue #4252

…dikoff This is an extraction from #4695 issue #4252

#5099) r=vladikoff The move is so these mixins can be used by any objects. This is an extraction from #4695. issue #4252

shane-tomlinson · 2017-05-24T21:55:14Z

@mozilla/fxa-devs - r? I'm calling this ready. This patch works for real using today's Nightly. Use fxa-dev-launcher to point to a local install, and give it a whirl!

Things to test:

Sign in to Sync, clear history, open about:preferences#sync and click "Manage Account"
Sign in to Sync, open a private browsing window, open about:preferences#sync and click "Manage Account"
Set history settings to "Never remember" (permanent Private Browsing mode), sign in to Sync, open a private browsing window, open about:preferences#sync and click "Manage Account".
With no user signed in to Sync, sign into an OAuth relier. Sign into Sync (user will not be pre-filled). Sign into the OAuth relier again, when redirecting to FxA, original email used to sign into relier should be remembered.
With a user signed in to Sync, sign into an OAuth relier - at FxA, Sync user should be prefilled. Sign into the OAuth relier again, when redirecting to FxA, Sync email should still be remembered.

Another thing to note - I've started a selectors file for the functional tests - https://github.com/mozilla/fxa-content-server/pull/4695/files#diff-fc925842e92dad5a948ae59844a1a901. Instead of hard coding selectors in the tests, let's add entries to that file.

philbooth · 2017-05-25T07:37:05Z

app/scripts/lib/channels/web.js

+     *   Defaults to `this.getUserAgentString()`
+     * @returns {Boolean}
+     */
+    isFxaStatusSupported (userAgent = this.getUserAgentString()) {


TIL that default values can be function calls and have access to this.

philbooth · 2017-05-25T07:43:49Z

app/scripts/models/auth_brokers/base.js

+
+      const channel = this._notificationChannel;
+      return p().delay(TEST_REQUEST_DELAY_MS)
+        // OAuth reliers will have `service` set to the client_id, Sync to `sync`


Not sure if it's just me but part of this comment reads back to front. Should it be "...will have the client_id set to service"?

No, it shouldn't. Sorry, just woken up here.

I was just thinking this comment doesn't really provide any value. I'll just rip it out instead of causing more confusion. The comment on lines 125-127 are the informative ones.

philbooth · 2017-05-25T07:50:27Z

app/scripts/models/user.js

+      // If trying to sign in to an OAuth relier, prefer any users that are
+      // stored in localStorage and only use the browser's state if no
+      // user is stored.
+      return !! (service === Constants.SYNC_SERVICE || this.getSignedInAccount().isDefault());


Looks like the explicit !! is unnecessary here? === returns a boolean and so does account.isDefault(). Not a big deal obviously but may be a trifle more readable without it?

philbooth · 2017-05-25T08:33:24Z

@shane-tomlinson, this is great! LGTM, WFM, r+.

* Remove a comment that makes no sense. * Remove an extra !!, the value was always going to be Boolean anyways.

shane-tomlinson added waffle:active WIP labels Feb 2, 2017

shane-tomlinson added this to the FxA-0: quality milestone Feb 6, 2017

vladikoff assigned shane-tomlinson Feb 6, 2017

vladikoff added waffle:next and removed waffle:active labels Feb 6, 2017

vladikoff self-assigned this Feb 6, 2017

shane-tomlinson added waffle:backlog and removed waffle:next labels Feb 9, 2017

shane-tomlinson force-pushed the issue-4252-sync-me-with-the-browser branch from 1460fd5 to 71614ac Mar 2, 2017

shane-tomlinson reviewed Mar 2, 2017

View changes

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

shane-tomlinson force-pushed the issue-4252-sync-me-with-the-browser branch 5 times, most recently from 8dab6bf to 7dd3bec Mar 3, 2017

shane-tomlinson changed the title ~~[WIP] feat(client): Sync state with the browser!~~ feat(client): Sync state with the browser! Mar 3, 2017

shane-tomlinson removed the WIP label Mar 3, 2017

shane-tomlinson force-pushed the issue-4252-sync-me-with-the-browser branch from 7dd3bec to 62da580 Mar 3, 2017

shane-tomlinson requested review from rfk and vladikoff Mar 3, 2017

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

rfk reviewed Mar 6, 2017

View changes

shane-tomlinson removed the waffle:review label Mar 7, 2017

shane-tomlinson added waffle:active and removed waffle:next labels May 24, 2017

shane-tomlinson force-pushed the issue-4252-sync-me-with-the-browser branch from 3825534 to bee596a May 24, 2017

shane-tomlinson pushed a commit that referenced this pull request May 24, 2017

refactor(mixins): Move search-param-mixin and user-agent-mixin to /lib

e7a69d6

The move is so these mixins can be used by any objects. This is an extraction from #4695. issue #4252

shane-tomlinson mentioned this pull request May 24, 2017

refactor(mixins): Move search-param-mixin and user-agent-mixin to /lib #5099

Merged

shane-tomlinson pushed a commit that referenced this pull request May 24, 2017

refactor(test): In app-start.js, convert to fat arrows.

51cc718

This is an extraction from #4695 issue #4252

shane-tomlinson mentioned this pull request May 24, 2017

refactor(test): In app-start.js, convert to fat arrows. #5100

Merged

shane-tomlinson force-pushed the issue-4252-sync-me-with-the-browser branch from bee596a to 5c1641e May 24, 2017

shane-tomlinson pushed a commit that referenced this pull request May 24, 2017

refactor(mixins): Move search-param-mixin and user-agent-mixin to /lib

67b4d4b

The move is so these mixins can be used by any objects. This is an extraction from #4695. issue #4252

vladikoff added a commit that referenced this pull request May 24, 2017

refactor(test): In app-start.js, convert to fat arrows. (#5100) r=vla…

5f7d8b6

…dikoff This is an extraction from #4695 issue #4252

vladikoff added a commit that referenced this pull request May 24, 2017

refactor(mixins): Move search-param-mixin and user-agent-mixin to /lib (

efea825

#5099) r=vladikoff The move is so these mixins can be used by any objects. This is an extraction from #4695. issue #4252

feat(client): Sync state with the browser!

6a51725

shane-tomlinson force-pushed the issue-4252-sync-me-with-the-browser branch from 5c1641e to b89af11 May 24, 2017

refactor(client): try to refactor my way out of the complexity

b6b54c9

shane-tomlinson force-pushed the issue-4252-sync-me-with-the-browser branch from b89af11 to b6b54c9 May 24, 2017

shane-tomlinson added waffle:review and removed waffle:active labels May 24, 2017

philbooth reviewed May 25, 2017

View changes

Shane Tomlinson added 2 commits May 25, 2017

fix(client): Cleanup based on @philbooth's comments.

e1c46eb

* Remove a comment that makes no sense. * Remove an extra !!, the value was always going to be Boolean anyways.

Merge branch 'master' into issue-4252-sync-me-with-the-browser

6fe7dc6

shane-tomlinson deleted the issue-4252-sync-me-with-the-browser branch May 25, 2017

shane-tomlinson removed the waffle:review label May 25, 2017

vladikoff mentioned this pull request Jun 1, 2017

Server is not loading in latest Nightly #5114

Closed

shane-tomlinson mentioned this pull request Jun 19, 2017

"Manage Account" and sign-in confirmation interact badly #4252

Closed

mozilla / fxa-content-server Archived

feat(client): Sync state with the browser! #4695

feat(client): Sync state with the browser! #4695

shane-tomlinson commented Feb 2, 2017 •

edited by vladikoff

This comment has been minimized.

shane-tomlinson commented Mar 3, 2017

shane-tomlinson commented Mar 3, 2017

rfk left a comment

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.

rfk commented Mar 6, 2017

shane-tomlinson commented May 24, 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.

philbooth commented May 25, 2017

mozilla / fxa-content-server Archived

feat(client): Sync state with the browser! #4695

feat(client): Sync state with the browser! #4695

Conversation

shane-tomlinson commented Feb 2, 2017 • edited by vladikoff

This comment has been minimized.

shane-tomlinson Mar 2, 2017 Author Member

shane-tomlinson commented Mar 3, 2017

shane-tomlinson commented Mar 3, 2017

rfk left a comment

This comment has been minimized.

rfk Mar 6, 2017 Member

This comment has been minimized.

shane-tomlinson Mar 6, 2017 Author Member

This comment has been minimized.

rfk Mar 6, 2017 Member

This comment has been minimized.

shane-tomlinson Mar 6, 2017 Author Member

This comment has been minimized.

rfk Mar 6, 2017 Member

This comment has been minimized.

shane-tomlinson Mar 6, 2017 Author Member

rfk commented Mar 6, 2017

shane-tomlinson commented May 24, 2017

This comment has been minimized.

philbooth May 25, 2017 Contributor

This comment has been minimized.

philbooth May 25, 2017 Contributor

This comment has been minimized.

philbooth May 25, 2017 Contributor

This comment has been minimized.

shane-tomlinson May 25, 2017 Author Member

This comment has been minimized.

philbooth May 25, 2017 • edited Contributor

philbooth commented May 25, 2017

shane-tomlinson commented Feb 2, 2017 •

edited by vladikoff

shane-tomlinson Mar 2, 2017
Author Member

rfk Mar 6, 2017
Member

shane-tomlinson Mar 6, 2017
Author Member

rfk Mar 6, 2017
Member

shane-tomlinson Mar 6, 2017
Author Member

rfk Mar 6, 2017
Member

shane-tomlinson Mar 6, 2017
Author Member

philbooth May 25, 2017
Contributor

philbooth May 25, 2017
Contributor

philbooth May 25, 2017
Contributor

shane-tomlinson May 25, 2017
Author Member

philbooth May 25, 2017 •

edited

Contributor