[chores:fix] Skipped WHOIS/Estimated Location tasks for deactivated devices #1325

Fixes #1325

---------

Co-authored-by: Gagan Deep <pandafy.dev@gmail.com>
Co-authored-by: Federico Capoano <f.capoano@openwisp.io>

Author: 3 people

openwisp_controller/config/controller/views.py

@@ -93,7 +93,11 @@ def _remove_duplicated_management_ip(self, device):
             where &= Q(organization_id=device.organization_id)
 
         queryset = self.model.objects.filter(where).exclude(pk=device.pk)
-        for dupe in queryset.only("pk", "key", "management_ip"):
+        for dupe in queryset.only("pk", "key", "management_ip", "_is_deactivated"):
+            # Include _is_deactivated in .only() even though it is not referenced
+            # in this loop: dupe.save() triggers signal handlers that call
+            # is_deactivated(), and omitting the field would cause a SELECT
+            # per save (N+1 query).
             dupe.management_ip = ""
             dupe.save(update_fields=["management_ip"])
 
@@ -108,7 +112,12 @@ def _remove_duplicated_last_ip(self, device):
             where &= Q(organization_id=device.organization_id)
 
         queryset = self.model.objects.filter(where).exclude(pk=device.pk)
-        for dupe in queryset.only("pk", "key", "last_ip"):
+        # Include _is_deactivated and organization to avoid N+1 queries:
+        # dupe.save() triggers signal handlers that call is_deactivated()
+        # and WHOIS checks that read device.organization.
+        for dupe in queryset.select_related("organization").only(
+            "pk", "key", "last_ip", "_is_deactivated", "organization__id"
+        ):
             dupe.last_ip = ""
             dupe.save(update_fields=["last_ip"])
 

openwisp_controller/config/tests/test_controller.py

@@ -3,8 +3,10 @@
 
 from django.core.cache import cache
 from django.core.exceptions import ValidationError
+from django.db import connection
 from django.http.response import Http404
 from django.test import TestCase
+from django.test.utils import CaptureQueriesContext
 from django.urls import reverse, reverse_lazy
 from swapper import load_model
 
@@ -1508,6 +1510,39 @@ def test_ip_fields_not_duplicated(self):
             cached_device1 = view.get_device()
             self.assertIsNone(cached_device1.management_ip)
 
+    @patch.object(app_settings, "WHOIS_CONFIGURED", True)
+    def test_remove_duplicated_last_ip_no_nplus1_queries(self):
+        # When WHOIS is configured, clearing a duplicate's last_ip runs
+        # process_ip_data_and_location during save(), which reads
+        # device.organization via is_whois_enabled. If organization is not
+        # selected upfront, each duplicate triggers extra SELECTs (N+1).
+        # The marginal cost per duplicate must be a single UPDATE only.
+        def _measure(n):
+            ip = f"192.168.40.{n}"
+            org = self._create_org(name=f"dupes-{n}", shared_secret=f"dupes-secret-{n}")
+            incoming = self._create_device(
+                organization=org,
+                name=f"incoming-{n}",
+                mac_address=f"00:11:22:33:{n:02x}:99",
+                last_ip=ip,
+            )
+            for i in range(n):
+                self._create_device(
+                    organization=org,
+                    name=f"dupe-{n}-{i}",
+                    mac_address=f"00:11:22:33:{n:02x}:{i:02x}",
+                    last_ip=ip,
+                )
+            view = DeviceChecksumView()
+            with CaptureQueriesContext(connection) as ctx:
+                view._remove_duplicated_last_ip(incoming)
+            return len(ctx.captured_queries)
+
+        one = _measure(1)
+        three = _measure(3)
+        # two extra duplicates must add exactly two queries (one UPDATE each)
+        self.assertEqual(three - one, 2)
+
     @patch.object(app_settings, "SHARED_MANAGEMENT_IP_ADDRESS_SPACE", True)
     def test_organization_shares_management_ip_address_space(self):
         org1 = self._get_org()

openwisp_controller/config/tests/test_handlers.py

@@ -17,7 +17,7 @@ def test_organization_disabled_handler(self, mocked_task):
         with self.subTest("Test task executed on changing active to inactive org"):
             org.is_active = False
             org.save()
-            mocked_task.assert_called_once()
+            mocked_task.assert_called_once_with(str(org.id))
 
         mocked_task.reset_mock()
         with self.subTest("Test task not executed on saving inactive org"):

