Fix issue 636, pass request object to authenticate function.#643
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Pull Request Test Coverage Report for Build 1163
💛 - Coveralls |
| """ | ||
| u=authenticate(username=username, password=password) | ||
| u=authenticate(request, username=username, password=password) | ||
| ifuisnotNoneandu.is_active: |
There was a problem hiding this comment.
is_active is already checked in django (>=1.11 I think) do we need to check this here again?
There was a problem hiding this comment.
This fix make sure request as a parameter to authenticate function, so authentication module can get request information.
There was a problem hiding this comment.
@cleder I agree regarding is_active, in fact Django also skips the check if user model doesn't implement is_active. Let's remove this part.
There was a problem hiding this comment.
It could be a separate PR though. Approving as the fix is correct.
adamchainz commented Jul 8, 2019
This would be useful for me. I just discovered django-axes, requires What's the blocker on this getting merged? I'm happy to take over. |
The [usage documentation](https://django-axes.readthedocs.io/en/latest/3_usage.html) advises to create subclass of `AxesBackend` to ignore the lack of `request` if necessary. I've done this in a project using `django-oauth-toolkit`, which doesn't pass `request` (though it should as per [this PR](django-oauth/django-oauth-toolkit#643)). This meant that the axes.W003 check was being triggered, so I've fixed it to check for subclasses of `AxesBackend` as well as the class itself.
The [usage documentation](https://django-axes.readthedocs.io/en/latest/3_usage.html) advises to create subclass of `AxesBackend` to ignore the lack of `request` if necessary. I've done this in a project using `django-oauth-toolkit`, which doesn't pass `request` (though it should as per [this PR](django-oauth/django-oauth-toolkit#643)). This meant that the axes.W003 check was being triggered, so I've fixed it to check for subclasses of `AxesBackend` as well as the class itself.
The [usage documentation](https://django-axes.readthedocs.io/en/latest/3_usage.html) advises to create subclass of `AxesBackend` to ignore the lack of `request` if necessary. I've done this in a project using `django-oauth-toolkit`, which doesn't pass `request` (though it should as per [this PR](django-oauth/django-oauth-toolkit#643)). This meant that the axes.W003 check was being triggered, so I've fixed it to check for subclasses of `AxesBackend` as well as the class itself.
The [usage documentation](https://django-axes.readthedocs.io/en/latest/3_usage.html) advises to create subclass of `AxesBackend` to ignore the lack of `request` if necessary. I've done this in a project using `django-oauth-toolkit`, which doesn't pass `request` (though it should as per [this PR](django-oauth/django-oauth-toolkit#643)). This meant that the axes.W003 check was being triggered, so I've fixed it to check for subclasses of `AxesBackend` as well as the class itself.
W0126 commented Aug 14, 2019
@adamchainz we may need at least 1 reviewers with write access to approve this. |
adamchainz commented Aug 14, 2019
@cleder ? Also I volunteer myself to be given write access if that helps :) |
IvanAnishchuk left a comment
IvanAnishchuk left a comment There was a problem hiding this comment.
is_active check could be improved but otherwise valid fix.
| """ | ||
| u=authenticate(username=username, password=password) | ||
| u=authenticate(request, username=username, password=password) | ||
| ifuisnotNoneandu.is_active: |
There was a problem hiding this comment.
@cleder I agree regarding is_active, in fact Django also skips the check if user model doesn't implement is_active. Let's remove this part.
| """ | ||
| u=authenticate(username=username, password=password) | ||
| u=authenticate(request, username=username, password=password) | ||
| ifuisnotNoneandu.is_active: |
There was a problem hiding this comment.
It could be a separate PR though. Approving as the fix is correct.
adamchainz commented Nov 19, 2019
Please revert this, it's passing a non django request to |
Exactly. |
I will try to look at this over the weekend. A PR with a failing test that shows the error and fixes it by reverting would be appreciated. Updated contributing guidelines hopefully reinforce why good test coverage is needed: https://django-oauth-toolkit.readthedocs.io/en/latest/contributing.html …On Thu, Mar 12, 2020 at 4:41 AM Nocturn ***@***.***> wrote: Please revert this, it's passing a non django request to authenticate() ! Exactly — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#643 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBHS5ZWFXRMQANG656YCSDRHCN4TANCNFSM4FUA46GQ> . |
indigodomo commented Mar 16, 2020
When is this going to get updated in pypi? The broken version is in there now. I really don't want to have to deploy with my own copy... |
n2ygk commented Mar 16, 2020
You can always use a git reference in your requirements.txt. For example: |
adamchainz commented Mar 16, 2020
Or install from a commit has on github: https://adamj.eu/tech/2019/03/11/pip-install-from-a-git-repository/ |
The [usage documentation](https://django-axes.readthedocs.io/en/latest/3_usage.html) advises to create subclass of `AxesBackend` to ignore the lack of `request` if necessary. I've done this in a project using `django-oauth-toolkit`, which doesn't pass `request` (though it should as per [this PR](django-oauth/django-oauth-toolkit#643)). This meant that the axes.W003 check was being triggered, so I've fixed it to check for subclasses of `AxesBackend` as well as the class itself.
9 participants
Pass request object to authenticate function, request may used in authenticate function.