Refactor BluetoothAllowedDevicesMap

content/browser/bluetooth/web_bluetooth_service_impl.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.
 
 // ID Not In Map Note: A service, characteristic, or descriptor ID not in the
 // corresponding WebBluetoothServiceImpl map [service_id_to_device_address_,
 // characteristic_id_to_service_id_, descriptor_id_to_characteristic_id_]
 // implies a hostile renderer because a renderer obtains the corresponding ID
 // from this class and it will be added to the map at that time.
 
 #include "content/browser/bluetooth/web_bluetooth_service_impl.h"
 
 #include <algorithm>
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/bluetooth/bluetooth_blocklist.h"
 #include "content/browser/bluetooth/bluetooth_device_chooser_controller.h"
 #include "content/browser/bluetooth/bluetooth_metrics.h"
 #include "content/browser/bluetooth/frame_connected_bluetooth_devices.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/browser/storage_partition_impl.h"
 #include "content/common/bluetooth/web_bluetooth_device_id.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "device/bluetooth/bluetooth_adapter_factory_wrapper.h"
 #include "device/bluetooth/bluetooth_remote_gatt_characteristic.h"
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor.h"
 
 using device::BluetoothAdapterFactoryWrapper;
 using device::BluetoothUUID;
@@ skipping 150 matching lines @@
   ClearState();
 }
 
 void WebBluetoothServiceImpl::SetClientConnectionErrorHandler(
     base::Closure closure) {
   binding_.set_connection_error_handler(closure);
 }
 
 bool WebBluetoothServiceImpl::IsDevicePaired(
     const std::string& device_address) {
-  return allowed_devices_map_.GetDeviceId(GetOrigin(), device_address) !=
-         nullptr;
+  return allowed_devices()->GetDeviceId(device_address) != nullptr;
 }
 
 void WebBluetoothServiceImpl::DidFinishNavigation(
     NavigationHandle* navigation_handle) {
   if (navigation_handle->HasCommitted() &&
       navigation_handle->GetRenderFrameHost() == render_frame_host_ &&
       !navigation_handle->IsSamePage()) {
     ClearState();
   }
 }
@@ skipping 177 matching lines @@
     blink::mojom::WebBluetoothGATTQueryQuantity quantity,
     const base::Optional<BluetoothUUID>& services_uuid,
     const RemoteServerGetPrimaryServicesCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       quantity == blink::mojom::WebBluetoothGATTQueryQuantity::SINGLE
           ? UMAWebBluetoothFunction::GET_PRIMARY_SERVICE
           : UMAWebBluetoothFunction::GET_PRIMARY_SERVICES);
   RecordGetPrimaryServicesServices(quantity, services_uuid);
 
