1333084 - Add WebAuthn to test_interfaces

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

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

Comment 1

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

Attachment: Bug 1333084 - Add WebAuthn to test_interfaces

Should have happened in Bug 1309284 but was missed since test_interfaces'
scheme wasn't https.

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

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

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

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

Yup-yup, that's what I/we want. On it now. Thanks bz!

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

I meant:

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

of course.  ;)

Comment 8

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

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

in Navigator.webidl?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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 10

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

Comment on attachment 8829472 [details]
Bug 1333084 - Add WebAuthn to test_interfaces

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

Comment 11

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

Comment on attachment 8829472 [details]
Bug 1333084 - Add WebAuthn to test_interfaces

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

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

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

Thanks qdot, bz! Try run looks good. Marking checkin-needed.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff6b0d4a62ec
Add WebAuthn to test_interfaces r=bz

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff6b0d4a62ec

Comment 17

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

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.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8829472 [details]
Bug 1333084 - Add WebAuthn to test_interfaces

Fixing the tests on aurora sounds good to me!

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d570c752fc9