openwisp_controller/config/whois/commands.py

@@ -37,9 +37,11 @@ def clear_last_ip_command(stdout, stderr, interactive=True):
         Device.objects.filter(_is_deactivated=False).select_related(
             "organization__config_settings"
         )
-        # include the FK field 'organization' in .only() so the related
-        # `organization__config_settings` traversal is not deferred
-        .only("last_ip", "organization", "key")
+        # Keep 'organization' and '_is_deactivated' loaded. The former is
+        # needed for the select_related() traversal, while the latter is
+        # accessed during save() signal handling via is_deactivated();
+        # deferring either field would result in additional queries.
+        .only("last_ip", "organization", "key", "_is_deactivated")
     )
     # Filter out devices that have WHOIS information for their last IP
     devices = devices.exclude(last_ip=None).exclude(

openwisp_controller/config/whois/service.py

@@ -186,10 +186,10 @@ def _need_whois_lookup(self, new_ip):
             - WHOIS is disabled in the organization settings. (query from db)
         """
         # Check cheap conditions first before hitting the database
-        if not self.is_whois_enabled:
-            return False
         if not self.is_valid_public_ip_address(new_ip):
             return False
+        if not self.is_whois_enabled:
+            return False
         whois_obj = self._get_whois_info_from_db(ip_address=new_ip).first()
         if whois_obj and not self.is_older(whois_obj.modified):
             return False
@@ -208,8 +208,14 @@ def get_device_whois_info(self):
     def process_ip_data_and_location(self, force_lookup=False):
         """
         Trigger WHOIS lookup based on the conditions of `_need_whois_lookup`.
+        Returns early if the device is deactivated.
         Tasks are triggered on commit to ensure redundant data is not created.
         """
+        # Do not trigger WHOIS fetch for deactivated devices.
+        # Returning here also suppresses the whois_lookup_skipped signal emitted
+        # below, so estimated location is not triggered for a deactivated device.
+        if self.device.is_deactivated():
+            return
         new_ip = self.device.last_ip
         initial_ip = self.device._initial_last_ip
         if force_lookup or self._need_whois_lookup(new_ip):
@@ -228,7 +234,11 @@ def update_whois_info(self):
         Update existing WHOIS data for the device
         when the data is older than
         ``OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS``.
+        Returns early if the device is deactivated.
         """
+        # Do not refresh WHOIS data for deactivated devices.
+        if self.device.is_deactivated():
+            return
         ip_address = self.device.last_ip
         if not self.is_valid_public_ip_address(ip_address):
             return

openwisp_controller/config/whois/tasks.py

@@ -72,6 +72,11 @@ def fetch_whois_details(self, device_pk, initial_ip_address):
     except Device.DoesNotExist:
         logger.warning(f"Device {device_pk} not found, skipping WHOIS lookup")
         return
+    # Defense in depth: a device can be deactivated after the task is queued,
+    # so re-check here and do not run the external lookup for it.
+    if device.is_deactivated():
+        logger.info(f"Device {device_pk} is deactivated, skipping WHOIS lookup")
+        return
     new_ip_address = device.last_ip
     whois_service = device.whois_service
     # If there is existing WHOIS older record then it needs to be updated

openwisp_controller/config/whois/tests/tests.py

@@ -177,9 +177,10 @@ def test_whois_enabled(self):
         )
 
         with self.subTest("Test WHOIS not configured does not allow enabling WHOIS"):
