[change] Added generic enforcement of deactivated devices #1338

openwisp_controller/config/base/config.py

@@ -482,6 +482,11 @@ def certificate_updated(cls, instance, created, **kwargs):
         except ObjectDoesNotExist:
             return
         else:
+            # Skip deactivated (and deactivating) configs: they are meant to stay
+            # inert, so a certificate renewal must not recompute their checksum or
+            # mark them modified again.
+            if config.is_deactivating_or_deactivated():
+                return
             transaction.on_commit(config.update_status_if_checksum_changed)
 
     @classmethod
@@ -996,6 +1001,8 @@ 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)
+        if config.is_deactivating_or_deactivated():
+            return
         device_group = config.device.group
         if not device_group:
             return

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,7 +481,7 @@ def create_default_config(self, **options):
         creates a new config instance to apply group templates
         if group has templates.
         """
-        if not (self.group and self.group.templates.exists()):
+        if self.is_deactivated() or not (self.group and self.group.templates.exists()):
             return
         config = self.get_temp_config_instance(
             backend=app_settings.DEFAULT_BACKEND, **options
@@ -499,6 +501,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")

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)

openwisp_controller/config/controller/views.py

@@ -399,7 +399,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:

openwisp_controller/config/tests/test_device.py

@@ -512,6 +512,23 @@ 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()
+        self.assertEqual(device.config.templates.count(), 0)
+        self.assertNotIn(new_template, device.config.templates.all())
+
     def test_device_field_changed_checks(self):
         self._create_device()
         device_group = self._create_device_group()

openwisp_controller/config/whois/service.py

@@ -210,6 +210,8 @@ def process_ip_data_and_location(self, force_lookup=False):
         Trigger WHOIS lookup based on the conditions of `_need_whois_lookup`.
         Tasks are triggered on commit to ensure redundant data is not created.
         """
+        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):

openwisp_controller/config/whois/tasks.py

@@ -72,6 +72,9 @@ 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
+    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 +117,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)

openwisp_controller/connection/apps.py

@@ -89,7 +89,7 @@ def _launch_update_config(cls, device):
         from .tasks import update_config
 
         # Check if push update should be skipped
-        if device.should_skip_push_update():
+        if device.is_fully_deactivated() or device.should_skip_push_update():
             return
         update_config.delay(str(device.pk))
 

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:

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")
@@ -76,6 +79,14 @@ def launch_command(command_id):
     except Command.DoesNotExist as e:
         logger.warning(f'launch_command("{command_id}") failed: {e}')
         return
+    # Do not execute commands on deactivated devices. Creation is already
+    # rejected by Command.clean(); this guards the case where the device was
+    # deactivated after the command had been queued.
+    if command.device.is_deactivated():
+        command.status = "failed"
+        command._add_output(_("Device is deactivated."))
+        command.save()
+        return
     try:
         command.execute()
     except SoftTimeLimitExceeded:

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,

openwisp_controller/connection/tests/test_models.py

@@ -349,6 +349,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 +1173,27 @@ 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()
+        )
+
+        conf.device.deactivate()
+
+        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))

openwisp_controller/connection/tests/test_tasks.py

@@ -163,6 +163,24 @@ def test_launch_command_exception(self, *args):
         self.assertEqual(command.status, "failed")
         self.assertEqual(command.output, "Internal system error: test error\n")
 
+    @mock.patch(_mock_execute)
+    def test_launch_command_deactivated_device(self, mocked_execute):
+        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_execute.assert_not_called()
+
 
 class TestTransactionTasks(
     TestRegistrationMixin, CreateConnectionsMixin, TransactionTestCase

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)

openwisp_controller/geo/estimated_location/tasks.py

@@ -121,6 +121,11 @@ def manage_estimated_locations(device_pk, ip_address):
             f"Device {device_pk} not found, skipping manage_estimated_locations"
         )
         return
+    if device.is_deactivated():
+        logger.info(
+            f"Device {device_pk} is deactivated, skipping manage_estimated_locations"
+        )
+        return
     devices_with_location = list(
         # "devicelocation" and "devicelocation__location" must be in only() to
         # prevent Django from deferring them, which would conflict with

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).count():
             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(