PairingAuthenticator implementation and plumbing.

remoting/protocol/pairing_authenticator_base.cc

Index: remoting/protocol/pairing_authenticator_base.cc
diff --git a/remoting/protocol/pairing_authenticator_base.cc b/remoting/protocol/pairing_authenticator_base.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d157af0a579853587ce1f4a4e9c237ff9bfbf2ea
--- /dev/null
+++ b/remoting/protocol/pairing_authenticator_base.cc
@@ -0,0 +1,171 @@
+// Copyright 2013 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/pairing_authenticator_base.h"
+
+#include "base/bind.h"
+#include "remoting/base/constants.h"
+#include "remoting/protocol/channel_authenticator.h"
+
+namespace remoting {
+namespace protocol {
+
+const buzz::StaticQName PairingAuthenticatorBase::kPairingInfoTag =
+    { kChromotingXmlNamespace, "pairing-info" };
+const buzz::StaticQName PairingAuthenticatorBase::kClientIdAttribute =
+    { "", "client-id" };
+
+namespace {
+const buzz::StaticQName kPairingFailedTag =

[rmsousa, 2013/05/21 23:17:07]

Nit: I'd make these class members for consistency.

[Jamie, 2013/05/22 00:19:14]

The reason I didn't is that they're an implementation detail that derived
classes don't need to know about it. I could make them private, but I prefer for
them to be hidden entirely.

+    { kChromotingXmlNamespace, "pairing-failed" };
+const buzz::StaticQName kPairingErrorAttribute = { "", "error" };
+}  // namespace
+
+
+PairingAuthenticatorBase::PairingAuthenticatorBase()
+    : using_paired_secret_(false),
+      waiting_for_authenticator_(false),
+      weak_factory_(this) {
+}
+
+Authenticator::State PairingAuthenticatorBase::state() const {
+  if (v2_authenticator_) {
+    return v2_authenticator_->state();
+  } else if (waiting_for_authenticator_) {
+    return PROCESSING_MESSAGE;
+  } else {
+    return WAITING_MESSAGE;

[rmsousa, 2013/05/21 23:17:07]

Nit: can you add a comment explaining why this state is WAITING_MESSAGE?

(also, since it's a host-specific case, it seems like this case should be
handled in PairingHostAuthenticator::state(), and then
PairingAuthenticatorBase::state() could just DCHECK(v2_authenticator_))

[Jamie, 2013/05/22 00:19:14]

Done (without the DCHECK, since operator-> has one already.

+  }
+}
+
+Authenticator::RejectionReason
+PairingAuthenticatorBase::rejection_reason() const {
+  if (!v2_authenticator_) {
+    return PROTOCOL_ERROR;

[rmsousa, 2013/05/21 23:17:07]

rejection_reason() is only queried if state() is REJECTED, so this is
unreachable by the state() definition. You might as well
DCHECK(v2_authenticator_).

(PairingHostAuthenticator overrides state() to make this possible, but it
overrides rejection_reason() as well, so this is still unreachable in that case)

[Jamie, 2013/05/22 00:19:14]

Wez asked me to add this on the basis that it's better to fail gracefully in
case of a coding error than to crash the host. LMK if you feel strongly and I'll
revert it (I don't have a strong opinion either way).

+  }
+  return v2_authenticator_->rejection_reason();
+}
+
+void PairingAuthenticatorBase::ProcessMessage(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback) {
+  DCHECK_EQ(state(), WAITING_MESSAGE);
+
+  // If pairing failed, and we haven't already done so, try again with the PIN.
+  if (using_paired_secret_ && HasErrorMessage(message)) {
+    using_paired_secret_ = false;
+    waiting_for_authenticator_ = true;
+    v2_authenticator_.reset();
+    SetAuthenticatorCallback set_authenticator = base::Bind(
+        &PairingAuthenticatorBase::SetAuthenticatorAndProcessMessage,
+        weak_factory_.GetWeakPtr(), base::Owned(new buzz::XmlElement(*message)),
+        resume_callback);
+    CreateV2AuthenticatorWithPIN(WAITING_MESSAGE, set_authenticator);
+    return;
+  }
+
+  // If there isn't already an underlying authenticator, ask the derived class
+  // to create one based on the contents of the message.
+  if (!v2_authenticator_) {

[rmsousa, 2013/05/21 23:17:07]

This whole block is host only. It should be part of
PairingHostAuthenticator::ProcessMessage instead. This can be done before the
block above, so PairingHostAuthenticator::Process message can just call
::ProcessMessage at the end).

That also removes a NOTREACHED in the client implementation (since
CreateV2AuthenticatorFromInitialMessage can be either inlined or private to
PairingHostAuthenticator)

[Jamie, 2013/05/22 00:19:14]

That's much cleaner, thanks. Done.

+    v2_authenticator_ = CreateV2AuthenticatorFromInitialMessage(message);
+    DCHECK(v2_authenticator_);
+    // Depending on the contents of the message, the derived class may decline
+    // to process it. For example, the PairingHostAuthenticator class does this
+    // if it doesn't recognize the client id.
+    if (state() != WAITING_MESSAGE) {
+      resume_callback.Run();
+      return;
+    }
+  }
+
+  // Pass the message to the underlying authenticator for processing, but
+  // check for a failed SPAKE exchange if we're using the paired secret. In
+  // this case the pairing protocol can continue by communicating the error
+  // to the peer and retrying with the PIN.
+  v2_authenticator_->ProcessMessage(
+      message,
+      base::Bind(&PairingAuthenticatorBase::CheckForFailedSpakeExchange,
+                 weak_factory_.GetWeakPtr(), resume_callback));
+}
+
+scoped_ptr<buzz::XmlElement> PairingAuthenticatorBase::GetNextMessage() {
+  DCHECK_EQ(state(), MESSAGE_READY);
+  scoped_ptr<buzz::XmlElement> result = v2_authenticator_->GetNextMessage();

[rmsousa, 2013/05/21 23:17:07]

Nit: In ThirdPartyAuth the two lines below are encompassed by an
implementation-specific AddPairingElements(). For consistency, maybe have an
AddPairingElements() whose base implementation is MaybeAddErrorMessages, and the
client implementation adds the AmendProtocolMessage code?

[Jamie, 2013/05/22 00:19:14]

I don't want the subclasses to be able to avoid the error element being added,
so I prefer to keep them separate.

+  AmendProtocolMessage(result.get());
+  MaybeAddErrorMessage(result.get());
+  return result.Pass();
+}
+
+scoped_ptr<ChannelAuthenticator>
+PairingAuthenticatorBase::CreateChannelAuthenticator() const {
+  return v2_authenticator_->CreateChannelAuthenticator();
+}
+
+void PairingAuthenticatorBase::MaybeAddErrorMessage(buzz::XmlElement* message) {
+  if (!error_message_.empty()) {
+    buzz::XmlElement* pairing_failed_tag =
+        new buzz::XmlElement(kPairingFailedTag);
+    pairing_failed_tag->AddAttr(kPairingErrorAttribute, error_message_);
+    message->AddElement(pairing_failed_tag);
+    error_message_.clear();
+  }
+}
+
+bool PairingAuthenticatorBase::HasErrorMessage(
+    const buzz::XmlElement* message) const {
+  const buzz::XmlElement* pairing_failed_tag =
+      message->FirstNamed(kPairingFailedTag);
+  if (pairing_failed_tag) {
+    // If pairing failed, and we haven't already done so, prompt the
+    // user for the PIN and try again.
+    std::string error = pairing_failed_tag->Attr(kPairingErrorAttribute);
+    LOG(INFO) << "Pairing failed: " << error;
+  }
+  return pairing_failed_tag != NULL;
+}
+
+void PairingAuthenticatorBase::CheckForFailedSpakeExchange(
+    const base::Closure& resume_callback) {
+  // If the SPAKE exchange failed due to invalid credentials, and those
+  // credentials were the paired secret, then notify the peer that the
+  // PIN-less connection failed and retry using the PIN.
+  if (v2_authenticator_->state() == REJECTED &&
+      v2_authenticator_->rejection_reason() == INVALID_CREDENTIALS &&
+      using_paired_secret_) {
+    using_paired_secret_ = false;
+    error_message_ = "invalid-shared-secret";
+    v2_authenticator_.reset();
+    SetAuthenticatorCallback set_authenticator = base::Bind(
+        &PairingAuthenticatorBase::SetAuthenticator,
+        weak_factory_.GetWeakPtr(), resume_callback);
+    CreateV2AuthenticatorWithPIN(MESSAGE_READY, set_authenticator);
+    return;
+  }
+
+  resume_callback.Run();
+}
+
+void PairingAuthenticatorBase::SetAuthenticator(
+    const base::Closure& resume_callback,
+    scoped_ptr<Authenticator> authenticator) {
+  DCHECK(!v2_authenticator_);
+  DCHECK(authenticator);
+  waiting_for_authenticator_ = false;
+  v2_authenticator_ = authenticator.Pass();
+  resume_callback.Run();
+}
+
+void PairingAuthenticatorBase::SetAuthenticatorAndProcessMessage(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback,
+    scoped_ptr<Authenticator> authenticator) {
+  DCHECK(!v2_authenticator_);
+  DCHECK(authenticator);
+  waiting_for_authenticator_ = false;
+  v2_authenticator_ = authenticator.Pass();
+  ProcessMessage(message, resume_callback);
+}
+
+}  // namespace protocol
+}  // namespace remoting