Adding a new authenticator which can be used to validate the remote user

remoting/protocol/validating_authenticator.cc

+// Copyright 2016 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/validating_authenticator.h"
+
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "remoting/protocol/authenticator.h"
+#include "remoting/protocol/channel_authenticator.h"
+#include "third_party/webrtc/libjingle/xmllite/xmlelement.h"
+
+namespace remoting {
+namespace protocol {
+
+ValidatingAuthenticator::ValidatingAuthenticator(
+    const std::string& remote_jid,
+    const ValidationCallback& validation_callback,
+    std::unique_ptr<Authenticator> current_authenticator)
+    : remote_jid_(remote_jid),
+      validation_callback_(validation_callback),
+      current_authenticator_(std::move(current_authenticator)),
+      weak_factory_(this) {
+  DCHECK(!remote_jid_.empty());
+  DCHECK(!validation_callback_.is_null());
+  DCHECK(current_authenticator_);
+}
+
+ValidatingAuthenticator::~ValidatingAuthenticator() {}
+
+Authenticator::State ValidatingAuthenticator::state() const {

[Jamie, 2016/08/26 23:12:08]

It seems we should be falling back on the underlying authenticator here.
Something like:

if (callback_pending) {
  return PROCESS_MESSAGE;
} else if (callback_rejected) {
  return REJECTED;
} else {
  return current_authenticator_.state();
}

That way there's no need to maintain our own copy of the authenticator state,
only the validation state, which seems like a cleaner separation of duties.

[joedow, 2016/08/29 23:43:22]

I started off using a branching structure like this but I didn't like having the
additional flags to track state when this is what |state_| already does.

The advantage of the approach I have is that there is a single state in this
class which is updated by the underlying authenticator (this is similar to the
NegotiatingAuthenticator which also wraps an authenticator).

The disadvantage of my approach is that a wrapper is needed around the
resume_callback passed to ProcessMessage().

I'd prefer to keep this mechanism as I think it is cleaner.

[Jamie, 2016/08/31 00:28:34]

I'll make a couple of points in favour of my approach; if you're still not
convinced then I'm fine with leaving it this way :)

Firstly, since this class is responsible for invoking a specified callback
before proceeding with an existing validator, I think it's cleaner for it to
maintain state only about that callback. Secondly, storing the state in this
class means it's essentially being stored twice--once in this authenticator and
once in the underlying one--which violates the general design principle of only
storing things once.

[joedow, 2016/08/31 17:42:29]

Discussed offline, I have tried this approach first but it wasn't as clean.

+  return state_;
+}
+
+bool ValidatingAuthenticator::started() const {
+  return current_authenticator_->started();
+}
+
+Authenticator::RejectionReason ValidatingAuthenticator::rejection_reason()
+    const {
+  return rejection_reason_;

[Jamie, 2016/08/26 23:12:07]

Similarly here, the underlying authenticator should have some say in the return
value.

[joedow, 2016/08/29 23:43:22]

Same as above, I think it is cleaner to just use |state_| and
|rejection_reason_| instead of multiple flags.

+}
+
+const std::string& ValidatingAuthenticator::GetAuthKey() const {
+  DCHECK_EQ(state_, ACCEPTED);

[Jamie, 2016/08/26 23:12:08]

I don't think you need this check; state() should never return ACCEPTED unless
the underlying state is ACCEPTED, in which case it's the responsibility of the
underlying authenticator to check this.

[joedow, 2016/08/29 23:43:22]

I'm fine removing this as you are correct that the underling authenticator
should check/DCHECK this as needed.  Other authenticator classes have strict
checking so I wanted to include that here as well but I'm ok removing it for the
methods which solely rely on the wrapped authenticator.

+  return current_authenticator_->GetAuthKey();
+}
+
+std::unique_ptr<ChannelAuthenticator>
+ValidatingAuthenticator::CreateChannelAuthenticator() const {
+  DCHECK_EQ(state_, ACCEPTED);
+  return current_authenticator_->CreateChannelAuthenticator();
+}
+
+void ValidatingAuthenticator::ProcessMessage(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback) {
+  DCHECK_EQ(state_, WAITING_MESSAGE);
+  state_ = PROCESSING_MESSAGE;
+
+  if (first_message_received_) {
+    current_authenticator_->ProcessMessage(
+        message, base::Bind(&ValidatingAuthenticator::UpdateState,
+                            weak_factory_.GetWeakPtr(), resume_callback));
+  } else {
+    first_message_received_ = true;
+    validation_callback_.Run(
+        remote_jid_, base::Bind(&ValidatingAuthenticator::OnValidateComplete,
+                                weak_factory_.GetWeakPtr(),
+                                base::Owned(new buzz::XmlElement(*message)),
+                                resume_callback));
+  }
+}
+
+std::unique_ptr<buzz::XmlElement> ValidatingAuthenticator::GetNextMessage() {
+  DCHECK_EQ(state_, MESSAGE_READY);
+
+  std::unique_ptr<buzz::XmlElement> result;
+  if (current_authenticator_->state() == MESSAGE_READY) {
+    result = current_authenticator_->GetNextMessage();
+  } else {
+    result = CreateEmptyAuthenticatorMessage();

[Jamie, 2016/08/26 23:12:07]

I don't think this code should be reachable; if the underlying authenticator
does not have a message ready, state() should not return MESSAGE_READY.

[joedow, 2016/08/29 23:43:22]

I think that's a reasonable assumption, I'll leave it to the underlying
authenticator to decide what to give the caller (and possibly DCHECK as needed).

+  }
+
+  state_ = current_authenticator_->state();
+  DCHECK(state_ == ACCEPTED || state_ == WAITING_MESSAGE);
+
+  return result;
+}
+
+void ValidatingAuthenticator::OnValidateComplete(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback,
+    Result validation_result) {
+  if (validation_result == Result::SUCCESS) {
+    current_authenticator_->ProcessMessage(
+        message, base::Bind(&ValidatingAuthenticator::UpdateState,
+                            weak_factory_.GetWeakPtr(), resume_callback));
+    return;
+  }
+
+  // |validation_result| represents a rejected state so map the result to a
+  // rejection reason and call the callback to let the caller know the result.
+  state_ = Authenticator::REJECTED;
+
+  switch (validation_result) {
+    case Result::ERROR_INVALID_CREDENTIALS:
+      rejection_reason_ = Authenticator::INVALID_CREDENTIALS;
+      break;
+
+    case Result::ERROR_INVALID_ACCOUNT:
+      rejection_reason_ = Authenticator::INVALID_ACCOUNT;
+      break;
+
+    case Result::ERROR_REJECTED_BY_USER:
+      rejection_reason_ = Authenticator::REJECTED_BY_USER;
+      break;
+
+    default:
+      // Log an error and use the default value for |rejection_result_|.

[Jamie, 2016/08/26 23:12:08]

By defaulting to INVALID_CREDENTIALS here, aren't we making the same mistake we
did with third-party auth? It seems like we should have an "unknown" error for
cases like this. Maybe too large a change for this CL, though.

[joedow, 2016/08/29 23:43:22]

What should happen is that the person adding the new value would see the
NOTREACHED macro trigger on their debug build and add it to the enum.  My
concern with adding an Unknown error is that it could be abused just as easily
and become a grab-bag error.

Perhaps the right thing to do is to force a failure on both retail and debug
builds, that way someone introducing a new validation error can't miss the
NOTREACHED macro on CHK builds.  Let me know if this is something you'd prefer.

[Jamie, 2016/08/31 00:28:34]

Better still, if you fold the SUCCESS case into the switch and get rid of
default, can't the compiler detect missing cases automatically?

Also, s/rejection_result_/rejection_reason_/

[joedow, 2016/08/31 17:42:29]

Fixed the comment.  It looks like the compiler doesn't pick up missing enum
values by default so I have made this a CHECK error (to prevent someone from
missing this will implementing their change).

+      NOTREACHED() << "Unknown validation result value: "
+                   << static_cast<unsigned int>(validation_result);
+  }
+
+  resume_callback.Run();
+}
+
+void ValidatingAuthenticator::UpdateState(
+    const base::Closure& resume_callback) {
+  DCHECK_EQ(state_, PROCESSING_MESSAGE);
+
+  // After the underlying authenticator finishes processing the message,
+  // ValidatingAuthenticator must update its own state before running
+  // |resume_callback|.
+  state_ = current_authenticator_->state();
+
+  // Verify the new state represents a valid state transition.
+  DCHECK(state_ == MESSAGE_READY || state_ == ACCEPTED || state_ == REJECTED)
+      << "State: " << state_;
+
+  if (state_ == REJECTED) {
+    rejection_reason_ = current_authenticator_->rejection_reason();
+  }
+
+  resume_callback.Run();
+}
+
+}  // namespace protocol
+}  // namespace remoting