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 Simon Kern)

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)

test.diff (2.2 KB ) - added by Simon Kern 2 years ago.
diff for the test
patch.diff (2.9 KB ) - added by Simon Kern 2 years ago.
Patch including tests

Download all attachments as: .zip

Change History (19)

comment:1 by Simon Kern, 2 years ago

Description: modified (diff)

comment:2 by Carlton Gibson, 2 years ago

Thanks for the report. Could you add a regression test for this to your patch Simon?

comment:3 by Carlton Gibson, 2 years ago

Needs tests: set

comment:4 by Simon Kern, 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 Carlton Gibson, 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 Tim Graham, 2 years ago

Triage Stage: UnreviewedAccepted

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 Simon Kern, 2 years ago

Thanks Carlton and Tim, I've added a test in UserChangeFormTest (auth_tests/test_forms.py).

by Simon Kern, 2 years ago

Attachment: test.diff added

diff for the test

by Simon Kern, 2 years ago

Attachment: patch.diff added

Patch including tests

comment:8 by Simon Kern, 2 years ago

Description: modified (diff)

comment:9 by Tim Graham, 2 years ago

Needs tests: unset

PR

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 Simon Kern, 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:11 by Tim Graham, 2 years ago

Yes

comment:12 by David Sanders, 2 years ago

Component: contrib.authcontrib.admin
Owner: changed from nobody to Simon Kern
Status: newassigned

comment:13 by David Sanders, 2 years ago

Component: contrib.admincontrib.auth

Actually sorry it is auth as the form is the problem

comment:14 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:15 by Simon Kern, 2 years ago

Patch needs improvement: unset

comment:16 by David Sanders, 2 years ago

Triage Stage: AcceptedReady for checkin

@felixx Simon's made the requested updates, should be good for you to re-review ☺️

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In de2c2127:

Fixed #34066 -- Fixed link to password reset view in UserChangeForm.password's help text when using to_field.

Co-Authored-By: David Sanders <shang.xiao.sanders@…>
Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@…>

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