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 FetchSecretCallback& fetch_secret_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_secret_callback_ = fetch_secret_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 {

[Sergey Ulanov, 2013/03/20 05:49:05]

nit: add return above and then you won't need "else"

Similarly you can avoid the else case below.

[rmsousa, 2013/03/20 20:17:16]

Done.

+    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(

[Sergey Ulanov, 2013/03/20 05:49:05]

Move this below so order of functions here matches order in .h file.

[rmsousa, 2013/03/20 20:17:16]

Done.

+    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);

[Sergey Ulanov, 2013/03/20 05:49:05]

here state can be either WAITING_MESSAGE or ACCEPTED. It's better to use
positive filter here.

[rmsousa, 2013/03/20 20:17:16]

Done.

+    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) {
+

[Sergey Ulanov, 2013/03/20 05:49:05]

nit: remove empty line

[rmsousa, 2013/03/20 20:17:16]

Done.

+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();

[Sergey Ulanov, 2013/03/20 05:49:05]

Do you need to set state_ here to something other than PROCESSING_MESSAGE?
JingleSession::ProcessAuthenticationStep() has a DCHECK for it. I wonder why
unittests didn't catch it.

[rmsousa, 2013/03/20 20:17:16]

The callbacks passed in here all call UpdateState (either directly or through
ProcessMessageInternal). It's only at that time (*after* all async processing is
done) that it's safe to change the state from PROCESSING_MESSAGE

I don't think we have unittests for the Session -> NegotiatingAuthenticator
integration. NegotiatingAuthenticator tests run the flow manually
(AuthenticatorTestBase), and Session tests use FakeAuthenticator

   } else {
-    current_authenticator_ = V2Authenticator::CreateForClient(
-        AuthenticationMethod::ApplyHashFunction(
-            current_method_.hash_function(),
-            authentication_tag_, shared_secret_), initial_state);
+    fetch_secret_callback_.Run(base::Bind(
+        &NegotiatingAuthenticator::CreateV2AuthenticatorWithSecret,
+        weak_factory_.GetWeakPtr(), preferred_initial_state, resume_callback));
   }
 }
 
+void NegotiatingAuthenticator::CreateV2AuthenticatorWithSecret(
+    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();

[Sergey Ulanov, 2013/03/20 05:49:05]

Set state_ to MESSAGE_READY?

[rmsousa, 2013/03/20 20:17:16]

Same applies, there's an UpdateState in the callback

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