diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index fd1fc01d2..3d0a08f17 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -857,13 +857,15 @@ def activate(self): self._invalidate_backend_instance_cache() old_checksum = self.checksum self.add_default_templates() - if self.device._get_group(): - self.device.manage_devices_group_templates( - device_ids=self.device.id, - old_group_ids=None, - group_id=self.device.group_id, + if self.device._has_group(): + # Call manage_group_templates directly rather than going through + # manage_devices_group_templates, which would skip the device because + # it is still marked as deactivated at this point in the activation flow. + self.manage_group_templates( + self.device.group.templates.all(), + self.get_template_model().objects.none(), ) - del self.backend_instance + self._invalidate_backend_instance_cache() if old_checksum == self.checksum: # Accelerate activation if the configuration remains # unchanged (i.e. empty configuration) @@ -996,6 +998,11 @@ def manage_backend_changed(cls, instance_id, old_backend, backend, **kwargs): Config = load_model("config", "Config") Template = load_model("config", "Template") config = Config.objects.get(pk=instance_id) + # All modification operations are blocked on deactivated devices. + # Thus, a user cannot edit the backend for device when it is deactivating. + # Therefore. it will be safe to block this operation here. + if config.is_deactivating_or_deactivated(): + return device_group = config.device.group if not device_group: return diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py index bcfff44ca..2290a4968 100644 --- a/openwisp_controller/config/base/device.py +++ b/openwisp_controller/config/base/device.py @@ -185,6 +185,11 @@ def _get_organization__config_settings(self): def is_deactivated(self): return self._is_deactivated + def is_fully_deactivated(self): + return self.is_deactivated() and ( + not self._has_config() or self.config.is_deactivated() + ) + def deactivate(self): if self.is_deactivated(): # The device has already been deactivated. @@ -299,10 +304,7 @@ def save(self, *args, **kwargs): self._check_changed_fields() def delete(self, using=None, keep_parents=False, check_deactivated=True): - if check_deactivated and ( - not self.is_deactivated() - or (self._has_config() and not self.config.is_deactivated()) - ): + if check_deactivated and (not self.is_fully_deactivated()): raise PermissionDenied("The device must be deactivated prior to deletion") return super().delete(using, keep_parents) @@ -479,6 +481,10 @@ def create_default_config(self, **options): creates a new config instance to apply group templates if group has templates. """ + if self.is_deactivated(): + # All modification operations are blocked on deactivated devices. + # Hence, default config should not be created for deactivated devices. + return if not (self.group and self.group.templates.exists()): return config = self.get_temp_config_instance( @@ -499,6 +505,11 @@ def manage_devices_group_templates(cls, device_ids, old_group_ids, group_id): old_group_ids = [old_group_ids] for device_id, old_group_id in zip(device_ids, old_group_ids): device = Device.objects.get(pk=device_id) + if device.is_deactivated(): + # Skip deactivated devices: their configuration is intentionally + # emptied during deactivation, so re-applying group templates + # would break that state and trigger a push to the device. + continue if not hasattr(device, "config"): device.create_default_config() config_created = hasattr(device, "config") diff --git a/openwisp_controller/config/base/device_group.py b/openwisp_controller/config/base/device_group.py index d7f6be31a..7983c1fd9 100644 --- a/openwisp_controller/config/base/device_group.py +++ b/openwisp_controller/config/base/device_group.py @@ -110,7 +110,7 @@ def manage_group_templates(cls, group_id, old_template_ids, template_ids): device_group = DeviceGroup.objects.get(id=group_id) templates = Template.objects.filter(pk__in=template_ids) old_templates = Template.objects.filter(pk__in=old_template_ids) - for device in device_group.device_set.iterator(): + for device in device_group.device_set.exclude(_is_deactivated=True).iterator(): if not hasattr(device, "config"): device.create_default_config() device.config.manage_group_templates(templates, old_templates) diff --git a/openwisp_controller/config/controller/views.py b/openwisp_controller/config/controller/views.py index 93f011757..5b4474d55 100644 --- a/openwisp_controller/config/controller/views.py +++ b/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,10 @@ 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"): + for dupe in queryset.only("pk", "key", "last_ip", "_is_deactivated"): + # Include _is_deactivated in .only() to avoid N+1 queries: + # dupe.save() triggers signal handlers that call is_deactivated(), + # so the flag is prefetched rather than loaded per row. dupe.last_ip = "" dupe.save(update_fields=["last_ip"]) @@ -221,6 +228,12 @@ def post(self, request, *args, **kwargs): bad_request = forbid_unallowed(request, "POST", "key", device.key) if bad_request: return bad_request + if device.is_deactivated(): + # Inventory metadata must not be recorded for deactivated devices. + # GetDeviceView already returns 404 once the config is fully + # "deactivated"; this additionally rejects the transient + # "deactivating" state, where the device flag is already set. + return ControllerResponse("error: device deactivated", status=403) # update device information for attr in self.UPDATABLE_FIELDS: if attr in request.POST: @@ -399,7 +412,9 @@ def post(self, request, *args, **kwargs): # (key is not None only if CONSISTENT_REGISTRATION is enabled) new = False try: - device = self.model.objects.get(key=key) + device = self.model.objects.select_related("config").get(key=key) + if device.is_deactivated(): + return ControllerResponse(_("error: device deactivated"), status=403) # update device info for attr in self.UPDATABLE_FIELDS: if attr in request.POST: diff --git a/openwisp_controller/config/tests/test_controller.py b/openwisp_controller/config/tests/test_controller.py index d8d43ecfa..97ee97df4 100644 --- a/openwisp_controller/config/tests/test_controller.py +++ b/openwisp_controller/config/tests/test_controller.py @@ -784,6 +784,22 @@ def test_device_consistent_registration_exists_no_config(self): d.refresh_from_db() self.assertIsNotNone(d.config) + @capture_any_output() + def test_register_deactivated_device(self): + device = self._create_device_config() + device.key = TEST_CONSISTENT_KEY + device.save() + device.deactivate() + original_os = device.os + params = self._get_reregistration_payload( + device, name=device.name, os="OpenWrt 22.03" + ) + response = self.client.post(self.register_url, params) + self.assertContains(response, "error: device deactivated", status_code=403) + device.refresh_from_db() + self.assertEqual(device.os, original_os) + self.assertEqual(device.config.templates.count(), 0) + def test_device_registration_update_hw_info(self): d = self._create_device_config() d.key = TEST_CONSISTENT_KEY @@ -1073,9 +1089,37 @@ def test_device_update_info(self): self.assertEqual(d.model, params["model"]) def test_deactivated_device_update_info(self): - self._test_deactivating_deactivated_device_view( - "device_update_info", method="post", data={} - ) + # Inventory metadata updates must be rejected for deactivated devices, + # including the transient "deactivating" state (which GetDeviceView does + # not exclude on its own). + self._create_template(required=True) + device = self._create_device_config() + url = reverse("controller:device_update_info", args=[device.pk]) + params = { + "key": device.key, + "model": "TP-Link TL-WDR4300 v2", + "os": "OpenWrt 18.06-SNAPSHOT r7312-e60be11330", + "system": "Atheros AR9344 rev 3", + } + device.deactivate() + device.refresh_from_db() + self.assertEqual(device.config.status, "deactivating") + initial = (device.os, device.model, device.system) + # payload must differ from current values so a missed block is detectable + self.assertNotEqual((params["os"], params["model"], params["system"]), initial) + + with self.subTest("rejected while deactivating"): + response = self.client.post(url, params) + self.assertEqual(response.status_code, 403) + self._check_header(response) + device.refresh_from_db() + # metadata must be left untouched + self.assertEqual((device.os, device.model, device.system), initial) + + with self.subTest("not found once fully deactivated"): + device.config.set_status_deactivated() + response = self.client.post(url, params) + self.assertEqual(response.status_code, 404) def test_device_update_info_bad_uuid(self): d = self._create_device_config() diff --git a/openwisp_controller/config/tests/test_device.py b/openwisp_controller/config/tests/test_device.py index d9bc3f59d..1e2bd4a3e 100644 --- a/openwisp_controller/config/tests/test_device.py +++ b/openwisp_controller/config/tests/test_device.py @@ -512,6 +512,27 @@ def test_device_group_changed_not_emitted_on_creation(self): self._create_device(name="test", organization=org, group=device_group) handler.assert_not_called() + def test_manage_devices_group_templates_skips_deactivated_devices(self): + org = self._get_org() + old_template = self._create_template(name="old-template", organization=org) + new_template = self._create_template(name="new-template", organization=org) + old_group = self._create_device_group(name="old-group", organization=org) + new_group = self._create_device_group(name="new-group", organization=org) + old_group.templates.add(old_template) + new_group.templates.add(new_template) + device = self._create_device(name="test", organization=org, group=old_group) + device.deactivate() + device.config.refresh_from_db() + self.assertEqual(device.config.templates.count(), 0) + Device.manage_devices_group_templates(device.pk, old_group.pk, new_group.pk) + device.config.refresh_from_db() + # Deactivated device config is skipped: adding templates to a cleared + # config would misrepresent what has been applied to the device. + self.assertEqual(device.config.templates.count(), 0) + self.assertNotIn(new_template, device.config.templates.all()) + # Status must remain "deactivated" — no config push is initiated. + self.assertEqual(device.config.status, "deactivated") + def test_device_field_changed_checks(self): self._create_device() device_group = self._create_device_group() diff --git a/openwisp_controller/config/whois/commands.py b/openwisp_controller/config/whois/commands.py index dafbe92e8..fe4dcb8b7 100644 --- a/openwisp_controller/config/whois/commands.py +++ b/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( diff --git a/openwisp_controller/config/whois/service.py b/openwisp_controller/config/whois/service.py index 43a6ab3db..0110ab307 100644 --- a/openwisp_controller/config/whois/service.py +++ b/openwisp_controller/config/whois/service.py @@ -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 diff --git a/openwisp_controller/config/whois/tasks.py b/openwisp_controller/config/whois/tasks.py index 27532b9fc..de20cd547 100644 --- a/openwisp_controller/config/whois/tasks.py +++ b/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 @@ -114,6 +119,8 @@ def delete_whois_record(ip_address, force=False): if force: queryset.delete() else: + # Only delete the WHOIS record if there are no active devices + # linked to the same IP address. if ( not Device.objects.filter(_is_deactivated=False) .filter(last_ip=ip_address) diff --git a/openwisp_controller/config/whois/tests/tests.py b/openwisp_controller/config/whois/tests/tests.py index 05aaf644e..8c0d284cd 100644 --- a/openwisp_controller/config/whois/tests/tests.py +++ b/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): diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index cb6d47523..68df995dd 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -141,6 +141,10 @@ def auto_add_to_devices(cls, credential_id, organization_id): DeviceConnection = load_model("connection", "DeviceConnection") Device = load_model("config", "Device") + # Deactivated devices are intentionally included: attaching credentials + # is a database-only operation with no network activity (the connection + # cannot be used while the device is deactivated), and having the + # credentials ready avoids extra setup when the device is reactivated. devices = Device.objects.exclude(config=None) if organization_id: devices = devices.filter(organization_id=organization_id) @@ -179,6 +183,9 @@ def auto_add_credentials_to_device(cls, instance, created, **kwargs): if not created: return device = instance.device + # Credentials are attached even when the device is deactivated: this only + # creates DeviceConnection rows (no network operation) and avoids extra + # setup on reactivation. See auto_add_to_devices for the same rationale. # select credentials which # - are flagged as auto_add # - belong to the same organization of the device @@ -266,6 +273,11 @@ def __init__(self, *args, **kwargs): def get_working_connection( cls, device, connector="openwisp_controller.connection.connectors.ssh.Ssh" ): + # Deactivated devices are not filtered out here on purpose: a device that + # is still "deactivating" needs one last connection so the cleared + # configuration can be pushed to it. connect() below refuses fully + # deactivated devices, so only the legitimate deactivating push gets + # through. qs = cls.objects.filter( device=device, enabled=True, @@ -345,6 +357,11 @@ def set_connector(self, connector): def connect(self): try: + # Refuse fully deactivated devices (device deactivated and its config + # already "deactivated"). A device that is only "deactivating" is let + # through so the final cleared configuration can still be pushed. + if self.device.is_fully_deactivated(): + raise RuntimeError(_("Device is deactivated")) self.connector_instance.connect() except Exception as e: self.is_working = False @@ -378,6 +395,8 @@ def save(self, *args, **kwargs): self._initial_failure_reason = self.failure_reason def send_is_working_changed_signal(self): + if self.device.is_fully_deactivated(): + return is_working_changed.send( sender=self.__class__, is_working=self.is_working, @@ -462,6 +481,8 @@ def __str__(self): return f'«{command}» {sent} {created.strftime("%d %b %Y at %I:%M %p")}' def clean(self): + if self.device.is_deactivated(): + raise ValidationError({"device": _("Device is deactivated.")}) self._verify_command_type_allowed() self._verify_connection() try: @@ -533,6 +554,14 @@ def execute(self): raise RuntimeError( "This command has already been executed, " "please create a new one." ) + # Guard against a device that was deactivated after the command was queued. + # Command.clean() already rejects creation for deactivated devices, but + # the device could be deactivated while the task is still pending. + if self.device.is_deactivated(): + self.status = "failed" + self._add_output(_("Device is deactivated.")) + self.save() + return exit_code = self._exec_command() # if output is None, the commands couldn't execute # because the system couldn't connect to the device diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index 56e6a7bb1..a52b826d8 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -46,6 +46,9 @@ def update_config(self, device_id): time.sleep(2) try: device = Device.objects.select_related("config").get(pk=device_id) + if device.is_fully_deactivated(): + logger.info(f"{device} (pk: {device_id}) is deactivated, skipping update") + return # abort operation if device shouldn't be updated if not device.can_be_updated(): logger.info(f"{device} (pk: {device_id}) is not going to be updated") diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index ab94494c1..d7eac7531 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -287,6 +287,7 @@ def test_endpoints_for_non_existent_device(self): self.assertDictEqual(response.data, device_not_found) def test_endpoints_for_deactivated_device(self): + command = self._create_command(device_conn=self.device_conn) self.device_conn.device.deactivate() with self.subTest("Test listing commands"): @@ -308,7 +309,6 @@ def test_endpoints_for_deactivated_device(self): self.assertEqual(response.status_code, 403) with self.subTest("Test retrieving commands"): - command = self._create_command(device_conn=self.device_conn) url = self._get_path("device_command_details", self.device_id, command.id) response = self.client.get( url, diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index 3dac7ffc6..51d8696a7 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -238,6 +238,31 @@ def test_ssh_connect_failure(self): self.assertIsNotNone(dc.last_attempt) self.assertEqual(dc.failure_reason, "Authentication failed.") + def test_connect_deactivated_device(self): + dc = self._create_device_connection() + + with self.subTest("fully deactivated: connect blocked, signal suppressed"): + dc.device.deactivate() + self.assertTrue(dc.device.is_fully_deactivated()) + with catch_signal(is_working_changed) as handler: + dc.connect() + self.assertEqual(dc.is_working, False) + self.assertEqual(dc.failure_reason, "Device is deactivated") + handler.assert_not_called() + + with self.subTest("deactivating: connect allowed through"): + cred2 = self._create_credentials(name="cred-deactivating") + device2 = self._create_device( + name="deactivating-device", mac_address="11:22:33:44:55:66" + ) + self._create_config(device=device2) + dc2 = self._create_device_connection(credentials=cred2, device=device2) + dc2.device._is_deactivated = True + self.assertEqual(dc2.device.is_fully_deactivated(), False) + with mock.patch.object(dc2.connector_instance, "connect") as mocked_conn: + dc2.connect() + mocked_conn.assert_called_once() + def test_credentials_schema(self): # unrecognized parameter try: @@ -349,6 +374,15 @@ def test_auto_add_to_new_device(self): self.assertEqual(d.deviceconnection_set.count(), 1) self.assertEqual(d.deviceconnection_set.first().credentials, c) + def test_auto_add_to_new_deactivated_device(self): + org = Organization.objects.first() + self._create_credentials(auto_add=True, organization=None) + device = self._create_device(organization=org, name="deactivated-device") + device.deactivate() + self._create_config(device=device) + device.refresh_from_db() + self.assertEqual(device.deviceconnection_set.count(), 1) + def test_auto_add_device_missing_config(self): org = Organization.objects.first() self._create_device(organization=org) @@ -1164,6 +1198,29 @@ def test_auto_add_to_existing_device_on_creation(self): self.assertEqual(d.deviceconnection_set.count(), 1) self.assertEqual(d.deviceconnection_set.first().credentials, c) + @mock.patch.object(DeviceConnection, "update_config") + @mock.patch.object(DeviceConnection, "get_working_connection") + @mock.patch("time.sleep") + def test_deactivating_device_update_config( + self, mocked_sleep, mocked_get_working_connection, mocked_update_config + ): + conf = self._prepare_conf_object() + conf.save() + mocked_get_working_connection.reset_mock() + mocked_update_config.reset_mock() + mocked_get_working_connection.return_value = ( + conf.device.deviceconnection_set.first() + ) + # Deactivate the device + conf.device.deactivate() + # Ensure that the config status is set to "deactivating" and + # update_config is called to apply the empty configuration for deactivated + # devices. + conf.refresh_from_db() + self.assertEqual(conf.status, "deactivating") + mocked_get_working_connection.assert_called_once_with(conf.device) + mocked_update_config.assert_called_once() + def test_chunk_size(self): org = self._get_org() self._create_config(device=self._create_device(organization=org)) diff --git a/openwisp_controller/connection/tests/test_tasks.py b/openwisp_controller/connection/tests/test_tasks.py index 818e21c4a..93ce232f3 100644 --- a/openwisp_controller/connection/tests/test_tasks.py +++ b/openwisp_controller/connection/tests/test_tasks.py @@ -13,6 +13,7 @@ from .utils import CreateConnectionsMixin Command = load_model("connection", "Command") +DeviceConnection = load_model("connection", "DeviceConnection") OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings") @@ -92,6 +93,25 @@ def test_update_config_missing_device(self, mocked_sleep, mocked_warning): ) mocked_sleep.assert_called_once() + @mock.patch("logging.Logger.info") + @mock.patch("time.sleep") + def test_update_config_skipped_for_deactivated_device( + self, mocked_sleep, mocked_info + ): + dc = self._create_device_connection() + device = dc.device + device.deactivate() + self.assertTrue(device.is_fully_deactivated()) + with mock.patch.object( + DeviceConnection, "get_working_connection" + ) as mocked_get_working_connection: + tasks.update_config.delay(device.pk) + mocked_get_working_connection.assert_not_called() + mocked_sleep.assert_called_once() + mocked_info.assert_called_with( + f"{device} (pk: {device.pk}) is deactivated, skipping update" + ) + @mock.patch("logging.Logger.warning") def test_launch_command_missing(self, mocked_warning): pk = uuid.uuid4() @@ -163,6 +183,26 @@ def test_launch_command_exception(self, *args): self.assertEqual(command.status, "failed") self.assertEqual(command.output, "Internal system error: test error\n") + @mock.patch( + "openwisp_controller.connection.base.models.AbstractCommand._exec_command" + ) + def test_launch_command_deactivated_device(self, mocked_exec_command): + dc = self._create_device_connection() + command = Command( + device=dc.device, + connection=dc, + type="custom", + input={"command": "/usr/sbin/exotic_command"}, + ) + command.full_clean() + command.save() + dc.device.deactivate() + tasks.launch_command.delay(command.pk) + command.refresh_from_db() + self.assertEqual(command.status, "failed") + self.assertEqual(command.output, "Device is deactivated.\n") + mocked_exec_command.assert_not_called() + class TestTransactionTasks( TestRegistrationMixin, CreateConnectionsMixin, TransactionTestCase diff --git a/openwisp_controller/geo/estimated_location/handlers.py b/openwisp_controller/geo/estimated_location/handlers.py index bb9ec6314..b2465e937 100644 --- a/openwisp_controller/geo/estimated_location/handlers.py +++ b/openwisp_controller/geo/estimated_location/handlers.py @@ -43,6 +43,7 @@ def whois_fetched_handler(sender, whois, updated_fields, device=None, **kwargs): if ( not updated_fields or not device + or device.is_deactivated() or not EstimatedLocationService.check_estimated_location_enabled( device.organization_id ) @@ -67,8 +68,13 @@ def whois_lookup_skipped_handler(sender, device, **kwargs): Handler for skipped WHOIS lookups. If estimated location is enabled for the device's organization, trigger an estimated location task. """ - if not EstimatedLocationService.check_estimated_location_enabled( - device.organization_id + # Skip deactivated devices: there is no value in estimating a location for + # a device that is no longer managed. + if ( + device.is_deactivated() + or not EstimatedLocationService.check_estimated_location_enabled( + device.organization_id + ) ): return estimated_location_service = EstimatedLocationService(device) diff --git a/openwisp_controller/geo/estimated_location/service.py b/openwisp_controller/geo/estimated_location/service.py index a3d661f81..86ff5215f 100644 --- a/openwisp_controller/geo/estimated_location/service.py +++ b/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", diff --git a/openwisp_controller/geo/estimated_location/tasks.py b/openwisp_controller/geo/estimated_location/tasks.py index 0ae63bdd8..e6f2b6363 100644 --- a/openwisp_controller/geo/estimated_location/tasks.py +++ b/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 diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py index 2218883a2..f96c47abf 100644 --- a/openwisp_controller/geo/estimated_location/tests/tests.py +++ b/openwisp_controller/geo/estimated_location/tests/tests.py @@ -194,6 +194,22 @@ def test_estimated_location_admin_add_whois_disabled(self): response = self.client.get(path) self.assertNotContains(response, "field-is_estimated") + @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) + @mock.patch( + "openwisp_controller.geo.estimated_location.service.current_app.send_task" + ) + def test_trigger_estimated_location_task_skips_when_deactivated( + self, mock_send_task + ): + org = self._get_org() + org.geo_settings.estimated_location_enabled = True + org.geo_settings.save() + device = self._create_device(last_ip="172.217.22.14") + device.deactivate() + service = EstimatedLocationService(device) + service.trigger_estimated_location_task(device.last_ip) + mock_send_task.assert_not_called() + class TestEstimatedLocationTransaction( TestEstimatedLocationMixin, WHOISTransactionMixin, TestGeoMixin, TransactionTestCase @@ -797,6 +813,19 @@ def test_manage_estimated_locations_no_coordinates_warning( "Estimated location cannot be determined." ) + @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) + @mock.patch(_ESTIMATED_LOCATION_INFO_LOGGER) + def test_manage_estimated_locations_skips_when_deactivated(self, mock_info): + # Create WHOIS info first to prevent eager task from attempting + # a real WHOIS lookup during device creation. + whois_obj = self._create_whois_info(ip_address="172.217.22.14") + device = self._create_device(last_ip=whois_obj.ip_address) + device.deactivate() + manage_estimated_locations(device.pk, device.last_ip) + mock_info.assert_called_once_with( + f"Device {device.pk} is deactivated, skipping estimated location task" + ) + @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) @mock.patch( "openwisp_controller.geo.estimated_location.service.current_app.send_task", diff --git a/openwisp_controller/geo/tests/utils.py b/openwisp_controller/geo/tests/utils.py index a81fcf331..4df89edc7 100644 --- a/openwisp_controller/geo/tests/utils.py +++ b/openwisp_controller/geo/tests/utils.py @@ -11,12 +11,12 @@ def _create_organization(self, **kwargs): options = dict(name="org1") options.update(kwargs) options.setdefault("slug", slugify(options["name"])) - if not Organization.objects.filter(**kwargs).count(): + if not Organization.objects.filter(**options).exists(): org = Organization(**options) org.full_clean() org.save() else: - org = Organization.objects.get(**kwargs) + org = Organization.objects.get(**options) return org def _add_default_org(self, kwargs): @@ -44,7 +44,10 @@ def _create_floorplan(self, **kwargs): def _create_object_location(self, **kwargs): if "location" not in kwargs: - kwargs["location"] = self._create_location() + location_kwargs = {} + if "content_object" in kwargs: + location_kwargs["organization"] = kwargs["content_object"].organization + kwargs["location"] = self._create_location(**location_kwargs) kwargs["organization"] = kwargs["location"].organization if "content_object" not in kwargs: kwargs["content_object"] = self._create_object( diff --git a/openwisp_controller/subnet_division/tests/test_models.py b/openwisp_controller/subnet_division/tests/test_models.py index 62e022563..7137d596c 100644 --- a/openwisp_controller/subnet_division/tests/test_models.py +++ b/openwisp_controller/subnet_division/tests/test_models.py @@ -815,6 +815,39 @@ def test_device_subnet_division_rule_existing_devices(self): self.ip_query.count(), (rule.number_of_subnets * rule.number_of_ips) ) + def test_device_subnet_division_rule_existing_devices_includes_deactivated(self): + subnet_query = self.subnet_query.filter(organization_id=self.org.id).exclude( + id=self.master_subnet.id + ) + self.config.device.deactivate() + self.assertEqual(subnet_query.count(), 0) + rule = self._get_device_subdivision_rule() + self.config.refresh_from_db() + self.assertTrue(self.config.device.is_deactivated()) + # Subnets are provisioned even for deactivated devices so re-activation + # does not require extra provisioning logic. + self.assertEqual(subnet_query.count(), rule.number_of_subnets) + self.assertEqual( + self.ip_query.count(), rule.number_of_subnets * rule.number_of_ips + ) + self.assertGreater(self.config.subnetdivisionindex_set.count(), 0) + + def test_device_subnet_division_rule_provisions_deactivated_on_config_creation( + self, + ): + # A rule already exists; subnets must be provisioned even when the config + # is created for a device that is already deactivated. + self._get_device_subdivision_rule() + device = self._create_device( + organization=self.org, + name="deactivated-device", + mac_address="00:11:22:33:44:66", + ) + device.deactivate() + config = self._create_config(device=device) + self.assertTrue(config.device.is_deactivated()) + self.assertGreater(config.subnetdivisionindex_set.count(), 0) + def test_vpn_subnet_division_rule_existing_devices(self): subnet_query = self.subnet_query.filter(organization_id=self.org.id).exclude( id=self.master_subnet.id