mirror of
https://github.com/retspen/webvirtcloud
synced 2026-11-08 11:16:47 +00:00
fix(admin): the Users pages keep an administrator in and say what their fields mean
- An administrator could block, delete or demote (unset Active or Superuser on) their own account, and with it the last one, leaving the panel without anyone to undo it. These are now refused. - The user form showed "No password set." for users who have one: its password field never read the user's password. Its "Set password" link went to ../password/, a page this panel does not have. The form now says whether a password is set and links the page that sets it. - "Staff status" said "can log into this admin site"; here it lets a user change and delete the template VMs given to them, and is now called Template editor. - A quota of 0 counted as no limit, like -1, so a user's quota could not be taken away. Now -1 has no limit and 0 allows none; the help says so, and the labels give the units: memory in MiB, disk in GiB. - A user's page for a user that does not exist answered with a server error instead of not found. - The user list said "None" for a user who never logged in; it says "never", and dates fit their column. - The password form's labels no longer break mid-word.
This commit is contained in:
parent
07a2e09c2f
commit
0e8130f1b0
11 changed files with 161 additions and 43 deletions
34
accounts/migrations/0011_quota_help.py
Normal file
34
accounts/migrations/0011_quota_help.py
Normal file
|
|
@ -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)'),
|
||||
),
|
||||
]
|
||||
|
|
@ -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)],
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@
|
|||
<div class="card-body">
|
||||
<form method="post" id="password-change">
|
||||
{% 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' %}
|
||||
</form>
|
||||
</div>
|
||||
<div class="card-footer">
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 <a href='{}'>this form</a>."""
|
||||
),
|
||||
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",
|
||||
|
|
|
|||
|
|
@ -13,6 +13,15 @@
|
|||
<form id="create-update" action="" method="post">
|
||||
{% csrf_token %}
|
||||
{% bootstrap_form user_form layout='horizontal' %}
|
||||
{% if edited_user %}
|
||||
<div class="row mb-3">
|
||||
<span class="col-sm-2 col-form-label">{% trans "Password" %}</span>
|
||||
<div class="col-sm-10 col-form-label">
|
||||
{% if edited_user.has_usable_password %}{% trans "Set" %}{% else %}{% trans "Not set" %}{% endif %}
|
||||
<a class="ms-2" href="{% url 'admin:user_update_password' edited_user.id %}">{% trans "Change password" %}</a>
|
||||
</div>
|
||||
</div>
|
||||
{% endif %}
|
||||
{% bootstrap_form attributes_form layout='horizontal' %}
|
||||
</form>
|
||||
<div class="float-end">
|
||||
|
|
|
|||
|
|
@ -32,7 +32,7 @@
|
|||
<th span="col">{% trans "Username" %}</th>
|
||||
<th span="col">{% trans "Status" %}</th>
|
||||
<th span="col">{% trans "Last Login" %}</th>
|
||||
<th span="col">{% trans "Staff" %}</th>
|
||||
<th span="col">{% trans "Template editor" %}</th>
|
||||
<th span="col">{% trans "Superuser" %}</th>
|
||||
<th span="col">{% trans "Can Clone" %}</th>
|
||||
<th span="col">{% trans "API tokens" %}</th>
|
||||
|
|
@ -53,7 +53,7 @@
|
|||
{% trans "Blocked" %}
|
||||
{% endif %}
|
||||
</td>
|
||||
<td>{{ user.last_login }}</td>
|
||||
<td>{{ user.last_login|date:"Y-m-d H:i"|default:_("never") }}</td>
|
||||
<td>{% if user.is_staff %}{% bs_icon 'check' %}{% endif %}</td>
|
||||
<td>{% if user.is_superuser %}{% bs_icon 'check' %}{% endif %}</td>
|
||||
<td>{% if can_clone %}{% bs_icon 'check' %}{% endif %}</td>
|
||||
|
|
|
|||
67
admin/tests/test_user_pages.py
Normal file
67
admin/tests/test_user_pages.py
Normal file
|
|
@ -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")
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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})"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue