Default to ES256 and RS256 if pubKeyCredParams is empty #1387
Conversation
|
@equalsJeffH, please take a look. Thanks! |
There was a problem hiding this comment.
Overall LGTM, thx @nsatragno :)
I suggest changing the handling of the if statement to be a 'switch', see nsatragno#2, and wonder if we still need step 9 at all?
index.bs
Outdated
| 1. If |credTypesAndPubKeyAlgs| [=list/is empty=] and <code>|options|.{{PublicKeyCredentialCreationOptions/pubKeyCredParams}}</code> | ||
| [=list/is not empty=], return a {{DOMException}} whose name is "{{NotSupportedError}}", and terminate this algorithm. | ||
| 1. If |credTypesAndPubKeyAlgs| [=list/is empty=], return a {{DOMException}} whose name is | ||
| "{{NotSupportedError}}", and terminate this algorithm. |
There was a problem hiding this comment.
Is this step even needed any longer? ISTM that credTypesAndPubKeyAlgs will now never be empty ...?
There was a problem hiding this comment.
It's necessary. It can still be empty if pubKeyCredParams is not empty and none of its algorithms are supported.
There was a problem hiding this comment.
What I'm thinking now is that perhaps it's better to move it to the "is non-zero" branch of the switch.
There was a problem hiding this comment.
It's necessary. It can still be empty if
pubKeyCredParamsis not empty and none of its algorithms are supported.
Right. doh.
There was a problem hiding this comment.
fyi/fwiw, I did not obviously see support for [=list/is not empty=] in infra.spec.whatwg.org (but should have looked at index.bs more closely because that construct is in there (doh)), hence suggesting leveraging the list's size for the switch. Either works so this is fine but would not object to using is (not) empty instead. no big deal either way.
|
@akshayku updated the PR to include a list of algorithms client platforms can pick (ECC and RSA), please take another look. |
index.bs
Outdated
| 1. If <code>|options|.{{PublicKeyCredentialCreationOptions/pubKeyCredParams}}</code>'s [=list/size=] | ||
| <dl class="switch"> | ||
| : is zero | ||
| :: Let |defaultPublicKeyAlgorithm| be a {{COSEAlgorithmIdentifier}} with one of the following values at | ||
| the discretion of the [=client platform=]: | ||
| * <code>-7</code> ("ES256"). | ||
| * <code>-257</code> ("RS256"). | ||
|
|
||
| [=list/Append=] the pair of {{PublicKeyCredentialType}} with value {{public-key}} and a | ||
| {{COSEAlgorithmIdentifier}} with value |defaultPublicKeyAlgorithm| to |credTypesAndPubKeyAlgs|. | ||
|
|
||
| : is non-zero | ||
| :: [=list/For each=] |current| of <code>|options|.{{PublicKeyCredentialCreationOptions/pubKeyCredParams}}</code>: | ||
|
|
||
| 1. If <code>|current|.{{PublicKeyCredentialParameters/type}}</code> does not contain a {{PublicKeyCredentialType}} supported | ||
| by this implementation, then [=continue=]. | ||
| 1. Let |alg| be <code>|current|.{{PublicKeyCredentialParameters/alg}}</code>. | ||
| 1. [=list/Append=] the pair of <code>|current|.{{PublicKeyCredentialParameters/type}}</code> and |alg| to | ||
| |credTypesAndPubKeyAlgs|. | ||
|
|
||
| If |credTypesAndPubKeyAlgs| [=list/is empty=], return a {{DOMException}} whose name is | ||
| "{{NotSupportedError}}", and terminate this algorithm. | ||
| </dl> |
There was a problem hiding this comment.
Not asking that client chooses one of the algorithm at its descretion. Would like the list be of two elements.
We just need to add one more step in existing flow. If <code>|options|.{{PublicKeyCredentialCreationOptions/pubKeyCredParams}}</code> is empty, append <code>-7</code> ("ES256") AND <code>-257</code> ("RS256") to the list.
There was a problem hiding this comment.
Oh I get you now. Updated the PR to append both -7 and -257 to the list.
Default to algorithm -7 ("ES256") when options.pubKeyCredParams is
empty.
Fixes issue 1383.
nsatragno
changed the title
Default to algorithm -7 ("ES256") when
options.pubKeyCredParamsisempty.
Fixes #1383.
Preview | Diff