Remove webauthn-json
#31
Conversation
|
|
||
| credential = current_user.webauthn_credentials.find_or_initialize_by( | ||
| external_id: Base64.strict_encode64(webauthn_credential.raw_id) | ||
| external_id: Base64.urlsafe_encode64(webauthn_credential.raw_id, padding: false) |
There was a problem hiding this comment.
Should we simply go with the following?
external_id: webauthn_credential.idDo we know why it was decided to encode in the first place?
There was a problem hiding this comment.
That's a good call actually... in fact that's how we do it in the README 😅
https://github.com/cedarcode/webauthn-ruby/blob/9405421/README.md#verification-phase-1
I guess is because we brought that code from our webauthn-rails-demo-app. Probably is done that way there because we didn't want to store the id encoded a base64url? Can really tell to be honest...
However... do we actually need to change this as part of this PR? What do you think about doing it as a separate PR?
|
|
||
| credential = current_user.webauthn_credentials.find_or_initialize_by( | ||
| external_id: Base64.strict_encode64(webauthn_credential.raw_id) | ||
| external_id: Base64.urlsafe_encode64(webauthn_credential.raw_id, padding: false) |
There was a problem hiding this comment.
That's a good call actually... in fact that's how we do it in the README 😅
https://github.com/cedarcode/webauthn-ruby/blob/9405421/README.md#verification-phase-1
I guess is because we brought that code from our webauthn-rails-demo-app. Probably is done that way there because we didn't want to store the id encoded a base64url? Can really tell to be honest...
However... do we actually need to change this as part of this PR? What do you think about doing it as a separate PR?
|
|
||
| credential = current_user.webauthn_credentials.find_or_initialize_by( | ||
| external_id: Base64.strict_encode64(webauthn_credential.raw_id) | ||
| external_id: Base64.urlsafe_encode64(webauthn_credential.raw_id, padding: false) |
There was a problem hiding this comment.
Out of curiosity: why is the padding: false needed?
There was a problem hiding this comment.
It removes the = character at the end.
Here's a comparison
Base64.urlsafe_encode64("lorem_ipsum") =>
"bG9yZW1faXBzdW0="
Base64.urlsafe_encode64("lorem_ipsum", padding: false) =>
"bG9yZW1faXBzdW0"With the padding at the end it fails:
EncodingError: Failed to execute 'parseRequestOptionsFromJSON' on 'PublicKeyCredential': 'allowCredentials' contains PublicKeyCredentialDescriptorJSON with invalid base64url data in 'id'
There was a problem hiding this comment.
Note that the id should be of type Base64URLString as can be seen here: https://www.w3.org/TR/webauthn-3/#dictdef-publickeycredentialdescriptorjson
The pad character "=" is typically percent-encoded when used in an
URI, but if the data length is known implicitly, this can be
avoided by skipping the padding.
This encoding may be referred to as "base64url".
There was a problem hiding this comment.
Ahh so the parseCreationOptionsJSON and parseRequestOptionsJSON expect the id as a base64url so we should either store in that format or send it to the client on such format.
Nice to know! As I said on this comment, I think we should do a separate PR to change that first and then we can continue with this one. What do you think?
There was a problem hiding this comment.
Done ✅
Merged this to main: #32
And rebased this branch, removing the changes for using urlsafe_encode64 instead of strict_encode64
joaquintomas2003
force-pushed
the
jt--remove_webauthn_json
branch
from
4e7c9e5 to
69dfbf8
Compare
Changes:
webauthn-jsonjs dependency and start usingnavigator.credentialsnative API calls instead:webauthn-jsonhas been deprecated: Add deprecation notice github/webauthn-json#94@github/webauthn-jsondependency from dummy app and stop injecting it in the generator script.References: