Skip to content
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

Support for Django Channels (ASGI) #712

Open
Archmonger opened this issue Jan 27, 2021 · 14 comments
Open

Support for Django Channels (ASGI) #712

Archmonger opened this issue Jan 27, 2021 · 14 comments

Comments

@Archmonger
Copy link

Archmonger commented Jan 27, 2021

Currently, Django Axes results in an exception when used with the login functionality within Django-Channels channels.auth.

If multiple backends are defined in settings.py, Django Channels requires the user to define a backend to authenticate with. For example,

            await login(
                self.scope,
                self.scope["user"],
                backend="django.contrib.auth.backends.ModelBackend",
            )

But when the user defines this backend, it breaks the Axes authentication stack.

ERROR conreq.utils.server_websockets: Websocket failed to connect: Login failure.
Traceback (most recent call last):
  File "C:\Users\Username\Repositories\Conreq\conreq\utils\server_websockets.py", line 32, in connect
    await login(
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\asgiref\sync.py", line 296, in __call__
    ret = await asyncio.wait_for(future, timeout=None)
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\asyncio\tasks.py", line 455, in wait_for
    return await fut
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\concurrent\futures\thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\channels\db.py", line 13, in thread_handler
    return super().thread_handler(loop, *args, **kwargs)
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\asgiref\sync.py", line 334, in thread_handler
    return func(*args, **kwargs)
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\channels\auth.py", line 108, in login
    user_logged_in.send(sender=user.__class__, request=None, user=user)
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\django\dispatch\dispatcher.py", line 177, in send
    return [
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\django\dispatch\dispatcher.py", line 178, in <listcomp>
    (receiver, receiver(signal=self, sender=sender, **named))
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\axes\signals.py", line 33, in handle_user_logged_in
    AxesProxyHandler.user_logged_in(*args, **kwargs)
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\axes\helpers.py", line 476, in inner
    return func(*args, **kwargs)
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\axes\handlers\proxy.py", line 108, in user_logged_in
    return cls.get_implementation().user_logged_in(sender, request, user, **kwargs)
  File "C:\Users\Username\Repositories\Conreq\venv\lib\site-packages\axes\handlers\database.py", line 199, in user_logged_in
    clean_expired_user_attempts(request.axes_attempt_time)
AttributeError: 'NoneType' object has no attribute 'axes_attempt_time'

Perhaps it's worth discussing with the Django Channels team whether a workaround can be reached with their login function.

@aleksihakli
Copy link
Member

Hey @Archmonger, thanks for reporting! I think ASGI support would be essential to have as well. This would also be a good target for version 6.0.0.

Since we'd need to define a target setup that should work out-of-the-box or otherwise, would you mind posting example settings that should work with Axes and ASGI enabled?

@Archmonger
Copy link
Author

Archmonger commented Jan 30, 2021

This is the GitHub repository that I've been attempting to reach compatibility with https://github.com/Archmonger/Conreq. Repo only has python dependencies.

@aleksihakli
Copy link
Member

aleksihakli commented Jan 30, 2021

That's good, let's go forward from there.

What other Django packages are you using by the way? It seems like the error is coming from here and the request should just be updated with the request.axes_attempt_time attribute, but it seems like the request is None. Where does that None request come from? Should we have a check for None request in user_logged_in similar to e.g. user_login_failed method?

@Archmonger
Copy link
Author

Archmonger commented Jan 30, 2021

The none value for request probably comes from the fact that Django channels refuses to work through multiple authentication backends.
If multiple backends are configured, it will force the user to define only one for every login call.

            await login(
                self.scope,
                self.scope["user"],

                backend="django.contrib.auth.backends.ModelBackend", # <<< This line right here
            )

So therefore the request never propagates up your Axes stack and all axes request attributes are never populated. Or at least that's my leading theory. I haven't looked at your team's source code and I'm not entirely familiar with the Django request stack.

Here's a list of django requirements I pulled out of my requirements.txt

awesome-django-timezones==0.3.0
channels==3.0.3
diskcache==5.1.0
django==3.1.5
django-htmlmin==0.11.0
django-solo==1.1.5
django-searchable-encrypted-fields==0.1.9
django-cleanup==5.1.0
daphne==3.0.1
django-project-version[git]==0.13.0
pwned-passwords-django==1.4.1
whitenoise[brotli]==5.2.0

@Archmonger
Copy link
Author

Just had a thought. Perhaps the to solution to "only one auth backend can be set" within channels is having the axes backend wrap one or more backends. For example a through a AXES_AUTH_BACKENDS variable.

@aleksihakli
Copy link
Member

@Archmonger wrapping is actually a nice idea, if it can be done cleanly! Would you be interested in inspecting this on the code level? The authentication backend API specification from Django docs would need to be followed and ASGI implementation checked as well.

@Archmonger
Copy link
Author

Sure. I'll consider a PR on axes as a part of my task list. I have a few higher priority cards so I'll get to it within roughly a month.

@Archmonger
Copy link
Author

My work schedule slowed down my OSS work. Still aiming to tackle this as a PR but unsure what my timeline looks like.

@aleksihakli
Copy link
Member

Roger that @Archmonger 👍 No need to worry about timelines since Axes is a voluntary FOSS project.

@Archmonger
Copy link
Author

Just jotting down some ideas I had today in regards to the implementation of this future PR...

We can automate the whole AUTHENTICATION_BACKENDS wrapper idea.

  1. Remove instructions for adding in a custom value to AUTHENTICATION_BACKENDS from the docs.
  2. Read os.environ["DJANGO_SETTINGS_MODULE"] and forcibly set AUTHENTICATION_BACKENDS to an array containing only axes.backends.AxesBackend
    • Probably want to forcibly set this value in django.conf. Will need testing to see what doesn't break Django.
  3. axes.backends.AxesBackend will perform login functionality identical to the way Django internally handles it based on the original value of AUTHENTICATION_BACKENDS.
    • We will iterate down and attempt auth for whatever was contained in the original AUTHENTICATION_BACKENDS array (essentially as a wrapper for all auth backends)
    • Ideally we use the exact same functions that Django core is internally using to ensure 1:1 logical parity

@aleksihakli
Copy link
Member

Forcibly setting the backend could lead to hard-to-debug problems in some stacks. Could there be alternatives to this approach or could we e.g. introduce a new flag for configuring another authentication backend separately in Axes that we could then invoke from e.g. a new Axes ASGI authentication wrapper backend?

@Archmonger
Copy link
Author

Archmonger commented Oct 13, 2021

Forcibly setting the values isn't necessary, I just thought it might be a clever way to automate some setup.

The alternative is:

AUTHENTICATION_BACKENDS = ['axes.backends.AxesBackend']
AXES_BACKENDS = [ 'example.one', 'example.two' ]

Reusing the axes.backends.AxesBackend for this would be backwards compatible, as we can preserve old behavior if AXES_BACKENDS is not set.


This wouldn't really improve debug capabilities though, I'd say it's equivalent to the last method except isn't not automating the configuration.

I don't really think adding axes in the loop in either scenarios would affect stack traces in any harmful way. The user would just see an axes stack preceding the authentication stack that suffers from an exception.

@aleksihakli
Copy link
Member

aleksihakli commented Oct 13, 2021

I'd rather have an added configuration key in the settings and think that "explicit is better than implicit" in this case. If we are using ASGI and have added the extra Axes settings, it might also be feasible to add system checks in the Django checks framework for making sure that the user has correctly configured the authentication stack i.e. has the AUTHENTICATION_BACKENDS and AXES_AUTHENTICATION_BACKENDS configured with correct looking types and values.

@Archmonger
Copy link
Author

Archmonger commented Oct 13, 2021

Agreed, adding some checks would be a good addition. We definitely want to raise django.core.exceptions.ImproperlyConfigured if the settings are bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants