Opened 2 years ago
Closed 2 years ago
#34066 closed Bug (fixed)
Accessing UserAdmin via to_field leads to link to PasswordResetForm being broken (404)
Reported by: | Simon Kern | Owned by: | Simon Kern |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | auth, password, reset, passwordreset |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
Accessing the UserAdmin
via another model's Admin that has a reference to User
(with to_field set, e.g., to_field="uuid"
) leads to the UserAdmin
being accessed via an url that looks similar to this one:
.../user/22222222-3333-4444-5555-666677778888/change/?_to_field=uuid
However the underlying form looks like this:
Code highlighting:
class UserChangeForm(forms.ModelForm): password = ReadOnlyPasswordHashField( label=_("Password"), help_text=_( "Raw passwords are not stored, so there is no way to see this " "user’s password, but you can change the password using " '<a href="{}">this form</a>.' ), ) ... ... def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) password = self.fields.get("password") if password: password.help_text = password.help_text.format("../password/") ... ...
This results in the link to the PasswordResetForm
being wrong and thus ending up in a 404. If we drop the assumption that UserAdmin is always accessed via its pk, then we're good to go. It's as simple as replacing password.help_text = password.help_text.format("../password/")
with password.help_text = password.help_text.format(f"../../{self.instance.pk}/password/")
I've opened a pull request on GitHub for this Ticket, please see:
PR
Attachments (2)
Change History (19)
comment:1 by , 2 years ago
Description: | modified (diff) |
---|
comment:2 by , 2 years ago
comment:3 by , 2 years ago
Needs tests: | set |
---|
comment:4 by , 2 years ago
Hi Carlton, I'd love to, but I don't find anything that looks similar in the auth_tests. So I am wondering what would be the best approach for this scenario. Could you point me in the right direction?
comment:5 by , 2 years ago
Hi Simon — so it's setting up some models with the relationships at question and then using the test client to go through the flow, hopefully showing the error.
There are various setups in auth_tests/models. You can add new models if needed to demonstrate the issue. Once we have a reproduce we can look at whether it's possible to simplify it, but all changes need regression tests.
comment:6 by , 2 years ago
Triage Stage: | Unreviewed → Accepted |
---|
I'd add a test in UserChangeFormTest
(auth_tests/test_forms.py
) that verifies the password field's help_text. I don't think the test client is needed.
comment:7 by , 2 years ago
Thanks Carlton and Tim, I've added a test in UserChangeFormTest (auth_tests/test_forms.py)
.
comment:8 by , 2 years ago
Description: | modified (diff) |
---|
comment:9 by , 2 years ago
Needs tests: | unset |
---|
By the way, there's no need to attach your patch to the ticket, but you should link to the pull request. I'll do that and uncheck "Needs tests" to put this patch in the review queue. In the future, check "According to the ticket's flags" on this page for the steps to move the ticket forward.
comment:10 by , 2 years ago
Thank you Tim, I was not aware that putting the PR in the initial post is not enough (it was in there since after my first edit). For the future: It's ok for me to uncheck todo flags like "Needs documentation" and "Needs tests" the underlying todo is done?
comment:12 by , 2 years ago
Component: | contrib.auth → contrib.admin |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:13 by , 2 years ago
Component: | contrib.admin → contrib.auth |
---|
Actually sorry it is auth as the form is the problem
comment:14 by , 2 years ago
Patch needs improvement: | set |
---|
comment:15 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:16 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
@felixx Simon's made the requested updates, should be good for you to re-review ☺️
Thanks for the report. Could you add a regression test for this to your patch Simon?