search

Stormbase / django-otp-webauthn

Passkey support for Django. Currently in early stages of development and not ready for production use!
BSD 3-Clause "New" or "Revised" License
10 stars 2 forks source link

Exception raised during authentication if multiple authentication backends are configured #7

Closed jmichalicek closed 1 month ago

jmichalicek commented 1 month ago

Tested with django-opt-webauthn 0.13, django 5.0.6, python 3.11.7

When django has multiple authentication backends configured and the user is not already logged in, such as using this for passwordless authentication, an exception is raised when calling django.contrib.auth's login() function. A full stack trace is at the bottom.

For example in django settings:

AUTHENTICATION_BACKENDS = (
    "django.contrib.auth.backends.ModelBackend",
    "social_core.backends.google.GoogleOAuth2",
)

I'm happy to play with this and make a PR, probably some time this coming week.

Based on work with other auth backend related code, I suspect the most django-ish approach to resolving this is to provide a very simple custom back end which can then be added to django's AUTHENTICATION_BACKENDS setting and then django.contrib.auth.authenticate() could be called either in BeginCredentialAuthenticationView if the user is not already authenticated or just before login() (as auth_login()) is called in CompleteCredentialAuthenticationView with the authenticating device passed into it . My feeling is that if the user already authenticated via username/password or some other method and this is just being used as 2fa then the original authentication backend should remain set on the user.

Alternately, that back end could just be set directly when calling login(), but that feels off to me to specify a backend which the user may not have set in their authentication backends, so I think the use of the built in authenticate() method is probably the way to go.

The custom backend feels a bit weird because I think there would be almost no logic since most of the logic needs to happen in the views whether this is being used for passwordless auth or not, and if some other auth happened first and the logic was in this new auth backend then logic which needs to be called anyway would end up skipped.

Traceback (most recent call last):
  File "/app/.venv/lib/python3.11/site-packages/django/contrib/auth/__init__.py", line 129, in login
    backend = backend or user.backend
                         ^^^^^^^^^^^^

During handling of the above exception ('User' object has no attribute 'backend'), another exception occurred:
  File "/app/.venv/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/sentry_sdk/integrations/django/views.py", line 90, in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django/views/decorators/csrf.py", line 65, in _view_wrapper
    return view_func(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django/utils/decorators.py", line 48, in _wrapper
    return bound_method(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django/views/decorators/cache.py", line 80, in _view_wrapper
    response = view_func(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django_otp_webauthn/views.py", line 58, in dispatch
    return super().dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
    ^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django_otp_webauthn/views.py", line 245, in post
    self.complete_auth(device)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django_otp_webauthn/views.py", line 200, in complete_auth
    auth_login(self.request, user)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.venv/lib/python3.11/site-packages/django/contrib/auth/__init__.py", line 135, in login
    raise ValueError(
    ^

Exception Type: ValueError at /webauthn/authentication/complete/
Exception Value: You have multiple authentication backends configured and therefore must provide the `backend` argument or set the `backend` attribute on the user.
Stormheg commented 1 month ago

Thanks for raising another bug report!

My feeling is that if the user already authenticated via username/password or some other method and this is just being used as 2fa then the original authentication backend should remain set on the user.

This sounds sensible to me, we shouldn't override the backend if the user is already authenticated.

Regarding the model backend, I'm not 100% sure what the best course of action would be.

Option 1 - special 'webauthn' custom backend

We could create a custom backend that performs essentially no logic and only functions to indicate a user was logged in through a Passkey.

Cons:

Option 2 - some way to indicate which backend to use

Alternatively, there could be some way to indicate which backend should be used in the call to login. Perhaps a setting or something? Looking at the documentation for selecting the authentication backend, perhaps we could set user.backend before calling login?

More thoughts and approaches welcome, I don't usually venture into this area of Django so my experiences are fairly limited.

jmichalicek commented 1 month ago

I've started playing and have what appears to be a functional back end. It works through django's built in django.contrib.auth.authenticate(). That just loops through the configured authentication backends in order and passes in whatever kwargs were passed to authenticate(). So there we use a kwarg which is specific to this backend (seems to be the norm. If there is some sort of collision for a user, they could always subclass this backend and change the kwarg it uses). Once a backend returns a user the authenticate() function sets user.backend, which is then used by django.contrib.auth.login() to know which backend to continue with. That is also stored in the session and is used by AuthenticationMiddleware to know which backend to call get_user() on after authentication and login have been completed successfully.

I want to sleep on it overnight and take another look tomorrow with fresh eyes (sort of fresh, it'll be after work).

jmichalicek commented 1 month ago

Alright, I think this does the trick. I've left a pretty lengthy explanation on the PR.

Thanks for all of your work on this. Adding passkeys support as a standard thing in our projects at work is one of our near future goals. I evaluated other options out there and was unhappy with them for several reasons. I started planning my own and then found this project, which is very similar to what I was planning, so I'm happy to help out with this rather than have to create and maintain my own from the ground up.

With that said, I've got a other PRs I'd like to make based on things I've seen (some definitely needing discussion first, this is your project, not mine and I just want to be helpful). The first one, off the top of my head, is just around some of the type annotations. I can make an issue or just a straight PR with the details. I'd really like to help get this off the ground and as useful as possible, so I'm happy to help in a number of places, depending on free time and/or my employer giving me time.

Stormheg commented 1 month ago

Thank you for the kind words, much appreciated 🙏

To add my own motivation for building this: I created this package because I wanted away with TOTP codes and wasn't happy with the Django packages out there that already implement Passkey support. If you care to watch: I gave a talk about wagtail-mfa; a package that uses django-otp-webauthn and integrates it with Wagtail. https://www.youtube.com/watch?v=qJwg2kFtFW4

I'd be open to work on the type annotations. I don't usually add them to my projects so not sure what the best practices are, feel free to educate me.

Right now, I think this package mostly needs:

  1. A good test suite
  2. Documentation
  3. Users to provide feedback (if you have ideas to make it better: happy to hear them)

I'll shoot you an email with my contact details, that would probably be a quicker and easier way to discuss your ideas than by creating issues I feel.

jmichalicek commented 1 month ago

I'll definitely check out your talk. I'm also a big wagtail user. I've been fortunate enough to be able to discover a bug and contribute back to the project, use it one one of my personal websites, and have used it (and very much abused and did terrible things with it) at work.

I'm more than happy to help with all of those things. I've been a big proponent of pushing testing and type hints (and various other code quality stuff, I'm happy to help with linters, formatters, making them simple to use, etc.) at work. And documentation is of course always good.

We've got a cookeicutter project template we use as a base for all of our projects (with everything from terraform stuff to configure standard services, github, etc. to standard django tooling and layout we use) which I am building this into so that it's always easily available to us. As we get some new projects going (agency life, it's always feast or famine) and I start needing to override and customize stuff, I'm sure I'll have feedback and suggestions there as well.

I just now got your email, so I'll definitely follow up this evening or tomorrow.

Stormheg commented 1 month ago

Fixed by #8 and shipped in v0.2.0 🚀