-            with mock.patch.object(
-                app_settings, "WHOIS_CONFIGURED", False
-            ), self.assertRaises(ValidationError) as context_manager:
+            with (
+                mock.patch.object(app_settings, "WHOIS_CONFIGURED", False),
+                self.assertRaises(ValidationError) as context_manager,
+            ):
                 org_settings_obj.full_clean()
             self.assertEqual(
                 context_manager.exception.message_dict["whois_enabled"][0],
@@ -483,9 +484,12 @@ def test_whois_task_called(self, mocked_lookup_task):
         org.config_settings.save()
 
         with self.subTest("WHOIS lookup task called when last_ip is public"):
-            with mock.patch(
-                "django.core.cache.cache.get", return_value=None
-            ) as mocked_get, mock.patch("django.core.cache.cache.set") as mocked_set:
+            with (
+                mock.patch(
+                    "django.core.cache.cache.get", return_value=None
+                ) as mocked_get,
+                mock.patch("django.core.cache.cache.set") as mocked_set,
+            ):
                 device = self._create_device(last_ip="172.217.22.14")
                 mocked_lookup_task.assert_called()
                 mocked_set.assert_called_once_with(
@@ -499,9 +503,10 @@ def test_whois_task_called(self, mocked_lookup_task):
         with self.subTest(
             "WHOIS lookup task called when last_ip is changed and is public"
         ):
-            with mock.patch("django.core.cache.cache.get") as mocked_get, mock.patch(
-                "django.core.cache.cache.set"
-            ) as mocked_set:
+            with (
+                mock.patch("django.core.cache.cache.get") as mocked_get,
+                mock.patch("django.core.cache.cache.set") as mocked_set,
+            ):
                 device.last_ip = "172.217.22.10"
                 device.save()
                 device.refresh_from_db()
@@ -823,11 +828,13 @@ def test_device_last_ip_deferred_checks(self):
             device._check_last_ip()
             self.assertTrue(device._is_deferred("last_ip"))
 
-        with self.subTest(
-            "Test update_whois does not run if last_ip is deferred"
-        ), mock.patch(
-            "openwisp_controller.config.whois.service.WHOISService.update_whois_info"
-        ) as mock_update_whois:
+        with (
+            self.subTest("Test update_whois does not run if last_ip is deferred"),
+            mock.patch(
+                "openwisp_controller.config.whois.service"
+                ".WHOISService.update_whois_info"
+            ) as mock_update_whois,
+        ):
             threshold = app_settings.WHOIS_REFRESH_THRESHOLD_DAYS + 1
             new_time = timezone.now() - timedelta(days=threshold)
             WHOISInfo.objects.filter(pk=whois_obj.pk).update(modified=new_time)
@@ -871,9 +878,12 @@ def test_whois_task_failure_notification(self, mock_info, mock_warn, mock_error)
         def assert_logging_on_exception(
             exception, info_calls=0, warn_calls=0, error_calls=1, notification_count=1
         ):
-            with self.subTest(
-                f"Test notification and logging when {exception.__name__} is raised"
-            ), mock.patch(self._WHOIS_GEOIP_CLIENT) as mock_client:
+            with (
+                self.subTest(
+                    f"Test notification and logging when {exception.__name__} is raised"
+                ),
+                mock.patch(self._WHOIS_GEOIP_CLIENT) as mock_client,
+            ):
                 mock_client.return_value.city.side_effect = exception("test")
                 Device.objects.all().delete()  # Clear existing devices
                 device = self._create_device(last_ip="172.217.22.14")
@@ -947,9 +957,10 @@ def trigger_error_and_assert_cached(exc, notification_count=0):
                 ):
                     trigger_error_and_assert_cached(subsequent_error, 0)
 
-        with self.subTest("Test cache updated on success"), mock.patch(
-            self._WHOIS_GEOIP_CLIENT
-        ) as mock_client:
+        with (
+            self.subTest("Test cache updated on success"),
+            mock.patch(self._WHOIS_GEOIP_CLIENT) as mock_client,
+        ):
             Device.objects.all().delete()
             mocked_response = self._mocked_client_response()
             mocked_response.location = None
@@ -965,9 +976,12 @@ def trigger_error_and_assert_cached(exc, notification_count=0):
     @mock.patch("openwisp_controller.config.whois.tasks.fetch_whois_details.retry")
     def test_whois_task_retry_mechanism(self, mock_retry):
         def assert_retry_on_exception(exception, should_retry):
-            with self.subTest(
-                f"Test retry mechanism when {exception.__name__} is raised"
-            ), mock.patch(self._WHOIS_GEOIP_CLIENT) as mock_client:
+            with (
+                self.subTest(
+                    f"Test retry mechanism when {exception.__name__} is raised"
+                ),
+                mock.patch(self._WHOIS_GEOIP_CLIENT) as mock_client,
+            ):
                 mock_client.return_value.city.side_effect = exception("test")
                 Device.objects.all().delete()
                 mock_retry.reset_mock()
@@ -995,6 +1009,20 @@ def test_fetch_whois_details_device_not_found(self, mock_warn):
             f"Device {invalid_pk} not found, skipping WHOIS lookup"
         )
 
