Usb UsbDeviceInfo as GrantDevicePermission's parameter.

This CL changes the third parameter of GrantDevicePermission in
UsbChooserContext from |guid| to |device_info|, and stores more info
about the ephemeral devices, so that we can get rid of direct
references to UsbService in subsequent CLs.

Bug: 699790
Change-Id: Ifaa741cec5f4ddb888c3377c2ad0742e677fb521
Reviewed-on: https://chromium-review.googlesource.com/1203877
Commit-Queue: Donna Wu <donna.wu@intel.com>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589830}

chrome/browser/BUILD.gn

@@ -1868,6 +1868,7 @@ jumbo_split_static_library("browser") {
     "//device/fido",
     "//device/gamepad/public/cpp:switches",
     "//device/usb/mojo",
+    "//device/usb/public/cpp",
     "//device/usb/public/mojom",
     "//device/vr/buildflags",
     "//extensions/buildflags",

chrome/browser/ui/page_info/page_info_unittest.cc

@@ -34,6 +34,8 @@
 #include "device/base/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/mojom/device.mojom.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_connection_status_flags.h"
@@ -393,9 +395,11 @@ TEST_F(PageInfoTest, OnSiteDataAccessed) {
 TEST_F(PageInfoTest, OnChosenObjectDeleted) {
   scoped_refptr<device::UsbDevice> device =
       new device::MockUsbDevice(0, 0, "Google", "Gizmo", "1234567890");
+  auto device_info = device::mojom::UsbDeviceInfo::From(*device);
+  DCHECK(device_info);
   usb_service().AddDevice(device);
   UsbChooserContext* store = UsbChooserContextFactory::GetForProfile(profile());
-  store->GrantDevicePermission(url(), url(), device->guid());
+  store->GrantDevicePermission(url(), url(), *device_info);
 
   EXPECT_CALL(*mock_ui(), SetIdentityInfo(_));
   EXPECT_CALL(*mock_ui(), SetCookieInfo(_));

chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc

@@ -24,6 +24,8 @@
 #include "device/base/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/mojom/device.mojom.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -303,10 +305,12 @@ TEST_F(PageInfoBubbleViewTest, SetPermissionInfoWithUsbDevice) {
   const GURL origin = GURL(kUrl).GetOrigin();
   scoped_refptr<device::UsbDevice> device =
       new device::MockUsbDevice(0, 0, "Google", "Gizmo", "1234567890");
+  auto device_info = device::mojom::UsbDeviceInfo::From(*device);
+  DCHECK(device_info);
   device_client_.usb_service()->AddDevice(device);
   UsbChooserContext* store =
       UsbChooserContextFactory::GetForProfile(web_contents_helper_.profile());
-  store->GrantDevicePermission(origin, origin, device->guid());
+  store->GrantDevicePermission(origin, origin, *device_info);
 
   PermissionInfoList list;
   api_->SetPermissionInfo(list);

chrome/browser/usb/usb_chooser_context.cc

@@ -45,8 +45,29 @@ void RecordPermissionRevocation(WebUsbPermissionRevoked kind) {
                             WEBUSB_PERMISSION_REVOKED_MAX);
 }
 
-bool CanStorePersistentEntry(const scoped_refptr<const UsbDevice>& device) {
-  return !device->serial_number().empty();
+bool CanStorePersistentEntry(const device::mojom::UsbDeviceInfo& device_info) {
+  return !device_info.serial_number->empty();
+}
+
+base::DictionaryValue DeviceInfoToDictValue(
+    const device::mojom::UsbDeviceInfo& device_info) {
+  base::DictionaryValue device_dict;
+  device_dict.SetKey(kDeviceNameKey,
+                     device_info.product_name
+                         ? base::Value(*device_info.product_name)
+                         : base::Value(""));
+
+  if (!CanStorePersistentEntry(device_info)) {
+    device_dict.SetKey(kGuidKey, base::Value(device_info.guid));
+    return device_dict;
+  }
+  device_dict.SetKey(kVendorIdKey, base::Value(device_info.vendor_id));
+  device_dict.SetKey(kProductIdKey, base::Value(device_info.product_id));
+  device_dict.SetKey(kSerialNumberKey,
+                     device_info.serial_number
+                         ? base::Value(*device_info.serial_number)
+                         : base::Value(""));
+  return device_dict;
 }
 
 }  // namespace
@@ -77,13 +98,9 @@ UsbChooserContext::GetGrantedObjects(const GURL& requesting_origin,
         std::make_pair(requesting_origin, embedding_origin));
     if (it != ephemeral_devices_.end()) {
       for (const std::string& guid : it->second) {
-        scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
-        DCHECK(device);
-        std::unique_ptr<base::DictionaryValue> object(
-            new base::DictionaryValue());
-        object->SetString(kDeviceNameKey, device->product_string());
-        object->SetString(kGuidKey, device->guid());
-        objects.push_back(std::move(object));
+        auto dict_it = ephemeral_dicts_.find(guid);
+        DCHECK(dict_it != ephemeral_dicts_.end());
+        objects.push_back(dict_it->second.CreateDeepCopy());
       }
     }
   }
@@ -104,13 +121,13 @@ UsbChooserContext::GetAllGrantedObjects() {
       continue;
 
     for (const std::string& guid : map_entry.second) {
-      scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
-      DCHECK(device);
-      base::DictionaryValue object;
-      object.SetString(kDeviceNameKey, device->product_string());
-      object.SetString(kGuidKey, device->guid());
+      auto dict_it = ephemeral_dicts_.find(guid);
+      DCHECK(dict_it != ephemeral_dicts_.end());
+      // ChooserContextBase::Object constructor will swap the object, so
+      // a deep copy is needed here.
+      auto object = dict_it->second.CreateDeepCopy();
       objects.push_back(std::make_unique<ChooserContextBase::Object>(
-          requesting_origin, embedding_origin, &object, "preference",
+          requesting_origin, embedding_origin, object.get(), "preference",
           is_incognito_));
     }
   }
@@ -139,25 +156,21 @@ void UsbChooserContext::RevokeObjectPermission(
   }
 }
 
