Close connection to HID when the user revokes permission

Bug: 1126689
Change-Id: I23b1ee8095a7f7ee14ab589e82e5e7e7a1a086b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2414292Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812650}

chrome/browser/hid/chrome_hid_delegate.cc

@@ -95,6 +95,15 @@ void ChromeHidDelegate::RemoveObserver(
   observer_list_.RemoveObserver(observer);
 }
 
+const device::mojom::HidDeviceInfo* ChromeHidDelegate::GetDeviceInfo(
+    content::WebContents* web_contents,
+    const std::string& guid) {
+  auto* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  auto* chooser_context = HidChooserContextFactory::GetForProfile(profile);
+  return chooser_context->GetDeviceInfo(guid);
+}
+
 void ChromeHidDelegate::OnPermissionRevoked(
     const url::Origin& requesting_origin,
     const url::Origin& embedding_origin) {

chrome/browser/hid/chrome_hid_delegate.h

@@ -40,6 +40,9 @@ class ChromeHidDelegate
                    content::HidDelegate::Observer* observer) override;
   void RemoveObserver(content::RenderFrameHost* frame,
                       content::HidDelegate::Observer* observer) override;
+  const device::mojom::HidDeviceInfo* GetDeviceInfo(
+      content::WebContents* web_contents,
+      const std::string& guid) override;
 
   // permissions::ChooserContextBase::PermissionObserver:
   void OnPermissionRevoked(const url::Origin& requesting_origin,

chrome/browser/hid/hid_chooser_context.cc

@@ -251,6 +251,13 @@ void HidChooserContext::GetDevices(
       FROM_HERE, base::BindOnce(std::move(callback), std::move(device_list)));
 }
 
