1330138 - Divide U2F and WebAuthn APIs into their own directories

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

While they achieve the same goal, the U2F and WebAuthn APIs don't really share much code at the moment. We should have them in different directories so that it is obvious they are separate APIs.

Comment 1

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

Attachment: Bug 1330138 - Divide U2F and WebAuthn into separate directories;

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

Comment 2

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

Attachment: Bug 1330138 - Differentiate prefs for u2f and webauthn apis

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

Comment 3

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

Attachment: Bug 1330138 - Remove unused USBToken class from U2F API;

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

Comment 4

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

Comment on attachment 8825970 [details]
Bug 1330138 - Divide U2F and WebAuthn into separate directories;

https://reviewboard.mozilla.org/r/104018/#review104760

A couple of nits, but I confirm this rename is good and didn't touch anything substantive.

::: dom/webauthn/WebAuthentication.cpp:22
(Diff revision 1)
> +#define PREF_U2F_USBTOKEN_ENABLED  "security.webauth.u2f_enable_usbtoken"
> +
> +namespace mozilla {
> +namespace dom {
> +
> +static mozilla::LazyLogModule gWebauthLog("awebauthn");

Nit: Out of curiosity, why rename the log module to `awebauthn` from `webauth_u2f`? (It's the `a` prefix that gets me)

::: dom/webauthn/tests/mochitest.ini:2
(Diff revision 1)
> +[DEFAULT]
> +# Feature does not function on e10s (Disabled in Bug 1297552)

nit: technically untrue in this file. but we can remove this incorrect comment later.

Comment 5

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

Comment on attachment 8825972 [details]
Bug 1330138 - Remove unused USBToken class from U2F API;

https://reviewboard.mozilla.org/r/104022/#review104762

Comment 6

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

Comment on attachment 8825971 [details]
Bug 1330138 - Differentiate prefs for u2f and webauthn apis

https://reviewboard.mozilla.org/r/104020/#review104764

Seems good to me.

Comment 7

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

Comment on attachment 8825970 [details]
Bug 1330138 - Divide U2F and WebAuthn into separate directories;

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

Comment 8

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

Comment on attachment 8825971 [details]
Bug 1330138 - Differentiate prefs for u2f and webauthn apis

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

Comment 9

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

Comment on attachment 8825972 [details]
Bug 1330138 - Remove unused USBToken class from U2F API;

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

Comment 10

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

(In reply to J.C. Jones [:jcj] from comment #4)
> ::: dom/webauthn/WebAuthentication.cpp:22
> (Diff revision 1)
> > +#define PREF_U2F_USBTOKEN_ENABLED  "security.webauth.u2f_enable_usbtoken"
> > +
> > +namespace mozilla {
> > +namespace dom {
> > +
> > +static mozilla::LazyLogModule gWebauthLog("awebauthn");
> 
> Nit: Out of curiosity, why rename the log module to `awebauthn` from
> `webauth_u2f`? (It's the `a` prefix that gets me)

That was a typo. Thanks for catching that. :)

> ::: dom/webauthn/tests/mochitest.ini:2
> (Diff revision 1)
> > +[DEFAULT]
> > +# Feature does not function on e10s (Disabled in Bug 1297552)
> 
> nit: technically untrue in this file. but we can remove this incorrect
> comment later.

I took it out now. Also, looking at the U2F stuff, it'll probably be able to use the same IPC protocol as WebAuthn with minimal changes, so it may start working in non-e10s also.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8825971 [details]
Bug 1330138 - Differentiate prefs for u2f and webauthn apis

https://reviewboard.mozilla.org/r/104020/#review104900

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3cdeb82990
Divide U2F and WebAuthn into separate directories; r=jcj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c582598b2065
Differentiate prefs for u2f and webauthn apis; r=jcj r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d2fc52fdee
Remove unused USBToken class from U2F API; r=jcj

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9f3cdeb82990
https://hg.mozilla.org/mozilla-central/rev/c582598b2065
https://hg.mozilla.org/mozilla-central/rev/a8d2fc52fdee