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.

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!

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.

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.

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.

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.

Comment on attachment 8870609 [details] bug 1332681 - part 4/4 - convert authentication.getAssertion to credentials.get https://reviewboard.mozilla.org/r/142048/#review150008

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].

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.

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

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.

Comment on attachment 8870609 [details] bug 1332681 - part 4/4 - convert authentication.getAssertion to credentials.get https://reviewboard.mozilla.org/r/142048/#review150338

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

Comment on attachment 8870608 [details] bug 1332681 - part 3/4 - convert authentication.makeCredential to credentials.create https://reviewboard.mozilla.org/r/142046/#review151430

Thanks for the reviews! Here's try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be70f5cb6c77

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

https://hg.mozilla.org/mozilla-central/rev/675f39600382 https://hg.mozilla.org/mozilla-central/rev/5d05100ea936 https://hg.mozilla.org/mozilla-central/rev/3c0d57fec9e5 https://hg.mozilla.org/mozilla-central/rev/3c876e859603