IP address model does not check if assigned_object exists

[amhn]

NetBox version

v3.3.1

Python version

3.10

Steps to Reproduce

1. Create IP address "127.0.0.1/8"
2. Create a PATCH request to /api/ipam/ip-addresses/:

   curl -X PATCH \
      -H  "accept: application/json" -H  "Authorization: Token $NETBOX_TOKEN" \
     -H "Content-Type: application/json" \
     --data '{"assigned_object_type": "virtualization.vminterface", "assigned_object_id":4711}' \
    "http://localhost:8000/api/ipam/ip-addresses/1/"

Expected Behavior

An error is returned that a vminterface with ID 4711 does not exist.

Observed Behavior

assigned_object_id and assigned_object_type are saved in the model and returned on subsequent GET requests.

{
  "id": 1,
  "url": "http://localhost:8000/api/ipam/ip-addresses/1/",
[...]
  "address": "127.0.0.1/8",
[...]
  "assigned_object_type": "virtualization.vminterface",
  "assigned_object_id": 4711,
  "assigned_object": null,
[...]
}

[jeremystretch]

I'm not sure we want to classify this as a bug, given that Django doesn't validate GenericForeignKey assignment out of the box. Consider what happens when assigning an invalid object ID directly on the instance:

>>> from netaddr import IPNetwork
>>> ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999)
>>> ip.full_clean()
>>> ip.save()
>>> ip = IPAddress.objects.get(pk=ip.pk)
>>> repr(ip.assigned_object)
'None'

Validation passes, and the assigned_object attribute returns None as expected. This would seem to be the case with all generic foreign keys and not just the assigned_object field on IPAddress.

This probably needs some deeper discussion to figure out how (and whether) to handle this.

[amhn]

With the following in IPAddress.clean() in file netbox/netbox/models/ip.py

        # Check if assigned object exists
        if (self.assigned_object_id != None or self.assigned_object_type != None):
            print("Validation")
            for cls, objtype in ((Interface, 'dcim | interface'), (VMInterface, 'virtualization | interface'),):
                print(objtype, str(self.assigned_object_type))
                print(type(objtype), type(self.assigned_object_type))
                if objtype == str(self.assigned_object_type):
                    print(objtype)
                    try:
                        assigned_object = cls.objects.get(
                            pk=self.assigned_object_id)
                    except VMInterface.DoesNotExist:
                        raise ValidationError({
                            'assigned_object': f"Assigned Object does not exist."
                        })

This becomes:

