-
Notifications
You must be signed in to change notification settings - Fork 1k
2FA (Webauthn) #5795
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
2FA (Webauthn) #5795
Conversation
Hey @brainwane @di @dstufft @ewdurbin -- this should be good for a review! We're still waiting on duo-labs/py_webauthn#41 for multiple WebAuthn credential support; we can either push forwards with this PR and begin a new one for multiple credentials, or wait on that (and begin another task) while they cut a release. Thoughts? |
I think I'd prefer to merge this for support w/ multiple credentials if the Duo team's timeline is compatible with ours. @mschwager, is there anything you can do to help us resolve duo-labs/py_webauthn#41? |
This seems functionally excellent. Looks like we still need to sort out:
|
Adds requisite view behavior, client-side handling, templates, interfaces, and services. Still incomplete.
Initial login work.
Adds the client-side JS needed for login, some of the form validation, some of the views, some of the services.
Set cookie properly on WebAuthn login.
@woodruffw I've now updated the UI: No 2fa2fa by TOTP onlySingle security keyMultiple security keysAdd key pageRemove key modal |
@woodruffw it seems I've broken the remove modal - when trying to remove a key, I am getting a 404 error. I can't work out how to fix it - could you please look into this? |
Sure, I'll take a look in a moment. |
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.
Once we get the signing counter issues sorted, this looks 💯 to me.
This should be done by the authenticator, but isn't.
For real this time.
Emphasizes that we do WebAuthn validation elsewhere. See: pypi#5795 (comment)
@@ -83,6 +84,7 @@ <h2><a href="#my-account">My Account</a></h2> | |||
<li><a href="#compromised-password">{{ compromised_password() }}</a></li> | |||
<li><a href="#2fa">{{ twoFA() }}</a></li> | |||
<li><a href="#totp">{{ totp() }}</a></li> | |||
<li><a href="#utfkey">{{ utfkey() }}</a></li> |
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.
I did a double-take at utf
🙂
IIUC this is about U2F keys, not Unicode’s UTF, right?
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 is about U2F keys and not Unicode's UTF; the T stands for "two". I'm ok with this since it's just an anchor tag, but if you want to change it I figure a PR would be pretty easy!
Begins work on Webauthn as a supported 2FA method.
TODO:
utils.webauthn
helper methodsWebauthn
model viaIUserService
navigator.credentials
)cc @brainwane @nlhkabu @di @ewdurbin @dstufft