1333592 - U2F regression with https://u2fdemo.appspot.com/

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[J.C. Jones [:jcj] (he/they)]

How to recreate:

* Open Firefox 52
* Enable U2F and its soft token in about:config
* Navigate to https://u2fdemo.appspot.com/
* Check the box "Allow Re-registration"
* Click "Register U2F Authenticator" twice
* Click "Test Authentication"

What happens: Nothing
What should happen: One of the boxes should light up green, saying it responded correctly.

This is a regression since Firefox 51. Multiple keys aren't tested in the current mochitests, so this should add tests and fix the regression.

Comment 1

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1333592 - Fix a regression with U2F sign() called with multiple keys

Add a test that U2F supports signing with multiple keys.

Reorder the WaitGroupDone calls to ensure they always fire, even if there
are multiple actions in flight.

Also, change the status lanbda captures to capture by reference, and disable
the copy constructor that would let the by-value work. Interestingly, the
compiler is choosing by-reference by default -- at least logs show that the
behavior is correct without this change, but still - this is the right thing to
do.

Review commit: https://reviewboard.mozilla.org/r/107004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107004/

Comment 2

[J.C. Jones [:jcj] (he/they)]

The tests here are failing as-if SpecialPowers.pushPrefEnv isn't working correctly to toggle the preference, or the preference isn't working properly to toggle visibility. I'm still investigating what's wrong .... I don't think there's an issue with the test, though it doesn't use an iframe like the other U2F tests. (WebAuthn tests also do not use an iframe)

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8830093 [details]
Bug 1333592 - Fix a regression with U2F sign() called with multiple keys

https://reviewboard.mozilla.org/r/107004/#review108454

LGTM.

Comment 4

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8830093 [details]
Bug 1333592 - Fix a regression with U2F sign() called with multiple keys

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107004/diff/1-2/

Comment 5

[J.C. Jones [:jcj] (he/they)]

Try run looks good now with the test-fix. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3f994cb5ac5

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/456d27bbbe74
Fix a regression with U2F sign() called with multiple keys r=keeler

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/456d27bbbe74

Comment 8

[Julien Cristau [:jcristau]]

Please request aurora/beta approval on this when you get a chance.

Comment 9

[J.C. Jones [:jcj] (he/they)]

Looking into it, I'd either need to rework the test or we'd need to uplift Bug 1286312 along with this into Beta and Aurora. I don't know that this regression is particularly worthy of that effort, so I'm going to mark 52 and 53 as wontfix instead. The only user impact, since U2F support is experimental and hidden behind a pref, is those experimenting need to move back to Nightly. That seems OK here.

Thanks Julien.


Approval Request Comment
[Feature/Bug causing the regression]: Caused in Bug 1297552
[User impact if declined]: U2F support is experimental and hidden behind a pref, but for those experimenting they'll need to move back to Nightly.
[Is this code covered by automated tests?]: A test was added to exercise the regression.
[Has the fix been verified in Nightly?]: Yes, fixed today.
[Needs manual test from QE? If yes, steps to reproduce]: No, and it's still experimental.
[List of other uplifts needed for the feature/fix]: Bug 1286312 adding https testing to mochitest, or I need to modify the tests to be more uplift-able.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It only affects the experimental U2F support which is still hidden behind a pref.
[String changes made/needed]: None.

Comment 10

[J.C. Jones [:jcj] (he/they)]

Oops, I meant to delete the "Approval Request Comment"; please disregard (or consider it a starting point if we for some reason change our mind)

Comment 11

[Andrei Vaida [:avaida]]

(In reply to J.C. Jones [:jcj] from comment #9)
> [Is this code covered by automated tests?]: A test was added to exercise the
> regression.
> [Needs manual test from QE? If yes, steps to reproduce]: No, and it's still
> experimental.

Setting qe-verify- based on J.C. Jones' assessment on manual testing needs.