Webapp changes to support third party authentication

remoting/webapp/third_party_auth.js

Index: remoting/webapp/third_party_auth.js
diff --git a/remoting/webapp/third_party_auth.js b/remoting/webapp/third_party_auth.js
new file mode 100644
index 0000000000000000000000000000000000000000..0d9d3e5dd9f6814fc80f3e2ac6b28642ec263cb4
--- /dev/null
+++ b/remoting/webapp/third_party_auth.js
@@ -0,0 +1,241 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview
+ * Third party authentication support for the remoting web-app.
+ *
+ * When third party authentication is being used, the client must request both a
+ * token and a shared secret from a third-party server (which may require the
+ * user to authenticate themselves). The client then sends only the token to the

[Jamie, 2013/03/28 23:29:36]

Can you clarify the parenthetical remark?

[rmsousa, 2013/03/29 04:08:21]

Done.

+ * host. The host signs the token, then contacts the third-party server to
+ * exchange the token for the shared secret. Once both client and host have the
+ * shared secret, they use a zero-disclosure mutual authentication protocol to
+ * negotiate an authentication key, which is used to establish the connection.
+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/**
+ * @constructor
+ * Encapsulates the logic to fetch a third party authentication token.
+ *
+ * @param {string} tokenUrl Token issue URL received from the host.

[Jamie, 2013/03/28 23:29:36]

s/Token issue/Token-issue/

[rmsousa, 2013/03/29 04:08:21]

Done.

+ * @param {string} hostPublicKey Host public key (DER and Base64 encoded).
+ * @param {string} scope OAuth scope to request the token for.
+ * @param {function(string, string):void} onThirdPartyTokenFetched Callback.
+ */
+remoting.ThirdPartyTokenFetcher = function(
+    tokenUrl, hostPublicKey, scope, onThirdPartyTokenFetched) {
+  // TODO(rmsousa): Validate the URL against a pattern obtained from the server.
+  this.tokenUrl_ = tokenUrl;
+  this.tokenScope_ = scope;
+  this.onThirdPartyTokenFetched_ = onThirdPartyTokenFetched;
+  this.xsrfToken_ = remoting.generateXsrfToken();
+  this.clientId_ = hostPublicKey;

[Jamie, 2013/03/28 23:29:36]

Could the member variable and the parameter have the same name?

[rmsousa, 2013/03/29 04:08:21]

Done.

[rmsousa, 2013/03/29 04:08:21]

Yeah, I had kinda mixed OAuth param names with actual contents here. Changed it
so that all member variables are named for the actual contents.

+  if (chrome.experimental && chrome.experimental.identity) {
+    this.fetchTokenInternal_ = this.fetchTokenIdentityApi_;
+    this.redirectUri_ = 'https://' + window.location.hostname +
+        '.chromiumapp.org/ThirdPartyAuth';

[Jamie, 2013/03/28 23:29:36]

Why chromiumapp.org? It's not clear from the docs, but I think that might just
be an example. I would expect us to use THIRD_PARTY_AUTH_REDIRECT_URI here too.

[rmsousa, 2013/03/29 04:08:21]

It's not just an example. The identity API captures a redirect to
https://<extension-id>.chromiumapp.org, and gives the resulting URL to the app
with <extension-id> to parse. See
http://developer.chrome.com/apps/app_identity.html#non and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

+  } else {
+    this.fetchTokenInternal_ = this.fetchTokenWindowOpen_;
+    this.redirectUri_ = remoting.settings.THIRD_PARTY_AUTH_REDIRECT_URI;
+  }
+};
+
+/**
+ * Fetch a token with the parameters configured in this object.
+ */
+remoting.ThirdPartyTokenFetcher.prototype.fetchToken = function() {
+  (new remoting.ThirdPartyHostPermissions(this.tokenUrl_)).getPermission(

[Jamie, 2013/03/28 23:29:36]

Please add a variable to store the result of 'new', for readability.

[rmsousa, 2013/03/29 04:08:21]

Done.

+    this.fetchTokenInternal_.bind(this),

[Jamie, 2013/03/28 23:29:36]

No need to bind a callback (see below for why it doesn't need to be a member
function).

[rmsousa, 2013/03/29 04:08:21]

The implementations reference |this|, so they need to be bound.

[Jamie, 2013/03/29 18:19:24]

Okay, I see now. It's not clear at this point of the code that
fetchTokenInternal_ is actually a member function masquerading as a callback
pointer. I think it would be clearer to do the bind at the point where you're
setting the member variable, rather than here. It would be a better match with
how bind is used elsewhere (and with the semantics of base::Bind in C++).

[rmsousa, 2013/03/29 20:55:48]

Agreed.

+    this.onThirdPartyTokenFetched_.bind(this, '', ''));

[Jamie, 2013/03/28 23:29:36]

No need for bind here either.

[rmsousa, 2013/03/29 04:08:21]

I need to bind the extra parameters (the empty strings).

[Jamie, 2013/03/29 18:19:24]

I think it would be clearer to have a separate member variable for the failure
case and bind it at the point of assignment.

[rmsousa, 2013/03/29 20:55:48]

Done.
Since I was cleaning this up, I decided to use function() {
onThirdPartyTokenFetched_('','') } rather than bind, to avoid passing "this" as
a bind parameter to a callback.

+};
+
+/**
+ * Fetch a token with the parameters configured in this object.
+ * @private
+ */
+remoting.ThirdPartyTokenFetcher.prototype.fetchTokenInternal_ = function() {};

