feat(oauth): support Fenix WebChannels #2320

vladikoff · 2019-08-26T20:54:24Z

Ref: mozilla-mobile/android-components#3881
Fixes mozilla/application-services#1594
Fixes #2317

vladikoff · 2019-08-29T23:10:53Z

@shane-tomlinson @rfk @vbudhram I'm finishing up the tests for this, but please take a look at the non-test code. We scoped it down with Grisha, and seems like there are not as many changes as anticipated for the v1 of this

rfk

seems like there are not as many changes as anticipated for the v1 of this

This is indeed a smaller change than I was expecting, 👍!

It all makes sense to me, and I left only nits below. But I don't feel like the right person to give this an explicit r+.

As I mentioned briefly in Portland, I think the use of a single "broker" abstraction is starting to strain a little bit here, and the fact that we need separate "oauth webchannel broker" and "pairing supplicant webchannel broker" models is a symptom of that. I suspect there are two related but distinct abstractions in play here, once which controls the kind of flow being performed (a pairing supplicant flow) and another which controls how the results of that flow are communicated out (via oauth webchannels).

It'd be nice to be able to decompose those, but that's a way bigger refactor than justified by this PR. Also it's something that @shane-tomlinson will likely have much better-informed opinions on that I do 😄.

rfk · 2019-08-29T23:32:00Z

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-webchannel-v1.js

+    });
+
+    return channel;
+  },


I wonder if it's worth some sort of WebChannelMixin that could provide shared implementations of this logic.

👍 - seems like a great suggestion to minimize duplication.

Now as easy as other methods, I filed a follow up for this #2406

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-webchannel-v1.js

rfk · 2019-08-29T23:41:57Z

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-webchannel-v1.js

