fix(accounts): migrate no longer overrides the change-password permission

SHOW_PROFILE_EDIT_PASSWORD was replaced in 2020 by the change_password
permission, settable per user or group. A post_migrate hook applied the
old setting to all users so existing installs kept their behavior, and
asked to remove the setting afterwards; but the settings template still
set it to True. Every migrate, so every upgrade, therefore granted the
permission to all users again and undid an admin's per-user or
per-group choice (with False it would have revoked it from everyone).

- Migration accounts.0008 applies the setting one last time, if a
  settings.py still has it; the post_migrate hook is gone.
- New users get the permission when they are created (any path,
  including LDAP); the admin user form ticks it and saves the admin's
  choice afterwards.
- The template no longer sets SHOW_PROFILE_EDIT_PASSWORD; a check
  (accounts.W001) asks to remove it from existing settings, and the
  README upgrade notes say so.

The test that removes the permission and runs migrate fails on the
previous code. An upgrade from master with users was simulated: the
setting is applied once, and a permission removed afterwards stays
removed through the next migrate.

README.md

@@ -405,6 +405,8 @@ Upgrading from Django 4.2, edit `webvirtcloud/settings.py`:
 
 Logging out now needs a POST; links or scripts that call `/accounts/logout/` with GET get 405.
 
+`SHOW_PROFILE_EDIT_PASSWORD` is applied to all users one last time by migration `accounts.0008` and then ignored; remove it from `settings.py` (system check `accounts.W001`). Whether a user may change their own password is the "Can change password" permission, per user or group; new users get it by default.
+
 Before running migrations, add `accounts.middleware.ForcePasswordChangeMiddleware`
 to `MIDDLEWARE` in your existing `webvirtcloud/settings.py`, after
 `django.contrib.auth.middleware.AuthenticationMiddleware` (and the OTP/login-required

accounts/apps.py

@@ -23,30 +23,16 @@ def _migrated(state_apps, model, field=None):
     return field is None or any(f.name == field for f in fields)
 
 
-def apply_change_password(sender, **kwargs):
-    """
-    Apply new change_password permission for all users
-    Depending on settings SHOW_PROFILE_EDIT_PASSWORD
-    """
-    from django.conf import settings
-    from django.contrib.auth.models import Permission, User
-
-    if not (_migrated(kwargs.get("apps"), "auth.Permission") and _migrated(kwargs.get("apps"), "auth.User")):
+def grant_change_password(sender, instance, created, raw=False, **kwargs):
+    """New users may change their own password unless an admin takes the
+    permission away; the user form saves its own choice after this."""
+    if not created or raw:
         return
-    if hasattr(settings, "SHOW_PROFILE_EDIT_PASSWORD"):
-        print("\033[1m! \033[92mSHOW_PROFILE_EDIT_PASSWORD is found inside settings.py\033[0m")
-        print("\033[1m* \033[92mApplying permission can_change_password for all users\033[0m")
-        users = User.objects.all()
-        permission = Permission.objects.get(codename="change_password")
-        if settings.SHOW_PROFILE_EDIT_PASSWORD:
-            print("\033[1m! \033[91mWarning!!! Setting to True for all users\033[0m")
-            for user in users:
-                user.user_permissions.add(permission)
-        else:
-            print("\033[1m* \033[91mWarning!!! Setting to False for all users\033[0m")
-            for user in users:
-                user.user_permissions.remove(permission)
-        print("\033[1m! Don`t forget to remove the option from settings.py\033[0m")
+    from django.contrib.auth.models import Permission
+
+    permission = Permission.objects.filter(content_type__app_label="accounts", codename="change_password").first()
+    if permission is not None:  # created by post_migrate on a fresh database
+        instance.user_permissions.add(permission)
 
 
 def _store_generated_password(password):
@@ -127,5 +113,8 @@ class AccountsConfig(AppConfig):
 
     def ready(self):
         post_migrate.connect(create_admin, sender=self)
-        post_migrate.connect(apply_change_password, sender=self)
+        from django.conf import settings
+        from django.db.models.signals import post_save
+
+        post_save.connect(grant_change_password, sender=settings.AUTH_USER_MODEL)
         from . import checks  # noqa: F401

accounts/checks.py

@@ -1,5 +1,5 @@
 from django.conf import settings
-from django.core.checks import Error, Tags, register
+from django.core.checks import Error, Tags, Warning, register
 
 
 @register(Tags.security)
@@ -36,3 +36,18 @@ def login_required_middleware_check(app_configs, **kwargs):
             )
         ]
     return []
+
+
+@register()
+def obsolete_settings_check(app_configs, **kwargs):
+    if hasattr(settings, "SHOW_PROFILE_EDIT_PASSWORD"):
+        return [
+            Warning(
+                "SHOW_PROFILE_EDIT_PASSWORD is no longer used.",
+                hint="Remove it from webvirtcloud/settings.py. Whether a user may change their own "
+                "password is the 'Can change password' permission, per user or group.",
+                id="accounts.W001",
+            )
+        ]
+    return []
+

accounts/migrations/0008_apply_show_profile_edit_password_once.py

