use an authentication backend to check if user is locked out. #315

markddavidoff · 2018-03-13T17:30:55Z

@camilonova First Draft for input.

USAGE

settings.py

AUTHENTICATION_BACKENDS = [
    'axes.middleware.DjangoAxesAuthBackend',
    'django.contrib.auth.backends.ModelBackend',
]

login view call example

user = User.objects.get(email=form.cleaned_data['email'])
response_context = {}
user = authenticate(
    request=request,
    username=user.username,
    password=form.cleaned_data['password'],
    response_context=response_context
)
...
if 'error' not in context:
    # if don't have a more specific error, use a generic one.
    context['error'] = 'Incorrect Login Information'
render(request, 'log_in.html', context)

markddavidoff · 2018-03-13T18:16:38Z

axes/middleware.py

+from django.contrib.auth.backends import ModelBackend
+from django.core.exceptions import PermissionDenied
+
+class DjangoAxesAuthBackend(ModelBackend):


Name suggestions?

markddavidoff · 2018-03-13T18:24:46Z

axes/middleware.py

+        if is_already_locked(request):
+            # locked out, don't try to authenticate, just update return_context and return
+            error_msg = get_lockout_message()
+            response_context = kwargs.get('response_context', {})


Its a bit weird to pass a context and expect a response value but its nice to get a "why" back.

camilonova · 2018-03-13T22:46:54Z

Looks good so far, please keep going.

aleksihakli · 2018-03-14T18:22:02Z

axes/middleware.py

+            # locked out, don't try to authenticate, just update return_context and return
+            error_msg = get_lockout_message()
+            response_context = kwargs.get('response_context', {})
+            response_context['error'] = error_msg


Does this mutate kwargs contents?

It expects a dict called 'response_context' to be passed as a kwarg and mutates that if it is present.

I agree its a bit weird to pass a context and expect a value set but its nice to get a "why" back and I couldn't figure out a better way to do this that still complies with the django auth backend structure.

aleksihakli · 2018-03-14T18:22:12Z

axes/conf.py

+    AXES_COOLOFF_MESSAGE = 'Account locked: too many login attempts. Please try again later'
+
+    # message to show when locked out and have cooloff disabled
+    AXES_PERMALOCK_MESSAGE= 'Account locked: too many login attempts. Contact an admin to unlock your account.'


Missing a newline.

also missing a space on the =

aleksihakli · 2018-03-14T18:22:32Z

axes/utils.py

+    if settings.AXES_COOLOFF_TIME:
+        return settings.AXES_COOLOFF_MESSAGE
+    else:
+        return settings.AXES_PERMALOCK_MESSAGE


Missing a newline.

aleksihakli · 2018-03-14T18:28:05Z

axes/middleware.py

+from django.core.exceptions import PermissionDenied
+
+
+class DjangoAxesAuthBackend(ModelBackend):


Maybe AxesModelBackend would be a neater name?

sure! I copy pasted a bunch of this from my subclass implementation and rushed to redact some of the naming and messages

aleksihakli · 2018-03-14T18:30:20Z

axes/middleware.py

+from axes.attempts import is_already_locked
+from axes.utils import get_lockout_message
+from django.contrib.auth.backends import ModelBackend
+from django.core.exceptions import PermissionDenied


A more traditional import order would be to import Python core packages first, then Django packages and then custom packages. Something like

from django import ... from axes import ...

aleksihakli · 2018-03-14T18:30:52Z

axes/middleware.py

@@ -0,0 +1,30 @@
+from axes.attempts import is_already_locked


This file is not middleware - it should be backends.py so the import for this module would be something like axes.backends.AxesModelBackend.

aleksihakli · 2018-03-14T18:34:14Z