>>> ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999)
>>> ip.full_clean()
Validation
dcim | interface dcim | interface
<class 'str'> <class 'django.contrib.contenttypes.models.ContentType'>
dcim | interface
Traceback (most recent call last):
  File "/usr/lib/python3.10/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
  File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/base.py", line 1390, in full_clean
    self.clean()
  File "/home/andy/python/netbox/netbox/ipam/models/ip.py", line 954, in clean
    assigned_object = cls.objects.get(
  File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/query.py", line 496, in get
    raise self.model.DoesNotExist(
dcim.models.device_components.Interface.DoesNotExist: Interface matching query does not exist.
>>> 

Not sure if that is the way you want to go. But it is a possibilty.

If you want to go forward with this, I can clean it up, add FHRPGroup and create a PR

[jeremystretch]

My two primary concerns with heading down this path are:

1. Avoiding introducing significant overhead in the form of additional database queries that check for an object's existence.
2. Implementing the validation in such a way that it is naturally and consistently replicated for all GFK fields.

[amhn]

These are valid concerns.

Regarding 1.: I looked further into it. And it seems on the Django shell the following is enough, because the assigned_object is pre_populated:

        # Check if assigned object exists
        if (self.assigned_object_id is not None or self.assigned_object_type is not None) and self.assigned_object is None:
            raise ValidationError({
                'assigned_object': f"Assigned Object does not exist."
            })

However this does not work with the Rest-API. For this I had to extend the validate method in ValidatedModelSerializer to populate the assigned_object field. Changes are in https://github.com/amhn/netbox/commit/1c74f3373faf11ae9e5ae32d8a2296b54e42e3fc

diff --git a/netbox/netbox/api/serializers/base.py b/netbox/netbox/api/serializers/base.py
index f1aea0e2b..d0aeaace9 100644
--- a/netbox/netbox/api/serializers/base.py
+++ b/netbox/netbox/api/serializers/base.py
@@ -1,3 +1,6 @@
+from contextlib import suppress
+from django.contrib.contenttypes.fields import GenericForeignKey
+from django.core.exceptions import ObjectDoesNotExist
 from django.db.models import ManyToManyField
 from rest_framework import serializers

@@ -38,6 +41,18 @@ class ValidatedModelSerializer(BaseModelSerializer):
             instance = self.instance
             for k, v in attrs.items():
                 setattr(instance, k, v)
+
+        # Update GenericForeignKey fields if either foreign_key or content_type has changed
+        for field in self.Meta.model._meta.get_fields():
+            if isinstance(field, GenericForeignKey) and getattr(instance, field.name, None) is None:
+                if field.ct_field in attrs.keys() or field.fk_field in attrs.keys():
+                    ct = attrs.get(field.ct_field, getattr(instance, field.ct_field))
+                    fk = attrs.get(field.fk_field, getattr(instance, field.fk_field))
+                    if ct is not None and fk is not None:
+                        with suppress(ObjectDoesNotExist):
+                            new_field = ct.model_class().objects.get(pk=fk)
+                            setattr(instance, field.name, new_field)
+
         instance.full_clean()

         return data

An additional advantage of this change: In the answer to a PATCH request, assigned_object is now populated, if an object is assigned to an uanssigned IPAdress. In the develop branch null is returned for assigned_object in this case.

This does not fully address your second concern, but the code that has to be added per field is a lot less than my first idea,

Related but not part of this issue: Looking at the docs I found the following notice https://docs.djangoproject.com/en/4.0/ref/contrib/contenttypes/#django.contrib.contenttypes.fields.GenericForeignKey:

Unlike for the [ForeignKey], a database index is not automatically created on the [GenericForeignKey],
so it’s recommended that you use [Meta.indexes]  to add your own multiple column index. This behavior [may change]
in the future.

Is it intentional that the mentioned index is not created for (assigned_object_id, assigned_object_type)? Shoud I create an issue to check all GFK fields for indexes?

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Do not attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our contributing guide.

[abhi1693]

How about this as a solution? Since, everything inherits from NetBoxModel a simple extension to clean method will solve this issue on the model level. This definitely works for the issue that Jerry stated at https://github.com/netbox-community/netbox/issues/10221#issuecomment-1233285651

diff --git a/netbox/netbox/models/__init__.py b/netbox/netbox/models/__init__.py
index a4c8e0ec2..be21b978a 100644
--- a/netbox/netbox/models/__init__.py
+++ b/netbox/netbox/models/__init__.py
@@ -1,4 +1,6 @@
 from django.conf import settings
+from django.contrib.contenttypes.fields import GenericForeignKey
+from django.contrib.contenttypes.models import ContentType
 from django.core.validators import ValidationError
 from django.db import models
 from mptt.models import MPTTModel, TreeForeignKey
@@ -58,6 +60,23 @@ class NetBoxModel(CloningMixin, NetBoxFeatureSet, models.Model):
     class Meta:
         abstract = True

+    def clean(self):
+        """
+        Validate the model for GenericForeignKey fields to ensure that the content type and object ID exist.
+        """
+        super().clean()
+
+        for field in self._meta.get_fields():
+            if isinstance(field, GenericForeignKey):
+                if getattr(self, field.ct_field) and getattr(self, field.fk_field):
+                    klass = getattr(self, field.ct_field).model_class()
+                    try:
+                        klass.objects.get(pk=getattr(self, field.fk_field))
+                    except klass.DoesNotExist:
+                        raise ValidationError({
+                            field.fk_field: f"Invalid {getattr(self, field.fk_field)}: object does not exist on {getattr(self, field.ct_field)}."
+                        })
+

 class PrimaryModel(NetBoxModel):
     """

Result

>>> from dcim.models import Interface
... from django.contrib.contenttypes.models import ContentType
... from ipam.models import IPAddress
... from netaddr import IPNetwork
... ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999)
... ip.full_clean()
... 
... 
Traceback (most recent call last):
  File "/snap/pycharm-professional/316/plugins/python/helpers/pydev/pydevconsole.py", line 364, in runcode
    coro = func()
  File "<input>", line 6, in <module>
  File "/home/asaharan/PycharmProjects/netbox/venv/lib/python3.10/site-packages/django/db/models/base.py", line 1477, in full_clean
    raise ValidationError(errors)
django.core.exceptions.ValidationError: {'assigned_object_id': ['Invalid 999999: object does not exist on dcim | interface.']}

Using API, result:

curl -X PATCH \                                             
       -H  "accept: application/json" -H  "Authorization: Token $NETBOX_TOKEN" \ 
      -H "Content-Type: application/json" \
      --data '{"assigned_object_type": "virtualization.vminterface", "assigned_object_id":4711}' \
     "http://localhost:8000/api/ipam/ip-addresses/1/"
{"assigned_object_id":["Invalid 4711: object does not exist on virtualization | interface."]}

@jeremystretch If you think this is doable, I can submit a PR for this

[candlerb]

            if getattr(self, field.ct_field) and getattr(self, field.fk_field):

That doesn't address the inconsistency which can occur if ct_field is not None but fk_field is None, or vice versa - see #11866.

I think an additional check for that case would be straightforward, but you'd have to beware leaving the user in limbo if the database is in this inconsistent state, because there is no way to fix it via the UI.

Maybe if either ct_field or fk_field is None, the other should be set to None automatically?