-void UsbChooserContext::GrantDevicePermission(const GURL& requesting_origin,
-                                              const GURL& embedding_origin,
-                                              const std::string& guid) {
-  scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
-  if (!device)
-    return;
-
-  if (CanStorePersistentEntry(device)) {
-    std::unique_ptr<base::DictionaryValue> device_dict(
-        new base::DictionaryValue());
-    device_dict->SetString(kDeviceNameKey, device->product_string());
-    device_dict->SetInteger(kVendorIdKey, device->vendor_id());
-    device_dict->SetInteger(kProductIdKey, device->product_id());
-    device_dict->SetString(kSerialNumberKey, device->serial_number());
+void UsbChooserContext::GrantDevicePermission(
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    const device::mojom::UsbDeviceInfo& device_info) {
+  if (CanStorePersistentEntry(device_info)) {
     GrantObjectPermission(requesting_origin, embedding_origin,
-                          std::move(device_dict));
+                          std::make_unique<base::DictionaryValue>(
+                              DeviceInfoToDictValue(device_info)));
   } else {
     ephemeral_devices_[std::make_pair(requesting_origin, embedding_origin)]
-        .insert(guid);
+        .insert(device_info.guid);
+    if (!base::ContainsKey(ephemeral_dicts_, device_info.guid)) {
+      ephemeral_dicts_.insert(
+          std::make_pair(device_info.guid, DeviceInfoToDictValue(device_info)));
+    }
   }
 }
 
@@ -232,4 +245,6 @@ void UsbChooserContext::OnDeviceRemovedCleanup(
     scoped_refptr<UsbDevice> device) {
   for (auto& map_entry : ephemeral_devices_)
     map_entry.second.erase(device->guid());
+
+  ephemeral_dicts_.erase(device->guid());
 }

chrome/browser/usb/usb_chooser_context.h

@@ -14,6 +14,7 @@
 
 #include "base/macros.h"
 #include "base/scoped_observer.h"
+#include "base/values.h"
 #include "chrome/browser/permissions/chooser_context_base.h"
 #include "device/usb/usb_service.h"
 
@@ -42,11 +43,10 @@ class UsbChooserContext : public ChooserContextBase,
                               const GURL& embedding_origin,
                               const base::DictionaryValue& object) override;
 
