Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721223002:
Add support for U2fHidDevice interaction (Closed)
|
Created:
3 years, 9 months ago by Casey Piper Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for U2fHidDevice interaction
Add U2fHidDevice class. The class has an internal state machine
to abstract to lazily connect and initialize the device upon
instruction to transact with the device.
BUG=697282
Review-Url: https://codereview.chromium.org/2721223002
Cr-Commit-Position: refs/heads/master@{#458196}
Committed: https://chromium.googlesource.com/chromium/src/+/3a4044706b783686a071046c96ba3a09ca781837
Patch Set 1 #Patch Set 2 : Modify unittest BUILD file #
Total comments: 32
Patch Set 3 : Add queueing of requests if device is busy & change Callback to OnceCallback #Patch Set 4 : Add missing else statement #
Total comments: 12
Patch Set 5 : Fix nits and segfault #
Total comments: 2
Patch Set 6 : Add comment and change logic for executing pending transactions #
Total comments: 2
Patch Set 7 : Ensure queued callbacks are called in error scenarios #
Total comments: 8
Patch Set 8 : Modify failure tests to use mock connection #Patch Set 9 : Add //device/hid:mocks to unittest build file #
Total comments: 8
Patch Set 10 : Use MockHidConnection to fail writes in unittest #
Dependent Patchsets: Messages
Total messages: 72 (52 generated)
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add support for U2fHidDevice interaction Add U2fHidDevice class. The class has an internal state machine to abstract to lazy connect and initialize the device upon instruction to transact with the device. BUG=697282 ========== to ========== Add support for U2fHidDevice interaction Add U2fHidDevice class. The class has an internal state machine to abstract to lazily connect and initialize the device upon instruction to transact with the device. BUG=697282 ==========
piperc@chromium.org changed reviewers: + juanlang@chromium.org, kpaulhamus@chromium.org, reillyg@chromium.org
piperc@chromium.org changed required reviewers: + reillyg@chromium.org
Now that I see more of how the message and command classes are going to be used I have a couple suggestions: * Instead of using net::IOBufferWithSize() try using std::vector<uint8_t> since I don't think you need the ability to share buffers anywhere. * It doesn't look like you need to be sharing U2fApduCommand or U2fMessage objects anywhere so you could switch to using std::unique_ptr to explicitly transfer ownership of them through their lifetime. If you agree these can both be done in followup patches. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_device.cc File device/u2f/u2f_device.cc (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_device.c... device/u2f/u2f_device.cc:6: #include "base/bind.h" nit: newline between including this file's header and other headers. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:48: switch (state_) { Since connection and channel allocation are asynchronous operations this function should either handle a caller trying to send two commands in sequence (before waiting for a callback) or detect this case and fail or DCHECK depending on if this is enforced at a higher layer. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:80: if (connection != nullptr) { if (connection) https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:92: // Send random nonce to device to verify received message Please add a comment documenting that this nonce does not have to be cryptographically secure, or use crypto::RandBytes(). https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:94: std::vector<uint8_t> nonce(rand, std::end(rand)); You can do this without copying: std::vector<uint8_t> nonce(8); base::RandBytes(nonce.data(), nonce.size()); https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:144: if (connection_ == nullptr || message == nullptr || if (!connection_ || !message || https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:163: scoped_refptr<net::IOBufferWithSize> buffer = message->PopNextPacket(); Can you replace these statements with: WriteMessage(message, response_expected, callback); https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:176: if (connection_ == nullptr) { if (!connection_) https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:199: if (read_message == nullptr) { if (!read_message) https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:247: if (success && message != nullptr) if (success && message) https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:256: if (!(capabilities_ | kWinkCapability) || state_ != State::IDLE) { capabilities_ & kWinkCapability? https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:277: return id.str(); return base::StringPrintf("hid:%d", device_info_->device_id()); https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.h (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.h:91: } nit: newline and // namespace device https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device_unittest.cc (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device_unittest.cc:34: } No braces around single-line if. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device_unittest.cc:112: for (auto it = u2f_devices.cbegin(); it != u2f_devices.end(); ++it) { for (auto& device : u2f_devices) https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device_unittest.cc:116: ASSERT_EQ(version, U2fDevice::ProtocolVersion::U2F_V2); EXPECT_EQ since this if this fails the test won't crash.
I agree that I should be able to use std::unique_ptr rather than shared pointers. I will update U2fMessage and U2fApdu classes to unique_ptrs in later patches. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_device.cc File device/u2f/u2f_device.cc (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_device.c... device/u2f/u2f_device.cc:6: #include "base/bind.h" On 2017/03/01 22:48:32, Reilly Grant wrote: > nit: newline between including this file's header and other headers. Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:48: switch (state_) { On 2017/03/01 22:48:33, Reilly Grant wrote: > Since connection and channel allocation are asynchronous operations this > function should either handle a caller trying to send two commands in sequence > (before waiting for a callback) or detect this case and fail or DCHECK depending > on if this is enforced at a higher layer. Done, any requests that arrive while the device is active will now be queued. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:80: if (connection != nullptr) { On 2017/03/01 22:48:33, Reilly Grant wrote: > if (connection) Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:92: // Send random nonce to device to verify received message On 2017/03/01 22:48:33, Reilly Grant wrote: > Please add a comment documenting that this nonce does not have to be > cryptographically secure, or use crypto::RandBytes(). Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:94: std::vector<uint8_t> nonce(rand, std::end(rand)); On 2017/03/01 22:48:33, Reilly Grant wrote: > You can do this without copying: > > std::vector<uint8_t> nonce(8); > base::RandBytes(nonce.data(), nonce.size()); Acknowledged. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:144: if (connection_ == nullptr || message == nullptr || On 2017/03/01 22:48:32, Reilly Grant wrote: > if (!connection_ || !message || Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:163: scoped_refptr<net::IOBufferWithSize> buffer = message->PopNextPacket(); On 2017/03/01 22:48:33, Reilly Grant wrote: > Can you replace these statements with: > > WriteMessage(message, response_expected, callback); Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:176: if (connection_ == nullptr) { On 2017/03/01 22:48:32, Reilly Grant wrote: > if (!connection_) Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:199: if (read_message == nullptr) { On 2017/03/01 22:48:32, Reilly Grant wrote: > if (!read_message) Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:247: if (success && message != nullptr) On 2017/03/01 22:48:32, Reilly Grant wrote: > if (success && message) Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:256: if (!(capabilities_ | kWinkCapability) || state_ != State::IDLE) { On 2017/03/01 22:48:32, Reilly Grant wrote: > capabilities_ & kWinkCapability? Ah, yeah. Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:277: return id.str(); On 2017/03/01 22:48:32, Reilly Grant wrote: > return base::StringPrintf("hid:%d", device_info_->device_id()); StringPrintf only seems to work when device_id is typedef'd from an integer. Since device_id is a uint64_t on Mac OS and a string on other platforms, that isn't compiling. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.h (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.h:91: } On 2017/03/01 22:48:33, Reilly Grant wrote: > nit: newline and // namespace device Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device_unittest.cc (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device_unittest.cc:34: } On 2017/03/01 22:48:33, Reilly Grant wrote: > No braces around single-line if. Done. https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device_unittest.cc:112: for (auto it = u2f_devices.cbegin(); it != u2f_devices.end(); ++it) { On 2017/03/01 22:48:33, Reilly Grant wrote: > for (auto& device : u2f_devices) Done.
lgtm with nits https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:277: return id.str(); On 2017/03/04 02:06:28, Casey Piper wrote: > On 2017/03/01 22:48:32, Reilly Grant wrote: > > return base::StringPrintf("hid:%d", device_info_->device_id()); > > StringPrintf only seems to work when device_id is typedef'd from an integer. > Since device_id is a uint64_t on Mac OS and a string on other platforms, that > isn't compiling. Curses, I suppose I can only blame myself for that. We generally try to avoid using STL streams but this usage seems fine. https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_device.h File device/u2f/u2f_device.h (right): https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_device.h... device/u2f/u2f_device.h:55: virtual std::string Id() = 0; GetId https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:256: state_ = State::IDLE; state_ = success ? State::IDLE : State::DEVICE_ERROR; https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:262: std::move((*pending_transactions_.begin()).first); pending_transactions_.front().first https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:263: DeviceCallback pending_cb = (*pending_transactions_.begin()).second; pending_transactions_.front().second https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:264: pending_transactions_.erase(pending_transactions_.begin()); I would use an std::list so you can use pop_front(). https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.h (right): https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.h:63: U2fHidMessageCallback); U2fHidMessageCallback callback
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
piperc@chromium.org changed reviewers: + davidben@chromium.org
piperc@chromium.org changed required reviewers: + davidben@chromium.org
https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_device.h File device/u2f/u2f_device.h (right): https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_device.h... device/u2f/u2f_device.h:55: virtual std::string Id() = 0; On 2017/03/06 20:49:23, Reilly Grant wrote: > GetId Done. https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:256: state_ = State::IDLE; On 2017/03/06 20:49:23, Reilly Grant wrote: > state_ = success ? State::IDLE : State::DEVICE_ERROR; Done. https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:262: std::move((*pending_transactions_.begin()).first); On 2017/03/06 20:49:23, Reilly Grant wrote: > pending_transactions_.front().first Done. https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:263: DeviceCallback pending_cb = (*pending_transactions_.begin()).second; On 2017/03/06 20:49:23, Reilly Grant wrote: > pending_transactions_.front().second Done. https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.cc:264: pending_transactions_.erase(pending_transactions_.begin()); On 2017/03/06 20:49:23, Reilly Grant wrote: > I would use an std::list so you can use pop_front(). Done. https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... File device/u2f/u2f_hid_device.h (right): https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_hid_devi... device/u2f/u2f_hid_device.h:63: U2fHidMessageCallback); On 2017/03/06 20:49:23, Reilly Grant wrote: > U2fHidMessageCallback callback Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2721223002/diff/120001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/120001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device.cc:265: } We should probably document that the reason for this logic is that callback.Run() may cause |this| to be invalidated. Perhaps this logic would be clearer: base::WeakPtr<U2fHidDevice> self = weak_factory_.GetWeakPtr(); callback.Run(success, response); // Executing |callback| may have freed |this|. Check |self| first. if (self && !pending_transactions_.empty()) { ... }
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
crypto DEPS lgtm. I didn't review the protocol or the rest of the code and assume you've gotten appropriate security review there.
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2721223002/diff/120001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/120001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device.cc:265: } On 2017/03/07 01:13:25, Reilly Grant wrote: > We should probably document that the reason for this logic is that > callback.Run() may cause |this| to be invalidated. Perhaps this logic would be > clearer: > > base::WeakPtr<U2fHidDevice> self = weak_factory_.GetWeakPtr(); > callback.Run(success, response); > > // Executing |callback| may have freed |this|. Check |self| first. > if (self && !pending_transactions_.empty()) { > ... > } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2721223002/diff/140001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/140001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device.cc:259: if (self && !pending_transactions_.empty()) { I just noticed that if !success then state_ == DEVICE_ERROR and so Transition will run pending_cb immediately but if there are other pending transactions they will never be completed. The same issue exists if we encounter a connection error or fail to allocate a channel. We should add tests for these cases.
Patchset #7 (id:150001) has been deleted
https://codereview.chromium.org/2721223002/diff/140001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/140001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device.cc:259: if (self && !pending_transactions_.empty()) { On 2017/03/07 23:08:38, Reilly Grant wrote: > I just noticed that if !success then state_ == DEVICE_ERROR and so Transition > will run pending_cb immediately but if there are other pending transactions they > will never be completed. > > The same issue exists if we encounter a connection error or fail to allocate a > channel. We should add tests for these cases. I modified the logic to always return through Transition, and for the Transition with a DEVICE_ERROR state to clear any queued requests.
Patchset #7 (id:170001) has been deleted
https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device.cc:69: // Executing |callback| may have freed |this|. Check |self| first. "Executing callbacks may free |this|. Check |self| first." https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device.cc:130: std::vector<uint8_t> payload = message->GetMessagePayload(); GetMessagePayload() is expensive so we shouldn't be calling it twice. https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device_unittest.cc (right): https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:116: class TestFailureCallback { There's a simpler way to do this: void ResponseCallback(scoped_refptr<U2fApduResponse>* output, bool success, scoped_refptr<U2fApduResponse> response) { *output = response; } Then in the test: scoped_refptr<U2fApduResponse> response1; device->DeviceTransact(U2fApduCommand::CreateVersion(), base::Bind(&ResponseCallback, &response1)); https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:199: HidService* hid_service = DeviceClient::Get()->GetHidService(); Since this logic is handling failure cases can you do this with a mock HidConnection?
Patchset #8 (id:210001) has been deleted
https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device.cc:69: // Executing |callback| may have freed |this|. Check |self| first. On 2017/03/09 00:37:03, Reilly Grant wrote: > "Executing callbacks may free |this|. Check |self| first." Done. https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device.cc:130: std::vector<uint8_t> payload = message->GetMessagePayload(); On 2017/03/09 00:37:03, Reilly Grant wrote: > GetMessagePayload() is expensive so we shouldn't be calling it twice. Acknowledged. https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device_unittest.cc (right): https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:116: class TestFailureCallback { On 2017/03/09 00:37:03, Reilly Grant wrote: > There's a simpler way to do this: > > void ResponseCallback(scoped_refptr<U2fApduResponse>* output, > bool success, > scoped_refptr<U2fApduResponse> response) { > *output = response; > } > > Then in the test: > > scoped_refptr<U2fApduResponse> response1; > device->DeviceTransact(U2fApduCommand::CreateVersion(), > base::Bind(&ResponseCallback, &response1)); Done. https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:199: HidService* hid_service = DeviceClient::Get()->GetHidService(); On 2017/03/09 00:37:03, Reilly Grant wrote: > Since this logic is handling failure cases can you do this with a mock > HidConnection? Switched to using mock device client and device connection.
Patchset #9 (id:250001) has been deleted
Patchset #8 (id:230001) has been deleted
Patchset #8 (id:270001) has been deleted
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device_unittest.cc (right): https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:285: std::unique_ptr<MockDeviceClient> client(new MockDeviceClient()); auto client = base::MakeUnique<MockDeviceClient>(); https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:299: EXPECT_EQ(static_cast<size_t>(1), u2f_devices.size()); ASSERT_EQ https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:300: if (!u2f_devices.empty()) { This condition isn't necessary since you have set up the HID service to always return a device. https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:313: device->state_ = U2fHidDevice::State::DEVICE_ERROR; The reason I suggested you use a MockHidConnection is that you can cause the U2fDevice object to enter the error state by having the MockHidConnection respond with an error instead of putting it into this state manually.
Patchset #10 (id:330001) has been deleted
https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... File device/u2f/u2f_hid_device_unittest.cc (right): https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:285: std::unique_ptr<MockDeviceClient> client(new MockDeviceClient()); On 2017/03/14 01:07:41, Reilly Grant wrote: > auto client = base::MakeUnique<MockDeviceClient>(); Done. https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:299: EXPECT_EQ(static_cast<size_t>(1), u2f_devices.size()); On 2017/03/14 01:07:41, Reilly Grant wrote: > ASSERT_EQ Done. https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:300: if (!u2f_devices.empty()) { On 2017/03/14 01:07:41, Reilly Grant wrote: > This condition isn't necessary since you have set up the HID service to always > return a device. Done. https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_dev... device/u2f/u2f_hid_device_unittest.cc:313: device->state_ = U2fHidDevice::State::DEVICE_ERROR; On 2017/03/14 01:07:41, Reilly Grant wrote: > The reason I suggested you use a MockHidConnection is that you can cause the > U2fDevice object to enter the error state by having the MockHidConnection > respond with an error instead of putting it into this state manually. Ok, switched to using a MockHidConnection that fails on every write attempt.
lgtm
The CQ bit was checked by piperc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2721223002/#ps350001 (title: "Use MockHidConnection to fail writes in unittest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by piperc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 350001, "attempt_start_ts": 1490041246227810,
"parent_rev": "43e217685e5c263404a0b71eadd5f3a30ff59ccf", "commit_rev":
"3a4044706b783686a071046c96ba3a09ca781837"}
Message was sent while issue was closed.
Description was changed from ========== Add support for U2fHidDevice interaction Add U2fHidDevice class. The class has an internal state machine to abstract to lazily connect and initialize the device upon instruction to transact with the device. BUG=697282 ========== to ========== Add support for U2fHidDevice interaction Add U2fHidDevice class. The class has an internal state machine to abstract to lazily connect and initialize the device upon instruction to transact with the device. BUG=697282 Review-Url: https://codereview.chromium.org/2721223002 Cr-Commit-Position: refs/heads/master@{#458196} Committed: https://chromium.googlesource.com/chromium/src/+/3a4044706b783686a071046c96ba... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:350001) as https://chromium.googlesource.com/chromium/src/+/3a4044706b783686a071046c96ba... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
