Check USB device permissions with a full device::usb::DeviceInfo.

UsbChooserContext::HasDevicePermission was originally written to take
the device GUID. The problem with this is that it means that the
device::UsbDevice object has to be looked up based on that GUID and if
the device has been disconnected (as happens when trying to fire the
navigator.usb.ondisconnect event) then that lookup fails and so the
permission check always fails. Thus, no events are delivered. Instead,
since WebUsbPermissionProvider has it anyways, HasDevicePermission can
take the DeviceInfo object representing the device which has enough
information to do a permission check even if the device has been
disconnected.

BUG=None

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

Cr-Commit-Position: refs/heads/master@{#379161}

4 files changed, 95 insertions, 102 deletions

diff --git a/chrome/browser/usb/usb_chooser_context.cc b/chrome/browser/usb/usb_chooser_context.cc
index 4aeb569..2fb3bfc 100644
--- a/chrome/browser/usb/usb_chooser_context.cc
+++ b/chrome/browser/usb/usb_chooser_context.cc
@@ -13,6 +13,7 @@
 #include "base/values.h"
 #include "chrome/browser/profiles/profile.h"
 #include "device/core/device_client.h"
+#include "device/usb/public/interfaces/device.mojom.h"
 #include "device/usb/usb_device.h"
 
 using device::UsbDevice;
@@ -47,28 +48,6 @@ bool CanStorePersistentEntry(const scoped_refptr<const UsbDevice>& device) {
   return !device->serial_number().empty();
 }
 
-const base::DictionaryValue* FindForDevice(
-    const std::vector<scoped_ptr<base::DictionaryValue>>& device_list,
-    const scoped_refptr<const UsbDevice>& device) {
-  const std::string utf8_serial_number =
-      base::UTF16ToUTF8(device->serial_number());
-
-  for (const scoped_ptr<base::DictionaryValue>& device_dict : device_list) {
-    int vendor_id;
-    int product_id;
-    std::string serial_number;
-    if (device_dict->GetInteger(kVendorIdKey, &vendor_id) &&
-        device->vendor_id() == vendor_id &&
-        device_dict->GetInteger(kProductIdKey, &product_id) &&
-        device->product_id() == product_id &&
-        device_dict->GetString(kSerialNumberKey, &serial_number) &&
-        utf8_serial_number == serial_number) {
-      return device_dict.get();
-    }
-  }
-  return nullptr;
-}
-
 }  // namespace
 
 UsbChooserContext::UsbChooserContext(Profile* profile)
