From 1bd67d5509b80e2b90354fc58400cd836918e03e Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Thu, 26 Jan 2023 14:35:27 +0000 Subject: [PATCH 01/42] html template deduplication --- .../netbox_acls/accesslist_edit.html | 13 +-- .../netbox_acls/aclextendedrule.html | 83 +----------------- .../aclinterfaceassignment_edit.html | 13 +-- .../templates/netbox_acls/aclrule.html | 84 +++++++++++++++++++ .../netbox_acls/aclstandardrule.html | 65 +------------- .../templates/netbox_acls/common_edit.html | 13 +++ 6 files changed, 101 insertions(+), 170 deletions(-) create mode 100644 netbox_acls/templates/netbox_acls/aclrule.html create mode 100644 netbox_acls/templates/netbox_acls/common_edit.html diff --git a/netbox_acls/templates/netbox_acls/accesslist_edit.html b/netbox_acls/templates/netbox_acls/accesslist_edit.html index a197daad..ae1590df 100644 --- a/netbox_acls/templates/netbox_acls/accesslist_edit.html +++ b/netbox_acls/templates/netbox_acls/accesslist_edit.html @@ -74,16 +74,5 @@

Host Assignment

-
-

Comments

- {% render_field form.comments %} -
-{% if form.custom_fields %} -
-
Custom Fields
-
- {% render_custom_fields form %} -
-
-{% endif %} +{% include "./common_edit.html" %} {% endblock %} diff --git a/netbox_acls/templates/netbox_acls/aclextendedrule.html b/netbox_acls/templates/netbox_acls/aclextendedrule.html index d4abbdfd..55de411c 100644 --- a/netbox_acls/templates/netbox_acls/aclextendedrule.html +++ b/netbox_acls/templates/netbox_acls/aclextendedrule.html @@ -1,82 +1 @@ -{% extends 'generic/object.html' %} - -{% block content %} -
-
-
-
ACL Extended Rule
-
- - - - - - - - - - - - - - -
ACL Extended Rule
Access List - {{ object.access_list }} -
Description{{ object.description|placeholder }}
Index{{ object.index }}
-
-
- {% include 'inc/panels/custom_fields.html' %} - {% include 'inc/panels/tags.html' %} -
-
-
-
Details
-
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Details
Remark{{ object.get_remark_display|placeholder }}
Protocol{{ object.get_protocol_display|placeholder }}
Source Prefix - {% if object.source_prefix %} - {{ object.source_prefix }} - {% else %} - {{ ''|placeholder }} - {% endif %} -
Source Ports{{ object.source_ports|join:", "|placeholder }}
Destination Prefix - {% if object.destination_prefix %} - {{ object.destination_prefix }} - {% else %} - {{ ''|placeholder }} - {% endif %} -
Destination Ports{{ object.destination_ports|join:", "|placeholder }}
Action{% badge object.get_action_display bg_color=object.get_action_color %}
-
-
-
-
-{% endblock content %} +{% include "./aclrule.html" %} diff --git a/netbox_acls/templates/netbox_acls/aclinterfaceassignment_edit.html b/netbox_acls/templates/netbox_acls/aclinterfaceassignment_edit.html index 4eae6f0b..a8fa27c1 100644 --- a/netbox_acls/templates/netbox_acls/aclinterfaceassignment_edit.html +++ b/netbox_acls/templates/netbox_acls/aclinterfaceassignment_edit.html @@ -53,16 +53,5 @@

Interface Assignment

-
-

Comments

- {% render_field form.comments %} -
-{% if form.custom_fields %} -
-
Custom Fields
-
- {% render_custom_fields form %} -
-
-{% endif %} +{% include "./common_edit.html" %} {% endblock %} diff --git a/netbox_acls/templates/netbox_acls/aclrule.html b/netbox_acls/templates/netbox_acls/aclrule.html new file mode 100644 index 00000000..7ef1278e --- /dev/null +++ b/netbox_acls/templates/netbox_acls/aclrule.html @@ -0,0 +1,84 @@ +{% extends 'generic/object.html' %} + +{% block content %} +
+
+
+
{{ object|meta:"verbose_name"|bettertitle }}
+
+ + + + + + + + + + + + + + +
{{ object|meta:"verbose_name"|bettertitle }}
Access List + {{ object.access_list }} +
Index{{ object.index }}
Description{{ object.description|placeholder }}
+
+
+ {% include 'inc/panels/custom_fields.html' %} + {% include 'inc/panels/tags.html' %} +
+
+
+
Details
+
+ + + + + + + + + + + + + + + {% if "extended" in object|meta:"verbose_name"|lower %} + + + + + + + + + + + + + {% endif %} + + + + +
Details
Remark{{ object.get_remark_display|placeholder }}
Protocol{{ object.get_protocol_display|placeholder }}
Source Prefix + {% if object.source_prefix %} + {{ object.source_prefix }} + {% else %} + {{ ''|placeholder }} + {% endif %} +
Source Ports{{ object.source_ports|join:", "|placeholder }}
Destination Prefix + {% if object.destination_prefix %} + {{ object.destination_prefix }} + {% else %} + {{ ''|placeholder }} + {% endif %} +
Destination Ports{{ object.destination_ports|join:", "|placeholder }}
Action{% badge object.get_action_display bg_color=object.get_action_color %}
+
+
+
+
+{% endblock content %} diff --git a/netbox_acls/templates/netbox_acls/aclstandardrule.html b/netbox_acls/templates/netbox_acls/aclstandardrule.html index c8e25625..55de411c 100644 --- a/netbox_acls/templates/netbox_acls/aclstandardrule.html +++ b/netbox_acls/templates/netbox_acls/aclstandardrule.html @@ -1,64 +1 @@ -{% extends 'generic/object.html' %} - -{% block content %} -
-
-
-
ACL Standard Rule
-
- - - - - - - - - - - - - - -
ACL Standard Rule
Access List - {{ object.access_list }} -
Index{{ object.index }}
Description{{ object.description|placeholder }}
-
-
- {% include 'inc/panels/custom_fields.html' %} - {% include 'inc/panels/tags.html' %} -
-
-
-
Details
-
- - - - - - - - - - - - - - - - - - -
Details
Remark{{ object.get_remark_display|placeholder }}
Protocol{{ object.get_protocol_display|placeholder }}
Source Prefix - {% if object.source_prefix %} - {{ object.source_prefix }} - {% else %} - {{ ''|placeholder }} - {% endif %} -
Action{% badge object.get_action_display bg_color=object.get_action_color %}
-
-
-
-
-{% endblock content %} +{% include "./aclrule.html" %} diff --git a/netbox_acls/templates/netbox_acls/common_edit.html b/netbox_acls/templates/netbox_acls/common_edit.html new file mode 100644 index 00000000..201aab69 --- /dev/null +++ b/netbox_acls/templates/netbox_acls/common_edit.html @@ -0,0 +1,13 @@ +{% load form_helpers %} +
+

Comments

+ {% render_field form.comments %} +
+{% if form.custom_fields %} +
+
Custom Fields
+
+ {% render_custom_fields form %} +
+
+{% endif %} \ No newline at end of file From 182b1c7b037df8cb901fe31a5dd1b2b59278a2bd Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Thu, 26 Jan 2023 14:36:25 +0000 Subject: [PATCH 02/42] abstract classes to minimize code duplication --- netbox_acls/forms/filtersets.py | 136 ++++++++++-------------- netbox_acls/models/access_list_rules.py | 16 +-- 2 files changed, 66 insertions(+), 86 deletions(-) diff --git a/netbox_acls/forms/filtersets.py b/netbox_acls/forms/filtersets.py index f227e3f7..42eaa2eb 100644 --- a/netbox_acls/forms/filtersets.py +++ b/netbox_acls/forms/filtersets.py @@ -27,6 +27,7 @@ ACLExtendedRule, ACLInterfaceAssignment, ACLStandardRule, + BaseACLRule, ) __all__ = ( @@ -37,11 +38,14 @@ ) -class AccessListFilterForm(NetBoxModelFilterSetForm): +class BaseACLFilterForm(NetBoxModelFilterSetForm): """ - GUI filter form to search the django AccessList model. + GUI filter inherited base form to search the django ACL and ACL Interface Assignment models. """ + class Meta: + abstract = True + model = AccessList region = DynamicModelChoiceField( queryset=Region.objects.all(), @@ -70,6 +74,14 @@ class AccessListFilterForm(NetBoxModelFilterSetForm): queryset=VirtualMachine.objects.all(), required=False, ) + + +class AccessListFilterForm(BaseACLFilterForm): + """ + GUI filter form to search the django AccessList model. + """ + + model = AccessList virtual_chassis = DynamicModelChoiceField( queryset=VirtualChassis.objects.all(), required=False, @@ -94,43 +106,20 @@ class AccessListFilterForm(NetBoxModelFilterSetForm): "site_group", "site", "device", - "virtual_chassis", "virtual_machine", + "virtual_chassis", ), ), ("ACL Details", ("type", "default_action")), ) -class ACLInterfaceAssignmentFilterForm(NetBoxModelFilterSetForm): +class ACLInterfaceAssignmentFilterForm(BaseACLFilterForm): """ GUI filter form to search the django AccessList model. """ model = ACLInterfaceAssignment - region = DynamicModelChoiceField( - queryset=Region.objects.all(), - required=False, - ) - site_group = DynamicModelChoiceField( - queryset=SiteGroup.objects.all(), - required=False, - label="Site Group", - ) - site = DynamicModelChoiceField( - queryset=Site.objects.all(), - required=False, - query_params={"region_id": "$region", "group_id": "$site_group"}, - ) - device = DynamicModelChoiceField( - queryset=Device.objects.all(), - query_params={ - "region_id": "$region", - "group_id": "$site_group", - "site_id": "$site", - }, - required=False, - ) interface = DynamicModelChoiceField( queryset=Interface.objects.all(), required=False, @@ -138,25 +127,13 @@ class ACLInterfaceAssignmentFilterForm(NetBoxModelFilterSetForm): "device_id": "$device", }, ) - virtual_machine = DynamicModelChoiceField( - queryset=VirtualMachine.objects.all(), - required=False, - label="Virtual Machine", - ) vminterface = DynamicModelChoiceField( queryset=VMInterface.objects.all(), required=False, query_params={ "virtual_machine_id": "$virtual_machine", }, - label="Interface", - ) - access_list = DynamicModelChoiceField( - queryset=AccessList.objects.all(), - query_params={ - "assigned_object": "$device", - }, - label="Access List", + label="VM Interface", ) direction = ChoiceField( choices=add_blank_choice(ACLAssignmentDirectionChoices), @@ -164,63 +141,71 @@ class ACLInterfaceAssignmentFilterForm(NetBoxModelFilterSetForm): ) tag = TagFilterField(model) - # fieldsets = ( - # (None, ('q', 'tag')), - # ('Host Details', ('region', 'site_group', 'site', 'device')), - # ('ACL Details', ('type', 'default_action')), - # ) + fieldsets = ( + (None, ("q", "tag")), + ( + "Host Details", + ( + "region", + "site_group", + "site", + "device", + "virtual_machine", + ), + ), + ("Interface Details", ("interface", "vminterface")), + ) -class ACLStandardRuleFilterForm(NetBoxModelFilterSetForm): +class BaseACLRuleFilterForm(NetBoxModelFilterSetForm): """ - GUI filter form to search the django ACLStandardRule model. + GUI filter inherited base form to search the django ACL rule models. """ - model = ACLStandardRule - tag = TagFilterField(model) + class Meta: + abstract = True + + model = BaseACLRule + index = forms.IntegerField( + required=False, + ) access_list = DynamicModelMultipleChoiceField( queryset=AccessList.objects.all(), required=False, ) + action = ChoiceField( + choices=add_blank_choice(ACLRuleActionChoices), + required=False, + ) source_prefix = DynamicModelMultipleChoiceField( queryset=Prefix.objects.all(), required=False, label="Source Prefix", ) - action = ChoiceField( - choices=add_blank_choice(ACLRuleActionChoices), - required=False, - ) fieldsets = ( (None, ("q", "tag")), ("Rule Details", ("access_list", "action", "source_prefix")), ) -class ACLExtendedRuleFilterForm(NetBoxModelFilterSetForm): +class ACLStandardRuleFilterForm(BaseACLRuleFilterForm): + """ + GUI filter form to search the django ACLStandardRule model. + """ + + model = ACLStandardRule + + tag = TagFilterField(model) + + +class ACLExtendedRuleFilterForm(BaseACLRuleFilterForm): """ GUI filter form to search the django ACLExtendedRule model. """ model = ACLExtendedRule - index = forms.IntegerField( - required=False, - ) tag = TagFilterField(model) - access_list = DynamicModelMultipleChoiceField( - queryset=AccessList.objects.all(), - required=False, - ) - action = ChoiceField( - choices=add_blank_choice(ACLRuleActionChoices), - required=False, - ) - source_prefix = DynamicModelMultipleChoiceField( - queryset=Prefix.objects.all(), - required=False, - label="Source Prefix", - ) - desintation_prefix = DynamicModelMultipleChoiceField( + destination_prefix = DynamicModelMultipleChoiceField( queryset=Prefix.objects.all(), required=False, label="Destination Prefix", @@ -229,16 +214,11 @@ class ACLExtendedRuleFilterForm(NetBoxModelFilterSetForm): choices=add_blank_choice(ACLProtocolChoices), required=False, ) - - fieldsets = ( - (None, ("q", "tag")), + fieldsets = BaseACLRuleFilterForm.fieldsets + ( ( "Rule Details", ( - "access_list", - "action", - "source_prefix", - "desintation_prefix", + "destination_prefix", "protocol", ), ), diff --git a/netbox_acls/models/access_list_rules.py b/netbox_acls/models/access_list_rules.py index 56567204..7df6be87 100644 --- a/netbox_acls/models/access_list_rules.py +++ b/netbox_acls/models/access_list_rules.py @@ -12,13 +12,13 @@ from .access_lists import AccessList __all__ = ( - "ACLRule", + "BaseACLRule", "ACLStandardRule", "ACLExtendedRule", ) -class ACLRule(NetBoxModel): +class BaseACLRule(NetBoxModel): """ Abstract model for ACL Rules. Inherrited by both ACLStandardRule and ACLExtendedRule. @@ -77,9 +77,9 @@ class Meta: unique_together = ["access_list", "index"] -class ACLStandardRule(ACLRule): +class ACLStandardRule(BaseACLRule): """ - Inherits ACLRule. + Inherits BaseACLRule. """ access_list = models.ForeignKey( @@ -101,7 +101,7 @@ def get_absolute_url(self): def get_prerequisite_models(cls): return [AccessList] - class Meta(ACLRule.Meta): + class Meta(BaseACLRule.Meta): """ Define the model properties adding to or overriding the inherited class: - default_related_name for any FK relationships @@ -113,9 +113,9 @@ class Meta(ACLRule.Meta): verbose_name_plural = "ACL Standard Rules" -class ACLExtendedRule(ACLRule): +class ACLExtendedRule(BaseACLRule): """ - Inherits ACLRule. + Inherits BaseACLRule. Add ACLExtendedRule specific fields: source_ports, desintation_prefix, destination_ports, and protocol """ @@ -166,7 +166,7 @@ def get_protocol_color(self): def get_prerequisite_models(cls): return [apps.get_model("ipam.Prefix"), AccessList] - class Meta(ACLRule.Meta): + class Meta(BaseACLRule.Meta): """ Define the model properties adding to or overriding the inherited class: - default_related_name for any FK relationships From e198cf64d2ab02ecbf6ad3fb62d66edd0f7b7d6e Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Thu, 26 Jan 2023 15:05:45 +0000 Subject: [PATCH 03/42] add end of file new line --- netbox_acls/templates/netbox_acls/common_edit.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox_acls/templates/netbox_acls/common_edit.html b/netbox_acls/templates/netbox_acls/common_edit.html index 201aab69..c2963581 100644 --- a/netbox_acls/templates/netbox_acls/common_edit.html +++ b/netbox_acls/templates/netbox_acls/common_edit.html @@ -10,4 +10,4 @@
Custom Fields
{% render_custom_fields form %} -{% endif %} \ No newline at end of file +{% endif %} From 2abc675b15f4ca13f98fb67bc320f71b1b38d0de Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Thu, 26 Jan 2023 15:14:48 +0000 Subject: [PATCH 04/42] move included templates to inc folder --- netbox_acls/templates/{netbox_acls => inc}/aclrule.html | 0 netbox_acls/templates/{netbox_acls => inc}/common_edit.html | 0 netbox_acls/templates/netbox_acls/accesslist_edit.html | 2 +- netbox_acls/templates/netbox_acls/aclextendedrule.html | 2 +- .../templates/netbox_acls/aclinterfaceassignment_edit.html | 2 +- netbox_acls/templates/netbox_acls/aclstandardrule.html | 2 +- 6 files changed, 4 insertions(+), 4 deletions(-) rename netbox_acls/templates/{netbox_acls => inc}/aclrule.html (100%) rename netbox_acls/templates/{netbox_acls => inc}/common_edit.html (100%) diff --git a/netbox_acls/templates/netbox_acls/aclrule.html b/netbox_acls/templates/inc/aclrule.html similarity index 100% rename from netbox_acls/templates/netbox_acls/aclrule.html rename to netbox_acls/templates/inc/aclrule.html diff --git a/netbox_acls/templates/netbox_acls/common_edit.html b/netbox_acls/templates/inc/common_edit.html similarity index 100% rename from netbox_acls/templates/netbox_acls/common_edit.html rename to netbox_acls/templates/inc/common_edit.html diff --git a/netbox_acls/templates/netbox_acls/accesslist_edit.html b/netbox_acls/templates/netbox_acls/accesslist_edit.html index ae1590df..4edd5763 100644 --- a/netbox_acls/templates/netbox_acls/accesslist_edit.html +++ b/netbox_acls/templates/netbox_acls/accesslist_edit.html @@ -74,5 +74,5 @@

Host Assignment

-{% include "./common_edit.html" %} +{% include "inc/common_edit.html" %} {% endblock %} diff --git a/netbox_acls/templates/netbox_acls/aclextendedrule.html b/netbox_acls/templates/netbox_acls/aclextendedrule.html index 55de411c..3749a5e7 100644 --- a/netbox_acls/templates/netbox_acls/aclextendedrule.html +++ b/netbox_acls/templates/netbox_acls/aclextendedrule.html @@ -1 +1 @@ -{% include "./aclrule.html" %} +{% include "inc/aclrule.html" %} diff --git a/netbox_acls/templates/netbox_acls/aclinterfaceassignment_edit.html b/netbox_acls/templates/netbox_acls/aclinterfaceassignment_edit.html index a8fa27c1..7f7c255e 100644 --- a/netbox_acls/templates/netbox_acls/aclinterfaceassignment_edit.html +++ b/netbox_acls/templates/netbox_acls/aclinterfaceassignment_edit.html @@ -53,5 +53,5 @@

Interface Assignment

