[WebAuthN][BLE] Simplify Design of FidoBleConnection |
|||||||||
Issue descriptionIn its current implementation, FidoBleConnection tries to be resilient to errors and re-connect to the underlying BLE device if appropriate. However, given the short lived nature of FidoBleConnection the re-connect logic should be handled higher up the stack, for example at the discovery level. Furthermore, the reconnect logic is ineffective anyway, as the first disconnect results in a kDeviceError state in the FidoBleDevice, which is considered terminal and the device never recovers. I propose the following changes: - Remove ConnectionStatusCallback and ReadCallback from FidoBleConnection's interface. ConnectionstatusCallback should be simplified to a simple Connection OnceCallback that reports whether the initial GattConnection succeeded. The ReadCallback should not be exposed to consumers, as this is an implementation detail and should only be handled in FidoBleConnection and FidoBleTransaction. - Stop listening to the DeviceAdded, DeviceChanged and DeviceAddressChanged events, as these should be handled in a higher level. Only the GattCharacteristicValueChanged and GattServicesDiscovered events are relevant to the FidoBleConnection. Lastly FidoBleConnection should support reading and writing the fidoServiceRevisionBitfield to agree on a Fido version with the device.
,
Sep 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1ea8adb0d4323a0829bcc0dc0dd8b991497004d0 commit 1ea8adb0d4323a0829bcc0dc0dd8b991497004d0 Author: Jan Wilken Doerrie <jdoerrie@chromium.org> Date: Fri Sep 07 12:27:30 2018 [Fido][BLE] Add Selecting a Service Revision to FidoBleConnection This change implements selecting a service revision to FidoBleConnection. This is mandated by the CTAP spec when the fidoServiceRevisionBitfield characteristic is present. Furthermore, this change simplifies FidoBleConnection's interface by dropping the unused ReadServiceRevisions() and WriteServiceRevision() APIs. Appropriate tests are added. Bug: 880053 Change-Id: I35f6b39be9f90e7c668f5714f6cc26830966ae1c Reviewed-on: https://chromium-review.googlesource.com/1204013 Reviewed-by: Balazs Engedy <engedy@chromium.org> Commit-Queue: Balazs Engedy <engedy@chromium.org> Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org> Cr-Commit-Position: refs/heads/master@{#589498} [modify] https://crrev.com/1ea8adb0d4323a0829bcc0dc0dd8b991497004d0/device/fido/ble/fido_ble_connection.cc [modify] https://crrev.com/1ea8adb0d4323a0829bcc0dc0dd8b991497004d0/device/fido/ble/fido_ble_connection.h [modify] https://crrev.com/1ea8adb0d4323a0829bcc0dc0dd8b991497004d0/device/fido/ble/fido_ble_connection_unittest.cc [modify] https://crrev.com/1ea8adb0d4323a0829bcc0dc0dd8b991497004d0/device/fido/ble/mock_fido_ble_connection.cc [modify] https://crrev.com/1ea8adb0d4323a0829bcc0dc0dd8b991497004d0/device/fido/ble/mock_fido_ble_connection.h
,
Sep 11
Requesting merge of r589498 into M70 (branch 3538).
,
Sep 11
,
Sep 11
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/447efab479d75b69cb19cf580ddafcd4c8a31ae3 commit 447efab479d75b69cb19cf580ddafcd4c8a31ae3 Author: Jan Wilken Doerrie <jdoerrie@chromium.org> Date: Wed Sep 12 11:37:43 2018 [Fido][BLE] Add Selecting a Service Revision to FidoBleConnection This change implements selecting a service revision to FidoBleConnection. This is mandated by the CTAP spec when the fidoServiceRevisionBitfield characteristic is present. Furthermore, this change simplifies FidoBleConnection's interface by dropping the unused ReadServiceRevisions() and WriteServiceRevision() APIs. Appropriate tests are added. Bug: 880053 Change-Id: I35f6b39be9f90e7c668f5714f6cc26830966ae1c Reviewed-on: https://chromium-review.googlesource.com/1204013 Reviewed-by: Balazs Engedy <engedy@chromium.org> Commit-Queue: Balazs Engedy <engedy@chromium.org> Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#589498}(cherry picked from commit 1ea8adb0d4323a0829bcc0dc0dd8b991497004d0) Reviewed-on: https://chromium-review.googlesource.com/1221229 Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#320} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/447efab479d75b69cb19cf580ddafcd4c8a31ae3/device/fido/ble/fido_ble_connection.cc [modify] https://crrev.com/447efab479d75b69cb19cf580ddafcd4c8a31ae3/device/fido/ble/fido_ble_connection.h [modify] https://crrev.com/447efab479d75b69cb19cf580ddafcd4c8a31ae3/device/fido/ble/fido_ble_connection_unittest.cc [modify] https://crrev.com/447efab479d75b69cb19cf580ddafcd4c8a31ae3/device/fido/ble/mock_fido_ble_connection.cc [modify] https://crrev.com/447efab479d75b69cb19cf580ddafcd4c8a31ae3/device/fido/ble/mock_fido_ble_connection.h
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a72231cfcd50715001035d0835e885b581578bd7 commit a72231cfcd50715001035d0835e885b581578bd7 Author: jdoerrie <jdoerrie@chromium.org> Date: Wed Sep 12 13:20:25 2018 [Fido] Simplify FidoBleConnection Connect Logic This change simplifies FidoBleConnection's connection logic by making the ConnectionStatusCallback a OnceCallback and taking it as argument in the Connect method. Furthermore, listeners for high level events are dropped, as these events should be handled at an upper layer. Appropriate tests are added. Bug: 880053 Change-Id: Ieb80d7042586916b020e77a1fa6e985a3f12e71d Reviewed-on: https://chromium-review.googlesource.com/1215543 Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org> Reviewed-by: Balazs Engedy <engedy@chromium.org> Cr-Commit-Position: refs/heads/master@{#590656} [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/ble/fido_ble_connection.cc [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/ble/fido_ble_connection.h [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/ble/fido_ble_connection_unittest.cc [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/ble/fido_ble_device.cc [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/ble/fido_ble_device.h [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/ble/fido_ble_device_unittest.cc [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/ble/mock_fido_ble_connection.cc [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/ble/mock_fido_ble_connection.h [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/cable/fido_cable_device_unittest.cc [modify] https://crrev.com/a72231cfcd50715001035d0835e885b581578bd7/device/fido/cable/fido_cable_handshake_handler_unittest.cc
,
Sep 13
,
Sep 13
Approved for M70
,
Sep 13
,
Sep 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9084b3c5fb29f8986841b50a488f04848e9d0891 commit 9084b3c5fb29f8986841b50a488f04848e9d0891 Author: jdoerrie <jdoerrie@chromium.org> Date: Thu Sep 13 17:54:50 2018 [Fido] Simplify FidoBleConnection Connect Logic This change simplifies FidoBleConnection's connection logic by making the ConnectionStatusCallback a OnceCallback and taking it as argument in the Connect method. Furthermore, listeners for high level events are dropped, as these events should be handled at an upper layer. Appropriate tests are added. Bug: 880053 Change-Id: Ieb80d7042586916b020e77a1fa6e985a3f12e71d Reviewed-on: https://chromium-review.googlesource.com/1215543 Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org> Reviewed-by: Balazs Engedy <engedy@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#590656}(cherry picked from commit a72231cfcd50715001035d0835e885b581578bd7) Reviewed-on: https://chromium-review.googlesource.com/1225170 Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#374} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/ble/fido_ble_connection.cc [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/ble/fido_ble_connection.h [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/ble/fido_ble_connection_unittest.cc [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/ble/fido_ble_device.cc [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/ble/fido_ble_device.h [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/ble/fido_ble_device_unittest.cc [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/ble/mock_fido_ble_connection.cc [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/ble/mock_fido_ble_connection.h [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/cable/fido_cable_device_unittest.cc [modify] https://crrev.com/9084b3c5fb29f8986841b50a488f04848e9d0891/device/fido/cable/fido_cable_handshake_handler_unittest.cc
,
Sep 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3848eaa5c1eb1bc30e1aa9d45c636b56f1905bc9 commit 3848eaa5c1eb1bc30e1aa9d45c636b56f1905bc9 Author: jdoerrie <jdoerrie@chromium.org> Date: Wed Sep 19 15:02:03 2018 [Fido][BLE] Fix Bugs in FidoBleTransaction This change fixes a number of bugs in FidoBleTransaction and adds corresponding tests. In particular, it fixes a race condition, where the completion callback was run before the last write was acknowlegded. In addition, it improves the robustness with regard to malformed input. Bug: 880053 Change-Id: I81f6d5c02d01b9d7ea1a184a7236cd74d52f356d Reviewed-on: https://chromium-review.googlesource.com/1224383 Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org> Reviewed-by: Balazs Engedy <engedy@chromium.org> Cr-Commit-Position: refs/heads/master@{#592398} [modify] https://crrev.com/3848eaa5c1eb1bc30e1aa9d45c636b56f1905bc9/device/BUILD.gn [modify] https://crrev.com/3848eaa5c1eb1bc30e1aa9d45c636b56f1905bc9/device/fido/ble/fido_ble_frames.cc [modify] https://crrev.com/3848eaa5c1eb1bc30e1aa9d45c636b56f1905bc9/device/fido/ble/fido_ble_frames.h [modify] https://crrev.com/3848eaa5c1eb1bc30e1aa9d45c636b56f1905bc9/device/fido/ble/fido_ble_transaction.cc [modify] https://crrev.com/3848eaa5c1eb1bc30e1aa9d45c636b56f1905bc9/device/fido/ble/fido_ble_transaction.h [add] https://crrev.com/3848eaa5c1eb1bc30e1aa9d45c636b56f1905bc9/device/fido/ble/fido_ble_transaction_unittest.cc
,
Sep 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2c1260056a44d87a13b864a1d00e93b9ea5c0ebc commit 2c1260056a44d87a13b864a1d00e93b9ea5c0ebc Author: Yuankai Yu <yyk@google.com> Date: Mon Sep 24 19:48:59 2018 [caBLE] Fix a bug that keep alive frames are not handled correctly. If there are more than 1 keep alive frames received before getting the ack on request, the second will be thought as a continuation fragment and fail with "Malformed Frame Continuation Fragment". Change-Id: I69fb5c024f14fe19bfc60da2a2392b663bdcea66 Bug: 880053 Reviewed-on: https://chromium-review.googlesource.com/1237267 Commit-Queue: Yuankai Yu <yyk@google.com> Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org> Cr-Commit-Position: refs/heads/master@{#593644} [modify] https://crrev.com/2c1260056a44d87a13b864a1d00e93b9ea5c0ebc/device/fido/ble/fido_ble_transaction.cc [modify] https://crrev.com/2c1260056a44d87a13b864a1d00e93b9ea5c0ebc/device/fido/ble/fido_ble_transaction_unittest.cc
,
Jan 16
(6 days ago)
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by jdoerrie@chromium.org
, Sep 3Labels: -Pri-3 Pri-2
Owner: jdoerrie@chromium.org
Status: Assigned (was: Untriaged)