@@ -0,0 +1,31 @@
+from django.conf import settings
+from django.db import migrations
+
+
+def apply_setting_once(apps, schema_editor):
+    """SHOW_PROFILE_EDIT_PASSWORD was replaced by the change_password
+    permission, but its value was applied to every user on each migrate,
+    undoing per-user and per-group choices. Apply it one last time."""
+    if not hasattr(settings, "SHOW_PROFILE_EDIT_PASSWORD"):
+        return
+    alias = schema_editor.connection.alias
+    Permission = apps.get_model("auth", "Permission")
+    User = apps.get_model(*settings.AUTH_USER_MODEL.split("."))
+    permission = Permission.objects.using(alias).filter(
+        content_type__app_label="accounts", codename="change_password"
+    ).first()
+    if permission is None:  # fresh database: no users and no permissions yet
+        return
+    for user in User.objects.using(alias).all():
+        if settings.SHOW_PROFILE_EDIT_PASSWORD:
+            user.user_permissions.add(permission)
+        else:
+            user.user_permissions.remove(permission)
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("accounts", "0007_userattributes_must_change_password"),
+        ("auth", "0012_alter_user_first_name_max_length"),
+    ]
+    operations = [migrations.RunPython(apply_setting_once, migrations.RunPython.noop)]

accounts/test_change_password_permission.py

@@ -0,0 +1,80 @@
+"""Who may change their own password is the change_password permission: new
+users get it, admins may take it away, and later migrate runs keep their choice."""
+
+import importlib
+from unittest.mock import MagicMock
+
+from django.apps import apps
+from django.contrib.auth import get_user_model
+from django.contrib.auth.models import Permission
+from django.core.management import call_command
+from django.test import TestCase, override_settings
+from django.urls import reverse
+
+from admin.forms import UserCreateForm
+
+migration = importlib.import_module("accounts.migrations.0008_apply_show_profile_edit_password_once")
+
+
+def permission():
+    return Permission.objects.get(content_type__app_label="accounts", codename="change_password")
+
+
+def can_change(user):
+    return get_user_model().objects.get(pk=user.pk).has_perm("accounts.change_password")
+
+
+class ChangePasswordPermissionTestCase(TestCase):
+    def test_new_users_may_change_their_password(self):
+        self.assertTrue(can_change(get_user_model().objects.create_user("new", password="pw")))
+
+    def test_user_without_the_permission_cannot_open_the_page(self):
+        user = get_user_model().objects.create_user("limited", password="pw")
+        user.user_permissions.remove(permission())
+        self.client.force_login(user)
+        self.assertEqual(self.client.get(reverse("accounts:change_password")).status_code, 403)
+
+    def test_create_form_ticks_it_and_the_admin_choice_wins(self):
+        self.assertIn(permission(), UserCreateForm().fields["user_permissions"].initial)
+        form = UserCreateForm(data={"username": "unticked", "password": "pw", "is_active": True})
+        self.assertTrue(form.is_valid(), form.errors)
+        self.assertFalse(can_change(form.save()))
+
+    @override_settings(SHOW_PROFILE_EDIT_PASSWORD=True)
+    def test_migrate_keeps_a_removed_permission(self):
+        user = get_user_model().objects.create_user("kept", password="pw")
+        user.user_permissions.remove(permission())
+
+        call_command("migrate", verbosity=0)
+
+        self.assertFalse(can_change(user))
+
+
+class ApplySettingOnceTestCase(TestCase):
+    def run_migration(self):
+        migration.apply_setting_once(apps, MagicMock(connection=MagicMock(alias="default")))
+
+    def setUp(self):
+        self.with_perm = get_user_model().objects.create_user("with", password="pw")
+        self.without = get_user_model().objects.create_user("without", password="pw")
+        self.without.user_permissions.remove(permission())
+
+    @override_settings(SHOW_PROFILE_EDIT_PASSWORD=True)
+    def test_true_grants_to_everyone(self):
+        self.run_migration()
+        self.assertTrue(can_change(self.with_perm) and can_change(self.without))
+
+    @override_settings(SHOW_PROFILE_EDIT_PASSWORD=False)
+    def test_false_revokes_from_everyone(self):
+        self.run_migration()
+        self.assertFalse(can_change(self.with_perm) or can_change(self.without))
+
+    def test_without_the_setting_nothing_changes(self):
+        with self.settings():
+            from django.conf import settings
+
+            if hasattr(settings, "SHOW_PROFILE_EDIT_PASSWORD"):
+                del settings.SHOW_PROFILE_EDIT_PASSWORD
+            self.run_migration()
+        self.assertTrue(can_change(self.with_perm))
+        self.assertFalse(can_change(self.without))

admin/forms.py

@@ -95,6 +95,13 @@ class UserForm(forms.ModelForm):
 class UserCreateForm(UserForm):
     password = forms.CharField(widget=forms.PasswordInput)
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        # New users may change their own password unless unticked here
+        self.fields["user_permissions"].initial = Permission.objects.filter(
+            content_type__app_label="accounts", codename="change_password"
+        )
+
     class Meta:
         model = User
         fields = [

webvirtcloud/settings.py.template

@@ -288,7 +288,6 @@ ALLOW_EMPTY_PASSWORD = False
 NEW_USER_DEFAULT_INSTANCES = []
 # Days the owners of a VM that vanished from its compute are kept, in case it comes back
 INSTANCE_OWNERSHIP_RETENTION_DAYS = 30
-SHOW_PROFILE_EDIT_PASSWORD = True
 
 
 # OTP Config

webvirtcloud/test_authorization_matrix.py

@@ -35,6 +35,7 @@ ALLOWED_FOR_ANY_USER = {
     "accounts:email_otp": "part of the login flow (its own issues are tracked separately)",
     "accounts:logout": "logout",
     "accounts:profile": "the user's own profile",
+    "accounts:change_password": "the user's own password; new users get the permission by default",
     "accounts:ssh_key_create": "the user's own SSH keys",
     "set_language": "UI language switch",
     "rest_framework:login": "DRF browsable API login",