Fixed #34429 -- Allowed setting unusable passwords for users in the auth forms. #16942

fsbraun · 2023-06-05T14:54:52Z

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.

github-actions

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 ⛵️!

ngnpope

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/static/admin/js/change_password_form.js

django/contrib/auth/forms.py

django/contrib/admin/templates/admin/auth/user/change_password.html

fsbraun · 2023-06-05T16:52:02Z

@ngnpope Thank you for the suggestions! Let me briefly comment and ask one more question:

Probably we do not need to wait for DOMContentLoaded since the script is loaded as async. It would probably miss the event (which might be fired before it is loaded). I'll remove the event in total.
Using the hidden property might clash with custom css which might set something like .form-row { display: block } effectively disabling the hidden property. OK, if I stay with style.display? I believe it is more robust.

Will, of course, also make the other changes.

tests/auth_tests/test_forms.py

fsbraun · 2023-06-05T18:02:16Z

One more thing to add: I did use function () { ... } instead of () => { ... } since otherwise this is not defined properly.

django/contrib/auth/forms.py

shangxiao · 2023-06-06T03:45:09Z

I have a remark about the wording

Allow login?
If unchecked, the user will not be able to log in.

I think people are going to be confused about what the difference is between this and the active flag on the edit screen:

Active.
Designates whether this user should be treated as active. Unselect this instead of deleting accounts.

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) 😛

shangxiao · 2023-06-06T03:53:01Z

Can anything be done to stop the jumping checkbox? 🤔

moving-checkbox.mov

shangxiao · 2023-06-06T03:55:39Z

Anyhoo nice work so far fsbraun! 🏆

fsbraun · 2023-06-06T05:44:39Z

@shangxiao Thanks very much for the comments:

The jumping can be stopped by putting the warning below the checkbox and above the submit button
Thanks for pointing out the "active" alternative. Really, an unusable password only prevents login using username and password (as @xi probably also pointed out). I tried to better name those.

knyghty

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.

xi · 2023-06-12T07:31:33Z

django/contrib/admin/templates/admin/auth/user/change_password.html

+  <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.


should be gender neutral "their" instead of "his" like in other places

Right! I try to pay attention to those things but they still slip through 🤦‍♂️

fsbraun · 2023-06-12T09:28:38Z

Thanks, everybody, for the constructive feedback. I have updated the PR:

@knyghty The help text now reads

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.
@xi This is gender-neural and clarifies that the current password will be lost
@knyghty I have moved the warning into the help_text div. This implies that once the form field sets the aria-describedby property, it will include the help text and the warning. No need for two id to be provided to aria-describedby (which would be possible as far as I understand the docs).

fsbraun · 2023-07-04T15:02:38Z

Is there anything I can do to move this forward?

felixxm

@fsbraun Thanks 👍 I left initial comments.

According to the ticket description we should have the same feature when adding a new user:

felixxm · 2023-07-12T11:36:19Z

django/contrib/admin/templates/admin/auth/user/change_password.html

+    <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>


Please correct indentation.

felixxm · 2023-07-12T11:48:01Z

django/contrib/auth/forms.py

