Closed jmichalicek closed 1 month ago
I jumped the gun on this PR, but fortunately caught a bug this morning before it had been merged and fixed it up along with a minor tweak for clarity and intent of some code.
Thanks again for the helpful PR @jmichalicek, great explanation about authentication backends too 👍
Would you please update the README instructions too to include adding this backend to the settings?
Ack. Yes, I meant to update the documentation. Good catch there.
django.contrib.auth.authenticate()
,django.contrib.auth.login()
, andAuthenicationMiddleware
CompleteCredentialAuthenticationView.complete_auth()
to calldjango.contrib.auth.authenticate()
beforedjango.contrib.auth.login()
if the user is not already authenticated via a different authentication backend.For a more general overview of what happens and how this resolves it, here's how the django auth and login systems work.
django.contrib.auth.authenticate()
loops over every backend configured insettings.AUTH_BACKENDS
, calling theauthenticate()
method on each one and passing in the kwargs passed intodjango.contrib.auth.authenticate()
.backend
attribute is set on the user to the backend which worked. A backend primarily determines what it will do based on the kwargs passed in. It may do nothing and returnNone
but also, In theory, two backends could take the same kwargs but do something different and so still work totally independently and one returnNone
and the other return a user.django.contrib.auth.login()
looks at several things.None
then it sets user torequest.user
backend
arg was passed in, preferring that, and then if the user instance has abackend
attribute set, preferring that nextsettings.AUTHENTICATION_BACKENDS
request.user
to the user, rotates the csrf token, and sends theuser_logged_in
signal.This bug was happening if multiple auth backends were configured, which is common, because if this was the primary/only authentication method then no backend was set, so the check for a backend arg and user.backend failed and then the check for only a single auth backend configured failed.
Additionally, once an auth backend is set, which is stored as part of the session,
AuthenticationMiddlware
looks at the session to see which backend was used, and then calls theget_user()
method on that backend, passing in the user id from the session. I kept this simple and just look up the user directly in that method. I initially considered going through the credential model fromget_credential_model()
but at this point all we have is a user id, so while that could verify that there is some webauthn credential we wouldn't know which one was used and only that one exists now, so just going straight to the user seems just as useful. Arguably going through the credential model at least ensures that some webauthn credential exists and so is probably the one which was used but the views already do enough work around that.I do feel like maybe more of the logic should actually live in this auth backend, but since we only want to use this backend if the user is not authenticated via a different method and the rest of the auth code is relevant even when using this as 2fa for an already authenticated user, sorting that logic out felt out of scope for this if it is even reasonably possible.