Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1502663003:
bluetooth: Implement allowed devices map (Closed)
|
Created:
5 years ago by ortuno Modified:
4 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@my-origin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Implement allowed devices map
Add a selected device to the list of allowed devices map and, before
interacting with the device, check that the origin is allowed to
access that device.
BUG=493459
Committed: https://crrev.com/154400787a01efedca9063d98a4b0d06ebf39089
Cr-Commit-Position: refs/heads/master@{#369870}
Patch Set 1 #Patch Set 2 : WIP 2: Pass and expect an ID rather than the address #Patch Set 3 : Clean up #Patch Set 4 : Add tests for map #Patch Set 5 : Moar cleanup #Patch Set 6 : Forgot test file #
Total comments: 40
Patch Set 7 : Address jyasskin's comments #
Total comments: 10
Patch Set 8 : Address jyasskin's comments #Patch Set 9 : Add comment for bad renderer check #
Total comments: 16
Patch Set 10 : Address palmer's comments #
Total comments: 15
Patch Set 11 : Address palmer's comments 2 #Patch Set 12 : Make global consts non-reference #Patch Set 13 : Fix merge conflicts #Dependent Patchsets: Messages
Total messages: 40 (19 generated)
Description was changed from ========== bluetooth: Send frame rounting id to browser The routing ID is needed by the browser to get the frame's origin which will be used to decide whether or not an origin can access a device. BUG=493459 ========== to ========== bluetooth: Implement allowed devices map BUG=493459 ==========
Description was changed from ========== bluetooth: Implement allowed devices map BUG=493459 ========== to ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. BUG=493459 ==========
Description was changed from ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. BUG=493459 ========== to ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. BUG=493459 ==========
Description was changed from ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. BUG=493459 ========== to ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 6. Consider adding another map to find addresses faster. BUG=493459 ==========
Description was changed from ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 6. Consider adding another map to find addresses faster. BUG=493459 ========== to ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 6. Consider adding another map to find addresses faster. 7. Tests BUG=493459 ==========
Description was changed from ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 6. Consider adding another map to find addresses faster. 7. Tests BUG=493459 ========== to ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 6. Consider adding another map to find addresses faster. 7. Tests 8. Typedefs BUG=493459 ==========
Description was changed from ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 6. Consider adding another map to find addresses faster. 7. Tests 8. Typedefs BUG=493459 ========== to ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 6. Consider adding another map to find addresses faster. 7. Tests 8. Typedefs 9. Change LOG to VLOG BUG=493459 ==========
Description was changed from ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 2. Repeat less code 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 6. Consider adding another map to find addresses faster. 7. Tests 8. Typedefs 9. Change LOG to VLOG BUG=493459 ========== to ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 7. Tests 9. Change LOG to VLOG BUG=493459 ==========
Description was changed from ========== bluetooth: Implement allowed devices map TODOs: 1. Fix function names 3. Crash renderer for unallowed devices. 4. Check if QueryCacheFor* should check with origin. 5. Move function to correct places. 7. Tests 9. Change LOG to VLOG BUG=493459 ========== to ========== bluetooth: Implement allowed devices map Add a selected device to the list of allowed devices map and, before interacting with the device, check that the origin is allowed to access that device. BUG=493459 ==========
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin: PTAL. Keep in mind that with mojo we won't have to pass in the frame_routing_id anymore, since mojo services are per frame, so some of the code is going to be more simple. I decided to do this before the mojo migration in case the migration takes longer than expected.
Sorry it took me so long to review this. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:26: bytes.size() + 1 /* lenght_with_null */), s/lenght/length/ https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:116: const std::string BluetoothAllowedDevicesMap::GenerateDeviceId( Don't make by-value return types const: it prevents moving from the result. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:127: return GetBase64Id(); Return device_id, right? https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:130: const std::set<std::string> BluetoothAllowedDevicesMap::UnionOfServices( Un-const this too. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:26: // When talking about |device_id|s we are refer to s/refer/referring/ https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:33: // Adds the Bluetooth Device with |device_address| to the map of allowed Does this still work if the device is already in the map? As you pointed out, it should add the services to this origin/device's allowed service list. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:49: bool HasDevicePermissionFromDeviceId(const std::string& origin, I might name these HasPermissionToAccessDeviceId() and HasPermissionToAccessDeviceAddress(). https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:61: // This function should never be called before checking if the origin It seems cheaper to unify the access check with the ID lookup. Would it make sense for this to return an empty string to mean the origin doesn't have permission? https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:77: // Returns an id guaranteed to be unique for the origin. Could you mention that this is also random, so that one origin can't guess an ID likely to be in use by another origin? https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:25: class BluetoothAllowedDevicesMapTest : public testing::Test {}; Could you add tests that: 1) A device added to the map for the same origin twice gets the same ID. 2) A device added for two different origins gets different IDs. (This'll flake every 2^128 runs, which should be ok.) 3) A device added for an origin, removed, and then added again gets a different ID. (Ditto) https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:28: // Test that a device is correctly added to the map. This comment is probably unnecessary. The test name already says it. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:65: // Test that the generated if has the correct format s/if/id/? https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:956: // Make sure the origin is allowed to access the device. Instead of "Make sure", we should probably say why returning is the right thing to do. e.g. "If the frame navigates to a different origin while the StopNotifications() request is in flight, we won't find the right device address, so we can't stop its notifications. Return instead." (Although that's probably not the right reason, since it would imply that we leak the notification sessions on navigation, and we shouldn't let that happen.) https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1117: // device_id is device_address since we passed it to the chooser. Maybe "is the device address that RequestDeviceSession passes to chooser->AddDevice()." https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1285: this, bad_message::BDH_DEVICE_NOT_ALLOWED_FOR_ORIGIN); I'm not sure we can kill the renderer for using a bad device: if the user revokes access while a message is in flight, I think we'll hit this case, and we don't want to kill that renderer. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1319: bad_message::ReceivedBadMessage( Ditto here; not sure we can kill this renderer. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:248: // Returns the origin for the frame with "frame_rounting_id" in this process. sp: rounting https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:248: // Returns the origin for the frame with "frame_rounting_id" in this process. s/this process/render_process_id_/
Description was changed from ========== bluetooth: Implement allowed devices map Add a selected device to the list of allowed devices map and, before interacting with the device, check that the origin is allowed to access that device. BUG=493459 ========== to ========== bluetooth: Implement allowed devices map Add a selected device to the list of allowed devices map and, before interacting with the device, check that the origin is allowed to access that device. BUG=493459 ==========
PTAL again. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:26: bytes.size() + 1 /* lenght_with_null */), On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > s/lenght/length/ Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:116: const std::string BluetoothAllowedDevicesMap::GenerateDeviceId( On 2016/01/06 at 00:47:56, Jeffrey Yasskin wrote: > Don't make by-value return types const: it prevents moving from the result. Done. You mean it prevents the optimization in which the object is moved rather than copied right? https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:127: return GetBase64Id(); On 2016/01/06 at 00:47:56, Jeffrey Yasskin wrote: > Return device_id, right? Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:130: const std::set<std::string> BluetoothAllowedDevicesMap::UnionOfServices( On 2016/01/06 at 00:47:56, Jeffrey Yasskin wrote: > Un-const this too. Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:26: // When talking about |device_id|s we are refer to On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > s/refer/referring/ Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:33: // Adds the Bluetooth Device with |device_address| to the map of allowed On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > Does this still work if the device is already in the map? As you pointed out, it should add the services to this origin/device's allowed service list. Implementing features that are not yet in the spec could lead to inconsistencies between browsers. So this implements what the spec says. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:49: bool HasDevicePermissionFromDeviceId(const std::string& origin, On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > I might name these HasPermissionToAccessDeviceId() and HasPermissionToAccessDeviceAddress(). Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:61: // This function should never be called before checking if the origin On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > It seems cheaper to unify the access check with the ID lookup. Would it make sense for this to return an empty string to mean the origin doesn't have permission? Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:77: // Returns an id guaranteed to be unique for the origin. On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > Could you mention that this is also random, so that one origin can't guess an ID likely to be in use by another origin? Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:25: class BluetoothAllowedDevicesMapTest : public testing::Test {}; On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > Could you add tests that: > 1) A device added to the map for the same origin twice gets the same ID. > 2) A device added for two different origins gets different IDs. (This'll flake every 2^128 runs, which should be ok.) > 3) A device added for an origin, removed, and then added again gets a different ID. (Ditto) Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:28: // Test that a device is correctly added to the map. On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > This comment is probably unnecessary. The test name already says it. Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:65: // Test that the generated if has the correct format On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > s/if/id/? Removed comment. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:956: // Make sure the origin is allowed to access the device. On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > Instead of "Make sure", we should probably say why returning is the right thing to do. e.g. "If the frame navigates to a different origin while the StopNotifications() request is in flight, we won't find the right device address, so we can't stop its notifications. Return instead." (Although that's probably not the right reason, since it would imply that we leak the notification sessions on navigation, and we shouldn't let that happen.) These checks are in place in case of a hostile renderer. There isn't much a hostile renderer could do with stopNotifications(), so I could remove it. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1117: // device_id is device_address since we passed it to the chooser. On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > Maybe "is the device address that RequestDeviceSession passes to chooser->AddDevice()." Done. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1285: this, bad_message::BDH_DEVICE_NOT_ALLOWED_FOR_ORIGIN); On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > I'm not sure we can kill the renderer for using a bad device: if the user revokes access while a message is in flight, I think we'll hit this case, and we don't want to kill that renderer. True. It would happen anytime you revoked access even if there are no messages in flight. Since it's not spec'ed yet and we don't support revoking permissions the only case in which the renderer would crash would be a hostile renderer. So I propose we leave this in until it's spec'ed and we support revoking permissions. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1319: bad_message::ReceivedBadMessage( On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > Ditto here; not sure we can kill this renderer. Same here. We should leave in case of hostile renderer until we support revoking permissions. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:248: // Returns the origin for the frame with "frame_rounting_id" in this process. On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > sp: rounting Done. On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > s/this process/render_process_id_/ Done.
I got through bluetooth_dispatcher_host.cc. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:116: const std::string BluetoothAllowedDevicesMap::GenerateDeviceId( On 2016/01/13 01:41:43, ortuno wrote: > On 2016/01/06 at 00:47:56, Jeffrey Yasskin wrote: > > Don't make by-value return types const: it prevents moving from the result. > > Done. You mean it prevents the optimization in which the object is moved rather > than copied right? Yep. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:33: // Adds the Bluetooth Device with |device_address| to the map of allowed On 2016/01/13 01:41:43, ortuno wrote: > On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > > Does this still work if the device is already in the map? As you pointed out, > it should add the services to this origin/device's allowed service list. > > Implementing features that are not yet in the spec could lead to inconsistencies > between browsers. So this implements what the spec says. Fair enough. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:956: // Make sure the origin is allowed to access the device. On 2016/01/13 01:41:44, ortuno wrote: > On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > > Instead of "Make sure", we should probably say why returning is the right > thing to do. e.g. "If the frame navigates to a different origin while the > StopNotifications() request is in flight, we won't find the right device > address, so we can't stop its notifications. Return instead." (Although that's > probably not the right reason, since it would imply that we leak the > notification sessions on navigation, and we shouldn't let that happen.) > > These checks are in place in case of a hostile renderer. There isn't much a > hostile renderer could do with stopNotifications(), so I could remove it. The effect would be that a renderer could guess another origin's characteristic ID, navigate there, and then stop their session. Because the IDs are 128 bits and random, that guess isn't going to happen, so I think it's safe to remove the early-return. But mention that analysis in a comment. https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1285: this, bad_message::BDH_DEVICE_NOT_ALLOWED_FOR_ORIGIN); On 2016/01/13 01:41:44, ortuno wrote: > On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > > I'm not sure we can kill the renderer for using a bad device: if the user > revokes access while a message is in flight, I think we'll hit this case, and we > don't want to kill that renderer. > > True. It would happen anytime you revoked access even if there are no messages > in flight. Since it's not spec'ed yet and we don't support revoking permissions > the only case in which the renderer would crash would be a hostile renderer. So > I propose we leave this in until it's spec'ed and we support revoking > permissions. 'k. https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:50: std::string device_id = GenerateDeviceId(origin); This can still be const, since it's never moved from. This function isn't long enough to make that important though. https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:68: std::string device_id = Assign this from GetDeviceId: const std::string device_id = GetDeviceId(origin, device_address); if (device_id.empty()) { return; } ... CHECK(origin_to_device_id_to_address_map_[origin].erase(device_id)); https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc (right): https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:62: test_origin2, device_address2, filters, optional_services); Add a test with s/device_address2/device_address1/, I think. I want to check that the second origin gets a different ID from the first. I think the test you have is primarily useful for the allowed_devices_map.GetDeviceAddress(test_origin1, device_id2)) == "" case; you can probably remove the check that retrieving the device address/id works, since it's tested in AddDeviceToMap. https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:127: // Test that we can retrieve the device address/id. "still retrieve the added device address/id"
LGTM after the comments in my previous email. https://codereview.chromium.org/1502663003/diff/120001/content/renderer/bluet... File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1502663003/diff/120001/content/renderer/bluet... content/renderer/bluetooth/bluetooth_dispatcher.cc:251: CurrentWorkerId(), request_id, frame_routing_id, I wish we had different types for routing IDs, request IDs, etc., so if we got this in the wrong order by accident, the compiler would tell us. You don't need to do anything in this CL; I'm just complaining.
Thanks! https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:50: std::string device_id = GenerateDeviceId(origin); On 2016/01/13 at 02:31:36, Jeffrey Yasskin wrote: > This can still be const, since it's never moved from. This function isn't long enough to make that important though. Done. https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:68: std::string device_id = On 2016/01/13 at 02:31:36, Jeffrey Yasskin wrote: > Assign this from GetDeviceId: > > const std::string device_id = GetDeviceId(origin, device_address); > if (device_id.empty()) { > return; > } > ... > CHECK(origin_to_device_id_to_address_map_[origin].erase(device_id)); Done. https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc (right): https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:62: test_origin2, device_address2, filters, optional_services); On 2016/01/13 at 02:31:36, Jeffrey Yasskin wrote: > Add a test with s/device_address2/device_address1/, I think. I want to check that the second origin gets a different ID from the first. > Done. > I think the test you have is primarily useful for the allowed_devices_map.GetDeviceAddress(test_origin1, device_id2)) == "" case; you can probably remove the check that retrieving the device address/id works, since it's tested in AddDeviceToMap. hrmm I kept the check for retrieving with address/id. I'm scared of replacing a previous device and not noticing or putting the new device in the wrong origin. Also added a test for when adding two devices from the same origin. I'm still trying to figure out how to write good unit tests so feel free to push back. https://codereview.chromium.org/1502663003/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:127: // Test that we can retrieve the device address/id. On 2016/01/13 at 02:31:36, Jeffrey Yasskin wrote: > "still retrieve the added device address/id" Done. https://codereview.chromium.org/1502663003/diff/120001/content/renderer/bluet... File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1502663003/diff/120001/content/renderer/bluet... content/renderer/bluetooth/bluetooth_dispatcher.cc:251: CurrentWorkerId(), request_id, frame_routing_id, On 2016/01/13 at 19:25:34, Jeffrey Yasskin wrote: > I wish we had different types for routing IDs, request IDs, etc., so if we got this in the wrong order by accident, the compiler would tell us. You don't need to do anything in this CL; I'm just complaining. I feel you. Hopefully with mojo we can get rid of a couple of these.
ortuno@chromium.org changed reviewers: + isherman@chromium.org, nick@chromium.org
nick: PTAL at bad_message.h isherman: PTAL at histograms
https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:956: // Make sure the origin is allowed to access the device. On 2016/01/13 at 02:31:36, Jeffrey Yasskin wrote: > On 2016/01/13 01:41:44, ortuno wrote: > > On 2016/01/06 at 00:47:57, Jeffrey Yasskin wrote: > > > Instead of "Make sure", we should probably say why returning is the right > > thing to do. e.g. "If the frame navigates to a different origin while the > > StopNotifications() request is in flight, we won't find the right device > > address, so we can't stop its notifications. Return instead." (Although that's > > probably not the right reason, since it would imply that we leak the > > notification sessions on navigation, and we shouldn't let that happen.) > > > > These checks are in place in case of a hostile renderer. There isn't much a > > hostile renderer could do with stopNotifications(), so I could remove it. > > The effect would be that a renderer could guess another origin's characteristic ID, navigate there, and then stop their session. Because the IDs are 128 bits and random, that guess isn't going to happen, so I think it's safe to remove the early-return. But mention that analysis in a comment. As discussed, I added a comment explaining why we need this check.
ortuno@chromium.org changed reviewers: + palmer@chromium.org
palmer: PTAL at content/common/bluetooth/bluetooth_messages.h
https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:26: bytes.size() + 1 /* length_with_null */), Possible performance nit: Will the call to |WriteInto| cause the string to re-allocate? It was born with 16 characters, and now it's being regrown to handle 17? https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:65: if (device_id.empty()) { Could this be a DCHECK? Do we expect it to happen in production, or for reasons other than programmer error? https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:90: const DeviceAddressToIdMap& device_address_to_id_map = Nit: this could be const auto&. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:111: if (id_iter == device_id_to_address_map.end()) { Nit: Potentially clearer code: return id_iter == device_id_to_address_map.end() ? base::EmptyString() : id_iter->second; https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:135: for (const BluetoothScanFilter& filter : filters) { Nit: Could use const auto& here. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right): https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:37: const std::string& origin, This should probably be a url::Origin, and not a string. Right? https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:343: GURL origin, It's possible/likely this should be url::Origin, too. We've been having bugs lately due to semantic/type mismatch of this kind. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:668: render_frame_host->GetLastCommittedURL().GetOrigin(), filters, Exactly this. This is likely to be better: url::Origin(render_frame_host->GetLastCommittedURL())
histograms.xml lgtm
palmer: Ready for review again! https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:26: bytes.size() + 1 /* length_with_null */), On 2016/01/13 at 22:52:06, palmer wrote: > Possible performance nit: Will the call to |WriteInto| cause the string to re-allocate? It was born with 16 characters, and now it's being regrown to handle 17? Done. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:65: if (device_id.empty()) { On 2016/01/13 at 22:52:06, palmer wrote: > Could this be a DCHECK? Do we expect it to happen in production, or for reasons other than programmer error? Done. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:90: const DeviceAddressToIdMap& device_address_to_id_map = On 2016/01/13 at 22:52:06, palmer wrote: > Nit: this could be const auto&. Done. Also done in the next function. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:111: if (id_iter == device_id_to_address_map.end()) { On 2016/01/13 at 22:52:06, palmer wrote: > Nit: Potentially clearer code: > > return id_iter == device_id_to_address_map.end() ? base::EmptyString() : id_iter->second; Done. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:135: for (const BluetoothScanFilter& filter : filters) { On 2016/01/13 at 22:52:06, palmer wrote: > Nit: Could use const auto& here. Done. I kept the other ones as is because filter.services doesn't give any indication the values are BluetoothUUIDs. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right): https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:37: const std::string& origin, On 2016/01/13 at 22:52:06, palmer wrote: > This should probably be a url::Origin, and not a string. Right? Done. https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:343: GURL origin, On 2016/01/13 at 22:52:06, palmer wrote: > It's possible/likely this should be url::Origin, too. We've been having bugs lately due to semantic/type mismatch of this kind. Done. I changed all except the one in RunBluetoothChooser, which was there already. I added a TODO and opened an issue though. Fixing it will require changing the interface and fixing all classes the implement that function :/ https://codereview.chromium.org/1502663003/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:668: render_frame_host->GetLastCommittedURL().GetOrigin(), filters, On 2016/01/13 at 22:52:06, palmer wrote: > Exactly this. This is likely to be better: > > url::Origin(render_frame_host->GetLastCommittedURL()) Changed to GetLastCommittedURL which was recently implemented. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
nick & palmer: ping?
LGTM from the perspective of IPC security. But, perhaps the code and comments could be made more clear with more typeful interfaces. If you decide to make those changes, you don't need to wait for my review though. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:22: const size_t kIdLength = 16 /* 128bits */; Nit: I don't know if these comments are strictly necessary. (But also they don't hurt anything.) https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:50: // trustworthy origins; since Bluetooth is only available for trustworthy Nit: The correct terminology might be "potentially-trustworthy"? Not sure. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right): https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:28: // the generated |device_id| for each origin for a given device. Nit: I'd reword this comment, along these lines: |AddDevice| generates device IDs, which are random strings that are unique for each (origin, device address) pair. (The |...| notation should be reserved for specific identifiers.) https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:35: // devices for that origin. Returns the generated |device_id| for |origin| ...then you can change the last sentence in this comment to something like: Generates and returns a device ID for the (|origin|, |device_address|) pair. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:57: // Returns the Bluetooth Device's address from |device_id_for_origin|. Returns Minor Nit: Maybe word this in such a way that the comment for this public function does not depend on the reader knowing about a private data member? Something like: For |device_id| in |origin|, returns the Bluetooth device's address. If there is no such |device_id| in |origin|, returns an empty string. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:74: std::map<url::Origin, DeviceAddressToIdMap> This 3-way mapping system seems complex. I wonder if it could be simplified, such as by combining the origin and the UUID? device-id := origin + "," + uuid or the like? Perhaps you've already considered this and rejected it. Just an idea. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:984: // Check the origin is allowed to access the device. We perform this check in Since this code is the same as that on line 1010, I wonder if you could extract it out into a helper function. A meaningful name and interface might even obviate the comments, e.g.: bool CanFrameAccessInstance(int frame_id, const std::string& characteristic_instance_id) { return QueryCacheForCharacteristic(GetOrigin(frame_id), characteristic_instance_id) .outcome != CacheQueryOutcome::BAD_RENDERER; } https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1147: // device_id is device_address that RequestDeviceSession passes to Nit: Delineate identifiers with |...|. Also, conflating device IDs with device addresses seems a bit confusing? I may have mentioned this before, but it may be that we need more typeful interfaces here, e.g. a |DeviceId| class that has-a string and whose constructor validates that the string adheres to the lexical rules for a device ID. Then |AddDevice| can explain itself more clearly by returning a |DeviceId|, functions can declare that they take |DeviceId|s as input, et c.
palmer: Thanks! I fixed the comments and added a new function for the repeated code. Also opened an issue on the Bluetooth spec to make the mapping easier and another issue to add a class to better distinguish between device_ids. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right): https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.cc:50: // trustworthy origins; since Bluetooth is only available for trustworthy On 2016/01/15 at 01:13:59, palmer wrote: > Nit: The correct terminology might be "potentially-trustworthy"? Not sure. Done. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right): https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:28: // the generated |device_id| for each origin for a given device. On 2016/01/15 at 01:13:59, palmer wrote: > Nit: I'd reword this comment, along these lines: > > |AddDevice| generates device IDs, which are random strings that are unique for each (origin, device address) pair. > > (The |...| notation should be reserved for specific identifiers.) Done. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:35: // devices for that origin. Returns the generated |device_id| for |origin| On 2016/01/15 at 01:13:59, palmer wrote: > ...then you can change the last sentence in this comment to something like: > > Generates and returns a device ID for the (|origin|, |device_address|) pair. Done. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:57: // Returns the Bluetooth Device's address from |device_id_for_origin|. Returns On 2016/01/15 at 01:13:59, palmer wrote: > Minor Nit: Maybe word this in such a way that the comment for this public function does not depend on the reader knowing about a private data member? Something like: > > For |device_id| in |origin|, returns the Bluetooth device's address. If there is no such |device_id| in |origin|, returns an empty string. Done. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_allowed_devices_map.h:74: std::map<url::Origin, DeviceAddressToIdMap> On 2016/01/15 at 01:13:59, palmer wrote: > This 3-way mapping system seems complex. I wonder if it could be simplified, such as by combining the origin and the UUID? > > device-id := origin + "," + uuid > > or the like? Perhaps you've already considered this and rejected it. Just an idea. Interesting. This class implements the spec so I opened an issue there: https://github.com/WebBluetoothCG/web-bluetooth/issues/199 https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:984: // Check the origin is allowed to access the device. We perform this check in On 2016/01/15 at 01:13:59, palmer wrote: > Since this code is the same as that on line 1010, I wonder if you could extract it out into a helper function. A meaningful name and interface might even obviate the comments, e.g.: > > bool CanFrameAccessInstance(int frame_id, const std::string& characteristic_instance_id) { > return QueryCacheForCharacteristic(GetOrigin(frame_id), > characteristic_instance_id) > .outcome != CacheQueryOutcome::BAD_RENDERER; > } Done. https://codereview.chromium.org/1502663003/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1147: // device_id is device_address that RequestDeviceSession passes to On 2016/01/15 at 01:13:59, palmer wrote: > Nit: Delineate identifiers with |...|. > > Also, conflating device IDs with device addresses seems a bit confusing? I may have mentioned this before, but it may be that we need more typeful interfaces here, e.g. a |DeviceId| class that has-a string and whose constructor validates that the string adheres to the lexical rules for a device ID. Then |AddDevice| can explain itself more clearly by returning a |DeviceId|, functions can declare that they take |DeviceId|s as input, et c. I know, I spent quite a bit of time trying to decide the best way to tackle this issue, which becomes even more confusing since BluetoothDevice's also have a device_id. I like your idea and opened an issue for it: http://crbug.com/577962
rubber stamp lgtm
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org, isherman@chromium.org, palmer@chromium.org Link to the patchset: https://codereview.chromium.org/1502663003/#ps220001 (title: "Make global consts non-reference")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1502663003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1502663003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, jyasskin@chromium.org, nick@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1502663003/#ps240001 (title: "Fix merge conflicts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1502663003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1502663003/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Implement allowed devices map Add a selected device to the list of allowed devices map and, before interacting with the device, check that the origin is allowed to access that device. BUG=493459 ========== to ========== bluetooth: Implement allowed devices map Add a selected device to the list of allowed devices map and, before interacting with the device, check that the origin is allowed to access that device. BUG=493459 Committed: https://crrev.com/154400787a01efedca9063d98a4b0d06ebf39089 Cr-Commit-Position: refs/heads/master@{#369870} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/154400787a01efedca9063d98a4b0d06ebf39089 Cr-Commit-Position: refs/heads/master@{#369870} |
