diff options
author | reillyg <reillyg@chromium.org> | 2015-09-09 13:58:55 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-09-09 20:59:42 +0000 |
commit | 17b8a58c99537bab4d8944ec2f96023dac0a15e7 (patch) | |
tree | 681eeb93ae7f3adeea7e3c0309d5dcf26c5dbdc1 | |
parent | 60b43d0fb14581c647034594f29e98f4ec2babf5 (diff) | |
download | chromium_src-17b8a58c99537bab4d8944ec2f96023dac0a15e7.zip chromium_src-17b8a58c99537bab4d8944ec2f96023dac0a15e7.tar.gz chromium_src-17b8a58c99537bab4d8944ec2f96023dac0a15e7.tar.bz2 |
Convert DeviceManagerDelegate to PermissionProvider mojo interface.
This changes the device::usb::DeviceManager service so that instead of
expecting a C++ implementation of the DeviceManagerDelegate interface it
depends on the client connecting to the devices app to provide an
implementation of the PermissionProvider mojo service interface. This
will allow the permissions checker to be injected by the FrameMojoShell
when a renderer requests a connection to the devices app.
BUG=492204
Review URL: https://codereview.chromium.org/1316203006
Cr-Commit-Position: refs/heads/master@{#347998}
-rw-r--r-- | chrome/browser/chrome_content_browser_client.cc | 8 | ||||
-rw-r--r-- | chrome/browser/usb/DEPS | 3 | ||||
-rw-r--r-- | chrome/browser/usb/OWNERS | 2 | ||||
-rw-r--r-- | chrome/browser/usb/web_usb_permission_provider.cc | 38 | ||||
-rw-r--r-- | chrome/browser/usb/web_usb_permission_provider.h | 38 | ||||
-rw-r--r-- | chrome/chrome_browser.gypi | 2 | ||||
-rw-r--r-- | device/BUILD.gn | 1 | ||||
-rw-r--r-- | device/devices_app/BUILD.gn | 1 | ||||
-rw-r--r-- | device/devices_app/devices_app.cc | 35 | ||||
-rw-r--r-- | device/devices_app/devices_app.gyp | 2 | ||||
-rw-r--r-- | device/devices_app/usb/device_manager_impl.cc | 181 | ||||
-rw-r--r-- | device/devices_app/usb/device_manager_impl.h | 32 | ||||
-rw-r--r-- | device/devices_app/usb/device_manager_impl_unittest.cc | 40 | ||||
-rw-r--r-- | device/devices_app/usb/public/cpp/BUILD.gn | 15 | ||||
-rw-r--r-- | device/devices_app/usb/public/cpp/device_manager_delegate.h | 27 | ||||
-rw-r--r-- | device/devices_app/usb/public/interfaces/BUILD.gn | 1 | ||||
-rw-r--r-- | device/devices_app/usb/public/interfaces/device_manager.mojom | 9 | ||||
-rw-r--r-- | device/devices_app/usb/public/interfaces/permission_provider.mojom | 14 |
18 files changed, 282 insertions, 167 deletions
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc index c8d718f..0149527 100644 --- a/chrome/browser/chrome_content_browser_client.cc +++ b/chrome/browser/chrome_content_browser_client.cc @@ -259,6 +259,10 @@ #include "chrome/browser/media/router/presentation_service_delegate_impl.h" #endif +#if !defined(OS_ANDROID) && !defined(OS_IOS) +#include "chrome/browser/usb/web_usb_permission_provider.h" +#endif + using base::FileDescriptor; using blink::WebWindowFeatures; using content::AccessTokenStore; @@ -2516,6 +2520,10 @@ void ChromeContentBrowserClient::RegisterFrameMojoShellServices( base::Bind(&chromeos::attestation::PlatformVerificationImpl::Create, render_frame_host)); #endif +#if !defined(OS_ANDROID) && !defined(OS_IOS) + registry->AddService( + base::Bind(&WebUSBPermissionProvider::Create, render_frame_host)); +#endif } void ChromeContentBrowserClient::RegisterInProcessMojoApplications( diff --git a/chrome/browser/usb/DEPS b/chrome/browser/usb/DEPS new file mode 100644 index 0000000..722f640 --- /dev/null +++ b/chrome/browser/usb/DEPS @@ -0,0 +1,3 @@ +include_rules = [ + "+device/devices_app/usb/public" +] diff --git a/chrome/browser/usb/OWNERS b/chrome/browser/usb/OWNERS new file mode 100644 index 0000000..b16946a --- /dev/null +++ b/chrome/browser/usb/OWNERS @@ -0,0 +1,2 @@ +reillyg@chromium.org +rockot@chromium.org diff --git a/chrome/browser/usb/web_usb_permission_provider.cc b/chrome/browser/usb/web_usb_permission_provider.cc new file mode 100644 index 0000000..0c86908 --- /dev/null +++ b/chrome/browser/usb/web_usb_permission_provider.cc @@ -0,0 +1,38 @@ +// Copyright 2015 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. + +#include "chrome/browser/usb/web_usb_permission_provider.h" + +#include "content/public/browser/browser_thread.h" + +// static +void WebUSBPermissionProvider::Create( + content::RenderFrameHost* render_frame_host, + mojo::InterfaceRequest<PermissionProvider> request) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + DCHECK(render_frame_host); + + // The created object is strongly bound to (and owned by) the pipe. + new WebUSBPermissionProvider(render_frame_host, request.Pass()); +} + +WebUSBPermissionProvider::~WebUSBPermissionProvider() {} + +WebUSBPermissionProvider::WebUSBPermissionProvider( + content::RenderFrameHost* render_frame_host, + mojo::InterfaceRequest<PermissionProvider> request) + : binding_(this, request.Pass()), + render_frame_host_(render_frame_host) {} + +void WebUSBPermissionProvider::HasDevicePermission( + mojo::Array<mojo::String> requested_guids, + const HasDevicePermissionCallback& callback) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + // TODO(reillyg): Look up permissions granted to the render frame host's + // current origin in the render frame process's browser context. Until then + // |render_frame_host_| is unused so this ignore_result() is needed. + ignore_result(render_frame_host_); + mojo::Array<mojo::String> allowed_guids(0); + callback.Run(allowed_guids.Pass()); +} diff --git a/chrome/browser/usb/web_usb_permission_provider.h b/chrome/browser/usb/web_usb_permission_provider.h new file mode 100644 index 0000000..05b1242 --- /dev/null +++ b/chrome/browser/usb/web_usb_permission_provider.h @@ -0,0 +1,38 @@ +// Copyright 2015 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. + +#ifndef CHROME_BROWSER_USB_WEB_USB_PERMISSION_PROVIDER_H_ +#define CHROME_BROWSER_USB_WEB_USB_PERMISSION_PROVIDER_H_ + +#include "device/devices_app/usb/public/interfaces/permission_provider.mojom.h" +#include "third_party/mojo/src/mojo/public/cpp/bindings/array.h" +#include "third_party/mojo/src/mojo/public/cpp/bindings/interface_request.h" +#include "third_party/mojo/src/mojo/public/cpp/bindings/strong_binding.h" + +namespace content { +class RenderFrameHost; +} + +class WebUSBPermissionProvider : public device::usb::PermissionProvider { + public: + static void Create( + content::RenderFrameHost* render_frame_host, + mojo::InterfaceRequest<device::usb::PermissionProvider> request); + + ~WebUSBPermissionProvider() override; + + private: + WebUSBPermissionProvider(content::RenderFrameHost* render_frame_host, + mojo::InterfaceRequest<PermissionProvider> request); + + // device::usb::PermissionProvider implementation. + void HasDevicePermission( + mojo::Array<mojo::String> requested_guids, + const HasDevicePermissionCallback& callback) override; + + mojo::StrongBinding<PermissionProvider> binding_; + content::RenderFrameHost* const render_frame_host_; +}; + +#endif // CHROME_BROWSER_USB_WEB_USB_PERMISSION_PROVIDER_H_ diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index a55c3dd..39d552c 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1277,6 +1277,8 @@ 'browser/sync/glue/synced_tab_delegate_desktop.cc', 'browser/sync/sync_ui_util.cc', 'browser/sync/sync_ui_util.h', + 'browser/usb/web_usb_permission_provider.cc', + 'browser/usb/web_usb_permission_provider.h', ], 'chrome_browser_win_sources': [ 'browser/browser_process_platform_part_aurawin.cc', diff --git a/device/BUILD.gn b/device/BUILD.gn index 8c5bf66f..4961292 100644 --- a/device/BUILD.gn +++ b/device/BUILD.gn @@ -126,7 +126,6 @@ test("device_unittests") { deps += [ "//device/core", "//device/devices_app:lib", - "//device/devices_app/usb/public/cpp", "//device/devices_app/usb/public/interfaces", "//device/usb", "//device/usb:mocks", diff --git a/device/devices_app/BUILD.gn b/device/devices_app/BUILD.gn index bb89b70..2450b63 100644 --- a/device/devices_app/BUILD.gn +++ b/device/devices_app/BUILD.gn @@ -18,7 +18,6 @@ source_set("lib") { deps = [ "//device/core", - "//device/devices_app/usb/public/cpp", "//device/devices_app/usb/public/interfaces", "//device/usb", "//net", diff --git a/device/devices_app/devices_app.cc b/device/devices_app/devices_app.cc index 1531651..599c64f 100644 --- a/device/devices_app/devices_app.cc +++ b/device/devices_app/devices_app.cc @@ -13,7 +13,6 @@ #include "base/time/time.h" #include "device/core/device_client.h" #include "device/devices_app/usb/device_manager_impl.h" -#include "device/devices_app/usb/public/cpp/device_manager_delegate.h" #include "device/usb/usb_service.h" #include "mojo/application/public/cpp/application_connection.h" #include "mojo/application/public/cpp/application_impl.h" @@ -28,32 +27,6 @@ namespace { // exiting the app. const int64 kIdleTimeoutInSeconds = 10; -// A usb::DeviceManagerDelegate implementation which provides origin-based -// device access control. -// -// TODO(rockot/reillyg): We should just get rid of DeviceManagerDelegate, or -// at least repurpose it as test support to provide device mocks. -class USBDeviceManagerDelegate : public usb::DeviceManagerDelegate { - public: - explicit USBDeviceManagerDelegate(const GURL& remote_url) - : remote_url_(remote_url) {} - ~USBDeviceManagerDelegate() override {} - - private: - // usb::DeviceManagerDelegate: - bool IsDeviceAllowed(const usb::DeviceInfo& device) override { - // Also let browser apps and mojo apptests talk to all devices. - if (remote_url_.SchemeIs("system") || - remote_url_ == GURL("mojo://devices_apptests/")) - return true; - return false; - } - - GURL remote_url_; - - DISALLOW_COPY_AND_ASSIGN(USBDeviceManagerDelegate); -}; - // A DeviceClient implementation to be constructed iff the app is not running // in an embedder that provides a DeviceClient (i.e. running as a standalone // Mojo app, not in Chrome). @@ -131,12 +104,14 @@ void DevicesApp::Quit() { void DevicesApp::Create(mojo::ApplicationConnection* connection, mojo::InterfaceRequest<usb::DeviceManager> request) { - scoped_ptr<usb::DeviceManagerDelegate> delegate(new USBDeviceManagerDelegate( - GURL(connection->GetRemoteApplicationURL()))); + // Bind the new device manager to the connecting application's permission + // provider. + usb::PermissionProviderPtr permission_provider; + connection->ConnectToService(&permission_provider); // Owned by its message pipe. usb::DeviceManagerImpl* device_manager = new usb::DeviceManagerImpl( - request.Pass(), delegate.Pass(), service_task_runner_); + request.Pass(), permission_provider.Pass(), service_task_runner_); device_manager->set_connection_error_handler( base::Bind(&DevicesApp::OnConnectionError, base::Unretained(this))); diff --git a/device/devices_app/devices_app.gyp b/device/devices_app/devices_app.gyp index 4d97aa9..11af914 100644 --- a/device/devices_app/devices_app.gyp +++ b/device/devices_app/devices_app.gyp @@ -20,7 +20,6 @@ 'usb/device_impl.h', 'usb/device_manager_impl.cc', 'usb/device_manager_impl.h', - 'usb/public/cpp/device_manager_delegate.h', 'usb/type_converters.cc', 'usb/type_converters.h', ], @@ -45,6 +44,7 @@ 'mojom_files': [ 'usb/public/interfaces/device.mojom', 'usb/public/interfaces/device_manager.mojom', + 'usb/public/interfaces/permission_provider.mojom', ], }, 'includes': [ diff --git a/device/devices_app/usb/device_manager_impl.cc b/device/devices_app/usb/device_manager_impl.cc index 559edf6..cfc45b6 100644 --- a/device/devices_app/usb/device_manager_impl.cc +++ b/device/devices_app/usb/device_manager_impl.cc @@ -14,7 +14,6 @@ #include "base/thread_task_runner_handle.h" #include "device/core/device_client.h" #include "device/devices_app/usb/device_impl.h" -#include "device/devices_app/usb/public/cpp/device_manager_delegate.h" #include "device/devices_app/usb/public/interfaces/device.mojom.h" #include "device/devices_app/usb/type_converters.h" #include "device/usb/usb_device.h" @@ -108,6 +107,23 @@ void OpenDeviceOnServiceThread( callback_task_runner)); } +void FilterDeviceListAndThen( + const DeviceManagerImpl::GetDevicesCallback& callback, + mojo::Array<DeviceInfoPtr> devices, + mojo::Array<mojo::String> allowed_guids) { + std::set<std::string> allowed_guid_set; + for (size_t i = 0; i < allowed_guids.size(); ++i) + allowed_guid_set.insert(allowed_guids[i]); + + mojo::Array<DeviceInfoPtr> allowed_devices(0); + for (size_t i = 0; i < devices.size(); ++i) { + if (ContainsKey(allowed_guid_set, devices[i]->guid)) + allowed_devices.push_back(devices[i].Pass()); + } + + callback.Run(allowed_devices.Pass()); +} + } // namespace class DeviceManagerImpl::ServiceThreadHelper @@ -122,17 +138,10 @@ class DeviceManagerImpl::ServiceThreadHelper base::MessageLoop::current()->RemoveDestructionObserver(this); } - static void Start( - scoped_ptr<ServiceThreadHelper> self, - const base::Callback<void(mojo::Array<DeviceInfoPtr>)>& callback) { + static void Start(scoped_ptr<ServiceThreadHelper> self) { UsbService* usb_service = DeviceClient::Get()->GetUsbService(); - if (usb_service) { + if (usb_service) self->observer_.Add(usb_service); - std::vector<UsbDeviceFilter> no_filters; - usb_service->GetDevices(base::Bind(&OnGetDevicesOnServiceThread, - no_filters, callback, - self->task_runner_)); - } // |self| now owned by the current message loop. base::MessageLoop::current()->AddDestructionObserver(self.release()); @@ -165,30 +174,38 @@ class DeviceManagerImpl::ServiceThreadHelper DeviceManagerImpl::DeviceManagerImpl( mojo::InterfaceRequest<DeviceManager> request, - scoped_ptr<DeviceManagerDelegate> delegate, + PermissionProviderPtr permission_provider, scoped_refptr<base::SequencedTaskRunner> service_task_runner) - : binding_(this, request.Pass()), - delegate_(delegate.Pass()), + : permission_provider_(permission_provider.Pass()), service_task_runner_(service_task_runner), + binding_(this, request.Pass()), weak_factory_(this) { + // This object owns itself and will be destroyed if either the message pipe + // it is bound to is closed or the PermissionProvider it depends on is + // unavailable. + binding_.set_connection_error_handler([this]() { delete this; }); + permission_provider_.set_connection_error_handler([this]() { delete this; }); + + scoped_ptr<ServiceThreadHelper> helper(new ServiceThreadHelper( + weak_factory_.GetWeakPtr(), base::ThreadTaskRunnerHandle::Get())); + helper_ = helper.get(); + service_task_runner_->PostTask( + FROM_HERE, + base::Bind(&ServiceThreadHelper::Start, base::Passed(&helper))); } DeviceManagerImpl::~DeviceManagerImpl() { - if (helper_) { - // It is safe to call this if |helper_| was already destroyed when - // |service_task_runner_| exited as the task will never execute. - service_task_runner_->DeleteSoon(FROM_HERE, helper_); - } -} - -void DeviceManagerImpl::set_connection_error_handler( - const mojo::Closure& error_handler) { - binding_.set_connection_error_handler(error_handler); + // It is safe to call this if |helper_| was already destroyed when + // |service_task_runner_| exited as the task will never execute. + service_task_runner_->DeleteSoon(FROM_HERE, helper_); + connection_error_handler_.Run(); } void DeviceManagerImpl::GetDevices(EnumerationOptionsPtr options, const GetDevicesCallback& callback) { - auto filters = options->filters.To<std::vector<UsbDeviceFilter>>(); + std::vector<UsbDeviceFilter> filters; + if (options) + filters = options->filters.To<std::vector<UsbDeviceFilter>>(); auto get_devices_callback = base::Bind(&DeviceManagerImpl::OnGetDevices, weak_factory_.GetWeakPtr(), callback); service_task_runner_->PostTask( @@ -199,53 +216,48 @@ void DeviceManagerImpl::GetDevices(EnumerationOptionsPtr options, void DeviceManagerImpl::GetDeviceChanges( const GetDeviceChangesCallback& callback) { - if (helper_) { - device_change_callbacks_.push(callback); - MaybeRunDeviceChangesCallback(); - } else { - scoped_ptr<ServiceThreadHelper> helper(new ServiceThreadHelper( - weak_factory_.GetWeakPtr(), base::ThreadTaskRunnerHandle::Get())); - helper_ = helper.get(); - auto get_devices_callback = - base::Bind(&DeviceManagerImpl::OnGetInitialDevices, - weak_factory_.GetWeakPtr(), callback); - service_task_runner_->PostTask( - FROM_HERE, base::Bind(&ServiceThreadHelper::Start, - base::Passed(&helper), get_devices_callback)); - } + device_change_callbacks_.push(callback); + MaybeRunDeviceChangesCallback(); } void DeviceManagerImpl::OpenDevice( const mojo::String& guid, mojo::InterfaceRequest<Device> device_request, const OpenDeviceCallback& callback) { + mojo::Array<mojo::String> requested_guids(1); + requested_guids[0] = guid; + permission_provider_->HasDevicePermission( + requested_guids.Pass(), + base::Bind(&DeviceManagerImpl::OnOpenDevicePermissionCheckComplete, + base::Unretained(this), base::Passed(&device_request), + callback)); +} + +void DeviceManagerImpl::OnOpenDevicePermissionCheckComplete( + mojo::InterfaceRequest<Device> device_request, + const OpenDeviceCallback& callback, + mojo::Array<mojo::String> allowed_guids) { + if (allowed_guids.size() == 0) { + callback.Run(OPEN_DEVICE_ERROR_ACCESS_DENIED); + return; + } + + DCHECK(allowed_guids.size() == 1); service_task_runner_->PostTask( - FROM_HERE, base::Bind(&OpenDeviceOnServiceThread, guid, + FROM_HERE, base::Bind(&OpenDeviceOnServiceThread, allowed_guids[0], base::Passed(&device_request), callback, base::ThreadTaskRunnerHandle::Get())); } void DeviceManagerImpl::OnGetDevices(const GetDevicesCallback& callback, mojo::Array<DeviceInfoPtr> devices) { - mojo::Array<DeviceInfoPtr> allowed_devices(0); - for (size_t i = 0; i < devices.size(); ++i) { - if (delegate_->IsDeviceAllowed(*devices[i])) - allowed_devices.push_back(devices[i].Pass()); - } - callback.Run(allowed_devices.Pass()); -} + mojo::Array<mojo::String> requested_guids(devices.size()); + for (size_t i = 0; i < devices.size(); ++i) + requested_guids[i] = devices[i]->guid; -void DeviceManagerImpl::OnGetInitialDevices( - const GetDeviceChangesCallback& callback, - mojo::Array<DeviceInfoPtr> devices) { - DeviceChangeNotificationPtr notification = DeviceChangeNotification::New(); - notification->devices_added = mojo::Array<DeviceInfoPtr>::New(0); - notification->devices_removed = mojo::Array<mojo::String>::New(0); - for (size_t i = 0; i < devices.size(); ++i) { - if (delegate_->IsDeviceAllowed(*devices[i])) - notification->devices_added.push_back(devices[i].Pass()); - } - callback.Run(notification.Pass()); + permission_provider_->HasDevicePermission( + requested_guids.Pass(), + base::Bind(&FilterDeviceListAndThen, callback, base::Passed(&devices))); } void DeviceManagerImpl::OnDeviceAdded(DeviceInfoPtr device) { @@ -270,18 +282,63 @@ void DeviceManagerImpl::OnDeviceRemoved(std::string device_guid) { } void DeviceManagerImpl::MaybeRunDeviceChangesCallback() { - if (!device_change_callbacks_.empty()) { + if (!permission_request_pending_ && !device_change_callbacks_.empty()) { + mojo::Array<DeviceInfoPtr> devices_added; + devices_added.Swap(&devices_added_); + std::set<std::string> devices_removed; + devices_removed.swap(devices_removed_); + + mojo::Array<mojo::String> requested_guids(devices_added.size() + + devices_removed.size()); + { + size_t i; + for (i = 0; i < devices_added.size(); ++i) + requested_guids[i] = devices_added[i]->guid; + for (const std::string& guid : devices_removed) + requested_guids[i++] = guid; + } + + permission_request_pending_ = true; + permission_provider_->HasDevicePermission( + requested_guids.Pass(), + base::Bind(&DeviceManagerImpl::OnEnumerationPermissionCheckComplete, + base::Unretained(this), base::Passed(&devices_added), + devices_removed)); + } +} + +void DeviceManagerImpl::OnEnumerationPermissionCheckComplete( + mojo::Array<DeviceInfoPtr> devices_added, + const std::set<std::string>& devices_removed, + mojo::Array<mojo::String> allowed_guids) { + permission_request_pending_ = false; + + if (allowed_guids.size() > 0) { + std::set<std::string> allowed_guid_set; + for (size_t i = 0; i < allowed_guids.size(); ++i) + allowed_guid_set.insert(allowed_guids[i]); + DeviceChangeNotificationPtr notification = DeviceChangeNotification::New(); - notification->devices_added.Swap(&devices_added_); + notification->devices_added.resize(0); + for (size_t i = 0; i < devices_added.size(); ++i) { + if (ContainsKey(allowed_guid_set, devices_added[i]->guid)) + notification->devices_added.push_back(devices_added[i].Pass()); + } + notification->devices_removed.resize(0); - for (const std::string& device : devices_removed_) - notification->devices_removed.push_back(device); - devices_removed_.clear(); + for (const std::string& guid : devices_removed) { + if (ContainsKey(allowed_guid_set, guid)) + notification->devices_removed.push_back(guid); + } + DCHECK(!device_change_callbacks_.empty()); const GetDeviceChangesCallback& callback = device_change_callbacks_.front(); callback.Run(notification.Pass()); device_change_callbacks_.pop(); } + + if (devices_added_.size() > 0 || !devices_removed_.empty()) + MaybeRunDeviceChangesCallback(); } } // namespace usb diff --git a/device/devices_app/usb/device_manager_impl.h b/device/devices_app/usb/device_manager_impl.h index 69b991e..b84f254 100644 --- a/device/devices_app/usb/device_manager_impl.h +++ b/device/devices_app/usb/device_manager_impl.h @@ -14,9 +14,10 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "device/devices_app/usb/public/interfaces/device_manager.mojom.h" +#include "device/devices_app/usb/public/interfaces/permission_provider.mojom.h" #include "third_party/mojo/src/mojo/public/cpp/bindings/array.h" +#include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h" #include "third_party/mojo/src/mojo/public/cpp/bindings/interface_request.h" -#include "third_party/mojo/src/mojo/public/cpp/bindings/strong_binding.h" namespace base { class SequencedTaskRunner; @@ -38,11 +39,13 @@ class DeviceManagerImpl : public DeviceManager { public: DeviceManagerImpl( mojo::InterfaceRequest<DeviceManager> request, - scoped_ptr<DeviceManagerDelegate> delegate, + PermissionProviderPtr permission_provider, scoped_refptr<base::SequencedTaskRunner> service_task_runner); ~DeviceManagerImpl() override; - void set_connection_error_handler(const mojo::Closure& error_handler); + void set_connection_error_handler(const mojo::Closure& error_handler) { + connection_error_handler_ = error_handler; + } private: class ServiceThreadHelper; @@ -55,20 +58,25 @@ class DeviceManagerImpl : public DeviceManager { mojo::InterfaceRequest<Device> device_request, const OpenDeviceCallback& callback) override; + void OnOpenDevicePermissionCheckComplete( + mojo::InterfaceRequest<Device> device_request, + const OpenDeviceCallback& callback, + mojo::Array<mojo::String> allowed_guids); + // Callbacks to handle the async responses from the underlying UsbService. void OnGetDevices(const GetDevicesCallback& callback, mojo::Array<DeviceInfoPtr> devices); - void OnGetInitialDevices(const GetDeviceChangesCallback& callback, - mojo::Array<DeviceInfoPtr> devices); // Methods called by |helper_| when devices are added or removed. void OnDeviceAdded(DeviceInfoPtr device); void OnDeviceRemoved(std::string device_guid); void MaybeRunDeviceChangesCallback(); + void OnEnumerationPermissionCheckComplete( + mojo::Array<DeviceInfoPtr> devices_added, + const std::set<std::string>& devices_removed, + mojo::Array<mojo::String> allowed_guids); - mojo::StrongBinding<DeviceManager> binding_; - - scoped_ptr<DeviceManagerDelegate> delegate_; + PermissionProviderPtr permission_provider_; scoped_refptr<base::SequencedTaskRunner> service_task_runner_; // If there are unfinished calls to GetDeviceChanges their callbacks @@ -78,11 +86,17 @@ class DeviceManagerImpl : public DeviceManager { std::queue<GetDeviceChangesCallback> device_change_callbacks_; mojo::Array<DeviceInfoPtr> devices_added_; std::set<std::string> devices_removed_; + // To ensure that GetDeviceChangesCallbacks are called in the correct order + // only perform a single request to |permission_provider_| at a time. + bool permission_request_pending_ = false; // |helper_| is owned by the service thread and holds a weak reference // back to the device manager that created it. - ServiceThreadHelper* helper_ = nullptr; + ServiceThreadHelper* helper_; + + mojo::Closure connection_error_handler_; + mojo::Binding<DeviceManager> binding_; base::WeakPtrFactory<DeviceManagerImpl> weak_factory_; DISALLOW_COPY_AND_ASSIGN(DeviceManagerImpl); diff --git a/device/devices_app/usb/device_manager_impl_unittest.cc b/device/devices_app/usb/device_manager_impl_unittest.cc index e084430..b66d8fc 100644 --- a/device/devices_app/usb/device_manager_impl_unittest.cc +++ b/device/devices_app/usb/device_manager_impl_unittest.cc @@ -14,7 +14,6 @@ #include "device/core/device_client.h" #include "device/devices_app/usb/device_impl.h" #include "device/devices_app/usb/device_manager_impl.h" -#include "device/devices_app/usb/public/cpp/device_manager_delegate.h" #include "device/usb/mock_usb_device.h" #include "device/usb/mock_usb_device_handle.h" #include "device/usb/mock_usb_service.h" @@ -29,14 +28,21 @@ namespace usb { namespace { -class TestDeviceManagerDelegate : public DeviceManagerDelegate { +class TestPermissionProvider : public PermissionProvider { public: - TestDeviceManagerDelegate() {} - ~TestDeviceManagerDelegate() override {} + TestPermissionProvider(mojo::InterfaceRequest<PermissionProvider> request) + : binding_(this, request.Pass()) {} + ~TestPermissionProvider() override {} + + void HasDevicePermission( + mojo::Array<mojo::String> requested_guids, + const HasDevicePermissionCallback& callback) override { + // Permission to access all devices granted. + callback.Run(requested_guids.Pass()); + } private: - // DeviceManagerDelegate implementation: - bool IsDeviceAllowed(const DeviceInfo& device_info) override { return true; } + mojo::StrongBinding<PermissionProvider> binding_; }; class TestDeviceClient : public DeviceClient { @@ -66,11 +72,12 @@ class USBDeviceManagerImplTest : public testing::Test { } DeviceManagerPtr ConnectToDeviceManager() { + PermissionProviderPtr permission_provider; + new TestPermissionProvider(mojo::GetProxy(&permission_provider)); DeviceManagerPtr device_manager; - new DeviceManagerImpl( - mojo::GetProxy(&device_manager), - scoped_ptr<DeviceManagerDelegate>(new TestDeviceManagerDelegate), - base::ThreadTaskRunnerHandle::Get()); + new DeviceManagerImpl(mojo::GetProxy(&device_manager), + permission_provider.Pass(), + base::ThreadTaskRunnerHandle::Get()); return device_manager.Pass(); } @@ -236,13 +243,14 @@ TEST_F(USBDeviceManagerImplTest, GetDeviceChanges) { DeviceManagerPtr device_manager = ConnectToDeviceManager(); { - std::set<std::string> added_guids; - std::set<std::string> removed_guids; - added_guids.insert(device0->guid()); + // Call GetDevices once to make sure the device manager is up and running + // or else we could end up waiting forever for device changes as the next + // block races with the ServiceThreadHelper startup. + std::set<std::string> guids; + guids.insert(device0->guid()); base::RunLoop loop; - device_manager->GetDeviceChanges(base::Bind(&ExpectDeviceChangesAndThen, - added_guids, removed_guids, - loop.QuitClosure())); + device_manager->GetDevices( + nullptr, base::Bind(&ExpectDevicesAndThen, guids, loop.QuitClosure())); loop.Run(); } diff --git a/device/devices_app/usb/public/cpp/BUILD.gn b/device/devices_app/usb/public/cpp/BUILD.gn deleted file mode 100644 index 9ca3cce..0000000 --- a/device/devices_app/usb/public/cpp/BUILD.gn +++ /dev/null @@ -1,15 +0,0 @@ -# Copyright 2015 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. - -source_set("cpp") { - sources = [ - "device_manager_delegate.h", - ] - - public_deps = [ - "//base", - "//device/devices_app/usb/public/interfaces", - "//third_party/mojo/src/mojo/public/cpp/bindings", - ] -} diff --git a/device/devices_app/usb/public/cpp/device_manager_delegate.h b/device/devices_app/usb/public/cpp/device_manager_delegate.h deleted file mode 100644 index 6400591..0000000 --- a/device/devices_app/usb/public/cpp/device_manager_delegate.h +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright 2015 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. - -#ifndef DEVICE_DEVICES_APP_USB_PUBLIC_CPP_DEVICE_MANAGER_DELEGATE_H_ -#define DEVICE_DEVICES_APP_USB_PUBLIC_CPP_DEVICE_MANAGER_DELEGATE_H_ - -#include "device/devices_app/usb/public/interfaces/device.mojom.h" - -namespace device { -namespace usb { - -// Interface used by DeviceManager instances to delegate certain decisions and -// behaviors out to their embedder. -class DeviceManagerDelegate { - public: - virtual ~DeviceManagerDelegate() {} - - // Determines whether a given device should be accessible to clients of the - // DeviceManager instance. - virtual bool IsDeviceAllowed(const DeviceInfo& info) = 0; -}; - -} // namespace usb -} // namespace device - -#endif // DEVICE_DEVICES_APP_USB_PUBLIC_CPP_DEVICE_MANAGER_DELEGATE_H_ diff --git a/device/devices_app/usb/public/interfaces/BUILD.gn b/device/devices_app/usb/public/interfaces/BUILD.gn index 14b9362..0b3ad9a 100644 --- a/device/devices_app/usb/public/interfaces/BUILD.gn +++ b/device/devices_app/usb/public/interfaces/BUILD.gn @@ -8,5 +8,6 @@ mojom("interfaces") { sources = [ "device.mojom", "device_manager.mojom", + "permission_provider.mojom", ] } diff --git a/device/devices_app/usb/public/interfaces/device_manager.mojom b/device/devices_app/usb/public/interfaces/device_manager.mojom index ffb05d5..a2d20ae 100644 --- a/device/devices_app/usb/public/interfaces/device_manager.mojom +++ b/device/devices_app/usb/public/interfaces/device_manager.mojom @@ -35,7 +35,7 @@ struct DeviceFilter { }; struct EnumerationOptions { - array<DeviceFilter> filters; + array<DeviceFilter>? filters; }; struct DeviceChangeNotification { @@ -46,12 +46,11 @@ struct DeviceChangeNotification { interface DeviceManager { // Retrieves information about all devices available to the DeviceManager // implementation. - GetDevices(EnumerationOptions options) => (array<DeviceInfo> results); + GetDevices(EnumerationOptions? options) => (array<DeviceInfo> results); // Retrieves information about changes to the set of devices available to the - // DeviceManager since the last call to this method. The first call will - // return the set of all devices available. Device added and removed between - // calls will not be included. + // DeviceManager since the last call to this method. A device that is added + // and removed between calls will not be included. GetDeviceChanges() => (DeviceChangeNotification changes); // Attempts to open a device given its GUID. diff --git a/device/devices_app/usb/public/interfaces/permission_provider.mojom b/device/devices_app/usb/public/interfaces/permission_provider.mojom new file mode 100644 index 0000000..90fd0ea --- /dev/null +++ b/device/devices_app/usb/public/interfaces/permission_provider.mojom @@ -0,0 +1,14 @@ +// Copyright 2015 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. + +module device.usb; + +import "device.mojom"; + +interface PermissionProvider { + // Filters a set of |requested_guids| down to the set of |allowed_guids| that + // should be accessible to clients of the DeviceManager instance. + HasDevicePermission(array<string> requested_guids) + => (array<string> allowed_guids); +}; |