-  if (!allowed_devices_map_.IsOriginAllowedToAccessAtLeastOneService(
-          GetOrigin(), device_id)) {
+  if (!allowed_devices()->IsAllowedToAccessAtLeastOneService(device_id)) {
     callback.Run(
         blink::mojom::WebBluetoothResult::NOT_ALLOWED_TO_ACCESS_ANY_SERVICE,
         base::nullopt /* service */);
     return;
   }
 
   if (services_uuid &&
-      !allowed_devices_map_.IsOriginAllowedToAccessService(
-          GetOrigin(), device_id, services_uuid.value())) {
+      !allowed_devices()->IsAllowedToAccessService(device_id,
+                                                   services_uuid.value())) {
     callback.Run(
         blink::mojom::WebBluetoothResult::NOT_ALLOWED_TO_ACCESS_SERVICE,
         base::nullopt /* service */);
     return;
   }
 
   const CacheQueryResult query_result = QueryCacheForDevice(device_id);
 
   if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
     return;
@@ skipping 435 matching lines @@
     device::BluetoothDevice* device) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(device->IsGattServicesDiscoveryComplete());
 
   std::vector<device::BluetoothRemoteGattService*> services =
       services_uuid ? device->GetPrimaryServicesByUUID(services_uuid.value())
                     : device->GetPrimaryServices();
 
   std::vector<blink::mojom::WebBluetoothRemoteGATTServicePtr> response_services;
   for (device::BluetoothRemoteGattService* service : services) {
-    if (!allowed_devices_map_.IsOriginAllowedToAccessService(
-            GetOrigin(), device_id, service->GetUUID())) {
+    if (!allowed_devices()->IsAllowedToAccessService(device_id,
+                                                     service->GetUUID())) {
       continue;
     }
     std::string service_instance_id = service->GetIdentifier();
     const std::string& device_address = device->GetAddress();
     auto insert_result = service_id_to_device_address_.insert(
         make_pair(service_instance_id, device_address));
     // If value is already in map, DCHECK it's valid.
     if (!insert_result.second)
       DCHECK_EQ(insert_result.first->second, device_address);
 
@@ skipping 36 matching lines @@
   const device::BluetoothDevice* const device =
       GetAdapter()->GetDevice(device_address);
   if (device == nullptr) {
     DVLOG(1) << "Device " << device_address << " no longer in adapter";
     RecordRequestDeviceOutcome(UMARequestDeviceOutcome::CHOSEN_DEVICE_VANISHED);
     callback.Run(blink::mojom::WebBluetoothResult::CHOSEN_DEVICE_VANISHED,
                  nullptr /* device */);
     return;
   }
 
-  const WebBluetoothDeviceId device_id_for_origin =
-      allowed_devices_map_.AddDevice(GetOrigin(), device_address, options);
+  const WebBluetoothDeviceId device_id =
+      allowed_devices()->AddDevice(device_address, options);
 
   DVLOG(1) << "Device: " << device->GetNameForDisplay();
 
   blink::mojom::WebBluetoothDevicePtr device_ptr =
       blink::mojom::WebBluetoothDevice::New();
-  device_ptr->id = device_id_for_origin;
+  device_ptr->id = device_id;
   device_ptr->name = device->GetName();
 
   RecordRequestDeviceOutcome(UMARequestDeviceOutcome::SUCCESS);
   callback.Run(blink::mojom::WebBluetoothResult::SUCCESS,
                std::move(device_ptr));
 }
 
 void WebBluetoothServiceImpl::OnGetDeviceFailed(
     const RequestDeviceCallback& callback,
     blink::mojom::WebBluetoothResult result) {
@@ skipping 114 matching lines @@
     device::BluetoothRemoteGattService::GattErrorCode error_code) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordDescriptorWriteValueOutcome(UMAGATTOperationOutcome::SUCCESS);
   callback.Run(TranslateGATTErrorAndRecord(error_code,
                                            UMAGATTOperation::DESCRIPTOR_WRITE));
 }
 
 CacheQueryResult WebBluetoothServiceImpl::QueryCacheForDevice(
     const WebBluetoothDeviceId& device_id) {
   const std::string& device_address =
-      allowed_devices_map_.GetDeviceAddress(GetOrigin(), device_id);
+      allowed_devices()->GetDeviceAddress(device_id);
   if (device_address.empty()) {
     CrashRendererAndClosePipe(bad_message::BDH_DEVICE_NOT_ALLOWED_FOR_ORIGIN);
     return CacheQueryResult(CacheQueryOutcome::BAD_RENDERER);
   }
 
   CacheQueryResult result;
   result.device = GetAdapter()->GetDevice(device_address);
 
   // When a device can't be found in the BluetoothAdapter, that generally
   // indicates that it's gone out of range. We reject with a NetworkError in
   // that case.
   if (result.device == nullptr) {
     result.outcome = CacheQueryOutcome::NO_DEVICE;
   }
   return result;
 }
 
 CacheQueryResult WebBluetoothServiceImpl::QueryCacheForService(
     const std::string& service_instance_id) {
   auto device_iter = service_id_to_device_address_.find(service_instance_id);
 
   // Kill the render, see "ID Not in Map Note" above.
   if (device_iter == service_id_to_device_address_.end()) {
     CrashRendererAndClosePipe(bad_message::BDH_INVALID_SERVICE_ID);
     return CacheQueryResult(CacheQueryOutcome::BAD_RENDERER);
   }
 
   const WebBluetoothDeviceId* device_id =
-      allowed_devices_map_.GetDeviceId(GetOrigin(), device_iter->second);
+      allowed_devices()->GetDeviceId(device_iter->second);
   // Kill the renderer if origin is not allowed to access the device.
   if (device_id == nullptr) {
     CrashRendererAndClosePipe(bad_message::BDH_DEVICE_NOT_ALLOWED_FOR_ORIGIN);
     return CacheQueryResult(CacheQueryOutcome::BAD_RENDERER);
   }
 
   CacheQueryResult result = QueryCacheForDevice(*device_id);
   if (result.outcome != CacheQueryOutcome::SUCCESS) {
     return result;
   }
 
   result.service = result.device->GetGattService(service_instance_id);
   if (result.service == nullptr) {
     result.outcome = CacheQueryOutcome::NO_SERVICE;
-  } else if (!allowed_devices_map_.IsOriginAllowedToAccessService(
-                 GetOrigin(), *device_id, result.service->GetUUID())) {
+  } else if (!allowed_devices()->IsAllowedToAccessService(
+                 *device_id, result.service->GetUUID())) {
     CrashRendererAndClosePipe(bad_message::BDH_SERVICE_NOT_ALLOWED_FOR_ORIGIN);
     return CacheQueryResult(CacheQueryOutcome::BAD_RENDERER);
   }
   return result;
 }
 
 CacheQueryResult WebBluetoothServiceImpl::QueryCacheForCharacteristic(
     const std::string& characteristic_instance_id) {
   auto characteristic_iter =
       characteristic_id_to_service_id_.find(characteristic_instance_id);
@@ skipping 59 matching lines @@
 void WebBluetoothServiceImpl::CrashRendererAndClosePipe(
     bad_message::BadMessageReason reason) {
   bad_message::ReceivedBadMessage(GetRenderProcessHost(), reason);
   binding_.Close();
 }
 
 url::Origin WebBluetoothServiceImpl::GetOrigin() {
   return render_frame_host_->GetLastCommittedOrigin();
 }
 
+BluetoothAllowedDevices* WebBluetoothServiceImpl::allowed_devices() {
+  if (!allowed_devices_) {
+    StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(

[scheib, 2017/02/03 23:28:13]

I suppose check with jam@, but whenever I see a static_cast I think something is
broken. Why isn't the GetBluetoothAllowedDevicesMap method on the
StoragePartition interface?

[juncai, 2017/02/08 06:43:49]

Hi, jam@, the three functions:
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h...
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h...
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h...
are not part of the StoragePartition interface, just wondering why. Since
GetBluetoothAllowedDevicesMap uses the similar pattern as the above three
functions, and here uses static_cast, similar to:
https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/servic...

[jam1, 2017/02/10 05:24:39]

the public interface is for getters that are exposed outside content.

for getters that are only used inside content, we cast to the impl since it's
the only implementation.

[scheib, 2017/02/10 06:33:59]

That's a bit unfortunate, as it's not clear that this is the only impl possible
without global knowledge of the code base. (BTW, there is a test subclass as
well).

Perhaps we can at least add a comment at StoragePartition that it's safe for
other content code to static_cast it.

Or, do something such as:
"""
// In public headers
class Impl;
class Interface {
    public:
        virtual Impl* GetImpl() = 0;
};

// In implementation only headers
class Impl : public Interface {
    public:
        Impl* GetImpl() override { return this; }

        std::string ImplOnlyMethod() { return "ImplOnlyMethod\n"; }
};

// Elsewhere in implementation
{
    Interface* i = new Impl();
    // ...
    std::cout << i->GetImpl()->ImplOnlyMethod();
}
"""

Test subclasses would return null.
Pro: the pattern is very clear that internals are available to anyone who has
access to the Impl class definition. 
Con: A virtual dispatch.

[juncai, 2017/02/10 22:30:43]

What about adding some comments about this in a separate CL? If so, I can start
a CL that does that.

Or if we decide to implement them using the approach that scheib@ mentioned, I
can also help with that, probably also in a separate CL.

What do you think?

[scheib, 2017/02/10 23:31:21]

Separate patch is OK, need to float to jam@ for thoughts, but using the existing
pattern is fine.

+        BrowserContext::GetDefaultStoragePartition(
+            web_contents()->GetBrowserContext()));
+    scoped_refptr<BluetoothAllowedDevicesMap> allowed_devices_map =
+        partition->GetBluetoothAllowedDevicesMap();
+    allowed_devices_ =
+        allowed_devices_map->GetOrCreateAllowedDevices(GetOrigin());
+    DCHECK(allowed_devices_);
+  }
+  return allowed_devices_;

[ortuno, 2017/02/03 20:16:50]

Can this be null? If so we need to DCHECK that it isn't before using it. If not
then just return a reference.

[juncai, 2017/02/08 06:43:49]

It can't be null. Changed it to return a reference.

Done.

+}
+
 void WebBluetoothServiceImpl::ClearState() {
   characteristic_id_to_notify_session_.clear();
   pending_primary_services_requests_.clear();
   descriptor_id_to_characteristic_id_.clear();
   characteristic_id_to_service_id_.clear();
   service_id_to_device_address_.clear();
   connected_devices_.reset(
       new FrameConnectedBluetoothDevices(render_frame_host_));
-  allowed_devices_map_ = BluetoothAllowedDevicesMap();
   device_chooser_controller_.reset();
   BluetoothAdapterFactoryWrapper::Get().ReleaseAdapter(this);
 }
 
 }  // namespace content