Closed
Bug 1333084
Opened 8 years ago
Closed 8 years ago
Add WebAuthn to test_interfaces
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: jcj, Assigned: jcj)
References
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
bzbarsky
:
review+
qdot
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
WebAuthn didn't get added to dom/tests/mochitest/general/test_interfaces.html because of the http/https scheme splits. Fix it.
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
Is the intent to ship these in 53? Do they actually do anything? The status of bug 1323339 makes me a bit unclear on the current situation.
Flags: needinfo?(jjones)
Assignee | ||
Comment 3•8 years ago
|
||
They are hidden behind a preference for 53. We're aiming for 54 for it to be working properly.
For some reason, marking {name: "WebAuthentication", disabled: true} doesn't work, though it's hidden by a pref in Navigator.webidl [1] the way the U2F is, but perhaps it's because the pref marking is a different hierarchy level [2]?
[1] http://searchfox.org/mozilla-central/source/dom/webidl/Navigator.webidl#385
[2] http://searchfox.org/mozilla-central/source/dom/webidl/U2F.webidl#71
Flags: needinfo?(jjones)
Comment 4•8 years ago
|
||
> They are hidden behind a preference for 53.
No, they're not. The IDL in WebAuthentication.webidl says:
[SecureContext]
interface WebAuthentication {
which is unconditional exposure of the interface object in all secure context windows. That is, you hid "navigator.authentication" behind a pref, but didn't hide "window.WebAuthentication". This last is what test_interfaces.html is supposed to catch, so the system kinda works. ;)
What you probably want is:
[SecureContext, Pref="security.webauth.w3c]
interface WebAuthentication {
and then backport that to the relevant branches, along with the test_interfaces changes if possible. And the same for all the other interfaces that should be conditioned on that pref.
In terms of what the test_interfaces annotations should say... Presumably they should in fact say {name: "WebAuthentication", disabled: true} and similar for the other webauthn interfaces for now, since that is in fact the expected behavior on all channels, right?
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8829472 [details]
Bug 1333084 - Add WebAuthn to test_interfaces
https://reviewboard.mozilla.org/r/106560/#review107550
r-. The way this is supposed to work is that test_interfaces expresses our shipping policy for the interface (which in this case is "disabled on all branches", per bug discussion), and then you fix your IDL to make the test pass, which ensures that our actual behavior matches the desired shipping policy.
Attachment #8829472 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 6•8 years ago
|
||
Yup-yup, that's what I/we want. On it now. Thanks bz!
Comment 7•8 years ago
|
||
> [SecureContext, Pref="security.webauth.w3c]
I meant:
[SecureContext, Pref="security.webauth.w3c"]
of course. ;)
Assignee | ||
Comment 8•8 years ago
|
||
Should I keep the:
[Pref="security.webauth.webauthn", SameObject]
in Navigator.webidl?
Comment 9•8 years ago
|
||
Yes, absolutely. Again, those are separate annotations: one is for "is this interface object exposed via a property on the global?" and one is for "is this attribute exposed on the Navigator object?". The two exposure decisions are, in general, independent of each other, so need to be annotated separately.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> > [SecureContext, Pref="security.webauth.w3c]
>
> I meant:
>
> [SecureContext, Pref="security.webauth.w3c"]
>
> of course. ;)
Note that this should be "security.webauth.webauthn" also, I changed that in bug 1330138 since we're splitting prefs for u2f and webauthn.
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8829472 [details]
Bug 1333084 - Add WebAuthn to test_interfaces
https://reviewboard.mozilla.org/r/106560/#review107616
r=me. Thank you for fixing this!
Don't forget to request the relevant backport approvals.
Attachment #8829472 -
Flags: review?(bzbarsky) → review+
Updated•8 years ago
|
Attachment #8829472 -
Flags: review?(kyle) → review+
Assignee | ||
Comment 14•8 years ago
|
||
Thanks qdot, bz! Try run looks good. Marking checkin-needed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efb15c49df41
Keywords: checkin-needed
Comment 15•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ff6b0d4a62ec
Add WebAuthn to test_interfaces r=bz
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8829472 [details]
Bug 1333084 - Add WebAuthn to test_interfaces
Seeking aurora uplift per comment 13
Approval Request Comment
[Feature/Bug causing the regression]: A combination of Bug 1309284 (Web Authentication) and Bug 1286312 (Support HTTPS for Mochitests).
[User impact if declined]: None, but mochitest's test_interfaces.html won't exercise [SecureContexts] on the trains without this patch.
[Is this code covered by automated tests?]: Yes, this fixes a test.
[Has the fix been verified in Nightly?]: Yes... It hasn't lit the tree aflame anyway!
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's only a metadata change to a unit test.
[String changes made/needed]: None.
Attachment #8829472 -
Flags: approval-mozilla-aurora?
status-firefox53:
--- → affected
Comment on attachment 8829472 [details]
Bug 1333084 - Add WebAuthn to test_interfaces
Fixing the tests on aurora sounds good to me!
Attachment #8829472 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•