Add credential protection policy and large blob support #128
Conversation
Co-authored-by: j-hellenberg <janeric.hellenberg@gmail.com>
Co-authored-by: j-hellenberg <janeric.hellenberg@gmail.com>
|
|
| read: Optional[bool] = None | ||
| write: Optional[bytes] = None | ||
|
|
||
| class AuthenticationExtensionClientInputs(WebAuthnBaseModel): |
There was a problem hiding this comment.
Can you please rename this to AuthenticationExtensionsClientInputs (with "Extensions" plural) to better match the spec here? https://www.w3.org/TR/webauthn-2/#iface-authentication-extensions-client-inputs
| class CredentialProtectionPolicy(str, Enum): | ||
| """Various registered values indicating whether a credential shall be protected (influences how discoverable credentials are handled). | ||
|
|
||
| Members: | ||
| `USER_VERIFICATION_OPTIONAL` | ||
| `USER_VERIFICATION_OPTIONAL_WITH_CREDENTIAL_ID_LIST` | ||
| `USER_VERIFICATION_REQUIRED` | ||
|
|
||
| https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#sctn-credProtect-extension | ||
| """ | ||
| USER_VERIFICATION_OPTIONAL = 'userVerificationOptional' | ||
| USER_VERIFICATION_OPTIONAL_WITH_CREDENTIAL_ID_LIST = 'userVerificationOptionalWithCredentialIDList' | ||
| USER_VERIFICATION_REQUIRED = 'userVerificationRequired' | ||
|
|
There was a problem hiding this comment.
I understand the value of this extension, but my goal is for this library to support only the functionality defined in the WebAuthn spec itself. Since this extension isn't formally defined in https://www.w3.org/TR/webauthn-2/#sctn-defined-extensions then I'm inclined to request that it not be included in this diff.
There was a problem hiding this comment.
There's some conversation going on in an issue in the spec repo that's educating me on how my position in which extensions to support may not be reasonable: w3c/webauthn#1703 (comment)
I'm going to revisit this PR next week when I get back in the office, your credProtect contribution will likely make it through in one way or another.
| if large_blob_extension is not None: | ||
| options.extensions = AuthenticationExtensionClientInputs( | ||
| large_blob=large_blob_extension | ||
| ) |
There was a problem hiding this comment.
Can you please write some tests for this new logic?
| options.extensions = AuthenticationExtensionClientInputs( | ||
| large_blob=large_blob_extension, | ||
| credential_protection_policy=credential_protection_policy, | ||
| enforce_credential_protection_policy=enforce_credential_protection_policy, | ||
| ) |
There was a problem hiding this comment.
Can you write some tests for this new logic please to ensure the extensions are set when specified? And I'm curious, what would extensions out of this method serialize to JSON as if none of the arguments are provided? An empty {}? Would they be omitted? I'd like to see extensions not get serialized at all if no extensions are specified.
|
I neglected to thank you for submitting this PR: thank you! I've been meaning to sit down and think about extension support with a bit more abstract API that wouldn't require you to know all of the values for a given extension. For example, why not have an optional
So there's no need to even include the value if you're not going to set it to And for authentication you can specify either These are the angles from which I was going to approach adding support for extensions like |
|
Closing for now due to inactivity |
We needed support for large blobs and credential protection policy in our project, which py_webauthn didn't provide. We added the appropriate structs and options to genereate_registration_options which worked fine for us.
closes #127