#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 14 months ago.
diff for the test
patch.diff (2.9 KB) - added by Simon Kern 14 months ago.
Patch including tests

Download all attachments as: .zip

Change History (19)

comment:1 Changed 15 months ago by Simon Kern

Description: modified (diff)

comment:2 Changed 15 months ago by Carlton Gibson

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

comment:3 Changed 15 months ago by Carlton Gibson

Needs tests: set

comment:4 Changed 15 months ago by Simon Kern

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 Changed 15 months ago by Carlton Gibson

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 Changed 15 months ago by Tim Graham

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 Changed 14 months ago by Simon Kern

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

Changed 14 months ago by Simon Kern

Attachment: test.diff added

diff for the test

Changed 14 months ago by Simon Kern

Attachment: patch.diff added

Patch including tests

comment:8 Changed 14 months ago by Simon Kern

Description: modified (diff)

comment:9 Changed 14 months ago by Tim Graham

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 Changed 14 months ago by Simon Kern

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 Changed 14 months ago by Tim Graham

Yes

comment:12 Changed 14 months ago by David Sanders

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

comment:13 Changed 14 months ago by David Sanders

Component: contrib.admincontrib.auth

Actually sorry it is auth as the form is the problem

comment:14 Changed 14 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:15 Changed 14 months ago by Simon Kern

Patch needs improvement: unset

comment:16 Changed 14 months ago by David Sanders

Triage Stage: AcceptedReady for checkin

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

comment:17 Changed 14 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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