From 8a293807268a4df9052a16ad74709443f5f048e4 Mon Sep 17 00:00:00 2001 From: Gary Snider <75227981+gsnider2195@users.noreply.github.com> Date: Mon, 7 Oct 2024 14:31:45 -0700 Subject: [PATCH] fix UI and model bugs (#274) --- changes/222.fixed | 1 + changes/233.fixed | 2 + changes/245.fixed | 2 + changes/275.fixed | 1 + nautobot_firewall_models/filters.py | 2 + nautobot_firewall_models/forms.py | 2 - .../0022_fix_policy_natpolicy_m2ms.py | 54 +++++++++++++++++++ nautobot_firewall_models/models/nat_policy.py | 10 +++- .../models/security_policy.py | 10 +++- .../inc/nat_policy_device_weight.html | 2 +- .../inc/nat_policy_dynamic_group_weight.html | 2 +- .../inc/policy_dynamic_group_weight.html | 2 +- nautobot_firewall_models/utils/capirca.py | 29 ++++++---- .../viewsets/nat_policy.py | 8 +-- 14 files changed, 105 insertions(+), 22 deletions(-) create mode 100644 changes/222.fixed create mode 100644 changes/233.fixed create mode 100644 changes/245.fixed create mode 100644 changes/275.fixed create mode 100644 nautobot_firewall_models/migrations/0022_fix_policy_natpolicy_m2ms.py diff --git a/changes/222.fixed b/changes/222.fixed new file mode 100644 index 00000000..34368dd3 --- /dev/null +++ b/changes/222.fixed @@ -0,0 +1 @@ +Fixed server error when navigating to Policy detail view. diff --git a/changes/233.fixed b/changes/233.fixed new file mode 100644 index 00000000..0785601d --- /dev/null +++ b/changes/233.fixed @@ -0,0 +1,2 @@ +Fixed name fields being optional on multiple forms. +Fixed assigned devices and assigned dynamic groups fields not marked as optional on NATPolicy and Policy. diff --git a/changes/245.fixed b/changes/245.fixed new file mode 100644 index 00000000..7357fa0e --- /dev/null +++ b/changes/245.fixed @@ -0,0 +1,2 @@ +Fixed server error when navigating to NATPolicy detail view. +Fixed server error when updating device/dynamic group weights on NATPolicy. diff --git a/changes/275.fixed b/changes/275.fixed new file mode 100644 index 00000000..720c933a --- /dev/null +++ b/changes/275.fixed @@ -0,0 +1 @@ +Fixed capirca failures with Nautobot v2.3.3 or higher. diff --git a/nautobot_firewall_models/filters.py b/nautobot_firewall_models/filters.py index 990c41f9..558778d7 100644 --- a/nautobot_firewall_models/filters.py +++ b/nautobot_firewall_models/filters.py @@ -163,6 +163,7 @@ def search(self, queryset, name, value): # pylint: disable=unused-argument """Construct Q filter for filterset.""" if not value.strip(): return queryset + # pylint: disable=unsupported-binary-operation return queryset.filter( Q(name__icontains=value) | Q(description__icontains=value) | Q(request_id__icontains=value) ) @@ -181,6 +182,7 @@ def search(self, queryset, name, value): # pylint: disable=unused-argument """Construct Q filter for filterset.""" if not value.strip(): return queryset + # pylint: disable=unsupported-binary-operation return queryset.filter( Q(name__icontains=value) | Q(description__icontains=value) | Q(request_id__icontains=value) ) diff --git a/nautobot_firewall_models/forms.py b/nautobot_firewall_models/forms.py index e0d9768f..d5b1a716 100644 --- a/nautobot_firewall_models/forms.py +++ b/nautobot_firewall_models/forms.py @@ -538,7 +538,6 @@ class PolicyRuleFilterForm(LocalContextFilterForm, NautobotFilterForm): class PolicyRuleForm(LocalContextModelForm, NautobotModelForm): """PolicyRule creation/edit form.""" - name = forms.CharField(required=False, label="Name") tags = DynamicModelMultipleChoiceField(queryset=Tag.objects.all(), required=False) source_users = DynamicModelMultipleChoiceField( queryset=models.UserObject.objects.all(), label="Source User Objects", required=False @@ -710,7 +709,6 @@ class NATPolicyRuleForm(LocalContextModelForm, NautobotModelForm): """NATPolicyRule creation/edit form.""" # Metadata - name = forms.CharField(required=False, label="Name") tags = DynamicModelMultipleChoiceField(queryset=Tag.objects.all(), required=False) request_id = forms.CharField(required=False, label="Optional field for request ticket identifier.") diff --git a/nautobot_firewall_models/migrations/0022_fix_policy_natpolicy_m2ms.py b/nautobot_firewall_models/migrations/0022_fix_policy_natpolicy_m2ms.py new file mode 100644 index 00000000..7257f65f --- /dev/null +++ b/nautobot_firewall_models/migrations/0022_fix_policy_natpolicy_m2ms.py @@ -0,0 +1,54 @@ +# Generated by Django 4.2.16 on 2024-10-03 21:29 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("dcim", "0014_location_status_data_migration"), + ("extras", "0047_enforce_custom_field_slug"), + ("nautobot_firewall_models", "0021_alter_addressobject_status_and_more"), + ] + + operations = [ + migrations.AlterField( + model_name="natpolicy", + name="assigned_devices", + field=models.ManyToManyField( + blank=True, + related_name="nat_policies", + through="nautobot_firewall_models.NATPolicyDeviceM2M", + to="dcim.device", + ), + ), + migrations.AlterField( + model_name="natpolicy", + name="assigned_dynamic_groups", + field=models.ManyToManyField( + blank=True, + related_name="nat_policies", + through="nautobot_firewall_models.NATPolicyDynamicGroupM2M", + to="extras.dynamicgroup", + ), + ), + migrations.AlterField( + model_name="policy", + name="assigned_devices", + field=models.ManyToManyField( + blank=True, + related_name="firewall_policies", + through="nautobot_firewall_models.PolicyDeviceM2M", + to="dcim.device", + ), + ), + migrations.AlterField( + model_name="policy", + name="assigned_dynamic_groups", + field=models.ManyToManyField( + blank=True, + related_name="firewall_policies", + through="nautobot_firewall_models.PolicyDynamicGroupM2M", + to="extras.dynamicgroup", + ), + ), + ] diff --git a/nautobot_firewall_models/models/nat_policy.py b/nautobot_firewall_models/models/nat_policy.py index da4c5285..6b1c2e95 100644 --- a/nautobot_firewall_models/models/nat_policy.py +++ b/nautobot_firewall_models/models/nat_policy.py @@ -252,10 +252,16 @@ class NATPolicy(PrimaryModel): to="nautobot_firewall_models.NATPolicyRule", blank=True, related_name="nat_policies" ) assigned_devices = models.ManyToManyField( - to="dcim.Device", through="NATPolicyDeviceM2M", related_name="nat_policies" + to="dcim.Device", + through="NATPolicyDeviceM2M", + related_name="nat_policies", + blank=True, ) assigned_dynamic_groups = models.ManyToManyField( - to="extras.DynamicGroup", through="NATPolicyDynamicGroupM2M", related_name="nat_policies" + to="extras.DynamicGroup", + through="NATPolicyDynamicGroupM2M", + related_name="nat_policies", + blank=True, ) status = StatusField( on_delete=models.PROTECT, diff --git a/nautobot_firewall_models/models/security_policy.py b/nautobot_firewall_models/models/security_policy.py index 356d6a29..e5fbd711 100644 --- a/nautobot_firewall_models/models/security_policy.py +++ b/nautobot_firewall_models/models/security_policy.py @@ -183,10 +183,16 @@ class Policy(PrimaryModel): name = models.CharField(max_length=100, unique=True) policy_rules = models.ManyToManyField(to=PolicyRule, blank=True, related_name="policies") assigned_devices = models.ManyToManyField( - to="dcim.Device", through="PolicyDeviceM2M", related_name="firewall_policies" + to="dcim.Device", + through="PolicyDeviceM2M", + related_name="firewall_policies", + blank=True, ) assigned_dynamic_groups = models.ManyToManyField( - to="extras.DynamicGroup", through="PolicyDynamicGroupM2M", related_name="firewall_policies" + to="extras.DynamicGroup", + through="PolicyDynamicGroupM2M", + related_name="firewall_policies", + blank=True, ) status = StatusField( on_delete=models.PROTECT, diff --git a/nautobot_firewall_models/templates/nautobot_firewall_models/inc/nat_policy_device_weight.html b/nautobot_firewall_models/templates/nautobot_firewall_models/inc/nat_policy_device_weight.html index 9f6d2cde..a1065717 100644 --- a/nautobot_firewall_models/templates/nautobot_firewall_models/inc/nat_policy_device_weight.html +++ b/nautobot_firewall_models/templates/nautobot_firewall_models/inc/nat_policy_device_weight.html @@ -6,7 +6,7 @@ Assign NAT Policy Weight to Device {% if object.natpolicydevicem2m_set.all %} -
+ {% csrf_token %} diff --git a/nautobot_firewall_models/templates/nautobot_firewall_models/inc/nat_policy_dynamic_group_weight.html b/nautobot_firewall_models/templates/nautobot_firewall_models/inc/nat_policy_dynamic_group_weight.html index 53ccbb3d..4c2c6088 100644 --- a/nautobot_firewall_models/templates/nautobot_firewall_models/inc/nat_policy_dynamic_group_weight.html +++ b/nautobot_firewall_models/templates/nautobot_firewall_models/inc/nat_policy_dynamic_group_weight.html @@ -6,7 +6,7 @@ Assign NAT Policy Weight to Dynamic Group {% if object.natpolicydynamicgroupm2m_set.all %} - + {% csrf_token %}
diff --git a/nautobot_firewall_models/templates/nautobot_firewall_models/inc/policy_dynamic_group_weight.html b/nautobot_firewall_models/templates/nautobot_firewall_models/inc/policy_dynamic_group_weight.html index de3c1a57..f8aee545 100644 --- a/nautobot_firewall_models/templates/nautobot_firewall_models/inc/policy_dynamic_group_weight.html +++ b/nautobot_firewall_models/templates/nautobot_firewall_models/inc/policy_dynamic_group_weight.html @@ -6,7 +6,7 @@ Assign Policy Weight to Dynamic Group {% if object.policydynamicgroupm2m_set.all %} - + {% csrf_token %}
diff --git a/nautobot_firewall_models/utils/capirca.py b/nautobot_firewall_models/utils/capirca.py index 0673978c..ac8f7801 100644 --- a/nautobot_firewall_models/utils/capirca.py +++ b/nautobot_firewall_models/utils/capirca.py @@ -133,18 +133,29 @@ def _check_for_empty(_type, start1, start2, end1, end2): "Capirca does not support the value you provided, and was removed from consideration" ) - def _format_data(self, data, _type): + def _format_data(self, data, _type): # pylint: disable=too-many-statements def format_address(data, name): """Format address objects, looking for the address type.""" - keys = ["ip_range", "fqdn", "prefix", "ip_address"] - for key in keys: - if data.get(key): - value = data[key]["display"] - if not self.address.get(name): - self.address[name] = {key: value} - break + if data.get("ip_range"): + value = data["ip_range"]["display"] + if not self.address.get(name): + self.address[name] = {"ip_range": value} + elif data.get("fqdn"): + value = data["fqdn"]["display"] + if not self.address.get(name): + self.address[name] = {"fqdn": value} + elif data.get("prefix"): + value = data["prefix"]["prefix"] + if not self.address.get(name): + self.address[name] = {"prefix": value} + elif data.get("ip_address"): + value = data["ip_address"]["address"] + if not self.address.get(name): + self.address[name] = {"ip_address": value} else: - raise ValidationError(f"Address object: `{name}` does not have a valid `{str(keys)}` object applied.") + raise ValidationError( + f"Address object: `{name}` does not have a valid `ip_range`, `fqdn`, `prefix`, or `ip_address` object applied." + ) def format_address_group(data, name): """Format address group objects, also adding the address objects as new ones are found.""" diff --git a/nautobot_firewall_models/viewsets/nat_policy.py b/nautobot_firewall_models/viewsets/nat_policy.py index 1d437aa2..306b91b2 100644 --- a/nautobot_firewall_models/viewsets/nat_policy.py +++ b/nautobot_firewall_models/viewsets/nat_policy.py @@ -78,10 +78,10 @@ def devices(self, request, pk, *args, **kwargs): form_data = dict(request.POST) form_data.pop("csrfmiddlewaretoken", None) for device, weight in form_data.items(): - m2m = NATPolicyDeviceM2M.objects.get(device=device, policy=pk) + m2m = NATPolicyDeviceM2M.objects.get(device=device, nat_policy=pk) m2m.weight = weight[0] m2m.validated_save() - return redirect(reverse("plugins:nautobot_firewall_models:nat_policy", kwargs={"pk": pk})) + return redirect(reverse("plugins:nautobot_firewall_models:natpolicy", kwargs={"pk": pk})) @action(detail=True, methods=["post"]) def dynamic_groups(self, request, pk, *args, **kwargs): @@ -90,10 +90,10 @@ def dynamic_groups(self, request, pk, *args, **kwargs): form_data = dict(request.POST) form_data.pop("csrfmiddlewaretoken", None) for group, weight in form_data.items(): - m2m = NATPolicyDynamicGroupM2M.objects.get(dynamic_group=group, policy=pk) + m2m = NATPolicyDynamicGroupM2M.objects.get(dynamic_group=group, nat_policy=pk) m2m.weight = weight[0] m2m.validated_save() - return redirect(reverse("plugins:nautobot_firewall_models:nat_policy", kwargs={"pk": pk})) + return redirect(reverse("plugins:nautobot_firewall_models:natpolicy", kwargs={"pk": pk})) def get_queryset(self): """Overload to overwrite permissiosn action map."""