Opened 5 years ago

Closed 3 years ago

#16919 closed New feature (fixed)

Pass user to set_password_form in GET requests

Reported by: jaimeirurzun Owned by: nobody
Component: contrib.auth Version: 1.3
Severity: Normal Keywords:
Cc: ethan.jucovy@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

SetPasswordForm is being passed None on GET requests even though there is always a user available at that point. This patch passes user, so you can use it in the form constructor for whatever - e.g. populate initial with values that depend on the user involved.

Attachments (2)

setpasswordform.patch (479 bytes) - added by Jaime Irurzun <jaime.irurzun@…> 5 years ago.
setpasswordform_withtests.diff (2.4 KB) - added by Ethan Jucovy 5 years ago.

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by Jaime Irurzun <jaime.irurzun@…>

Attachment: setpasswordform.patch added

comment:1 Changed 5 years ago by Aymeric Augustin

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Technically, the patch works.

However, I can't figure out a practical use case for prepopulating a password field that doesn't have security issues. I'd like to make sure this change doesn't encourage bad practices.

Could you explain what you're trying to achieve?

comment:2 Changed 5 years ago by jaimeirurzun

I was also concerned about the security implications that this patch might have when I wrote it, but given this only applies to the case in which the token has already been validated, I can't think of any security hole.

Basically I have a custom SetPasswordForm in which I give the user the opportunity to update a few fields from his profile that will be used in the password reset logic, so I want to fill the initial values with his current data, for which I need the user object.

comment:3 Changed 5 years ago by Aymeric Augustin

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:4 Changed 5 years ago by Ethan Jucovy

Cc: ethan.jucovy@… added

I have another use case for this: rendering the user's name in the registration/password_reset_confirm.html template.

Currently the password_reset_confirm view does not provide "user" as a template context variable, nor even "uidb36" and "token". Since the form also doesn't have the user object stored on a GET request, this means that there's no way for the template to say "{% if validlink %} Hello, {{ user.username }} -- reset your password here {% endif %}" -- short of forking the view, or some pretty hacky middleware that re-parses the request URL and re-fetches the user from the given uid+token.

If this patch were accepted, that would be possible, like so: "{% if validlink %} Hello, {{ form.user.username }} -- reset your password here {% endif %}"

I see that the "needs_tests" flag is set on this ticket .. what sort of test would be required for this patch to be merged?

Last edited 5 years ago by Ethan Jucovy (previous) (diff)

Changed 5 years ago by Ethan Jucovy

comment:5 Changed 5 years ago by Ethan Jucovy

Needs tests: unset

I've attached a new version of the patch including auth.views tests that double as demonstration of a use case for this behavior.

comment:6 Changed 3 years ago by anonymous

Another use case for this:

I can add "security question/answer" that user picks when registering and extend SetPasswordForm with CharField labeled with question user picked.

comment:7 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 1285ca67eba96045b4f6fe6f5c7fd6570571f1bd:

Fixed #16919 -- Passed user to set_password_form in GET requests.

Thanks Jaime Irurzun for the report and initial patch and
ejucovy for the test.

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