+const device::mojom::HidDeviceInfo* HidChooserContext::GetDeviceInfo(
+    const std::string& guid) {
+  DCHECK(is_initialized_);
+  auto it = devices_.find(guid);
+  return it == devices_.end() ? nullptr : it->second.get();
+}
+
 device::mojom::HidManager* HidChooserContext::GetHidManager() {
   EnsureHidManagerConnection();
   return hid_manager_.get();
@@ -344,6 +351,8 @@ void HidChooserContext::InitDeviceList(
   for (auto& device : devices)
     devices_.insert({device->guid, std::move(device)});
 
+  is_initialized_ = true;
+
   while (!pending_get_devices_requests_.empty()) {
     std::vector<device::mojom::HidDeviceInfoPtr> device_list;
     device_list.reserve(devices.size());

chrome/browser/hid/hid_chooser_context.h

@@ -79,6 +79,11 @@ class HidChooserContext : public permissions::ChooserContextBase,
   // Forward HidManager::GetDevices.
   void GetDevices(device::mojom::HidManager::GetDevicesCallback callback);
 
+  // Only call this if you're sure |devices_| has been initialized before-hand.
+  // The returned raw pointer is owned by |devices_| and will be destroyed when
+  // the device is removed.
+  const device::mojom::HidDeviceInfo* GetDeviceInfo(const std::string& guid);
+
   device::mojom::HidManager* GetHidManager();
 
   // Sets |manager| as the HidManager and registers this context as a

content/browser/hid/hid_service.cc

@@ -23,9 +23,13 @@ namespace content {
 
 HidService::HidService(RenderFrameHost* render_frame_host,
                        mojo::PendingReceiver<blink::mojom::HidService> receiver)
-    : FrameServiceBase(render_frame_host, std::move(receiver)) {
-  watchers_.set_disconnect_handler(base::BindRepeating(
-      &HidService::OnWatcherConnectionError, base::Unretained(this)));
+    : FrameServiceBase(render_frame_host, std::move(receiver)),
+      requesting_origin_(render_frame_host->GetLastCommittedOrigin()),
+      embedding_origin_(
+          render_frame_host->GetMainFrame()->GetLastCommittedOrigin()) {
+  watchers_.set_disconnect_handler(
+      base::BindRepeating(&HidService::OnWatcherRemoved, base::Unretained(this),
+                          true /* cleanup_watcher_ids */));
 
   HidDelegate* delegate = GetContentClient()->browser()->GetHidDelegate();
   if (delegate)
@@ -107,7 +111,10 @@ void HidService::Connect(
   }
 
   mojo::PendingRemote<device::mojom::HidConnectionWatcher> watcher;
-  watchers_.Add(this, watcher.InitWithNewPipeAndPassReceiver());
+  mojo::ReceiverId receiver_id =
+      watchers_.Add(this, watcher.InitWithNewPipeAndPassReceiver());
+  watcher_ids_.insert({device_guid, receiver_id});
+
   GetContentClient()
       ->browser()
       ->GetHidDelegate()
@@ -118,9 +125,16 @@ void HidService::Connect(
                          std::move(callback)));
 }
 
-void HidService::OnWatcherConnectionError() {
+void HidService::OnWatcherRemoved(bool cleanup_watcher_ids) {
   if (watchers_.empty())
     DecrementActiveFrameCount();
+
+  if (cleanup_watcher_ids) {
+    // Clean up any associated |watchers_ids_| entries.
+    base::EraseIf(watcher_ids_, [&](const auto& watcher_entry) {
+      return watcher_entry.second == watchers_.current_receiver();
+    });
+  }
 }
 
 void HidService::DecrementActiveFrameCount() {
@@ -165,8 +179,26 @@ void HidService::OnPermissionRevoked(const url::Origin& requesting_origin,
     return;
   }
 
-  // TODO(mattreynolds): Close connection between Blink and the device if the
-  // device lost permission.
+  HidDelegate* delegate = GetContentClient()->browser()->GetHidDelegate();
+  WebContents* web_contents =
+      WebContents::FromRenderFrameHost(render_frame_host());
+
+  base::EraseIf(watcher_ids_, [&](const auto& watcher_entry) {
+    const auto* device_info =
+        delegate->GetDeviceInfo(web_contents, watcher_entry.first);
+    if (!device_info)
+      return true;
+
+    if (delegate->HasDevicePermission(web_contents, origin(), *device_info)) {
+      return false;
+    }
+
+    watchers_.Remove(watcher_entry.second);
+    return true;
+  });
+
+  // If needed decrement the active frame count.
+  OnWatcherRemoved(false /* cleanup_watcher_ids */);
 }
 
 void HidService::FinishGetDevices(

content/browser/hid/hid_service.h

@@ -60,7 +60,7 @@ class HidService : public content::FrameServiceBase<blink::mojom::HidService>,
   HidService(RenderFrameHost*, mojo::PendingReceiver<blink::mojom::HidService>);
   ~HidService() override;
 
-  void OnWatcherConnectionError();
+  void OnWatcherRemoved(bool cleanup_watcher_ids);
   void DecrementActiveFrameCount();
 
   void FinishGetDevices(GetDevicesCallback callback,
@@ -84,6 +84,10 @@ class HidService : public content::FrameServiceBase<blink::mojom::HidService>,
   // the WebContentsImpl when an active connection indicator should be shown.
   mojo::ReceiverSet<device::mojom::HidConnectionWatcher> watchers_;
 
+  // Maps every receiver to a guid to allow closing particular connections when
+  // the user revokes a permission.
+  std::multimap<std::string, mojo::ReceiverId> watcher_ids_;
+
   base::WeakPtrFactory<HidService> weak_factory_{this};
 };
 

content/browser/hid/hid_service_unittest.cc

@@ -331,4 +331,56 @@ TEST_F(HidServiceTest, RegisterClient) {
   device_removed_loop.Run();
 }
 
+TEST_F(HidServiceTest, RevokeDevicePermission) {
+  NavigateAndCommit(GURL(kTestUrl));
+
+  mojo::Remote<blink::mojom::HidService> service;
+  contents()->GetMainFrame()->GetHidService(
+      service.BindNewPipeAndPassReceiver());
+
+  // For now the device has permission.
+  EXPECT_CALL(hid_delegate(), HasDevicePermission).WillOnce(Return(true));
+
+  // Create a new device.
+  auto device_info = device::mojom::HidDeviceInfo::New();
+  device_info->guid = kTestGuid;
+  ConnectDevice(*device_info);
+  EXPECT_CALL(hid_delegate(), GetDeviceInfo)
+      .WillOnce(Return(device_info.get()));
+
+  // Connect the device.
+  mojo::PendingRemote<device::mojom::HidConnectionClient> hid_connection_client;
+  connection_client()->Bind(
+      hid_connection_client.InitWithNewPipeAndPassReceiver());
+
+  EXPECT_FALSE(contents()->IsConnectedToHidDevice());
+
+  base::RunLoop run_loop;
+  mojo::Remote<device::mojom::HidConnection> connection;
+  service->Connect(
+      kTestGuid, std::move(hid_connection_client),
+      base::BindLambdaForTesting(
+          [&run_loop,
+           &connection](mojo::PendingRemote<device::mojom::HidConnection> c) {
+            connection.Bind(std::move(c));
+            run_loop.Quit();
+          }));
+  run_loop.Run();
+
+  EXPECT_TRUE(contents()->IsConnectedToHidDevice());
+  EXPECT_TRUE(connection);
+
+  // Simulate user revoking permission.
+  EXPECT_CALL(hid_delegate(), HasDevicePermission).WillOnce(Return(false));
+  url::Origin origin = url::Origin::Create(GURL(kTestUrl));
+  hid_delegate().OnPermissionRevoked(origin, origin);
+
+  // TODO(mattreynolds): Use a disconnect handler with a run loop instead of the
+  // potentially flaky `RunUntilIdle`. This depends on fixing
+  // `FakeHidConnection` to monitor the watcher just as `HidConnectionImpl`
+  // does.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(contents()->IsConnectedToHidDevice());
+}
+
 }  // namespace content

content/browser/hid/hid_test_utils.cc

@@ -44,6 +44,12 @@ void MockHidDelegate::OnDeviceRemoved(
     observer.OnDeviceRemoved(device);
 }
 
+void MockHidDelegate::OnPermissionRevoked(const url::Origin& requesting_origin,
+                                          const url::Origin& embedding_origin) {
+  for (auto& observer : observer_list_)
+    observer.OnPermissionRevoked(requesting_origin, embedding_origin);
+}
+
 HidTestContentBrowserClient::HidTestContentBrowserClient() = default;
 
 HidTestContentBrowserClient::~HidTestContentBrowserClient() = default;

content/browser/hid/hid_test_utils.h

@@ -14,6 +14,7 @@
 #include "services/device/public/mojom/hid.mojom-forward.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/blink/public/mojom/hid/hid.mojom-forward.h"
+#include "url/origin.h"
 
 namespace content {
 
@@ -41,6 +42,8 @@ class MockHidDelegate : public HidDelegate {
   // these methods to broadcast device connections to all delegate observers.
   void OnDeviceAdded(const device::mojom::HidDeviceInfo& device);
   void OnDeviceRemoved(const device::mojom::HidDeviceInfo& device);
+  void OnPermissionRevoked(const url::Origin& requesting_origin,
+                           const url::Origin& embedding_origin);
 
   MOCK_METHOD0(RunChooserInternal,
                std::vector<device::mojom::HidDeviceInfoPtr>());
@@ -53,6 +56,10 @@ class MockHidDelegate : public HidDelegate {
                     const device::mojom::HidDeviceInfo& device));
   MOCK_METHOD1(GetHidManager,
                device::mojom::HidManager*(content::WebContents* web_contents));
+  MOCK_METHOD2(
+      GetDeviceInfo,
+      const device::mojom::HidDeviceInfo*(content::WebContents* web_contents,
+                                          const std::string& guid));
 
  private:
   base::ObserverList<Observer> observer_list_;

content/public/browser/hid_delegate.h

@@ -76,6 +76,11 @@ class CONTENT_EXPORT HidDelegate {
   // object.
   virtual void AddObserver(RenderFrameHost* frame, Observer* observer) = 0;
   virtual void RemoveObserver(RenderFrameHost* frame, Observer* observer) = 0;
+
+  // Gets the device info for a particular device, identified by its guid.
+  virtual const device::mojom::HidDeviceInfo* GetDeviceInfo(
+      content::WebContents* web_contents,
+      const std::string& guid) = 0;
 };
 
 }  // namespace content