+    const supportedEngines =
+      response.capabilities && response.capabilities.engines;
+    if (supportedEngines) {
+      // supportedEngines override the defaults


This is a subtle change in behaviour from the desktop broker, I think it would be worth calling out more explicitly in e.g. the new protocol doc for the oauth-webchannel broker.

So Fenix passes back all of the engines it supports and there are no defaults?

(Thought I answered this somewhere already) Yes, the native app like Fenix will send in the engines in supports

packages/fxa-content-server/server/lib/configuration.js

packages/fxa-content-server/docs/relier-communication-protocols/oauth-webchannel.md

vbudhram

@vladikoff This looks 👍🏽 to me, I don't really have any objections with the approach here.

packages/fxa-content-server/app/tests/spec/models/auth_brokers/oauth-webchannel-v1.js

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-webchannel-v1.js

vbudhram · 2019-08-30T15:01:03Z

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-webchannel-v1.js

+  },
+
+  getCommand(commandName) {
+    if (!this.commands) {


Would this ever be undefined and throw?

Yeah some brokers can have this be undefined

vbudhram · 2019-08-30T15:06:58Z

packages/fxa-content-server/docs/relier-communication-protocols/oauth-webchannel.md

+as part of the [firefox-accounts](https://github.com/mozilla-mobile/android-components/blob/master/components/service/firefox-accounts/README.md) Android component.
+
+```
+ * Communication channel is established from web content to this class via webextension, as follows:


This was helpful 👍🏽

shane-tomlinson

The overall approach looks good, and the PR is definitely moving in the right direction. I left a bunch of questions/suggestions/possible simplifications, I don't think anything too major.

packages/fxa-content-server/app/scripts/lib/channels/receivers/web-channel.js

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-redirect.js

shane-tomlinson · 2019-09-03T15:54:05Z

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-webchannel-v1.js

+    });
+
+    return channel;
+  },


👍 - seems like a great suggestion to minimize duplication.

shane-tomlinson · 2019-09-03T15:56:52Z

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-webchannel-v1.js

+    const supportedEngines =
+      response.capabilities && response.capabilities.engines;
+    if (supportedEngines) {
+      // supportedEngines override the defaults


So Fenix passes back all of the engines it supports and there are no defaults?

packages/fxa-content-server/app/scripts/models/auth_brokers/pairing/supplicant-webchannel.js

packages/fxa-content-server/docs/relier-communication-protocols/oauth-webchannel.md

shane-tomlinson · 2019-09-03T16:51:20Z

packages/fxa-content-server/docs/relier-communication-protocols/oauth-webchannel.md

+ * [fxa-web-content] <--js events--> [fxawebchannel.js webextension] <--port messages--> [WebChannelFeature]
+ *
+ * [fxa-web-channel]            [WebChannelFeature]         Notes:
+ *     fxa-status       ------>          |                  web content requests account status & device capabilities


Do you need/want to document fxaccounts:can_link_account?

No, we are dropping that for now

shane-tomlinson · 2019-09-03T16:53:19Z

packages/fxa-content-server/tests/functional/oauth_webchannel.js

+          .then(
+            openFxaFromRp('signup', {
+              query: {
+                context: 'oauth_webchannel_v1',


No longer need ?service=sync? Wow

packages/fxa-content-server/tests/functional/oauth_webchannel.js

shane-tomlinson · 2019-09-03T16:57:36Z

packages/fxa-content-server/tests/functional/oauth_webchannel.js

+
+          // switch to the original window
+          .then(closeCurrentWindow())
+          .then(testIsBrowserNotified('fxaccounts:oauth_login'))


Do you need to test for an expected screen here?

Not here, for now we take over on login

vladikoff

updated

vladikoff · 2019-09-04T20:00:57Z

@shane-tomlinson we landed the client part in mozilla-mobile/android-components#4221 , I've rebased this PR against train-145. Re-Requested review, let me know!

shane-tomlinson

r+ - looks great. There is one var that could be a const, and one initialize method that doesn't seem to be needed, plus you might want to add functional tests for the email first flow if those are used by Fenix.

shane-tomlinson · 2019-09-05T11:13:14Z

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-redirect.js

@@ -345,7 +354,10 @@ export default BaseAuthenticationBroker.extend({
      this.clearOriginalTabMarker();
      return this.getOAuthResult(account).then(result => {
        result = _.extend(result, additionalResultData);
-        return this.sendOAuthResultToRelier(result);


The way you reordered the metrics flush and sending the result to the relier makes much more sense. 👍

shane-tomlinson · 2019-09-05T11:24:31Z

packages/fxa-content-server/app/scripts/models/auth_brokers/oauth-webchannel-v1.js

+   */
+  sendOAuthResultToRelier(result, account) {
+    result.redirect = Constants.OAUTH_WEBCHANNEL_REDIRECT;
+    if (account && account.get('declinedSyncEngines')) {


packages/fxa-content-server/app/scripts/models/auth_brokers/pairing/supplicant-webchannel.js

packages/fxa-content-server/app/tests/spec/models/auth_brokers/oauth-webchannel-v1.js

shane-tomlinson · 2019-09-05T11:41:32Z

packages/fxa-content-server/tests/functional/oauth_webchannel.js

+    );
+  },
+  tests: {
+    signup: function() {


Does Fenix make use of the email first flow? If so, should probably test that too.

Ref: mozilla-mobile/android-components#3881 Fixes mozilla/application-services#1594 Fixes #2317

vladikoff mentioned this pull request Aug 26, 2019

[WIP] Add oauth_webchannel_v1 for Fenix and GeckoView apps #2043

Closed

vladikoff force-pushed the feature.fenix-channel-clean branch 4 times, most recently from 8ee8749 to 4fb0a6d Compare August 29, 2019 23:09

vladikoff marked this pull request as ready for review August 29, 2019 23:09

rfk reviewed Aug 29, 2019

View reviewed changes

vbudhram reviewed Aug 30, 2019

View reviewed changes

clouserw mentioned this pull request Aug 30, 2019

CWTS: need to have full control of selectable engines for Fenix #2317

Closed

shane-tomlinson suggested changes Sep 3, 2019

View reviewed changes

vladikoff force-pushed the feature.fenix-channel-clean branch 2 times, most recently from d52f95e to 56971a5 Compare September 4, 2019 19:36

vladikoff mentioned this pull request Sep 4, 2019

[fxa-content-server] Remove createChannel duplication in webchannel and sync broker #2406

Closed

vladikoff commented Sep 4, 2019

View reviewed changes

vladikoff requested a review from shane-tomlinson September 4, 2019 19:50

vladikoff mentioned this pull request Sep 4, 2019

[fxa-content-server] Convert from _.extend to ... in createAuthBroker #2407

Closed

vladikoff changed the base branch from master to train-145 September 4, 2019 19:53

vladikoff force-pushed the feature.fenix-channel-clean branch from 56971a5 to 5523dbf Compare September 4, 2019 19:56

vladikoff force-pushed the feature.fenix-channel-clean branch 2 times, most recently from 4dc790f to b67f200 Compare September 4, 2019 20:46

shane-tomlinson approved these changes Sep 5, 2019

View reviewed changes

feat(oauth): support Fenix WebChannels

25f33db

Ref: mozilla-mobile/android-components#3881 Fixes mozilla/application-services#1594 Fixes #2317

vladikoff force-pushed the feature.fenix-channel-clean branch from b67f200 to 25f33db Compare September 5, 2019 14:07

vladikoff mentioned this pull request Sep 5, 2019

[fxa-content-server] Add email first test for OAuth WebChannels #2415

Closed

vladikoff merged commit fc5cd27 into train-145 Sep 5, 2019

vladikoff deleted the feature.fenix-channel-clean branch September 5, 2019 14:41

vladikoff mentioned this pull request Sep 5, 2019

Please tag 145.1 with PR2320 #2418

Closed

data-sync-user mentioned this pull request Jun 21, 2021

[fxa-content-server] Remove createChannel duplication in webchannel and sync broker #9447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(oauth): support Fenix WebChannels #2320

feat(oauth): support Fenix WebChannels #2320

vladikoff commented Aug 26, 2019 •

edited

Loading

vladikoff commented Aug 29, 2019

rfk left a comment

rfk Aug 29, 2019

shane-tomlinson Sep 3, 2019

vladikoff Sep 4, 2019

rfk Aug 29, 2019

shane-tomlinson Sep 3, 2019

vladikoff Sep 4, 2019

vbudhram left a comment

vbudhram Aug 30, 2019

vladikoff Sep 4, 2019

vbudhram Aug 30, 2019

shane-tomlinson left a comment

shane-tomlinson Sep 3, 2019

shane-tomlinson Sep 3, 2019

shane-tomlinson Sep 3, 2019

vladikoff Sep 4, 2019

shane-tomlinson Sep 3, 2019

shane-tomlinson Sep 3, 2019

vladikoff Sep 4, 2019

vladikoff left a comment

vladikoff commented Sep 4, 2019

shane-tomlinson left a comment

shane-tomlinson Sep 5, 2019

shane-tomlinson Sep 5, 2019

shane-tomlinson Sep 5, 2019

vladikoff Sep 5, 2019

feat(oauth): support Fenix WebChannels #2320

feat(oauth): support Fenix WebChannels #2320

Conversation

vladikoff commented Aug 26, 2019 • edited Loading

vladikoff commented Aug 29, 2019

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-tomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikoff left a comment

Choose a reason for hiding this comment

vladikoff commented Sep 4, 2019

shane-tomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikoff commented Aug 26, 2019 •

edited

Loading