Closed Bug 1379580 Opened 2 years ago Closed 2 years ago

U2FTokenTransport promises should resolve to U2F data buffers

Categories

(Core :: DOM: Device Interfaces, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Whiteboard: [webauthn])

Attachments

(1 file)

Currently, we call U2FTokenTransport::Register() and Sign() and pass stack references that we expect to be filled when the promise resolves. The then() callback itself copies the nsTArrays, so this probably works by accident?

Well, this is bad. We should have proper Register/Sign promises that return a result specific to the request type. That result then holds the u8 buffers returned by the token.
Summary: U2FTokenTransport promises should return U2F data → U2FTokenTransport promises should resolve to U2F data buffers
Comment on attachment 8884769 [details] [diff] [review]
0001-Bug-1379580-U2FTokenTransport-promises-should-resolv.patch

Review of attachment 8884769 [details] [diff] [review]:
-----------------------------------------------------------------

r+, though I do have one question.

::: dom/webauthn/U2FTokenManager.cpp
@@ +322,5 @@
>  }
>  
>  void
>  U2FTokenManager::MaybeConfirmSign(uint64_t aTransactionId,
> +                                  U2FSignResult&& aResult)

Just curious, why the move semantics here and in register? I think the call to SendConfirmSign/Register is just going to copy these elements into a message block for IPC, so afaik there's nothing after this that really needs the ownership? Doesn't really hurt anything and I suppose if you're following the Promise API, it's fine. Just want to make sure I'm not missing something.
Attachment #8884769 - Flags: review?(kyle) → review+
(In reply to Kyle Machulis [:qdot] [:kmachulis] from comment #2)
> >  void
> >  U2FTokenManager::MaybeConfirmSign(uint64_t aTransactionId,
> > +                                  U2FSignResult&& aResult)
> 
> Just curious, why the move semantics here and in register? I think the call
> to SendConfirmSign/Register is just going to copy these elements into a
> message block for IPC, so afaik there's nothing after this that really needs
> the ownership? Doesn't really hurt anything and I suppose if you're
> following the Promise API, it's fine. Just want to make sure I'm not missing
> something.

You're right, I went a little overboard with the move semantics here. We actually just need to pass by reference from the promise->Then() callback.

Thanks!
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/695e774df6c6
U2FTokenTransport promises should resolve to U2F data buffers r=qDot
https://hg.mozilla.org/mozilla-central/rev/695e774df6c6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.