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