@@ -134,7 +113,13 @@ void UsbChooserContext::RevokeObjectPermission(
     const base::DictionaryValue& object) {
   std::string guid;
   if (object.GetString(kGuidKey, &guid)) {
-    RevokeDevicePermission(requesting_origin, embedding_origin, guid);
+    auto it = ephemeral_devices_.find(
+        std::make_pair(requesting_origin, embedding_origin));
+    if (it != ephemeral_devices_.end()) {
+      it->second.erase(guid);
+      if (it->second.empty())
+        ephemeral_devices_.erase(it);
+    }
     RecordPermissionRevocation(WEBUSB_PERMISSION_REVOKED_EPHEMERAL);
   } else {
     ChooserContextBase::RevokeObjectPermission(requesting_origin,
@@ -164,43 +149,34 @@ void UsbChooserContext::GrantDevicePermission(const GURL& requesting_origin,
   }
 }
 
-void UsbChooserContext::RevokeDevicePermission(const GURL& requesting_origin,
-                                               const GURL& embedding_origin,
-                                               const std::string& guid) {
+bool UsbChooserContext::HasDevicePermission(
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    const device::usb::DeviceInfo& device_info) {
   auto it = ephemeral_devices_.find(
       std::make_pair(requesting_origin, embedding_origin));
-  if (it != ephemeral_devices_.end()) {
-    it->second.erase(guid);
-    if (it->second.empty())
-      ephemeral_devices_.erase(it);
+  if (it != ephemeral_devices_.end() &&
+      ContainsValue(it->second, device_info.guid)) {
+    return true;
   }
 
-  scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
-  if (!device)
-    return;
-
   std::vector<scoped_ptr<base::DictionaryValue>> device_list =
       GetGrantedObjects(requesting_origin, embedding_origin);
-  const base::DictionaryValue* entry = FindForDevice(device_list, device);
-  if (entry)
-    RevokeObjectPermission(requesting_origin, embedding_origin, *entry);
-}
-
-bool UsbChooserContext::HasDevicePermission(const GURL& requesting_origin,
-                                            const GURL& embedding_origin,
-                                            const std::string& guid) {
-  auto it = ephemeral_devices_.find(
-      std::make_pair(requesting_origin, embedding_origin));
-  if (it != ephemeral_devices_.end())
-    return ContainsValue(it->second, guid);
-
-  scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
-  if (!device)
-    return false;
+  for (const scoped_ptr<base::DictionaryValue>& device_dict : device_list) {
+    int vendor_id;
+    int product_id;
+    std::string serial_number;
+    if (device_dict->GetInteger(kVendorIdKey, &vendor_id) &&
+        device_info.vendor_id == vendor_id &&
+        device_dict->GetInteger(kProductIdKey, &product_id) &&
+        device_info.product_id == product_id &&
+        device_dict->GetString(kSerialNumberKey, &serial_number) &&
+        device_info.serial_number == serial_number) {
+      return true;
+    }
+  }
 
-  std::vector<scoped_ptr<base::DictionaryValue>> device_list =
-      GetGrantedObjects(requesting_origin, embedding_origin);
-  return FindForDevice(device_list, device) != nullptr;
+  return false;
 }
 
 bool UsbChooserContext::IsValidObject(const base::DictionaryValue& object) {
diff --git a/chrome/browser/usb/usb_chooser_context.h b/chrome/browser/usb/usb_chooser_context.h
index 9b26b8b..3fa604f 100644
--- a/chrome/browser/usb/usb_chooser_context.h
+++ b/chrome/browser/usb/usb_chooser_context.h
@@ -16,6 +16,12 @@
 #include "chrome/browser/permissions/chooser_context_base.h"
 #include "device/usb/usb_service.h"
 
+namespace device {
+namespace usb {
+class DeviceInfo;
+}
+}
+
 class UsbChooserContext : public ChooserContextBase,
                           public device::UsbService::Observer {
  public:
@@ -39,17 +45,11 @@ class UsbChooserContext : public ChooserContextBase,
                              const GURL& embedding_origin,
                              const std::string& guid);
 
-  // Revokes |requesting_origin|'s access to the USB device known to
-  // device::UsbService as |guid|.
-  void RevokeDevicePermission(const GURL& requesting_origin,
-                              const GURL& embedding_origin,
-                              const std::string& guid);
-
   // Checks if |requesting_origin| (when embedded within |embedding_origin| has
-  // access to the USB device known to device::UsbService as |guid|.
+  // access to a device with |device_info|.
   bool HasDevicePermission(const GURL& requesting_origin,
                            const GURL& embedding_origin,
-                           const std::string& guid);
+                           const device::usb::DeviceInfo& device_info);
 
  private:
   // ChooserContextBase implementation.
diff --git a/chrome/browser/usb/usb_chooser_context_unittest.cc b/chrome/browser/usb/usb_chooser_context_unittest.cc
index cb5985e..f2ccc8d 100644
--- a/chrome/browser/usb/usb_chooser_context_unittest.cc
+++ b/chrome/browser/usb/usb_chooser_context_unittest.cc
@@ -11,8 +11,11 @@
 #include "device/core/mock_device_client.h"
 #include "device/usb/mock_usb_device.h"
 #include "device/usb/mock_usb_service.h"
+#include "device/usb/mojo/type_converters.h"
+#include "device/usb/public/interfaces/device.mojom.h"
 
 using device::MockUsbDevice;
+using device::UsbDevice;
 
 class UsbChooserContextTest : public testing::Test {
  public:
@@ -31,8 +34,10 @@ class UsbChooserContextTest : public testing::Test {
 
 TEST_F(UsbChooserContextTest, CheckGrantAndRevokePermission) {
   GURL origin("https://www.google.com");
-  scoped_refptr<MockUsbDevice> device =
+  scoped_refptr<UsbDevice> device =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "123ABC");
+  device::usb::DeviceInfoPtr device_info =
+      device::usb::DeviceInfo::From(*device);
   device_client_.usb_service()->AddDevice(device);
   UsbChooserContext* store = UsbChooserContextFactory::GetForProfile(profile());
 
@@ -42,23 +47,23 @@ TEST_F(UsbChooserContextTest, CheckGrantAndRevokePermission) {
   object_dict.SetInteger("product-id", 0);
   object_dict.SetString("serial-number", "123ABC");
 
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   store->GrantDevicePermission(origin, origin, device->guid());
-  EXPECT_TRUE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device_info));
   std::vector<scoped_ptr<base::DictionaryValue>> objects =
       store->GetGrantedObjects(origin, origin);
-  EXPECT_EQ(1u, objects.size());
+  ASSERT_EQ(1u, objects.size());
   EXPECT_TRUE(object_dict.Equals(objects[0].get()));
   std::vector<scoped_ptr<ChooserContextBase::Object>> all_origin_objects =
       store->GetAllGrantedObjects();
-  EXPECT_EQ(1u, all_origin_objects.size());
+  ASSERT_EQ(1u, all_origin_objects.size());
   EXPECT_EQ(origin, all_origin_objects[0]->requesting_origin);
   EXPECT_EQ(origin, all_origin_objects[0]->embedding_origin);
   EXPECT_TRUE(object_dict.Equals(&all_origin_objects[0]->object));
   EXPECT_FALSE(all_origin_objects[0]->incognito);
 
-  store->RevokeDevicePermission(origin, origin, device->guid());
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device->guid()));
+  store->RevokeObjectPermission(origin, origin, *objects[0]);
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   objects = store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(0u, objects.size());
   all_origin_objects = store->GetAllGrantedObjects();
@@ -67,10 +72,14 @@ TEST_F(UsbChooserContextTest, CheckGrantAndRevokePermission) {
 
 TEST_F(UsbChooserContextTest, CheckGrantAndRevokeEphemeralPermission) {
   GURL origin("https://www.google.com");
-  scoped_refptr<MockUsbDevice> device =
+  scoped_refptr<UsbDevice> device =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "");
-  scoped_refptr<MockUsbDevice> other_device =
+  device::usb::DeviceInfoPtr device_info =
+      device::usb::DeviceInfo::From(*device);
+  scoped_refptr<UsbDevice> other_device =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "");
+  device::usb::DeviceInfoPtr other_device_info =
+      device::usb::DeviceInfo::From(*other_device);
   device_client_.usb_service()->AddDevice(device);
   UsbChooserContext* store = UsbChooserContextFactory::GetForProfile(profile());
 
@@ -78,11 +87,10 @@ TEST_F(UsbChooserContextTest, CheckGrantAndRevokeEphemeralPermission) {
   object_dict.SetString("name", "Gizmo");
   object_dict.SetString("ephemeral-guid", device->guid());
 
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   store->GrantDevicePermission(origin, origin, device->guid());
-  EXPECT_TRUE(store->HasDevicePermission(origin, origin, device->guid()));
-  EXPECT_FALSE(
-      store->HasDevicePermission(origin, origin, other_device->guid()));
+  EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device_info));
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *other_device_info));
   std::vector<scoped_ptr<base::DictionaryValue>> objects =
       store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(1u, objects.size());
