Closed Bug 1332681 Opened 7 years ago Closed 7 years ago

Update WebAuthn JS API to the WD-05 working draft

Categories

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

Product:
Core
Component:
DOM: Device Interfaces
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jcj, Assigned: keeler)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [webauthn])

Attachments

(4 files)

bug 1332681 - part 1/4 - authentication.makeCredential: return a PublicKeyCredential instead of a ScopedCredentialInfo
59 bytes, text/x-review-board-request
jcj
: review+
qdot
: review+
Details
bug 1332681 - part 2/4 - authentication.getAssertion: return a PublicKeyCredential instead of a WebAuthnAssertion
59 bytes, text/x-review-board-request
jcj
: review+
qdot
: review+
Details
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create
59 bytes, text/x-review-board-request
jcj
: review+
qdot
: review+
Details
bug 1332681 - part 4/4 - convert authentication.getAssertion to credentials.get
59 bytes, text/x-review-board-request
jcj
: review+
qdot
: review+
Details 
      No description provided.
WD-05 is published at https://www.w3.org/TR/2017/WD-webauthn-20170505/. Once Bug 1323339 lands, this is unblocked. Edge and Chrome are also planning to implement WD-05, so it should be a good interop point.
URL: https://www.w3.org/TR/2017/WD-webauth...
Depends on: 1323339
Summary: Update WebAuthn JS API to the WD-04 working draft → Update WebAuthn JS API to the WD-05 working draft
Whiteboard: [webauthn]
Assignee: jjones → dkeeler
J.C. - here's a first cut at this (I basically just changed the names and moved things around as necessary - I didn't yet check to see if there were new requirements from the specification that have to be added.) If you could have a look to see if this is a good approach, that would be great. Thanks!
Flags: needinfo?(jjones)
Comment on attachment 8870606 [details]
bug 1332681 - part 1/4 - authentication.makeCredential: return a PublicKeyCredential instead of a ScopedCredentialInfo

https://reviewboard.mozilla.org/r/142042/#review145720

Re: part 1/4 - This looks like what I was expecting. Also, super-kudos for having the forethought to break this up into these chunks - brilliant.

The main thing I see here is the memory management on the refcounted objects, and sadly I, too, am science dog when it comes to that. That said, getting those right is something we'll want to do in two steps: 

1) Ensure the leaktests are 0 on a mochitest run
2) Ask qdot to review them
Comment on attachment 8870608 [details]
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create

https://reviewboard.mozilla.org/r/142046/#review146632

