[Django] #29071: Backend authentication does not reset `credentials` on each iteration.
Django
----------------------------------------------+------------------------
Reporter: Sjoerd Job Postmus | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------------+------------------------
For Django 1.11 and Django 2.0, there's a problem with the "graceful
deprecation" layer that's being used, which has already caused some issues
from what I can see. It's regarding the {{{authenticate}}} and
{{{_authenticate_with_backend}}}.
Regarding ticket:28207: there was an issue regarding the {{{credentials}}}
dictionary being polluted inside each loop.
In commit 3008f30f194af386c354416be4c483f0f6b15f33, this was solved by
moving the work inside each iteration into a separate function, getting
the arguments using kwargs-passing-style ({{{**credentials}}}), so that
each iteration would get a clean version of the credentials to work on.
In commit a3ba2662cdaa36183fdfb8a26dfa157e26fca76a, however, that was
completely undone, by passing the original dictionary instead
({{{**credentials}}} was changed into {{{credentials}}}), which again
meant the original dictionary would get mutated.
I think the easiest fix is to add the following line:
{{{
credentials = dict(credentials)
}}}
at the start of {{{_authenticate_with_backend}}}.
This change is no longer necessary for Django 2.1, as this calling-style
is not even supported anymore. However, for both Django 1.11 and Django
2.0 it can cause problems. In our case, it was due to two specific
overrides from different apps: one was still using the old convention
{{{authenticate(self, username=None, password=None, **kwargs)}}}, while
the other was a wrapper around the {{{ModelBackend}}}: as follows:
{{{
class ActiveUserBackend(ModelBackend):
def authenticate(self, *args, **kwargs):
# some code
return super()(*args, **kwargs)
}}}
However, seeing as the first backend caused {{{credentials}}} to be
changed to username/password/request, it was now called as follows:
{{{
authenticate(request, **{'username': 'foo', 'password': 'bar', 'request':
request})
}}}
ticket:28207 would again be fixed I believe by the following simple patch
applied to Django 1.11 and 2.0.
{{{
diff --git a/django/contrib/auth/__init__.py
b/django/contrib/auth/__init__.py
index d96300c503..2d4b525f67 100644
--- a/django/contrib/auth/__init__.py
+++ b/django/contrib/auth/__init__.py
@@ -82,6 +82,8 @@ def authenticate(request=None, **credentials):
def _authenticate_with_backend(backend, backend_path, request,
credentials):
+ # Make sure that changes to `credentials` argument are not propagated
to caller
+ credentials = dict(credentials)
args = (request,)
# Does the backend accept a request argument?
try:
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29071>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Sjoerd Job Postmus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* version: 2.0 => 1.11
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29071#comment:1>
Django
Reporter: Sjoerd Job Postmus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/9641 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29071#comment:2>
Django
Reporter: Sjoerd Job Postmus | Owner: nobody
Component: contrib.auth | Version: 1.11
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"55e16f25e9d2050e95e448f9ab2e4b9fc845a9e5" 55e16f2]:
{{{
#!CommitTicketReference repository=""
revision="55e16f25e9d2050e95e448f9ab2e4b9fc845a9e5"
[2.0.x] Fixed #29071 -- Fixed contrib.auth.authenticate() crash if a
backend doesn't accept a request but a later one does.
Regression in a3ba2662cdaa36183fdfb8a26dfa157e26fca76a.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29071#comment:3>
Django
Reporter: Sjoerd Job Postmus | Owner: nobody
Component: contrib.auth | Version: 1.11
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"4430b83e4bcefbd7da1831df543f711d9da6bc26" 4430b83e]:
{{{
#!CommitTicketReference repository=""
revision="4430b83e4bcefbd7da1831df543f711d9da6bc26"
[1.11.x] Fixed #29071 -- Fixed contrib.auth.authenticate() crash if a
backend doesn't accept a request but a later one does.
Regression in a3ba2662cdaa36183fdfb8a26dfa157e26fca76a.
Backport of 55e16f25e9d2050e95e448f9ab2e4b9fc845a9e5 from stable/2.0.x
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29071#comment:4>
Django
Reporter: Sjoerd Job Postmus | Owner: nobody
Component: contrib.auth | Version: 1.11
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"de59132afcd979a28a9db269b550d4b8356ef325" de59132]:
{{{
#!CommitTicketReference repository=""
revision="de59132afcd979a28a9db269b550d4b8356ef325"
Refs #29071 -- Forwardported 2.0.2/1.11.10 release notes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29071#comment:5>