Move permission checking from UsbDeviceManager to WebUsbService.

This CL moves permission checking from device/usb/mojo directory to
chrome/browser/usb. This is a preparation step for moving device/usb
to service/device.

BUG=699790

Change-Id: Ic30d6ff94b670950fcb2571d7036dd7c0f39a24b
Reviewed-on: https://chromium-review.googlesource.com/1172154
Commit-Queue: Donna Wu <donna.wu@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583979}

chrome/browser/usb/usb_blocklist.cc

@@ -10,6 +10,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "components/variations/variations_associated_data.h"
+#include "device/usb/public/mojom/device.mojom.h"
 #include "device/usb/usb_device.h"
 
 namespace {
@@ -129,6 +130,15 @@ bool UsbBlocklist::IsExcluded(
                           device->device_version()));
 }
 
+bool UsbBlocklist::IsExcluded(
+    const device::mojom::UsbDeviceInfo& device_info) const {
+  uint16_t device_version = device_info.device_version_major << 8 |
+                            device_info.device_version_minor << 4 |
+                            device_info.device_version_subminor;
+  return IsExcluded(
+      Entry(device_info.vendor_id, device_info.product_id, device_version));
+}
+
 void UsbBlocklist::ResetToDefaultValuesForTest() {
   dynamic_entries_.clear();
   PopulateWithServerProvidedValues();

chrome/browser/usb/usb_blocklist.h

@@ -15,6 +15,10 @@
 
 namespace device {
 class UsbDevice;
+
+namespace mojom {
+class UsbDeviceInfo;
+}
 }
 
 class UsbBlocklist final {
@@ -43,6 +47,7 @@ class UsbBlocklist final {
   // Returns if a device is excluded from access.
   bool IsExcluded(const Entry& entry) const;
   bool IsExcluded(const scoped_refptr<const device::UsbDevice>& device) const;
+  bool IsExcluded(const device::mojom::UsbDeviceInfo& device_info) const;
 
   // Size of the blocklist.
   size_t GetDynamicEntryCountForTest() const { return dynamic_entries_.size(); }

chrome/browser/usb/usb_chooser_context.cc

@@ -13,6 +13,7 @@
 #include "base/values.h"
 #include "chrome/browser/profiles/profile.h"
 #include "device/base/device_client.h"
+#include "device/usb/mojo/type_converters.h"
 #include "device/usb/public/mojom/device.mojom.h"
 #include "device/usb/usb_device.h"
 
@@ -163,14 +164,14 @@ void UsbChooserContext::GrantDevicePermission(const GURL& requesting_origin,
 bool UsbChooserContext::HasDevicePermission(
     const GURL& requesting_origin,
     const GURL& embedding_origin,
-    scoped_refptr<const device::UsbDevice> device) {
+    const device::mojom::UsbDeviceInfo& device_info) {
   if (!CanRequestObjectPermission(requesting_origin, embedding_origin))
     return false;
 
   auto it = ephemeral_devices_.find(
       std::make_pair(requesting_origin, embedding_origin));
   if (it != ephemeral_devices_.end() &&
-      base::ContainsKey(it->second, device->guid())) {
+      base::ContainsKey(it->second, device_info.guid)) {
     return true;
   }
 
@@ -182,11 +183,11 @@ bool UsbChooserContext::HasDevicePermission(
     int product_id;
     base::string16 serial_number;
     if (device_dict->GetInteger(kVendorIdKey, &vendor_id) &&
-        device->vendor_id() == vendor_id &&
+        device_info.vendor_id == vendor_id &&
         device_dict->GetInteger(kProductIdKey, &product_id) &&
-        device->product_id() == product_id &&
+        device_info.product_id == product_id &&
         device_dict->GetString(kSerialNumberKey, &serial_number) &&
-        device->serial_number() == serial_number) {
+        device_info.serial_number == serial_number) {
       return true;
     }
   }
@@ -194,6 +195,20 @@ bool UsbChooserContext::HasDevicePermission(
   return false;
 }
 
+bool UsbChooserContext::HasDevicePermission(
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    scoped_refptr<const device::UsbDevice> device) {
+  if (!device)
+    return false;
+
+  device::mojom::UsbDeviceInfoPtr device_info =
+      device::mojom::UsbDeviceInfo::From(*device);
+  DCHECK(device_info);
+
+  return HasDevicePermission(requesting_origin, embedding_origin, *device_info);
+}
+
 base::WeakPtr<UsbChooserContext> UsbChooserContext::AsWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }

chrome/browser/usb/usb_chooser_context.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_USB_USB_CHOOSER_CONTEXT_H_
 
 #include <map>
+#include <memory>
 #include <set>
 #include <string>
 #include <utility>
@@ -18,6 +19,10 @@
 
 namespace device {
 class UsbDevice;
+
+namespace mojom {
+class UsbDeviceInfo;
+}
 }
 
 class UsbChooserContext : public ChooserContextBase,
@@ -45,6 +50,10 @@ class UsbChooserContext : public ChooserContextBase,
 
   // Checks if |requesting_origin| (when embedded within |embedding_origin| has
   // access to a device with |device_info|.
+  bool HasDevicePermission(const GURL& requesting_origin,
+                           const GURL& embedding_origin,
+                           const device::mojom::UsbDeviceInfo& device_info);
+
   bool HasDevicePermission(const GURL& requesting_origin,
                            const GURL& embedding_origin,
                            scoped_refptr<const device::UsbDevice> device);

chrome/browser/usb/usb_chooser_controller.cc

@@ -25,7 +25,6 @@
 #include "device/usb/mojo/type_converters.h"
 #include "device/usb/public/cpp/filter_utils.h"
 #include "device/usb/usb_device.h"
-#include "device/usb/usb_ids.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
@@ -125,13 +124,15 @@ base::string16 UsbChooserController::GetOption(size_t index) const {
 }
 
 bool UsbChooserController::IsPaired(size_t index) const {
-  scoped_refptr<UsbDevice> device = devices_[index].first;
-
   if (!chooser_context_)
     return false;
 
+  auto device_info = device::mojom::UsbDeviceInfo::From(*devices_[index].first);
+  DCHECK(device_info);
+
   return WebUSBPermissionProvider::HasDevicePermission(
-      chooser_context_.get(), requesting_origin_, embedding_origin_, device);
+      chooser_context_.get(), requesting_origin_, embedding_origin_,
+      *device_info);
 }
 
 void UsbChooserController::Select(const std::vector<size_t>& indices) {

chrome/browser/usb/web_usb_permission_provider.cc

@@ -16,7 +16,6 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
-#include "device/usb/usb_device.h"
 
 using content::RenderFrameHost;
 using content::WebContents;
@@ -26,14 +25,14 @@ bool WebUSBPermissionProvider::HasDevicePermission(
     UsbChooserContext* chooser_context,
     const GURL& requesting_origin,
     const GURL& embedding_origin,
-    scoped_refptr<const device::UsbDevice> device) {
+    const device::mojom::UsbDeviceInfo& device_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (UsbBlocklist::Get().IsExcluded(device))
+  if (UsbBlocklist::Get().IsExcluded(device_info))
     return false;
 
   return chooser_context->HasDevicePermission(requesting_origin,
-                                              embedding_origin, device);
+                                              embedding_origin, device_info);
 }
 
 WebUSBPermissionProvider::WebUSBPermissionProvider(
@@ -51,7 +50,7 @@ WebUSBPermissionProvider::GetWeakPtr() {
 }
 
 bool WebUSBPermissionProvider::HasDevicePermission(
-    scoped_refptr<const device::UsbDevice> device) const {
+    const device::mojom::UsbDeviceInfo& device_info) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   WebContents* web_contents =
@@ -63,7 +62,7 @@ bool WebUSBPermissionProvider::HasDevicePermission(
   return HasDevicePermission(
       UsbChooserContextFactory::GetForProfile(profile),
       render_frame_host_->GetLastCommittedURL().GetOrigin(),
-      main_frame->GetLastCommittedURL().GetOrigin(), device);
+      main_frame->GetLastCommittedURL().GetOrigin(), device_info);
 }
 
 void WebUSBPermissionProvider::IncrementConnectionCount() {

chrome/browser/usb/web_usb_permission_provider.h

@@ -14,6 +14,12 @@ namespace content {
 class RenderFrameHost;
 }
 
+namespace device {
+namespace mojom {
+class UsbDeviceInfo;
+}
+}  // namespace device
+
 class GURL;
 class UsbChooserContext;
 
@@ -27,7 +33,7 @@ class WebUSBPermissionProvider : public device::usb::PermissionProvider {
       UsbChooserContext* chooser_context,
       const GURL& requesting_origin,
       const GURL& embedding_origin,
-      scoped_refptr<const device::UsbDevice> device);
+      const device::mojom::UsbDeviceInfo& device_info);
 
   explicit WebUSBPermissionProvider(
       content::RenderFrameHost* render_frame_host);
@@ -37,7 +43,7 @@ class WebUSBPermissionProvider : public device::usb::PermissionProvider {
 
   // device::usb::PermissionProvider implementation.
   bool HasDevicePermission(
-      scoped_refptr<const device::UsbDevice> device) const override;
+      const device::mojom::UsbDeviceInfo& device_info) const override;
   void IncrementConnectionCount() override;
   void DecrementConnectionCount() override;
 

chrome/browser/usb/web_usb_service_impl.cc

@@ -34,8 +34,7 @@ WebUsbServiceImpl::WebUsbServiceImpl(
   // Bind |device_manager_| to UsbDeviceManager and set error handler.
   // TODO(donna.wu@intel.com): Request UsbDeviceManagerPtr from the Device
   // Service after moving //device/usb to //services/device.
-  device::usb::DeviceManagerImpl::Create(permission_provider_,
-                                         mojo::MakeRequest(&device_manager_));
+  device::usb::DeviceManagerImpl::Create(mojo::MakeRequest(&device_manager_));
   device_manager_.set_connection_error_handler(base::BindOnce(
       &WebUsbServiceImpl::OnConnectionError, base::Unretained(this)));
   // Listen for add/remove device events from UsbService.
@@ -52,14 +51,30 @@ WebUsbServiceImpl::WebUsbServiceImpl(
 WebUsbServiceImpl::~WebUsbServiceImpl() = default;
 
 void WebUsbServiceImpl::GetDevices(GetDevicesCallback callback) {
-  device_manager_->GetDevices(nullptr, std::move(callback));
+  device_manager_->GetDevices(
+      nullptr, base::BindOnce(&WebUsbServiceImpl::OnGetDevices,
+                              weak_factory_.GetWeakPtr(), std::move(callback)));
+}
+
+void WebUsbServiceImpl::OnGetDevices(
+    GetDevicesCallback callback,
+    std::vector<device::mojom::UsbDeviceInfoPtr> device_info_list) {
+  std::vector<device::mojom::UsbDeviceInfoPtr> device_infos;
+  for (auto& device_info : device_info_list) {
+    if (device_info && permission_provider_ &&
+        permission_provider_->HasDevicePermission(*device_info)) {
+      device_infos.push_back(device_info.Clone());
+    }
+  }
+  std::move(callback).Run(std::move(device_infos));
 }
 
 void WebUsbServiceImpl::GetDevice(
     const std::string& guid,
     device::mojom::UsbDeviceRequest device_request) {
   // Try to bind with the new device to be created for DeviceOpened/Closed
-  // events.
+  // events. It is safe to pass this request directly to |device_manager_|
+  // because |guid| is unguessable.
   device::mojom::UsbDeviceClientPtr device_client;
   device_client_bindings_.AddBinding(this, mojo::MakeRequest(&device_client));
   device_manager_->GetDevice(guid, std::move(device_request),
@@ -81,17 +96,21 @@ void WebUsbServiceImpl::SetClient(
 }
 
 void WebUsbServiceImpl::OnDeviceAdded(scoped_refptr<device::UsbDevice> device) {
+  auto device_info = device::mojom::UsbDeviceInfo::From(*device);
+  DCHECK(device_info);
   if (client_ && permission_provider_ &&
-      permission_provider_->HasDevicePermission(device)) {
-    client_->OnDeviceAdded(device::mojom::UsbDeviceInfo::From(*device));
+      permission_provider_->HasDevicePermission(*device_info)) {
+    client_->OnDeviceAdded(std::move(device_info));
   }
 }
 
 void WebUsbServiceImpl::OnDeviceRemoved(
     scoped_refptr<device::UsbDevice> device) {
+  auto device_info = device::mojom::UsbDeviceInfo::From(*device);
+  DCHECK(device_info);
   if (client_ && permission_provider_ &&
-      permission_provider_->HasDevicePermission(device)) {
-    client_->OnDeviceRemoved(device::mojom::UsbDeviceInfo::From(*device));
+      permission_provider_->HasDevicePermission(*device_info)) {
+    client_->OnDeviceRemoved(std::move(device_info));
   }
 }
 

chrome/browser/usb/web_usb_service_impl.h

@@ -52,6 +52,10 @@ class WebUsbServiceImpl : public blink::mojom::WebUsbService,
       GetPermissionCallback callback) override;
   void SetClient(device::mojom::UsbDeviceManagerClientPtr client) override;
 
+  void OnGetDevices(
+      GetDevicesCallback callback,
+      std::vector<device::mojom::UsbDeviceInfoPtr> device_info_list);
+
   // device::UsbService::Observer implementation:
   void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override;
   void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override;

device/usb/mojo/device_manager_impl.cc

@@ -15,7 +15,6 @@
 #include "base/memory/ptr_util.h"
 #include "device/base/device_client.h"
 #include "device/usb/mojo/device_impl.h"
-#include "device/usb/mojo/permission_provider.h"
 #include "device/usb/mojo/type_converters.h"
 #include "device/usb/public/cpp/filter_utils.h"
 #include "device/usb/public/mojom/device.mojom.h"
@@ -27,26 +26,19 @@ namespace usb {
 
 // static
 void DeviceManagerImpl::Create(
-    base::WeakPtr<PermissionProvider> permission_provider,
     mojom::UsbDeviceManagerRequest request) {
   DCHECK(DeviceClient::Get());
   UsbService* service = DeviceClient::Get()->GetUsbService();
   if (!service)
     return;
 
-  auto* device_manager_impl =
-      new DeviceManagerImpl(std::move(permission_provider), service);
+  auto* device_manager_impl = new DeviceManagerImpl(service);
   device_manager_impl->binding_ = mojo::MakeStrongBinding(
       base::WrapUnique(device_manager_impl), std::move(request));
 }
 
-DeviceManagerImpl::DeviceManagerImpl(
-    base::WeakPtr<PermissionProvider> permission_provider,
-    UsbService* usb_service)
-    : permission_provider_(permission_provider),
-      usb_service_(usb_service),
-      observer_(this),
-      weak_factory_(this) {
+DeviceManagerImpl::DeviceManagerImpl(UsbService* usb_service)
+    : usb_service_(usb_service), observer_(this), weak_factory_(this) {
   // This object owns itself and will be destroyed if the message pipe it is
   // bound to is closed, the message loop is destructed, or the UsbService is
   // shut down.
@@ -69,11 +61,8 @@ void DeviceManagerImpl::GetDevice(const std::string& guid,
   if (!device)
     return;
 
-  if (permission_provider_ &&
-      permission_provider_->HasDevicePermission(device)) {
   DeviceImpl::Create(std::move(device), std::move(device_request),
                      std::move(device_client));
-  }
 }
 
 void DeviceManagerImpl::SetClient(mojom::UsbDeviceManagerClientPtr client) {
@@ -91,25 +80,20 @@ void DeviceManagerImpl::OnGetDevices(
   std::vector<mojom::UsbDeviceInfoPtr> device_infos;
   for (const auto& device : devices) {
     if (UsbDeviceFilterMatchesAny(filters, *device)) {
-      if (permission_provider_ &&
-          permission_provider_->HasDevicePermission(device)) {
       device_infos.push_back(mojom::UsbDeviceInfo::From(*device));
     }
   }
-  }
 
   std::move(callback).Run(std::move(device_infos));
 }
 
 void DeviceManagerImpl::OnDeviceAdded(scoped_refptr<UsbDevice> device) {
-  if (client_ && permission_provider_ &&
-      permission_provider_->HasDevicePermission(device))
+  if (client_)
     client_->OnDeviceAdded(mojom::UsbDeviceInfo::From(*device));
 }
 
 void DeviceManagerImpl::OnDeviceRemoved(scoped_refptr<UsbDevice> device) {
-  if (client_ && permission_provider_ &&
-      permission_provider_->HasDevicePermission(device))
+  if (client_)
     client_->OnDeviceRemoved(mojom::UsbDeviceInfo::From(*device));
 }
 

device/usb/mojo/device_manager_impl.h

@@ -8,6 +8,8 @@
 #include <memory>
 #include <queue>
 #include <set>
+#include <string>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
@@ -24,21 +26,17 @@ class UsbDevice;
 
 namespace usb {
 
-class PermissionProvider;
-
 // Implements the public Mojo UsbDeviceManager interface by wrapping the
 // UsbService instance.
 class DeviceManagerImpl : public mojom::UsbDeviceManager,
                           public UsbService::Observer {
  public:
-  static void Create(base::WeakPtr<PermissionProvider> permission_provider,
-                     mojom::UsbDeviceManagerRequest request);
+  static void Create(mojom::UsbDeviceManagerRequest request);
 
   ~DeviceManagerImpl() override;
 
  private:
-  DeviceManagerImpl(base::WeakPtr<PermissionProvider> permission_provider,
-                    UsbService* usb_service);
+  explicit DeviceManagerImpl(UsbService* usb_service);
 
   // DeviceManager implementation:
   void GetDevices(mojom::UsbEnumerationOptionsPtr options,
@@ -61,7 +59,6 @@ class DeviceManagerImpl : public mojom::UsbDeviceManager,
   void MaybeRunDeviceChangesCallback();
 
   mojo::StrongBindingPtr<mojom::UsbDeviceManager> binding_;
-  base::WeakPtr<PermissionProvider> permission_provider_;
 
   UsbService* usb_service_;
   ScopedObserver<UsbService, UsbService::Observer> observer_;

device/usb/mojo/device_manager_impl_unittest.cc

@@ -22,7 +22,6 @@
 #include "device/usb/mock_usb_device_handle.h"
 #include "device/usb/mock_usb_service.h"
 #include "device/usb/mojo/device_impl.h"
-#include "device/usb/mojo/mock_permission_provider.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -56,15 +55,13 @@ class USBDeviceManagerImplTest : public testing::Test {
  protected:
   UsbDeviceManagerPtr ConnectToDeviceManager() {
     UsbDeviceManagerPtr device_manager;
-    DeviceManagerImpl::Create(permission_provider_.GetWeakPtr(),
-                              mojo::MakeRequest(&device_manager));
+    DeviceManagerImpl::Create(mojo::MakeRequest(&device_manager));
     return device_manager;
   }
 
   MockDeviceClient device_client_;
 
  private:
-  MockPermissionProvider permission_provider_;
   std::unique_ptr<base::MessageLoop> message_loop_;
 };
 

device/usb/mojo/mock_permission_provider.cc

@@ -23,8 +23,9 @@ MockPermissionProvider::MockPermissionProvider() : weak_factory_(this) {}
 MockPermissionProvider::~MockPermissionProvider() = default;
 
 bool MockPermissionProvider::HasDevicePermission(
-    scoped_refptr<const UsbDevice> device) const {
-  return device->serial_number() == base::ASCIIToUTF16(kRestrictedSerialNumber)
+    const mojom::UsbDeviceInfo& device_info) const {
+  return device_info.serial_number ==
+                 base::ASCIIToUTF16(kRestrictedSerialNumber)
              ? false
              : true;
 }

device/usb/mojo/mock_permission_provider.h

@@ -9,10 +9,14 @@
 
 #include "base/memory/weak_ptr.h"
 #include "device/usb/mojo/permission_provider.h"
-#include "device/usb/usb_device.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace device {
+
+namespace mojom {
+class UsbDeviceInfo;
+}
+
 namespace usb {
 
 class MockPermissionProvider : public PermissionProvider {
@@ -24,7 +28,7 @@ class MockPermissionProvider : public PermissionProvider {
 
   base::WeakPtr<PermissionProvider> GetWeakPtr();
   bool HasDevicePermission(
-      scoped_refptr<const UsbDevice> device) const override;
+      const mojom::UsbDeviceInfo& device_info) const override;
 
   MOCK_METHOD0(IncrementConnectionCount, void());
   MOCK_METHOD0(DecrementConnectionCount, void());

device/usb/mojo/permission_provider.h

@@ -5,11 +5,11 @@
 #ifndef DEVICE_USB_MOJO_PERMISSION_PROVIDER_H_
 #define DEVICE_USB_MOJO_PERMISSION_PROVIDER_H_
 
-#include "base/memory/ref_counted.h"
-
 namespace device {
 
-class UsbDevice;
+namespace mojom {
+class UsbDeviceInfo;
+}
 
 namespace usb {
 
@@ -21,7 +21,7 @@ class PermissionProvider {
   virtual ~PermissionProvider();
 
   virtual bool HasDevicePermission(
-      scoped_refptr<const UsbDevice> device) const = 0;
+      const mojom::UsbDeviceInfo& device_info) const = 0;
   virtual void IncrementConnectionCount() = 0;
   virtual void DecrementConnectionCount() = 0;
 };