-
-
Notifications
You must be signed in to change notification settings - Fork 266
feat: add webauthn #895
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
base: master
Are you sure you want to change the base?
feat: add webauthn #895
Conversation
|
No idea how Copilot came up with the idea of reviewing this (should be disabled 🤔), but please ignore those comments. I'll give this a review as soon as possible. Thanks for your contribution. |
Well I've opened it on my side as I wondered how it worked :) Don't be that hurry as I'm going to take a exam and maybe work on it again next weekend or later. BTW: I'm aware that I forgot to use snake-case for attestation json somewhere, and I should brotil the webauthn library too. Will fix them next time I'm free. |
Done |
muety
left a comment
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.
Thanks a lot for this PR! Sorry for taking so long with this review and sorry in advance for the number of comments I put. Please have a look at them anyway.
Also, could you please:
- Add some basic tests for this, if possible? Perhaps do some research how to best tests stuff like this.
- Add a config option to explicitly enable / disable WebAuthn login, just like you can disable OIDC.
…redential storing problems, and update settings UI
Will do some research next time I'm free. A questions while browsing existing tests: how do you generate the mocks? Are they generated by a program (like mockery )? Or are they written by hand?
Before implementing it, I want to know: should the users registered through OIDC be allowed to set up passkeys and log in with a passkey? (and why OIDC users should not be able to set a password for themselves, BTW) |
Should it be a unit test or an API test? And found a library descope/virtualwebauthn for virtual testing.
Done. There's currently a |
closes #613