Opened 2 months ago

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

Download all attachments as: .zip

Change History (19)

comment:1 Changed 2 months ago by Simon Kern

Description: modified (diff)

comment:2 Changed 2 months ago by Carlton Gibson

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

comment:3 Changed 2 months ago by Carlton Gibson

Needs tests: set

comment:4 Changed 2 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 2 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 2 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 2 months ago by Simon Kern

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

Changed 2 months ago by Simon Kern

Attachment: test.diff added

diff for the test

Changed 2 months ago by Simon Kern

Attachment: patch.diff added

Patch including tests

comment:8 Changed 2 months ago by Simon Kern

Description: modified (diff)

comment:9 Changed 2 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 2 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 2 months ago by Tim Graham

Yes

comment:12 Changed 2 months ago by David Sanders

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

comment:13 Changed 2 months ago by David Sanders

Component: contrib.admincontrib.auth

Actually sorry it is auth as the form is the problem

comment:14 Changed 2 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:15 Changed 2 months ago by Simon Kern

Patch needs improvement: unset

comment:16 Changed 2 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 6 weeks 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