fix(forms): stop InterfaceForm crashing on invalid or IP-less interfaces

mdb/forms.py

@@ -59,16 +59,25 @@ class Ip4ModelChoiceField(ModelChoiceField):
 
 
 class InterfaceForm(forms.ModelForm):
-    model = Interface
+    class Meta:
+        model = Interface
+        fields = "__all__"
 
     def clean(self):
-        macaddr = self.cleaned_data["macaddr"]
+        cleaned_data = super().clean()
-        ip4address = self.cleaned_data["ip4address"]
+        # Missing when the field itself failed validation, and the address is optional.
+        macaddr = cleaned_data.get("macaddr")
+        ip4address = cleaned_data.get("ip4address")
+        if not macaddr or ip4address is None:
+            return cleaned_data
 
-        for interface in Interface.objects.filter(macaddr=macaddr):
+        # dhcpd matches MAC addresses whatever their case.
-            subnet = interface.ip4address.subnet
+        clashes = Interface.objects.filter(
-            if subnet == ip4address.subnet and interface != self.instance:
+            macaddr__iexact=macaddr, ip4address__subnet=ip4address.subnet
-                raise ValidationError("Macaddr already in this subnet")
+        ).exclude(pk=self.instance.pk)
+        if clashes.exists():
+            raise ValidationError("Macaddr already in this subnet")
+        return cleaned_data
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -76,4 +85,7 @@ class InterfaceForm(forms.ModelForm):
         # Join Ip4Address with tables interface and host,
         # before printing the select labels that walk those two relations.
         queryset = Ip4Address.objects.select_related("interface", "interface__host")
-        self.fields["ip4address"] = Ip4ModelChoiceField(queryset=queryset)
+        # Not required, as on the model: a host can have interfaces without IPv4.
+        self.fields["ip4address"] = Ip4ModelChoiceField(
+            queryset=queryset, required=False
+        )

tests/test_forms.py

@@ -0,0 +1,64 @@
+import pytest
+from django.core.management import call_command
+
+from mdb.forms import InterfaceForm
+from mdb.models import Host, Interface, Ip4Address
+
+
+@pytest.fixture
+def fixtures(db):
+    call_command("loaddata", "test_data", verbosity=0)
+
+
+def form(instance=None, **overrides):
+    data = {
+        "name": "eth1",
+        "macaddr": "00:11:22:33:44:55",
+        "host": Host.objects.get().pk,
+        "domain": 1,
+        "ip4address": Ip4Address.objects.get(address="10.0.3.10").pk,
+        "pxe_filename": "",
+    } | overrides
+    return InterfaceForm(data=data, instance=instance)
+
+
+def test_valid(fixtures):
+    f = form()
+    assert f.is_valid(), f.errors
+
+
+def test_invalid_mac_is_a_form_error(fixtures):
+    # Used to raise KeyError, a 500 in the admin.
+    f = form(macaddr="not a mac")
+    assert not f.is_valid()
+    assert "macaddr" in f.errors
+
+
+def test_interface_without_ip4address(fixtures):
+    f = form(ip4address="")
+    assert f.is_valid(), f.errors
+
+
+@pytest.mark.parametrize("macaddr", ["f0:de:f1:58:be:1a", "F0:DE:F1:58:BE:1A"])
+def test_mac_already_in_subnet(fixtures, macaddr):
+    f = form(macaddr=macaddr)
+    assert not f.is_valid()
+    assert f.non_field_errors() == ["Macaddr already in this subnet"]
+
+
+def test_mac_in_use_by_interface_without_ip4address(fixtures):
+    # Used to raise AttributeError on the other interface's missing address.
+    Interface.objects.update(ip4address=None)
+    f = form(macaddr="f0:de:f1:58:be:1a")
+    assert f.is_valid(), f.errors
+
+
+def test_editing_interface_does_not_clash_with_itself(fixtures):
+    interface = Interface.objects.get()
+    f = form(
+        instance=interface,
+        name="eth0",
+        macaddr=interface.macaddr,
+        ip4address=interface.ip4address.pk,
+    )
+    assert f.is_valid(), f.errors