-{% include "./common_edit.html" %} +{% include "inc/common_edit.html" %} {% endblock %} diff --git a/netbox_acls/templates/netbox_acls/aclstandardrule.html b/netbox_acls/templates/netbox_acls/aclstandardrule.html index 55de411c..3749a5e7 100644 --- a/netbox_acls/templates/netbox_acls/aclstandardrule.html +++ b/netbox_acls/templates/netbox_acls/aclstandardrule.html @@ -1 +1 @@ -{% include "./aclrule.html" %} +{% include "inc/aclrule.html" %} From 976542dc9b620cd17457ee31cb16ea929916f6bb Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 14:53:32 +0000 Subject: [PATCH 05/42] add docstrings everywhere --- netbox_acls/api/serializers.py | 23 ++++--- netbox_acls/forms/filtersets.py | 8 +++ netbox_acls/forms/models.py | 79 +++++++++++++++++-------- netbox_acls/models/access_list_rules.py | 18 ++++++ netbox_acls/models/access_lists.py | 23 +++++++ netbox_acls/tables.py | 16 +++++ netbox_acls/tests/test_api.py | 14 ++++- netbox_acls/views.py | 60 +++++++++++++++++++ 8 files changed, 207 insertions(+), 34 deletions(-) diff --git a/netbox_acls/api/serializers.py b/netbox_acls/api/serializers.py index b84a0bd5..10da14a9 100644 --- a/netbox_acls/api/serializers.py +++ b/netbox_acls/api/serializers.py @@ -29,18 +29,19 @@ "ACLExtendedRuleSerializer", ] +# TODO: Check Constants across codebase for consistency. # Sets a standard error message for ACL rules with an action of remark, but no remark set. -error_message_no_remark = "Action is set to remark, you MUST add a remark." +ERROR_MESSAGE_NO_REMARK = "Action is set to remark, you MUST add a remark." # Sets a standard error message for ACL rules with an action of remark, but no source_prefix is set. -error_message_action_remark_source_prefix_set = ( +ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET = ( "Action is set to remark, Source Prefix CANNOT be set." ) # Sets a standard error message for ACL rules with an action not set to remark, but no remark is set. -error_message_remark_without_action_remark = ( +ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK = ( "CANNOT set remark unless action is set to remark." ) # Sets a standard error message for ACL rules no associated to an ACL of the same type. -error_message_acl_type = "Provided parent Access List is not of right type." +ERROR_MESSAGE_ACL_TYPE = "Provided parent Access List is not of right type." class AccessListSerializer(NetBoxModelSerializer): @@ -83,6 +84,9 @@ class Meta: @swagger_serializer_method(serializer_or_field=serializers.DictField) def get_assigned_object(self, obj): + """ + Returns the assigned object for the Access List. + """ serializer = get_serializer_for_model( obj.assigned_object, prefix=NESTED_SERIALIZER_PREFIX, @@ -166,6 +170,9 @@ class Meta: @swagger_serializer_method(serializer_or_field=serializers.DictField) def get_assigned_object(self, obj): + """ + Returns the assigned object for the ACLInterfaceAssignment. + """ serializer = get_serializer_for_model( obj.assigned_object, prefix=NESTED_SERIALIZER_PREFIX, @@ -272,11 +279,11 @@ def validate(self, data): # Check if action set to remark, but no remark set. if data.get("action") == "remark" and data.get("remark") is None: - error_message["remark"] = [error_message_no_remark] + error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] # Check if action set to remark, but source_prefix set. if data.get("source_prefix"): error_message["source_prefix"] = [ - error_message_action_remark_source_prefix_set, + ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, ] if error_message: @@ -346,11 +353,11 @@ def validate(self, data): # Check if action set to remark, but no remark set. if data.get("action") == "remark" and data.get("remark") is None: - error_message["remark"] = [error_message_no_remark] + error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] # Check if action set to remark, but source_prefix set. if data.get("source_prefix"): error_message["source_prefix"] = [ - error_message_action_remark_source_prefix_set, + ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, ] # Check if action set to remark, but source_ports set. if data.get("source_ports"): diff --git a/netbox_acls/forms/filtersets.py b/netbox_acls/forms/filtersets.py index 42eaa2eb..7767e9a0 100644 --- a/netbox_acls/forms/filtersets.py +++ b/netbox_acls/forms/filtersets.py @@ -44,6 +44,10 @@ class BaseACLFilterForm(NetBoxModelFilterSetForm): """ class Meta: + """ + Sets the parent class as an abstract class to be inherited by other classes. + """ + abstract = True model = AccessList @@ -163,6 +167,10 @@ class BaseACLRuleFilterForm(NetBoxModelFilterSetForm): """ class Meta: + """ + Sets the parent class as an abstract class to be inherited by other classes. + """ + abstract = True model = BaseACLRule diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 4dc17384..99278d98 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -37,20 +37,20 @@ "*Note: CANNOT be set if action is set to remark.", ) # Sets a standard help_text value to be used by the various classes for acl action -help_text_acl_action = "Action the rule will take (remark, deny, or allow)." +HELP_TEXT_ACL_ACTION = "Action the rule will take (remark, deny, or allow)." # Sets a standard help_text value to be used by the various classes for acl index -help_text_acl_rule_index = ( +HELP_TEXT_ACL_RULE_INDEX = ( "Determines the order of the rule in the ACL processing. AKA Sequence Number." ) # Sets a standard error message for ACL rules with an action of remark, but no remark set. -error_message_no_remark = "Action is set to remark, you MUST add a remark." +ERROR_MESSAGE_NO_REMARK = "Action is set to remark, you MUST add a remark." # Sets a standard error message for ACL rules with an action of remark, but no source_prefix is set. -error_message_action_remark_source_prefix_set = ( +ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET = ( "Action is set to remark, Source Prefix CANNOT be set." ) # Sets a standard error message for ACL rules with an action not set to remark, but no remark is set. -error_message_remark_without_action_remark = ( +ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK = ( "CANNOT set remark unless action is set to remark." ) @@ -133,6 +133,10 @@ class AccessListForm(NetBoxModelForm): comments = CommentField() class Meta: + """ + Defines the Model and fields to be used by the form. + """ + model = AccessList fields = ( "region", @@ -156,16 +160,19 @@ class Meta: } def __init__(self, *args, **kwargs): + """ + Initializes the form + """ # Initialize helper selectors instance = kwargs.get("instance") initial = kwargs.get("initial", {}).copy() if instance: - if type(instance.assigned_object) is Device: + if isinstance(instance.assigned_object, Device): initial["device"] = instance.assigned_object - elif type(instance.assigned_object) is VirtualChassis: + elif isinstance(instance.assigned_object, VirtualChassis): initial["virtual_chassis"] = instance.assigned_object - elif type(instance.assigned_object) is VirtualMachine: + elif isinstance(instance.assigned_object, VirtualMachine): initial["virtual_machine"] = instance.assigned_object kwargs["initial"] = initial @@ -231,12 +238,16 @@ def clean(self): host_type: [error_same_acl_name], "name": [error_same_acl_name], } + # Check if Access List has no existing rules before change the Access List's type. if self.instance.pk: - # Check if Access List has no existing rules before change the Access List's type. if ( acl_type == ACLTypeChoices.TYPE_EXTENDED and self.instance.aclstandardrules.exists() - ) or ( + ): + error_message["type"] = [ + "This ACL has ACL rules associated, CANNOT change ACL type.", + ] + elif ( acl_type == ACLTypeChoices.TYPE_STANDARD and self.instance.aclextendedrules.exists() ): @@ -250,7 +261,9 @@ def clean(self): return cleaned_data def save(self, *args, **kwargs): - # Set assigned object + """ + Set assigned object + """ self.instance.assigned_object = ( self.cleaned_data.get("device") or self.cleaned_data.get("virtual_chassis") @@ -315,15 +328,17 @@ class ACLInterfaceAssignmentForm(NetBoxModelForm): comments = CommentField() def __init__(self, *args, **kwargs): + """ + Initialize helper selectors + """ - # Initialize helper selectors instance = kwargs.get("instance") initial = kwargs.get("initial", {}).copy() if instance: - if type(instance.assigned_object) is Interface: + if isinstance(instance.assigned_object, Interface): initial["interface"] = instance.assigned_object initial["device"] = "device" - elif type(instance.assigned_object) is VMInterface: + elif isinstance(instance.assigned_object, VMInterface): initial["vminterface"] = instance.assigned_object initial["virtual_machine"] = "virtual_machine" kwargs["initial"] = initial @@ -331,6 +346,10 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) class Meta: + """ + Defines the Model and fields to be used by the form. + """ + model = ACLInterfaceAssignment fields = ( "access_list", @@ -441,7 +460,9 @@ def clean(self): return cleaned_data def save(self, *args, **kwargs): - # Set assigned object + """ + Set assigned object + """ self.instance.assigned_object = self.cleaned_data.get( "interface", ) or self.cleaned_data.get("vminterface") @@ -478,6 +499,10 @@ class ACLStandardRuleForm(NetBoxModelForm): ) class Meta: + """ + Defines the Model and fields to be used by the form. + """ + model = ACLStandardRule fields = ( "access_list", @@ -489,8 +514,8 @@ class Meta: "description", ) help_texts = { - "index": help_text_acl_rule_index, - "action": help_text_acl_action, + "index": HELP_TEXT_ACL_RULE_INDEX, + "action": HELP_TEXT_ACL_ACTION, "remark": mark_safe( "*Note: CANNOT be set if source prefix OR action is set.", ), @@ -511,15 +536,15 @@ def clean(self): if cleaned_data.get("action") == "remark": # Check if action set to remark, but no remark set. if not cleaned_data.get("remark"): - error_message["remark"] = [error_message_no_remark] + error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] # Check if action set to remark, but source_prefix set. if cleaned_data.get("source_prefix"): error_message["source_prefix"] = [ - error_message_action_remark_source_prefix_set, + ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, ] # Check remark set, but action not set to remark. elif cleaned_data.get("remark"): - error_message["remark"] = [error_message_remark_without_action_remark] + error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] if error_message: raise forms.ValidationError(error_message) @@ -574,6 +599,10 @@ class ACLExtendedRuleForm(NetBoxModelForm): ) class Meta: + """ + Defines the Model and fields to be used by the form. + """ + model = ACLExtendedRule fields = ( "access_list", @@ -589,9 +618,9 @@ class Meta: "description", ) help_texts = { - "action": help_text_acl_action, + "action": HELP_TEXT_ACL_ACTION, "destination_ports": help_text_acl_rule_logic, - "index": help_text_acl_rule_index, + "index": HELP_TEXT_ACL_RULE_INDEX, "protocol": help_text_acl_rule_logic, "remark": mark_safe( "*Note: CANNOT be set if action is not set to remark.", @@ -619,11 +648,11 @@ def clean(self): if cleaned_data.get("action") == "remark": # Check if action set to remark, but no remark set. if not cleaned_data.get("remark"): - error_message["remark"] = [error_message_no_remark] + error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] # Check if action set to remark, but source_prefix set. if cleaned_data.get("source_prefix"): error_message["source_prefix"] = [ - error_message_action_remark_source_prefix_set, + ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, ] # Check if action set to remark, but source_ports set. if cleaned_data.get("source_ports"): @@ -647,7 +676,7 @@ def clean(self): ] # Check if action not set to remark, but remark set. elif cleaned_data.get("remark"): - error_message["remark"] = [error_message_remark_without_action_remark] + error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] if error_message: raise forms.ValidationError(error_message) diff --git a/netbox_acls/models/access_list_rules.py b/netbox_acls/models/access_list_rules.py index 7df6be87..5ec96127 100644 --- a/netbox_acls/models/access_list_rules.py +++ b/netbox_acls/models/access_list_rules.py @@ -55,13 +55,22 @@ class BaseACLRule(NetBoxModel): clone_fields = ("access_list", "action", "source_prefix") def __str__(self): + """ + Return a string representation of the model. + """ return f"{self.access_list}: Rule {self.index}" def get_action_color(self): + """ + Return the color for the action. + """ return ACLRuleActionChoices.colors.get(self.action) @classmethod def get_prerequisite_models(cls): + """ + Return a list of prerequisite models for this model. + """ return [apps.get_model("ipam.Prefix"), AccessList] class Meta: @@ -99,6 +108,9 @@ def get_absolute_url(self): @classmethod def get_prerequisite_models(cls): + """ + Return a list of prerequisite models for this model. + """ return [AccessList] class Meta(BaseACLRule.Meta): @@ -160,10 +172,16 @@ def get_absolute_url(self): return reverse("plugins:netbox_acls:aclextendedrule", args=[self.pk]) def get_protocol_color(self): + """ + Return the color for the protocol. + """ return ACLProtocolChoices.colors.get(self.protocol) @classmethod def get_prerequisite_models(cls): + """ + Return a list of prerequisite models for this model. + xs""" return [apps.get_model("ipam.Prefix"), AccessList] class Meta(BaseACLRule.Meta): diff --git a/netbox_acls/models/access_lists.py b/netbox_acls/models/access_lists.py index 1feb6353..40a59bb4 100644 --- a/netbox_acls/models/access_lists.py +++ b/netbox_acls/models/access_lists.py @@ -65,12 +65,19 @@ class AccessList(NetBoxModel): ) class Meta: + """ + Define the ordering, unique constraints, and verbose names for the model. + """ + unique_together = ["assigned_object_type", "assigned_object_id", "name"] ordering = ["assigned_object_type", "assigned_object_id", "name"] verbose_name = "Access List" verbose_name_plural = "Access Lists" def __str__(self): + """ + Return a string representation of the model. + """ return self.name def get_absolute_url(self): @@ -81,9 +88,15 @@ def get_absolute_url(self): return reverse("plugins:netbox_acls:accesslist", args=[self.pk]) def get_default_action_color(self): + """ + Define the color of the default action. + """ return ACLActionChoices.colors.get(self.default_action) def get_type_color(self): + """ + Define the color of the type. + """ return ACLTypeChoices.colors.get(self.type) @@ -121,6 +134,10 @@ class ACLInterfaceAssignment(NetBoxModel): clone_fields = ("access_list", "direction") class Meta: + """ + Define the ordering, unique constraints, and verbose names for the model. + """ + unique_together = [ "assigned_object_type", "assigned_object_id", @@ -148,9 +165,15 @@ def get_absolute_url(self): @classmethod def get_prerequisite_models(cls): + """ + Return a list of models that must be defined before this model. + """ return [AccessList] def get_direction_color(self): + """ + Define the color of the direction. + """ return ACLAssignmentDirectionChoices.colors.get(self.direction) diff --git a/netbox_acls/tables.py b/netbox_acls/tables.py index 8552d789..a47988f6 100644 --- a/netbox_acls/tables.py +++ b/netbox_acls/tables.py @@ -54,6 +54,10 @@ class AccessListTable(NetBoxTable): ) class Meta(NetBoxTable.Meta): + """ + Defines the table view for the AccessList model. + """ + model = AccessList fields = ( "pk", @@ -103,6 +107,10 @@ class ACLInterfaceAssignmentTable(NetBoxTable): ) class Meta(NetBoxTable.Meta): + """ + Defines the table view for the ACLInterfaceAssignment model. + """ + model = ACLInterfaceAssignment fields = ( "pk", @@ -140,6 +148,10 @@ class ACLStandardRuleTable(NetBoxTable): ) class Meta(NetBoxTable.Meta): + """ + Defines the table view for the ACLStandardRule model. + """ + model = ACLStandardRule fields = ( "pk", @@ -182,6 +194,10 @@ class ACLExtendedRuleTable(NetBoxTable): protocol = ChoiceFieldColumn() class Meta(NetBoxTable.Meta): + """ + Defines the table view for the ACLExtendedRule model. + """ + model = ACLExtendedRule fields = ( "pk", diff --git a/netbox_acls/tests/test_api.py b/netbox_acls/tests/test_api.py index afbd4fa4..94bc2e33 100644 --- a/netbox_acls/tests/test_api.py +++ b/netbox_acls/tests/test_api.py @@ -9,7 +9,14 @@ class AppTest(APITestCase): + """ + Test the API root + """ + def test_root(self): + """ + Test the API root + """ url = reverse("plugins-api:netbox_acls-api:api-root") response = self.client.get(f"{url}?format=api", **self.header) @@ -23,7 +30,9 @@ class ACLTestCase( APIViewTestCases.UpdateObjectViewTestCase, APIViewTestCases.DeleteObjectViewTestCase, ): - """Test the AccessList Test""" + """ + Test the AccessList Test + """ model = AccessList view_namespace = "plugins-api:netbox_acls" @@ -31,6 +40,9 @@ class ACLTestCase( @classmethod def setUpTestData(cls): + """ + Create a test site, manufacturer, device type, device role, and device + """ site = Site.objects.create(name="Site 1", slug="site-1") manufacturer = Manufacturer.objects.create( name="Manufacturer 1", diff --git a/netbox_acls/views.py b/netbox_acls/views.py index d9869450..95eca55e 100644 --- a/netbox_acls/views.py +++ b/netbox_acls/views.py @@ -104,6 +104,10 @@ class AccessListDeleteView(generic.ObjectDeleteView): class AccessListBulkDeleteView(generic.BulkDeleteView): + """ + Defines the bulk delete view for the AccessLists django model. + """ + queryset = models.AccessList.objects.prefetch_related("tags") filterset = filtersets.AccessListFilterSet table = tables.AccessListTable @@ -120,6 +124,9 @@ class AccessListChildView(generic.ObjectChildrenView): template_name = "inc/view_tab.html" def get_extra_context(self, request, instance): + """ + Returns the extra context for the child view. + """ return { "table_config": self.table.__name__, "model_type": self.queryset.model._meta.verbose_name.replace(" ", "_"), @@ -127,6 +134,9 @@ def get_extra_context(self, request, instance): } def prep_table_data(self, request, queryset, parent): + """ + Prepares the table data for the child view. + """ return queryset.annotate( rule_count=Count("aclextendedrules") + Count("aclstandardrules"), ) @@ -134,6 +144,10 @@ def prep_table_data(self, request, queryset, parent): @register_model_view(Device, "access_lists") class DeviceAccessListView(AccessListChildView): + """ + Defines the child view for the AccessLists model. + """ + queryset = Device.objects.prefetch_related("tags") tab = ViewTab( label="Access Lists", @@ -142,6 +156,9 @@ class DeviceAccessListView(AccessListChildView): ) def get_children(self, request, parent): + """ + Returns the children of the parent object. + """ return self.child_model.objects.restrict(request.user, "view").filter( device=parent, ) @@ -149,6 +166,10 @@ def get_children(self, request, parent): @register_model_view(VirtualChassis, "access_lists") class VirtualChassisAccessListView(AccessListChildView): + """ + Defines the child view for the AccessLists model. + """ + queryset = VirtualChassis.objects.prefetch_related("tags") tab = ViewTab( label="Access Lists", @@ -157,6 +178,9 @@ class VirtualChassisAccessListView(AccessListChildView): ) def get_children(self, request, parent): + """ + Returns the children of the parent object. + """ return self.child_model.objects.restrict(request.user, "view").filter( virtual_chassis=parent, ) @@ -164,6 +188,10 @@ def get_children(self, request, parent): @register_model_view(VirtualMachine, "access_lists") class VirtualMachineAccessListView(AccessListChildView): + """ + Defines the child view for the AccessLists model. + """ + queryset = VirtualMachine.objects.prefetch_related("tags") tab = ViewTab( label="Access Lists", @@ -172,6 +200,9 @@ class VirtualMachineAccessListView(AccessListChildView): ) def get_children(self, request, parent): + """ + Returns the children of the parent object. + """ return self.child_model.objects.restrict(request.user, "view").filter( virtual_machine=parent, ) @@ -246,6 +277,10 @@ class ACLInterfaceAssignmentDeleteView(generic.ObjectDeleteView): class ACLInterfaceAssignmentBulkDeleteView(generic.BulkDeleteView): + """ + Defines the bulk delete view for the ACLInterfaceAssignments django model. + """ + queryset = models.ACLInterfaceAssignment.objects.prefetch_related( "access_list", "tags", @@ -265,6 +300,9 @@ class ACLInterfaceAssignmentChildView(generic.ObjectChildrenView): template_name = "inc/view_tab.html" def get_extra_context(self, request, instance): + """ + Returns the extra context for the view. + """ return { "table_config": self.table.__name__, "model_type": self.queryset.model._meta.verbose_name.replace(" ", "_"), @@ -274,6 +312,10 @@ def get_extra_context(self, request, instance): @register_model_view(Interface, "acl_interface_assignments") class InterfaceACLInterfaceAssignmentView(ACLInterfaceAssignmentChildView): + """ + Defines the child view for the ACLInterfaceAssignments model. + """ + queryset = Interface.objects.prefetch_related("device", "tags") tab = ViewTab( label="ACL Interface Assignments", @@ -284,6 +326,9 @@ class InterfaceACLInterfaceAssignmentView(ACLInterfaceAssignmentChildView): ) def get_children(self, request, parent): + """ + Returns the children of the parent object. + """ return self.child_model.objects.restrict(request.user, "view").filter( interface=parent, ) @@ -293,6 +338,10 @@ def get_children(self, request, parent): class VirtualMachineInterfaceACLInterfaceAssignmentView( ACLInterfaceAssignmentChildView, ): + """ + Defines the child view for the ACLInterfaceAssignments model. + """ + queryset = VMInterface.objects.prefetch_related("virtual_machine", "tags") tab = ViewTab( label="ACL Interface Assignments", @@ -303,6 +352,9 @@ class VirtualMachineInterfaceACLInterfaceAssignmentView( ) def get_children(self, request, parent): + """ + Returns the children of the parent object. + """ return self.child_model.objects.restrict(request.user, "view").filter( vminterface=parent, ) @@ -379,6 +431,10 @@ class ACLStandardRuleDeleteView(generic.ObjectDeleteView): class ACLStandardRuleBulkDeleteView(generic.BulkDeleteView): + """ + Defines the bulk delete view for the ACLStandardRule django model. + """ + queryset = models.ACLStandardRule.objects.prefetch_related( "access_list", "tags", @@ -463,6 +519,10 @@ class ACLExtendedRuleDeleteView(generic.ObjectDeleteView): class ACLExtendedRuleBulkDeleteView(generic.BulkDeleteView): + """ + Defines bulk delete view for the ACLExtendedRules django model. + """ + queryset = models.ACLExtendedRule.objects.prefetch_related( "access_list", "tags", From 2640369744d627caada3736cae24a9ca0c24c6b2 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 14:53:46 +0000 Subject: [PATCH 06/42] update pre-commit --- .pre-commit-config.yaml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9ebc05c2..efe81708 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -19,11 +19,13 @@ repos: - id: isort args: - "--profile=black" + exclude: ^.devcontainer/ - repo: https://github.com/psf/black rev: 22.12.0 hooks: - id: black language_version: python3 + exclude: ^.devcontainer/ - repo: https://github.com/asottile/add-trailing-comma rev: v2.4.0 hooks: @@ -34,6 +36,7 @@ repos: rev: 6.0.0 hooks: - id: flake8 + exclude: ^.devcontainer/ - repo: https://github.com/asottile/pyupgrade rev: v3.3.1 hooks: @@ -44,6 +47,12 @@ repos: rev: v1.29.0 hooks: - id: yamllint + - repo: https://github.com/econchick/interrogate + rev: 1.5.0 + hooks: + - id: interrogate + args: [--fail-under=90, --verbose] + exclude: (^.devcontainer/|^netbox_acls/migrations/) #- repo: https://github.com/Lucas-C/pre-commit-hooks-nodejs # rev: v1.1.2 # hooks: From 33e8717ffe35b4e7bb1b2ade35817e194a337403 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 14:54:22 +0000 Subject: [PATCH 07/42] fix ACLExtendedRule filterset tuple Rule Details --- netbox_acls/forms/filtersets.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/netbox_acls/forms/filtersets.py b/netbox_acls/forms/filtersets.py index 7767e9a0..a833e7b1 100644 --- a/netbox_acls/forms/filtersets.py +++ b/netbox_acls/forms/filtersets.py @@ -222,12 +222,9 @@ class ACLExtendedRuleFilterForm(BaseACLRuleFilterForm): choices=add_blank_choice(ACLProtocolChoices), required=False, ) - fieldsets = BaseACLRuleFilterForm.fieldsets + ( + fieldsets = BaseACLRuleFilterForm.fieldsets[:-1] + ( ( "Rule Details", - ( - "destination_prefix", - "protocol", - ), + BaseACLRuleFilterForm.fieldsets[-1][1] + ("destination_prefix", "protocol"), ), ) From b8d7267c47432feb2a4ef4269abf21576deb0833 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 14:59:22 +0000 Subject: [PATCH 08/42] consolidate acl type vs rule check --- netbox_acls/forms/models.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 99278d98..1fc7f104 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -243,11 +243,7 @@ def clean(self): if ( acl_type == ACLTypeChoices.TYPE_EXTENDED and self.instance.aclstandardrules.exists() - ): - error_message["type"] = [ - "This ACL has ACL rules associated, CANNOT change ACL type.", - ] - elif ( + ) or ( acl_type == ACLTypeChoices.TYPE_STANDARD and self.instance.aclextendedrules.exists() ): From d7f42889963adcd7f86fe5286e840815541cfd71 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 10:46:40 -0500 Subject: [PATCH 09/42] merge neste ifs & create new function --- netbox_acls/forms/models.py | 78 ++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 1fc7f104..5d6fba9f 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -239,17 +239,20 @@ def clean(self): "name": [error_same_acl_name], } # Check if Access List has no existing rules before change the Access List's type. - if self.instance.pk: - if ( + if ( + self.instance.pk + and ( acl_type == ACLTypeChoices.TYPE_EXTENDED and self.instance.aclstandardrules.exists() - ) or ( + ) + or ( acl_type == ACLTypeChoices.TYPE_STANDARD and self.instance.aclextendedrules.exists() - ): - error_message["type"] = [ - "This ACL has ACL rules associated, CANNOT change ACL type.", - ] + ) + ): + error_message["type"] = [ + "This ACL has ACL rules associated, CANNOT change ACL type.", + ] if error_message: raise forms.ValidationError(error_message) @@ -642,38 +645,41 @@ def clean(self): # No need to check for unique_together since there is no usage of GFK if cleaned_data.get("action") == "remark": - # Check if action set to remark, but no remark set. - if not cleaned_data.get("remark"): - error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] - # Check if action set to remark, but source_prefix set. - if cleaned_data.get("source_prefix"): - error_message["source_prefix"] = [ - ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, - ] - # Check if action set to remark, but source_ports set. - if cleaned_data.get("source_ports"): - error_message["source_ports"] = [ - "Action is set to remark, Source Ports CANNOT be set.", - ] - # Check if action set to remark, but destination_prefix set. - if cleaned_data.get("destination_prefix"): - error_message["destination_prefix"] = [ - "Action is set to remark, Destination Prefix CANNOT be set.", - ] - # Check if action set to remark, but destination_ports set. - if cleaned_data.get("destination_ports"): - error_message["destination_ports"] = [ - "Action is set to remark, Destination Ports CANNOT be set.", - ] - # Check if action set to remark, but protocol set. - if cleaned_data.get("protocol"): - error_message["protocol"] = [ - "Action is set to remark, Protocol CANNOT be set.", - ] - # Check if action not set to remark, but remark set. + self._extracted_from_clean_20(cleaned_data, error_message) elif cleaned_data.get("remark"): error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] if error_message: raise forms.ValidationError(error_message) return cleaned_data + + # TODO: Consolidate this function with the one in ACLStandardRuleForm + def _extracted_from_clean_20(self, cleaned_data, error_message): + # Check if action set to remark, but no remark set. + if not cleaned_data.get("remark"): + error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] + # Check if action set to remark, but source_prefix set. + if cleaned_data.get("source_prefix"): + error_message["source_prefix"] = [ + ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, + ] + # Check if action set to remark, but source_ports set. + if cleaned_data.get("source_ports"): + error_message["source_ports"] = [ + "Action is set to remark, Source Ports CANNOT be set.", + ] + # Check if action set to remark, but destination_prefix set. + if cleaned_data.get("destination_prefix"): + error_message["destination_prefix"] = [ + "Action is set to remark, Destination Prefix CANNOT be set.", + ] + # Check if action set to remark, but destination_ports set. + if cleaned_data.get("destination_ports"): + error_message["destination_ports"] = [ + "Action is set to remark, Destination Ports CANNOT be set.", + ] + # Check if action set to remark, but protocol set. + if cleaned_data.get("protocol"): + error_message["protocol"] = [ + "Action is set to remark, Protocol CANNOT be set.", + ] From d0d892e0f619b29cbcd6702b2b6505180f399bd0 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 10:46:48 -0500 Subject: [PATCH 10/42] update setup.py --- setup.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/setup.py b/setup.py index 50d40559..5059faa6 100644 --- a/setup.py +++ b/setup.py @@ -1,21 +1,30 @@ +""" +Configuration for setuptools. +""" import codecs import os.path from setuptools import find_packages, setup -here = os.path.abspath(os.path.dirname(__file__)) +script_dir = os.path.abspath(os.path.dirname(__file__)) -with open(os.path.join(here, "README.md"), encoding="utf-8") as fh: +with open(os.path.join(script_dir, "README.md"), encoding="utf-8") as fh: long_description = fh.read() -def read(rel_path): - with codecs.open(os.path.join(here, rel_path), "r") as fp: +def read(relative_path): + """ + Read a file and return its contents. + """ + with codecs.open(os.path.join(script_dir, relative_path), "r") as fp: return fp.read() -def get_version(rel_path): - for line in read(rel_path).splitlines(): +def get_version(relative_path): + """ + Extract the version number from a file without importing it. + """ + for line in read(relative_path).splitlines(): if not line.startswith("__version__"): raise RuntimeError("Unable to find version string.") delim = '"' if '"' in line else "'" From 007877ffd2fecb747ab49a889205f756bbdfc737 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 20:16:28 +0000 Subject: [PATCH 11/42] fix constants --- netbox_acls/forms/models.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 5d6fba9f..67eacf2d 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -33,7 +33,7 @@ ) # Sets a standard mark_safe help_text value to be used by the various classes -help_text_acl_rule_logic = mark_safe( +HELP_TEXT_ACL_RULE_LOGIC = mark_safe( "*Note: CANNOT be set if action is set to remark.", ) # Sets a standard help_text value to be used by the various classes for acl action @@ -488,7 +488,7 @@ class ACLStandardRuleForm(NetBoxModelForm): source_prefix = DynamicModelChoiceField( queryset=Prefix.objects.all(), required=False, - help_text=help_text_acl_rule_logic, + help_text=HELP_TEXT_ACL_RULE_LOGIC, label="Source Prefix", ) @@ -571,13 +571,13 @@ class ACLExtendedRuleForm(NetBoxModelForm): source_prefix = DynamicModelChoiceField( queryset=Prefix.objects.all(), required=False, - help_text=help_text_acl_rule_logic, + help_text=HELP_TEXT_ACL_RULE_LOGIC, label="Source Prefix", ) destination_prefix = DynamicModelChoiceField( queryset=Prefix.objects.all(), required=False, - help_text=help_text_acl_rule_logic, + help_text=HELP_TEXT_ACL_RULE_LOGIC, label="Destination Prefix", ) fieldsets = ( @@ -618,13 +618,13 @@ class Meta: ) help_texts = { "action": HELP_TEXT_ACL_ACTION, - "destination_ports": help_text_acl_rule_logic, + "destination_ports": HELP_TEXT_ACL_RULE_LOGIC, "index": HELP_TEXT_ACL_RULE_INDEX, - "protocol": help_text_acl_rule_logic, + "protocol": HELP_TEXT_ACL_RULE_LOGIC, "remark": mark_safe( "*Note: CANNOT be set if action is not set to remark.", ), - "source_ports": help_text_acl_rule_logic, + "source_ports": HELP_TEXT_ACL_RULE_LOGIC, } def clean(self): From fb15e3be92565340e487a01ff97d4d34e0e66acd Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 20:39:27 +0000 Subject: [PATCH 12/42] fix bug with acl input --- netbox_acls/forms/models.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 67eacf2d..0d3a2563 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -239,9 +239,8 @@ def clean(self): "name": [error_same_acl_name], } # Check if Access List has no existing rules before change the Access List's type. - if ( - self.instance.pk - and ( + if self.instance.pk and ( + ( acl_type == ACLTypeChoices.TYPE_EXTENDED and self.instance.aclstandardrules.exists() ) From 62dc45610672938080c09403ed6fdf1356dd3bc5 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 21:40:20 +0000 Subject: [PATCH 13/42] move constants --- netbox_acls/forms/constants.py | 26 ++++++++++++++++++++++++++ netbox_acls/forms/models.py | 30 ++++++++---------------------- 2 files changed, 34 insertions(+), 22 deletions(-) create mode 100644 netbox_acls/forms/constants.py diff --git a/netbox_acls/forms/constants.py b/netbox_acls/forms/constants.py new file mode 100644 index 00000000..18dbb22f --- /dev/null +++ b/netbox_acls/forms/constants.py @@ -0,0 +1,26 @@ +""" +Constants for forms +""" +from django.utils.safestring import mark_safe + +# Sets a standard mark_safe help_text value to be used by the various classes +HELP_TEXT_ACL_RULE_LOGIC = mark_safe( + "*Note: CANNOT be set if action is set to remark.", +) +# Sets a standard help_text value to be used by the various classes for acl action +HELP_TEXT_ACL_ACTION = "Action the rule will take (remark, deny, or allow)." +# Sets a standard help_text value to be used by the various classes for acl index +HELP_TEXT_ACL_RULE_INDEX = ( + "Determines the order of the rule in the ACL processing. AKA Sequence Number." +) + +# Sets a standard error message for ACL rules with an action of remark, but no remark set. +ERROR_MESSAGE_NO_REMARK = "Action is set to remark, you MUST add a remark." +# Sets a standard error message for ACL rules with an action of remark, but no source_prefix is set. +ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET = ( + "Action is set to remark, Source Prefix CANNOT be set." +) +# Sets a standard error message for ACL rules with an action not set to remark, but no remark is set. +ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK = ( + "CANNOT set remark unless action is set to remark." +) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 0d3a2563..ac794cc6 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -18,6 +18,14 @@ ) from ..choices import ACLTypeChoices +from .constants import ( + ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, + ERROR_MESSAGE_NO_REMARK, + ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK, + HELP_TEXT_ACL_ACTION, + HELP_TEXT_ACL_RULE_INDEX, + HELP_TEXT_ACL_RULE_LOGIC, +) from ..models import ( AccessList, ACLExtendedRule, @@ -32,28 +40,6 @@ "ACLExtendedRuleForm", ) -# Sets a standard mark_safe help_text value to be used by the various classes -HELP_TEXT_ACL_RULE_LOGIC = mark_safe( - "*Note: CANNOT be set if action is set to remark.", -) -# Sets a standard help_text value to be used by the various classes for acl action -HELP_TEXT_ACL_ACTION = "Action the rule will take (remark, deny, or allow)." -# Sets a standard help_text value to be used by the various classes for acl index -HELP_TEXT_ACL_RULE_INDEX = ( - "Determines the order of the rule in the ACL processing. AKA Sequence Number." -) - -# Sets a standard error message for ACL rules with an action of remark, but no remark set. -ERROR_MESSAGE_NO_REMARK = "Action is set to remark, you MUST add a remark." -# Sets a standard error message for ACL rules with an action of remark, but no source_prefix is set. -ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET = ( - "Action is set to remark, Source Prefix CANNOT be set." -) -# Sets a standard error message for ACL rules with an action not set to remark, but no remark is set. -ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK = ( - "CANNOT set remark unless action is set to remark." -) - class AccessListForm(NetBoxModelForm): """ From a8ae98d55d8e7ee5cca9927cca35ebe518999db2 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Fri, 27 Jan 2023 21:44:50 +0000 Subject: [PATCH 14/42] stash conflict --- netbox_acls/forms/models.py | 151 ++++++++++++++---------------------- 1 file changed, 57 insertions(+), 94 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index ac794cc6..125fd5a4 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -453,11 +453,9 @@ def save(self, *args, **kwargs): return super().save(*args, **kwargs) -class ACLStandardRuleForm(NetBoxModelForm): +class BaseACLRuleForm(NetBoxModelForm): """ - GUI form to add or edit Standard Access List. - Requires an access_list, an index, and ACL rule type. - See the clean function for logic on other field requirements. + GUI form to add or edit Access List Rules to be inherited by other classes """ access_list = DynamicModelChoiceField( @@ -506,26 +504,13 @@ class Meta: } def clean(self): - """ - Validates form inputs before submitting: - - Check if action set to remark, but no remark set. - - Check if action set to remark, but source_prefix set. - - Check remark set, but action not set to remark. - """ cleaned_data = super().clean() error_message = {} # No need to check for unique_together since there is no usage of GFK if cleaned_data.get("action") == "remark": - # Check if action set to remark, but no remark set. - if not cleaned_data.get("remark"): - error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] - # Check if action set to remark, but source_prefix set. - if cleaned_data.get("source_prefix"): - error_message["source_prefix"] = [ - ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, - ] + self._extracted_from_clean_20(cleaned_data, error_message, "extended") # Check remark set, but action not set to remark. elif cleaned_data.get("remark"): error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] @@ -535,7 +520,57 @@ def clean(self): return cleaned_data -class ACLExtendedRuleForm(NetBoxModelForm): + def _extracted_from_clean_20(self, cleaned_data, error_message, rule_type): + """ + Validates form inputs before submitting: + - Check if action set to remark, but no remark set. + - Check if action set to remark, but source_prefix set. + - Check if action set to remark, but source_ports set. + - Check if action set to remark, but destination_prefix set. + - Check if action set to remark, but destination_ports set. + - Check if action set to remark, but destination_ports set. + - Check if action set to remark, but protocol set. + """ + # Check if action set to remark, but no remark set. + if not cleaned_data.get("remark"): + error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] + # Check if action set to remark, but source_prefix set. + if cleaned_data.get("source_prefix"): + error_message["source_prefix"] = [ + ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, + ] + if rule_type == "extended": + # Check if action set to remark, but source_ports set. + if cleaned_data.get("source_ports"): + error_message["source_ports"] = [ + "Action is set to remark, Source Ports CANNOT be set.", + ] + # Check if action set to remark, but destination_prefix set. + if cleaned_data.get("destination_prefix"): + error_message["destination_prefix"] = [ + "Action is set to remark, Destination Prefix CANNOT be set.", + ] + # Check if action set to remark, but destination_ports set. + if cleaned_data.get("destination_ports"): + error_message["destination_ports"] = [ + "Action is set to remark, Destination Ports CANNOT be set.", + ] + # Check if action set to remark, but protocol set. + if cleaned_data.get("protocol"): + error_message["protocol"] = [ + "Action is set to remark, Protocol CANNOT be set.", + ] + + +class ACLStandardRuleForm(BaseACLRuleForm): + """ + GUI form to add or edit Standard Access List. + Requires an access_list, an index, and ACL rule type. + See the clean function for logic on other field requirements. + """ + + +class ACLExtendedRuleForm(BaseACLRuleForm): """ GUI form to add or edit Extended Access List. Requires an access_list, an index, and ACL rule type. @@ -553,35 +588,20 @@ class ACLExtendedRuleForm(NetBoxModelForm): label="Access List", ) - source_prefix = DynamicModelChoiceField( - queryset=Prefix.objects.all(), - required=False, - help_text=HELP_TEXT_ACL_RULE_LOGIC, - label="Source Prefix", - ) destination_prefix = DynamicModelChoiceField( queryset=Prefix.objects.all(), required=False, help_text=HELP_TEXT_ACL_RULE_LOGIC, label="Destination Prefix", ) - fieldsets = ( - ("Access List Details", ("access_list", "description", "tags")), + fieldsets = BaseACLRuleForm.fieldsets[:-1] + ( ( "Rule Definition", - ( - "index", - "action", - "remark", - "source_prefix", - "source_ports", - "destination_prefix", - "destination_ports", - "protocol", - ), + BaseACLRuleForm.fieldsets[-1][1] + ("source_ports", "destination_prefix", "destination_ports", "protocol"), ), ) + class Meta: """ Defines the Model and fields to be used by the form. @@ -611,60 +631,3 @@ class Meta: ), "source_ports": HELP_TEXT_ACL_RULE_LOGIC, } - - def clean(self): - """ - Validates form inputs before submitting: - - Check if action set to remark, but no remark set. - - Check if action set to remark, but source_prefix set. - - Check if action set to remark, but source_ports set. - - Check if action set to remark, but destination_prefix set. - - Check if action set to remark, but destination_ports set. - - Check if action set to remark, but destination_ports set. - - Check if action set to remark, but protocol set. - - Check remark set, but action not set to remark. - """ - cleaned_data = super().clean() - error_message = {} - - # No need to check for unique_together since there is no usage of GFK - - if cleaned_data.get("action") == "remark": - self._extracted_from_clean_20(cleaned_data, error_message) - elif cleaned_data.get("remark"): - error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] - - if error_message: - raise forms.ValidationError(error_message) - return cleaned_data - - # TODO: Consolidate this function with the one in ACLStandardRuleForm - def _extracted_from_clean_20(self, cleaned_data, error_message): - # Check if action set to remark, but no remark set. - if not cleaned_data.get("remark"): - error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] - # Check if action set to remark, but source_prefix set. - if cleaned_data.get("source_prefix"): - error_message["source_prefix"] = [ - ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, - ] - # Check if action set to remark, but source_ports set. - if cleaned_data.get("source_ports"): - error_message["source_ports"] = [ - "Action is set to remark, Source Ports CANNOT be set.", - ] - # Check if action set to remark, but destination_prefix set. - if cleaned_data.get("destination_prefix"): - error_message["destination_prefix"] = [ - "Action is set to remark, Destination Prefix CANNOT be set.", - ] - # Check if action set to remark, but destination_ports set. - if cleaned_data.get("destination_ports"): - error_message["destination_ports"] = [ - "Action is set to remark, Destination Ports CANNOT be set.", - ] - # Check if action set to remark, but protocol set. - if cleaned_data.get("protocol"): - error_message["protocol"] = [ - "Action is set to remark, Protocol CANNOT be set.", - ] From 98fbb5352cbc45c684c353cc77e94bca720f7536 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Sat, 28 Jan 2023 06:18:39 +0000 Subject: [PATCH 15/42] use ACLTypeChoices.TYPE_EXTENDED --- netbox_acls/models/access_list_rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox_acls/models/access_list_rules.py b/netbox_acls/models/access_list_rules.py index 5ec96127..465f5629 100644 --- a/netbox_acls/models/access_list_rules.py +++ b/netbox_acls/models/access_list_rules.py @@ -135,7 +135,7 @@ class ACLExtendedRule(BaseACLRule): on_delete=models.CASCADE, to=AccessList, verbose_name="Extended Access List", - limit_choices_to={"type": "extended"}, + limit_choices_to={"type": ACLTypeChoices.TYPE_EXTENDED}, related_name="aclextendedrules", ) source_ports = ArrayField( From c21b4c755edcd7afe6a6e68460ef1eb63d938071 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Sat, 28 Jan 2023 06:20:53 +0000 Subject: [PATCH 16/42] refactor --- netbox_acls/forms/models.py | 152 +++++++++++++++++++----------------- 1 file changed, 79 insertions(+), 73 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 125fd5a4..886a1f16 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -176,43 +176,63 @@ def clean(self): error_message = {} if self.errors.get("name"): return cleaned_data + name = cleaned_data.get("name") acl_type = cleaned_data.get("type") - device = cleaned_data.get("device") - virtual_chassis = cleaned_data.get("virtual_chassis") - virtual_machine = cleaned_data.get("virtual_machine") # Check if more than one host type selected. - if ( - (device and virtual_chassis) - or (device and virtual_machine) - or (virtual_chassis and virtual_machine) - ): + host_types = self.get_host_types() + + # Check if no hosts selected. + self.validate_host_types(host_types) + + host_type, host = host_types[0] + + # Check if duplicate entry. + self.validate_duplicate_entry(name, host_type, host, error_message) + # Check if Access List has no existing rules before change the Access List's type. + self.validate_acl_type_change(acl_type, error_message) + + if error_message: + raise forms.ValidationError(error_message) + + return cleaned_data + + def get_host_types(self): + """ + Get host type assigned to the Access List. + """ + device = self.cleaned_data.get("device") + virtual_chassis = self.cleaned_data.get("virtual_chassis") + virtual_machine = self.cleaned_data.get("virtual_machine") + host_types = [ + ("device", device), + ("virtual_chassis", virtual_chassis), + ("virtual_machine", virtual_machine), + ] + return [x for x in host_types if x[1]] + + def validate_host_types(self, host_types): + """ + Check if more than one host type selected. + """ + if len(host_types) > 1: raise forms.ValidationError( "Access Lists must be assigned to one host (either a device, virtual chassis or virtual machine) at a time.", ) # Check if no hosts selected. - if not device and not virtual_chassis and not virtual_machine: + if not host_types: raise forms.ValidationError( "Access Lists must be assigned to a device, virtual chassis or virtual machine.", ) - if device: - host_type = "device" - existing_acls = AccessList.objects.filter(name=name, device=device).exists() - elif virtual_machine: - host_type = "virtual_machine" - existing_acls = AccessList.objects.filter( - name=name, - virtual_machine=virtual_machine, - ).exists() - else: - host_type = "virtual_chassis" - existing_acls = AccessList.objects.filter( - name=name, - virtual_chassis=virtual_chassis, - ).exists() - + def validate_duplicate_entry(self, name, host_type, host, error_message): + """ + Check if duplicate entry. (Because of GFK.) + """ + existing_acls = AccessList.objects.filter( + name=name, **{host_type: host} + ).exists() # Check if duplicate entry. if ( "name" in self.changed_data or host_type in self.changed_data @@ -224,7 +244,11 @@ def clean(self): host_type: [error_same_acl_name], "name": [error_same_acl_name], } - # Check if Access List has no existing rules before change the Access List's type. + + def validate_acl_type_change(self, acl_type, error_message): + """ + Check if Access List has no existing rules before change the Access List's type. + """ if self.instance.pk and ( ( acl_type == ACLTypeChoices.TYPE_EXTENDED @@ -242,8 +266,6 @@ def clean(self): if error_message: raise forms.ValidationError(error_message) - return cleaned_data - def save(self, *args, **kwargs): """ Set assigned object @@ -412,19 +434,19 @@ def clean(self): assigned_object_type: [error_acl_not_assigned_to_host], host_type: [error_acl_not_assigned_to_host], } - # Check for duplicate entry. - if ACLInterfaceAssignment.objects.filter( - access_list=access_list, - assigned_object_id=assigned_object_id, - assigned_object_type=assigned_object_type_id, - direction=direction, - ).exists(): - error_duplicate_entry = "An ACL with this name is already associated to this interface & direction." - error_message |= { - "access_list": [error_duplicate_entry], - "direction": [error_duplicate_entry], - assigned_object_type: [error_duplicate_entry], - } + ## Check for duplicate entry. + # if ACLInterfaceAssignment.objects.filter( + # access_list=access_list, + # assigned_object_id=assigned_object_id, + # assigned_object_type=assigned_object_type_id, + # direction=direction, + # ).exists(): + # error_duplicate_entry = "An ACL with this name is already associated to this interface & direction." + # error_message |= { + # "access_list": [error_duplicate_entry], + # "direction": [error_duplicate_entry], + # assigned_object_type: [error_duplicate_entry], + # } # Check that the interface does not have an existing ACL applied in the direction already. if ACLInterfaceAssignment.objects.filter( assigned_object_id=assigned_object_id, @@ -454,9 +476,7 @@ def save(self, *args, **kwargs): class BaseACLRuleForm(NetBoxModelForm): - """ - GUI form to add or edit Access List Rules to be inherited by other classes - """ + """GUI form to add or edit Access List Rules to be inherited by other classes""" access_list = DynamicModelChoiceField( queryset=AccessList.objects.all(), @@ -464,7 +484,7 @@ class BaseACLRuleForm(NetBoxModelForm): "type": ACLTypeChoices.TYPE_STANDARD, }, help_text=mark_safe( - "*Note: This field will only display Standard ACLs.", + "*Note: This field will only display Standard ACLs." ), label="Access List", ) @@ -481,9 +501,7 @@ class BaseACLRuleForm(NetBoxModelForm): ) class Meta: - """ - Defines the Model and fields to be used by the form. - """ + """Defines the Model and fields to be used by the form.""" model = ACLStandardRule fields = ( @@ -496,11 +514,14 @@ class Meta: "description", ) help_texts = { - "index": HELP_TEXT_ACL_RULE_INDEX, "action": HELP_TEXT_ACL_ACTION, + "destination_ports": HELP_TEXT_ACL_RULE_LOGIC, + "index": HELP_TEXT_ACL_RULE_INDEX, + "protocol": HELP_TEXT_ACL_RULE_LOGIC, "remark": mark_safe( - "*Note: CANNOT be set if source prefix OR action is set.", + "*Note: CANNOT be set if action is not set to remark." ), + "source_ports": HELP_TEXT_ACL_RULE_LOGIC, } def clean(self): @@ -519,7 +540,6 @@ def clean(self): raise forms.ValidationError(error_message) return cleaned_data - def _extracted_from_clean_20(self, cleaned_data, error_message, rule_type): """ Validates form inputs before submitting: @@ -537,13 +557,13 @@ def _extracted_from_clean_20(self, cleaned_data, error_message, rule_type): # Check if action set to remark, but source_prefix set. if cleaned_data.get("source_prefix"): error_message["source_prefix"] = [ - ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, + ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET ] if rule_type == "extended": # Check if action set to remark, but source_ports set. if cleaned_data.get("source_ports"): error_message["source_ports"] = [ - "Action is set to remark, Source Ports CANNOT be set.", + "Action is set to remark, Source Ports CANNOT be set." ] # Check if action set to remark, but destination_prefix set. if cleaned_data.get("destination_prefix"): @@ -553,12 +573,12 @@ def _extracted_from_clean_20(self, cleaned_data, error_message, rule_type): # Check if action set to remark, but destination_ports set. if cleaned_data.get("destination_ports"): error_message["destination_ports"] = [ - "Action is set to remark, Destination Ports CANNOT be set.", + "Action is set to remark, Destination Ports CANNOT be set." ] # Check if action set to remark, but protocol set. if cleaned_data.get("protocol"): error_message["protocol"] = [ - "Action is set to remark, Protocol CANNOT be set.", + "Action is set to remark, Protocol CANNOT be set." ] @@ -579,9 +599,7 @@ class ACLExtendedRuleForm(BaseACLRuleForm): access_list = DynamicModelChoiceField( queryset=AccessList.objects.all(), - query_params={ - "type": ACLTypeChoices.TYPE_EXTENDED, - }, + query_params={"type": ACLTypeChoices.TYPE_EXTENDED}, help_text=mark_safe( "*Note: This field will only display Extended ACLs.", ), @@ -597,15 +615,13 @@ class ACLExtendedRuleForm(BaseACLRuleForm): fieldsets = BaseACLRuleForm.fieldsets[:-1] + ( ( "Rule Definition", - BaseACLRuleForm.fieldsets[-1][1] + ("source_ports", "destination_prefix", "destination_ports", "protocol"), + BaseACLRuleForm.fieldsets[-1][1] + + ("source_ports", "destination_prefix", "destination_ports", "protocol"), ), ) - class Meta: - """ - Defines the Model and fields to be used by the form. - """ + """Defines the Model and fields to be used by the form.""" model = ACLExtendedRule fields = ( @@ -621,13 +637,3 @@ class Meta: "tags", "description", ) - help_texts = { - "action": HELP_TEXT_ACL_ACTION, - "destination_ports": HELP_TEXT_ACL_RULE_LOGIC, - "index": HELP_TEXT_ACL_RULE_INDEX, - "protocol": HELP_TEXT_ACL_RULE_LOGIC, - "remark": mark_safe( - "*Note: CANNOT be set if action is not set to remark.", - ), - "source_ports": HELP_TEXT_ACL_RULE_LOGIC, - } From 1dbe0b74567c3d082e3da2cc5b82b88d83751aaa Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Sat, 28 Jan 2023 11:26:58 -0500 Subject: [PATCH 17/42] tweaks --- netbox_acls/forms/models.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 886a1f16..a1044ad1 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -214,7 +214,7 @@ def get_host_types(self): def validate_host_types(self, host_types): """ - Check if more than one host type selected. + Check number of host types selected. """ if len(host_types) > 1: raise forms.ValidationError( @@ -434,19 +434,19 @@ def clean(self): assigned_object_type: [error_acl_not_assigned_to_host], host_type: [error_acl_not_assigned_to_host], } - ## Check for duplicate entry. - # if ACLInterfaceAssignment.objects.filter( - # access_list=access_list, - # assigned_object_id=assigned_object_id, - # assigned_object_type=assigned_object_type_id, - # direction=direction, - # ).exists(): - # error_duplicate_entry = "An ACL with this name is already associated to this interface & direction." - # error_message |= { - # "access_list": [error_duplicate_entry], - # "direction": [error_duplicate_entry], - # assigned_object_type: [error_duplicate_entry], - # } + # Check for duplicate entry. + if ACLInterfaceAssignment.objects.filter( + access_list=access_list, + assigned_object_id=assigned_object_id, + assigned_object_type=assigned_object_type_id, + direction=direction, + ).exists(): + error_duplicate_entry = "An ACL with this name is already associated to this interface & direction." + error_message |= { + "access_list": [error_duplicate_entry], + "direction": [error_duplicate_entry], + assigned_object_type: [error_duplicate_entry], + } # Check that the interface does not have an existing ACL applied in the direction already. if ACLInterfaceAssignment.objects.filter( assigned_object_id=assigned_object_id, From 7789df33097afc46ea671bb1d8285e305cdafaf3 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Sat, 28 Jan 2023 12:10:53 -0500 Subject: [PATCH 18/42] adjust indenting for docstring --- netbox_acls/forms/models.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index a1044ad1..ac86bab6 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -167,10 +167,10 @@ def __init__(self, *args, **kwargs): def clean(self): """ Validates form inputs before submitting: - - Check if more than one host type selected. - - Check if no hosts selected. - - Check if duplicate entry. (Because of GFK.) - - Check if Access List has no existing rules before change the Access List's type. + - Check if more than one host type selected. + - Check if no hosts selected. + - Check if duplicate entry. (Because of GFK.) + - Check if Access List has no existing rules before change the Access List's type. """ cleaned_data = super().clean() error_message = {} @@ -376,12 +376,12 @@ class Meta: def clean(self): """ Validates form inputs before submitting: - - Check if both interface and vminterface are set. - - Check if neither interface nor vminterface are set. - - Check that an interface's parent device/virtual_machine is assigned to the Access List. - - Check that an interface's parent device/virtual_machine is assigned to the Access List. - - Check for duplicate entry. (Because of GFK) - - Check that the interface does not have an existing ACL applied in the direction already. + - Check if both interface and vminterface are set. + - Check if neither interface nor vminterface are set. + - Check that an interface's parent device/virtual_machine is assigned to the Access List. + - Check that an interface's parent device/virtual_machine is assigned to the Access List. + - Check for duplicate entry. (Because of GFK) + - Check that the interface does not have an existing ACL applied in the direction already. """ cleaned_data = super().clean() error_message = {} From 1861c56d7887dcfccf1d77c93ff68c00d5b3032f Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Sun, 29 Jan 2023 11:33:36 -0500 Subject: [PATCH 19/42] TODOs --- TODO | 1 + netbox_acls/forms/models.py | 1 + 2 files changed, 2 insertions(+) create mode 100644 TODO diff --git a/TODO b/TODO new file mode 100644 index 00000000..8e086202 --- /dev/null +++ b/TODO @@ -0,0 +1 @@ +- TODO: single underscore functions https://stackoverflow.com/questions/1301346/what-is-the-meaning-of-single-and-double-underscore-before-an-object-name diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index ac86bab6..148fd5b4 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -172,6 +172,7 @@ def clean(self): - Check if duplicate entry. (Because of GFK.) - Check if Access List has no existing rules before change the Access List's type. """ + # TODO: Refactor this method to fix error message logic. cleaned_data = super().clean() error_message = {} if self.errors.get("name"): From ced12c5af65b1e9a93809a9a2c91dd3c946822dd Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 14:59:39 +0000 Subject: [PATCH 20/42] improve ACLInterfaceAssignmentForm clean method --- netbox_acls/forms/models.py | 198 ++++++++++++++++++++++-------------- 1 file changed, 119 insertions(+), 79 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 148fd5b4..3c1f7995 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -182,24 +182,24 @@ def clean(self): acl_type = cleaned_data.get("type") # Check if more than one host type selected. - host_types = self.get_host_types() + host_types = self._get_host_types() # Check if no hosts selected. - self.validate_host_types(host_types) + self._validate_host_types(host_types) host_type, host = host_types[0] # Check if duplicate entry. - self.validate_duplicate_entry(name, host_type, host, error_message) + self._validate_duplicate_entry(name, host_type, host, error_message) # Check if Access List has no existing rules before change the Access List's type. - self.validate_acl_type_change(acl_type, error_message) + self._validate_acl_type_change(acl_type, error_message) if error_message: raise forms.ValidationError(error_message) return cleaned_data - def get_host_types(self): + def _get_host_types(self): """ Get host type assigned to the Access List. """ @@ -213,7 +213,7 @@ def get_host_types(self): ] return [x for x in host_types if x[1]] - def validate_host_types(self, host_types): + def _validate_host_types(self, host_types): """ Check number of host types selected. """ @@ -227,7 +227,7 @@ def validate_host_types(self, host_types): "Access Lists must be assigned to a device, virtual chassis or virtual machine.", ) - def validate_duplicate_entry(self, name, host_type, host, error_message): + def _validate_duplicate_entry(self, name, host_type, host, error_message): """ Check if duplicate entry. (Because of GFK.) """ @@ -246,7 +246,7 @@ def validate_duplicate_entry(self, name, host_type, host, error_message): "name": [error_same_acl_name], } - def validate_acl_type_change(self, acl_type, error_message): + def _validate_acl_type_change(self, acl_type, error_message): """ Check if Access List has no existing rules before change the Access List's type. """ @@ -374,6 +374,7 @@ class Meta: ), } + def clean(self): """ Validates form inputs before submitting: @@ -384,87 +385,126 @@ def clean(self): - Check for duplicate entry. (Because of GFK) - Check that the interface does not have an existing ACL applied in the direction already. """ + # Call the parent class's `clean` method cleaned_data = super().clean() - error_message = {} - access_list = cleaned_data.get("access_list") - direction = cleaned_data.get("direction") - interface = cleaned_data.get("interface") - vminterface = cleaned_data.get("vminterface") - - # Check if both interface and vminterface are set. - if interface and vminterface: - error_too_many_interfaces = "Access Lists must be assigned to one type of interface at a time (VM interface or physical interface)" - error_message |= { - "interface": [error_too_many_interfaces], - "vminterface": [error_too_many_interfaces], - } - elif not (interface or vminterface): - error_no_interface = ( - "An Access List assignment but specify an Interface or VM Interface." - ) - error_message |= { - "interface": [error_no_interface], - "vminterface": [error_no_interface], - } - else: - if interface: - assigned_object = interface - assigned_object_type = "interface" - host_type = "device" + + # Get the interface types assigned to the Access List + interface_types = self._get_interface_types() + + # Initialize an error message variable + error_message = self._validate_interface_types(interface_types) + + if not error_message: + assigned_object_type, assigned_object = interface_types[0] + host_type = ("device" if assigned_object_type == "interface" else "virtual_machine") + + # Get the parent host (device or virtual machine) of the assigned interface + if assigned_object_type == "interface": host = Interface.objects.get(pk=assigned_object.pk).device assigned_object_id = Interface.objects.get(pk=assigned_object.pk).pk else: - assigned_object = vminterface - assigned_object_type = "vminterface" - host_type = "virtual_machine" host = VMInterface.objects.get(pk=assigned_object.pk).virtual_machine assigned_object_id = VMInterface.objects.get(pk=assigned_object.pk).pk - assigned_object_type_id = ContentType.objects.get_for_model( - assigned_object, - ).pk - access_list_host = AccessList.objects.get(pk=access_list.pk).assigned_object - - # Check that an interface's parent device/virtual_machine is assigned to the Access List. - if access_list_host != host: - error_acl_not_assigned_to_host = ( - "Access List not present on selected host." - ) - error_message |= { - "access_list": [error_acl_not_assigned_to_host], - assigned_object_type: [error_acl_not_assigned_to_host], - host_type: [error_acl_not_assigned_to_host], - } - # Check for duplicate entry. - if ACLInterfaceAssignment.objects.filter( - access_list=access_list, - assigned_object_id=assigned_object_id, - assigned_object_type=assigned_object_type_id, - direction=direction, - ).exists(): - error_duplicate_entry = "An ACL with this name is already associated to this interface & direction." - error_message |= { - "access_list": [error_duplicate_entry], - "direction": [error_duplicate_entry], - assigned_object_type: [error_duplicate_entry], - } - # Check that the interface does not have an existing ACL applied in the direction already. - if ACLInterfaceAssignment.objects.filter( - assigned_object_id=assigned_object_id, - assigned_object_type=assigned_object_type_id, - direction=direction, - ).exists(): - error_interface_already_assigned = ( - "Interfaces can only have 1 Access List assigned in each direction." - ) - error_message |= { - "direction": [error_interface_already_assigned], - assigned_object_type: [error_interface_already_assigned], - } + # Get the ContentType id for the assigned object + assigned_object_type_id = ContentType.objects.get_for_model(assigned_object).pk + + if not error_message: + # Check if the parent host is assigned to the Access List + error_message |= self._check_if_interface_parent_is_assigned_to_access_list(cleaned_data.get("access_list"), assigned_object_type, host_type, host) + + if not error_message: + # Check for duplicate entries in the Access List + error_message |= self._check_for_duplicate_entry(cleaned_data.get("access_list"), assigned_object_id, assigned_object_type_id, cleaned_data.get("direction"),) + + if not error_message: + # Check if the interface already has an ACL applied in the specified direction + error_message |= self._check_if_interface_already_has_acl_in_direction(assigned_object_id, assigned_object_type_id, cleaned_data.get("direction")) if error_message: raise forms.ValidationError(error_message) - return cleaned_data + else: + return cleaned_data + + + def _get_interface_types(self): + """ + Get interface type/model assigned to the Access List. + """ + interface = self.cleaned_data.get("interface") + vminterface = self.cleaned_data.get("vminterface") + interface_types = [ + ("interface", interface), + ("vminterface", vminterface), + ] + return [x for x in interface_types if x[1]] + + def _validate_interface_types(self, interface_types): + """ + Check if number of interface type selected is 1. + """ + # Check if more than 1 hosts selected. + if len(interface_types) > 1: + return "Assignment can only be to one interface at a time (either a interface or vm_interface)." + # Check if no hosts selected. + elif not interface_types: + return "No interface or vm_interface selected." + else: + return {} + + def _check_if_interface_parent_is_assigned_to_access_list(self, access_list, assigned_object_type, host_type, host): + """ + Check that an interface's parent device/virtual_machine is assigned to the Access List. + """ + + access_list_host = AccessList.objects.get(pk=access_list.pk).assigned_object + + if access_list_host != host: + ERROR_ACL_NOT_ASSIGNED_TO_HOST = ( + "Access List not present on selected host." + ) + return { + "access_list": [ERROR_ACL_NOT_ASSIGNED_TO_HOST], + assigned_object_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], + host_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], + } + else: + return {} + + def _check_for_duplicate_entry(self, access_list, assigned_object_id, assigned_object_type_id, direction): + """ + Check for duplicate entry. (Because of GFK) + """ + + if ACLInterfaceAssignment.objects.filter( + access_list=access_list, + assigned_object_id=assigned_object_id, + assigned_object_type=assigned_object_type_id, + direction=direction, + ).exists(): + return {"access_list": ["Duplicate entry."]} + else: + return {} + + def _check_if_interface_already_has_acl_in_direction(self, assigned_object_id, assigned_object_type_id, direction): + """ + Check that the interface does not have an existing ACL applied in the direction already. + """ + if ACLInterfaceAssignment.objects.filter( + assigned_object_id=assigned_object_id, + assigned_object_type=assigned_object_type_id, + direction=direction, + ).exists(): + error_interface_already_assigned = ( + "Interfaces can only have 1 Access List assigned in each direction." + ) + return { + "direction": [error_interface_already_assigned], + assigned_object_type: [error_interface_already_assigned], + } + else: + return {} + def save(self, *args, **kwargs): """ From cce94e46cad8688d9b762a844c566b06bca9bb1d Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 15:03:40 +0000 Subject: [PATCH 21/42] refactor --- netbox_acls/forms/models.py | 52 ++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 3c1f7995..256b5060 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -374,7 +374,6 @@ class Meta: ), } - def clean(self): """ Validates form inputs before submitting: @@ -396,7 +395,9 @@ def clean(self): if not error_message: assigned_object_type, assigned_object = interface_types[0] - host_type = ("device" if assigned_object_type == "interface" else "virtual_machine") + host_type = ( + "device" if assigned_object_type == "interface" else "virtual_machine" + ) # Get the parent host (device or virtual machine) of the assigned interface if assigned_object_type == "interface": @@ -407,26 +408,38 @@ def clean(self): assigned_object_id = VMInterface.objects.get(pk=assigned_object.pk).pk # Get the ContentType id for the assigned object - assigned_object_type_id = ContentType.objects.get_for_model(assigned_object).pk + assigned_object_type_id = ContentType.objects.get_for_model( + assigned_object + ).pk if not error_message: # Check if the parent host is assigned to the Access List - error_message |= self._check_if_interface_parent_is_assigned_to_access_list(cleaned_data.get("access_list"), assigned_object_type, host_type, host) + error_message |= self._check_if_interface_parent_is_assigned_to_access_list( + cleaned_data.get("access_list"), assigned_object_type, host_type, host + ) if not error_message: # Check for duplicate entries in the Access List - error_message |= self._check_for_duplicate_entry(cleaned_data.get("access_list"), assigned_object_id, assigned_object_type_id, cleaned_data.get("direction"),) + error_message |= self._check_for_duplicate_entry( + cleaned_data.get("access_list"), + assigned_object_id, + assigned_object_type_id, + cleaned_data.get("direction"), + ) if not error_message: # Check if the interface already has an ACL applied in the specified direction - error_message |= self._check_if_interface_already_has_acl_in_direction(assigned_object_id, assigned_object_type_id, cleaned_data.get("direction")) + error_message |= self._check_if_interface_already_has_acl_in_direction( + assigned_object_id, + assigned_object_type_id, + cleaned_data.get("direction"), + ) if error_message: raise forms.ValidationError(error_message) else: return cleaned_data - def _get_interface_types(self): """ Get interface type/model assigned to the Access List. @@ -452,7 +465,9 @@ def _validate_interface_types(self, interface_types): else: return {} - def _check_if_interface_parent_is_assigned_to_access_list(self, access_list, assigned_object_type, host_type, host): + def _check_if_interface_parent_is_assigned_to_access_list( + self, access_list, assigned_object_type, host_type, host + ): """ Check that an interface's parent device/virtual_machine is assigned to the Access List. """ @@ -460,18 +475,18 @@ def _check_if_interface_parent_is_assigned_to_access_list(self, access_list, ass access_list_host = AccessList.objects.get(pk=access_list.pk).assigned_object if access_list_host != host: - ERROR_ACL_NOT_ASSIGNED_TO_HOST = ( - "Access List not present on selected host." - ) + ERROR_ACL_NOT_ASSIGNED_TO_HOST = "Access List not present on selected host." return { "access_list": [ERROR_ACL_NOT_ASSIGNED_TO_HOST], assigned_object_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], host_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], - } + } else: return {} - def _check_for_duplicate_entry(self, access_list, assigned_object_id, assigned_object_type_id, direction): + def _check_for_duplicate_entry( + self, access_list, assigned_object_id, assigned_object_type_id, direction + ): """ Check for duplicate entry. (Because of GFK) """ @@ -486,15 +501,19 @@ def _check_for_duplicate_entry(self, access_list, assigned_object_id, assigned_o else: return {} - def _check_if_interface_already_has_acl_in_direction(self, assigned_object_id, assigned_object_type_id, direction): + def _check_if_interface_already_has_acl_in_direction( + self, assigned_object_id, assigned_object_type_id, direction + ): """ Check that the interface does not have an existing ACL applied in the direction already. """ - if ACLInterfaceAssignment.objects.filter( + if not ACLInterfaceAssignment.objects.filter( assigned_object_id=assigned_object_id, assigned_object_type=assigned_object_type_id, direction=direction, ).exists(): + return {} + else: error_interface_already_assigned = ( "Interfaces can only have 1 Access List assigned in each direction." ) @@ -502,9 +521,6 @@ def _check_if_interface_already_has_acl_in_direction(self, assigned_object_id, a "direction": [error_interface_already_assigned], assigned_object_type: [error_interface_already_assigned], } - else: - return {} - def save(self, *args, **kwargs): """ From bde16ddaef2440e5442e42d156b73f8dfe349ad3 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 15:07:34 +0000 Subject: [PATCH 22/42] refactor --- netbox_acls/forms/models.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 256b5060..f2639112 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -513,14 +513,14 @@ def _check_if_interface_already_has_acl_in_direction( direction=direction, ).exists(): return {} - else: - error_interface_already_assigned = ( - "Interfaces can only have 1 Access List assigned in each direction." - ) - return { - "direction": [error_interface_already_assigned], - assigned_object_type: [error_interface_already_assigned], - } + + error_interface_already_assigned = ( + "Interfaces can only have 1 Access List assigned in each direction." + ) + return { + "direction": [error_interface_already_assigned], + assigned_object_type: [error_interface_already_assigned], + } def save(self, *args, **kwargs): """ From 349b040d2639e9f72311b9177ff27f347d16cd61 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 16:34:44 +0000 Subject: [PATCH 23/42] refactor form models --- netbox_acls/forms/models.py | 143 ++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 79 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index f2639112..cffaf2ff 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -391,54 +391,35 @@ def clean(self): interface_types = self._get_interface_types() # Initialize an error message variable - error_message = self._validate_interface_types(interface_types) + self._validate_interface_types(interface_types) - if not error_message: - assigned_object_type, assigned_object = interface_types[0] - host_type = ( - "device" if assigned_object_type == "interface" else "virtual_machine" - ) + # Get the assigned interface & interface type + assigned_object_type, assigned_object = interface_types[0] - # Get the parent host (device or virtual machine) of the assigned interface - if assigned_object_type == "interface": - host = Interface.objects.get(pk=assigned_object.pk).device - assigned_object_id = Interface.objects.get(pk=assigned_object.pk).pk - else: - host = VMInterface.objects.get(pk=assigned_object.pk).virtual_machine - assigned_object_id = VMInterface.objects.get(pk=assigned_object.pk).pk - - # Get the ContentType id for the assigned object - assigned_object_type_id = ContentType.objects.get_for_model( - assigned_object - ).pk - - if not error_message: - # Check if the parent host is assigned to the Access List - error_message |= self._check_if_interface_parent_is_assigned_to_access_list( - cleaned_data.get("access_list"), assigned_object_type, host_type, host - ) + # Get the parent host (device or virtual machine) of the assigned interface + if assigned_object_type == "interface": + assigned_object_id = Interface.objects.get(pk=assigned_object.pk).pk + else: + assigned_object_id = VMInterface.objects.get(pk=assigned_object.pk).pk - if not error_message: - # Check for duplicate entries in the Access List - error_message |= self._check_for_duplicate_entry( - cleaned_data.get("access_list"), - assigned_object_id, - assigned_object_type_id, - cleaned_data.get("direction"), - ) + # Get the ContentType id for the assigned object + assigned_object_type_id = ContentType.objects.get_for_model(assigned_object).pk - if not error_message: - # Check if the interface already has an ACL applied in the specified direction - error_message |= self._check_if_interface_already_has_acl_in_direction( - assigned_object_id, - assigned_object_type_id, - cleaned_data.get("direction"), - ) + # Check if the parent host is assigned to the Access List + self._check_if_interface_parent_is_assigned_to_access_list( + cleaned_data.get("access_list"), assigned_object_type, assigned_object + ) - if error_message: - raise forms.ValidationError(error_message) - else: - return cleaned_data + # Check for duplicate entries in the Access List + self._check_if_interface_already_has_acl_in_direction( + cleaned_data.get("access_list"), + assigned_object_id, + assigned_object_type, + assigned_object_type_id, + cleaned_data.get("direction"), + ) + + return cleaned_data def _get_interface_types(self): """ @@ -458,69 +439,73 @@ def _validate_interface_types(self, interface_types): """ # Check if more than 1 hosts selected. if len(interface_types) > 1: - return "Assignment can only be to one interface at a time (either a interface or vm_interface)." + raise forms.ValidationError( + "Assignment can only be to one interface at a time (either a interface or vminterface)." + ) # Check if no hosts selected. elif not interface_types: - return "No interface or vm_interface selected." - else: - return {} + raise forms.ValidationError("No interface or vminterface selected.") def _check_if_interface_parent_is_assigned_to_access_list( - self, access_list, assigned_object_type, host_type, host + self, access_list, assigned_object_type, assigned_object ): """ Check that an interface's parent device/virtual_machine is assigned to the Access List. """ - access_list_host = AccessList.objects.get(pk=access_list.pk).assigned_object + host_type = ( + "device" if assigned_object_type == "interface" else "virtual_machine" + ) + if assigned_object_type == "interface": + host = Interface.objects.get(pk=assigned_object.pk).device + else: + host = VMInterface.objects.get(pk=assigned_object.pk).virtual_machine if access_list_host != host: ERROR_ACL_NOT_ASSIGNED_TO_HOST = "Access List not present on selected host." - return { - "access_list": [ERROR_ACL_NOT_ASSIGNED_TO_HOST], - assigned_object_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], - host_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], - } - else: - return {} + raise forms.ValidationError( + { + "access_list": [ERROR_ACL_NOT_ASSIGNED_TO_HOST], + assigned_object_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], + host_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], + } + ) - def _check_for_duplicate_entry( - self, access_list, assigned_object_id, assigned_object_type_id, direction + def _check_if_interface_already_has_acl_in_direction( + self, + access_list, + assigned_object_id, + assigned_object_type, + assigned_object_type_id, + direction, ): """ - Check for duplicate entry. (Because of GFK) + Check that the interface does not have an existing ACL applied in the direction already. """ + # Check for duplicate entry. (Because of GFK) if ACLInterfaceAssignment.objects.filter( access_list=access_list, assigned_object_id=assigned_object_id, assigned_object_type=assigned_object_type_id, direction=direction, ).exists(): - return {"access_list": ["Duplicate entry."]} - else: - return {} - - def _check_if_interface_already_has_acl_in_direction( - self, assigned_object_id, assigned_object_type_id, direction - ): - """ - Check that the interface does not have an existing ACL applied in the direction already. - """ - if not ACLInterfaceAssignment.objects.filter( + raise forms.ValidationError({"access_list": ["Duplicate entry."]}) + # Check that the interface does not have an existing ACL applied in the direction already. + elif ACLInterfaceAssignment.objects.filter( assigned_object_id=assigned_object_id, assigned_object_type=assigned_object_type_id, direction=direction, ).exists(): - return {} - - error_interface_already_assigned = ( - "Interfaces can only have 1 Access List assigned in each direction." - ) - return { - "direction": [error_interface_already_assigned], - assigned_object_type: [error_interface_already_assigned], - } + error_interface_already_assigned = ( + "Interfaces can only have 1 Access List assigned in each direction." + ) + raise forms.ValidationError( + { + "direction": [error_interface_already_assigned], + assigned_object_type: [error_interface_already_assigned], + } + ) def save(self, *args, **kwargs): """ From 5f5f24a979df4589e5550dd2e98d54c88aa5b385 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 17:07:36 +0000 Subject: [PATCH 24/42] refactor to reduce complexity --- netbox_acls/forms/models.py | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index cffaf2ff..18a5605f 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -250,22 +250,20 @@ def _validate_acl_type_change(self, acl_type, error_message): """ Check if Access List has no existing rules before change the Access List's type. """ - if self.instance.pk and ( - ( + if self.instance.pk: + error_message["type"] = [ + "This ACL has ACL rules associated, CANNOT change ACL type.", + ] + if ( acl_type == ACLTypeChoices.TYPE_EXTENDED and self.instance.aclstandardrules.exists() - ) - or ( + ): + raise forms.ValidationError(error_message) + if ( acl_type == ACLTypeChoices.TYPE_STANDARD and self.instance.aclextendedrules.exists() - ) - ): - error_message["type"] = [ - "This ACL has ACL rules associated, CANNOT change ACL type.", - ] - - if error_message: - raise forms.ValidationError(error_message) + ): + raise forms.ValidationError(error_message) def save(self, *args, **kwargs): """ @@ -406,12 +404,12 @@ def clean(self): assigned_object_type_id = ContentType.objects.get_for_model(assigned_object).pk # Check if the parent host is assigned to the Access List - self._check_if_interface_parent_is_assigned_to_access_list( + self._validate_if_interface_parent_is_assigned_to_access_list( cleaned_data.get("access_list"), assigned_object_type, assigned_object ) # Check for duplicate entries in the Access List - self._check_if_interface_already_has_acl_in_direction( + self._validate_if_interface_already_has_acl_in_direction( cleaned_data.get("access_list"), assigned_object_id, assigned_object_type, @@ -446,7 +444,7 @@ def _validate_interface_types(self, interface_types): elif not interface_types: raise forms.ValidationError("No interface or vminterface selected.") - def _check_if_interface_parent_is_assigned_to_access_list( + def _validate_if_interface_parent_is_assigned_to_access_list( self, access_list, assigned_object_type, assigned_object ): """ @@ -471,7 +469,7 @@ def _check_if_interface_parent_is_assigned_to_access_list( } ) - def _check_if_interface_already_has_acl_in_direction( + def _validate_if_interface_already_has_acl_in_direction( self, access_list, assigned_object_id, @@ -573,7 +571,7 @@ def clean(self): # No need to check for unique_together since there is no usage of GFK if cleaned_data.get("action") == "remark": - self._extracted_from_clean_20(cleaned_data, error_message, "extended") + self._validate_acl_rules(cleaned_data, error_message, "extended") # Check remark set, but action not set to remark. elif cleaned_data.get("remark"): error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] @@ -582,7 +580,7 @@ def clean(self): raise forms.ValidationError(error_message) return cleaned_data - def _extracted_from_clean_20(self, cleaned_data, error_message, rule_type): + def _validate_acl_rules(self, cleaned_data, error_message, rule_type): """ Validates form inputs before submitting: - Check if action set to remark, but no remark set. From e9aa9189dfe47a90ce66d2fea1fc185f6ac96661 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 18:02:39 +0000 Subject: [PATCH 25/42] optimize form models --- netbox_acls/forms/constants.py | 7 +-- netbox_acls/forms/models.py | 100 +++++++++++++++------------------ 2 files changed, 47 insertions(+), 60 deletions(-) diff --git a/netbox_acls/forms/constants.py b/netbox_acls/forms/constants.py index 18dbb22f..dac7a8e6 100644 --- a/netbox_acls/forms/constants.py +++ b/netbox_acls/forms/constants.py @@ -7,8 +7,10 @@ HELP_TEXT_ACL_RULE_LOGIC = mark_safe( "*Note: CANNOT be set if action is set to remark.", ) + # Sets a standard help_text value to be used by the various classes for acl action HELP_TEXT_ACL_ACTION = "Action the rule will take (remark, deny, or allow)." + # Sets a standard help_text value to be used by the various classes for acl index HELP_TEXT_ACL_RULE_INDEX = ( "Determines the order of the rule in the ACL processing. AKA Sequence Number." @@ -16,10 +18,7 @@ # Sets a standard error message for ACL rules with an action of remark, but no remark set. ERROR_MESSAGE_NO_REMARK = "Action is set to remark, you MUST add a remark." -# Sets a standard error message for ACL rules with an action of remark, but no source_prefix is set. -ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET = ( - "Action is set to remark, Source Prefix CANNOT be set." -) + # Sets a standard error message for ACL rules with an action not set to remark, but no remark is set. ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK = ( "CANNOT set remark unless action is set to remark." diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 18a5605f..58176713 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -19,7 +19,6 @@ from ..choices import ACLTypeChoices from .constants import ( - ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, ERROR_MESSAGE_NO_REMARK, ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK, HELP_TEXT_ACL_ACTION, @@ -185,14 +184,14 @@ def clean(self): host_types = self._get_host_types() # Check if no hosts selected. - self._validate_host_types(host_types) + self._clean_check_host_types(host_types) host_type, host = host_types[0] # Check if duplicate entry. - self._validate_duplicate_entry(name, host_type, host, error_message) + self._clean_check_duplicate_entry(name, host_type, host, error_message) # Check if Access List has no existing rules before change the Access List's type. - self._validate_acl_type_change(acl_type, error_message) + self._clean_check_acl_type_change(acl_type, error_message) if error_message: raise forms.ValidationError(error_message) @@ -213,9 +212,9 @@ def _get_host_types(self): ] return [x for x in host_types if x[1]] - def _validate_host_types(self, host_types): + def _clean_check_host_types(self, host_types): """ - Check number of host types selected. + Used by parent class's clean method. Check number of host types selected. """ if len(host_types) > 1: raise forms.ValidationError( @@ -227,9 +226,9 @@ def _validate_host_types(self, host_types): "Access Lists must be assigned to a device, virtual chassis or virtual machine.", ) - def _validate_duplicate_entry(self, name, host_type, host, error_message): + def _clean_check_duplicate_entry(self, name, host_type, host, error_message): """ - Check if duplicate entry. (Because of GFK.) + Used by parent class's clean method. Check if duplicate entry. (Because of GFK.) """ existing_acls = AccessList.objects.filter( name=name, **{host_type: host} @@ -246,9 +245,9 @@ def _validate_duplicate_entry(self, name, host_type, host, error_message): "name": [error_same_acl_name], } - def _validate_acl_type_change(self, acl_type, error_message): + def _clean_check_acl_type_change(self, acl_type, error_message): """ - Check if Access List has no existing rules before change the Access List's type. + Used by parent class's clean method. Check if Access List has no existing rules before change the Access List's type. """ if self.instance.pk: error_message["type"] = [ @@ -386,10 +385,10 @@ def clean(self): cleaned_data = super().clean() # Get the interface types assigned to the Access List - interface_types = self._get_interface_types() + interface_types = self._clean_get_interface_types() # Initialize an error message variable - self._validate_interface_types(interface_types) + self._clean_check_interface_types(interface_types) # Get the assigned interface & interface type assigned_object_type, assigned_object = interface_types[0] @@ -404,12 +403,12 @@ def clean(self): assigned_object_type_id = ContentType.objects.get_for_model(assigned_object).pk # Check if the parent host is assigned to the Access List - self._validate_if_interface_parent_is_assigned_to_access_list( + self._clean_check_if_interface_parent_is_assigned_to_access_list( cleaned_data.get("access_list"), assigned_object_type, assigned_object ) # Check for duplicate entries in the Access List - self._validate_if_interface_already_has_acl_in_direction( + self._clean_check_if_interface_already_has_acl_in_direction( cleaned_data.get("access_list"), assigned_object_id, assigned_object_type, @@ -419,9 +418,9 @@ def clean(self): return cleaned_data - def _get_interface_types(self): + def _clean_get_interface_types(self): """ - Get interface type/model assigned to the Access List. + Used by parent class's clean method. Get interface type/model assigned to the Access List. """ interface = self.cleaned_data.get("interface") vminterface = self.cleaned_data.get("vminterface") @@ -431,9 +430,9 @@ def _get_interface_types(self): ] return [x for x in interface_types if x[1]] - def _validate_interface_types(self, interface_types): + def _clean_check_interface_types(self, interface_types): """ - Check if number of interface type selected is 1. + Used by parent class's clean method. Check if number of interface type selected is 1. """ # Check if more than 1 hosts selected. if len(interface_types) > 1: @@ -444,11 +443,11 @@ def _validate_interface_types(self, interface_types): elif not interface_types: raise forms.ValidationError("No interface or vminterface selected.") - def _validate_if_interface_parent_is_assigned_to_access_list( + def _clean_check_if_interface_parent_is_assigned_to_access_list( self, access_list, assigned_object_type, assigned_object ): """ - Check that an interface's parent device/virtual_machine is assigned to the Access List. + Used by parent class's clean method. Check that an interface's parent device/virtual_machine is assigned to the Access List. """ access_list_host = AccessList.objects.get(pk=access_list.pk).assigned_object host_type = ( @@ -469,7 +468,7 @@ def _validate_if_interface_parent_is_assigned_to_access_list( } ) - def _validate_if_interface_already_has_acl_in_direction( + def _clean_check_if_interface_already_has_acl_in_direction( self, access_list, assigned_object_id, @@ -489,8 +488,9 @@ def _validate_if_interface_already_has_acl_in_direction( direction=direction, ).exists(): raise forms.ValidationError({"access_list": ["Duplicate entry."]}) + # Check that the interface does not have an existing ACL applied in the direction already. - elif ACLInterfaceAssignment.objects.filter( + if ACLInterfaceAssignment.objects.filter( assigned_object_id=assigned_object_id, assigned_object_type=assigned_object_type_id, direction=direction, @@ -571,7 +571,7 @@ def clean(self): # No need to check for unique_together since there is no usage of GFK if cleaned_data.get("action") == "remark": - self._validate_acl_rules(cleaned_data, error_message, "extended") + self._clean_check_acl_rules(cleaned_data, error_message, "extended") # Check remark set, but action not set to remark. elif cleaned_data.get("remark"): error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] @@ -580,45 +580,33 @@ def clean(self): raise forms.ValidationError(error_message) return cleaned_data - def _validate_acl_rules(self, cleaned_data, error_message, rule_type): + def _clean_check_acl_rules(self, cleaned_data, error_message, rule_type): """ - Validates form inputs before submitting: - - Check if action set to remark, but no remark set. - - Check if action set to remark, but source_prefix set. - - Check if action set to remark, but source_ports set. - - Check if action set to remark, but destination_prefix set. - - Check if action set to remark, but destination_ports set. - - Check if action set to remark, but destination_ports set. - - Check if action set to remark, but protocol set. + Used by parent class's clean method. Checks form inputs before submitting: + - Check if action set to remark, but no remark set. + - Check if action set to remark, but other rule attributes set. """ # Check if action set to remark, but no remark set. if not cleaned_data.get("remark"): error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] - # Check if action set to remark, but source_prefix set. - if cleaned_data.get("source_prefix"): - error_message["source_prefix"] = [ - ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET - ] + + # list all the fields of a rule besides the remark if rule_type == "extended": - # Check if action set to remark, but source_ports set. - if cleaned_data.get("source_ports"): - error_message["source_ports"] = [ - "Action is set to remark, Source Ports CANNOT be set." - ] - # Check if action set to remark, but destination_prefix set. - if cleaned_data.get("destination_prefix"): - error_message["destination_prefix"] = [ - "Action is set to remark, Destination Prefix CANNOT be set.", - ] - # Check if action set to remark, but destination_ports set. - if cleaned_data.get("destination_ports"): - error_message["destination_ports"] = [ - "Action is set to remark, Destination Ports CANNOT be set." - ] - # Check if action set to remark, but protocol set. - if cleaned_data.get("protocol"): - error_message["protocol"] = [ - "Action is set to remark, Protocol CANNOT be set." + rule_attributes = [ + "source_prefix", + "source_ports", + "destination_prefix", + "destination_ports", + "protocol", + ] + else: + rule_attributes = ["source_prefix"] + + # Check if action set to remark, but other fields set. + for attribute in rule_attributes: + if cleaned_data.get(attribute): + error_message[attribute] = [ + f'Action is set to remark, {attribute.replace("_", " ").title()} CANNOT be set.' ] From 930a40cc47505506e6e19aec160b45897eef7ff6 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 21:03:23 +0000 Subject: [PATCH 26/42] update acl forms --- netbox_acls/templates/inc/common_edit.html | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/netbox_acls/templates/inc/common_edit.html b/netbox_acls/templates/inc/common_edit.html index c2963581..cca62078 100644 --- a/netbox_acls/templates/inc/common_edit.html +++ b/netbox_acls/templates/inc/common_edit.html @@ -1,13 +1,11 @@ {% load form_helpers %} +{% if form.custom_fields %} +
+

