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

Issue 2821263005: Add U2F request state machines (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by Casey Piper
Modified:
3 days, 14 hours 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
Commit queue not available (can’t edit this change).

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 ...
1 week, 1 day ago (2017-04-20 23:31:08 UTC) #18
Reilly Grant
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&. ...
1 week 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: > ...
1 week ago (2017-04-21 18:42:27 UTC) #29
Reilly Grant
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: ...
1 week 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: > ...
1 week ago (2017-04-21 22:55:16 UTC) #31
Reilly Grant
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 ...
1 week 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 ...
1 week 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
1 week 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
1 week 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 ...
1 week ago (2017-04-22 06:18:26 UTC) #44
pkalinnikov
5 days, 3 hours 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)..
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46