Closed Bug 1333592 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- unaffected
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

()

Details

Attachments

(1 file)

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.
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 on attachment 8830093 [details]
Bug 1333592 - Fix a regression with U2F sign() called with multiple keys

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

LGTM.
Attachment #8830093 - Flags: review?(dkeeler) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/456d27bbbe74
Fix a regression with U2F sign() called with multiple keys r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/456d27bbbe74
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request aurora/beta approval on this when you get a chance.
Flags: needinfo?(jjones)
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.
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)
Depends on: 1334388
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.