::: dom/webauthn/WebAuthnManager.cpp:408
(Diff revision 1)
>  
>    nsTArray<WebAuthnScopedCredentialDescriptor> excludeList;
>    if (aOptions.mExcludeList.WasPassed()) {
>      for (const auto& s: aOptions.mExcludeList.Value()) {
>        WebAuthnScopedCredentialDescriptor c;
> -      c.type() = static_cast<uint32_t>(s.mType);
> +      c.type() = static_cast<uint32_t>(s.mType); // XXX hmmm

Yeaah, kind of sketchy.
The approach looks good, Mr. Keeler. The most obvious criticism is that whoever comes in to do CredentialManagement's other functions is going to have to write something that gets called by CredentialsContainer::Get and CredentialsContainer::Create, switches on the type, and then calls into WebAuthnManager, but that's alright. I think the necessary extensiblity is there as you have it.
Flags: needinfo?(jjones)
Thanks!

(In reply to J.C. Jones [:jcj] from comment #8)
> Comment on attachment 8870608 [details]
> bug 1332681 - part 3/4 - convert authentication.makeCredential to
> credentials.create
> 
> https://reviewboard.mozilla.org/r/142046/#review146632
> 
> ::: dom/webauthn/WebAuthnManager.cpp:408
> (Diff revision 1)
> >  
> >    nsTArray<WebAuthnScopedCredentialDescriptor> excludeList;
> >    if (aOptions.mExcludeList.WasPassed()) {
> >      for (const auto& s: aOptions.mExcludeList.Value()) {
> >        WebAuthnScopedCredentialDescriptor c;
> > -      c.type() = static_cast<uint32_t>(s.mType);
> > +      c.type() = static_cast<uint32_t>(s.mType); // XXX hmmm
> 
> Yeaah, kind of sketchy.

So it turns out we're not actually using either the type or the transports list yet, so I figured it might be best to remove them for now and properly implement conversion/serialization when we actually use them.
Just a heads up, I /am/ actually working on this review, it's just slow going due to a couple of other things in my queue.
Comment on attachment 8870606 [details]
bug 1332681 - part 1/4 - authentication.makeCredential: return a PublicKeyCredential instead of a ScopedCredentialInfo

https://reviewboard.mozilla.org/r/142042/#review147220

This looks good to me. I'm in-progress on the other reviews, I'm sorry that my being out this week sick is slowing this down! I'll do my best to catch up ASAP.

::: dom/webauthn/AuthenticatorAttestationResponse.cpp:28
(Diff revision 2)
> +AuthenticatorAttestationResponse::~AuthenticatorAttestationResponse()
> +{}
> +
> +JSObject*
> +AuthenticatorAttestationResponse::WrapObject(JSContext* aCx,
> +                                JS::Handle<JSObject*> aGivenProto)

nit: Indent

::: dom/webauthn/PublicKeyCredential.cpp:21
(Diff revision 2)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(PublicKeyCredential,
> +                                                  Credential)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(PublicKeyCredential, Credential)
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(PublicKeyCredential, Credential)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(PublicKeyCredential)
> +NS_INTERFACE_MAP_END_INHERITING(Credential)

qdot - please check this stuff, because it's black magic to me.

::: dom/webauthn/PublicKeyCredential.cpp:53
(Diff revision 2)
>  
> -already_AddRefed<ScopedCredential>
> -ScopedCredentialInfo::Credential() const
> +void
> +PublicKeyCredential::GetRawId(JSContext* aCx,
> +                              JS::MutableHandle<JSObject*> aRetVal) const
>  {
> -  RefPtr<ScopedCredential> temp(mCredential);
> +  aRetVal.set(mRawId.ToUint8Array(aCx));

I realize I did this, too - but I don't know what happens when a JSObject ptr is null - as `ToUint8Array` certainly can return `nullptr` if it's out of memory.

::: dom/webauthn/AuthenticatorResponse.cpp:31
(Diff revision 2)
> -WebAuthnAttestation::~WebAuthnAttestation()
> +AuthenticatorResponse::~AuthenticatorResponse()
>  {}
>  
>  JSObject*
> -WebAuthnAttestation::WrapObject(JSContext* aCx,
> +AuthenticatorResponse::WrapObject(JSContext* aCx,
>                                  JS::Handle<JSObject*> aGivenProto)

Nit: Indent is off?

::: dom/webauthn/tests/test_webauthn_loopback.html:125
(Diff revision 2)
>      let acct = {rpDisplayName: "none", displayName: "none", id: "none"};
>      let param = {type: "ScopedCred", algorithm: "p-256"};
>      let options = {rpId: document.origin,
> -                   excludeList: [aCredInfo.credential]};
> +                   excludeList: [{ type: "ScopedCred",
> +                                   id: Uint8Array.from(aCredInfo.rawId),
> +                                   transports: [ "usb"] }]};

spacing here in `[ "usb"]`

::: dom/webidl/WebAuthentication.webidl:16
(Diff revision 2)
>  
>  [SecureContext, Pref="security.webauth.webauthn"]
> -interface ScopedCredentialInfo {
> -    readonly attribute ScopedCredential    credential;
> -    readonly attribute WebAuthnAttestation attestation;
> +interface PublicKeyCredential : Credential {
> +    readonly attribute ArrayBuffer           rawId;
> +    readonly attribute AuthenticatorResponse response;
> +    // Extensions are not supported yet.

Let's file a follow-on for the extension support. We at least don't want to cause JS errors because the field isn't there.
Attachment #8870606 - Flags: review?(jjones) → review+
Comment on attachment 8870607 [details]
bug 1332681 - part 2/4 - authentication.getAssertion: return a PublicKeyCredential instead of a WebAuthnAssertion

https://reviewboard.mozilla.org/r/142044/#review149762

Note to qdot: This patch is, while self-contained, not an easily-identified substate of the spec. It's probably not worth trying to tie it to any specific spec changes. I'd recommend just looking at the code alone.
Attachment #8870607 - Flags: review?(jjones) → review+
Comment on attachment 8870608 [details]
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create

https://reviewboard.mozilla.org/r/142046/#review150002

::: dom/webauthn/WebAuthnManager.cpp:298
(Diff revision 2)
> -  nsTArray<ScopedCredentialParameters> normalizedParams;
> -  for (size_t a = 0; a < aCryptoParameters.Length(); ++a) {
> +  nsTArray<PublicKeyCredentialParameters> normalizedParams;
> +  for (size_t a = 0; a < aOptions.mParameters.Length(); ++a) {
>      // Let current be the currently selected element of
>      // cryptoParameters.
>  
>      // If current.type does not contain a ScopedCredentialType

Nit: comment's type is out of date

::: dom/webidl/CredentialManagement.webidl:13
(Diff revision 2)
> +interface CredentialsContainer {
> +  Promise<Credential?> create(optional CredentialCreationOptions options);
> +};
> +
> +dictionary CredentialCreationOptions {
> +  MakeCredentialOptions publicKey; // nullable dictionary not allowed in our implementation? :(

That's odd. We should probably file a bug to support nullable dictionaries, in that case.
Attachment #8870608 - Flags: review?(jjones) → review+
Comment on attachment 8870609 [details]
bug 1332681 - part 4/4 - convert authentication.getAssertion to credentials.get

https://reviewboard.mozilla.org/r/142048/#review150008
Attachment #8870609 - Flags: review?(jjones) → review+
Comment on attachment 8870606 [details]
bug 1332681 - part 1/4 - authentication.makeCredential: return a PublicKeyCredential instead of a ScopedCredentialInfo

https://reviewboard.mozilla.org/r/142042/#review148480

This is looking pretty good, but I'd like to see the webidl updates and make sure we've got our cycle collection right before I r+ it.

::: dom/webauthn/AuthenticatorAttestationResponse.cpp:43
(Diff revision 2)
> +}
> +
> +nsresult
> +AuthenticatorAttestationResponse::SetAttestationObject(CryptoBuffer& aBuffer)
> +{
> +  if (!mAttestationObject.Assign(aBuffer)) {

nit: Add NS_WARN_IF to if statement

::: dom/webauthn/PublicKeyCredential.h:31
(Diff revision 2)
>  
> -public:
> +  explicit PublicKeyCredential(nsPIDOMWindowInner* aParent);
> -  explicit ScopedCredentialInfo(nsPIDOMWindowInner* aParent);
>  
>  protected:
> -  ~ScopedCredentialInfo();
> +  virtual ~PublicKeyCredential() override;

Final classes don't need virtual on destructors.

::: dom/webauthn/PublicKeyCredential.cpp:15
(Diff revision 2)
>  
>  namespace mozilla {
>  namespace dom {
>  
> -// Only needed for refcounted objects.
> -NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(ScopedCredentialInfo, mParent)
> +// I think this doesn't actually traverse mResponse?
> +// I'm basically the "I have no idea what I'm doing" science dog here.

Yeah, good catch. This should've traversed mAttestation in the last patch, and should traverse/unlink mResponse here. Since this now inherited from a wrapper cached class, this should be a NS_IMPL_CYCLE_COLLECTION_INHERITED, which you pass mResponse to also. Using that macro will take care of the traverse/unlink calls for you, so you can remove those from here.

::: dom/webauthn/PublicKeyCredential.cpp:66
(Diff revision 2)
>  }
>  
> -void
> -ScopedCredentialInfo::SetCredential(RefPtr<ScopedCredential> aCredential)
> +nsresult
> +PublicKeyCredential::SetRawId(CryptoBuffer& aBuffer)
>  {
> -  mCredential = aCredential;
> +  if (!mRawId.Assign(aBuffer)) {

NS_WARN_IF

::: dom/webauthn/AuthenticatorResponse.cpp:46
(Diff revision 2)
>  }
>  
>  nsresult
> -WebAuthnAttestation::SetAttestation(CryptoBuffer& aBuffer)
> +AuthenticatorResponse::SetClientDataJSON(CryptoBuffer& aBuffer)
>  {
> -  if (!mAttestation.Assign(aBuffer)) {
> +  if (!mClientDataJSON.Assign(aBuffer)) {

NS_WARN_IF

::: dom/webidl/CredentialManagement.webidl:1
(Diff revision 2)
> +[Exposed=Window, SecureContext, Pref="security.webauth.webauthn"]

This is missing the usual webidl editor/license headers, plus it needs the reference link to the spec it was taken from.

::: dom/webidl/CredentialManagement.webidl:3
(Diff revision 2)
> +[Exposed=Window, SecureContext, Pref="security.webauth.webauthn"]
> +interface Credential {
> +  readonly attribute USVString id;

According to the spec (https://w3c.github.io/webappsec-credential-management/), both of these properties should be marked [SameObject].
Attachment #8870606 - Flags: review?(kyle) → review-
Comment on attachment 8870607 [details]
bug 1332681 - part 2/4 - authentication.getAssertion: return a PublicKeyCredential instead of a WebAuthnAssertion

https://reviewboard.mozilla.org/r/142044/#review150320

::: dom/webauthn/AuthenticatorAssertionResponse.cpp:35
(Diff revision 2)
> -  aRetVal.set(mClientData.ToUint8Array(aCx));
> -}
> -
> -void
> -WebAuthnAssertion::GetAuthenticatorData(JSContext* aCx,
> -                                        JS::MutableHandle<JSObject*> aRetVal) const
> +                                                   JS::MutableHandle<JSObject*> aRetVal) const

nit: I think indentation might be off here? Mozreview is making it difficult to tell.
Attachment #8870607 - Flags: review?(kyle) → review+
Comment on attachment 8870608 [details]
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create

https://reviewboard.mozilla.org/r/142046/#review150328

::: dom/webidl/CredentialManagement.webidl:13
(Diff revision 2)
> +interface CredentialsContainer {
> +  Promise<Credential?> create(optional CredentialCreationOptions options);
> +};
> +
> +dictionary CredentialCreationOptions {
> +  MakeCredentialOptions publicKey; // nullable dictionary not allowed in our implementation? :(

Nullable dictionaries as dictionary members aren't allowed via the WebIDL spec.

https://www.w3.org/TR/WebIDL-1/#idl-nullable-type

I filed a spec bug for this.

https://github.com/w3c/webauthn/issues/490
Attachment #8870608 - Flags: review?(kyle) → review+
Comment on attachment 8870608 [details]
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create

https://reviewboard.mozilla.org/r/142046/#review150336

Whoops. r- actually, interface needs to be pref'd.

::: dom/webidl/CredentialManagement.webidl:7
(Diff revision 2)
>  interface Credential {
>    readonly attribute USVString id;
>    readonly attribute DOMString type;
>  };
> +
> +[Exposed=Window, SecureContext]

Pref this.
Attachment #8870608 - Flags: review+ → review-
Comment on attachment 8870609 [details]
bug 1332681 - part 4/4 - convert authentication.getAssertion to credentials.get

https://reviewboard.mozilla.org/r/142048/#review150338
Attachment #8870609 - Flags: review?(kyle) → review+
Comment on attachment 8870606 [details]
bug 1332681 - part 1/4 - authentication.makeCredential: return a PublicKeyCredential instead of a ScopedCredentialInfo

https://reviewboard.mozilla.org/r/142042/#review147220

Thanks!

> nit: Indent

Fixed.

> I realize I did this, too - but I don't know what happens when a JSObject ptr is null - as `ToUint8Array` certainly can return `nullptr` if it's out of memory.

I don't know the right answer. qdot?

> Nit: Indent is off?

Fixed.

> spacing here in `[ "usb"]`

Fixed - I also made other areas more consistent.

> Let's file a follow-on for the extension support. We at least don't want to cause JS errors because the field isn't there.

Filed bug 1370728.
Comment on attachment 8870606 [details]
bug 1332681 - part 1/4 - authentication.makeCredential: return a PublicKeyCredential instead of a ScopedCredentialInfo

https://reviewboard.mozilla.org/r/142042/#review148480

Thanks!

> nit: Add NS_WARN_IF to if statement

Fixed.

> Final classes don't need virtual on destructors.

Fixed.

> Yeah, good catch. This should've traversed mAttestation in the last patch, and should traverse/unlink mResponse here. Since this now inherited from a wrapper cached class, this should be a NS_IMPL_CYCLE_COLLECTION_INHERITED, which you pass mResponse to also. Using that macro will take care of the traverse/unlink calls for you, so you can remove those from here.

Ok - thanks. I'm still not sure I did this correctly, so definitely double-check the next version.

> NS_WARN_IF

Fixed.

> NS_WARN_IF

Fixed.

> This is missing the usual webidl editor/license headers, plus it needs the reference link to the spec it was taken from.

Added, I think (I basically copy/pasted from WebAuthentication.webidl and updated the link).

> According to the spec (https://w3c.github.io/webappsec-credential-management/), both of these properties should be marked [SameObject].

I'm getting the error "WebIDL.WebIDLError: error: An attribute with [SameObject] must have an interface type as its type" when I do that for those attributes. Spec bug or implementation bug?
Comment on attachment 8870607 [details]
bug 1332681 - part 2/4 - authentication.getAssertion: return a PublicKeyCredential instead of a WebAuthnAssertion

https://reviewboard.mozilla.org/r/142044/#review150320

> nit: I think indentation might be off here? Mozreview is making it difficult to tell.

Yep - good catch.
Comment on attachment 8870608 [details]
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create

https://reviewboard.mozilla.org/r/142046/#review150002

> Nit: comment's type is out of date

Fixed.
Comment on attachment 8870608 [details]
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create

https://reviewboard.mozilla.org/r/142046/#review150328

> Nullable dictionaries as dictionary members aren't allowed via the WebIDL spec.
> 
> https://www.w3.org/TR/WebIDL-1/#idl-nullable-type
> 
> I filed a spec bug for this.
> 
> https://github.com/w3c/webauthn/issues/490

Cool - thanks.
Comment on attachment 8870608 [details]
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create

https://reviewboard.mozilla.org/r/142046/#review150336

> Pref this.

Fixed.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #26)

> I'm getting the error "WebIDL.WebIDLError: error: An attribute with
> [SameObject] must have an interface type as its type" when I do that for
> those attributes. Spec bug or implementation bug?

Whoops, saw that on the CredentialsContainer but didn't notice it's on a bunch of USVString/DOMString types. That's a spec bug (see https://heycam.github.io/webidl/#SameObject). Filed at 

https://github.com/w3c/webappsec-credential-management/issues/88
Comment on attachment 8870606 [details]
bug 1332681 - part 1/4 - authentication.makeCredential: return a PublicKeyCredential instead of a ScopedCredentialInfo

https://reviewboard.mozilla.org/r/142042/#review151428
Attachment #8870606 - Flags: review?(kyle) → review+
Comment on attachment 8870608 [details]
bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create

https://reviewboard.mozilla.org/r/142046/#review151430
Attachment #8870608 - Flags: review?(kyle) → review+
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/675f39600382
part 1/4 - authentication.makeCredential: return a PublicKeyCredential instead of a ScopedCredentialInfo r=jcj,qdot
https://hg.mozilla.org/integration/autoland/rev/5d05100ea936
part 2/4 - authentication.getAssertion: return a PublicKeyCredential instead of a WebAuthnAssertion r=jcj,qdot
https://hg.mozilla.org/integration/autoland/rev/3c0d57fec9e5
part 3/4 - convert authentication.makeCredential to credentials.create r=jcj,qdot
https://hg.mozilla.org/integration/autoland/rev/3c876e859603
part 4/4 - convert authentication.getAssertion to credentials.get r=jcj,qdot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: