diff --git a/accounts/migrations/0011_quota_help.py b/accounts/migrations/0011_quota_help.py new file mode 100644 index 00000000..a389d250 --- /dev/null +++ b/accounts/migrations/0011_quota_help.py @@ -0,0 +1,34 @@ +# Generated by Django 5.2.18 on 2026-10-25 19:29 + +import django.core.validators +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('accounts', '0010_api_token_last_used'), + ] + + operations = [ + migrations.AlterField( + model_name='userattributes', + name='max_cpus', + field=models.IntegerField(default=2, help_text='-1: no limit; 0: none allowed', validators=[django.core.validators.MinValueValidator(-1)], verbose_name='max CPUs'), + ), + migrations.AlterField( + model_name='userattributes', + name='max_disk_size', + field=models.IntegerField(default=20, help_text='-1: no limit; 0: none allowed', validators=[django.core.validators.MinValueValidator(-1)], verbose_name='max disk size (GiB)'), + ), + migrations.AlterField( + model_name='userattributes', + name='max_instances', + field=models.IntegerField(default=2, help_text='-1: no limit; 0: none allowed', validators=[django.core.validators.MinValueValidator(-1)], verbose_name='max instances'), + ), + migrations.AlterField( + model_name='userattributes', + name='max_memory', + field=models.IntegerField(default=2048, help_text='-1: no limit; 0: none allowed', validators=[django.core.validators.MinValueValidator(-1)], verbose_name='max memory (MiB)'), + ), + ] diff --git a/accounts/models.py b/accounts/models.py index dcdcaaf8..61f7187a 100644 --- a/accounts/models.py +++ b/accounts/models.py @@ -46,25 +46,25 @@ class UserAttributes(models.Model): max_instances = models.IntegerField( _("max instances"), default=2, - help_text=_("-1 for unlimited. Any integer value"), + help_text=_("-1: no limit; 0: none allowed"), validators=[MinValueValidator(-1)], ) max_cpus = models.IntegerField( _("max CPUs"), default=2, - help_text=_("-1 for unlimited. Any integer value"), + help_text=_("-1: no limit; 0: none allowed"), validators=[MinValueValidator(-1)], ) max_memory = models.IntegerField( - _("max memory"), + _("max memory (MiB)"), default=2048, - help_text=_("-1 for unlimited. Any integer value"), + help_text=_("-1: no limit; 0: none allowed"), validators=[MinValueValidator(-1)], ) max_disk_size = models.IntegerField( - _("max disk size"), + _("max disk size (GiB)"), default=20, - help_text=_("-1 for unlimited. Any integer value"), + help_text=_("-1: no limit; 0: none allowed"), validators=[MinValueValidator(-1)], ) diff --git a/accounts/templates/accounts/change_password_form.html b/accounts/templates/accounts/change_password_form.html index 35dab319..046447e8 100644 --- a/accounts/templates/accounts/change_password_form.html +++ b/accounts/templates/accounts/change_password_form.html @@ -16,7 +16,7 @@
{% csrf_token %} - {% bootstrap_form form layout='horizontal' %} + {% bootstrap_form form layout='horizontal' horizontal_label_class='col-sm-4' horizontal_field_class='col-sm-8' horizontal_field_offset_class='offset-sm-4' %}
diff --git a/accounts/views.py b/accounts/views.py index bf1f7d0b..e91256d2 100644 --- a/accounts/views.py +++ b/accounts/views.py @@ -146,7 +146,7 @@ def ssh_key_delete(request, pk): @superuser_only def account(request, user_id): - user = User.objects.get(id=user_id) + user = get_object_or_404(User, id=user_id) user_insts = UserInstance.objects.filter(user_id=user_id) instances = Instance.objects.all().order_by("name") publickeys = UserSSHKey.objects.filter(user_id=user_id) diff --git a/admin/forms.py b/admin/forms.py index 4ebf689c..63f523ac 100644 --- a/admin/forms.py +++ b/admin/forms.py @@ -1,10 +1,7 @@ from django import forms from django.contrib.auth import password_validation -from django.contrib.auth.forms import ReadOnlyPasswordHashField from django.contrib.auth.models import Group, User from django.core.exceptions import ValidationError -from django.urls import reverse_lazy -from django.utils.text import format_lazy from django.utils.translation import gettext_lazy as _ from accounts.models import UserAttributes @@ -72,26 +69,8 @@ class UserForm(forms.ModelForm): "is_active", "is_superuser", ] - - def __init__(self, *args, **kwargs): - super(UserForm, self).__init__(*args, **kwargs) - if self.instance.id: - password = ReadOnlyPasswordHashField( - label=_("Password"), - help_text=format_lazy( - _( - """Raw passwords are not stored, so there is no way to see this user's password, - but you can change the password using this form.""" - ), - reverse_lazy( - "admin:user_update_password", - args=[ - self.instance.id, - ], - ), - ), - ) - self.fields["Password"] = password + labels = {"is_staff": _("Template editor")} + help_texts = {"is_staff": _("May change and delete the template VMs given to them")} class UserCreateForm(UserForm): @@ -125,8 +104,7 @@ class UserCreateForm(UserForm): self.save_m2m() return user - class Meta: - model = User + class Meta(UserForm.Meta): fields = [ "username", "password", diff --git a/admin/templates/admin/user_form.html b/admin/templates/admin/user_form.html index fb502a64..bf2e9977 100644 --- a/admin/templates/admin/user_form.html +++ b/admin/templates/admin/user_form.html @@ -13,6 +13,15 @@
{% csrf_token %} {% bootstrap_form user_form layout='horizontal' %} + {% if edited_user %} +
+ {% trans "Password" %} +
+ {% if edited_user.has_usable_password %}{% trans "Set" %}{% else %}{% trans "Not set" %}{% endif %} + {% trans "Change password" %} +
+
+ {% endif %} {% bootstrap_form attributes_form layout='horizontal' %}
diff --git a/admin/templates/admin/user_list.html b/admin/templates/admin/user_list.html index 108a968f..ea69cfbc 100644 --- a/admin/templates/admin/user_list.html +++ b/admin/templates/admin/user_list.html @@ -32,7 +32,7 @@ {% trans "Username" %} {% trans "Status" %} {% trans "Last Login" %} - {% trans "Staff" %} + {% trans "Template editor" %} {% trans "Superuser" %} {% trans "Can Clone" %} {% trans "API tokens" %} @@ -53,7 +53,7 @@ {% trans "Blocked" %} {% endif %} - {{ user.last_login }} + {{ user.last_login|date:"Y-m-d H:i"|default:_("never") }} {% if user.is_staff %}{% bs_icon 'check' %}{% endif %} {% if user.is_superuser %}{% bs_icon 'check' %}{% endif %} {% if can_clone %}{% bs_icon 'check' %}{% endif %} diff --git a/admin/tests/test_user_pages.py b/admin/tests/test_user_pages.py new file mode 100644 index 00000000..d9fa6512 --- /dev/null +++ b/admin/tests/test_user_pages.py @@ -0,0 +1,67 @@ +"""The Users pages: an administrator cannot lock their own account out, and +the user form says what its fields mean.""" + +from accounts.models import UserAttributes +from django.contrib.auth import get_user_model +from django.contrib.messages import get_messages +from django.test import TestCase +from django.urls import reverse +from lxml import html + + +class UserPagesTests(TestCase): + def setUp(self): + User = get_user_model() + self.admin = User.objects.create_superuser("up-admin", "up@example.com", "pw") + self.alice = User.objects.create_user("up-alice", password="Alice-pw-123") + UserAttributes.objects.get_or_create(user=self.alice) + self.client.force_login(self.admin) + + def messages(self, response): + return [str(m) for m in get_messages(response.wsgi_request)] + + def form_data(self, user, **changes): + data = {"username": user.username, "is_active": "on", "is_superuser": "on" if user.is_superuser else "", + "max_instances": "2", "max_cpus": "2", "max_memory": "2048", "max_disk_size": "20", **changes} + return {key: value for key, value in data.items() if value != ""} + + def test_an_administrator_cannot_block_delete_or_demote_themselves(self): + own = self.admin.pk + for response in (self.client.post(reverse("admin:user_block", args=[own])), + self.client.post(reverse("admin:user_delete", args=[own]))): + self.assertIn("You cannot block, delete or demote your own account", self.messages(response)) + for change in ({"is_superuser": ""}, {"is_active": ""}): + response = self.client.post(reverse("admin:user_update", args=[own]), self.form_data(self.admin, **change)) + self.assertContains(response, "You cannot block, delete or demote your own account") + self.admin.refresh_from_db() + self.assertTrue(self.admin.is_active and self.admin.is_superuser) + # another user's account is theirs to change + self.client.post(reverse("admin:user_block", args=[self.alice.pk])) + self.alice.refresh_from_db() + self.assertFalse(self.alice.is_active) + + def test_the_form_tells_the_password_state_and_links_its_page(self): + page = html.fromstring(self.client.get(reverse("admin:user_update", args=[self.alice.pk])).content) + text = " ".join(page.text_content().split()) + self.assertIn("Password Set Change password", text) + self.assertNotIn("No password set", text) + self.assertEqual(page.xpath("//a[normalize-space()='Change password']/@href"), + [reverse("admin:user_update_password", args=[self.alice.pk])]) + self.assertEqual(page.xpath("//a[@href='../password/']"), []) + + def test_staff_and_quotas_say_what_they_mean(self): + page = self.client.get(reverse("admin:user_update", args=[self.alice.pk])).content.decode() + self.assertIn("Template editor", page) + self.assertIn("May change and delete the template VMs given to them", page) + self.assertNotIn("log into this admin site", page) + self.assertIn("Max memory (MiB)", page) + self.assertIn("Max disk size (GiB)", page) + self.assertIn("-1: no limit; 0: none allowed", page) + + def test_a_missing_users_profile_is_not_found(self): + self.assertEqual(self.client.get(reverse("accounts:account", args=[99999])).status_code, 404) + + def test_a_user_who_never_logged_in_says_never(self): + page = html.fromstring(self.client.get(reverse("admin:user_list")).content) + (row,) = [r for r in page.xpath("//tbody/tr") if r.xpath("td")[0].text_content().strip() == "up-alice"] + self.assertEqual(row.xpath("td")[2].text_content().strip(), "never") diff --git a/admin/views.py b/admin/views.py index bf74928f..74d3b063 100644 --- a/admin/views.py +++ b/admin/views.py @@ -96,6 +96,11 @@ def group_delete(request, pk): ) +# an administrator who blocks, deletes or demotes their own account is out of +# these pages at once, and may have been the last one +OWN_ACCOUNT = _("You cannot block, delete or demote your own account") + + @superuser_only def user_list(request): # API tokens that still work: an expired one is deleted only at the user's next create @@ -145,7 +150,11 @@ def user_update(request, pk): attributes_form = forms.UserAttributesForm( request.POST or None, instance=attributes ) - if user_form.is_valid() and attributes_form.is_valid(): + valid = user_form.is_valid() and attributes_form.is_valid() + if valid and user == request.user and not (user_form.cleaned_data["is_active"] and user_form.cleaned_data["is_superuser"]): + user_form.add_error(None, OWN_ACCOUNT) + valid = False + if valid: with audit(request, None, subject, "change") as entry: user_form.save() attributes_form.save() @@ -166,6 +175,7 @@ def user_update(request, pk): { "user_form": user_form, "attributes_form": attributes_form, + "edited_user": user, "title": _("Update User"), }, ) @@ -202,6 +212,9 @@ def user_update_password(request, pk): @superuser_only def user_delete(request, pk): user = get_object_or_404(User, pk=pk) + if user == request.user: + messages.error(request, OWN_ACCOUNT) + return redirect("admin:user_list") if request.method == "POST": with audit(request, None, f"User {user.username}", "delete"): user.delete() @@ -219,6 +232,9 @@ def user_delete(request, pk): @superuser_only def user_block(request, pk): user: User = get_object_or_404(User, pk=pk) + if user == request.user: + messages.error(request, OWN_ACCOUNT) + return redirect("admin:user_list") with audit(request, None, f"User {user.username}", "block") as entry: entry.skip = not user.is_active # already blocked user.is_active = False diff --git a/instances/tests/test_quota.py b/instances/tests/test_quota.py index b40fc99b..cd8cebff 100644 --- a/instances/tests/test_quota.py +++ b/instances/tests/test_quota.py @@ -89,7 +89,7 @@ class QuotaTests(TestCase): self.assertEqual(self.check(0, 0, -512, 0), ("", 0)) def test_unlimited_users_are_not_checked(self): - UserAttributes.objects.filter(user=self.user).update(max_instances=-1, max_cpus=-1, max_memory=0, max_disk_size=-1) + UserAttributes.objects.filter(user=self.user).update(max_instances=-1, max_cpus=-1, max_memory=-1, max_disk_size=-1) self.own(self.down, 1) self.assertEqual(self.check(1, 4, 4096, 50), ("", 0)) @@ -111,6 +111,19 @@ class QuotaTests(TestCase): Instance.objects.filter(pk=vm.pk).update(is_template=True) self.assertEqual(self.check(0, 2, 0, 0), ("", 3)) + def test_zero_allows_none_and_minus_one_has_no_limit(self): + # 0 takes a quota away from the user; -1 lifts it + ua = UserAttributes.objects.get(user=self.user) + for field, delta, refused in (("max_instances", (1, 0, 0, 0), "instance"), ("max_cpus", (0, 1, 0, 0), "cpu"), + ("max_memory", (0, 0, 512, 0), "memory"), ("max_disk_size", (0, 0, 0, 1), "disk")): + with self.subTest(field=field): + setattr(ua, field, 0) + ua.save() + self.assertEqual(self.check(*delta)[0], refused) + setattr(ua, field, -1) + ua.save() + self.assertEqual(self.check(*delta)[0], "") + def test_the_numbers_show_only_with_quota_debug_on(self): # the setting is the text "True" or "False": "False" turns them off from appsettings.models import AppSettings diff --git a/instances/utils.py b/instances/utils.py index 39a5463c..d0ab61dd 100644 --- a/instances/utils.py +++ b/instances/utils.py @@ -49,7 +49,8 @@ QUOTA_UNVERIFIED = "unverified" def check_user_quota(user, instance, cpu, memory, disk_size): """ Which quota the user would exceed by adding these amounts (instances, - vCPUs, memory MiB, disk GiB) to their VMs, or "". QUOTA_UNVERIFIED if a + vCPUs, memory MiB, disk GiB) to their VMs, or "". A quota of -1 has no + limit; 0 allows none. QUOTA_UNVERIFIED if a VM that counts cannot be read (its host is down, the VM is gone): the usage is unknown, so an increase is refused. """ @@ -68,12 +69,12 @@ def check_user_quota(user, instance, cpu, memory, disk_size): "instance__compute" ) instance += user_instances.count() - if ua.max_instances > 0 and instance > ua.max_instances: + if ua.max_instances >= 0 and instance > ua.max_instances: msg = "instance" if quota_debug: msg += f" ({instance} > {ua.max_instances})" return msg - if ua.max_cpus <= 0 and ua.max_memory <= 0 and ua.max_disk_size <= 0: + if ua.max_cpus < 0 and ua.max_memory < 0 and ua.max_disk_size < 0: return msg computes = {} # status is cached per object: check each host once @@ -99,15 +100,15 @@ def check_user_quota(user, instance, cpu, memory, disk_size): except libvirtError: return QUOTA_UNVERIFIED - if ua.max_cpus > 0 and cpu > ua.max_cpus: + if ua.max_cpus >= 0 and cpu > ua.max_cpus: msg = "cpu" if quota_debug: msg += f" ({cpu} > {ua.max_cpus})" - if ua.max_memory > 0 and memory > ua.max_memory: + if ua.max_memory >= 0 and memory > ua.max_memory: msg = "memory" if quota_debug: msg += f" ({memory} > {ua.max_memory})" - if ua.max_disk_size > 0 and disk_size > ua.max_disk_size: + if ua.max_disk_size >= 0 and disk_size > ua.max_disk_size: msg = "disk" if quota_debug: msg += f" ({disk_size} > {ua.max_disk_size})"