@@ -95,8 +103,8 @@ TEST_F(UsbChooserContextTest, CheckGrantAndRevokeEphemeralPermission) {
   EXPECT_TRUE(object_dict.Equals(&all_origin_objects[0]->object));
   EXPECT_FALSE(all_origin_objects[0]->incognito);
 
-  store->RevokeDevicePermission(origin, origin, device->guid());
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device->guid()));
+  store->RevokeObjectPermission(origin, origin, *objects[0]);
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   objects = store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(0u, objects.size());
   all_origin_objects = store->GetAllGrantedObjects();
@@ -105,14 +113,16 @@ TEST_F(UsbChooserContextTest, CheckGrantAndRevokeEphemeralPermission) {
 
 TEST_F(UsbChooserContextTest, DisconnectDeviceWithPermission) {
   GURL origin("https://www.google.com");
-  scoped_refptr<MockUsbDevice> device =
+  scoped_refptr<UsbDevice> device =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "123ABC");
+  device::usb::DeviceInfoPtr device_info =
+      device::usb::DeviceInfo::From(*device);
   device_client_.usb_service()->AddDevice(device);
   UsbChooserContext* store = UsbChooserContextFactory::GetForProfile(profile());
 
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   store->GrantDevicePermission(origin, origin, device->guid());
-  EXPECT_TRUE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device_info));
   std::vector<scoped_ptr<base::DictionaryValue>> objects =
       store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(1u, objects.size());
@@ -121,17 +131,17 @@ TEST_F(UsbChooserContextTest, DisconnectDeviceWithPermission) {
   EXPECT_EQ(1u, all_origin_objects.size());
 
   device_client_.usb_service()->RemoveDevice(device);
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device_info));
   objects = store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(1u, objects.size());
   all_origin_objects = store->GetAllGrantedObjects();
   EXPECT_EQ(1u, all_origin_objects.size());
 
-  scoped_refptr<MockUsbDevice> reconnected_device =
+  scoped_refptr<UsbDevice> reconnected_device =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "123ABC");
   device_client_.usb_service()->AddDevice(reconnected_device);
-  EXPECT_TRUE(
-      store->HasDevicePermission(origin, origin, reconnected_device->guid()));
+  device_info = device::usb::DeviceInfo::From(*reconnected_device);
+  EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device_info));
   objects = store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(1u, objects.size());
   all_origin_objects = store->GetAllGrantedObjects();
