Protocol / client plugin changes to support interactively asking for a PIN

remoting/protocol/negotiating_authenticator.cc

Index: remoting/protocol/negotiating_authenticator.cc
diff --git a/remoting/protocol/negotiating_authenticator.cc b/remoting/protocol/negotiating_authenticator.cc
index 7b794d2a05f69a1f8de26ea3dc64b6d80b4773fe..e2f639a0e2473f444075f253e2c02a86551ddb0b 100644
--- a/remoting/protocol/negotiating_authenticator.cc
+++ b/remoting/protocol/negotiating_authenticator.cc
@@ -14,6 +14,8 @@
 #include "remoting/base/rsa_key_pair.h"
 #include "remoting/protocol/channel_authenticator.h"
 #include "remoting/protocol/v2_authenticator.h"
+#include "remoting/protocol/pin_client_authenticator.h"
+#include "remoting/protocol/pin_fetcher_factory.h"
 #include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
 
 namespace remoting {
@@ -30,20 +32,16 @@ const char kSupportedMethodsSeparator = ',';
 }  // namespace
 
 // static
-bool NegotiatingAuthenticator::IsNegotiableMessage(
-    const buzz::XmlElement* message) {
-  return message->HasAttr(kSupportedMethodsAttributeQName);
-}
-
-// static
 scoped_ptr<Authenticator> NegotiatingAuthenticator::CreateForClient(
     const std::string& authentication_tag,
     const std::string& shared_secret,
+    PinFetcherFactory* pin_fetcher_factory,
     const std::vector<AuthenticationMethod>& methods) {
   scoped_ptr<NegotiatingAuthenticator> result(
       new NegotiatingAuthenticator(MESSAGE_READY));
   result->authentication_tag_ = authentication_tag;
   result->shared_secret_ = shared_secret;
+  result->pin_fetcher_factory_ = pin_fetcher_factory;
 
   DCHECK(!methods.empty());
   for (std::vector<AuthenticationMethod>::const_iterator it = methods.begin();
@@ -145,23 +143,36 @@ void NegotiatingAuthenticator::ProcessMessage(
 
     // Drop the current message because we've chosen a different
     // method.
-    state_ = MESSAGE_READY;
-  }
-
-  DCHECK(method.is_valid());
-
-  // Replace current authenticator if the method has changed.
-  if (method != current_method_) {
     current_method_ = method;
-    CreateAuthenticator(state_);
+    state_ = PROCESSING_MESSAGE;
+    CreateAuthenticator(MESSAGE_READY, base::Bind(
+        &NegotiatingAuthenticator::UpdateState,
+        base::Unretained(this), resume_callback));
+  } else {
+    if (method != current_method_) {
+      current_method_ = method;
+      state_ = PROCESSING_MESSAGE;
+      // Copy the message since the authenticator may process it asynchronously.
+      CreateAuthenticator(WAITING_MESSAGE, base::Bind(
+          &NegotiatingAuthenticator::ProcessMessageInternal,
+          base::Unretained(this), base::Owned(new buzz::XmlElement(*message)),
+          resume_callback));
+    } else {
+      ProcessMessageInternal(message, resume_callback);
+    }
   }
-  if (state_ == WAITING_MESSAGE) {
+}
+
+void NegotiatingAuthenticator::ProcessMessageInternal(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback) {
+  if (current_authenticator_->state() == WAITING_MESSAGE) {
+    // If the message was not discarded and the authenticator is waiting for it,
+    // give it to the underlying authenticator to process.
     // |current_authenticator_| is owned, so Unretained() is safe here.
     current_authenticator_->ProcessMessage(message, base::Bind(
         &NegotiatingAuthenticator::UpdateState,
         base::Unretained(this), resume_callback));
-  } else {
-    UpdateState(resume_callback);
   }
 }
 
@@ -179,27 +190,12 @@ void NegotiatingAuthenticator::UpdateState(
 scoped_ptr<buzz::XmlElement> NegotiatingAuthenticator::GetNextMessage() {
   DCHECK_EQ(state(), MESSAGE_READY);
 
-  bool add_supported_methods_attr = false;
+  scoped_ptr<buzz::XmlElement> result;
 
-  // Initialize current method in case it is not initialized
-  // yet. Normally happens only on client.
+  // No method yet, just send a message with the list of supported methods.
+  // Normally happens only on client.
   if (!current_method_.is_valid()) {
-    CHECK(!methods_.empty());
-
-    // Initially try the first method.
-    current_method_ = methods_[0];
-    CreateAuthenticator(MESSAGE_READY);
-    add_supported_methods_attr = true;
-  }
-
-  scoped_ptr<buzz::XmlElement> result =
-      current_authenticator_->GetNextMessage();
-  state_ = current_authenticator_->state();
-  DCHECK_NE(state_, REJECTED);
-
-  result->AddAttr(kMethodAttributeQName, current_method_.ToString());
-
-  if (add_supported_methods_attr) {
+    result = CreateEmptyAuthenticatorMessage();
     std::stringstream supported_methods(std::stringstream::out);
     for (std::vector<AuthenticationMethod>::iterator it = methods_.begin();
          it != methods_.end(); ++it) {
@@ -208,6 +204,17 @@ scoped_ptr<buzz::XmlElement> NegotiatingAuthenticator::GetNextMessage() {
       supported_methods << it->ToString();
     }
     result->AddAttr(kSupportedMethodsAttributeQName, supported_methods.str());
+    state_ = WAITING_MESSAGE;
+  } else {
+    if (current_authenticator_->state() == MESSAGE_READY) {
+      result = current_authenticator_->GetNextMessage();
+    } else {
+      result = CreateEmptyAuthenticatorMessage();
+    }
+    state_ = current_authenticator_->state();
+    DCHECK_NE(state_, REJECTED);
+    DCHECK_NE(state_, MESSAGE_READY);
+    result->AddAttr(kMethodAttributeQName, current_method_.ToString());
   }
 
   return result.Pass();
@@ -228,16 +235,31 @@ bool NegotiatingAuthenticator::is_host_side() const {
   return local_key_pair_.get() != NULL;
 }
 
-void NegotiatingAuthenticator::CreateAuthenticator(State initial_state) {
+
+void NegotiatingAuthenticator::CreateAuthenticator(
+    Authenticator::State preferred_initial_state,
+    const base::Closure& resume_callback) {
   if (is_host_side()) {
     current_authenticator_ = V2Authenticator::CreateForHost(
-        local_cert_, local_key_pair_, shared_secret_hash_, initial_state);
+        local_cert_, local_key_pair_, shared_secret_hash_,
+        preferred_initial_state);
   } else {
-    current_authenticator_ = V2Authenticator::CreateForClient(
-        AuthenticationMethod::ApplyHashFunction(
-            current_method_.hash_function(),
-            authentication_tag_, shared_secret_), initial_state);
+    if (pin_fetcher_factory_) {

[Sergey Ulanov, 2013/03/17 21:29:21]

Do we really need to distinguish between these two cases here? Can
ChromotingInstance just always pass the PIN fetcher - it would just implement it
differently depending on how the PIN is fetched.

[rmsousa, 2013/03/18 21:07:26]

I think that would get ugly in the upper layers, when we have to deal with older
clients. (But I'm open to doing it that way)

An older client (or It2Me) would just call connect() with the sharedSecret in
the parameters. To make a separate pin_fetcher_ implementation we'd need to, in
the plugin connect() call, check that asyncPinFetching is not enabled, and if
it's not, save the sharedSecret, so that a later FetchPin would return it.
ChromotingInstance has no "connection" object, so that data would live
"globally" in the ChromotingInstance object. But then we need to clean up that
data at the right times, so that we don't accidentally use it.

Finally, Pinfetcher is kind of a transition object. It may eventually evolve to
something that allows you to pair a host (and name that pairing), etc, etc. None
of that will make sense for IT2Me. I feel it's cleaner to handle IT2Me like it
really is - a mode where the passphrase is given at connect time, and that's it.

[Sergey Ulanov, 2013/03/19 02:44:53]

I think this is just a question of whether we should put the logic of handling
differences between different clients. We could either put it here or in the
plugin layer. I think it belongs to the plugin. It will be possible to remove it
later once all clients support the feature. Putting that logic here makes it
harder to do. In general any nullable parameters add complexity (at least when
reading the code) and it's better to avoid when possible.

[rmsousa, 2013/03/19 23:14:31]

Done.

+      // No pre-entered secret, need to use the interactive authenticator.
+      current_authenticator_.reset(new PinClientAuthenticator(
+          current_method_.hash_function(), authentication_tag_,
+          pin_fetcher_factory_->CreatePinFetcher(), preferred_initial_state,

[Sergey Ulanov, 2013/03/17 21:29:21]

Why do we need the factory interface? Is there any reason you would need
multiple fetchers? All fetchers are the same, so you don't need more than one
instance, hence no need to have the factory. Am I missing something?

[rmsousa, 2013/03/18 21:07:26]

It's to manage the lifetime for the callback appropriately - If PinFetcher is a
shared instance pointer, I would need to make sure that it won't call the
callback after the authenticator is deleted (e.g. by making
PinClientAuthenticator a weakptr, or adding an explicit Cancel call). I also
need to make sure that if we ever switch between two PinClientAuthenticators
instances, there's no garbage left in the PinFetcher between the two. Putting
all* state in a scoped_ptr<PinFetcher> makes this simpler - it's owned by
PinClientAuthenticator, so everything will be cleaned up if it's destroyed.

* Not exactly all state - there's still an assumption in ChromotingInstance that
there's only one live PinFetcher, but if we needed to change that it's just a
matter of assinging PinFetcher ids, and changign from a pointer to a map.

[Sergey Ulanov, 2013/03/19 02:44:53]

The issue with possible lifetime mismatch is easy to solve. You could just use
ref-counted callback target or weak pointers. 

[rmsousa, 2013/03/19 23:14:31]

Done.

+          resume_callback));
+      return;
+    } else {
+      // Pre-entered secret (legacy client), just proceed with V2 authenticator.
+      current_authenticator_ = V2Authenticator::CreateForClient(
+          AuthenticationMethod::ApplyHashFunction(
+              current_method_.hash_function(),
+              authentication_tag_, shared_secret_), preferred_initial_state);

[Sergey Ulanov, 2013/03/17 21:29:21]

nit: move the last parameter to a separate line for readability.

[rmsousa, 2013/03/18 21:07:26]

Done.

+    }
   }
+  resume_callback.Run();
 }
 
 }  // namespace protocol