feat: pass request to AXES_COOLOFF_TIME callback
#1222
Conversation
If the AXES_COOLOFF_TIME is a callable or path to a callable taking an argument, pass the username to it. This should enable users to customize the cool off to be user dependant, and possibly implement a growing cool-off time: - First lockout cools off after 5 mins - Second one after 10 mins - etc...
browniebroke
force-pushed
the
cool-off-time-callback-username-argument
branch
from
b22f022
to
982ebe3
Compare
|
Looks like the test failures are the same as another PR (#1221) and they don't look related to my change so I'm going to ignore them for this. Any idea how to fix them? I'll try to take a look if I find time |
There was a problem hiding this comment.
Looks good but I think the implementation should just take the request if a parametrizable callback is implemented as that contains the username and other metadata that is useful for processing as well, offering users wanting to customize more flexibility for the same price.
axes/helpers.py
Outdated
| if cool_off is None: | ||
| return None | ||
| return int(cool_off.total_seconds()) | ||
|
|
||
|
|
||
| def get_cool_off() -> Optional[timedelta]: | ||
| def get_cool_off(username: Optional[str] = None) -> Optional[timedelta]: |
There was a problem hiding this comment.
Would it be better to just supply the request object here so the user can use any field present in there? IP address or other metadata could be interesting as well. The request can also be used to carry attached attributes used in upstream / downstream processing in the middleware stack, if needed. username seems a bit limiting if we're implementing features here.
Adding the use of request object requires a few modifications in other places as well, but should be easy enough to test.
There was a problem hiding this comment.
That suggestion made a lot of sense when I first read it, but now that I'm trying to make the changes, I remembered why I went for this initial implementation...
The place where it's called, user_login_failed calls a function to resolve the username get_client_username, passing down some credentials. I can see that this function defaults to using request.data or request.POST.
I think I'm lacking a bit of context on the lifecycle where this is used, is that fair to assume that this will be called in a POST request where the username will be included in the body? It feels it may not always be there in some cases like SSO/OAuth flow. Are these kind of flows out of the scope of django-axes?
There was a problem hiding this comment.
Hey, a good question on the authentication methods and flows, I initially missed this one. Sorry about that.
SSO flows such as SAML and OAuth are absolutely in scope of this plugin. Ideally django-axes should be login method agnostic, if possible.
If anyone else is checking this discussion it might be useful to see the Django authentication signals documentation at https://docs.djangoproject.com/en/dev/ref/contrib/auth/#module-django.contrib.auth.signals for reference.
You are right in that the request object might not contain all the relevant information needed to dig up e.g. username or other identifying login data if the login is coming in from e.g. a SAML request, but request is the only persistent information, really, that we get from all the login flows i.e. going up and down the middleware stack and in all the login signals as described at e.g. https://django-axes.readthedocs.io/en/latest/7_architecture.html
At the current time we always expect a request for the get_lockout_response callable and credentials as an optional.
It actually might be nice to pass the credentials argument as well, as you pointed out.
If you're open to it I think it'd be OK to add that argument as well and bake a version 7.x from that. If you want to make a PR I'll be happy to make a new release for that.
There was a failing test that was produced by a change in the Django upstream implementation that limits header sizes. I updated the test, if you rebase this PR changeset on top of jazzband/django-axes repository master branch the test should be fixed in this branch as well. |
As we pass down the whole request, we no longer need to extract the axes_attempt_time anymore. This is a potential breaking change, but the impacted functions are not part of the documented API.
browniebroke
changed the title
AXES_COOLOFF_TIME callbackAXES_COOLOFF_TIME callback
|
I've pushed more updates, trying to me more granular with my commits. I didn't rebase anything yet to help you keep track of the changes you already reviewed, and the new ones I've added. I can rebase them when/if you want me to. |
There was a problem hiding this comment.
Thanks @browniebroke the changeset looks good 👍 Do you think it would be feasible to drop the _maybe_partial implementation and just directly invoke the settings.AXES_COOLOFF_TIME callable without the functools.partial and inspect additions? I think that'd make for an easier-to-understand API in the long run and we can just bake a breaking release for this changeset with e.g. major release version 7 if needed, it's not a big effort. I can write up the changelog and bake a release after this has been merged.
|
I think it's all done now. Let me know if I should update the changelog with some "unreleased changes". |
|
Yeah looks good to me, thank you 👍 I'll merge and bake a changelog and a release for this. |
|
Hey @browniebroke I made a release version 7.0.0 with associated changelog and notes for upgrading, do let me know if there's anything missing. I also missed your earlier comment regarding e.g. SSO logins, please check it out and let me know if you have any thoughts on that. Cheers for the good PR 👍 |
|
Thanks a lot for the quick feedback and very prompt release! |
What does this PR do?
If the
AXES_COOLOFF_TIMEis a callable or path to a callable taking an argument, pass the username to it.This should enable users to customize the cool off to be user dependant, and possibly implement a growing cool-off time:
Past references of this:
This PR should be backwards compatible with previous behaviour: if the
AXES_COOLOFF_TIMEis a callable that takes no argument, it will be called without parameters.Before submitting