[Jamie, 2013/03/28 23:29:36]

You seem to be usig fetchTokenInternal_ both as a member variable in the ctor
and as a member function. I think the former is what you want, so you don't need
this declaration.

[rmsousa, 2013/03/29 04:08:21]

Done.

+
+/**
+ * Parse the access token from the URL to which we were redirected.
+ *
+ * @param {string} responseUrl The URL to which we were redirected.
+ * @private
+ */
+remoting.ThirdPartyTokenFetcher.prototype.parseRedirectUrl_ =
+    function(responseUrl) {
+  var code = '';
+  var sharedSecret = '';
+  if (responseUrl &&
+      responseUrl.substring(0, this.redirectUri_.length + 1) ==
+        (this.redirectUri_ + '#')) {

[Jamie, 2013/03/28 23:29:36]

Would this be clearer as responseUrl.search(this.redirectUri_ + '#') == 0?

[rmsousa, 2013/03/29 04:08:21]

Done.

+    var query = responseUrl.substring(this.redirectUri_.length + 1);
+    var parts = query.split('&');
+    var queryArgs = {};

[Jamie, 2013/03/28 23:29:36]

Can you explicitly declare this as type Object.<String> to avoid the type
coercion below?

[rmsousa, 2013/03/29 04:08:21]

Done.

+    for (var i = 0; i < parts.length; i++) {
+      var pair = parts[i].split('=');
+      queryArgs[pair[0]] = pair[1];
+    }
+    var state = decodeURIComponent(queryArgs['state']);
+    // In the OAuth code/token exchange semantics, 'code' refers to the value
+    // that can be exchanged for an 'access_token'. For our purposes, 'code' is

[Jamie, 2013/03/28 23:29:36]

Is this naming under our control? It might be clearer not to reuse the term
'code'. Perhaps something like 'ticket' might be better?

[rmsousa, 2013/03/29 04:08:21]

The OAuth terms are not under our control. "code" and "token" refer to specific
types of response and semantics in OAuth (a "code" is something the user gets
when it authenticates, and a "token" is what the application must exchange the
"code" for in order to authenticate".

In our case, basically the OAuth "code" semantics is what we desire for what we
call internally our "token", and the OAuth "token" semantics is what we want for
our "sharedSecret". This piece of code traverses between the two naming schemes,
and tries to explain that mapping. I updated the comment to explain the
mapping/semantics, and tried to make at least the naming that is under our
control consistent with our internal usage (even though it results in a variable
ending up with the generic name "token").

+    // the part that is sent to the host, while the 'access_token' is the shared
+    // secret that the host can exchange it for.
+    if (state == this.xsrfToken_ && 'code' in queryArgs) {

[Jamie, 2013/03/28 23:29:36]

'states' and 'tokens' are very different concepts. Why are we comparing them for
equality here? It seems that one or the other has a misleading name.

[rmsousa, 2013/03/29 04:08:21]

Again, OAuth terminology vs. reality. The name "state" is the name of the OAuth
parameter that the client sends to the server and receives verbatim on the
redirect URL. What we (and pretty much everyone who uses OAuth) use that
parameter for is for XSRF prevention, by means of an XSRF token. See
http://tools.ietf.org/html/draft-ietf-oauth-v2-31#section-10.12

We can change the name of the variable to xsrfToken, but there will still be a
"var xsrfToken = decodeURIComponent(queryArgs['state'])" line above.

[Jamie, 2013/03/29 18:19:24]

I think that would be clearer, since that's the line where we're taking
something named by OAuth and storing it in something where we choose the name.

[rmsousa, 2013/03/29 20:55:48]

Done.

+      code = decodeURIComponent(queryArgs['code']);
+      if ('access_token' in queryArgs) {
+        sharedSecret = /** @type string */ queryArgs['access_token'];

[Jamie, 2013/03/28 23:29:36]

{} around the type, if it's needed at all (see above).

[rmsousa, 2013/03/29 04:08:21]

Done.

+      }
+    }
+  }
+  this.onThirdPartyTokenFetched_(code, sharedSecret);
+};
+
+/**
+ * Build a full token request URL from the parameters in this object.
+ *
+ * @return {string} Full URL to request a token.
+ * @private
+ */
+remoting.ThirdPartyTokenFetcher.prototype.getFullTokenUrl_ = function() {
+  return this.tokenUrl_ + '?' + remoting.xhr.urlencodeParamHash({
+    'redirect_uri': this.redirectUri_,
+    'scope': this.tokenScope_,
+    'client_id': this.clientId_,
+    // The webapp uses an "implicit" OAuth flow with multiple response types to
+    // obtain both the code and the shared secret in a single request.
+    'response_type': 'code token',
+    'state': this.xsrfToken_
+  });
+};
+
+/**
+ * Fetch a token by opening a new window and redirecting to a content script.
+ * @private
+ */
+remoting.ThirdPartyTokenFetcher.prototype.fetchTokenWindowOpen_ = function() {
+  /** @type remoting.ThirdPartyTokenFetcher */

[Jamie, 2013/03/28 23:29:36]

{} around types, here and elsewhere.

[rmsousa, 2013/03/29 04:08:21]

Done.

+  var that = this;
+  var fullTokenUrl = this.getFullTokenUrl_();
+  // The function below can't be anonymous, since it needs to reference itself.
+  /** @param {string} message Message received from the content script. */
+  function tokenMessageListener(message) {
+    that.parseRedirectUrl_(message);
+    chrome.extension.onMessage.removeListener(tokenMessageListener);
+  }
+  chrome.extension.onMessage.addListener(tokenMessageListener);
+  window.open(fullTokenUrl);
+};
+
+/**
+ * Fetch a token from a token server using the identity.launchWebAuthFlow API.
+ * @private
+ */
+remoting.ThirdPartyTokenFetcher.prototype.fetchTokenIdentityApi_ = function() {
+  /** @type remoting.ThirdPartyTokenFetcher */
+  var that = this;
+  var fullTokenUrl = this.getFullTokenUrl_();
+  // TODO(rmsousa): chrome.identity.launchWebAuthFlow is experimental.
+  chrome.experimental.identity.launchWebAuthFlow(
+    {'url': fullTokenUrl, 'interactive': true},
+    /** @param {string} responseUrl The full redirection URL. */
+    function(responseUrl) {
+      that.parseRedirectUrl_(responseUrl); });

[Jamie, 2013/03/28 23:29:36]

This would be clearer with bind.

[rmsousa, 2013/03/29 04:08:21]

Done.

+};
+
+/**
+ * @constructor
+ * Encapsulates the UI to check/request permissions to a new host.
+ *
+ * @param {string} url The URL to request permission for.
+ */
+remoting.ThirdPartyHostPermissions = function(url) {
+  this.url_ = url;
+  this.permissions_ = {'origins': [url]};
+};
+
+/**
+ * Get permissions to the URL, asking interactively if necessary.
+ *
+ * @param {function(): void} onOk Called if the permission is granted.
+ * @param {function(): void} onError Called if the permission is denied.
+ */
+remoting.ThirdPartyHostPermissions.prototype.getPermission = function(
+    onOk, onError) {
+  /** @type remoting.ThirdPartyHostPermissions */
+  var that = this;
+  chrome.permissions.contains(this.permissions_,
+    /** @param {boolean} allowed Whether this extension has this permission. */
+    function(allowed) {
+    if (allowed) {

[Jamie, 2013/03/28 23:29:36]

Nit: Indentation.

[rmsousa, 2013/03/29 04:08:21]

Done.

+      onOk();
+    } else {
+      // Optional permissions must be requested in a user action context. This
+      // is called from an asynchronous plugin callback, so we have to open a
+      // confirmation dialog to perform the request on an interactive event.
+      // In any case, we can use this dialog to explain to the user why we are
+      // asking for the additional permission.
+      that.showPermissionConfirmation_(onOk, onError);
+    }
+  });
+};
+
+/**
+ * Show an interactive dialog informing the user of the new permissions.
+ *
+ * @param {function(): void} onOk Called if the permission is granted.
+ * @param {function(): void} onError Called if the permission is denied.
+ * @private
+ */
+remoting.ThirdPartyHostPermissions.prototype.showPermissionConfirmation_ =
+    function(onOk, onError) {
+  /** @type {HTMLElement} */
+  var dialog = document.getElementById('third-party-auth-dialog');
+  /** @type {HTMLElement} */
+  var button = document.getElementById('third-party-auth-button');
+  /** @type {HTMLElement} */
+  var message = document.getElementById('third-party-auth-message');
+  l10n.localizeElement(message, this.url_);
+
+  /** @type remoting.ThirdPartyHostPermissions */
+  var that = this;
+
+  var consentGranted = function(event) {
+    remoting.setMode(remoting.AppMode.CLIENT_CONNECTING);
+    button.removeEventListener('click', consentGranted, false);
+    that.requestPermission_(onOk, onError);
+  };
+
+  button.addEventListener('click', consentGranted, false);
+  remoting.setMode(remoting.AppMode.CLIENT_THIRD_PARTY_AUTH);
+};
+
+
+/**
+ * Request permission from the user to access the token issue URL.
+ *
+ * @param {function(): void} onOk Called if the permission is granted.
+ * @param {function(): void} onError Called if the permission is denied.
+ * @private
+ */
+remoting.ThirdPartyHostPermissions.prototype.requestPermission_ = function(
+    onOk, onError) {
+  /** @type remoting.ThirdPartyHostPermissions */
+  var that = this;

[Jamie, 2013/03/28 23:29:36]

You don't seem to be using this.

[rmsousa, 2013/03/29 04:08:21]

Done.

+  chrome.permissions.request(
+    this.permissions_,
+    /** @param {boolean} result Whether the permission was granted. */
+    function(result) {
+      if (result) {
+        onOk();
+      } else {
+        onError();
+      }
+  });
+};