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

remoting/protocol/negotiating_authenticator.cc

 // 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.
 
 #include "remoting/protocol/negotiating_authenticator.h"
 
 #include <algorithm>
 #include <sstream>
 
 #include "base/bind.h"
@@ skipping 12 matching lines @@
 
 const buzz::StaticQName kMethodAttributeQName = { "", "method" };
 const buzz::StaticQName kSupportedMethodsAttributeQName =
     { "", "supported-methods" };
 
 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,
+    const FetchPinCallback& fetch_pin_callback,
     const std::vector<AuthenticationMethod>& methods) {
   scoped_ptr<NegotiatingAuthenticator> result(
       new NegotiatingAuthenticator(MESSAGE_READY));
   result->authentication_tag_ = authentication_tag;
-  result->shared_secret_ = shared_secret;
+  result->fetch_pin_callback_ = fetch_pin_callback;
 
   DCHECK(!methods.empty());
   for (std::vector<AuthenticationMethod>::const_iterator it = methods.begin();
        it != methods.end(); ++it) {
     result->AddMethod(*it);
   }
 
   return scoped_ptr<Authenticator>(result.Pass());
 }
 
@@ skipping 11 matching lines @@
 
   result->AddMethod(AuthenticationMethod::Spake2(hash_function));
 
   return scoped_ptr<Authenticator>(result.Pass());
 }
 
 NegotiatingAuthenticator::NegotiatingAuthenticator(
     Authenticator::State initial_state)
     : current_method_(AuthenticationMethod::Invalid()),
       state_(initial_state),
-      rejection_reason_(INVALID_CREDENTIALS) {
+      rejection_reason_(INVALID_CREDENTIALS),
+      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 NegotiatingAuthenticator::~NegotiatingAuthenticator() {
 }
 
 Authenticator::State NegotiatingAuthenticator::state() const {
   return state_;
 }
 
 Authenticator::RejectionReason
@@ skipping 49 matching lines @@
     if (!method.is_valid()) {
       // Failed to find a common auth method.
       state_ = REJECTED;
       rejection_reason_ = PROTOCOL_ERROR;
       resume_callback.Run();
       return;
     }
 
     // Drop the current message because we've chosen a different
     // method.
-    state_ = MESSAGE_READY;
+    current_method_ = method;
+    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);
+    }
   }
+}
 
-  DCHECK(method.is_valid());
-
-  // Replace current authenticator if the method has changed.
-  if (method != current_method_) {
-    current_method_ = method;
-    CreateAuthenticator(state_);
-  }
-  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);
   }
 }
 
 void NegotiatingAuthenticator::UpdateState(
     const base::Closure& resume_callback) {
   // After the underlying authenticator finishes processing the message, the
   // NegotiatingAuthenticator must update its own state before running the
   // |resume_callback| to resume the session negotiation.
   state_ = current_authenticator_->state();
   if (state_ == REJECTED)
     rejection_reason_ = current_authenticator_->rejection_reason();
   resume_callback.Run();
 }
 
 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) {
       if (it != methods_.begin())
         supported_methods << kSupportedMethodsSeparator;
       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();
 }
 
 void NegotiatingAuthenticator::AddMethod(const AuthenticationMethod& method) {
   DCHECK(method.is_valid());
   methods_.push_back(method);
 }
 
 scoped_ptr<ChannelAuthenticator>
 NegotiatingAuthenticator::CreateChannelAuthenticator() const {
   DCHECK_EQ(state(), ACCEPTED);
   return current_authenticator_->CreateChannelAuthenticator();
 }
 
 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);
+    resume_callback.Run();
   } else {
-    current_authenticator_ = V2Authenticator::CreateForClient(
-        AuthenticationMethod::ApplyHashFunction(
-            current_method_.hash_function(),
-            authentication_tag_, shared_secret_), initial_state);
+    fetch_pin_callback_.Run(base::Bind(
+        &NegotiatingAuthenticator::OnPinFetched,
+        weak_factory_.GetWeakPtr(), preferred_initial_state, resume_callback));

[Wez, 2013/03/20 01:36:31]

This is a strange way to arrange things.

Why can't we create the V2Authenticator synchronously and pass it the
PIN-fetcher callback to use?  

[rmsousa, 2013/03/20 04:54:54]

The V2Authenticator is unusable until it has a secret to use. We need to do
something asynchronous somewhere, and only when it finishes can we continue
processing the message.

Our options are:
* Put the asynchronous call inside the authenticator constructor. This is how it
was organized in the previous patchset (see pin_client_authenticator). The
problem with that is that if the pin fetch calls resume_callback synchronously,
resume_callback will run before current_authenticator_ is assigned (aside from
that, doing that kind of work in the constructor is kind of icky).

* Make the constructor return an unusable/uninitialized object, then use a
separate Init() method to start the asynchronous pin fetch.

I started with these approaches, but then I realized that
PinClientAuthenticator's only purpose was to exist in an unusable state until
the pin fetch finished, and the rest of its code after that was done was mostly
proxying stuff to v2authenticator, so I decided to remove all this cruft and
just fetch the pin and directly construct a v2authenticator.

It kinda breaks encapsulation that negotiating_authenticator knows how to fetch
the secret, but this object already knew low-level stuff like how to hash the
secret, as well as all of the v2authenticator-specific parameters, so we might
as well fetch the secret here too.

   }
 }
 
+void NegotiatingAuthenticator::OnPinFetched(
+    Authenticator::State initial_state,
+    const base::Closure& resume_callback,
+    const std::string& shared_secret) {
+  current_authenticator_ = V2Authenticator::CreateForClient(
+      AuthenticationMethod::ApplyHashFunction(
+          current_method_.hash_function(), authentication_tag_, shared_secret),
+      initial_state);
+  resume_callback.Run();

[Wez, 2013/03/20 01:36:31]

Why does this need a |resume_callback|?

[rmsousa, 2013/03/20 04:54:54]

All of this is happening while the negotiatingauthenticator is processing a
message. The full sequence is:

1) JingleSession calls ProcessMessage with the message received from the peer,
and a callback to be called when processing is done.
2) NegotiatingAuthenticator selects the method specified in the message, and
starts creating the appropriate authenticator.
(... async steps ...)
3) Once the authenticator is ready, it finally uses it to process the message
received from the host.
(... possibly more async steps ...)
4) once underlying message processing is finished, this object's state is
updated to reflect the underlying state.
5) We call the original resume_callback received from the JingleSession
ProcessMessage call.

The resume_callback here encapsulates steps 3, 4, 5. (or 4, 5, depending on
which state the authenticator was created on)

+}
+
 }  // namespace protocol
 }  // namespace remoting