Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30037 closed New feature (fixed)

RemoteUserBackend should pass request into configure_user()

Reported by: Joshua Cannon Owned by: Joshua Cannon
Component: contrib.auth Version: dev
Severity: Normal Keywords: RemoteUserBackend
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Clients that wish to populate a user's information by subclassing RemoteUserBackend and overriding configure_user find themselves a bit restricted on what they can configure. If a client wants to fill out the user's first name and last name using HTTP headers, there is no easy solution for them.

Instead, the request should also be sent to configure_user (or something similar that wouldn't break compatibility).

Change History (11)

comment:1 by Simon Charette, 5 years ago

Triage Stage: UnreviewedAccepted
Version: 2.1master

Now that authenticate gets passed request I don't see a reason why this can't be done for configure_user.

I guess we could reuse configure_user(request, user) with a deprecation shim that warns about a configure_user(user) signature through inspect reflection just like when the request argument was introduced to authenticate in #25187.

Here's an example:

https://github.com/django/django/commit/4b9330ccc04575f9e5126529ec355a450d12e77c#diff-82ec6fd74a1cab41408fbf5bf0998f58R73

comment:2 by Greg W, 5 years ago

Is this as simple as adding request to configure_user(user) and updating the documentation regarding the change? If so, I can take it.

comment:3 by Simon Charette, 5 years ago

Hello Greg!

The idea is to follow the usual approach used when a function signature is changed in a backward incompatible way. That is:

  1. Before calling the function inspect it to determine whether or not it uses the new or old signature. This can be done using inspect.getcallargs(self.configure_user, request, user) in this case.
  2. If a TypeError is raised that means the function still uses the old signature. That means a RemovedInDjango31Warning warning should be emitted and the function should be called with the old signature, that is without passing request.
  3. In no exceptions are raised that means the function was either not overridden or switched to the new signature so it can be directly called with the new request argument.

Have a look at 4b9330ccc04575f9e5126529ec355a450d12e77c as an example.

This will also require documentation updates and tests as demonstrated in the above example as well.

comment:4 by Joshua Cannon, 5 years ago

Actually, if you don't mind, I'd love to take a stab at it. It seems easy enough as a first PR, and it's something I'm needing anyways :)

comment:5 by Joshua Cannon, 5 years ago

Owner: changed from nobody to Joshua Cannon
Status: newassigned

comment:6 by Joshua Cannon, 5 years ago

Has patch: set

comment:7 by Greg W, 5 years ago

Thanks for the explanation Simon!

Joshua - sure thing, have at it.

comment:8 by Joshua Cannon, 5 years ago

What's the protocol for "bump"-ing a review? Is there a reviewer or reviewers I should add? Or should I comment on the PR?

comment:9 by Simon Charette, 5 years ago

Hello Joshua, your patch is currently in the review list but it might take a few days before fellows get to it because of holidays.

comment:10 by Tim Graham <timograham@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In db1b10ef:

Fixed #30037 -- Added request arg to RemoteUserBackend.configure_user().

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In d17be88a:

Refs #30037 -- Required the RemoteUserBackend.configure_user() to have request as the first positional argument.

Per deprecation timeline.

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