Fix handling of optional fields such as user_handle. #174
Conversation
Instead of relying on input types to drive conversion - set up a field_validator that enumerates all spec-defined base64 fields and convert them. Remove the base64url decoding from the generic field_validator. change (and delete) some tests around use of memoryviews and other bytes equivalents - the idea is that the RP needs the flexibility to store its values in any bytes-equivalent manner - but the responses from the authenticator needs to follow the spec.
|
|
| raw_id=base64url_to_memoryview( | ||
| raw_id= | ||
| "fq9Nj0nS24B5y6Pkw_h3-9GEAEA3-0LBPxE2zvTdLjDqtSeCSNYFe9VMRueSpAZxT3YDc6L1lWXdQNwI-sVNYrefEcRR1Nsb_0jpHE955WEtFud2xxZg3MvoLMxHLet63i5tajd1fHtP7I-00D6cehM8ZWlLp2T3s9lfZgVIFcA" | ||
| ), | ||
| , | ||
| response=AuthenticatorAssertionResponse( | ||
| authenticator_data=base64url_to_memoryview( | ||
| authenticator_data= | ||
| "SZYN5YgOjGh0NBcPZHZgW4_krrmihjLHmVzzuoMdl2MBAAAABw" | ||
| ), | ||
| client_data_json=base64url_to_memoryview( | ||
| , | ||
| client_data_json= | ||
| "eyJ0eXBlIjoid2ViYXV0aG4uZ2V0IiwiY2hhbGxlbmdlIjoiZVo0ZWVBM080ank1Rkl6cURhU0o2SkROR3UwYkJjNXpJMURqUV9rTHNvMVdOcWtHNms1bUNZZjFkdFFoVlVpQldaV2xaa3pSNU1GZWVXQ3BKUlVOWHciLCJvcmlnaW4iOiJodHRwOi8vbG9jYWxob3N0OjUwMDAiLCJjcm9zc09yaWdpbiI6ZmFsc2V9" | ||
| ), | ||
| signature=base64url_to_memoryview( | ||
| , | ||
| signature= | ||
| "RRWV8mYDRvK7YdQgdtZD4pJ2dh1D_IWZ_D6jsZo6FHJBoenbj0CVT5nA20vUzlRhN4R6dOEUHmUwP1F8eRBhBg" | ||
| ), | ||
| , |
There was a problem hiding this comment.
I don't understand why this test was changed. base64url_to_memoryview() emulates an RP that chooses to use a different data structure than plain bytes, and then decides to map their data to the shape of AuthenticationCredential...
Maybe what's missing here is that AuthenticationCredential is an instance of the spec's PublicKeyCredential structure, which says rawId is an ArrayBuffer - that's captured in this library as raw_id: bytes. If you want to make it possible for AuthenticationCredential.raw_id to be a str then I don't think that makes sense, since the library will ensure that raw_id bytes can be base64url-encoded to be the same value as id. If an RP wants to use something else to store raw_id bytes in their database prior to the bytes ever making it to webauthn.verify_*_response(), then that should be up to them. They just need to get it back to bytes before this library's methods are called.
There was a problem hiding this comment.
Also #172 introduced webauthn.helpers.parse_registration_credential_json and webauthn.helpers.parse_authentication_credential_json. They're called automatically by the respective verify_*_response() methods, which now support stringified JSON or a json.parse()'d JSON dict. Perhaps that solves the developer ergonomic issue that you were trying to solve here?
|
Thanks for the comments - I will try tests again once 1.11 is out. I changed the test since my understanding is that these tests are faking up an AuthenticationAssertionResponse - which is a client to RP data structure which the spec dictates the data types for. An RP never creates an AuthenticationAssertionResponse. An RP may, in the case of say user_handle - strore ITS version as a memory view or bytes - and wants to be able to compare that to a properly decoded AuthenticationAssertionResponse - which the current code doesn't properly decode. The gist of this PR is to say that how client data structures are decoded should be dictated by the spec, not by python/pydantic data structures) |
|
This was an overly-aggressive change - working on something more focused.... |
UserHandle is tagged as Optional[bytes] so field.annotation == bytes doesn't match - add explicit match for user_handle. The conversion from base64 should only be done if input was json - there was a comment to this effect that that didn't work when using parse_raw - but recent releases, using Pydantic 2.0 no longer use those deprecated methods.
Instead of relying on input types to drive conversion - set up a field_validator that enumerates all spec-defined base64 fields and convert them. Remove the base64url decoding from the generic field_validator.
change (and delete) some tests around use of memoryviews and other bytes equivalents - the idea is that the RP needs the flexibility to store its values in any bytes-equivalent manner - but the responses from the authenticator needs to follow the spec.
closes #168