Custom Fields

+ {% render_custom_fields form %} +
+{% endif %}

Comments

{% render_field form.comments %}
-{% if form.custom_fields %} -
-
Custom Fields
-
- {% render_custom_fields form %} -
-
-{% endif %} From 9cdbf589cd8064219cde2a8fb186f3e4113a2fa2 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 21:04:08 +0000 Subject: [PATCH 27/42] update BaseACLRuleForm --- netbox_acls/forms/models.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 58176713..cb3880f0 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -543,16 +543,17 @@ class BaseACLRuleForm(NetBoxModelForm): class Meta: """Defines the Model and fields to be used by the form.""" - model = ACLStandardRule + abstract = True + model = BaseACLRule fields = ( "access_list", "index", "action", "remark", - "source_prefix", "tags", "description", ) + # Adds a help text to the form fields where the field is not defined in the form class. help_texts = { "action": HELP_TEXT_ACL_ACTION, "destination_ports": HELP_TEXT_ACL_RULE_LOGIC, @@ -617,6 +618,13 @@ class ACLStandardRuleForm(BaseACLRuleForm): See the clean function for logic on other field requirements. """ + class Meta(BaseACLRuleForm.Meta): + """Defines the Model and fields to be used by the form.""" + + model = ACLStandardRule + # Need to add source_prefix to the tuple here instead of base or it will cause a ValueError + fields = BaseACLRuleForm.Meta.fields + ("source_prefix",) + class ACLExtendedRuleForm(BaseACLRuleForm): """ @@ -648,20 +656,14 @@ class ACLExtendedRuleForm(BaseACLRuleForm): ), ) - class Meta: + class Meta(BaseACLRuleForm.Meta): """Defines the Model and fields to be used by the form.""" model = ACLExtendedRule - fields = ( - "access_list", - "index", - "action", - "remark", + fields = BaseACLRuleForm.Meta.fields + ( "source_prefix", "source_ports", "destination_prefix", "destination_ports", "protocol", - "tags", - "description", ) From f31b99cda084eb98b9e1ede8c3eb536805922030 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 21:04:21 +0000 Subject: [PATCH 28/42] update forms error order --- netbox_acls/forms/models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index cb3880f0..f71f13c3 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -30,6 +30,7 @@ ACLExtendedRule, ACLInterfaceAssignment, ACLStandardRule, + BaseACLRule, ) __all__ = ( @@ -463,8 +464,8 @@ def _clean_check_if_interface_parent_is_assigned_to_access_list( raise forms.ValidationError( { "access_list": [ERROR_ACL_NOT_ASSIGNED_TO_HOST], - assigned_object_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], host_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], + assigned_object_type: [ERROR_ACL_NOT_ASSIGNED_TO_HOST], } ) From b316b749d3479e5808bf76b0a59328ba8f6496d6 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Mon, 30 Jan 2023 23:15:17 +0000 Subject: [PATCH 29/42] refactor forms models --- netbox_acls/forms/models.py | 98 ++++++++++++++++++++++++------------- 1 file changed, 65 insertions(+), 33 deletions(-) diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index f71f13c3..4c1a79ba 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -174,7 +174,6 @@ def clean(self): """ # TODO: Refactor this method to fix error message logic. cleaned_data = super().clean() - error_message = {} if self.errors.get("name"): return cleaned_data @@ -190,12 +189,9 @@ def clean(self): host_type, host = host_types[0] # Check if duplicate entry. - self._clean_check_duplicate_entry(name, host_type, host, error_message) + self._clean_check_duplicate_entry(name, host_type, host) # Check if Access List has no existing rules before change the Access List's type. - self._clean_check_acl_type_change(acl_type, error_message) - - if error_message: - raise forms.ValidationError(error_message) + self._clean_check_acl_type_change(acl_type) return cleaned_data @@ -227,7 +223,7 @@ def _clean_check_host_types(self, host_types): "Access Lists must be assigned to a device, virtual chassis or virtual machine.", ) - def _clean_check_duplicate_entry(self, name, host_type, host, error_message): + def _clean_check_duplicate_entry(self, name, host_type, host): """ Used by parent class's clean method. Check if duplicate entry. (Because of GFK.) """ @@ -241,29 +237,40 @@ def _clean_check_duplicate_entry(self, name, host_type, host, error_message): error_same_acl_name = ( "An ACL with this name is already associated to this host." ) - error_message |= { - host_type: [error_same_acl_name], - "name": [error_same_acl_name], - } + raise forms.ValidationError( + { + host_type: [error_same_acl_name], + "name": [error_same_acl_name], + } + ) - def _clean_check_acl_type_change(self, acl_type, error_message): + def _clean_check_acl_type_change(self, acl_type): """ Used by parent class's clean method. Check if Access List has no existing rules before change the Access List's type. """ if self.instance.pk: - error_message["type"] = [ + ERROR_MESSAGE_EXISTING_RULES = ( "This ACL has ACL rules associated, CANNOT change ACL type.", - ] + ) if ( acl_type == ACLTypeChoices.TYPE_EXTENDED and self.instance.aclstandardrules.exists() ): - raise forms.ValidationError(error_message) + raise forms.ValidationError( + { + "type": ERROR_MESSAGE_EXISTING_RULES, + } + ) + if ( acl_type == ACLTypeChoices.TYPE_STANDARD and self.instance.aclextendedrules.exists() ): - raise forms.ValidationError(error_message) + raise forms.ValidationError( + { + "type": ERROR_MESSAGE_EXISTING_RULES, + } + ) def save(self, *args, **kwargs): """ @@ -566,23 +573,7 @@ class Meta: "source_ports": HELP_TEXT_ACL_RULE_LOGIC, } - def clean(self): - cleaned_data = super().clean() - error_message = {} - - # No need to check for unique_together since there is no usage of GFK - - if cleaned_data.get("action") == "remark": - self._clean_check_acl_rules(cleaned_data, error_message, "extended") - # Check remark set, but action not set to remark. - elif cleaned_data.get("remark"): - error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] - - if error_message: - raise forms.ValidationError(error_message) - return cleaned_data - - def _clean_check_acl_rules(self, cleaned_data, error_message, rule_type): + def clean_check_remark_rule(self, cleaned_data, error_message, rule_type): """ Used by parent class's clean method. Checks form inputs before submitting: - Check if action set to remark, but no remark set. @@ -611,6 +602,9 @@ def _clean_check_acl_rules(self, cleaned_data, error_message, rule_type): f'Action is set to remark, {attribute.replace("_", " ").title()} CANNOT be set.' ] + if error_message: + forms.ValidationError(error_message) + class ACLStandardRuleForm(BaseACLRuleForm): """ @@ -626,6 +620,25 @@ class Meta(BaseACLRuleForm.Meta): # Need to add source_prefix to the tuple here instead of base or it will cause a ValueError fields = BaseACLRuleForm.Meta.fields + ("source_prefix",) + def clean(self): + cleaned_data = super().clean() + error_message = {} + + # Check if action set to remark, but no remark set OR other fields set. + if cleaned_data.get("action") == "remark": + BaseACLRuleForm.clean_check_remark_rule( + self, cleaned_data, error_message, rule_type="standard" + ) + + # Check remark set, but action not set to remark. + elif cleaned_data.get("remark"): + error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] + + if error_message: + raise forms.ValidationError(error_message) + + return cleaned_data + class ACLExtendedRuleForm(BaseACLRuleForm): """ @@ -668,3 +681,22 @@ class Meta(BaseACLRuleForm.Meta): "destination_ports", "protocol", ) + + def clean(self): + cleaned_data = super().clean() + error_message = {} + + # Check if action set to remark, but no remark set OR other fields set. + if cleaned_data.get("action") == "remark": + BaseACLRuleForm.clean_check_remark_rule( + self, cleaned_data, error_message, rule_type="extended" + ) + + # Check remark set, but action not set to remark. + elif cleaned_data.get("remark"): + error_message["remark"] = [ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK] + + if error_message: + raise forms.ValidationError(error_message) + + return cleaned_data From 4c64827c767b1883c0ec75dd6d1a069d77634d57 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Tue, 31 Jan 2023 04:46:47 +0000 Subject: [PATCH 30/42] minimize redudant code in serializers --- netbox_acls/api/serializers.py | 238 ++++++++++++++++----------------- 1 file changed, 117 insertions(+), 121 deletions(-) diff --git a/netbox_acls/api/serializers.py b/netbox_acls/api/serializers.py index 10da14a9..9664d2e9 100644 --- a/netbox_acls/api/serializers.py +++ b/netbox_acls/api/serializers.py @@ -16,6 +16,7 @@ from ..constants import ACL_HOST_ASSIGNMENT_MODELS, ACL_INTERFACE_ASSIGNMENT_MODELS from ..models import ( AccessList, + BaseACLRule, ACLExtendedRule, ACLInterfaceAssignment, ACLStandardRule, @@ -44,6 +45,27 @@ ERROR_MESSAGE_ACL_TYPE = "Provided parent Access List is not of right type." +def validate_gfk(data): + """ + Check that the GFK object is valid. + """ + # TODO: This can removed after https://github.com/netbox-community/netbox/issues/10221 is fixed. + try: + assigned_object = data[ # noqa: F841 + "assigned_object_type" + ].get_object_for_this_type( + id=data["assigned_object_id"], + ) + except ObjectDoesNotExist as e: + error_message_invalid_gfk = f"Invalid assigned_object {data['assigned_object_type']} ID {data['assigned_object_id']}" + raise serializers.ValidationError( + { + "assigned_object_type": [error_message_invalid_gfk], + "assigned_object_id": [error_message_invalid_gfk], + } + ) from e + + class AccessListSerializer(NetBoxModelSerializer): """ Defines the serializer for the django AccessList model & associates it to a view. @@ -97,25 +119,11 @@ def get_assigned_object(self, obj): def validate(self, data): """ Validates api inputs before processing: - - Check that the GFK object is valid. - - Check if Access List has no existing rules before change the Access List's type. + - Check that the GFK object is valid. + - Check if Access List has no existing rules before change the Access List's type. """ - error_message = {} - # Check that the GFK object is valid. - if "assigned_object_type" in data and "assigned_object_id" in data: - # TODO: This can removed after https://github.com/netbox-community/netbox/issues/10221 is fixed. - try: - assigned_object = data[ # noqa: F841 - "assigned_object_type" - ].get_object_for_this_type( - id=data["assigned_object_id"], - ) - except ObjectDoesNotExist: - # Sets a standard error message for invalid GFK - error_message_invalid_gfk = f"Invalid assigned_object {data['assigned_object_type']} ID {data['assigned_object_id']}" - error_message["assigned_object_type"] = [error_message_invalid_gfk] - error_message["assigned_object_id"] = [error_message_invalid_gfk] + assigned_object = validate_gfk(data) # Check if Access List has no existing rules before change the Access List's type. if ( @@ -123,9 +131,9 @@ def validate(self, data): and self.instance.type != data.get("type") and self.instance.rule_count > 0 ): - error_message["type"] = [ - "This ACL has ACL rules associated, CANNOT change ACL type.", - ] + raise serializers.ValidationError( + {"type": ["This ACL has ACL rules associated, CANNOT change ACL type."]} + ) if error_message: raise serializers.ValidationError(error_message) @@ -180,51 +188,51 @@ def get_assigned_object(self, obj): context = {"request": self.context["request"]} return serializer(obj.assigned_object, context=context).data + def _validate_get_interface_host(self, data, assigned_object): + """ + Check that the associated interface's parent host has the selected ACL defined. + """ + MODEL_MAPPING = { + "interface": "device", + "vminterface": "virtual_machine", + } + + assigned_object_model = data["assigned_object_type"].model + + return getattr(assigned_object, MODEL_MAPPING.get(assigned_object_model, None)) + + def _validate_acl_host(self, acl_host, interface_host): + """ + Check that the associated interface's parent host has the selected ACL defined. + """ + if acl_host == interface_host: + return {} + + error_acl_not_assigned_to_host = ( + "Access List not present on the selected interface's host." + ) + return { + "access_list": [error_acl_not_assigned_to_host], + "assigned_object_id": [error_acl_not_assigned_to_host], + } + def validate(self, data): """ Validate the AccessList django model's inputs before allowing it to update the instance. - - Check that the GFK object is valid. - - Check that the associated interface's parent host has the selected ACL defined. + - Check that the GFK object is valid. + - Check that the associated interface's parent host has the selected ACL defined. """ + + # Check that the GFK object is valid. + assigned_object = validate_gfk(data) + error_message = {} acl_host = data["access_list"].assigned_object - # Check that the GFK object is valid. - if "assigned_object_type" in data and "assigned_object_id" in data: - # TODO: This can removed after https://github.com/netbox-community/netbox/issues/10221 is fixed. - try: - assigned_object = data[ # noqa: F841 - "assigned_object_type" - ].get_object_for_this_type( - id=data["assigned_object_id"], - ) - except ObjectDoesNotExist: - # Sets a standard error message for invalid GFK - error_message_invalid_gfk = f"Invalid assigned_object {data['assigned_object_type']} ID {data['assigned_object_id']}" - error_message["assigned_object_type"] = [error_message_invalid_gfk] - error_message["assigned_object_id"] = [error_message_invalid_gfk] - - if data["assigned_object_type"].model == "interface": - interface_host = ( - data["assigned_object_type"] - .get_object_for_this_type(id=data["assigned_object_id"]) - .device - ) - elif data["assigned_object_type"].model == "vminterface": - interface_host = ( - data["assigned_object_type"] - .get_object_for_this_type(id=data["assigned_object_id"]) - .virtual_machine - ) - else: - interface_host = None - # Check that the associated interface's parent host has the selected ACL defined. - if acl_host != interface_host: - error_acl_not_assigned_to_host = ( - "Access List not present on the selected interface's host." - ) - error_message["access_list"] = [error_acl_not_assigned_to_host] - error_message["assigned_object_id"] = [error_acl_not_assigned_to_host] + interface_host = self._validate_get_interface_host(data, assigned_object) + acl_host = data["access_list"].assigned_object + + error_message |= self._validate_acl_host(acl_host, interface_host) if error_message: raise serializers.ValidationError(error_message) @@ -232,14 +240,11 @@ def validate(self, data): return super().validate(data) -class ACLStandardRuleSerializer(NetBoxModelSerializer): +class BaseACLRuleSerializer(NetBoxModelSerializer): """ - Defines the serializer for the django ACLStandardRule model & associates it to a view. + Defines the serializer for the django BaseACLRule model & associates it to a view. """ - url = serializers.HyperlinkedIdentityField( - view_name="plugins-api:netbox_acls-api:aclstandardrule-detail", - ) access_list = NestedAccessListSerializer() source_prefix = NestedPrefixSerializer( required=False, @@ -249,10 +254,11 @@ class ACLStandardRuleSerializer(NetBoxModelSerializer): class Meta: """ - Associates the django model ACLStandardRule & fields to the serializer. + Associates the django model BaseACLRule & fields to the serializer. """ - model = ACLStandardRule + abstract = True + model = BaseACLRule fields = ( "id", "url", @@ -271,9 +277,9 @@ class Meta: def validate(self, data): """ - Validate the ACLStandardRule django model's inputs before allowing it to update the instance: - - Check if action set to remark, but no remark set. - - Check if action set to remark, but source_prefix set. + Validate the BaseACLRule django model's inputs before allowing it to update the instance: + - Check if action set to remark, but no remark set. + - Check if action set to remark, but source_prefix set. """ error_message = {} @@ -292,7 +298,24 @@ def validate(self, data): return super().validate(data) -class ACLExtendedRuleSerializer(NetBoxModelSerializer): +class ACLStandardRuleSerializer(BaseACLRuleSerializer): + """ + Defines the serializer for the django ACLStandardRule model & associates it to a view. + """ + + url = serializers.HyperlinkedIdentityField( + view_name="plugins-api:netbox_acls-api:aclstandardrule-detail", + ) + + class Meta(BaseACLRuleSerializer.Meta): + """ + Associates the django model ACLStandardRule & fields to the serializer. + """ + + model = ACLStandardRule + + +class ACLExtendedRuleSerializer(BaseACLRuleSerializer): """ Defines the serializer for the django ACLExtendedRule model & associates it to a view. """ @@ -300,37 +323,21 @@ class ACLExtendedRuleSerializer(NetBoxModelSerializer): url = serializers.HyperlinkedIdentityField( view_name="plugins-api:netbox_acls-api:aclextendedrule-detail", ) - access_list = NestedAccessListSerializer() - source_prefix = NestedPrefixSerializer( - required=False, - allow_null=True, - default=None, - ) destination_prefix = NestedPrefixSerializer( required=False, allow_null=True, default=None, ) - class Meta: + class Meta(BaseACLRuleSerializer.Meta): """ Associates the django model ACLExtendedRule & fields to the serializer. """ model = ACLExtendedRule - fields = ( - "id", - "url", - "display", - "access_list", - "index", - "action", - "tags", - "description", - "created", - "custom_fields", - "last_updated", - "source_prefix", + + # Add the additional fields to the serializer to support Extended ACL Rules. + fields = BaseACLRuleSerializer.Meta.fields + ( "source_ports", "destination_prefix", "destination_ports", @@ -341,44 +348,33 @@ class Meta: def validate(self, data): """ Validate the ACLExtendedRule django model's inputs before allowing it to update the instance: - - Check if action set to remark, but no remark set. - - Check if action set to remark, but source_prefix set. - - Check if action set to remark, but source_ports set. - - Check if action set to remark, but destination_prefix set. - - Check if action set to remark, but destination_ports set. - - Check if action set to remark, but protocol set. - - Check if action set to remark, but protocol set. + - Check if action set to remark, but no remark set. + - Check if action set to remark, but source_prefix set. + - Check if action set to remark, but source_ports set. + - Check if action set to remark, but destination_prefix set. + - Check if action set to remark, but destination_ports set. + - Check if action set to remark, but protocol set. + - Check if action set to remark, but protocol set. """ error_message = {} + rule_attributes = [ + "source_prefix", + "source_ports", + "destination_prefix", + "destination_ports", + "protocol", + ] # Check if action set to remark, but no remark set. if data.get("action") == "remark" and data.get("remark") is None: error_message["remark"] = [ERROR_MESSAGE_NO_REMARK] - # Check if action set to remark, but source_prefix set. - if data.get("source_prefix"): - error_message["source_prefix"] = [ - ERROR_MESSAGE_ACTION_REMARK_SOURCE_PREFIX_SET, - ] - # Check if action set to remark, but source_ports set. - if data.get("source_ports"): - error_message["source_ports"] = [ - "Action is set to remark, Source Ports CANNOT be set.", - ] - # Check if action set to remark, but destination_prefix set. - if data.get("destination_prefix"): - error_message["destination_prefix"] = [ - "Action is set to remark, Destination Prefix CANNOT be set.", - ] - # Check if action set to remark, but destination_ports set. - if data.get("destination_ports"): - error_message["destination_ports"] = [ - "Action is set to remark, Destination Ports CANNOT be set.", - ] - # Check if action set to remark, but protocol set. - if data.get("protocol"): - error_message["protocol"] = [ - "Action is set to remark, Protocol CANNOT be set.", - ] + + # Check if action set to remark, but other fields set. + for attribute in rule_attributes: + if data.get(attribute): + error_message[attribute] = [ + f'Action is set to remark, {attribute.replace("_", " ").title()} CANNOT be set.' + ] if error_message: raise serializers.ValidationError(error_message) From 0c71815f57d235baccde55b2e8c6195dfe0a5ae6 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Tue, 31 Jan 2023 04:46:52 +0000 Subject: [PATCH 31/42] housekeeping --- Makefile | 3 +++ TODO | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index aa6c2f9a..75cbb1ff 100644 --- a/Makefile +++ b/Makefile @@ -70,6 +70,9 @@ start: .PHONY: all ## Run all PLUGIN DEV targets all: setup makemigrations migrate collectstatic initializers start +.PHONY: rebuild ## Run PLUGIN DEV targets to rebuild +rebuild: setup makemigrations migrate collectstatic start + #.PHONY: test #test: # ${VENV_PY_PATH} /opt/netbox/netbox/manage.py runserver test ${PLUGIN_NAME} diff --git a/TODO b/TODO index 8e086202..8bddc8cf 100644 --- a/TODO +++ b/TODO @@ -1 +1,7 @@ -- TODO: single underscore functions https://stackoverflow.com/questions/1301346/what-is-the-meaning-of-single-and-double-underscore-before-an-object-name +- TODO: ACL Form Bubble/ICON Extended/Standard +- TODO: Add an Access List to an Interface Custom Fields after comments - DONE +- TODO: ACL rules, look at last number and increment to next 10 +- TODO: Clone for ACL Interface should include device +- TODO: Inconsistent errors for add/edit (where model is using a generic page) +- TODO: Check Constants across codebase for consistency. +- TODO: Test API and Forms From 8354c4fffd385932d27c859fbf54fad5171285c8 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Tue, 31 Jan 2023 13:30:49 +0000 Subject: [PATCH 32/42] add ai tooling to development --- .devcontainer/devcontainer.json | 4 +++- .devcontainer/requirements-dev.txt | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index a3fc26fb..37404e7c 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -91,7 +91,9 @@ "mutantdino.resourcemonitor", "paulomenezes.duplicated-code", "searKing.preview-vscode", - "sourcery.sourcery" + "sourcery.sourcery", + "GitHub.copilot", + "GitHub.copilot-labs" ] } }, diff --git a/.devcontainer/requirements-dev.txt b/.devcontainer/requirements-dev.txt index 13a832d2..d4a50d5e 100644 --- a/.devcontainer/requirements-dev.txt +++ b/.devcontainer/requirements-dev.txt @@ -12,3 +12,4 @@ pylint pylint-django wily yapf +sourcery-analytics From 09f67a4ba0796b26c0bd4589ee0326b886ba3b2a Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Tue, 31 Jan 2023 13:52:12 +0000 Subject: [PATCH 33/42] add in devcontainer testing --- Makefile | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 75cbb1ff..fb3ba16e 100644 --- a/Makefile +++ b/Makefile @@ -73,9 +73,10 @@ all: setup makemigrations migrate collectstatic initializers start .PHONY: rebuild ## Run PLUGIN DEV targets to rebuild rebuild: setup makemigrations migrate collectstatic start -#.PHONY: test -#test: -# ${VENV_PY_PATH} /opt/netbox/netbox/manage.py runserver test ${PLUGIN_NAME} +.PHONY: test +test: setup + ${VENV_PY_PATH} ${NETBOX_MANAGE_PATH}/manage.py makemigrations ${PLUGIN_NAME} --check + ${VENV_PY_PATH} ${NETBOX_MANAGE_PATH}/manage.py test ${PLUGIN_NAME} #relpatch: # $(eval GSTATUS := $(shell git status --porcelain)) From 18b95c43499930e4fb0259a94d17a66988f49620 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Tue, 31 Jan 2023 13:52:17 +0000 Subject: [PATCH 34/42] update TODO --- TODO | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/TODO b/TODO index 8bddc8cf..5218b1db 100644 --- a/TODO +++ b/TODO @@ -4,4 +4,4 @@ - TODO: Clone for ACL Interface should include device - TODO: Inconsistent errors for add/edit (where model is using a generic page) - TODO: Check Constants across codebase for consistency. -- TODO: Test API and Forms +- TODO: Test API, Forms, & Models - https://github.com/k01ek/netbox-bgp/tree/main/netbox_bgp/tests , https://github.com/DanSheps/netbox-secretstore/tree/develop/netbox_secretstore/tests & https://github.com/FlxPeters/netbox-plugin-prometheus-sd/tree/main/netbox_prometheus_sd/tests From c04be89d1f79b7cf4aa1c78c3a74c6c4b63e3b1d Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Wed, 1 Feb 2023 13:27:05 +0000 Subject: [PATCH 35/42] black formatting --- netbox_acls/migrations/0002_alter_accesslist_options_and_more.py | 1 - netbox_acls/migrations/0003_netbox_acls.py | 1 - 2 files changed, 2 deletions(-) diff --git a/netbox_acls/migrations/0002_alter_accesslist_options_and_more.py b/netbox_acls/migrations/0002_alter_accesslist_options_and_more.py index ea5ab861..902248f0 100644 --- a/netbox_acls/migrations/0002_alter_accesslist_options_and_more.py +++ b/netbox_acls/migrations/0002_alter_accesslist_options_and_more.py @@ -6,7 +6,6 @@ class Migration(migrations.Migration): - dependencies = [ ("contenttypes", "0002_remove_content_type_name"), ("netbox_acls", "0001_initial"), diff --git a/netbox_acls/migrations/0003_netbox_acls.py b/netbox_acls/migrations/0003_netbox_acls.py index 562e0f3d..0af5aef4 100644 --- a/netbox_acls/migrations/0003_netbox_acls.py +++ b/netbox_acls/migrations/0003_netbox_acls.py @@ -5,7 +5,6 @@ class Migration(migrations.Migration): - dependencies = [ ("netbox_acls", "0002_alter_accesslist_options_and_more"), ] From 85861457dcb535cd3c23e73c7554d7e039939698 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Wed, 1 Feb 2023 22:56:14 +0000 Subject: [PATCH 36/42] fix acl name validator --- netbox_acls/migrations/0004_netbox_acls.py | 26 ++++++++++++++++++++++ netbox_acls/models/access_lists.py | 2 +- 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 netbox_acls/migrations/0004_netbox_acls.py diff --git a/netbox_acls/migrations/0004_netbox_acls.py b/netbox_acls/migrations/0004_netbox_acls.py new file mode 100644 index 00000000..aeac4363 --- /dev/null +++ b/netbox_acls/migrations/0004_netbox_acls.py @@ -0,0 +1,26 @@ +# Generated by Django 4.1.5 on 2023-02-01 22:47 + +import django.core.validators +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("netbox_acls", "0003_netbox_acls"), + ] + + operations = [ + migrations.AlterField( + model_name="accesslist", + name="name", + field=models.CharField( + max_length=500, + validators=[ + django.core.validators.RegexValidator( + "^[a-zA-Z0-9-_]+$", + "Only alphanumeric, hyphens, and underscores characters are allowed.", + ) + ], + ), + ), + ] diff --git a/netbox_acls/models/access_lists.py b/netbox_acls/models/access_lists.py index 40a59bb4..33c61123 100644 --- a/netbox_acls/models/access_lists.py +++ b/netbox_acls/models/access_lists.py @@ -21,7 +21,7 @@ alphanumeric_plus = RegexValidator( - r"^[0-9a-zA-Z,-,_]*$", + r"^[a-zA-Z0-9-_]+$", "Only alphanumeric, hyphens, and underscores characters are allowed.", ) From be1a975324261e9b49928fd0aacb10621058245c Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Wed, 1 Feb 2023 23:14:05 +0000 Subject: [PATCH 37/42] fixing missing error_message --- netbox_acls/api/serializers.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/netbox_acls/api/serializers.py b/netbox_acls/api/serializers.py index 9664d2e9..80e95c51 100644 --- a/netbox_acls/api/serializers.py +++ b/netbox_acls/api/serializers.py @@ -122,6 +122,8 @@ def validate(self, data): - Check that the GFK object is valid. - Check if Access List has no existing rules before change the Access List's type. """ + error_message = {} + # Check that the GFK object is valid. assigned_object = validate_gfk(data) From d094bf62d6732e859b18abc88edbee931daca593 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Wed, 1 Feb 2023 23:14:38 +0000 Subject: [PATCH 38/42] isort --- netbox_acls/api/serializers.py | 2 +- netbox_acls/forms/models.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/netbox_acls/api/serializers.py b/netbox_acls/api/serializers.py index 80e95c51..46f24cbd 100644 --- a/netbox_acls/api/serializers.py +++ b/netbox_acls/api/serializers.py @@ -16,10 +16,10 @@ from ..constants import ACL_HOST_ASSIGNMENT_MODELS, ACL_INTERFACE_ASSIGNMENT_MODELS from ..models import ( AccessList, - BaseACLRule, ACLExtendedRule, ACLInterfaceAssignment, ACLStandardRule, + BaseACLRule, ) from .nested_serializers import NestedAccessListSerializer diff --git a/netbox_acls/forms/models.py b/netbox_acls/forms/models.py index 4c1a79ba..8c1c4c0c 100644 --- a/netbox_acls/forms/models.py +++ b/netbox_acls/forms/models.py @@ -18,13 +18,6 @@ ) from ..choices import ACLTypeChoices -from .constants import ( - ERROR_MESSAGE_NO_REMARK, - ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK, - HELP_TEXT_ACL_ACTION, - HELP_TEXT_ACL_RULE_INDEX, - HELP_TEXT_ACL_RULE_LOGIC, -) from ..models import ( AccessList, ACLExtendedRule, @@ -32,6 +25,13 @@ ACLStandardRule, BaseACLRule, ) +from .constants import ( + ERROR_MESSAGE_NO_REMARK, + ERROR_MESSAGE_REMARK_WITHOUT_ACTION_REMARK, + HELP_TEXT_ACL_ACTION, + HELP_TEXT_ACL_RULE_INDEX, + HELP_TEXT_ACL_RULE_LOGIC, +) __all__ = ( "AccessListForm", From 3f4c9d7c627ad5e132de1bbd80f58874dd5b5d55 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Thu, 2 Feb 2023 16:21:13 +0000 Subject: [PATCH 39/42] update tests --- netbox_acls/tests/test_api.py | 42 +++++-- netbox_acls/tests/test_models.py | 187 +++++++++++++++++++++++++++++++ 2 files changed, 217 insertions(+), 12 deletions(-) create mode 100644 netbox_acls/tests/test_models.py diff --git a/netbox_acls/tests/test_api.py b/netbox_acls/tests/test_api.py index 94bc2e33..2dd5745c 100644 --- a/netbox_acls/tests/test_api.py +++ b/netbox_acls/tests/test_api.py @@ -18,17 +18,17 @@ def test_root(self): Test the API root """ url = reverse("plugins-api:netbox_acls-api:api-root") - response = self.client.get(f"{url}?format=api", **self.header) + response = self.client.get("{}?format=api".format(url), **self.header) self.assertEqual(response.status_code, status.HTTP_200_OK) class ACLTestCase( + APIViewTestCases.CreateObjectViewTestCase, + APIViewTestCases.DeleteObjectViewTestCase, APIViewTestCases.GetObjectViewTestCase, APIViewTestCases.ListObjectsViewTestCase, - APIViewTestCases.CreateObjectViewTestCase, APIViewTestCases.UpdateObjectViewTestCase, - APIViewTestCases.DeleteObjectViewTestCase, ): """ Test the AccessList Test @@ -65,49 +65,67 @@ def setUpTestData(cls): access_lists = ( AccessList( - name="testacl1", + name="testacl-standard-deny-1", assigned_object_type=ContentType.objects.get_for_model(Device), assigned_object_id=device.id, type=ACLTypeChoices.TYPE_STANDARD, default_action=ACLActionChoices.ACTION_DENY, + comments="Comment Test 1", ), AccessList( - name="testacl2", + name="testacl-standard-permit-1", assigned_object_type=ContentType.objects.get_for_model(Device), assigned_object_id=device.id, type=ACLTypeChoices.TYPE_STANDARD, - default_action=ACLActionChoices.ACTION_DENY, + default_action=ACLActionChoices.ACTION_PERMIT, + comments="Comment Test 2", ), AccessList( - name="testacl3", + name="testacl-extended-deny-1", assigned_object_type=ContentType.objects.get_for_model(Device), assigned_object_id=device.id, - type=ACLTypeChoices.TYPE_STANDARD, + type=ACLTypeChoices.TYPE_EXTENDED, default_action=ACLActionChoices.ACTION_DENY, ), + AccessList( + name="testacl-extended-permit-1", + assigned_object_type=ContentType.objects.get_for_model(Device), + assigned_object_id=device.id, + type=ACLTypeChoices.TYPE_EXTENDED, + default_action=ACLActionChoices.ACTION_PERMIT, + ), ) AccessList.objects.bulk_create(access_lists) cls.create_data = [ { - "name": "testacl4", + "name": "testacl-standard-deny-2", "assigned_object_type": "dcim.device", "assigned_object_id": device.id, "type": ACLTypeChoices.TYPE_STANDARD, "default_action": ACLActionChoices.ACTION_DENY, + "comments": "Comment Test 3", }, { - "name": "testacl5", + "name": "testacl-standard-permit-2", + "assigned_object_type": "dcim.device", + "assigned_object_id": device.id, + "type": ACLTypeChoices.TYPE_STANDARD, + "default_action": ACLActionChoices.ACTION_DENY, + "comments": "Comment Test 4", + }, + { + "name": "testacl-extended-deny-2", "assigned_object_type": "dcim.device", "assigned_object_id": device.id, "type": ACLTypeChoices.TYPE_EXTENDED, "default_action": ACLActionChoices.ACTION_DENY, }, { - "name": "testacl6", + "name": "testacl-extended-permit-2", "assigned_object_type": "dcim.device", "assigned_object_id": device.id, - "type": ACLTypeChoices.TYPE_STANDARD, + "type": ACLTypeChoices.TYPE_EXTENDED, "default_action": ACLActionChoices.ACTION_DENY, }, ] diff --git a/netbox_acls/tests/test_models.py b/netbox_acls/tests/test_models.py new file mode 100644 index 00000000..a6f6fbc8 --- /dev/null +++ b/netbox_acls/tests/test_models.py @@ -0,0 +1,187 @@ +from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site +from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import ValidationError +from django.test import TestCase +from ipam.models import Prefix +from netaddr import IPNetwork +from virtualization.models import Cluster, ClusterType, VirtualMachine, VMInterface + +from netbox_acls.choices import * +from netbox_acls.models import * + + +class BaseTestCase(TestCase): + """ + Base test case for netbox_acls models. + """ + + @classmethod + def setUpTestData(cls): + """ + Create base data to test using including: + - 1 of each of the following: test site, manufacturer, device type, device role, cluster type, cluster, & virtual machine + - 2 devices, prefixes, 2 interfaces, and 2 vminterfaces + """ + + site = Site.objects.create(name="Site 1", slug="site-1") + manufacturer = Manufacturer.objects.create( + name="Manufacturer 1", + slug="manufacturer-1", + ) + devicetype = DeviceType.objects.create( + manufacturer=manufacturer, + model="Device Type 1", + ) + devicerole = DeviceRole.objects.create( + name="Device Role 1", + slug="device-role-1", + ) + device = Device.objects.create( + name="Device 1", + site=site, + device_type=devicetype, + device_role=devicerole, + ) + cluster_member = Device.objects.create( + name="Cluster Device", + site=site, + device_type=devicetype, + device_role=devicerole, + ) + clustertype = ClusterType.objects.create(name="Cluster Type 1") + cluster = Cluster.objects.create( + name="Cluster 1", + type=clustertype, + ) + virtual_machine = VirtualMachine.objects.create(name="VirtualMachine 1") + + interfaces = Interface.objects.bulk_create( + ( + Interface(name="Interface 1", device=device, type="1000baset"), + Interface(name="Interface 2", device=device, type="1000baset"), + ) + ) + vminterfaces = VMInterface.objects.bulk_create( + ( + VMInterface(name="Interface 1", virtual_machine=virtual_machine), + VMInterface(name="Interface 2", virtual_machine=virtual_machine), + ) + ) + prefixes = Prefix.objects.bulk_create( + ( + Prefix(prefix=IPNetwork("10.0.0.0/24")), + Prefix(prefix=IPNetwork("192.168.1.0/24")), + ) + ) + + +class TestAccessList(BaseTestCase): + """ + Test AccessList model. + """ + + def test_alphanumeric_plus_success(self): + """ + Test that AccessList names with alphanumeric characters, '_', or '-' pass validation. + """ + acl_good_name = AccessList( + name="Testacl-good_name-1", + assigned_object_type=ContentType.objects.get_for_model(Device), + assigned_object_id=1, + type=ACLTypeChoices.TYPE_EXTENDED, + default_action=ACLActionChoices.ACTION_PERMIT, + ) + self.assertRaises(ValidationError, acl_good_name.clean) + + def test_duplicate_name_success(self): + """ + Test that AccessList names can be non-unique if associated to different devices. + """ + params = { + "name": "GOOD-DUPLICATE-ACL", + "type": ACLTypeChoices.TYPE_STANDARD, + "default_action": ACLActionChoices.ACTION_PERMIT, + } + AccessList.objects.create( + **params, + assigned_object_type=ContentType.objects.get_for_model(Device), + assigned_object_id=1, + ) + new_acl = AccessList( + **params, + assigned_object_type=ContentType.objects.get_for_model(VirtualMachine), + assigned_object_id=1, + ) + self.assertIsNone(new_acl.clean()) + # TODO: test_duplicate_name_fail - VM & Cluster + + def test_alphanumeric_plus_fail(self): + """ + Test that AccessList names with non-alphanumeric (exluding '_' and '-') characters fail validation. + """ + non_alphanumeric_plus_chars = " !@#$%^&*()[]{};:,./<>?\|~=+" + + for i, char in enumerate(non_alphanumeric_plus_chars, start=1): + bade_acl_name = AccessList( + name=f"Testacl-bad_name_{i}_{char}", + assigned_object_type=ContentType.objects.get_for_model(Device), + assigned_object_id=1, + type=ACLTypeChoices.TYPE_EXTENDED, + default_action=ACLActionChoices.ACTION_PERMIT, + comments=f'ACL with "{char}" in name', + ) + self.assertIsNone(bade_acl_name.clean) + + def test_duplicate_name_fail(self): + """ + Test that AccessList names must be unique per device. + """ + params = { + "name": "FAIL-DUPLICATE-ACL", + "assigned_object_type": "dcim.device", + "assigned_object_id": 1, + "type": ACLTypeChoices.TYPE_STANDARD, + "default_action": ACLActionChoices.ACTION_PERMIT, + } + AccessList.objects.create(**params) + duplicate_acl = AccessList(**params) + self.assertRaises(ValidationError, duplicate_acl.clean()) + # TODO: test_duplicate_name_fail - VM & Cluster + + # TODO: Test choices for AccessList Model + + +class TestACLInterfaceAssignment(BaseTestCase): + """ + Test ACLInterfaceAssignment model. + """ + + def test_acl_assignment_success(self): + """ + Test that ACLInterfaceAssignment passes validation if the ACL is assigned to the host and not already assigned to the interface and direction. + """ + pass + # TODO: test_acl_assignment_success - VM & Device + + def test_acl_assignment_fail(self): + """ + Test that ACLInterfaceAssignment fails validation if the ACL is not assigned to the parent host. + """ + pass + # TODO: test_acl_assignment_fail - VM & Device + + def test_duplicate_assignment_fail(self): + """ + Test that ACLInterfaceAssignment fails validation if the ACL already is assigned to the same interface and direction. + """ + pass + # TODO: test_duplicate_assignment_fail - VM & Device + + def test_acl_already_assinged_fail(self): + """ + Test that ACLInterfaceAssignment fails validation if the interface already has an ACL assigned in the same direction. + """ + pass + ## TODO: test_acl_already_assinged_fail - VM & Device + + # TODO: Test choices for ACLInterfaceAssignment Model From 81db4daf3cf2ba871b64030ea4c050ee7399130c Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Thu, 2 Feb 2023 17:22:33 +0000 Subject: [PATCH 40/42] add vs code extensions --- .devcontainer/devcontainer.json | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 37404e7c..2d3b513f 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -78,7 +78,10 @@ "extensions": [ "DavidAnson.vscode-markdownlint", "GitHub.codespaces", + "GitHub.copilot", + "GitHub.copilot-labs", "GitHub.vscode-pull-request-github", + "Gruntfuggly.todo-tree", "Tyriar.sort-lines", "aaron-bond.better-comments", "batisteo.vscode-django", @@ -91,9 +94,7 @@ "mutantdino.resourcemonitor", "paulomenezes.duplicated-code", "searKing.preview-vscode", - "sourcery.sourcery", - "GitHub.copilot", - "GitHub.copilot-labs" + "sourcery.sourcery" ] } }, From c46f7688c70c74edfc1bf233770d1a075d7325c5 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Thu, 2 Feb 2023 17:22:39 +0000 Subject: [PATCH 41/42] fix tests --- netbox_acls/tests/test_models.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/netbox_acls/tests/test_models.py b/netbox_acls/tests/test_models.py index a6f6fbc8..24d7b619 100644 --- a/netbox_acls/tests/test_models.py +++ b/netbox_acls/tests/test_models.py @@ -91,7 +91,7 @@ def test_alphanumeric_plus_success(self): type=ACLTypeChoices.TYPE_EXTENDED, default_action=ACLActionChoices.ACTION_PERMIT, ) - self.assertRaises(ValidationError, acl_good_name.clean) + acl_good_name.full_clean() def test_duplicate_name_success(self): """ @@ -112,7 +112,7 @@ def test_duplicate_name_success(self): assigned_object_type=ContentType.objects.get_for_model(VirtualMachine), assigned_object_id=1, ) - self.assertIsNone(new_acl.clean()) + new_acl.full_clean() # TODO: test_duplicate_name_fail - VM & Cluster def test_alphanumeric_plus_fail(self): @@ -122,7 +122,7 @@ def test_alphanumeric_plus_fail(self): non_alphanumeric_plus_chars = " !@#$%^&*()[]{};:,./<>?\|~=+" for i, char in enumerate(non_alphanumeric_plus_chars, start=1): - bade_acl_name = AccessList( + bad_acl_name = AccessList( name=f"Testacl-bad_name_{i}_{char}", assigned_object_type=ContentType.objects.get_for_model(Device), assigned_object_id=1, @@ -130,7 +130,8 @@ def test_alphanumeric_plus_fail(self): default_action=ACLActionChoices.ACTION_PERMIT, comments=f'ACL with "{char}" in name', ) - self.assertIsNone(bade_acl_name.clean) + with self.assertRaises(ValidationError): + bad_acl_name.full_clean() def test_duplicate_name_fail(self): """ @@ -138,14 +139,16 @@ def test_duplicate_name_fail(self): """ params = { "name": "FAIL-DUPLICATE-ACL", - "assigned_object_type": "dcim.device", + "assigned_object_type": ContentType.objects.get_for_model(Device), "assigned_object_id": 1, "type": ACLTypeChoices.TYPE_STANDARD, "default_action": ACLActionChoices.ACTION_PERMIT, } - AccessList.objects.create(**params) - duplicate_acl = AccessList(**params) - self.assertRaises(ValidationError, duplicate_acl.clean()) + acl_1 =AccessList.objects.create(**params) + acl_1.save() + acl_2 = AccessList(**params) + with self.assertRaises(ValidationError): + acl_2.full_clean() # TODO: test_duplicate_name_fail - VM & Cluster # TODO: Test choices for AccessList Model From 5aec75699de201da327e14971e212defa861cd77 Mon Sep 17 00:00:00 2001 From: ryanmerolle Date: Thu, 2 Feb 2023 20:42:14 +0000 Subject: [PATCH 42/42] update models --- netbox_acls/tests/test_models.py | 90 +++++++++++++++++++++----------- 1 file changed, 60 insertions(+), 30 deletions(-) diff --git a/netbox_acls/tests/test_models.py b/netbox_acls/tests/test_models.py index 24d7b619..31fe5af2 100644 --- a/netbox_acls/tests/test_models.py +++ b/netbox_acls/tests/test_models.py @@ -1,4 +1,12 @@ -from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site +from dcim.models import ( + Device, + DeviceRole, + DeviceType, + Interface, + Manufacturer, + Site, + VirtualChassis, +) from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ValidationError from django.test import TestCase @@ -19,7 +27,7 @@ class BaseTestCase(TestCase): def setUpTestData(cls): """ Create base data to test using including: - - 1 of each of the following: test site, manufacturer, device type, device role, cluster type, cluster, & virtual machine + - 1 of each of the following: test site, manufacturer, device type, device role, cluster type, cluster, virtual_chassis, & virtual machine - 2 devices, prefixes, 2 interfaces, and 2 vminterfaces """ @@ -42,6 +50,15 @@ def setUpTestData(cls): device_type=devicetype, device_role=devicerole, ) + virtual_chassis = VirtualChassis.objects.create(name="Virtual Chassis 1") + virtual_chassis_member = Device.objects.create( + name="VC Device", + site=site, + device_type=devicetype, + device_role=devicerole, + virtual_chassis=virtual_chassis, + vc_position=1, + ) cluster_member = Device.objects.create( name="Cluster Device", site=site, @@ -55,25 +72,6 @@ def setUpTestData(cls): ) virtual_machine = VirtualMachine.objects.create(name="VirtualMachine 1") - interfaces = Interface.objects.bulk_create( - ( - Interface(name="Interface 1", device=device, type="1000baset"), - Interface(name="Interface 2", device=device, type="1000baset"), - ) - ) - vminterfaces = VMInterface.objects.bulk_create( - ( - VMInterface(name="Interface 1", virtual_machine=virtual_machine), - VMInterface(name="Interface 2", virtual_machine=virtual_machine), - ) - ) - prefixes = Prefix.objects.bulk_create( - ( - Prefix(prefix=IPNetwork("10.0.0.0/24")), - Prefix(prefix=IPNetwork("192.168.1.0/24")), - ) - ) - class TestAccessList(BaseTestCase): """ @@ -97,6 +95,7 @@ def test_duplicate_name_success(self): """ Test that AccessList names can be non-unique if associated to different devices. """ + params = { "name": "GOOD-DUPLICATE-ACL", "type": ACLTypeChoices.TYPE_STANDARD, @@ -107,13 +106,18 @@ def test_duplicate_name_success(self): assigned_object_type=ContentType.objects.get_for_model(Device), assigned_object_id=1, ) - new_acl = AccessList( + vm_acl = AccessList( **params, assigned_object_type=ContentType.objects.get_for_model(VirtualMachine), assigned_object_id=1, ) - new_acl.full_clean() - # TODO: test_duplicate_name_fail - VM & Cluster + vm_acl.full_clean() + vc_acl = AccessList( + **params, + assigned_object_type=ContentType.objects.get_for_model(VirtualChassis), + assigned_object_id=1, + ) + vc_acl.full_clean() def test_alphanumeric_plus_fail(self): """ @@ -144,12 +148,12 @@ def test_duplicate_name_fail(self): "type": ACLTypeChoices.TYPE_STANDARD, "default_action": ACLActionChoices.ACTION_PERMIT, } - acl_1 =AccessList.objects.create(**params) + acl_1 = AccessList.objects.create(**params) acl_1.save() acl_2 = AccessList(**params) with self.assertRaises(ValidationError): acl_2.full_clean() - # TODO: test_duplicate_name_fail - VM & Cluster + # TODO: test_duplicate_name_fail - VM & VC # TODO: Test choices for AccessList Model @@ -159,19 +163,45 @@ class TestACLInterfaceAssignment(BaseTestCase): Test ACLInterfaceAssignment model. """ - def test_acl_assignment_success(self): + @classmethod + def setUpTestData(cls): + """ + Extend BaseTestCase's setUpTestData() to create additional data for testing. + """ + super().setUpTestData() + + interfaces = Interface.objects.bulk_create( + ( + Interface(name="Interface 1", device=device, type="1000baset"), + Interface(name="Interface 2", device=device, type="1000baset"), + ) + ) + vminterfaces = VMInterface.objects.bulk_create( + ( + VMInterface(name="Interface 1", virtual_machine=virtual_machine), + VMInterface(name="Interface 2", virtual_machine=virtual_machine), + ) + ) + # prefixes = Prefix.objects.bulk_create( + # ( + # Prefix(prefix=IPNetwork("10.0.0.0/24")), + # Prefix(prefix=IPNetwork("192.168.1.0/24")), + # ) + # ) + + def test_acl_interface_assignment_success(self): """ Test that ACLInterfaceAssignment passes validation if the ACL is assigned to the host and not already assigned to the interface and direction. """ pass - # TODO: test_acl_assignment_success - VM & Device + # TODO: test_acl_interface_assignment_success - VM & Device - def test_acl_assignment_fail(self): + def test_acl_interface_assignment_fail(self): """ Test that ACLInterfaceAssignment fails validation if the ACL is not assigned to the parent host. """ pass - # TODO: test_acl_assignment_fail - VM & Device + # TODO: test_acl_interface_assignment_fail - VM & Device def test_duplicate_assignment_fail(self): """