Bluetooth: treat unpairable devices as Trusted/Paired

A small number of devices cannot be paired, but can at least be
connected to insecurely. Support these by marking the devices as
Trusted, which causes BlueZ to remember the device and which allows
future incoming connections from them.

Make IsPaired() return true for these devices, there's no reason in
the UI to treat these any differently than paired devices.

TBR=youngki@chromium.org
BUG=232145
TEST=device_unittests

Review URL: https://codereview.chromium.org/14225017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@195021 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 106 insertions, 33 deletions

diff --git a/device/bluetooth/bluetooth_device_chromeos.cc b/device/bluetooth/bluetooth_device_chromeos.cc
index b456cf4..67666b8 100644
--- a/device/bluetooth/bluetooth_device_chromeos.cc
+++ b/device/bluetooth/bluetooth_device_chromeos.cc
@@ -163,8 +163,9 @@ void BluetoothDeviceChromeOS::Connect(
     // Connection to already paired or connected device.
     ConnectApplications(wrapped_callback, wrapped_error_callback);
 
-  } else if (!pairing_delegate) {
-    // No pairing delegate supplied, initiate low-security connection only.
+  } else if (!pairing_delegate || !IsPairable()) {
+    // No pairing delegate supplied, or unpairable device; initiate
+    // low-security connection only.
     DBusThreadManager::Get()->GetBluetoothAdapterClient()->
         CreateDevice(adapter_->object_path_,
                      address_,
@@ -410,16 +411,7 @@ void BluetoothDeviceChromeOS::OnCreateDevice(
         << object_path_.value();
   }
 
-  // Mark the device trusted so it can connect to us automatically, and
-  // we can connect after rebooting. This information is part of the
-  // pairing information of the device, and is unique to the combination
-  // of our bluetooth address and the device's bluetooth address. A
-  // different host needs a new pairing, so it's not useful to sync.
-  DBusThreadManager::Get()->GetBluetoothDeviceClient()->
-      GetProperties(object_path_)->trusted.Set(
-          true,
-          base::Bind(&BluetoothDeviceChromeOS::OnSetTrusted,
-                     weak_ptr_factory_.GetWeakPtr()));
+  SetTrusted();
 
   // In parallel with the |trusted| property change, call GetServiceRecords to
   // retrieve the SDP from the device and then, either on success or failure,
@@ -490,6 +482,19 @@ void BluetoothDeviceChromeOS::CollectServiceRecordsCallback(
   callback.Run(service_records_);
 }
 
+void BluetoothDeviceChromeOS::SetTrusted() {
+  // Unconditionally send the property change, rather than checking the value
+  // first; there's no harm in doing this and it solves any race conditions
+  // with the property becoming true or false and this call happening before
+  // we get the D-Bus signal about the earlier change.
+  DBusThreadManager::Get()->GetBluetoothDeviceClient()->
+      GetProperties(object_path_)->trusted.Set(
+          true,
+          base::Bind(
+              &BluetoothDeviceChromeOS::OnSetTrusted,
+              weak_ptr_factory_.GetWeakPtr()));
+}
+
 void BluetoothDeviceChromeOS::OnSetTrusted(bool success) {
   LOG_IF(WARNING, !success) << "Failed to set device as trusted: " << address_;
 }
@@ -613,6 +618,7 @@ void BluetoothDeviceChromeOS::OnConnect(const base::Closure& callback,
   // run on the same thread, which is true).
   if (connecting_applications_counter_ == -1) {
     connecting_applications_counter_ = 0;
+    SetTrusted();
     callback.Run();
   }
 }
diff --git a/device/bluetooth/bluetooth_device_chromeos.h b/device/bluetooth/bluetooth_device_chromeos.h
index a4c213d..558ccce 100644
--- a/device/bluetooth/bluetooth_device_chromeos.h
+++ b/device/bluetooth/bluetooth_device_chromeos.h
@@ -139,9 +139,11 @@ class BluetoothDeviceChromeOS
       const BluetoothDeviceClient::ServiceMap& service_map,
       bool success);
 
-  // Called by BluetoothProperty when the call to Set() for the Trusted
-  // property completes. |success| indicates whether or not the request
-  // succeeded.
+  // Set the device as trusted. Trusted devices can connect to us automatically,
+  // and we can connect to them after rebooting. This also causes the device to
+  // be remembered by the stack even if not paired. |success| to the callback
+  // indicates whether or not the request succeeded.
+  void SetTrusted();
   void OnSetTrusted(bool success);
 
   // Called by BluetoothAdapterClient when a call to GetServiceRecords()
diff --git a/device/bluetooth/bluetooth_device_experimental_chromeos.cc b/device/bluetooth/bluetooth_device_experimental_chromeos.cc
index fb36c845..3376775 100644
--- a/device/bluetooth/bluetooth_device_experimental_chromeos.cc
+++ b/device/bluetooth/bluetooth_device_experimental_chromeos.cc
@@ -74,7 +74,10 @@ bool BluetoothDeviceExperimentalChromeOS::IsPaired() const {
           GetProperties(object_path_);
   DCHECK(properties);
 
-  return properties->paired.value();
+  // Trusted devices are devices that don't support pairing but that the
+  // user has explicitly connected; it makes no sense for UI purposes to
+  // treat them differently from each other.
+  return properties->paired.value() || properties->trusted.value();
 }
 
 bool BluetoothDeviceExperimentalChromeOS::IsConnected() const {
@@ -139,8 +142,8 @@ void BluetoothDeviceExperimentalChromeOS::Connect(
   VLOG(1) << object_path_.value() << ": Connecting, " << num_connecting_calls_
           << " in progress";
 
-  if (IsPaired() || IsConnected() || !pairing_delegate) {
-    // No need to pair, skip straight to connection.
+  if (IsPaired() || IsConnected() || !pairing_delegate || !IsPairable()) {
+    // No need to pair, or unable to, skip straight to connection.
     ConnectInternal(callback, error_callback);
   } else {
     // Initiate high-security connection with pairing.
@@ -396,6 +399,8 @@ void BluetoothDeviceExperimentalChromeOS::OnConnect(
   VLOG(1) << object_path_.value() << ": Connected, " << num_connecting_calls_
         << " still in progress";
 
+  SetTrusted();
+
   callback.Run();
 }
 
@@ -465,21 +470,8 @@ void BluetoothDeviceExperimentalChromeOS::OnPair(
     const base::Closure& callback,
     const ConnectErrorCallback& error_callback) {
   VLOG(1) << object_path_.value() << ": Paired";
-
-  // Now that we're paired, we need to set the device as trusted so that
-  // incoming connections will be accepted. This should only ever fail if
-  // the device is removed mid-pairing, so do it in the background while
-  // we connect and don't worry about errors.
-  DBusThreadManager::Get()->GetExperimentalBluetoothDeviceClient()->
-      GetProperties(object_path_)->trusted.Set(
-          true,
-          base::Bind(
-              &BluetoothDeviceExperimentalChromeOS::OnSetTrusted,
-              weak_ptr_factory_.GetWeakPtr()));
-
   UnregisterAgent();
-
-  // Now we can connect to the device!
+  SetTrusted();
   ConnectInternal(callback, error_callback);
 }
 
@@ -520,6 +512,19 @@ void BluetoothDeviceExperimentalChromeOS::OnCancelPairingError(
                << error_name << ": " << error_message;
 }
 
+void BluetoothDeviceExperimentalChromeOS::SetTrusted() {
+  // Unconditionally send the property change, rather than checking the value
+  // first; there's no harm in doing this and it solves any race conditions
+  // with the property becoming true or false and this call happening before
+  // we get the D-Bus signal about the earlier change.
+  DBusThreadManager::Get()->GetExperimentalBluetoothDeviceClient()->
+      GetProperties(object_path_)->trusted.Set(
+          true,
+          base::Bind(
+              &BluetoothDeviceExperimentalChromeOS::OnSetTrusted,
+              weak_ptr_factory_.GetWeakPtr()));
+}
+
 void BluetoothDeviceExperimentalChromeOS::OnSetTrusted(bool success) {
   LOG_IF(WARNING, !success) << object_path_.value()
                             << ": Failed to set device as trusted";
diff --git a/device/bluetooth/bluetooth_device_experimental_chromeos.h b/device/bluetooth/bluetooth_device_experimental_chromeos.h
index aa24657..e30fdf8 100644
--- a/device/bluetooth/bluetooth_device_experimental_chromeos.h
+++ b/device/bluetooth/bluetooth_device_experimental_chromeos.h
@@ -131,7 +131,11 @@ class BluetoothDeviceExperimentalChromeOS
   void OnCancelPairingError(const std::string& error_name,
                             const std::string& error_message);
 
-  // Called by dbus:: on completion of the trusted property change.
+  // Internal method to set the device as trusted. Trusted devices can connect
+  // to us automatically, and we can connect to them after rebooting; it also
+  // causes the device to be remembered by the stack even if not paired.
+  // |success| to the callback indicates whether or not the request succeeded.
+  void SetTrusted();
   void OnSetTrusted(bool success);
 
   // Internal method to unregister the pairing agent and method called by dbus::
diff --git a/device/bluetooth/bluetooth_experimental_chromeos_unittest.cc b/device/bluetooth/bluetooth_experimental_chromeos_unittest.cc
index 7bd7c25..a58585a 100644
--- a/device/bluetooth/bluetooth_experimental_chromeos_unittest.cc
+++ b/device/bluetooth/bluetooth_experimental_chromeos_unittest.cc
@@ -881,6 +881,56 @@ TEST_F(BluetoothExperimentalChromeOSTest, ForgetDevice) {
   ASSERT_EQ(0U, devices.size());
 }
 
+TEST_F(BluetoothExperimentalChromeOSTest, ForgetUnpairedDevice) {
+  GetAdapter();
+  DiscoverDevices();
+
+  BluetoothDevice* device = adapter_->GetDevice(
+      FakeBluetoothDeviceClient::kMicrosoftMouseAddress);
+  ASSERT_TRUE(device != NULL);
+  ASSERT_FALSE(device->IsPaired());
+
+  // Connect the device so it becomes trusted and remembered.
+  device->Connect(
+      NULL,
+      base::Bind(&BluetoothExperimentalChromeOSTest::Callback,
+                 base::Unretained(this)),
+      base::Bind(&BluetoothExperimentalChromeOSTest::ConnectErrorCallback,
+                 base::Unretained(this)));
+
+  ASSERT_EQ(1, callback_count_);
+  ASSERT_EQ(0, error_callback_count_);
+  callback_count_ = 0;
+
+  ASSERT_TRUE(device->IsConnected());
+  ASSERT_FALSE(device->IsConnecting());
+
+  // Make sure the trusted property has been set to true.
+  FakeBluetoothDeviceClient::Properties* properties =
+      fake_bluetooth_device_client_->GetProperties(
+          dbus::ObjectPath(FakeBluetoothDeviceClient::kMicrosoftMousePath));
+  ASSERT_TRUE(properties->trusted.value());
+
+  // Install an observer; expect the DeviceRemoved method to be called
+  // with the device we remove.
+  TestObserver observer(adapter_);
+  adapter_->AddObserver(&observer);
+
+  device->Forget(
+      base::Bind(&BluetoothExperimentalChromeOSTest::ErrorCallback,
+                 base::Unretained(this)));
+  EXPECT_EQ(0, error_callback_count_);
+
+  EXPECT_EQ(1, observer.device_removed_count_);
+  EXPECT_EQ(FakeBluetoothDeviceClient::kMicrosoftMouseAddress,
+            observer.last_device_address_);
+
+  // GetDevices shouldn't return the device either.
+  device = adapter_->GetDevice(
+      FakeBluetoothDeviceClient::kMicrosoftMouseAddress);
+  EXPECT_FALSE(device != NULL);
+}
+
 TEST_F(BluetoothExperimentalChromeOSTest, ConnectPairedDevice) {
   GetAdapter();
 
@@ -940,6 +990,12 @@ TEST_F(BluetoothExperimentalChromeOSTest, ConnectUnpairableDevice) {
 
   EXPECT_TRUE(device->IsConnected());
   EXPECT_FALSE(device->IsConnecting());
+
+  // Make sure the trusted property has been set to true.
+  FakeBluetoothDeviceClient::Properties* properties =
+      fake_bluetooth_device_client_->GetProperties(
+          dbus::ObjectPath(FakeBluetoothDeviceClient::kMicrosoftMousePath));
+  EXPECT_TRUE(properties->trusted.value());
 }
 
 TEST_F(BluetoothExperimentalChromeOSTest, ConnectConnectedDevice) {