New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #34429 -- Allowed setting unusable passwords for users in the auth forms. #16942
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, I've made some comments.
Please can you update the PR title to match the format in the contributing guidelines:
Fixed #34429 -- Allowed setting unusable password via admin UI.
This will also need documentation changes and release notes adding.
When you've done this you can uncheck Needs documentation
and Patch needs improvement
on ticket-34429.
django/contrib/admin/templates/admin/auth/user/change_password.html
Outdated
Show resolved
Hide resolved
django/contrib/admin/templates/admin/auth/user/change_password.html
Outdated
Show resolved
Hide resolved
@ngnpope Thank you for the suggestions! Let me briefly comment and ask one more question:
Will, of course, also make the other changes. |
One more thing to add: I did use |
I have a remark about the wording
I think people are going to be confused about what the difference is between this and the active flag on the edit screen:
Nick I know you requested the addition of ? and I'm not saying it's good or bad but it is now inconsistent with other checkboxes (I'd just leave it off personally) 😛 |
Can anything be done to stop the jumping checkbox? 🤔 moving-checkbox.mov |
Anyhoo nice work so far fsbraun! 🏆 |
@shangxiao Thanks very much for the comments:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check locally, but:
I have a feeling that we should add aria-describedby="id_unusable_warning"
when the warning is shown. However I'm not sure how that would interact with #16920 - there is still some ongoing discussion here around what to do when aria-describedby
has multiple IDs set.
Just from a usability point of view as well, I am concerned that it's not clear to the user that this isn't reversible - that they cannot simply recheck the box after submission to revert to the user's old password.
<li class="warning"> | ||
{% blocktranslate %} | ||
Upon submission an unusable password will be set which prevents the user from logging in | ||
using his username and password. Potential other means of authentication remain unaffected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be gender neutral "their" instead of "his" like in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! I try to pay attention to those things but they still slip through 🤦♂️
Thanks, everybody, for the constructive feedback. I have updated the PR:
|
Is there anything I can do to move this forward? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fsbraun Thanks 👍 I left initial comments.
According to the ticket description we should have the same feature when adding a new user:
<ul id="id_unusable_warning" class="messagelist"> | ||
<li class="warning"> | ||
{% blocktranslate %} | ||
Upon submission an unusable password will be set which prevents the user from logging in | ||
using their username and password. The user's current password will be lost. Potential other | ||
means of authentication remain unaffected. | ||
{% endblocktranslate %} | ||
</li> | ||
</ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct indentation.
django/contrib/auth/forms.py
Outdated
usable_password = forms.BooleanField( | ||
label=_("Allow login with username and password"), | ||
required=False, | ||
initial=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be initialized with the has_usable_password()
?
django/contrib/auth/forms.py
Outdated
@@ -469,42 +469,70 @@ class AdminPasswordChangeForm(forms.Form): | |||
attrs={"autocomplete": "new-password", "autofocus": True} | |||
), | |||
strip=False, | |||
required=False, # Can be empty if usable_password is False | |||
validators=[], # Password validation happens in the check_password1() method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? Looks unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's both needed and related. The password validation can only happen in the form's clean method, since a password is only required or needs to be validated if has_usable_password
is True
.
tests/auth_tests/test_forms.py
Outdated
@@ -1371,3 +1402,20 @@ def test_html_autocomplete_attributes(self): | |||
self.assertEqual( | |||
form.fields[field_name].widget.attrs["autocomplete"], autocomplete | |||
) | |||
|
|||
def test_unusable_password(self): | |||
"""Tests if deselecting usable password sets an unusable password""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add more explanation, docstrings should state the expected behavior and omit prefixes like "Tests if" since all tests test things. This guideline is from Python coding style.
tests/auth_tests/test_forms.py
Outdated
{ | ||
"password1": "", | ||
"password2": "test", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be single-lined.
document.getElementById("id_unusable_warning").hidden = this.checked; | ||
}); | ||
usable_password_field.dispatchEvent(new Event("change")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a Selenium test to check that layout is changing.
<label class="vCheckboxLabel" for="id_usable_password">{{ form.usable_password.label }}</label> | ||
</div> | ||
{% if form.usable_password.help_text %} | ||
<div class="help"{% if form.usable_password.id_for_label %} id="{{ form.usable_password.id_for_label }}_helptext">{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
">" should be outside of {% if ... %}...{% endif %}
/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! 🤦♂️
@felixxm Thank you for your detailed feedback. Indeed, I overlooked that this should also apply to the user add form. This is fixed now:
Please let me know if there is anything else I can do! |
@felixxm Is there anything I can do to get this PR over the finish line? Would you like to take a final look? |
I started reviewing this PR and the first think I thought was exactly this. Was there a consideration of being more "honest" with the admin user operating the form, something like: Label: "Unset the password" |
@nessita Thanks for taking the time to look through this. Indeed, the wording has been discussed here (and I also tried to get some input here). I did consider "Set unusable password" but this would require the user to understand the implications, namely, that the user cannot log in using a username and password. That's the root of the wording. IMHO, "unsetting" a password could be confused with "set no password". Also, even with an unusable password, you can potentially still log in with a different authentication backend. Current text: Help text: Warning: I am not hung up on this, but also would like it to be as clear as possible. What's your call? |
My main concern with this sentence is that, given that Django allows the User model to be customized so that the Also, considering that this flow occurs within the context of the admin, I would suggest to be more direct with the admin user and explicitly saying that the password will be set and it will be unususable. Otherwise I can see admin user scratching their head wondering what's the difference with marking a user as inactive.
Same concern about saying
I would omit "upon submission", I think this is implied from every help text. With that in mind, I would suggest: An unusable password will be set which prevents the user from logging in until a new password is set. The user's current password will be lost, though other means of authentication (if configured) remain unaffected.
I proposed a few alternatives above. The summary would be:
|
@nessita Indeed, I initially had the password fields enabled by default. @felixxm requested it differently here: #16942 (comment) I can revert the change. A third option might be to change the help text if the user does not have a usable password to something like: "Currently this user does not have a usable password. Enable this to set a password for this user". An update for wording is coming up. |
Oh, I see. I can understand the goal of having the form properly initialized, but I guess is weird when coming from "no password set". I guess we can proceed as is and then evaluate potential/future ticket reports.
No need for now.
I like this proposal. May I suggest: The current password is not usable, check this option to set a new password for user authentication. |
Not being sure is completely fine! Also do note that you need to update your The big advantage for using rebasing (IMHO) is that your commits always end up at the "top of the stack of commits". For example,
Doing this consistently and often, ensures that your branch history looks like this:
When newer revnos are landed in main, say revnos
To the contrary, when doing merges from main into your branch, your history starts looking like this:
The difference is that I can go on and on about this, but I figured you could continue the investigation on your own with these pointers.
I agree. I have separated and simplified the test in PR #17729. I removed the testing on both values of password1 because as far as I understood, the fact that password1 would be empty or not was irrelevant for the call:
If I'm missing something here, please let me know.
What I did was doing multiple interactive rebases. An interactive rebase is a rebase as described above but before replaying the commits from your branch, you get a screen to "interact" with git and instruct it to do things, perhaps differently, with the commits that is about to replay. Specifically for this PR, which had 76 commits before my work, the rebasing was very painful and slow. It took me almost 2 hours to resolve all the conflicts that were arising after every commit from the 76 pending ones was applied. I think I got the squash and rebase correct. Will push the result in a few. |
One thing that I noticed while rebasing and fixing conflicts is that the adding in various places (tests) of |
If you wanna give it a shot to splitting the Mixin into its own commit, this article may help when it comes to finding the necessary git magic incantations. |
@nessita Is this what you expected? |
4889f21
to
4083adb
Compare
Yes, quite close! Could you please adjust the Mixin docstring to not mention the unusable password bit in the first commit? That piece should only be added in the second commit. |
@nessita There you go!! 😎😃 Thanks to @thibaudcolas for his patient introduction to interactive rebase. |
ddf6c63
to
6b1e76e
Compare
22ec146
to
61dc326
Compare
Hey @fsbraun, thanks for your patience. Last week and today I've been working on final tweaks for this PR. I have reviewed logic and tests in detail, making some adjustments to the UI and UX, and adding more tests to ensure proper coverage of changes. Tomorrow I'll give the docs another pass and hopefully I will approve and merge. 🤞 |
…n logic in auth forms. Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
61dc326
to
53c180d
Compare
I believe this is ready for merge, will do so later today after the CI runs complete. |
…uth forms. Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
53c180d
to
e626716
Compare
ticket-34429
Feature added
This PR introduces a new functionality to the admin change password form: the checkbox (on by default) "Allow login"
If unchecked, the password field disappears and the form sets an unusable password upon submission. The user is warned before.
Tests
Existing tests have been changed to contain the new form field.
A new test was added to test the functionality.
Other effects
This PR adds three new strings which will have to be translated.