Add WebAuthn to test_interfaces

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jcj, Assigned: jcj)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
WebAuthn didn't get added to dom/tests/mochitest/general/test_interfaces.html because of the http/https scheme splits. Fix it.
(Assignee)

Updated

a year ago
No longer blocks: 1268804
Comment hidden (mozreview-request)
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

a year 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)
> 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 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

a year ago
Yup-yup, that's what I/we want. On it now. Thanks bz!
>  [SecureContext, Pref="security.webauth.w3c]

I meant:

  [SecureContext, Pref="security.webauth.w3c"]

of course.  ;)
(Assignee)

Comment 8

a year ago
Should I keep the:
  [Pref="security.webauth.webauthn", SameObject]

in Navigator.webidl?
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)
(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 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+
(Assignee)

Comment 14

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff6b0d4a62ec
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Comment 17

a year 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

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/4d570c752fc9
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.