1407829 - Web Authentication - Implement CredMan's Store method

Status: RESOLVED FIXED

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

Description

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

Credential Management defines a Store operation, which isn't quite in WebAuthn's WD-07 yet but will be shortly.

We need to implement that -- it just returns an error immediately.

Comment 1

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

Attachment: Bug 1407829 - WebAuthn: Implement CredMan's Store method

Credential Management defines a Store operation [1], which needs to be
implemented for WebAuthn's spec compliance. It only returns a NotSupportedError
for WebAuthn [2], so it's pretty simple.

[1] https://w3c.github.io/webappsec-credential-management/#dom-credentialscontainer-store
[2] https://w3c.github.io/webauthn/#storeCredential

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

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8918062 [details]
Bug 1407829 - WebAuthn: Implement CredMan's Store method

https://reviewboard.mozilla.org/r/188958/#review194416

::: dom/webauthn/WebAuthnManager.cpp:694
(Diff revision 1)
>  
> +already_AddRefed<Promise>
> +WebAuthnManager::Store(nsPIDOMWindowInner* aParent,
> +                       const Credential& aCredential)
> +{
> +  MOZ_ASSERT(aParent);

nit: `MOZ_ASSERT(NS_IsMainThread())`

::: dom/webauthn/WebAuthnManager.cpp:702
(Diff revision 1)
> +
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aParent);
> +
> +  ErrorResult rv;
> +  RefPtr<Promise> promise = Promise::Create(global, rv);
> +  if(rv.Failed()) {

nit: add a space after if

::: dom/webauthn/tests/test_webauthn_store_credential.html:37
(Diff revision 1)
> +      isnot(navigator.credentials, undefined, "Credential Management API endpoint must exist");
> +      isnot(navigator.credentials.create, undefined, "CredentialManagement create API endpoint must exist");
> +      isnot(navigator.credentials.get, undefined, "CredentialManagement get API endpoint must exist");
> +      isnot(navigator.credentials.store, undefined, "CredentialManagement store API endpoint must exist");
> +
> +      let gCredentialChallenge = new Uint8Array(16);

nit: The variable name makes it look like that's a global.

Comment 3

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

Comment on attachment 8918062 [details]
Bug 1407829 - WebAuthn: Implement CredMan's Store method

https://reviewboard.mozilla.org/r/188958/#review194416

Thanks, Tim. Updated.

Comment 4

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

Comment on attachment 8918062 [details]
Bug 1407829 - WebAuthn: Implement CredMan's Store method

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

Comment 5

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

Comment on attachment 8918062 [details]
Bug 1407829 - WebAuthn: Implement CredMan's Store method

https://reviewboard.mozilla.org/r/188958/#review195140

::: dom/webauthn/WebAuthnManager.h:94
(Diff revision 2)
>    GetAssertion(nsPIDOMWindowInner* aParent,
>                 const PublicKeyCredentialRequestOptions& aOptions);
>  
> +  already_AddRefed<Promise>
> +  Store(nsPIDOMWindowInner* aParent,
> +               const Credential& aCredential);

nit: identation

Comment 6

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

Comment on attachment 8918062 [details]
Bug 1407829 - WebAuthn: Implement CredMan's Store method

https://reviewboard.mozilla.org/r/188958/#review195140

Thanks for the reviews!

Comment 7

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

Comment on attachment 8918062 [details]
Bug 1407829 - WebAuthn: Implement CredMan's Store method

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ebe3f571ab8
WebAuthn: Implement CredMan's Store method r=qdot,ttaubert

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8ebe3f571ab8

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d99dbb659ecc
Fix merge bustage in WebAuthnManager.h on a CLOSED TREE r=me

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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