+    @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True)
+    @mock.patch(_WHOIS_TASKS_INFO_LOGGER)
+    @mock.patch(_WHOIS_GEOIP_CLIENT)
+    def test_fetch_whois_details_skips_when_deactivated(self, mock_client, mock_info):
+        whois_obj = self._create_whois_info(ip_address="8.8.8.8")
+        device = self._create_device(last_ip=whois_obj.ip_address)
+        mock_client.reset_mock()
+        device.deactivate()
+        fetch_whois_details(device_pk=device.pk, initial_ip_address="10.0.0.1")
+        mock_info.assert_called_once_with(
+            f"Device {device.pk} is deactivated, skipping WHOIS lookup"
+        )
+        mock_client.assert_not_called()
+
     @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True)
     @mock.patch(_WHOIS_GEOIP_CLIENT)
     def test_fetch_whois_details_record_already_exists(self, mock_client):
@@ -1077,6 +1105,67 @@ def test_get_whois_info_when_not_configured(self):
         result = get_whois_info(pk=device.pk)
         self.assertIsNone(result)
 
+    @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True)
+    @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay")
+    def test_process_ip_skips_when_deactivated(self, mock_task):
+        # Public IP that would normally trigger a lookup, so the only reason
+        # the task is not enqueued is the deactivation guard under test.
+        device = self._create_device()
+        org_settings = device.organization.config_settings
+        org_settings.whois_enabled = True
+        org_settings.save()
+        device._initial_last_ip = None
+        device.last_ip = "8.8.8.8"
+        device.save()
+        mock_task.reset_mock()
+        device.deactivate()
+        service = WHOISService(device)
+        service.process_ip_data_and_location()
+        self.assertEqual(mock_task.call_count, 0)
+
+    @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True)
+    @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay")
+    def test_process_ip_runs_when_active(self, mock_task):
+        device = self._create_device()
+        org_settings = device.organization.config_settings
+        org_settings.whois_enabled = True
+        org_settings.save()
+        device._initial_last_ip = None
+        device.last_ip = "8.8.8.8"
+        device.save()
+        mock_task.reset_mock()
+        service = WHOISService(device)
+        service.process_ip_data_and_location()
+        # transaction.on_commit executes immediately in TransactionTestCase,
+        # so the task is triggered synchronously here
+        # _initial_last_ip is '8.8.8.8' here because _check_last_ip in
+        # device.save() set it to device.last_ip after the first save.
+        mock_task.assert_called_once_with(
+            device_pk=device.pk,
+            initial_ip_address="8.8.8.8",
+        )
+
+    @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True)
+    @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay")
+    def test_update_whois_skips_when_deactivated(self, mock_task):
+        device = self._create_device()
+        org_settings = device.organization.config_settings
+        org_settings.whois_enabled = True
+        org_settings.save()
+        ip = "8.8.8.8"
+        device._initial_last_ip = None
+        device.last_ip = ip
+        device.save()
+        threshold = app_settings.WHOIS_REFRESH_THRESHOLD_DAYS + 1
+        expired_time = timezone.now() - timedelta(days=threshold)
+        whois_obj = self._create_whois_info(ip_address=ip)
+        WHOISInfo.objects.filter(pk=whois_obj.pk).update(modified=expired_time)
+        device.deactivate()
+        mock_task.reset_mock()
+        service = WHOISService(device)
+        service.update_whois_info()
+        mock_task.assert_not_called()
+
 
 @tag("selenium_tests")
 class TestWHOISSelenium(CreateWHOISMixin, SeleniumTestMixin, StaticLiveServerTestCase):

openwisp_controller/geo/estimated_location/service.py

@@ -63,6 +63,9 @@ def is_estimated_location_enabled(self):
         return self.check_estimated_location_enabled(self.device.organization_id)
 
     def trigger_estimated_location_task(self, ip_address):
+        # Do not re-derive estimated location for deactivated devices.
+        if self.device.is_deactivated():
+            return
         try:
             current_app.send_task(
                 "whois_estimated_location_task",

openwisp_controller/geo/estimated_location/tasks.py

@@ -121,6 +121,13 @@ def manage_estimated_locations(device_pk, ip_address):
             f"Device {device_pk} not found, skipping manage_estimated_locations"
         )
         return
+    # Defense in depth: a device can be deactivated after the task is queued,
+    # so re-check here.
+    if device.is_deactivated():
+        logger.info(
+            f"Device {device_pk} is deactivated, skipping estimated location task"
+        )
+        return
     devices_with_location = list(
         # "devicelocation" and "devicelocation__location" must be in only() to
         # prevent Django from deferring them, which would conflict with