From 99dabbdcec66f14242191835efe56ff7267a4196 Mon Sep 17 00:00:00 2001 From: catborise Date: Sun, 4 Oct 2026 20:26:53 +0300 Subject: [PATCH] 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 | 2 + accounts/apps.py | 37 +++------ accounts/checks.py | 17 +++- ...8_apply_show_profile_edit_password_once.py | 31 +++++++ accounts/test_change_password_permission.py | 80 +++++++++++++++++++ admin/forms.py | 7 ++ webvirtcloud/settings.py.template | 1 - webvirtcloud/test_authorization_matrix.py | 1 + 8 files changed, 150 insertions(+), 26 deletions(-) create mode 100644 accounts/migrations/0008_apply_show_profile_edit_password_once.py create mode 100644 accounts/test_change_password_permission.py diff --git a/README.md b/README.md index 0d731e9d..700ddc92 100644 --- a/README.md +++ b/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 diff --git a/accounts/apps.py b/accounts/apps.py index eb6a7d58..931d6620 100644 --- a/accounts/apps.py +++ b/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 diff --git a/accounts/checks.py b/accounts/checks.py index d001eaa9..f250cd2f 100644 --- a/accounts/checks.py +++ b/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 [] + diff --git a/accounts/migrations/0008_apply_show_profile_edit_password_once.py b/accounts/migrations/0008_apply_show_profile_edit_password_once.py new file mode 100644 index 00000000..06f18176 --- /dev/null +++ b/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)] diff --git a/accounts/test_change_password_permission.py b/accounts/test_change_password_permission.py new file mode 100644 index 00000000..163a06b4 --- /dev/null +++ b/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)) diff --git a/admin/forms.py b/admin/forms.py index 3f79ddce..6d44b4c0 100644 --- a/admin/forms.py +++ b/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 = [ diff --git a/webvirtcloud/settings.py.template b/webvirtcloud/settings.py.template index 34640cb5..19781f24 100644 --- a/webvirtcloud/settings.py.template +++ b/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 diff --git a/webvirtcloud/test_authorization_matrix.py b/webvirtcloud/test_authorization_matrix.py index e746c677..4e1e3b75 100644 --- a/webvirtcloud/test_authorization_matrix.py +++ b/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",