Webapp changes to support asking for a PIN

Webapp changes to support asking for a PIN

This implements a fetchPin interface, that can be called when the plugin layer needs a PIN, and calls onPinFetched with the PIN once it's obtained from the user.

With old plugins, that don't support the useAsyncPinDialog API, this runs the dialog immediately before connecting, and then connects using the existing APIs.

Finally, for It2Me nothing is changed, the support code is used as the passphrase as usual.

BUG=115899
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190207

[rmsousa, 2013-03-16 01:05:56]

See https://codereview.chromium.org/12518027/ for the corresponding Protocol
changes

This makes the client ask for the PIN on demand.

* We need to create the plugin to check if it supports asking for the PIN. If it
doesn't, we ask for the PIN immediately, and connect with the response. If it
does, we just connect, and respond to pin requests as they come.

* We can't constrain the focus to the plugin element until after we're connected
(otherwise the user can't type the PIN).

* Nothing changes for It2Me - we send the support code as a passPhrase on
connect.

[Sergey Ulanov, 2013-03-17 20:45:50]

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_plugin.js
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_plugin...
remoting/webapp/client_plugin.js:62: USE_ASYNC_PIN_DIALOG: 'useAsyncPinDialog',
Why do these need to be two separate features? Would we ever need one without
the other? Feature names do not need to match method names. I think you could
have just one feature (e.g. called asyncPin)

[Jamie, 2013-03-18 20:28:23]

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_plugin.js
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_plugin...
remoting/webapp/client_plugin.js:62: USE_ASYNC_PIN_DIALOG: 'useAsyncPinDialog',
On 2013/03/17 20:45:50, sergeyu wrote:
> Why do these need to be two separate features? Would we ever need one without
> the other? Feature names do not need to match method names. I think you could
> have just one feature (e.g. called asyncPin) 

+1

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_plugin...
remoting/webapp/client_plugin.js:186: function(pin) {};
Nit: No need to wrap this line.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:30: * @param {string} passPhrase The
pre-determined pass phrase for connection.
I think it's worth keeping the style of the original comment here. In other
words, explain what this parameter is for IT2Me and for Me2Me.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:32: *     when a PIN needs to be obtained
interactively.
I think we should be consistent with referring to PIN vs. pass phrase.
Reintroducing the term PIN in the previous parameter description is probably
sufficient

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:42: fetchPin,
Nit: I'm not sure what the intended line-break scheme was here. Either
one-per-line or only as needed would be fine, but there's no need for fetchPin
to have a line to itself (hostJid and clientJid are sufficiently closely-related
that they can stay as they are if you prefer).

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:50: this.fetchPin_ = fetchPin;
Please add @private annotations to these members.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:301: * Constrains the focus to the plugin
element.
Add @private

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:599: /** @type remoting.ClientPlugin */
Brackets around the type, please.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:622:
remoting.wcsSandbox.setOnIq(onIncomingIq);
Blank line here, please.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:623: // Pass phrase was already supplied
before connecting (It2Me case).
Can you move this comment inside the if, for consistency with the else clauses?
Also consider blank lines before the elses for readability.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:646: * @private
Nit: We're pretty consistent about @private being at the end of the annotations.

[rmsousa, 2013-03-18 21:58:00]

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_plugin.js
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_plugin...
remoting/webapp/client_plugin.js:62: USE_ASYNC_PIN_DIALOG: 'useAsyncPinDialog',
On 2013/03/18 20:28:23, Jamie wrote:
> On 2013/03/17 20:45:50, sergeyu wrote:
> > Why do these need to be two separate features? Would we ever need one
without
> > the other? Feature names do not need to match method names. I think you
could
> > have just one feature (e.g. called asyncPin) 
> 
> +1

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_plugin...
remoting/webapp/client_plugin.js:186: function(pin) {};
On 2013/03/18 20:28:23, Jamie wrote:
> Nit: No need to wrap this line.

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:30: * @param {string} passPhrase The
pre-determined pass phrase for connection.
On 2013/03/18 20:28:23, Jamie wrote:
> I think it's worth keeping the style of the original comment here. In other
> words, explain what this parameter is for IT2Me and for Me2Me.

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:32: *     when a PIN needs to be obtained
interactively.
On 2013/03/18 20:28:23, Jamie wrote:
> I think we should be consistent with referring to PIN vs. pass phrase.
> Reintroducing the term PIN in the previous parameter description is probably
> sufficient

I tried to differentiate between a user-generated PIN (Me2Me), and a non-user
pass phrase (It2Me). ut since the latter is only used for IT2Me anyway, we might
as well call it accessCode explicitly.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:42: fetchPin,
On 2013/03/18 20:28:23, Jamie wrote:
> Nit: I'm not sure what the intended line-break scheme was here. Either
> one-per-line or only as needed would be fine, but there's no need for fetchPin
> to have a line to itself (hostJid and clientJid are sufficiently
closely-related
> that they can stay as they are if you prefer).

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:50: this.fetchPin_ = fetchPin;
On 2013/03/18 20:28:23, Jamie wrote:
> Please add @private annotations to these members.

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:301: * Constrains the focus to the plugin
element.
On 2013/03/18 20:28:23, Jamie wrote:
> Add @private

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:599: /** @type remoting.ClientPlugin */
On 2013/03/18 20:28:23, Jamie wrote:
> Brackets around the type, please.

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:622:
remoting.wcsSandbox.setOnIq(onIncomingIq);
On 2013/03/18 20:28:23, Jamie wrote:
> Blank line here, please.

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:623: // Pass phrase was already supplied
before connecting (It2Me case).
On 2013/03/18 20:28:23, Jamie wrote:
> Can you move this comment inside the if, for consistency with the else
clauses?
> Also consider blank lines before the elses for readability.

Done.

https://codereview.chromium.org/12867004/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:646: * @private
On 2013/03/18 20:28:23, Jamie wrote:
> Nit: We're pretty consistent about @private being at the end of the
annotations.

Done.

[Jamie, 2013-03-18 22:31:06]

lgtm

[Sergey Ulanov, 2013-03-20 06:37:28]

LGTM, in case you were waiting for LGTM from me (no need to wait for it next
time - I had just one drive-by comment and you addressed it ( ͡° ͜ʖ ͡°) )

[rmsousa, 2013-03-20 20:52:31]

On 2013/03/20 06:37:28, sergeyu wrote:
> LGTM, in case you were waiting for LGTM from me (no need to wait for it next
> time - I had just one drive-by comment and you addressed it ( ͡° ͜ʖ ͡°) )
Thanks, I was just waiting for the corresponding protocol changes to land, just
in case they required some changes in this layer.

[commit-bot: I haz the power, 2013-03-20 21:03:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12867004/7001

[commit-bot: I haz the power, 2013-03-20 21:03:24]

Failed to apply patch for remoting/webapp/client_session.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file remoting/webapp/client_session.js
  Hunk #2 FAILED at 37.
  Hunk #3 succeeded at 295 (offset 4 lines).
  Hunk #4 succeeded at 599 (offset 4 lines).
  Hunk #5 succeeded at 621 (offset 4 lines).
  Hunk #6 succeeded at 670 (offset 4 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
remoting/webapp/client_session.js.rej

Patch:       remoting/webapp/client_session.js
Index: remoting/webapp/client_session.js
diff --git a/remoting/webapp/client_session.js
b/remoting/webapp/client_session.js
index
89fcc82eb70c599e38ebb407f7156e06890978b4..0cea8c863b3110d95a23a8c8203dc48d4b16bd03
100644
--- a/remoting/webapp/client_session.js
+++ b/remoting/webapp/client_session.js
@@ -27,8 +27,9 @@ var remoting = remoting || {};
  * @param {string} clientJid The jid of the WCS client.
  * @param {string} hostPublicKey The base64 encoded version of the host's
  *     public key.
- * @param {string} sharedSecret The access code for IT2Me or the PIN
- *     for Me2Me.
+ * @param {string} accessCode The IT2Me access code. Blank for Me2Me.
+ * @param {function(function(string): void): void} fetchPin Called by Me2Me
+ *     connections when a PIN needs to be obtained interactively.
  * @param {string} authenticationMethods Comma-separated list of
  *     authentication methods the client should attempt to use.
  * @param {string} hostId The host identifier for Me2Me, or empty for IT2Me.
@@ -36,15 +37,18 @@ var remoting = remoting || {};
  * @param {remoting.ClientSession.Mode} mode The mode of this connection.
  * @constructor
  */
-remoting.ClientSession = function(hostJid, clientJid,
-                                  hostPublicKey, sharedSecret,
-                                  authenticationMethods, hostId, mode) {
+remoting.ClientSession = function(hostJid, clientJid, hostPublicKey,
accessCode,
+                                  fetchPin, authenticationMethods, hostId,
+                                  mode) {
   this.state = remoting.ClientSession.State.CREATED;
 
   this.hostJid = hostJid;
   this.clientJid = clientJid;
   this.hostPublicKey = hostPublicKey;
-  this.sharedSecret = sharedSecret;
+  /** @private */
+  this.accessCode_ = accessCode;
+  /** @private */
+  this.fetchPin_ = fetchPin;
   this.authenticationMethods = authenticationMethods;
   this.hostId = hostId;
   /** @type {remoting.ClientSession.Mode} */
@@ -290,14 +294,20 @@ remoting.ClientSession.prototype.onHostSettingsLoaded_ =
function(options) {
         options[remoting.ClientSession.KEY_SHRINK_TO_FIT];
   }
 
-  this.plugin.element().focus();
-
   /** @param {boolean} result */
   this.plugin.initialize(this.onPluginInitialized_.bind(this));
+};
+
+/**
+ * Constrains the focus to the plugin element.
+ * @private
+ */
+remoting.ClientSession.prototype.setFocusHandlers_ = function() {
   this.plugin.element().addEventListener(
       'focus', this.callPluginGotFocus_, false);
   this.plugin.element().addEventListener(
       'blur', this.callPluginLostFocus_, false);
+  this.plugin.element().focus();
 };
 
 /**
@@ -588,6 +598,7 @@ remoting.ClientSession.prototype.sendIq_ = function(msg) {
  */
 remoting.ClientSession.prototype.connectPluginToWcs_ = function() {
   remoting.formatIq.setJids(this.clientJid, this.hostJid);
+  /** @type {remoting.ClientPlugin} */
   var plugin = this.plugin;
   var forwardIq = plugin.onIncomingIq.bind(plugin);
   /** @param {string} stanza The IQ stanza received. */
@@ -609,10 +620,41 @@ remoting.ClientSession.prototype.connectPluginToWcs_ =
function() {
     console.log(remoting.timestamp(),
                 remoting.formatIq.prettifyReceiveIq(stanza));
     forwardIq(stanza);
-  }
+  };
   remoting.wcsSandbox.setOnIq(onIncomingIq);
+
+  if (this.accessCode_) {
+    // Shared secret was already supplied before connecting (It2Me case).
+    this.connectToHost_(this.accessCode_);
+
+  } else if (plugin.hasFeature(
+      remoting.ClientPlugin.Feature.ASYNC_PIN)) {
+    // Plugin supports asynchronously asking for the PIN.
+    plugin.useAsyncPinDialog();
+    /** @type remoting.ClientSession */
+    var that = this;
+    var fetchPin = function() {
+      that.fetchPin_(plugin.onPinFetched.bind(plugin));
+    };
+    plugin.fetchPinHandler = fetchPin;
+    this.connectToHost_('');
+
+  } else {
+    // Plugin doesn't support asynchronously asking for the PIN, ask now.
+    this.fetchPin_(this.connectToHost_.bind(this));
+  }
+};
+
+/**
+ * Connects to the host.
+ *
+ * @param {string} sharedSecret Shared secret for SPAKE negotiation.
+ * @return {void} Nothing.
+ * @private
+ */
+remoting.ClientSession.prototype.connectToHost_ = function(sharedSecret) {
   this.plugin.connect(this.hostJid, this.hostPublicKey, this.clientJid,
-                      this.sharedSecret, this.authenticationMethods,
+                      sharedSecret, this.authenticationMethods,
                       this.hostId);
 };
 
@@ -627,6 +669,7 @@ remoting.ClientSession.prototype.connectPluginToWcs_ =
function() {
 remoting.ClientSession.prototype.onConnectionStatusUpdate_ =
     function(status, error) {
   if (status == remoting.ClientSession.State.CONNECTED) {
+    this.setFocusHandlers_();
     this.onDesktopSizeChanged_();
     if (this.resizeToClient_) {
       this.plugin.notifyClientResolution(window.innerWidth,

[commit-bot: I haz the power, 2013-03-20 23:01:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12867004/19001

[commit-bot: I haz the power, 2013-03-21 00:01:25]

Retried try job too often on linux_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-21 20:28:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12867004/19001

[commit-bot: I haz the power, 2013-03-22 18:58:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12867004/19001

[commit-bot: I haz the power, 2013-03-23 15:40:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12867004/19001

[commit-bot: I haz the power, 2013-03-24 05:26:05]

Change committed as 190207