[CrOS MultiDevice] Create SecureChannelDisconnector.

This class disconnects cryptauth::SecureChannel objects, which have an
asynchronous disconnection flow. It is an error to delete
cryptauth::SecureChannel objects before they are fully disconnected, as
this can cause the underlying connection to remain open.

Bug: 824568, 752273
Change-Id: I42ebf3814f27f9f45c6d90173cb3cb8513bc800b
Reviewed-on: https://chromium-review.googlesource.com/1101515
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567474}

chromeos/services/secure_channel/BUILD.gn

@@ -84,6 +84,9 @@ static_library("secure_channel") {
     "pending_connection_request_delegate.h",
     "secure_channel_base.cc",
     "secure_channel_base.h",
+    "secure_channel_disconnector.h",
+    "secure_channel_disconnector_impl.cc",
+    "secure_channel_disconnector_impl.h",
     "secure_channel_impl.cc",
     "secure_channel_impl.h",
     "secure_channel_service.cc",
@@ -158,6 +161,8 @@ static_library("test_support") {
     "fake_pending_connection_request_delegate.h",
     "fake_secure_channel.cc",
     "fake_secure_channel.h",
+    "fake_secure_channel_disconnector.cc",
+    "fake_secure_channel_disconnector.h",
     "fake_single_client_message_proxy.cc",
     "fake_single_client_message_proxy.h",
     "fake_timer_factory.cc",
@@ -197,6 +202,7 @@ source_set("unit_tests") {
     "pending_ble_listener_connection_request_unittest.cc",
     "pending_connection_manager_impl_unittest.cc",
     "pending_connection_request_base_unittest.cc",
+    "secure_channel_disconnector_impl_unittest.cc",
     "secure_channel_service_unittest.cc",
     "shared_resource_scheduler_unittest.cc",
     "single_client_message_proxy_impl_unittest.cc",

chromeos/services/secure_channel/fake_secure_channel_disconnector.cc

+// Copyright 2018 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 "chromeos/services/secure_channel/fake_secure_channel_disconnector.h"
+
+namespace chromeos {
+
+namespace secure_channel {
+
+FakeSecureChannelDisconnector::FakeSecureChannelDisconnector() = default;
+
+FakeSecureChannelDisconnector::~FakeSecureChannelDisconnector() = default;
+
+bool FakeSecureChannelDisconnector::WasChannelHandled(
+    cryptauth::SecureChannel* secure_channel) {
+  for (const auto& channel : handled_channels_) {
+    if (channel.get() == secure_channel)
+      return true;
+  }
+  return false;
+}
+
+void FakeSecureChannelDisconnector::DisconnectSecureChannel(
+    std::unique_ptr<cryptauth::SecureChannel> channel_to_disconnect) {
+  handled_channels_.insert(std::move(channel_to_disconnect));
+}
+
+}  // namespace secure_channel
+
+}  // namespace chromeos

chromeos/services/secure_channel/fake_secure_channel_disconnector.h

+// Copyright 2018 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.
+
+#ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_SECURE_CHANNEL_DISCONNECTOR_H_
+#define CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_SECURE_CHANNEL_DISCONNECTOR_H_
+
+#include <memory>
+
+#include "base/containers/flat_set.h"
+#include "base/macros.h"
+#include "chromeos/services/secure_channel/secure_channel_disconnector.h"
+#include "components/cryptauth/secure_channel.h"
+
+namespace chromeos {
+
+namespace secure_channel {
+
+// Test SecureChannelDisconnector implementation.
+class FakeSecureChannelDisconnector : public SecureChannelDisconnector {
+ public:
+  FakeSecureChannelDisconnector();
+  ~FakeSecureChannelDisconnector() override;
+
+  const base::flat_set<std::unique_ptr<cryptauth::SecureChannel>>&
+  handled_channels() {
+    return handled_channels_;
+  }
+
+  bool WasChannelHandled(cryptauth::SecureChannel* secure_channel);
+
+ private:
+  // SecureChannelDisconnector:
+  void DisconnectSecureChannel(
+      std::unique_ptr<cryptauth::SecureChannel> channel_to_disconnect) override;
+
+  base::flat_set<std::unique_ptr<cryptauth::SecureChannel>> handled_channels_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeSecureChannelDisconnector);
+};
+
+}  // namespace secure_channel
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_SECURE_CHANNEL_DISCONNECTOR_H_

chromeos/services/secure_channel/secure_channel_disconnector.h

+// Copyright 2018 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.
+
+#ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_SECURE_CHANNEL_DISCONNECTOR_H_
+#define CHROMEOS_SERVICES_SECURE_CHANNEL_SECURE_CHANNEL_DISCONNECTOR_H_
+
+#include <memory>
+
+#include "base/macros.h"
+
+namespace cryptauth {
+class SecureChannel;
+}  // namespace cryptauth
+
+namespace chromeos {
+
+namespace secure_channel {
+
+// Disconnects cryptauth::SecureChannel objects, which have an asynchronous
+// disconnection flow. Deleting cryptauth::SecureChannel objects before they are
+// fully disconnected can cause the underlying connection to remain open, which
+// causes instability on the next connection attempt. See
+// https://crbug.com/763604.
+class SecureChannelDisconnector {
+ public:
+  virtual ~SecureChannelDisconnector() = default;
+
+  virtual void DisconnectSecureChannel(
+      std::unique_ptr<cryptauth::SecureChannel> channel_to_disconnect) = 0;
+
+ protected:
+  SecureChannelDisconnector() = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SecureChannelDisconnector);
+};
+
+}  // namespace secure_channel
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_SECURE_CHANNEL_SECURE_CHANNEL_DISCONNECTOR_H_

chromeos/services/secure_channel/secure_channel_disconnector_impl.cc

+// Copyright 2018 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 "chromeos/services/secure_channel/secure_channel_disconnector_impl.h"
+
+#include "base/memory/ptr_util.h"
+#include "base/no_destructor.h"
+#include "chromeos/components/proximity_auth/logging/logging.h"
+
+namespace chromeos {
+
+namespace secure_channel {
+
+// static
+SecureChannelDisconnectorImpl::Factory*
+    SecureChannelDisconnectorImpl::Factory::test_factory_ = nullptr;
+
+// static
+SecureChannelDisconnectorImpl::Factory*
+SecureChannelDisconnectorImpl::Factory::Get() {
+  if (test_factory_)
+    return test_factory_;
+
+  static base::NoDestructor<Factory> factory;
+  return factory.get();
+}
+
+// static
+void SecureChannelDisconnectorImpl::Factory::SetFactoryForTesting(
+    Factory* test_factory) {
+  test_factory_ = test_factory;
+}
+
+SecureChannelDisconnectorImpl::Factory::~Factory() = default;
+
+std::unique_ptr<SecureChannelDisconnector>
+SecureChannelDisconnectorImpl::Factory::BuildInstance() {
+  return base::WrapUnique(new SecureChannelDisconnectorImpl());
+}
+
+SecureChannelDisconnectorImpl::SecureChannelDisconnectorImpl() = default;
+
+SecureChannelDisconnectorImpl::~SecureChannelDisconnectorImpl() = default;
+
+void SecureChannelDisconnectorImpl::DisconnectSecureChannel(
+    std::unique_ptr<cryptauth::SecureChannel> channel_to_disconnect) {
+  // If |channel_to_disconnect| was already DISCONNECTING, this function is a
+  // no-op. If |channel_to_disconnecting| was CONNECTING, this function
+  // immediately causes the channel to switch to DISCONNECTED. Both of these
+  // cases trigger an early return below.
+  channel_to_disconnect->Disconnect();
+  if (channel_to_disconnect->status() ==
+      cryptauth::SecureChannel::Status::DISCONNECTED) {
+    return;
+  }
+
+  // If no early return occurred, |channel_to_disconnect| is now DISCONNECTING.
+  DCHECK_EQ(cryptauth::SecureChannel::Status::DISCONNECTING,
+            channel_to_disconnect->status());
+
+  // Observe |channel_to_disconnect| so that we can be alerted when it does
+  // eventually transition to DISCONNECTED.
+  channel_to_disconnect->AddObserver(this);
+  disconnecting_channels_.insert(std::move(channel_to_disconnect));
+}
+
+void SecureChannelDisconnectorImpl::OnSecureChannelStatusChanged(
+    cryptauth::SecureChannel* secure_channel,
+    const cryptauth::SecureChannel::Status& old_status,
+    const cryptauth::SecureChannel::Status& new_status) {
+  if (new_status != cryptauth::SecureChannel::Status::DISCONNECTED)
+    return;
+
+  for (auto it = disconnecting_channels_.begin();
+       it != disconnecting_channels_.end(); ++it) {
+    if (secure_channel == it->get()) {
+      (*it)->RemoveObserver(this);
+      disconnecting_channels_.erase(it);
+      return;
+    }
+  }
+
+  PA_LOG(ERROR) << "SecureChannelDisconnectorImpl::"
+                << "OnSecureChannelStatusChanged(): Channel was disconnected, "
+                << "but it was not being tracked.";
+  NOTREACHED();
+}
+
+}  // namespace secure_channel
+
+}  // namespace chromeos

chromeos/services/secure_channel/secure_channel_disconnector_impl.h

+// Copyright 2018 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.
+
+#ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_SECURE_CHANNEL_DISCONNECTOR_IMPL_H_
+#define CHROMEOS_SERVICES_SECURE_CHANNEL_SECURE_CHANNEL_DISCONNECTOR_IMPL_H_
+
+#include <memory>
+
+#include "base/containers/flat_set.h"
+#include "base/macros.h"
+#include "chromeos/services/secure_channel/secure_channel_disconnector.h"
+#include "components/cryptauth/secure_channel.h"
+
+namespace chromeos {
+
+namespace secure_channel {
+
+// Concrete SecureChannelDisconnector implementation.
+class SecureChannelDisconnectorImpl
+    : public SecureChannelDisconnector,
+      public cryptauth::SecureChannel::Observer {
+ public:
+  class Factory {
+   public:
+    static Factory* Get();
+    static void SetFactoryForTesting(Factory* test_factory);
+    virtual ~Factory();
+    virtual std::unique_ptr<SecureChannelDisconnector> BuildInstance();
+
+   private:
+    static Factory* test_factory_;
+  };
+
+  ~SecureChannelDisconnectorImpl() override;
+
+ private:
+  SecureChannelDisconnectorImpl();
+
+  // SecureChannelDisconnector:
+  void DisconnectSecureChannel(
+      std::unique_ptr<cryptauth::SecureChannel> channel_to_disconnect) override;
+
+  // cryptauth::SecureChannel::Observer:
+  void OnSecureChannelStatusChanged(
+      cryptauth::SecureChannel* secure_channel,
+      const cryptauth::SecureChannel::Status& old_status,
+      const cryptauth::SecureChannel::Status& new_status) override;
+
+  base::flat_set<std::unique_ptr<cryptauth::SecureChannel>>
+      disconnecting_channels_;
+
+  DISALLOW_COPY_AND_ASSIGN(SecureChannelDisconnectorImpl);
+};
+
+}  // namespace secure_channel
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_SECURE_CHANNEL_SECURE_CHANNEL_DISCONNECTOR_IMPL_H_

chromeos/services/secure_channel/secure_channel_disconnector_impl_unittest.cc

+// Copyright 2018 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 "chromeos/services/secure_channel/secure_channel_disconnector_impl.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/containers/flat_set.h"
+#include "base/macros.h"
+#include "base/stl_util.h"
+#include "base/unguessable_token.h"
+#include "components/cryptauth/fake_connection.h"
+#include "components/cryptauth/fake_secure_channel.h"
+#include "components/cryptauth/remote_device_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+
+namespace secure_channel {
+
+class SecureChannelSecureChannelDisconnectorImplTest : public testing::Test {
+ protected:
+  SecureChannelSecureChannelDisconnectorImplTest() = default;
+  ~SecureChannelSecureChannelDisconnectorImplTest() override = default;
+
+  // testing::Test:
+  void SetUp() override {
+    disconnector_ =
+        SecureChannelDisconnectorImpl::Factory::Get()->BuildInstance();
+  }
+
+  // Returns an ID associated with the request as well as a pointer to the
+  // SecureChannel to be disconnected.
+  std::pair<base::UnguessableToken, cryptauth::FakeSecureChannel*>
+  CallDisconnectSecureChannel() {
+    auto fake_secure_channel = std::make_unique<cryptauth::FakeSecureChannel>(
+        std::make_unique<cryptauth::FakeConnection>(
+            cryptauth::CreateRemoteDeviceRefForTest()));
+    fake_secure_channel->ChangeStatus(
+        cryptauth::SecureChannel::Status::CONNECTED);
+
+    cryptauth::FakeSecureChannel* fake_secure_channel_raw =
+        fake_secure_channel.get();
+    base::UnguessableToken id = base::UnguessableToken::Create();
+
+    fake_secure_channel->set_destructor_callback(base::BindOnce(
+        &SecureChannelSecureChannelDisconnectorImplTest::OnSecureChannelDeleted,
+        base::Unretained(this), id));
+
+    disconnector_->DisconnectSecureChannel(std::move(fake_secure_channel));
+
+    return std::make_pair(id, fake_secure_channel_raw);
+  }
+
+  bool HasChannelBeenDeleted(const base::UnguessableToken id) {
+    return base::ContainsKey(deleted_request_ids_, id);
+  }
+
+ private:
+  void OnSecureChannelDeleted(const base::UnguessableToken& id) {
+    deleted_request_ids_.insert(id);
+  }
+
+  base::flat_set<base::UnguessableToken> deleted_request_ids_;
+
+  std::unique_ptr<SecureChannelDisconnector> disconnector_;
+
+  DISALLOW_COPY_AND_ASSIGN(SecureChannelSecureChannelDisconnectorImplTest);
+};
+
+TEST_F(SecureChannelSecureChannelDisconnectorImplTest,
+       TestDoesNotDeleteUntilDisconnected) {
+  // Call disconnect. The channel should not have yet been deleted.
+  auto id_and_channel_pair_1 = CallDisconnectSecureChannel();
+  EXPECT_FALSE(HasChannelBeenDeleted(id_and_channel_pair_1.first));
+
+  // Call disconnect on a second channel; neither should have been deleted.
+  auto id_and_channel_pair_2 = CallDisconnectSecureChannel();
+  EXPECT_FALSE(HasChannelBeenDeleted(id_and_channel_pair_2.first));
+
+  // Update to disconnecting. This should not cause the channel to be deleted.
+  id_and_channel_pair_1.second->ChangeStatus(
+      cryptauth::SecureChannel::Status::DISCONNECTING);
+  EXPECT_FALSE(HasChannelBeenDeleted(id_and_channel_pair_1.first));
+  id_and_channel_pair_2.second->ChangeStatus(
+      cryptauth::SecureChannel::Status::DISCONNECTING);
+  EXPECT_FALSE(HasChannelBeenDeleted(id_and_channel_pair_2.first));
+
+  // Update to disconnected. The channels should be deleted.
+  id_and_channel_pair_1.second->ChangeStatus(
+      cryptauth::SecureChannel::Status::DISCONNECTED);
+  EXPECT_TRUE(HasChannelBeenDeleted(id_and_channel_pair_1.first));
+  id_and_channel_pair_2.second->ChangeStatus(
+      cryptauth::SecureChannel::Status::DISCONNECTED);
+  EXPECT_TRUE(HasChannelBeenDeleted(id_and_channel_pair_2.first));
+}
+
+}  // namespace secure_channel
+
+}  // namespace chromeos

components/cryptauth/fake_secure_channel.cc

@@ -4,6 +4,8 @@
 
 #include "components/cryptauth/fake_secure_channel.h"
 
+#include <utility>
+
 #include "base/logging.h"
 
 namespace cryptauth {
@@ -15,7 +17,10 @@ FakeSecureChannel::SentMessage::SentMessage(const std::string& feature,
 FakeSecureChannel::FakeSecureChannel(std::unique_ptr<Connection> connection)
     : SecureChannel(std::move(connection)) {}
 
-FakeSecureChannel::~FakeSecureChannel() {}
+FakeSecureChannel::~FakeSecureChannel() {
+  if (destructor_callback_)
+    std::move(destructor_callback_).Run();
+}
 
 void FakeSecureChannel::ChangeStatus(const Status& new_status) {
   Status old_status = status_;

components/cryptauth/fake_secure_channel.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_CRYPTAUTH_FAKE_SECURE_CHANNEL_H_
 #define COMPONENTS_CRYPTAUTH_FAKE_SECURE_CHANNEL_H_
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "components/cryptauth/secure_channel.h"
 
@@ -16,6 +17,10 @@ class FakeSecureChannel : public SecureChannel {
   FakeSecureChannel(std::unique_ptr<Connection> connection);
   ~FakeSecureChannel() override;
 
+  void set_destructor_callback(base::OnceClosure destructor_callback) {
+    destructor_callback_ = std::move(destructor_callback);
+  }
+
   struct SentMessage {
     SentMessage(const std::string& feature, const std::string& payload);
 
@@ -44,6 +49,8 @@ class FakeSecureChannel : public SecureChannel {
   std::vector<Observer*> observers_;
   std::vector<SentMessage> sent_messages_;
 
+  base::OnceClosure destructor_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeSecureChannel);
 };