+    usable_password = forms.BooleanField(
+        label=_("Allow login with username and password"),
+        required=False,
+        initial=True,


Shouldn't this be initialized with the has_usable_password()?

felixxm · 2023-07-12T11:49:36Z

django/contrib/auth/forms.py

@@ -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


Is this needed? Looks unrelated.

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.

docs/topics/auth/default.txt

felixxm · 2023-07-12T11:51:31Z

tests/auth_tests/test_forms.py

@@ -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"""


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.

felixxm · 2023-07-12T11:51:53Z

tests/auth_tests/test_forms.py

+            {
+                "password1": "",
+                "password2": "test",
+            },


This can be single-lined.

felixxm · 2023-07-12T11:52:41Z

django/contrib/admin/static/admin/js/change_password_form.js

+            document.getElementById("id_unusable_warning").hidden = this.checked;
+        });
+        usable_password_field.dispatchEvent(new Event("change"));
+    }


I'd add a Selenium test to check that layout is changing.

felixxm · 2023-07-12T12:02:37Z

django/contrib/admin/templates/admin/auth/user/change_password.html

+    <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 %}


">" should be outside of {% if ... %}...{% endif %}/

Indeed! 🤦‍♂️

tests/auth_tests/test_forms.py

fsbraun · 2023-07-25T13:37:53Z

@felixxm Thank you for your detailed feedback. Indeed, I overlooked that this should also apply to the user add form. This is fixed now:

Since the change password and user add form are quite different (form with template and an admin form) I have refactored the HTML to avoid repetition and make it usable for both forms (using the <template> tag).
Indentation has been fixed
The initial value of usable_password now is has_usable_password()
I annotated the docs with versionchanged
Updated the docstrings for the tests
Added a Selenium test
Fixed the HTML layout issue (wrongly placed >)

Please let me know if there is anything else I can do!

fsbraun · 2023-09-12T16:49:49Z

@felixxm Is there anything I can do to get this PR over the finish line? Would you like to take a final look?

nessita · 2023-09-13T12:37:08Z

I think people are going to be confused about what the difference is between this and the active flag on the edit screen:

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"
Help text: "Unsetting the password will render the login credentials unusable. The User will no longer be able to log in until a new password is set."

fsbraun · 2023-09-13T13:03:59Z

@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:
Allow login with username and password

Help text:
If unchecked, the user will have an unusable password and not be able to log in using their username and any password. (I just realized this had a spelling error)

Warning:
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.

I am not hung up on this, but also would like it to be as clear as possible. What's your call?

nessita · 2023-09-13T14:04:00Z

@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: Allow login with username and password

My main concern with this sentence is that, given that Django allows the User model to be customized so that the USERNAME_FIELD could be something other than username, like email or something system specific, saying username here would not be a accurate for those cases.

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.

Help text: If unchecked, the user will have an unusable password and not be able to log in using their username and any password. (I just realized this had a spelling error)

Same concern about saying username, we could say "credentials" as I suggested?

Warning: 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.

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 am not hung up on this, but also would like it to be as clear as possible. What's your call?

I proposed a few alternatives above. The summary would be:

not saying "username and password" explicitly, and
being clear about how this action is different from making a user inactive

nessita · 2023-09-13T14:33:36Z

There is also a potentially awkward user flow when:

A user tesuser1 has an unusable password
Later on, the admin user goes to this user's change form to set a valid password:
The admin user is presented with a "change password" form where no password fields are available:

I wonder if we should enable the password fields when going from "change the password using "?

fsbraun · 2023-09-13T17:02:52Z

@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.

nessita · 2023-09-13T20:23:48Z

@nessita Indeed, I initially had the password fields enabled by default. @felixxm requested it differently here: #16942 (comment)

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.

I can revert the change.

No need for now.

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".

I like this proposal. May I suggest:

The current password is not usable, check this option to set a new password for user authentication.

nessita · 2024-01-12T20:29:06Z

@nessita OK! I just feel not sure how to do that exactly. It might be my limited mastery of git that scares me. (I just clicked update branch on my fork in GitHub, for example. Next time I will run git rebase main.)

Not being sure is completely fine! Also do note that you need to update your main before doing the rebase.

The big advantage for using rebasing (IMHO) is that your commits always end up at the "top of the stack of commits". For example, git rebase <ref> means (I'll handwave a bit, there are tons of articles out there explaining this with more technically correct terms):

all your commits since <ref> are put aside
repo is updated with latest changes for <ref> (this is why you need to update main before rebasing)
the commits are re-played on top of the latest changes

Doing this consistently and often, ensures that your branch history looks like this:

main-hash-1
main-hash-2
...
main-hash-N
your-branch-hash-1
your-branch-hash-2
...
your-branch-hash-M

When newer revnos are landed in main, say revnos main-hash-N+1 and main-hash-N+2, after a rebase your branch looks like this:

main-hash-1
main-hash-2
...
main-hash-N
main-hash-N+1
main-hash-N+2
your-branch-hash-1
your-branch-hash-2
...
your-branch-hash-M

To the contrary, when doing merges from main into your branch, your history starts looking like this:

main-hash-1
main-hash-2
...
main-hash-N
your-branch-hash-1
your-branch-hash-2
...
your-branch-hash-M
your-branch-hash-M+1

The difference is that your-branch-hash-M+1 is a single merge commit of both revnos from main ( main-hash-N+1 and main-hash-N+2 merged into a single commit). So your branch history incorporates newer changes from main but starts diverging quite considerably.

I can go on and on about this, but I figured you could continue the investigation on your own with these pointers.

Isolating the changes to the tests should be easy. There's probably some thinking involved when separating out the mixin.

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:

password_validation.validate_password(password2, self.user)

If I'm missing something here, please let me know.

Out of curiosity, how do you do this? You would need to "undo" all commits while keeping the changes and then redoing the changes in a new order.

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.

nessita · 2024-01-12T20:39:27Z

One thing that I noticed while rebasing and fixing conflicts is that the adding in various places (tests) of "usable_password": "true" to form data could be removed, since that's the default and I don't think is necessary to set it explicitly in tests that are not relevant to this feature. Let me know if there is a reason to add it!

nessita · 2024-01-12T20:44:39Z

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.

django/contrib/auth/forms.py

tests/admin_views/test_password_form.py

docs/topics/auth/default.txt

tests/admin_views/test_password_form.py

tests/auth_tests/test_views.py

fsbraun · 2024-01-12T22:27:55Z

@nessita Is this what you expected?

nessita · 2024-01-15T19:34:33Z

@nessita Is this what you expected?

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.

fsbraun · 2024-01-23T15:48:49Z

@nessita There you go!! 😎😃

Thanks to @thibaudcolas for his patient introduction to interactive rebase.

nessita · 2024-02-20T02:59:33Z

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>

nessita · 2024-02-20T14:58:42Z

I believe this is ready for merge, will do so later today after the CI runs complete.

…s tests.

…uth forms. Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>

fsbraun changed the title ~~Ticker 34429: Add checkbox "Allow login" to admin password change form.~~ Ticket 34429: Add checkbox "Allow login" to admin password change form. Jun 5, 2023

fsbraun changed the title ~~Ticket 34429: Add checkbox "Allow login" to admin password change form.~~ Ticket 34429: Allow to set unusable password via admin UI Jun 5, 2023

github-actions bot reviewed Jun 5, 2023

View reviewed changes

ngnpope suggested changes Jun 5, 2023

View reviewed changes

fsbraun changed the title ~~Ticket 34429: Allow to set unusable password via admin UI~~ Fixed #34429 -- Allowed setting unusable password via admin UI. Jun 5, 2023

ngnpope suggested changes Jun 5, 2023

View reviewed changes

tests/auth_tests/test_forms.py Outdated Show resolved Hide resolved

tests/auth_tests/test_forms.py Outdated Show resolved Hide resolved

xi reviewed Jun 5, 2023

View reviewed changes

django/contrib/auth/forms.py Outdated Show resolved Hide resolved

fsbraun requested review from ngnpope and xi June 6, 2023 05:53

felixxm requested review from a team and removed request for xi June 6, 2023 09:27

knyghty reviewed Jun 8, 2023

View reviewed changes

xi reviewed Jun 12, 2023

View reviewed changes

felixxm reviewed Jul 12, 2023

View reviewed changes

nessita force-pushed the ticket_34429 branch from dd3d08c to 4c9ca1e Compare January 12, 2024 20:41