diff --git a/appsettings/migrations/0020_fix_setting_choices.py b/appsettings/migrations/0020_fix_setting_choices.py new file mode 100644 index 00000000..d316fa65 --- /dev/null +++ b/appsettings/migrations/0020_fix_setting_choices.py @@ -0,0 +1,32 @@ +from django.db import migrations + +# key: (its choices, the value a removed choice becomes) +FIXES = { + "CLONE_INSTANCE_DEFAULT_PREFIX": ("", None), # a text, not True or False + "INSTANCE_VOLUME_DEFAULT_SCSI_CONTROLLER": ("virtio-scsi,lsilogic", "virtio-scsi"), # virtio-blk is no SCSI controller + "INSTANCE_MACHINE_DEFAULT_TYPE": ("q35,pc,virt", "q35"), # x86_64 is an architecture + "INSTANCE_CDROM_ADD": ("None,ide,sata,scsi", "sata"), # a CD-ROM has no virtio bus +} + + +def fix_choices(apps, schema_editor): + """Choices the panel cannot use, a space that kept lsilogic from being + shown as chosen, and a text setting listed as True or False. Not undone + on the way back, as nothing needs the old choices.""" + AppSettings = apps.get_model("appsettings", "AppSettings") + for setting in AppSettings.objects.using(schema_editor.connection.alias).filter(key__in=FIXES): + choices, fallback = FIXES[setting.key] + setting.choices = choices + if choices and setting.value not in choices.split(","): + setting.value = fallback + setting.save() + + +class Migration(migrations.Migration): + dependencies = [ + ("appsettings", "0019_prebuilt_themes"), + ] + + operations = [ + migrations.RunPython(fix_choices, migrations.RunPython.noop), + ] diff --git a/appsettings/templates/appsettings.html b/appsettings/templates/appsettings.html index acce2fef..6e60a7fd 100644 --- a/appsettings/templates/appsettings.html +++ b/appsettings/templates/appsettings.html @@ -50,7 +50,7 @@ {% if setting.choices %} {% else %} diff --git a/appsettings/tests/test_appsettings.py b/appsettings/tests/test_appsettings.py index ca739d6c..52b34cab 100644 --- a/appsettings/tests/test_appsettings.py +++ b/appsettings/tests/test_appsettings.py @@ -54,3 +54,32 @@ class AppSettingsTestCase(TestCase): response = self.client.post(reverse("appsettings"), {"WORKER_CONCURRENCY": "0"}) self.assertEqual(AppSettings.objects.get(key="WORKER_CONCURRENCY").value, "4") self.assertTrue(any("between 1 and 16" in str(m) for m in get_messages(response.wsgi_request))) + + def post(self, key, value): + response = self.client.post(reverse("appsettings"), {key: value}) + return [str(m) for m in get_messages(response.wsgi_request)] + + def test_a_listed_setting_takes_only_its_choices(self): + messages = self.post("INSTANCE_FIRMWARE_DEFAULT_TYPE", "Coreboot") + self.assertEqual(AppSettings.objects.get(key="INSTANCE_FIRMWARE_DEFAULT_TYPE").value, "BIOS") + self.assertTrue(any("BIOS, UEFI" in m for m in messages), messages) + + def test_number_settings_take_whole_numbers_in_their_range(self): + # read with int() elsewhere: a letter would break the logs page or VM create + for key, bad, good in (("LOGS_PER_PAGE", "abc", "50"), ("LOGS_PER_PAGE", "0", "1000"), + ("INSTANCE_VOLUME_DEFAULT_OWNER_UID", "-1", "107"), + ("INSTANCE_VOLUME_DEFAULT_OWNER_GID", "x", "4294967294"), + ("LOGIN_FAILURE_LIMIT", "0", "10"), ("SNAPSHOT_FREE_SPACE_PERCENT", "101", "0")): + with self.subTest(key=key, bad=bad): + before = AppSettings.objects.get(key=key).value + self.assertTrue(any("whole number" in m for m in self.post(key, bad))) + self.assertEqual(AppSettings.objects.get(key=key).value, before) + self.post(key, good) + self.assertEqual(AppSettings.objects.get(key=key).value, good) + + def test_the_clone_name_prefix_is_text(self): + self.post("CLONE_INSTANCE_DEFAULT_PREFIX", "web-") + self.assertEqual(AppSettings.objects.get(key="CLONE_INSTANCE_DEFAULT_PREFIX").value, "web-") + page = self.client.get(reverse("appsettings")).content.decode() + self.assertIn('name="CLONE_INSTANCE_DEFAULT_PREFIX" value="web-"', page) + diff --git a/appsettings/tests/test_migrations.py b/appsettings/tests/test_migrations.py index a85be710..ee44dc46 100644 --- a/appsettings/tests/test_migrations.py +++ b/appsettings/tests/test_migrations.py @@ -61,3 +61,29 @@ class WorkerSettingsTestCase(TestCase): limit = AppSettings._meta.get_field(field).max_length if limit: self.assertLessEqual(len(getattr(setting, field)), limit, f"{key} {field}") + + +choices_fix = importlib.import_module("appsettings.migrations.0020_fix_setting_choices") + + +class ChoicesFixTestCase(TestCase): + """Choices that offered values the panel cannot use, and a text setting + that showed as a True/False list.""" + + def test_the_broken_choices_are_fixed_and_their_values_moved(self): + broken = { + "CLONE_INSTANCE_DEFAULT_PREFIX": ("instance", "True,False"), + "INSTANCE_VOLUME_DEFAULT_SCSI_CONTROLLER": ("virtio-blk", "virtio-scsi, lsilogic, virtio-blk"), + "INSTANCE_MACHINE_DEFAULT_TYPE": ("x86_64", "q35,x86_64,virt"), + "INSTANCE_CDROM_ADD": ("virtio", "None,ide,sata,scsi,virtio"), + } + for key, (value, choices) in broken.items(): + AppSettings.objects.filter(key=key).update(value=value, choices=choices) + choices_fix.fix_choices(apps, SimpleNamespace(connection=connection)) + fixed = {s.key: (s.value, s.choices) for s in AppSettings.objects.filter(key__in=broken)} + self.assertEqual(fixed, { + "CLONE_INSTANCE_DEFAULT_PREFIX": ("instance", ""), + "INSTANCE_VOLUME_DEFAULT_SCSI_CONTROLLER": ("virtio-scsi", "virtio-scsi,lsilogic"), + "INSTANCE_MACHINE_DEFAULT_TYPE": ("q35", "q35,pc,virt"), + "INSTANCE_CDROM_ADD": ("sata", "None,ide,sata,scsi"), + }) diff --git a/appsettings/views.py b/appsettings/views.py index 4a2f63fa..290ee934 100644 --- a/appsettings/views.py +++ b/appsettings/views.py @@ -15,6 +15,32 @@ from appsettings.models import AppSettings # the themes "manage.py build_themes" compiles; base.html links the chosen one THEMES_DIR = Path(settings.BASE_DIR, "static", "css", "themes") +# the whole-number settings and their range: they are read as numbers, and a +# letter would break the page or the job that reads one +NUMBERS = { + **{key: bounds[:2] for key, bounds in BOUNDS.items()}, + "LOGS_PER_PAGE": (1, 1000), + "LOGIN_FAILURE_LIMIT": (1, 1000), + "LOGIN_LOCKOUT_MINUTES": (1, 10080), + "SNAPSHOT_FREE_SPACE_PERCENT": (0, 100), + "SNAPSHOT_FREE_SPACE_MIN_GIB": (0, 100000), + "INSTANCE_VOLUME_DEFAULT_OWNER_UID": (0, 4294967294), + "INSTANCE_VOLUME_DEFAULT_OWNER_GID": (0, 4294967294), +} + + +def refusal(setting, value): + """Why the value cannot be saved for the setting, or None.""" + if setting.key in NUMBERS: + low, high = NUMBERS[setting.key] + if not (value.isascii() and value.isdigit() and len(value) <= 10 and low <= int(value) <= high): + return _("%(setting)s must be a whole number between %(low)s and %(high)s") % { + "setting": setting.name, "low": low, "high": high} + elif setting.choices and value not in setting.choices_as_list(): + return _("%(setting)s must be one of %(choices)s") % { + "setting": setting.name, "choices": ", ".join(setting.choices_as_list())} + return None + @superuser_only def appsettings(request): @@ -46,14 +72,11 @@ def appsettings(request): for setting in appsettings: if setting.key in request.POST: - if setting.key in BOUNDS: - low, high, _default = BOUNDS[setting.key] - raw = request.POST.get(setting.key, "") - if not (raw.isascii() and raw.isdigit() and len(raw) <= 4 and low <= int(raw) <= high): - messages.error(request, _("%(setting)s must be a whole number between %(low)s and %(high)s") - % {"setting": setting.name, "low": low, "high": high}) - return HttpResponseRedirect(request.get_full_path()) value = request.POST.get(setting.key, "") + refused = refusal(setting, value) + if refused: + messages.error(request, refused) + return HttpResponseRedirect(request.get_full_path()) try: if value != setting.value: with audit(request, None, f"Setting {setting.name}", f"change to {value}"): diff --git a/instances/tests/test_quota.py b/instances/tests/test_quota.py index 4c9757e6..b40fc99b 100644 --- a/instances/tests/test_quota.py +++ b/instances/tests/test_quota.py @@ -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_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 + + for i in range(5): + self.own(self.up, i) + for value, expected in (("True", "instance (6 > 5)"), ("False", "instance")): + with self.subTest(value=value): + AppSettings.objects.filter(key="QUOTA_DEBUG").update(value=value) + get_settings() + with patch.object(Compute, "status", True), patch("instances.utils.wvmInstance", side_effect=vm_reader()): + self.assertEqual(check_user_quota(self.user, 1, 0, 0, 0), expected) + class QuotaViewTests(TestCase): """The resize views turn the result into a message and charge only what changes.""" diff --git a/instances/utils.py b/instances/utils.py index 6a4973e2..39a5463c 100644 --- a/instances/utils.py +++ b/instances/utils.py @@ -62,7 +62,7 @@ def check_user_quota(user, instance, cpu, memory, disk_size): if instance <= 0 and cpu <= 0 and memory <= 0 and disk_size <= 0: return msg - quota_debug = app_settings.QUOTA_DEBUG + quota_debug = app_settings.QUOTA_DEBUG == "True" user_instances = UserInstance.objects.filter(user=user, instance__is_template=False).select_related( "instance__compute"