@@ -140,14 +150,16 @@ TEST_F(UsbChooserContextTest, DisconnectDeviceWithPermission) {
 
 TEST_F(UsbChooserContextTest, DisconnectDeviceWithEphemeralPermission) {
   GURL origin("https://www.google.com");
-  scoped_refptr<MockUsbDevice> device =
+  scoped_refptr<UsbDevice> device =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "");
+  device::usb::DeviceInfoPtr device_info =
+      device::usb::DeviceInfo::From(*device);
   device_client_.usb_service()->AddDevice(device);
   UsbChooserContext* store = UsbChooserContextFactory::GetForProfile(profile());
 
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   store->GrantDevicePermission(origin, origin, device->guid());
-  EXPECT_TRUE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device_info));
   std::vector<scoped_ptr<base::DictionaryValue>> objects =
       store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(1u, objects.size());
@@ -156,17 +168,17 @@ TEST_F(UsbChooserContextTest, DisconnectDeviceWithEphemeralPermission) {
   EXPECT_EQ(1u, all_origin_objects.size());
 
   device_client_.usb_service()->RemoveDevice(device);
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device->guid()));
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   objects = store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(0u, objects.size());
   all_origin_objects = store->GetAllGrantedObjects();
   EXPECT_EQ(0u, all_origin_objects.size());
 
-  scoped_refptr<MockUsbDevice> reconnected_device =
+  scoped_refptr<UsbDevice> reconnected_device =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "");
+  device_info = device::usb::DeviceInfo::From(*reconnected_device);
   device_client_.usb_service()->AddDevice(reconnected_device);
-  EXPECT_FALSE(
-      store->HasDevicePermission(origin, origin, reconnected_device->guid()));
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   objects = store->GetGrantedObjects(origin, origin);
   EXPECT_EQ(0u, objects.size());
   all_origin_objects = store->GetAllGrantedObjects();
@@ -179,25 +191,29 @@ TEST_F(UsbChooserContextTest, GrantPermissionInIncognito) {
   UsbChooserContext* incognito_store = UsbChooserContextFactory::GetForProfile(
       profile()->GetOffTheRecordProfile());
 
-  scoped_refptr<MockUsbDevice> device1 =
+  scoped_refptr<UsbDevice> device1 =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "");
-  scoped_refptr<MockUsbDevice> device2 =
+  device::usb::DeviceInfoPtr device1_info =
+      device::usb::DeviceInfo::From(*device1);
+  scoped_refptr<UsbDevice> device2 =
       new MockUsbDevice(0, 0, "Google", "Gizmo", "");
+  device::usb::DeviceInfoPtr device2_info =
+      device::usb::DeviceInfo::From(*device2);
   device_client_.usb_service()->AddDevice(device1);
   device_client_.usb_service()->AddDevice(device2);
 
   store->GrantDevicePermission(origin, origin, device1->guid());
-  EXPECT_TRUE(store->HasDevicePermission(origin, origin, device1->guid()));
+  EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device1_info));
   EXPECT_FALSE(
-      incognito_store->HasDevicePermission(origin, origin, device1->guid()));
+      incognito_store->HasDevicePermission(origin, origin, *device1_info));
 
   incognito_store->GrantDevicePermission(origin, origin, device2->guid());
-  EXPECT_TRUE(store->HasDevicePermission(origin, origin, device1->guid()));
-  EXPECT_FALSE(store->HasDevicePermission(origin, origin, device2->guid()));
+  EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device1_info));
+  EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device2_info));
   EXPECT_FALSE(
-      incognito_store->HasDevicePermission(origin, origin, device1->guid()));
+      incognito_store->HasDevicePermission(origin, origin, *device1_info));
   EXPECT_TRUE(
-      incognito_store->HasDevicePermission(origin, origin, device2->guid()));
+      incognito_store->HasDevicePermission(origin, origin, *device2_info));
 
   {
     std::vector<scoped_ptr<base::DictionaryValue>> objects =
diff --git a/chrome/browser/usb/web_usb_permission_provider.cc b/chrome/browser/usb/web_usb_permission_provider.cc
index 01de4bf..a037432 100644
--- a/chrome/browser/usb/web_usb_permission_provider.cc
+++ b/chrome/browser/usb/web_usb_permission_provider.cc
@@ -86,12 +86,13 @@ void WebUSBPermissionProvider::HasDevicePermission(
 
   mojo::Array<mojo::String> allowed_guids;
   for (size_t i = 0; i < requested_devices.size(); ++i) {
-    const device::usb::DeviceInfoPtr& device = requested_devices[i];
-    if (FindOriginInDescriptorSet(device->webusb_allowed_origins.get(),
+    const device::usb::DeviceInfo& device = *requested_devices[i];
+    if (FindOriginInDescriptorSet(device.webusb_allowed_origins.get(),
                                   requesting_origin, nullptr, nullptr) &&
         chooser_context->HasDevicePermission(requesting_origin,
-                                             embedding_origin, device->guid))
-      allowed_guids.push_back(device->guid);
+                                             embedding_origin, device)) {
+      allowed_guids.push_back(device.guid);
+    }
   }
   callback.Run(std::move(allowed_guids));
 }