fix(appsettings): settings take only the values the panel can use

The Settings page saved whatever was posted, and some lists offered
values the panel cannot use:

- Quota Debug could not be turned off: "False" is a non-empty text and
  counted as on.
- VM Clone Name Prefix showed as a True/False list, so no prefix could
  be typed, and touching it set the prefix to True.
- Disk SCSI Controller listed " lsilogic" with a space, never shown as
  chosen, and virtio-blk, which is no SCSI controller.
- VM Machine Type listed x86_64, an architecture; VM CD-ROM Device
  listed virtio, a bus a CD-ROM cannot use.
- Logs per Page and Disk Owner UID/GID are read with int(): a letter
  broke the logs page and VM create.

A migration fixes the lists and moves a value they no longer offer to
the default (the clone prefix keeps its text). A listed setting now
takes one of its choices, and every number setting a whole number in
its range, checked before it is saved.

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),
+    ]

appsettings/templates/appsettings.html

@@ -50,7 +50,7 @@
                     {% if setting.choices %}
                         <select class="form-select" name="{{ setting.key }}"  onchange="this.form.submit()" title="{% trans setting.description %}">
                             {% for choice in setting.choices_as_list %}
-                                <option {% if setting.value == choice %} selected {% endif %} value={{ choice }}>{% trans choice %}</option>
+                                <option {% if setting.value == choice %} selected {% endif %} value="{{ choice }}">{% trans choice %}</option>
                             {% endfor %}
                         </select>
                     {% else %}

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)
+

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"),
+        })

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}"):

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

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"