-
Notifications
You must be signed in to change notification settings - Fork 14
Populate allowCredentials for authenticators without residential keys #89
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
Conversation
Latest version New parameter for authentication debugging Fixing credential list Struct for a credential added documentation more documentation final Fixing type Removin transports Fixing typo
Thank you @Terbium-135 for opening this PR. I plan on reviewing this in the next week or two. |
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.
@Terbium-135 Sorry it took so long to give this a proper review. I appreciate the changes in this PR, and I've left feedback to help move this across the finish line.
To keep it from languishing too much longer, I may go ahead with applying the changes I requested in a few weeks if you haven't had time to address them. It's been several months since you opened the PR, so it's understandable if you are hesitant to follow up.
<span :if={@show_icon?} class="aspect-square w-4 opacity-70"><.icon_key /></span> | ||
<span>{@display_text}</span> |
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.
These changes should be omitted since they're not related to the goals of the PR.
It may still be too early to convert to the new brackets syntax ({@display_text}
) since there are possibly applications using this package with older versions of LiveView. I will consider making this change in a separate branch while updating dependencies.
userVerification, | ||
}; | ||
|
||
console.log(publicKey); |
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.
This console log should be removed.
// authenticatorAttachment: "platform", | ||
authenticatorAttachment: "all", |
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.
It appears "all"
is not a valid option here.
https://www.w3.org/TR/webauthn-3/#enumdef-authenticatorattachment
In fact, I could not find "all"
anywhere in the spec, even going back to past drafts.
Ultimately, the plan is to make all of these parameters configurable from the Elixir code instead of hardcoding things in JS. That's out of scope for this branch, but worth noting.
defmodule WebauthnComponents.RegistrationComponent do | ||
@moduledoc """ | ||
A LiveComponent for registering a new Passkey via the WebAuthn API. | ||
A LiveComponent for registering a new Passkey via the WebAuthn API! |
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.
This should be omitted.
Closing this PR since it goes a bit beyond its scope and may break compatibility with older versions of LiveView. #98 is currently pending and should allow more flexibility when registering credentials. At any rate, thank you for working on this. |
Overview
Some authenticators don't have resident keys. They need the application to support a list of credentials to select from
Changes
Added a few variables to the authentication component:
If the authenticator is not supporting resident keys you have to provide a list of
%WebauthnCredentials{}
either by using:@user
(Optional) A user identifier like e-mail or user name as a string@retrieve_credentials_function
(Optional) A function of the type@spec retrieve_credentials_for(binary) :: [%WebauthnCredential{}]
which is supposed to get the IDs and the public keys for a given useror
@allow_credentials
(Optional) A list of%WebauthnCredentials{}
If these variables are missing the component falls back to simple passkey behaviour.
Added a credential struct
WebauthnComponents.WebauthnCredential
to serialize and transfer the needed credentials.Mix test