WIP: Add OAuth->browser plumbing. #1423

shane-tomlinson · 2014-07-21T16:39:43Z

No description provided.

shane-tomlinson · 2014-07-21T16:40:26Z

shane-tomlinson · 2014-07-21T16:41:04Z

app/scripts/lib/channels/custom_events.js

+      return redirectParams && Url.searchParam(name, redirectParams);
+    },
+
+    completeOAuthNoInteraction: true,


Not sure about this name.

shane-tomlinson · 2014-07-21T16:41:32Z

app/scripts/lib/channels/factory.js

+      // A channel that attempts to listen for messages from the browser.
+      // unsure of how this is different from FxDesktopChannel
+      channel = new CustomEventsChannel();
+    } else {


I'm going to plug the iframe channel in right here.

shane-tomlinson · 2014-07-21T16:42:29Z

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

-        // params listed in:
-        // https://github.com/mozilla/fxa-oauth-server/blob/master/docs/api.md#post-v1authorization
-        params = Url.searchParams(this.window.location.search,
-                  ['client_id', 'redirect_uri', 'state', 'scope', 'action']);


Hey, where'd the whitelist go?

Hm not sure, did I move it?

I think I did.

shane-tomlinson · 2014-07-21T16:43:10Z

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

+      });
+    },
+
+    _getRedirectParam: function(redirect, name) {


Is this needed?

shane-tomlinson · 2014-07-21T16:44:10Z

app/scripts/views/ready.js

@@ -88,7 +102,9 @@ function (_, BaseView, FormView, Template, Session, Xss, Strings, ServiceMixin,

    submit: function () {
      if (this.isOAuthSameBrowser()) {
-        return this.finishOAuthFlow();
+        return this.finishOAuthFlow({
+          source: this.type


Should we rename source to something like sourceFlow or loginType?

sourceFlow works for me

shane-tomlinson · 2014-07-21T16:44:44Z

app/tests/mocks/window.js

@@ -38,7 +38,7 @@ function (_, Backbone) {

  _.extend(WindowMock.prototype, Backbone.Events, {
    dispatchEvent: function (event) {
-      var msg = event.detail.command;
+      var msg = event.detail.command || event.detail.message;


The FxADesktopChannel uses command whereas the CustomEventsChannel uses message :/

Yeah, hm, WebChannel sends messages on webChannelIds

vladikoff · 2014-07-21T16:51:57Z

app/scripts/lib/app-start.js

@@ -41,8 +40,7 @@ function (
  Translator,
  Session,
  Url,
-  WebChannel,
-  FxDesktopChannel,
+  channelFactory,


Yeah, it's a singleton since it's a factory.

open to suggestions for clarity.

vladikoff · 2014-07-21T16:52:52Z

app/scripts/lib/channels/factory.js

+    } else if (options.webChannelId) {
+      // A channel that attempts to listen for messages from the browser.
+      // unsure of how this is different from FxDesktopChannel
+      channel = new CustomEventsChannel();


probably should be called WebChannel

I feel that WebChannel has a connotation that does not match the actual implementation.

We are trying to keep it similar to what is going into the browser module.
See draft https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/WebChannel.jsm

That makes sense, can we call it the FirefoxWebChannel or MozillaWebChannel?

In the content-server, I think so. FirefoxWebChannel sounds good to me. @ckarlof might have feedback on that.

I'm fine with FirefoxWebChannel.

I don't know about this factory for channels. It seems like an unnecessary level of abstraction, especially since the API is weird. Sometimes it looks at options.context and otherwise options.webChannelId. I would just keep app-start the same by invoking the constructors and have _createChannel just call the WebChannel constructor. If a factory becomes necessary in the future to manage complexity we can add it. Right now it seems like it makes something simple more complex.

shane-tomlinson · 2014-07-21T17:01:52Z

app/scripts/lib/channels/custom_events.js

+      var data = e.detail;
+
+      if (data.webChannelId === this._webChannelId && data.message) {
+        this._messageCallbacks.forEach(function(callback) {


forEach will blow up old IEs. Do we care?

zaach · 2014-07-21T22:56:58Z

👀 Keeping my eyes on this one.

vladikoff · 2014-07-21T23:32:13Z

@ckarlof, earlier from #1368

…d simplify the Oauth completion logic. * Appease jscs and jshint * Extract all channel creation logic into lib/channels/factory.js * Rename the WebChannel to FirefoxWebChannel * Simplify service-mixin by checking whether a channel has a completeOAuth function. If it does, the channel knows what to do with the OAuth result and can complete the transaction. If the function returns a promise, it is asynchronous. * Add completeOAuth to the FirefoxWeb channel. * Create a 'web' channel that redirects after oauth completion. * in service-mixin.js, Do NOT submit the form from context because context is called for each translation and could cause multiple submissions. Instead call submit from afterRender. * rename fx-desktop.js -> fx_desktop.js * rename web.js-> fx_web.js

shane-tomlinson · 2014-07-22T09:56:18Z

app/scripts/lib/channels/fx_web.js

+        command: 'oauth_complete',
+        state: state,
+        code: code,
+        closeWindow: source === 'signin'


It seems a bit strange to order the receiver to close the window. Does it make more sense to send source to the receiver and let them decide what to do?

shane-tomlinson · 2014-07-22T09:57:01Z

app/scripts/lib/channels/web.js


-define(function () {
  function WebChannel() {


Maybe rename this to URLChannel?

Sorry I don't think that will work because the browser api is called "WebChannel", this would probably be confusing. We can go with FirefoxWebChannel, could be the least confusing. However, what if we want to write some add-ons for Chrome or iOS and use this WebChannel to communicate?

On 22/07/2014 16:35, Vlad Filippov wrote:

In app/scripts/lib/channels/web.js:

-define(function () {
function WebChannel() {

Sorry I don't think that will work because the browser api is called
"WebChannel", this would probably be confusing. We can go with
|FirefoxWebChannel|, could be the least confusing. However, what if we
want to write some add-ons for Chrome or iOS and use this WebChannel to
communicate?

—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/fxa-content-server/pull/1423/files#r15235006.

Looking at old code? The most current has custom_event.js renamed to
fx_web.js with the module named FirefoxWebChannel. web.js is renamed to
null.js and NullChannel.

shane-tomlinson · 2014-07-22T09:57:59Z

app/scripts/views/ready.js

+      return serviceName;
+    },
+
+    _shouldSubmitWithoutInteraction: function () {


Put this into the service-mixin

* Add `completeOAuthError` to the relevant channels. * Add a `URLChannel` which passes OAuth information over the URL. * Attempt to simplify how the spinner is presented in the service-mixin.

shane-tomlinson · 2014-07-22T15:48:12Z

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

+      if (this._oAuthParams.webChannelId) {
+        channel = new FxWebChannel();
+        channel.init({
+          webChannelId: this._oAuthParams.webChannelId


@vladikoff - does the webChannelId come from the URL search parameters?

Yeap. It is also saved into _oAuthParams because we want to be able to finish the OAuth flow in a new tab (same browser)

shane-tomlinson · 2014-07-22T15:58:55Z

app/scripts/lib/channels/url.js

+  function UrlChannel() {
+  }
+
+  _.extend(UrlChannel.prototype, Backbone.Events, {


This doesn't really need Backbone.Events.

shane-tomlinson · 2014-07-23T11:46:28Z

Closing this in favor of a slimmed down version in #1437

shane-tomlinson changed the title ~~Oauth msg 3~~ WIP: Add OAuth->browser plumbing. Jul 21, 2014