Opened 23 months ago

Closed 23 months ago

Last modified 23 months ago

#29071 closed Bug (fixed)

Backend authentication does not reset `credentials` on each iteration.

Reported by: Sjoerd Job Postmus Owned by: nobody
Component: contrib.auth Version: 1.11
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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:

Change History (5)

comment:1 Changed 23 months ago by Tim Graham

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Version: 2.01.11

comment:2 Changed 23 months ago by Tim Graham

Has patch: set

comment:3 Changed 23 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 55e16f2:

[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.

comment:4 Changed 23 months ago by Tim Graham <timograham@…>

In 4430b83e:

[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

comment:5 Changed 23 months ago by Tim Graham <timograham@…>

In de59132:

Refs #29071 -- Forwardported 2.0.2/1.11.10 release notes.

Note: See TracTickets for help on using tickets.
Back to Top