Looks good, the naming is a bit conflicting as this patchset implements an authentication backend instead of middleware (though it's easy to see it as middlewareish in functionality).

The approach, however, is very nice. An authentication backend denying logins is MUCH more neater and universal than monkey patching view dispatchers which is the current functionality.

If you could clean this up slightly and implement unit tests for the backend it would be nice to get this merged 👍

camilonova · 2018-03-21T16:56:29Z

@markddavidoff can we have this soon? Looks amazing.

markddavidoff · 2018-03-21T17:07:16Z

Yeah, i hope to get to this this week or next week.

…

On Wed, Mar 21, 2018, 9:56 AM Camilo Nova ***@***.***> wrote: @markddavidoff <https://github.com/markddavidoff> can we have this soon? Looks amazing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTCn2vSicLER5I7PJ9oPZvfdiMHh-cCks5tgoZAgaJpZM4SpIXo> .

markddavidoff · 2018-04-06T22:57:30Z

axes/backends.py

@@ -21,6 +21,9 @@ def authenticate(self, request, username=None, password=None, **kwargs):
        :return: Nothing, but will update return_context with lockout message if user is locked out.
        """

+        if request is None:


@camilonova @aleksihakli addressed pr concerns, but i have another query as to handle when request is None.

Django Rest Framework (which is very widely used) does not set request when passing it to authenticate, so things like @authentication_classes((BasicAuthentication,)) will error out.

options:

Error out like this

no-op if no request and log a warning

But i think we need to address this as just a no-op is a way that axes could not be called due to programmer error (and i think security type things should be on-by-default).

I think this behaviour is fine for the time being. If someone wants to use this custom backend in their project they need to make sure it can function correctly.

aleksihakli · 2018-04-07T09:12:52Z

Looks good to me! I think this is ready for merging. I'll let @camilonova check and merge this :)

markddavidoff · 2018-04-07T19:18:45Z

moved the exception to a subclass and updated the documentation as well

aleksihakli

A few small cleanups could be used here :)

The RequestParameterRequired exception could maybe subclass the Django SuspiciousOperation exception?

This looks good for merging, though.

aleksihakli · 2018-04-08T08:26:35Z

axes/backends.py

@@ -7,6 +7,13 @@

 class AxesModelBackend(ModelBackend):

+    class RequestParameterRequired(Exception):


This exception should really live in axes.exceptions as per good naming conventions. You just create that exceptions.py file. There is no need to put this inside the AxesModelBackend class, nesting classes just makes this harder to read and find.

aleksihakli · 2018-04-08T08:28:46Z

docs/configuration.rst

+- make sure any auth libraries you use that call the authentication middleware stack pass request. Notably Django Rest
+  Framework (DRF) ``BasicAuthentication`` does not pass request. `Here is an example workaround for DRF`_.
+
+.. _Here is an example workaround for DRF: http://www.python.org


This doesn't seem like the right link.

Woops. Will fix.

aleksihakli · 2018-04-08T08:29:03Z

docs/configuration.rst

+
+- run ``python manage.py migrate`` to sync the database.
+
+Things to you might need to change in your code, especially if you get a ``AxesModelBackend.RequestParameterRequired``:


This can also be just axes.exceptions.RequestParameterRequired.

aleksihakli · 2018-04-08T08:34:23Z

axes/backends.py

+
+class AxesModelBackend(ModelBackend):
+
+    class RequestParameterRequired(Exception):


This should be moved into axes.exceptions file (you can just create that) as per good Django package conventions. Exceptions are easier to find when they are in standard locations.

I strongly disagree eith moving the exception and using suspicious operation, although im always open to naming changes.

Moving indicates it is a failure on the module level, which i dont think applies here. The convention, to me, suggests a "Misconfigured" or "BadParameter" parent exc in exceptions.py that can be subclassed here, but i think the subclass should live here as it is tied to the backend not the module.

Is this a suspicious operation? I think its more programmer error?

But maybe im thinking app as part of a big project wise and not standalone library wise

OK, I think it's valid to have this here as well. Would be nice to have some tests for this, but I think we can implement those later.

aleksihakli · 2018-04-08T08:38:27Z

axes/backends.py

+            response_context['error'] = error_msg
+            raise PermissionDenied(error_msg)
+
+        # No-op


We could check for the request authentication status in the model backend and lock out if there are duplicate requests.

Can you please clarify what you mean by 'check for the request authentication status'?

Never mind, I think this is already handled by our signal handler - backend combination.

aleksihakli · 2018-04-08T08:39:12Z

I guess the big question is if we want to do the access checking and locking out in the model backend?

markddavidoff · 2018-04-08T09:14:20Z

@aleksihakli, you mean as the default recommended way?

aleksihakli · 2018-04-10T21:44:02Z

@markddavidoff yeah! I think the best place for all that checking is the AxesModelBackend introduced here. That way we could catch all authentication requests and check their authentication status neatly in one place instead of monkey patching any views or affecting any other code.

markddavidoff · 2018-04-11T13:45:56Z

@aleksihakli I'm still confused what you mean, i believe this does all the checking the monkeypatched view does already. Am i missing other logic?

aleksihakli · 2018-04-11T17:02:24Z

@markddavidoff if the Django authentication signals which have handlers defined in axes/signals.py fire correctly, then axes gets information about user login attempts and logouts. Then this backend should work nicely.

I wondered if we should attempt to do access attempt logging in the backend level, but I suppose that isn't necessary in the case all signals work correctly. We should be able to remove the monkey patching with this change from axes altogether if everything works nicely with this new backend.

I'll go ahead and merge this changeset, let's refine it further in a separate PR if necessary :)

aleksihakli · 2018-04-11T19:23:30Z

@markddavidoff I ran a bunch of tests on top of the new backend and it seems that authentication succesfully fails from the signals, but raising the PermissionDenied does not correctly produce a 403 code in the backend in our test setup :(

aleksihakli · 2018-04-11T19:37:45Z

It seems that authenticate does not indeed return a 403 but just skips the authentication:

https://github.com/django/django/blob/master/django/contrib/auth/__init__.py#L62

So we will either have to modify the tests that check response code on an attempted login, implement a custom middleware that returns the 403 code, or continue monkey patching stuff.

markddavidoff · 2018-04-11T19:59:24Z

This is meant to be used in conjunction with another middleware that actually authenicates like in the example i added to the docs. We can use modelbackend in the tests. Any suggestions to make that more clear in code and docs.

…

On Wed, Apr 11, 2018, 3:37 PM Aleksi Häkli ***@***.***> wrote: It seems that authenticate does not indeed return a 403 but just skips the authentication: https://github.com/django/django/blob/master/django/contrib/auth/__init__.py#L62 So we will either have to modify the tests that check response code on an attempted login, implement a custom middleware that returns the 403 code, or continue monkey patching stuff. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTCn4ZPviLpglYq20gfgcYxxnGl-5eyks5tnluKgaJpZM4SpIXo> .

markddavidoff mentioned this pull request Mar 13, 2018

LoginView.dispatch vs Authentication backend #313

Closed

markddavidoff commented Mar 13, 2018

View reviewed changes

markddavidoff force-pushed the add-middleware branch from 3263cb4 to c4769c7 Compare March 13, 2018 18:22

markddavidoff commented Mar 13, 2018

View reviewed changes

markddavidoff force-pushed the add-middleware branch from c4769c7 to 0674f27 Compare March 13, 2018 21:56

markddavidoff mentioned this pull request Mar 13, 2018

Do not monkeypatch LoginView methods. #277

Closed

aleksihakli reviewed Mar 14, 2018

View reviewed changes

aleksihakli requested a review from camilonova April 1, 2018 11:33

aleksihakli added the enhancement label Apr 1, 2018

markddavidoff changed the title ~~add a middleware to check if user is locked out.~~ use an authentication backend to check if user is locked out. Apr 6, 2018

markddavidoff force-pushed the add-middleware branch from 818e882 to cd3c074 Compare April 6, 2018 22:52

markddavidoff commented Apr 6, 2018

View reviewed changes

markddavidoff added 2 commits April 6, 2018 16:01

add an authentication backend to check if user is locked out.

8580c40

add a check to make sure request is not None

d8cbb42

markddavidoff force-pushed the add-middleware branch from cd3c074 to d8cbb42 Compare April 6, 2018 23:01

markddavidoff added 2 commits April 7, 2018 11:41

change ValueError to specific Exception

c8acf69

updated docs

63e571f

aleksihakli approved these changes Apr 8, 2018

View reviewed changes

aleksihakli reviewed Apr 8, 2018

View reviewed changes

Fix link

07a9adb

aleksihakli merged commit d08aaa5 into jazzband:master Apr 11, 2018

		from django.core.exceptions import PermissionDenied


		class DjangoAxesAuthBackend(ModelBackend):

		@@ -7,6 +7,13 @@

		class AxesModelBackend(ModelBackend):

		class RequestParameterRequired(Exception):


		- run ``python manage.py migrate`` to sync the database.

		Things to you might need to change in your code, especially if you get a ``AxesModelBackend.RequestParameterRequired``:

use an authentication backend to check if user is locked out. #315

use an authentication backend to check if user is locked out. #315

Conversation

markddavidoff commented Mar 13, 2018 • edited

USAGE

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilonova commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksihakli commented Mar 14, 2018

camilonova commented Mar 21, 2018

markddavidoff commented Mar 21, 2018 via email

markddavidoff Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksihakli commented Apr 7, 2018

markddavidoff commented Apr 7, 2018

aleksihakli left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksihakli commented Apr 8, 2018

markddavidoff commented Apr 8, 2018

aleksihakli commented Apr 10, 2018

markddavidoff commented Apr 11, 2018

aleksihakli commented Apr 11, 2018

aleksihakli commented Apr 11, 2018

aleksihakli commented Apr 11, 2018

markddavidoff commented Apr 11, 2018 via email

markddavidoff commented Mar 13, 2018 •

edited

markddavidoff Apr 6, 2018 •

edited

aleksihakli left a comment •

edited