Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2821263005: Add U2F request state machines (Closed)

Created:
7 months ago by Casey Piper
Modified:
6 months, 3 weeks ago
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add U2F request state machines U2fRequest base class is extended by each type of U2F request. Common functions of enumeration and device iteration are defined in the base class. U2fSign performs an entire sign request on any attached U2F devices, and U2FRegister performs the registration request on the devices. BUG=686310 Review-Url: https://codereview.chromium.org/2821263005 Cr-Commit-Position: refs/heads/master@{#466529} Committed: https://chromium.googlesource.com/chromium/src/+/892eb34117f811aa62c821c9129dc53110266e9a

Patch Set 1 : Add U2fRequest, U2fSign, and U2fRegister classes #

Total comments: 8

Patch Set 2 : Compilation changes #

Total comments: 17

Patch Set 3 : Switch to const vectors and modify tests #

Total comments: 20

Patch Set 4 : Move return code to own header. Fix review comments #

Total comments: 6

Patch Set 5 : Call Start before adding devices #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1117 lines, -23 lines) Patch
M device/BUILD.gn View 1 3 chunks +7 lines, -3 lines 0 comments Download
M device/u2f/BUILD.gn View 1 2 3 4 2 chunks +22 lines, -0 lines 0 comments Download
A device/u2f/mock_u2f_device.h View 1 1 chunk +43 lines, -0 lines 0 comments Download
A device/u2f/mock_u2f_device.cc View 1 chunk +53 lines, -0 lines 0 comments Download
M device/u2f/u2f_apdu_response.h View 1 chunk +1 line, -0 lines 0 comments Download
M device/u2f/u2f_device.h View 1 2 3 2 chunks +2 lines, -7 lines 0 comments Download
M device/u2f/u2f_device.cc View 1 2 3 4 chunks +12 lines, -13 lines 0 comments Download
A device/u2f/u2f_register.h View 1 2 3 1 chunk +37 lines, -0 lines 0 comments Download
A device/u2f/u2f_register.cc View 1 2 3 1 chunk +61 lines, -0 lines 0 comments Download
A device/u2f/u2f_register_unittest.cc View 1 2 3 4 1 chunk +130 lines, -0 lines 0 comments Download
A device/u2f/u2f_request.h View 1 2 3 1 chunk +65 lines, -0 lines 0 comments Download
A device/u2f/u2f_request.cc View 1 2 3 1 chunk +148 lines, -0 lines 0 comments Download
A device/u2f/u2f_request_unittest.cc View 1 2 3 1 chunk +159 lines, -0 lines 0 comments Download
A device/u2f/u2f_return_code.h View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
A device/u2f/u2f_sign.h View 1 2 3 1 chunk +51 lines, -0 lines 0 comments Download
A device/u2f/u2f_sign.cc View 1 2 3 1 chunk +92 lines, -0 lines 0 comments Download
A device/u2f/u2f_sign_unittest.cc View 1 2 3 4 1 chunk +215 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 45 (34 generated)
Casey Piper
Hey Reilly, can you take a look? I can split this into smaller patches if ...
7 months ago (2017-04-20 23:31:08 UTC) #18
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_register.h File device/u2f/u2f_register.h (right): https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_register.h#newcode17 device/u2f/u2f_register.h:17: std::vector<uint8_t> app_param, std::vector is usually passed as a const&. ...
7 months ago (2017-04-21 17:19:17 UTC) #28
Casey Piper
https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_register.cc File device/u2f/u2f_register.cc (right): https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_register.cc#newcode28 device/u2f/u2f_register.cc:28: if (request) On 2017/04/21 17:19:16, Reilly Grant wrote: > ...
7 months ago (2017-04-21 18:42:27 UTC) #29
Reilly Grant (use Gerrit)
I think you missed my comments on patch 1. https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_request.h File device/u2f/u2f_request.h (right): https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_request.h#newcode18 device/u2f/u2f_request.h:18: ...
7 months ago (2017-04-21 20:10:02 UTC) #30
Casey Piper
https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_register.h File device/u2f/u2f_register.h (right): https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_register.h#newcode17 device/u2f/u2f_register.h:17: std::vector<uint8_t> app_param, On 2017/04/21 17:19:16, Reilly Grant wrote: > ...
7 months ago (2017-04-21 22:55:16 UTC) #31
Reilly Grant (use Gerrit)
lgtm with nits https://codereview.chromium.org/2821263005/diff/160001/device/u2f/BUILD.gn File device/u2f/BUILD.gn (right): https://codereview.chromium.org/2821263005/diff/160001/device/u2f/BUILD.gn#newcode28 device/u2f/BUILD.gn:28: ] Add "u2f_return_code.h" here. https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_register_unittest.cc File ...
7 months ago (2017-04-22 00:16:44 UTC) #32
Casey Piper
https://codereview.chromium.org/2821263005/diff/160001/device/u2f/BUILD.gn File device/u2f/BUILD.gn (right): https://codereview.chromium.org/2821263005/diff/160001/device/u2f/BUILD.gn#newcode28 device/u2f/BUILD.gn:28: ] On 2017/04/22 00:16:43, Reilly Grant wrote: > Add ...
7 months ago (2017-04-22 00:50:08 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2821263005/180001
7 months ago (2017-04-22 02:48:50 UTC) #40
commit-bot: I haz the power
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/892eb34117f811aa62c821c9129dc53110266e9a
7 months ago (2017-04-22 02:53:56 UTC) #43
findit-for-me
Findit(https://goo.gl/kROfz5) identified this CL at revision 466529 as the culprit for failures in the build ...
7 months ago (2017-04-22 06:18:26 UTC) #44
pkalinnikov
6 months, 4 weeks ago (2017-04-24 11:49:34 UTC) #45
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:180001) has been created in
https://codereview.chromium.org/2838573002/ by pkalinnikov@chromium.org.

The reason for reverting is: Reverting because U2fRequestTest.TestBasicMachine
fails consistently on MSan bots (Linux MSan, Linux ChromiumOS MSan)..

Powered by Google App Engine
This is Rietveld efc10ee0f