-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix handling of optional fields such as user_handle. #180
Conversation
So I found two years ago, from a developer ergonomics story, that it's much better to not base64url-encode/decode The problem over here in py_webauthn land is that I designed this API to pair well with @simplewebauthn/browser methods that use UTF-8 encoding/decoding on the front end specifically for I think that leaves docstrings as an option here as clearly the use of UTF-8 is non-obvious to anyone but me 🫠 |
I have looked at the spec over and over - and still get confused. One thing that really confused me was this section: https://www.w3.org/TR/webauthn-2/#sctn-automation-add-credential I don't really care either way - as long as it's documented. With pydantic V1 - it is explicitly called out as b64encoded in _object_hook_base64url_to_bytes when decoding JSON (along with a bunch of other fields). With pydantic V2 - since it is looking at the python typing information, it is looking for ALL fields that are bytes, and any 'string' is b64decoded - the reason user_handle isn't being decoded is JUST because it is typed as Optional[bytes] and trying to test for 'Optional' is difficult (I googled it). In my first PR - I was overly aggressive - but felt that using python types as a proxy for how the fields are encoded seems fragile - as opposed to how you did in with pydantic V1 - where you explicitly coded which fields should be b64encoded. In any case - what I think is missing is explicit documentation for the wire protocol that py_webauthn accepts and emits - which would allow front ends and RPs to be written. If you feel like with the move to pydanticV2 and 1.11 release that user_handle should be raw bytes - I can easily change my front end and RP. Finally - and the reason I initially filed the issue is that as the code currently is - there is a difference of how user_handle is managed depending on whether the application uses pydanticV1 or V2 - so effectively the wire protocol that py_webauthn exposes is different (and not backwards compatible) - that should be documented - and if you think it would be better to have user_handle be UTF-8 - this would be a great time to break compatibility! |
Ok - I hopefully fixed mypy - I really believe that supporting standard JSON from front-end to RP is important - and therefore each of the fields passed needs to be json-serializable - and b64urlencode is common. So webauthn really should support that for user_handle. |
Hello! any chance we could either get this in or figure out how to solve this regression when moving to pydantic v2? Thanks! |
You know what, I just realized that I've been base64url-encoding Let me look again at what you're proposing in this PR, I thought it was changing this behavior but you're only trying to ensure that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay @jwag956, I just have a few requests before this can get merged.
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.
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.
closes #174