[change] Remove WhoIs field if not configured

DragnEmperor · DragnEmperor · commit 592eb3a4cb6f · 2025-06-23T21:11:57.000+05:30
If who_is is not configured then Orgs should
not be allowed to enable/disable the feature
from admin.
Added test cases for the same and also for
the possible scenarios for WhoIs as a feature.

Signed-off-by: DragnEmperor &lt;dragnemperor@gmail.com&gt;
diff --git a/docs/user/settings.rst b/docs/user/settings.rst
@@ -758,7 +758,7 @@ Allows to enable WhoIs lookup feature.
 
 This feature is used to fetch details of a device based on its last
 reported public IP address. The fetched details include ASN, CIDR,
-address, timezone and organization name.
+address, timezone and name of organization associated with ASN.
 
 This feature is disabled by default and requires setting the
 :ref:`OPENWISP_CONTROLLER_GEOIP_ACCOUNT_ID
diff --git a/docs/user/who-is.rst b/docs/user/who-is.rst
@@ -19,7 +19,8 @@ WHOIS Lookup
     Both of the settings :ref:`OPENWISP_CONTROLLER_GEOIP_ACCOUNT_ID` and
     :ref:`OPENWISP_CONTROLLER_GEOIP_LICENSE_KEY` are required to be set,
     to ensure the WhoIs Lookup feature can be enabled/disabled for each
-    organization. Else, the feature will be disabled globally.
+    organization. Else, the feature will be disabled globally and the
+    field will not be available to configure on admin interface.
 
 WhoIs feature includes fetching details of the last public ip address
 reported by a device to ensure better device management.
diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py
@@ -1362,6 +1362,13 @@ class ConfigSettingsInline(admin.StackedInline):
         model = OrganizationConfigSettings
         form = ConfigSettingsForm
 
+        def get_fields(self, request, obj=None):
+            fields = super().get_fields(request, obj)
+            # hide the 'who_is_enabled' field if WhoIs is not Configured
+            if not app_settings.WHO_IS_CONFIGURED:
+                fields = [f for f in fields if f != "who_is_enabled"]
+            return fields
+
     OrganizationAdmin.save_on_top = True
     OrganizationAdmin.inlines.insert(0, ConfigSettingsInline)
     limits_inline_position = 1
diff --git a/openwisp_controller/config/apps.py b/openwisp_controller/config/apps.py
@@ -53,8 +53,7 @@ def ready(self, *args, **kwargs):
         self.register_dashboard_charts()
         self.register_menu_groups()
         self.notification_cache_update()
-        if app_settings.WHO_IS_CONFIGURED:
-            connect_who_is_handlers()
+        connect_who_is_handlers()
 
     def __setmodels__(self):
         self.device_model = load_model("config", "Device")
diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py
@@ -30,13 +30,7 @@ class AbstractDevice(OrgMixin, BaseModel):
     physical properties of a network device
     """
 
-    _changed_checked_fields = [
-        "name",
-        "group_id",
-        "management_ip",
-        "organization_id",
-        "last_ip",
-    ]
+    _changed_checked_fields = ["name", "group_id", "management_ip", "organization_id"]
 
     name = models.CharField(
         max_length=64,
@@ -126,6 +120,11 @@ class Meta:
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
+        # if WhoIs is configured, we need to look out for changes
+        # in last_ip field as well
+        if app_settings.WHO_IS_CONFIGURED:
+            self._changed_checked_fields.append("last_ip")
+
         self._set_initial_values_for_changed_checked_fields()
 
     def _set_initial_values_for_changed_checked_fields(self):
@@ -287,8 +286,7 @@ def save(self, *args, **kwargs):
                 self.key = self.generate_key(shared_secret)
         state_adding = self._state.adding
         super().save(*args, **kwargs)
-        # We need to check for last_ip when device is created/updated to perform
-        # WhoIs lookup. This is run only when WhoIs is configured.
+        # WhoIs Lookup runs only when WhoIs is configured.
         if app_settings.WHO_IS_CONFIGURED:
             self._check_last_ip()
         if state_adding and self.group and self.group.templates.exists():
diff --git a/openwisp_controller/config/base/multitenancy.py b/openwisp_controller/config/base/multitenancy.py
@@ -62,7 +62,7 @@ def get_context(self):
         return deepcopy(self.context)
 
     def clean(self):
-        if self.who_is_enabled and not app_settings.WHO_IS_CONFIGURED:
+        if not app_settings.WHO_IS_CONFIGURED and self.who_is_enabled:
             raise ValidationError(
                 _(
                     "GEOIP_ACCOUNT_ID and GEOIP_LICENSE_KEY must be set "
diff --git a/openwisp_controller/config/who_is/handlers.py b/openwisp_controller/config/who_is/handlers.py
@@ -1,8 +1,15 @@
 from django.db.models.signals import post_delete, post_save
 from swapper import load_model
 
+from .. import settings as app_settings
+
 
 def connect_who_is_handlers():
+    # if WHO_IS_CONFIGURED is False, we do not connect the handlers
+    # and return early
+    if not app_settings.WHO_IS_CONFIGURED:
+        return
+
     Device = load_model("config", "Device")
     WhoIsInfo = load_model("config", "WhoIsInfo")
     OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings")
diff --git a/openwisp_controller/config/who_is/test_who_is.py b/openwisp_controller/config/who_is/test_who_is.py
@@ -1,11 +1,14 @@
+import importlib
 from unittest import mock
 
-from django.core.exceptions import ValidationError
+from django.core.exceptions import ImproperlyConfigured, ValidationError
+from django.db.models.signals import post_delete, post_save
 from django.test import TestCase, TransactionTestCase, override_settings
 from django.urls import reverse
 from geoip2 import errors
 from swapper import load_model
 
+from ...tests.utils import TestAdminMixin
 from .. import settings as app_settings
 from .handlers import connect_who_is_handlers
 from .utils import CreateWhoIsMixin
@@ -18,26 +21,124 @@
 notification_qs = Notification.objects.all()
 
 
-class TestWhoIsInfoModel(CreateWhoIsMixin, TestCase):
-    def setUp(self):
-        self.admin = self._get_admin()
-        # connect the signals related to who_is
-        connect_who_is_handlers()
+class TestWhoIsFeature(CreateWhoIsMixin, TestAdminMixin, TestCase):
+    @override_settings(
+        OPENWISP_CONTROLLER_WHO_IS_ENABLED=False,
+        OPENWISP_CONTROLLER_GEOIP_ACCOUNT_ID=None,
+        OPENWISP_CONTROLLER_GEOIP_LICENSE_KEY=None,
+    )
+    def test_who_is_configuration_setting(self):
+        # disconnect previously connected signals on app start, if any
+        self._disconnect_signals()
+        # reload app_settings to apply the overridden settings
+        importlib.reload(app_settings)
+
+        with self.subTest("Test Signals not connected when WHO_IS_CONFIGURED is False"):
+            # should not connect any handlers since WHO_IS_CONFIGURED is False
+            connect_who_is_handlers()
+
+            assert not any(
+                "device.delete_who_is_info" in str(r[0]) for r in post_delete.receivers
+            )
+            assert not any(
+                "invalidate_org_config_cache_on_org_config_save" in str(r[0])
+                for r in post_save.receivers
+            )
+            assert not any(
+                "invalidate_org_config_cache_on_org_config_delete" in str(r[0])
+                for r in post_delete.receivers
+            )
+
+        with self.subTest(
+            "Test WhoIs field hidden on admin when WHO_IS_CONFIGURED is False"
+        ):
+            self._login()
+            url = reverse(
+                "admin:openwisp_users_organization_change",
+                args=[self._get_org().pk],
+            )
+            response = self.client.get(url)
+            self.assertNotContains(response, 'name="config_settings-0-who_is_enabled"')
 
-    def test_who_is_feature(self):
+        with self.subTest(
+            "Test ImproperlyConfigured raised when WHO_IS_CONFIGURED is False"
+        ):
+            with override_settings(OPENWISP_CONTROLLER_WHO_IS_ENABLED=True):
+                with self.assertRaises(ImproperlyConfigured):
+                    # reload app_settings to apply the overridden settings
+                    importlib.reload(app_settings)
+
+        with override_settings(
+            OPENWISP_CONTROLLER_GEOIP_ACCOUNT_ID="test_account_id",
+            OPENWISP_CONTROLLER_GEOIP_LICENSE_KEY="test_license_key",
+        ):
+            importlib.reload(app_settings)
+            with self.subTest(
+                "Test WHO_IS_CONFIGURED is True when both settings are set"
+            ):
+                self.assertTrue(app_settings.WHO_IS_CONFIGURED)
+
+            with self.subTest(
+                "Test WhoIs field visible on admin when WHO_IS_CONFIGURED is True"
+            ):
+                self._login()
+                url = reverse(
+                    "admin:openwisp_users_organization_change",
+                    args=[self._get_org().pk],
+                )
+                response = self.client.get(url)
+                self.assertContains(response, 'name="config_settings-0-who_is_enabled"')
+
+    def test_who_is_enabled(self):
         org = self._get_org()
         org_settings_obj = OrganizationConfigSettings(
             organization=org, who_is_enabled=True
         )
-        with self.assertRaises(ValidationError):
+
+        with self.subTest("Test WhoIs not configured does not allow enabling who_is"):
+            with mock.patch.object(
+                app_settings, "WHO_IS_CONFIGURED", False
+            ), self.assertRaises(ValidationError):
+                org_settings_obj.full_clean()
+
+        # create org settings object with WHO_IS_CONFIGURED set to True
+        with mock.patch.object(app_settings, "WHO_IS_CONFIGURED", True):
             org_settings_obj.full_clean()
             org_settings_obj.save()
 
-    def test_validate_who_is_fields(self):
+        with self.subTest("Test setting who_is enabled to True"):
+            org.config_settings.who_is_enabled = True
+            org.config_settings.save(update_fields=["who_is_enabled"])
+            org.config_settings.refresh_from_db(fields=["who_is_enabled"])
+            self.assertEqual(getattr(org.config_settings, "who_is_enabled"), True)
+
+        with self.subTest("Test setting who_is enabled to False"):
+            org.config_settings.who_is_enabled = False
+            org.config_settings.save(update_fields=["who_is_enabled"])
+            org.config_settings.refresh_from_db(fields=["who_is_enabled"])
+            self.assertEqual(getattr(org.config_settings, "who_is_enabled"), False)
+
+        with self.subTest(
+            "Test setting who_is enabled to None fallbacks to global setting"
+        ):
+            # reload app_settings to ensure latest settings are applied
+            importlib.reload(app_settings)
+            org.config_settings.who_is_enabled = None
+            org.config_settings.save(update_fields=["who_is_enabled"])
+            org.config_settings.refresh_from_db(fields=["who_is_enabled"])
+            self.assertEqual(
+                getattr(org.config_settings, "who_is_enabled"),
+                app_settings.WHO_IS_ENABLED,
+            )
+
+
+class TestWhoIsInfoModel(CreateWhoIsMixin, TestCase):
+    def test_who_is_model_fields_validation(self):
         """
         Test db_constraints and validators for WhoIsInfo model fields.
         """
         org = self._get_org()
+        # using `create` to bypass `clean` method validation
         OrganizationConfigSettings.objects.create(organization=org, who_is_enabled=True)
 
         with self.assertRaises(ValidationError):
@@ -55,27 +156,6 @@ def test_validate_who_is_fields(self):
         with self.assertRaises(ValidationError):
             self._create_who_is_info(asn="InvalidASN")
 
-    def test_who_is_enabled(self):
-        org = self._get_org()
-        OrganizationConfigSettings.objects.create(organization=org)
-
-        with self.subTest("Test who_is enabled set to True"):
-            org.config_settings.who_is_enabled = True
-            self.assertEqual(getattr(org.config_settings, "who_is_enabled"), True)
-
-        with self.subTest("Test who_is enabled set to False"):
-            org.config_settings.who_is_enabled = False
-            self.assertEqual(getattr(org.config_settings, "who_is_enabled"), False)
-
-        with self.subTest("Test who_is enabled set to None"):
-            org.config_settings.who_is_enabled = None
-            org.config_settings.save(update_fields=["who_is_enabled"])
-            org.config_settings.refresh_from_db(fields=["who_is_enabled"])
-            self.assertEqual(
-                getattr(org.config_settings, "who_is_enabled"),
-                app_settings.WHO_IS_ENABLED,
-            )
-
 
 class TestWhoIsTransaction(CreateWhoIsMixin, TransactionTestCase):
     _WHO_IS_GEOIP_CLIENT = (
@@ -89,8 +169,6 @@ class TestWhoIsTransaction(CreateWhoIsMixin, TransactionTestCase):
 
     def setUp(self):
         self.admin = self._get_admin()
-        # connect the signals related to who_is
-        connect_who_is_handlers()
 
     @mock.patch.object(app_settings, "WHO_IS_CONFIGURED", True)
     @mock.patch(
@@ -99,6 +177,7 @@ def setUp(self):
     def test_task_called(self, mocked_task):
         org = self._get_org()
         OrganizationConfigSettings.objects.create(organization=org, who_is_enabled=True)
+        connect_who_is_handlers()
 
         with self.subTest("task called when last_ip is public"):
             device = self._create_device(last_ip="172.217.22.14")
@@ -117,8 +196,9 @@ def test_task_called(self, mocked_task):
             mocked_task.assert_not_called()
         mocked_task.reset_mock()
 
-        with self.subTest("task not called when last_ip is not changed"):
-            device.last_ip = "10.0.0.1"
+        with self.subTest("task not called when last_ip has related WhoIsInfo"):
+            device.last_ip = "172.217.22.10"
+            self._create_who_is_info(ip_address=device.last_ip)
             device.save()
             mocked_task.assert_not_called()
         mocked_task.reset_mock()
@@ -127,15 +207,15 @@ def test_task_called(self, mocked_task):
             Device.objects.all().delete()  # Clear existing devices
             org.config_settings.who_is_enabled = False
             # Invalidates old org config settings cache
-            org.config_settings.save()
+            org.config_settings.save(update_fields=["who_is_enabled"])
             device = self._create_device(last_ip="172.217.22.14")
             mocked_task.assert_not_called()
         mocked_task.reset_mock()
 
         with self.subTest("task called via DeviceChecksumView when who_is is enabled"):
             org.config_settings.who_is_enabled = True
             # Invalidates old org config settings cache
-            org.config_settings.save()
+            org.config_settings.save(update_fields=["who_is_enabled"])
             # config is required for checksum view to work
             self._create_config(device=device)
             # setting remote address field to a public IP
diff --git a/openwisp_controller/config/who_is/utils.py b/openwisp_controller/config/who_is/utils.py
@@ -1,8 +1,11 @@
+from django.db.models.signals import post_delete, post_save
 from swapper import load_model
 
 from ..tests.utils import CreateConfigMixin
 
+Device = load_model("config", "Device")
 WhoIsInfo = load_model("config", "WhoIsInfo")
+OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings")
 
 
 class CreateWhoIsMixin(CreateConfigMixin):
@@ -26,3 +29,24 @@ def _create_who_is_info(self, **kwargs):
         w.full_clean()
         w.save()
         return w
+
+    # Signals are connected when apps are loaded,
+    # and if WhoIs is Configured all related WhoIs
+    # handlers are also connected. Thus we need to
+    # disconnect them.
+    def _disconnect_signals(self):
+        post_delete.disconnect(
+            WhoIsInfo.device_who_is_info_delete_handler,
+            sender=Device,
+            dispatch_uid="device.delete_who_is_info",
+        )
+        post_save.disconnect(
+            WhoIsInfo.invalidate_org_settings_cache,
+            sender=OrganizationConfigSettings,
+            dispatch_uid="invalidate_org_config_cache_on_org_config_save",
+        )
+        post_delete.disconnect(
+            WhoIsInfo.invalidate_org_settings_cache,
+            sender=OrganizationConfigSettings,
+            dispatch_uid="invalidate_org_config_cache_on_org_config_delete",
+        )