-  // Grants |requesting_origin| access to the USB device known to
-  // device::UsbService as |guid|.
+  // Grants |requesting_origin| access to the USB device.
   void GrantDevicePermission(const GURL& requesting_origin,
                              const GURL& embedding_origin,
-                             const std::string& guid);
+                             const device::mojom::UsbDeviceInfo& device_info);
 
   // Checks if |requesting_origin| (when embedded within |embedding_origin| has
   // access to a device with |device_info|.
@@ -71,6 +71,7 @@ class UsbChooserContext : public ChooserContextBase,
   bool is_incognito_;
   std::map<std::pair<GURL, GURL>, std::set<std::string>> ephemeral_devices_;
   device::UsbService* usb_service_;
+  std::map<std::string, base::DictionaryValue> ephemeral_dicts_;
   ScopedObserver<device::UsbService, device::UsbService::Observer> observer_;
   base::WeakPtrFactory<UsbChooserContext> weak_factory_;
 

chrome/browser/usb/usb_chooser_controller.cc

@@ -38,11 +38,15 @@ using device::UsbDevice;
 
 namespace {
 
-base::string16 FormatUsbDeviceName(scoped_refptr<device::UsbDevice> device) {
-  base::string16 device_name = device->product_string();
+base::string16 FormatUsbDeviceName(
+    const device::mojom::UsbDeviceInfo& device_info) {
+  base::string16 device_name;
+  if (device_info.product_name)
+    device_name = *device_info.product_name;
+
   if (device_name.empty()) {
-    uint16_t vendor_id = device->vendor_id();
-    uint16_t product_id = device->product_id();
+    uint16_t vendor_id = device_info.vendor_id;
+    uint16_t product_id = device_info.product_id;
 #if !defined(OS_ANDROID)
     if (const char* product_name =
             device::UsbIds::GetProductName(vendor_id, product_id)) {
@@ -116,23 +120,24 @@ base::string16 UsbChooserController::GetOption(size_t index) const {
   const base::string16& device_name = devices_[index].second;
   const auto& it = device_name_map_.find(device_name);
   DCHECK(it != device_name_map_.end());
-  return it->second == 1
-             ? device_name
-             : l10n_util::GetStringFUTF16(
-                   IDS_DEVICE_CHOOSER_DEVICE_NAME_WITH_ID, device_name,
-                   devices_[index].first->serial_number());
+
+  if (it->second == 1)
+    return device_name;
+
+  return l10n_util::GetStringFUTF16(IDS_DEVICE_CHOOSER_DEVICE_NAME_WITH_ID,
+                                    device_name,
+                                    devices_[index].first->serial_number
+                                        ? *devices_[index].first->serial_number
+                                        : base::string16());
 }
 
 bool UsbChooserController::IsPaired(size_t index) const {
   if (!chooser_context_)
     return false;
 
-  auto device_info = device::mojom::UsbDeviceInfo::From(*devices_[index].first);
-  DCHECK(device_info);
-
   return WebUsbServiceImpl::HasDevicePermission(
       chooser_context_.get(), requesting_origin_, embedding_origin_,
-      *device_info);
+      *devices_[index].first);
 }
 
 void UsbChooserController::Select(const std::vector<size_t>& indices) {
@@ -142,14 +147,13 @@ void UsbChooserController::Select(const std::vector<size_t>& indices) {
 
   if (chooser_context_) {
     chooser_context_->GrantDevicePermission(
-        requesting_origin_, embedding_origin_, devices_[index].first->guid());
+        requesting_origin_, embedding_origin_, *devices_[index].first);
   }
 
-  std::move(callback_).Run(
-      device::mojom::UsbDeviceInfo::From(*devices_[index].first));
+  std::move(callback_).Run(devices_[index].first.Clone());
 
   RecordWebUsbChooserClosure(
-      devices_[index].first->serial_number().empty()
+      devices_[index].first->serial_number->empty()
           ? WEBUSB_CHOOSER_CLOSED_EPHEMERAL_PERMISSION_GRANTED
           : WEBUSB_CHOOSER_CLOSED_PERMISSION_GRANTED);
 }
@@ -170,9 +174,11 @@ void UsbChooserController::OpenHelpCenterUrl() const {
 }
 
 void UsbChooserController::OnDeviceAdded(scoped_refptr<UsbDevice> device) {
-  if (DisplayDevice(device)) {
-    base::string16 device_name = FormatUsbDeviceName(device);
-    devices_.push_back(std::make_pair(device, device_name));
+  auto device_info = device::mojom::UsbDeviceInfo::From(*device);
+  DCHECK(device_info);
+  if (DisplayDevice(*device_info)) {
+    base::string16 device_name = FormatUsbDeviceName(*device_info);
+    devices_.push_back(std::make_pair(std::move(device_info), device_name));
     ++device_name_map_[device_name];
     if (view())
       view()->OnOptionAdded(devices_.size() - 1);
@@ -180,8 +186,10 @@ void UsbChooserController::OnDeviceAdded(scoped_refptr<UsbDevice> device) {
 }
 
 void UsbChooserController::OnDeviceRemoved(scoped_refptr<UsbDevice> device) {
+  auto device_info = device::mojom::UsbDeviceInfo::From(*device);
+  DCHECK(device_info);
   for (auto it = devices_.begin(); it != devices_.end(); ++it) {
-    if (it->first == device) {
+    if (it->first->guid == device_info->guid) {
       size_t index = it - devices_.begin();
       DCHECK_GT(device_name_map_[it->second], 0);
       if (--device_name_map_[it->second] == 0)
@@ -199,9 +207,11 @@ void UsbChooserController::OnDeviceRemoved(scoped_refptr<UsbDevice> device) {
 void UsbChooserController::GotUsbDeviceList(
     const std::vector<scoped_refptr<UsbDevice>>& devices) {
   for (const auto& device : devices) {
-    if (DisplayDevice(device)) {
-      base::string16 device_name = FormatUsbDeviceName(device);
-      devices_.push_back(std::make_pair(device, device_name));
+    auto device_info = device::mojom::UsbDeviceInfo::From(*device);
+    DCHECK(device_info);
+    if (DisplayDevice(*device_info)) {
+      base::string16 device_name = FormatUsbDeviceName(*device_info);
+      devices_.push_back(std::make_pair(std::move(device_info), device_name));
       ++device_name_map_[device_name];
     }
   }
@@ -210,11 +220,11 @@ void UsbChooserController::GotUsbDeviceList(
 }
 
 bool UsbChooserController::DisplayDevice(
-    scoped_refptr<UsbDevice> device) const {
-  if (!UsbDeviceFilterMatchesAny(filters_, *device))
+    const device::mojom::UsbDeviceInfo& device_info) const {
+  if (!device::UsbDeviceFilterMatchesAny(filters_, device_info))
     return false;
 
-  if (UsbBlocklist::Get().IsExcluded(device))
+  if (UsbBlocklist::Get().IsExcluded(device_info))
     return false;
 
   return true;

chrome/browser/usb/usb_chooser_controller.h

@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
 #include "chrome/browser/chooser_controller/chooser_controller.h"
+#include "device/usb/public/mojom/device.mojom.h"
 #include "device/usb/usb_service.h"
 #include "third_party/blink/public/mojom/usb/web_usb_service.mojom.h"
 #include "url/gurl.h"
@@ -58,7 +59,7 @@ class UsbChooserController : public ChooserController,
  private:
   void GotUsbDeviceList(
       const std::vector<scoped_refptr<device::UsbDevice>>& devices);
-  bool DisplayDevice(scoped_refptr<device::UsbDevice> device) const;
+  bool DisplayDevice(const device::mojom::UsbDeviceInfo& device) const;
 
   std::vector<device::mojom::UsbDeviceFilterPtr> filters_;
   blink::mojom::WebUsbService::GetPermissionCallback callback_;
@@ -71,7 +72,7 @@ class UsbChooserController : public ChooserController,
       usb_service_observer_;
 
   // Each pair is a (device, device name).
-  std::vector<std::pair<scoped_refptr<device::UsbDevice>, base::string16>>
+  std::vector<std::pair<device::mojom::UsbDeviceInfoPtr, base::string16>>
       devices_;
   // Maps from device name to number of devices.
   std::unordered_map<base::string16, int> device_name_map_;

chrome/browser/usb/web_usb_service_impl_unittest.cc

@@ -21,7 +21,8 @@
 #include "device/base/mock_device_client.h"
 #include "device/usb/mock_usb_device.h"
 #include "device/usb/mock_usb_service.h"
-#include "device/usb/mojo/device_impl.h"
+#include "device/usb/mojo/type_converters.h"
+#include "device/usb/public/mojom/device.mojom.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -35,6 +36,7 @@ using device::mojom::UsbDeviceInfoPtr;
 using device::mojom::UsbDeviceManagerClient;
 using device::mojom::UsbDeviceManagerClientPtr;
 using device::MockUsbDevice;
+using device::UsbDevice;
 
 namespace {
 
@@ -68,9 +70,9 @@ class WebUsbServiceImplTest : public ChromeRenderViewHostTestHarness {
 
   void GrantDevicePermission(const GURL& requesting_origin,
                              const GURL& embedding_origin,
-                             const std::string& guid) {
+                             const device::mojom::UsbDeviceInfo& device_info) {
     UsbChooserContextFactory::GetForProfile(profile())->GrantDevicePermission(
-        requesting_origin, embedding_origin, guid);
+        requesting_origin, embedding_origin, device_info);
   }
 
   device::MockDeviceClient device_client_;
@@ -121,17 +123,20 @@ void ExpectDevicesAndThen(const std::set<std::string>& expected_guids,
 TEST_F(WebUsbServiceImplTest, NoPermissionDevice) {
   GURL origin(kDefaultTestUrl);
 
-  scoped_refptr<MockUsbDevice> device0 =
+  scoped_refptr<UsbDevice> device0 =
       new MockUsbDevice(0x1234, 0x5678, "ACME", "Frobinator", "ABCDEF");
-  scoped_refptr<MockUsbDevice> device1 =
+  scoped_refptr<UsbDevice> device1 =
       new MockUsbDevice(0x1234, 0x5679, "ACME", "Frobinator+", "GHIJKL");
-  scoped_refptr<MockUsbDevice> no_permission_device1 =
+  scoped_refptr<UsbDevice> no_permission_device1 =
       new MockUsbDevice(0xffff, 0x567b, "ACME", "Frobinator II", "MNOPQR");
-  scoped_refptr<MockUsbDevice> no_permission_device2 =
+  scoped_refptr<UsbDevice> no_permission_device2 =
       new MockUsbDevice(0xffff, 0x567c, "ACME", "Frobinator Xtreme", "STUVWX");
 
+  auto device_info_0 = device::mojom::UsbDeviceInfo::From(*device0);
+  auto device_info_1 = device::mojom::UsbDeviceInfo::From(*device1);
+
   device_client_.usb_service()->AddDevice(device0);
-  GrantDevicePermission(origin, origin, device0->guid());
+  GrantDevicePermission(origin, origin, *device_info_0);
   device_client_.usb_service()->AddDevice(no_permission_device1);
 
   WebUsbServicePtr web_usb_service = ConnectToService();
@@ -152,7 +157,7 @@ TEST_F(WebUsbServiceImplTest, NoPermissionDevice) {
   }
 
   device_client_.usb_service()->AddDevice(device1);
-  GrantDevicePermission(origin, origin, device1->guid());
+  GrantDevicePermission(origin, origin, *device_info_1);
   device_client_.usb_service()->AddDevice(no_permission_device2);
   device_client_.usb_service()->RemoveDevice(device0);
   device_client_.usb_service()->RemoveDevice(device1);

device/usb/public/cpp/filter_utils.cc

@@ -40,6 +40,42 @@ bool UsbDeviceFilterMatches(const mojom::UsbDeviceFilter& filter,
   return true;
 }
 
+bool UsbDeviceFilterMatches(const mojom::UsbDeviceFilter& filter,
+                            const mojom::UsbDeviceInfo& device_info) {
+  if (filter.has_vendor_id) {
+    if (device_info.vendor_id != filter.vendor_id)
+      return false;
+
+    if (filter.has_product_id && device_info.product_id != filter.product_id)
+      return false;
+  }
+
+  if (filter.serial_number &&
+      device_info.serial_number != *filter.serial_number) {
+    return false;
+  }
+
+  if (filter.has_class_code) {
+    for (auto& config : device_info.configurations) {
+      for (auto& iface : config->interfaces) {
+        for (auto& alternate_info : iface->alternates) {
+          if (alternate_info->class_code == filter.class_code &&
+              (!filter.has_subclass_code ||
+               (alternate_info->subclass_code == filter.subclass_code &&
+                (!filter.has_protocol_code ||
+                 alternate_info->protocol_code == filter.protocol_code)))) {
+            return true;
+          }
+        }
+      }
+    }
+
+    return false;
+  }
+
+  return true;
+}
+
 bool UsbDeviceFilterMatchesAny(
     const std::vector<mojom::UsbDeviceFilterPtr>& filters,
     const UsbDevice& device) {
@@ -53,4 +89,17 @@ bool UsbDeviceFilterMatchesAny(
   return false;
 }
 
+bool UsbDeviceFilterMatchesAny(
+    const std::vector<mojom::UsbDeviceFilterPtr>& filters,
+    const mojom::UsbDeviceInfo& device_info) {
+  if (filters.empty())
+    return true;
+
+  for (const auto& filter : filters) {
+    if (UsbDeviceFilterMatches(*filter, device_info))
+      return true;
+  }
+  return false;
+}
+
 }  // namespace device

device/usb/public/cpp/filter_utils.h

@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "device/usb/public/mojom/device.mojom.h"
 #include "device/usb/public/mojom/device_manager.mojom.h"
 
 namespace device {
@@ -16,10 +17,17 @@ class UsbDevice;
 bool UsbDeviceFilterMatches(const mojom::UsbDeviceFilter& filter,
                             const UsbDevice& device);
 
+bool UsbDeviceFilterMatches(const mojom::UsbDeviceFilter& filter,
+                            const mojom::UsbDeviceInfo& device_info);
+
 bool UsbDeviceFilterMatchesAny(
     const std::vector<mojom::UsbDeviceFilterPtr>& filters,
     const UsbDevice& device);
 
+bool UsbDeviceFilterMatchesAny(
+    const std::vector<mojom::UsbDeviceFilterPtr>& filters,
+    const mojom::UsbDeviceInfo& device_info);
+
 }  // namespace device
 
 #endif  // DEVICE_USB_PUBLIC_CPP_FILTER_UTILS_H_