From 392d4c8e6e2d2c30b9c2666003f4ad6e46cb42f4 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 20 Nov 2024 04:48:08 +0530 Subject: [PATCH] [feature] Implemented device deactivation and reactivation #625 #849 Closes #625 Closes #849 * [feature] Do not delete related Certs when device is deactivated * [feature] Set device status to deactivated if current status is deactivating * [feature] Return 404 checksum for deactivated devices * [feature] Added activate and deactivate button on the device page * [feature] Added "config_deactivating" signal * [change] Show delete action after deactivate action * [feature] Emit device_deactivated signal when device is deactivated * [fix] Don't show any extra form on deactivated devices * [change] Clear management IP after the device is deactivated * [change] Added API endpoints for activating/deactivating device * [change] Updated device delete API endpoint * [change] Disable API operations on deactivated devices * [feature] Added "device_activated" signal * [docs] Updated docs * [fix] Don't allow PUT/PATCH request for deactivated device --------- Co-authored-by: Federico Capoano --- docs/developer/utils.rst | 49 +++ docs/index.rst | 1 + docs/user/device-config-status.rst | 38 ++ docs/user/rest-api.rst | 19 + openwisp_controller/config/admin.py | 200 ++++++++- openwisp_controller/config/api/serializers.py | 2 + openwisp_controller/config/api/urls.py | 10 + openwisp_controller/config/api/views.py | 43 +- openwisp_controller/config/apps.py | 18 + openwisp_controller/config/base/config.py | 134 +++++- openwisp_controller/config/base/device.py | 61 ++- .../config/controller/views.py | 19 +- .../migrations/0054_device__is_deactivated.py | 17 + .../migrations/0055_alter_config_status.py | 39 ++ openwisp_controller/config/signals.py | 16 + .../admin/config/device/change_form.html | 34 ++ .../device/delete_selected_confirmation.html | 36 ++ .../config/tests/test_admin.py | 390 ++++++++++++++++++ openwisp_controller/config/tests/test_api.py | 61 ++- openwisp_controller/config/tests/test_apps.py | 16 +- .../config/tests/test_config.py | 18 +- .../config/tests/test_controller.py | 69 +++- .../config/tests/test_device.py | 91 +++- .../config/tests/test_template.py | 2 +- openwisp_controller/config/tests/test_vpn.py | 8 +- openwisp_controller/connection/admin.py | 8 +- openwisp_controller/connection/api/views.py | 20 +- openwisp_controller/connection/apps.py | 5 +- .../connection/tests/test_api.py | 101 ++++- openwisp_controller/geo/admin.py | 6 +- openwisp_controller/geo/api/views.py | 14 +- openwisp_controller/geo/tests/test_api.py | 72 +++- openwisp_controller/mixins.py | 31 ++ .../subnet_division/tests/test_admin.py | 2 + .../subnet_division/tests/test_models.py | 6 +- openwisp_controller/tests/test_selenium.py | 5 +- tests/openwisp2/sample_config/api/views.py | 16 + ...ice__is_deactivated_alter_config_status.py | 41 ++ .../migrations/0007_alter_config_status.py | 39 ++ 39 files changed, 1690 insertions(+), 67 deletions(-) create mode 100644 docs/user/device-config-status.rst create mode 100644 openwisp_controller/config/migrations/0054_device__is_deactivated.py create mode 100644 openwisp_controller/config/migrations/0055_alter_config_status.py create mode 100644 openwisp_controller/config/templates/admin/config/device/change_form.html create mode 100644 openwisp_controller/config/templates/admin/config/device/delete_selected_confirmation.html create mode 100644 tests/openwisp2/sample_config/migrations/0006_device__is_deactivated_alter_config_status.py create mode 100644 tests/openwisp2/sample_config/migrations/0007_alter_config_status.py diff --git a/docs/developer/utils.rst b/docs/developer/utils.rst index 996fe4fd8..8cd9a47b8 100644 --- a/docs/developer/utils.rst +++ b/docs/developer/utils.rst @@ -146,6 +146,55 @@ object are changed, but only on ``post_add`` or ``post_remove`` actions, ``post_clear`` is ignored for the same reason explained in the previous section. +``config_deactivating`` +~~~~~~~~~~~~~~~~~~~~~~~ + +**Path**: ``openwisp_controller.config.signals.config_deactivating`` + +**Arguments**: + +- ``instance``: instance of the object being deactivated +- ``previous_status``: previous status of the object before deactivation + +This signal is emitted when a configuration status of device is set to +``deactivating``. + +``config_deactivated`` +~~~~~~~~~~~~~~~~~~~~~~ + +**Path**: ``openwisp_controller.config.signals.config_deactivated`` + +**Arguments**: + +- ``instance``: instance of the object being deactivated +- ``previous_status``: previous status of the object before deactivation + +This signal is emitted when a configuration status of device is set to +``deactivated``. + +``device_deactivated`` +~~~~~~~~~~~~~~~~~~~~~~ + +**Path**: ``openwisp_controller.config.signals.device_deactivated`` + +**Arguments**: + +- ``instance``: instance of the device being deactivated + +This signal is emitted when a device is flagged for deactivation. + +``device_activated`` +~~~~~~~~~~~~~~~~~~~~ + +**Path**: ``openwisp_controller.config.signals.device_activated`` + +**Arguments**: + +- ``instance``: instance of the device being activated + +This signal is emitted when a device is flagged for activation (after +deactivation). + .. _config_backend_changed: ``config_backend_changed`` diff --git a/docs/index.rst b/docs/index.rst index 40d4e9301..a708bd0ff 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -35,6 +35,7 @@ the OpenWISP architecture. :maxdepth: 1 user/intro.rst + user/device-config-status.rst user/templates.rst user/variables.rst user/device-groups.rst diff --git a/docs/user/device-config-status.rst b/docs/user/device-config-status.rst new file mode 100644 index 000000000..68d6c540b --- /dev/null +++ b/docs/user/device-config-status.rst @@ -0,0 +1,38 @@ +Device Configuration Status +=========================== + +The device's configuration status (`Device.config.status`) indicates the +current state of the configuration as managed by OpenWISP. The possible +statuses and their meanings are explained below. + +``modified`` +------------ + +The device configuration has been updated in OpenWISP, but these changes +have not yet been applied to the device. The device is pending an update. + +``applied`` +----------- + +The device has successfully applied the configuration changes made in +OpenWISP. The current configuration on the device matches the latest +changes. + +``error`` +--------- + +An issue occurred while applying the configuration to the device, causing +the device to revert to its previous working configuration. + +``deactivating`` +---------------- + +The device is in the process of being deactivated. The configuration is +scheduled to be removed from the device. + +``deactivated`` +--------------- + +The device has been deactivated. The configuration applied through +OpenWISP has been removed, and any other operation to manage the device +will be prevented or rejected. diff --git a/docs/user/rest-api.rst b/docs/user/rest-api.rst index d4deba46d..a66519a7e 100644 --- a/docs/user/rest-api.rst +++ b/docs/user/rest-api.rst @@ -233,10 +233,29 @@ from the config of a device, Delete Device ~~~~~~~~~~~~~ +.. note:: + + A device must be deactivated before it can be deleted. Otherwise, an + ``HTTP 403 Forbidden`` response will be returned. + .. code-block:: text DELETE /api/v1/controller/device/{id}/ +Deactivate Device +~~~~~~~~~~~~~~~~~ + +.. code-block:: text + + POST /api/v1/controller/device/{id}/deactivate/ + +Activate Device +~~~~~~~~~~~~~~~ + +.. code-block:: text + + POST /api/v1/controller/device/{id}/activate/ + List Device Connections ~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index f51194af5..ddc3c84b5 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -19,7 +19,9 @@ from django.template.loader import get_template from django.template.response import TemplateResponse from django.urls import path, re_path, reverse +from django.utils.html import format_html, mark_safe from django.utils.translation import gettext_lazy as _ +from django.utils.translation import ngettext_lazy from flat_json_widget.widgets import FlatJsonWidget from import_export.admin import ImportExportMixin from openwisp_ipam.filters import SubnetFilter @@ -73,6 +75,30 @@ class BaseAdmin(TimeReadonlyAdminMixin, ModelAdmin): history_latest_first = True +class DeactivatedDeviceReadOnlyMixin(object): + def _has_permission(self, request, obj, perm): + if not obj or getattr(request, '_recover_view', False): + return perm + return perm and not obj.is_deactivated() + + def has_add_permission(self, request, obj): + perm = super().has_add_permission(request, obj) + return self._has_permission(request, obj, perm) + + def has_change_permission(self, request, obj=None): + perm = super().has_change_permission(request, obj) + return self._has_permission(request, obj, perm) + + def has_delete_permission(self, request, obj=None): + perm = super().has_delete_permission(request, obj) + return self._has_permission(request, obj, perm) + + def get_extra(self, request, obj=None, **kwargs): + if obj and obj.is_deactivated(): + return 0 + return super().get_extra(request, obj, **kwargs) + + class BaseConfigAdmin(BaseAdmin): change_form_template = 'admin/config/change_form.html' preview_template = None @@ -390,6 +416,7 @@ class Meta(BaseForm.Meta): class ConfigInline( + DeactivatedDeviceReadOnlyMixin, MultitenantAdminMixin, TimeReadonlyAdminMixin, SystemDefinedVariableMixin, @@ -452,6 +479,10 @@ def __init__(self, org_id, **kwargs): class DeviceAdmin(MultitenantAdminMixin, BaseConfigAdmin, UUIDAdmin): + change_form_template = 'admin/config/device/change_form.html' + delete_selected_confirmation_template = ( + 'admin/config/device/delete_selected_confirmation.html' + ) list_display = [ 'name', 'backend', @@ -499,7 +530,12 @@ class DeviceAdmin(MultitenantAdminMixin, BaseConfigAdmin, UUIDAdmin): ] inlines = [ConfigInline] conditional_inlines = [] - actions = ['change_group'] + actions = [ + 'change_group', + 'deactivate_device', + 'activate_device', + 'delete_selected', + ] org_position = 1 if not app_settings.HARDWARE_ID_ENABLED else 2 list_display.insert(org_position, 'organization') _state_adding = False @@ -520,6 +556,20 @@ class Media(BaseConfigAdmin.Media): f'{prefix}js/relevant_templates.js', ] + def has_change_permission(self, request, obj=None): + perm = super().has_change_permission(request) + if not obj or getattr(request, '_recover_view', False): + return perm + return perm and not obj.is_deactivated() + + def has_delete_permission(self, request, obj=None): + perm = super().has_delete_permission(request) + if not obj: + return perm + if obj._has_config(): + perm = perm and obj.config.is_deactivated() + return perm and obj.is_deactivated() + def save_form(self, request, form, change): self._state_adding = form.instance._state.adding return super().save_form(request, form, change) @@ -624,6 +674,114 @@ def change_group(self, request, queryset): request, 'admin/config/change_device_group.html', context ) + def _get_device_path(self, device): + app_label = self.opts.app_label + model_name = self.model._meta.model_name + return format_html( + '{}', + reverse( + f'admin:{app_label}_{model_name}_change', + args=[device.id], + ), + device, + ) + + _device_status_messages = { + 'deactivate': { + messages.SUCCESS: ngettext_lazy( + 'The device %(devices_html)s was deactivated successfully.', + ( + 'The following devices were deactivated successfully:' + ' %(devices_html)s.' + ), + 'devices', + ), + messages.ERROR: ngettext_lazy( + 'An error occurred while deactivating the device %(devices_html)s.', + ( + 'An error occurred while deactivating the following devices:' + ' %(devices_html)s.' + ), + 'devices', + ), + }, + 'activate': { + messages.SUCCESS: ngettext_lazy( + 'The device %(devices_html)s was activated successfully.', + 'The following devices were activated successfully: %(devices_html)s.', + 'devices', + ), + messages.ERROR: ngettext_lazy( + 'An error occurred while activating the device %(devices_html)s.', + ( + 'An error occurred while activating the following devices:' + ' %(devices_html)s.' + ), + 'devices', + ), + }, + } + + def _message_user_device_status(self, request, devices, method, message_level): + if not devices: + return + if len(devices) == 1: + devices_html = devices[0] + else: + devices_html = ', '.join(devices[:-1]) + ' and ' + devices[-1] + message = self._device_status_messages[method][message_level] + self.message_user( + request, + mark_safe( + message % {'devices_html': devices_html, 'devices': len(devices)} + ), + message_level, + ) + + def _change_device_status(self, request, queryset, method): + """ + This helper method provides re-usability of code for + device activation and deactivation actions. + """ + success_devices = [] + error_devices = [] + for device in queryset.iterator(): + try: + getattr(device, method)() + except Exception: + error_devices.append(self._get_device_path(device)) + else: + success_devices.append(self._get_device_path(device)) + self._message_user_device_status( + request, success_devices, method, messages.SUCCESS + ) + self._message_user_device_status(request, error_devices, method, messages.ERROR) + + @admin.action(description=_('Deactivate selected devices'), permissions=['change']) + def deactivate_device(self, request, queryset): + self._change_device_status(request, queryset, 'deactivate') + + @admin.action(description=_('Activate selected devices'), permissions=['change']) + def activate_device(self, request, queryset): + self._change_device_status(request, queryset, 'activate') + + def get_deleted_objects(self, objs, request, *args, **kwargs): + # Ensure that all selected devices can be deleted, i.e. + # the device should be flagged as deactivated and if it has + # a config object, it's status should be "deactivated". + active_devices = [] + for obj in objs: + if not self.has_delete_permission(request, obj): + active_devices.append(obj) + if active_devices: + return ( + active_devices, + {self.model._meta.verbose_name_plural: len(active_devices)}, + ['active_devices'], + [], + ) + return super().get_deleted_objects(objs, request, *args, **kwargs) + def get_fields(self, request, obj=None): """ Do not show readonly fields in add form @@ -642,7 +800,12 @@ def ip(self, obj): ip.short_description = _('IP address') def config_status(self, obj): - return obj.config.status + if obj._has_config(): + return obj.config.status + # The device does not have a related config object + if obj.is_deactivated(): + return _('deactivated') + return _('unknown') config_status.short_description = _('config status') @@ -687,6 +850,35 @@ def get_urls(self): def get_extra_context(self, pk=None): ctx = super().get_extra_context(pk) + if pk: + device = self.model.objects.select_related('config').get(id=pk) + ctx.update( + { + 'show_deactivate': not device.is_deactivated(), + 'show_activate': device.is_deactivated(), + 'action_checkbox_name': helpers.ACTION_CHECKBOX_NAME, + } + ) + if device.is_deactivated(): + ctx['additional_buttons'].append( + { + 'raw_html': mark_safe( + '' + ) + } + ) + else: + ctx['additional_buttons'].append( + { + 'raw_html': mark_safe( + '

' + '' + '

' + ) + } + ) ctx.update( { 'relevant_template_url': reverse( @@ -704,6 +896,10 @@ def add_view(self, request, form_url='', extra_context=None): extra_context = self.get_extra_context() return super().add_view(request, form_url, extra_context) + def recover_view(self, request, version_id, extra_context=None): + request._recover_view = True + return super().recover_view(request, version_id, extra_context) + def get_inlines(self, request, obj): inlines = super().get_inlines(request, obj) # this only makes sense in existing devices diff --git a/openwisp_controller/config/api/serializers.py b/openwisp_controller/config/api/serializers.py index 701dfb943..50033f2b2 100644 --- a/openwisp_controller/config/api/serializers.py +++ b/openwisp_controller/config/api/serializers.py @@ -249,6 +249,7 @@ class DeviceDetailConfigSerializer(BaseConfigSerializer): class DeviceDetailSerializer(DeviceConfigMixin, BaseSerializer): config = DeviceDetailConfigSerializer(allow_null=True) + is_deactivated = serializers.BooleanField(read_only=True) class Meta(BaseMeta): model = Device @@ -261,6 +262,7 @@ class Meta(BaseMeta): 'key', 'last_ip', 'management_ip', + 'is_deactivated', 'model', 'os', 'system', diff --git a/openwisp_controller/config/api/urls.py b/openwisp_controller/config/api/urls.py index 89020b7e6..ca5a1de16 100644 --- a/openwisp_controller/config/api/urls.py +++ b/openwisp_controller/config/api/urls.py @@ -53,6 +53,16 @@ def get_api_urls(api_views): api_views.device_detail, name='device_detail', ), + path( + 'controller/device//activate/', + api_views.device_activate, + name='device_activate', + ), + path( + 'controller/device//deactivate/', + api_views.device_deactivate, + name='device_deactivate', + ), path( 'controller/group/', api_views.devicegroup_list, diff --git a/openwisp_controller/config/api/views.py b/openwisp_controller/config/api/views.py index dd06aaf72..c56f1437c 100644 --- a/openwisp_controller/config/api/views.py +++ b/openwisp_controller/config/api/views.py @@ -4,14 +4,18 @@ from django.http import Http404 from django.urls.base import reverse from django_filters.rest_framework import DjangoFilterBackend -from rest_framework import pagination +from rest_framework import pagination, serializers, status from rest_framework.generics import ( + GenericAPIView, ListCreateAPIView, RetrieveAPIView, RetrieveUpdateDestroyAPIView, ) +from rest_framework.response import Response from swapper import load_model +from openwisp_users.api.permissions import DjangoModelPermissions + from ...mixins import ProtectedAPIMixin from .filters import ( DeviceGroupListFilter, @@ -70,6 +74,14 @@ class VpnDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView): queryset = Vpn.objects.all() +class DevicePermission(DjangoModelPermissions): + def has_object_permission(self, request, view, obj): + perm = super().has_object_permission(request, view, obj) + if request.method not in ['PUT', 'PATCH']: + return perm + return perm and not obj.is_deactivated() + + class DeviceListCreateView(ProtectedAPIMixin, ListCreateAPIView): """ Templates: Templates flagged as required will be added automatically @@ -93,6 +105,33 @@ class DeviceDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView): serializer_class = DeviceDetailSerializer queryset = Device.objects.select_related('config', 'group', 'organization') + permission_classes = ProtectedAPIMixin.permission_classes + (DevicePermission,) + + +class DeviceActivateView(ProtectedAPIMixin, GenericAPIView): + serializer_class = serializers.Serializer + queryset = Device.objects.filter(_is_deactivated=True) + + def post(self, request, *args, **kwargs): + device = self.get_object() + device.activate() + serializer = DeviceDetailSerializer( + device, context=self.get_serializer_context() + ) + return Response(serializer.data, status=status.HTTP_200_OK) + + +class DeviceDeactivateView(ProtectedAPIMixin, GenericAPIView): + serializer_class = serializers.Serializer + queryset = Device.objects.filter(_is_deactivated=False) + + def post(self, request, *args, **kwargs): + device = self.get_object() + device.deactivate() + serializer = DeviceDetailSerializer( + device, context=self.get_serializer_context() + ) + return Response(serializer.data, status=status.HTTP_200_OK) class DeviceGroupListCreateView(ProtectedAPIMixin, ListCreateAPIView): @@ -240,6 +279,8 @@ def certificate_delete_invalidates_cache(cls, organization_id, common_name): vpn_detail = VpnDetailView.as_view() device_list = DeviceListCreateView.as_view() device_detail = DeviceDetailView.as_view() +device_activate = DeviceActivateView.as_view() +device_deactivate = DeviceDeactivateView.as_view() devicegroup_list = DeviceGroupListCreateView.as_view() devicegroup_detail = DeviceGroupDetailView.as_view() devicegroup_commonname = DeviceGroupCommonName.as_view() diff --git a/openwisp_controller/config/apps.py b/openwisp_controller/config/apps.py index 846725c2e..84dbc3ba3 100644 --- a/openwisp_controller/config/apps.py +++ b/openwisp_controller/config/apps.py @@ -22,6 +22,8 @@ from . import settings as app_settings from .signals import ( config_backend_changed, + config_deactivated, + config_deactivating, config_modified, device_group_changed, device_name_changed, @@ -305,6 +307,18 @@ def enable_cache_invalidation(self): sender=self.device_model, dispatch_uid='invalidate_get_device_cache', ) + config_deactivated.connect( + self.device_model.config_deactivated_clear_management_ip, + dispatch_uid='config_deactivated_clear_management_ip', + ) + config_deactivated.connect( + DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated, + dispatch_uid='config_deactivated_invalidate_get_device_cache', + ) + config_deactivating.connect( + DeviceChecksumView.invalidate_checksum_cache, + dispatch_uid='config_deactivated_invalidate_get_device_cache', + ) config_modified.connect( DeviceChecksumView.invalidate_checksum_cache, dispatch_uid='invalidate_checksum_cache', @@ -359,11 +373,15 @@ def register_dashboard_charts(self): 'applied': '#267126', 'modified': '#ffb442', 'error': '#a72d1d', + 'deactivating': '#353c44', + 'deactivated': '#000', }, 'labels': { 'applied': _('applied'), 'modified': _('modified'), 'error': _('error'), + 'deactivating': _('deactivating'), + 'deactivated': _('deactivated'), }, }, ) diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 181cc566a..1aa5e6382 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -14,7 +14,13 @@ from swapper import get_model_name, load_model from .. import settings as app_settings -from ..signals import config_backend_changed, config_modified, config_status_changed +from ..signals import ( + config_backend_changed, + config_deactivated, + config_deactivating, + config_modified, + config_status_changed, +) from ..sortedm2m.fields import SortedManyToManyField from ..utils import get_default_templates_queryset from .base import BaseConfig @@ -62,13 +68,16 @@ class AbstractConfig(BaseConfig): blank=True, ) - STATUS = Choices('modified', 'applied', 'error') + STATUS = Choices('modified', 'applied', 'error', 'deactivating', 'deactivated') status = StatusField( _('configuration status'), help_text=_( '"modified" means the configuration is not applied yet; \n' '"applied" means the configuration is applied successfully; \n' - '"error" means the configuration caused issues and it was rolled back;' + '"error" means the configuration caused issues and it was rolled back; \n' + '"deactivating" means the device has been deactivated and the' + ' configuration is being removed; \n' + '"deactivated" means the configuration has been removed from the device;' ), ) error_reason = models.CharField( @@ -105,6 +114,8 @@ def __init__(self, *args, **kwargs): self._just_created = False self._initial_status = self.status self._send_config_modified_after_save = False + self._send_config_deactivated = False + self._send_config_deactivating = False self._send_config_status_changed = False def __str__(self): @@ -236,9 +247,20 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs): This method is called from a django signal (m2m_changed) see config.apps.ConfigConfig.connect_signals """ - # execute only after a config has been saved or deleted - if action not in ['post_add', 'post_remove'] or instance._state.adding: + if instance._state.adding or action not in [ + 'post_add', + 'post_remove', + 'post_clear', + ]: + return + + if action == 'post_clear': + if instance.is_deactivating_or_deactivated(): + # If the device is deactivated or in the process of deactivatiing, then + # delete all vpn clients and return. + instance.vpnclient_set.all().delete() return + vpn_client_model = cls.vpn.through # coming from signal if isinstance(pk_set, set): @@ -331,6 +353,8 @@ def enforce_required_templates( """ if action not in ['pre_remove', 'post_clear']: return False + if instance.is_deactivating_or_deactivated(): + return raw_data = raw_data or {} template_query = models.Q(required=True, backend=instance.backend) # trying to remove a required template will raise PermissionDenied @@ -467,9 +491,7 @@ def save(self, *args, **kwargs): result = super().save(*args, **kwargs) # add default templates if config has just been created if created: - default_templates = self.get_default_templates() - if default_templates: - self.templates.add(*default_templates) + self.add_default_templates() if self._old_backend and self._old_backend != self.backend: self._send_config_backend_changed_signal() self._old_backend = None @@ -480,9 +502,27 @@ def save(self, *args, **kwargs): if self._send_config_status_changed: self._send_config_status_changed_signal() self._send_config_status_changed = False + if self._send_config_deactivating and self.is_deactivating(): + self._send_config_deactivating_signal() + if self._send_config_deactivated and self.is_deactivated(): + self._send_config_deactivated_signal() self._initial_status = self.status return result + def add_default_templates(self): + default_templates = self.get_default_templates() + if default_templates: + self.templates.add(*default_templates) + + def is_deactivating_or_deactivated(self): + return self.status in ['deactivating', 'deactivated'] + + def is_deactivating(self): + return self.status == 'deactivating' + + def is_deactivated(self): + return self.status == 'deactivated' + def _check_changes(self): current = self._meta.model.objects.only( 'backend', 'config', 'context', 'status' @@ -520,6 +560,27 @@ def _send_config_modified_signal(self, action): device=self.device, ) + def _send_config_deactivating_signal(self): + """ + Emits ``config_deactivating`` signal. + """ + config_deactivating.send( + sender=self.__class__, + instance=self, + device=self.device, + previous_status=self._initial_status, + ) + + def _send_config_deactivated_signal(self): + """ + Emits ``config_deactivated`` signal. + """ + config_deactivated.send( + sender=self.__class__, + instance=self, + previous_status=self._initial_status, + ) + def _send_config_backend_changed_signal(self): """ Emits ``config_backend_changed`` signal. @@ -539,9 +600,10 @@ def _send_config_status_changed_signal(self): """ config_status_changed.send(sender=self.__class__, instance=self) - def _set_status(self, status, save=True, reason=None): + def _set_status(self, status, save=True, reason=None, extra_update_fields=None): self._send_config_status_changed = True - update_fields = ['status'] + extra_update_fields = extra_update_fields or [] + update_fields = ['status'] + extra_update_fields # The error reason should be updated when # 1. the configuration is in "error" status # 2. the configuration has changed from error status @@ -563,6 +625,58 @@ def set_status_applied(self, save=True): def set_status_error(self, save=True, reason=None): self._set_status('error', save, reason) + def set_status_deactivating(self, save=True): + """ + Set Config status as deactivating and + clears configuration and templates. + """ + self._send_config_deactivating = True + self._set_status('deactivating', save, extra_update_fields=['config']) + + def set_status_deactivated(self, save=True): + self._send_config_deactivated = True + self._set_status('deactivated', save) + + def deactivate(self): + """ + Clears configuration and templates and set status as deactivating. + """ + # Invalidate cached property before checking checksum. + self._invalidate_backend_instance_cache() + old_checksum = self.checksum + self.config = {} + self.set_status_deactivating() + self.templates.clear() + del self.backend_instance + if old_checksum == self.checksum: + # Accelerate deactivation if the configuration remains + # unchanged (i.e. empty configuration) + self.set_status_deactivated() + + def activate(self): + """ + Applies required, default and group templates when device is activated. + """ + # Invalidate cached property before checking checksum. + 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, + ) + del self.backend_instance + if old_checksum == self.checksum: + # Accelerate activation if the configuration remains + # unchanged (i.e. empty configuration) + self.set_status_applied() + + def _invalidate_backend_instance_cache(self): + if hasattr(self, 'backend_instance'): + del self.backend_instance + def _has_device(self): return hasattr(self, 'device') diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py index f849987bc..e3892a08b 100644 --- a/openwisp_controller/config/base/device.py +++ b/openwisp_controller/config/base/device.py @@ -1,7 +1,7 @@ from hashlib import md5 -from django.core.exceptions import ObjectDoesNotExist, ValidationError -from django.db import models +from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError +from django.db import models, transaction from django.db.models import Q from django.utils.translation import gettext_lazy as _ from swapper import get_model_name, load_model @@ -10,7 +10,13 @@ from openwisp_utils.base import KeyField from .. import settings as app_settings -from ..signals import device_group_changed, device_name_changed, management_ip_changed +from ..signals import ( + device_activated, + device_deactivated, + device_group_changed, + device_name_changed, + management_ip_changed, +) from ..validators import device_name_validator, mac_address_validator from .base import BaseModel @@ -96,6 +102,10 @@ class AbstractDevice(OrgMixin, BaseModel): ), ) hardware_id = models.CharField(**(app_settings.HARDWARE_ID_OPTIONS)) + # This is an internal field which is used to track if + # the device has been deactivated. This field should not be changed + # directly, use the deactivate() method instead. + _is_deactivated = models.BooleanField(default=False) class Meta: unique_together = ( @@ -163,6 +173,35 @@ def _get_organization__config_settings(self): organization=self.organization if hasattr(self, 'organization') else None ) + def is_deactivated(self): + return self._is_deactivated + + def deactivate(self): + if self.is_deactivated(): + # The device has already been deactivated. + # No further operation is required. + return + with transaction.atomic(): + if self._has_config(): + self.config.deactivate() + else: + self.management_ip = '' + self._is_deactivated = True + self.save() + device_deactivated.send(sender=self.__class__, instance=self) + + def activate(self): + if not self.is_deactivated(): + # The device is already active. + # No further operation is required. + return + with transaction.atomic(): + if self._has_config(): + self.config.activate() + self._is_deactivated = False + self.save() + device_activated.send(sender=self.__class__, instance=self) + def get_context(self): config = self._get_config() return config.get_context() @@ -247,6 +286,14 @@ def save(self, *args, **kwargs): if not state_adding: 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()) + ): + raise PermissionDenied('The device must be deactivated prior to deletion') + return super().delete(using, keep_parents) + def _check_changed_fields(self): self._get_initial_values_for_checked_fields() # Execute method for checked for each field in self._changed_checked_fields @@ -434,3 +481,11 @@ def manage_devices_group_templates(cls, device_ids, old_group_ids, group_id): old_group = DeviceGroup.objects.get(pk=old_group_id) old_group_templates = old_group.templates.all() device.config.manage_group_templates(group_templates, old_group_templates) + + @classmethod + def config_deactivated_clear_management_ip(cls, instance, *args, **kwargs): + """ + Clear management IP of the device when the device's config status + is changed to 'deactivated'. + """ + cls.objects.filter(pk=instance.device_id).update(management_ip='') diff --git a/openwisp_controller/config/controller/views.py b/openwisp_controller/config/controller/views.py index dfbcc40ec..9059d1141 100644 --- a/openwisp_controller/config/controller/views.py +++ b/openwisp_controller/config/controller/views.py @@ -54,8 +54,10 @@ def get_object(self, *args, **kwargs): 'organization__created', 'organization__modified', ) - queryset = self.model.objects.select_related('organization', 'config').defer( - *defer + queryset = ( + self.model.objects.select_related('organization', 'config') + .defer(*defer) + .exclude(config__status='deactivated') ) return get_object_or_404(queryset, *args, **kwargs) @@ -168,6 +170,14 @@ def invalidate_get_device_cache(cls, instance, **kwargs): view.get_device.invalidate(view) logger.debug(f'invalidated view cache for device ID {pk}') + @classmethod + def invalidate_get_device_cache_on_config_deactivated(cls, instance, **kwargs): + """ + Called from signal receiver which performs cache invalidation + when the configuration status is set to "deactivated". + """ + cls.invalidate_get_device_cache(instance=instance.device, **kwargs) + @classmethod def invalidate_checksum_cache(cls, instance, device, **kwargs): """ @@ -247,6 +257,11 @@ def post(self, request, *args, **kwargs): # mantain backward compatibility with old agents # ("running" was changed to "applied") status = status if status != 'running' else 'applied' + # If the Config.status is "deactivating", then set the + # status to "deactivated". This will stop the device + # from receiving new configurations. + if config.status == 'deactivating': + status = 'deactivated' # call set_status_{status} method on Config model method_name = f'set_status_{status}' if status == 'error': diff --git a/openwisp_controller/config/migrations/0054_device__is_deactivated.py b/openwisp_controller/config/migrations/0054_device__is_deactivated.py new file mode 100644 index 000000000..3cb2739f6 --- /dev/null +++ b/openwisp_controller/config/migrations/0054_device__is_deactivated.py @@ -0,0 +1,17 @@ +# Generated by Django 3.2.20 on 2024-02-29 11:56 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('config', '0053_vpnclient_secret'), + ] + + operations = [ + migrations.AddField( + model_name='device', + name='_is_deactivated', + field=models.BooleanField(default=False), + ), + ] diff --git a/openwisp_controller/config/migrations/0055_alter_config_status.py b/openwisp_controller/config/migrations/0055_alter_config_status.py new file mode 100644 index 000000000..c9ac1c600 --- /dev/null +++ b/openwisp_controller/config/migrations/0055_alter_config_status.py @@ -0,0 +1,39 @@ +# Generated by Django 4.2.10 on 2024-03-01 16:35 + +import model_utils.fields +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("config", "0054_device__is_deactivated"), + ] + + operations = [ + migrations.AlterField( + model_name="config", + name="status", + field=model_utils.fields.StatusField( + choices=[ + ("modified", "modified"), + ("applied", "applied"), + ("error", "error"), + ("deactivating", "deactivating"), + ("deactivated", "deactivated"), + ], + default="modified", + help_text=( + '"modified" means the configuration is not applied yet; \n' + '"applied" means the configuration is applied successfully; \n' + '"error" means the configuration caused issues and it was' + ' rolled back; \n"deactivating" means the device has been' + ' deactivated and the configuration is being removed; \n' + '"deactivated" means the configuration has been removed ' + 'from the device;' + ), + max_length=100, + no_check_for_status=True, + verbose_name="configuration status", + ), + ), + ] diff --git a/openwisp_controller/config/signals.py b/openwisp_controller/config/signals.py index 57a661d76..325cfb628 100644 --- a/openwisp_controller/config/signals.py +++ b/openwisp_controller/config/signals.py @@ -17,10 +17,26 @@ config_modified.__doc__ = """ Providing arguments: ['instance', 'device', 'config', 'previous_status', 'action'] """ +config_deactivated = Signal() +config_deactivated.__doc__ = """ +Providing arguments: ['instance', 'previous_status'] +""" +config_deactivating = Signal() +config_deactivating.__doc__ = """ +Providing arguments: ['instance', 'previous_status'] +""" device_registered = Signal() device_registered.__doc__ = """ Providing arguments: ['instance', 'is_new'] """ +device_deactivated = Signal() +device_deactivated.__doc__ = """ +Providing arguments: ['instance'] +""" +device_activated = Signal() +device_activated.__doc__ = """ +Providing arguments: ['instance'] +""" management_ip_changed = Signal() management_ip_changed.__doc__ = """ Providing arguments: ['instance', 'management_ip', 'old_management_ip'] diff --git a/openwisp_controller/config/templates/admin/config/device/change_form.html b/openwisp_controller/config/templates/admin/config/device/change_form.html new file mode 100644 index 000000000..e97999f75 --- /dev/null +++ b/openwisp_controller/config/templates/admin/config/device/change_form.html @@ -0,0 +1,34 @@ +{% extends "admin/config/change_form.html" %} +{% load admin_urls i18n l10n %} + +{% block messages %} + {{ block.super }} + {% if original and original.is_deactivated %} +
    +
  • {% trans "This device has been deactivated." %}
    • +
+ {% endif %} +{% endblock messages %} + +{% block content %} + {% comment %} + Due to HTML's limitation in supporting nested forms, we employ a + workaround for activating and deactivating device operations within + the change form. + + We utilize a distinct form element (id="act_deact_device_form") + specifically for these actions. The form attribute of the submit buttons (Acivate/Deactivate) + within the submit-row div references this form. By doing so, we ensure that + these actions can be submitted independently without causing any + disruption to the device form. + + For further information, refer to: https://www.impressivewebs.com/html5-form-attribute/ + {% endcomment %} + {% url opts|admin_urlname:'changelist' as changelist_url %} +
+ {% csrf_token %} + + +
+ {{ block.super }} +{% endblock content %} diff --git a/openwisp_controller/config/templates/admin/config/device/delete_selected_confirmation.html b/openwisp_controller/config/templates/admin/config/device/delete_selected_confirmation.html new file mode 100644 index 000000000..a66130f86 --- /dev/null +++ b/openwisp_controller/config/templates/admin/config/device/delete_selected_confirmation.html @@ -0,0 +1,36 @@ +{% extends "admin/delete_selected_confirmation.html" %} +{% load i18n l10n admin_urls static %} + +{% block content %} +{% if perms_lacking %} + {% if perms_lacking|first == 'active_devices' %} +

{% blocktranslate %}You have selected the following active device{{ model_count | pluralize }} to delete:{% endblocktranslate %}

+
    {{ deletable_objects|first|unordered_list }}
+

{% blocktrans %}It is required to flag the device as deactivated before deleting the device. If the device has configuration, then wait till the configuration status changes to "deactivated" before deleting the device.{% endblocktrans %}

+ {% else %} +

{% blocktranslate %}Deleting the selected {{ objects_name }} would result in deleting related objects, but your account doesn't have permission to delete the following types of objects:{% endblocktranslate %}

+
    {{ perms_lacking|unordered_list }}
+ {% endif %} +{% elif protected %} +

{% blocktranslate %}Deleting the selected {{ objects_name }} would require deleting the following protected related objects:{% endblocktranslate %}

+
    {{ protected|unordered_list }}
+{% else %} +

{% blocktranslate %}Are you sure you want to delete the selected {{ objects_name }}? All of the following objects and their related items will be deleted:{% endblocktranslate %}

+ {% include "admin/includes/object_delete_summary.html" %} +

{% translate "Objects" %}

+ {% for deletable_object in deletable_objects %} +
    {{ deletable_object|unordered_list }}
+ {% endfor %} +
{% csrf_token %} +
+ {% for obj in queryset %} + + {% endfor %} + + + + {% translate "No, take me back" %} +
+
+{% endif %} +{% endblock %} diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 737a5e8ad..f2da37829 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -8,6 +8,7 @@ from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from django.core.files.base import ContentFile +from django.db import IntegrityError from django.db.models.signals import post_save from django.test import TestCase, TransactionTestCase from django.urls import reverse @@ -1163,6 +1164,15 @@ def test_download_device_config(self): self.assertEqual(response.status_code, 200) self.assertEqual(response.get('content-type'), 'application/octet-stream') + def test_download_deactivated_device_config(self): + device = self._create_device(name='download') + self._create_config(device=device) + device.deactivate() + path = reverse(f'admin:{self.app_label}_device_download', args=[device.pk.hex]) + response = self.client.get(path) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.get('content-type'), 'application/octet-stream') + def test_download_device_config_404(self): d = self._create_device(name='download') path = reverse(f'admin:{self.app_label}_device_download', args=[d.pk]) @@ -1467,6 +1477,41 @@ def test_device_search(self): response = self.client.get(path, {'q': 'Estonia'}) self.assertContains(response, 'admin-search-test') + def test_device_changelist_config_status(self): + device = self._create_device() + path = reverse(f'admin:{self.app_label}_device_changelist') + expected_html = '{expected_status}' + with self.subTest('Test device does not have a config object'): + response = self.client.get(path) + self.assertContains( + response, expected_html.format(expected_status='unknown') + ) + # Device without config is deactivated + device.deactivate() + response = self.client.get(path) + self.assertContains( + response, expected_html.format(expected_status='deactivated') + ) + + device.activate() + self._create_template(required=True) + self._create_config(device=device) + with self.subTest('Test device has config object'): + response = self.client.get(path) + self.assertContains( + response, expected_html.format(expected_status='modified') + ) + device.config.deactivate() + response = self.client.get(path) + self.assertContains( + response, expected_html.format(expected_status='deactivating') + ) + device.config.set_status_deactivated() + response = self.client.get(path) + self.assertContains( + response, expected_html.format(expected_status='deactivated') + ) + def test_default_template_backend(self): path = reverse(f'admin:{self.app_label}_template_add') response = self.client.get(path) @@ -1950,6 +1995,351 @@ def tearDownClass(cls): super().tearDownClass() devnull.close() + @patch.object(Device, 'deactivate') + def test_device_changelist_activate_deactivate_admin_action_security( + self, mocked_deactivate + ): + org1 = self._get_org() + org2 = self._create_org(name='org2') + org1_device = self._create_device(organization=org1) + org2_device = self._create_device(organization=org2) + path = reverse(f'admin:{self.app_label}_device_changelist') + + with self.subTest('Test superuser deactivates different org devices'): + self.client.post( + path, + { + 'action': 'deactivate_device', + '_selected_action': [str(org1_device.pk), str(org2_device.pk)], + }, + follow=True, + ) + self.assertEqual(mocked_deactivate.call_count, 2) + + mocked_deactivate.reset_mock() + with self.subTest('Test user deactivates device of unmanaged org'): + # The device changelist page is filtered with the devices of + # the organizations managed by the user. The selected device + # pks are also filtered from this queryset before executing the + # deactivate action. Therefore, no operation is performed on + # the devices of other organization. + administrator = self._create_administrator(organizations=[org1]) + self.client.force_login(administrator) + self.client.post( + path, + { + 'action': 'deactivate_device', + '_selected_action': [str(org2_device.pk)], + }, + follow=True, + ) + self.assertEqual(mocked_deactivate.call_count, 0) + + mocked_deactivate.reset_mock() + with self.subTest('Test user deactivates device of managed org'): + self.client.post( + path, + { + 'action': 'deactivate_device', + '_selected_action': [str(org1_device.pk)], + }, + follow=True, + ) + self.assertEqual(mocked_deactivate.call_count, 1) + + +class TestTransactionAdmin( + CreateConfigTemplateMixin, + TestAdminMixin, + TransactionTestCase, +): + app_label = 'config' + _deactivated_device_warning = ( + '
  • This device has been deactivated.
    • ' + ) + _deactivate_btn_html = ( + '

    ' + ) + _activate_btn_html = ( + '' + ) + _save_btn_html = '' + + def setUp(self): + self.client.force_login(self._get_admin()) + + def _get_delete_btn_html(self, device): + return ( + '

    ' + 'Delete

    ' + ) + + def test_device_with_config_change_deactivate_deactivate(self): + """ + This test checks the following things + - deactivate button is shown on device's change page + - all the fields become readonly on deactivated device + - deleting a device is possible once device's config.status is deactivated + - activate button is shown on deactivated device + """ + self._create_template(required=True) + device = self._create_config(organization=self._get_org()).device + path = reverse(f'admin:{self.app_label}_device_change', args=[device.pk]) + delete_btn_html = self._get_delete_btn_html(device) + # Deactivate button is shown instead of delete button + response = self.client.get(path) + self.assertEqual(response.status_code, 200) + self.assertContains( + response, + self._deactivate_btn_html, + ) + # Verify the inline objects can be added and deleted + self.assertContains(response, 'TOTAL_FORMS" value="1"', count=3) + self.assertContains(response, '', + 22, + ) + # Save buttons are absent on deactivated device + self.assertNotContains(response, self._save_btn_html) + # Delete button is not present if config status is deactivating + self.assertEqual(device.config.status, 'deactivating') + self.assertNotContains(response, delete_btn_html) + self.assertNotContains(response, self._deactivate_btn_html) + self.assertContains(response, self._activate_btn_html) + # Verify adding a new DeviceLocation and DeviceConnection is not allowed + self.assertContains(response, '-TOTAL_FORMS" value="0"', count=2) + # Verify deleting existing Inline objects is not allowed + self.assertNotContains(response, 'The device {device_name}' + f' was {html_method}ed successfully.' + ) + multiple_success_message_html = ( + f'
  • The following devices were {html_method}ed' + f' successfully: ' + f'{device1.name}, ' + f'{device2.name} and {device3.name}.
    • ' + ) + single_error_message_html = ( + f'
  • An error occurred while {html_method}ing the device' + f' {device_name}.
    • ' + ) + multiple_error_message_html = ( + f'
  • An error occurred while {html_method}ing the following' + f' devices: ' + f'{device1.name}, {device2.name} and {device3.name}.
    • ' + ) + path = reverse(f'admin:{self.app_label}_device_changelist') + + with self.subTest(f'Test {method}ing a single device'): + response = self.client.post( + path, + { + 'action': f'{method}_device', + '_selected_action': str(device1.pk), + }, + follow=True, + ) + self.assertEqual(response.status_code, 200) + for device in [device1, device2, device3]: + device.refresh_from_db(fields=['_is_deactivated']) + self.assertEqual(device1.is_deactivated(), not is_initially_deactivated) + self.assertEqual(device2.is_deactivated(), is_initially_deactivated) + self.assertEqual(device3.is_deactivated(), is_initially_deactivated) + self.assertContains( + response, + single_success_message_html.format( + device_id=device1.id, + device_name=device1.name, + ), + ) + + with self.subTest(f'Test {html_method}ing multiple devices'): + response = self.client.post( + path, + { + 'action': f'{method}_device', + '_selected_action': [ + str(device1.pk), + str(device2.pk), + str(device3.pk), + ], + }, + follow=True, + ) + self.assertEqual(response.status_code, 200) + for device in [device1, device2, device3]: + device.refresh_from_db(fields=['_is_deactivated']) + self.assertEqual(device.is_deactivated(), not is_initially_deactivated) + self.assertContains(response, multiple_success_message_html) + + with self.subTest(f'Test error occurred {html_method}ing a single device'): + with patch.object(Device, method, side_effect=IntegrityError): + response = self.client.post( + path, + { + 'action': f'{method}_device', + '_selected_action': str(device1.pk), + }, + follow=True, + ) + self.assertEqual(response.status_code, 200) + self.assertContains( + response, + single_error_message_html.format( + device_id=device1.id, device_name=device1.name + ), + ) + + with self.subTest(f'Test error occurred {html_method}ing multiple devices'): + with patch.object(Device, method, side_effect=IntegrityError): + response = self.client.post( + path, + { + 'action': f'{method}_device', + '_selected_action': [ + str(device1.pk), + str(device2.pk), + str(device3.pk), + ], + }, + follow=True, + ) + self.assertEqual(response.status_code, 200) + self.assertContains(response, multiple_error_message_html) + + with self.subTest('Test mix of error and success operations'): + with patch.object(Device, method, side_effect=[None, IntegrityError]): + response = self.client.post( + path, + { + 'action': f'{method}_device', + '_selected_action': [str(device1.pk), str(device2.pk)], + }, + follow=True, + ) + self.assertEqual(response.status_code, 200) + self.assertContains( + response, + single_success_message_html.format( + device_name=device1.name, device_id=device1.id + ), + ) + self.assertContains( + response, + single_error_message_html.format( + device_name=device2.name, device_id=device2.id + ), + ) + + def test_device_changelist_activate_admin_action(self): + """ + This test verifies that activate admin action works as expected. + It also asserts for the success and error messages. + """ + self._test_device_changelist_activate_deactivate_admin_action( + method='activate', + is_initially_deactivated=True, + ) + + def test_device_changelist_deactivate_admin_action(self): + """ + This test verifies that deactivate admin action works as expected. + It also asserts for the success and error messages. + """ + self._test_device_changelist_activate_deactivate_admin_action( + method='deactivate', + is_initially_deactivated=False, + ) + class TestDeviceGroupAdmin( CreateDeviceGroupMixin, diff --git a/openwisp_controller/config/tests/test_api.py b/openwisp_controller/config/tests/test_api.py index 2327175be..13ae381b4 100644 --- a/openwisp_controller/config/tests/test_api.py +++ b/openwisp_controller/config/tests/test_api.py @@ -482,13 +482,62 @@ def test_device_download_api(self): r = self.client.get(path) self.assertEqual(r.status_code, 200) + def test_update_deactivated_device(self): + device = self._create_device(_is_deactivated=True) + path = reverse('config_api:device_detail', args=[device.pk]) + data = {'notes': 'test'} + response = self.client.patch(path, data, content_type='application/json') + self.assertEqual(response.status_code, 403) + response = self.client.put(path, data, content_type='application/json') + self.assertEqual(response.status_code, 403) + + def test_device_deactivate_api(self): + device = self._create_device() + path = reverse('config_api:device_deactivate', args=[device.pk]) + response = self.client.post(path) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data['is_deactivated'], True) + device.refresh_from_db() + self.assertEqual(device.is_deactivated(), True) + + def test_device_activate_api(self): + device = self._create_device(_is_deactivated=True) + path = reverse('config_api:device_activate', args=[device.pk]) + response = self.client.post(path) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data['is_deactivated'], False) + device.refresh_from_db() + self.assertEqual(device.is_deactivated(), False) + def test_device_delete_api(self): - d1 = self._create_device() - self._create_config(device=d1) - path = reverse('config_api:device_detail', args=[d1.pk]) - r = self.client.delete(path) - self.assertEqual(r.status_code, 204) - self.assertEqual(Device.objects.count(), 0) + self._create_template(required=True) + device = self._create_device() + config = self._create_config(device=device) + path = reverse('config_api:device_detail', args=[device.pk]) + + with self.subTest('Test deleting device without deactivating'): + response = self.client.delete(path) + self.assertEqual(response.status_code, 403) + + device.deactivate() + device.refresh_from_db() + config.refresh_from_db() + + with self.subTest('Test deleting device with config in deactivating state'): + self.assertEqual(device.is_deactivated(), True) + self.assertEqual(config.is_deactivating(), True) + response = self.client.delete(path) + self.assertEqual(response.status_code, 403) + + config.set_status_deactivated() + config.refresh_from_db() + + with self.subTest('Test deleting device with config in deactivated state'): + self.assertEqual(device.is_deactivated(), True) + self.assertEqual(config.is_deactivated(), True) + response = self.client.delete(path) + self.assertEqual(response.status_code, 204) + self.assertEqual(Device.objects.count(), 0) def test_template_create_no_org_api(self): self.assertEqual(Template.objects.count(), 0) diff --git a/openwisp_controller/config/tests/test_apps.py b/openwisp_controller/config/tests/test_apps.py index 88a8eff14..eb1f6ed8d 100644 --- a/openwisp_controller/config/tests/test_apps.py +++ b/openwisp_controller/config/tests/test_apps.py @@ -12,8 +12,20 @@ def test_config_status_chart_registered(self): 'model': 'device', 'group_by': 'config__status', }, - 'colors': {'applied': '#267126', 'modified': '#ffb442', 'error': '#a72d1d'}, - 'labels': {'applied': 'applied', 'error': 'error', 'modified': 'modified'}, + 'colors': { + 'applied': '#267126', + 'modified': '#ffb442', + 'error': '#a72d1d', + 'deactivating': '#353c44', + 'deactivated': '#000', + }, + 'labels': { + 'applied': 'applied', + 'error': 'error', + 'modified': 'modified', + 'deactivating': 'deactivating', + 'deactivated': 'deactivated', + }, } chart_config = DASHBOARD_CHARTS.get(1, None) self.assertIsNotNone(chart_config) diff --git a/openwisp_controller/config/tests/test_config.py b/openwisp_controller/config/tests/test_config.py index 1f9059649..d01ac66b3 100644 --- a/openwisp_controller/config/tests/test_config.py +++ b/openwisp_controller/config/tests/test_config.py @@ -519,6 +519,22 @@ def test_cert_not_deleted_on_config_change(self): self.assertEqual(c.vpnclient_set.count(), 1) self.assertEqual(c.vpnclient_set.first(), vpnclient) + def test_auto_cert_not_deleted_on_device_deactivation(self): + self._create_template(type='vpn', vpn=self._create_vpn(), default=True) + config = self._create_config(organization=self._get_org()) + self.assertEqual(config.templates.count(), 1) + cert = config.vpnclient_set.first().cert + self.assertEqual(cert.revoked, False) + + config.deactivate() + config.refresh_from_db() + # Since it is possible to refresh the cert object from the + # database, it means that the cert object is not deleted. + cert.refresh_from_db() + self.assertEqual(config.status, 'deactivating') + self.assertEqual(config.templates.count(), 0) + self.assertEqual(cert.revoked, True) + def _get_vpn_context(self): self.test_create_cert() c = Config.objects.get(device__name='test-create-cert') @@ -665,7 +681,7 @@ def test_remove_duplicate_files(self): else: self.assertIn('# path: /etc/vpnserver1', result) - config.device.delete() + config.device.delete(check_deactivated=False) config.delete() with self.subTest('Test template applied after creating config object'): config = self._create_config(organization=org) diff --git a/openwisp_controller/config/tests/test_controller.py b/openwisp_controller/config/tests/test_controller.py index 05443e5f3..06d9f0a2d 100644 --- a/openwisp_controller/config/tests/test_controller.py +++ b/openwisp_controller/config/tests/test_controller.py @@ -71,6 +71,35 @@ def _test_view_organization_disabled( response = method(url, {'key': obj.key}) self.assertEqual(response.status_code, 404) + def _test_deactivating_deactivated_device_view( + self, url_name, method='get', data=None + ): + data = data or {} + device = self._create_device_config() + config = device.config + # The endpoint returns 200 when config.status is modified + config.set_status_modified() + path = reverse(f'controller:{url_name}', args=[device.pk]) + payload = {'key': device.key, **data} + response = getattr(self.client, method)(path, payload) + self.assertEqual(response.status_code, 200) + + # The endpoint returns 200 when config.status is deactivating + config.set_status_deactivating() + path = reverse('controller:device_checksum', args=[device.pk]) + response = self.client.get(path, {'key': device.key}) + self.assertEqual(response.status_code, 200) + config.refresh_from_db() + self.assertEqual(config.status, 'deactivating') + + # The endpoint returns 404 when config.status is deactivated + config.set_status_deactivated() + path = reverse('controller:device_checksum', args=[device.pk]) + response = self.client.get(path, {'key': device.key}) + self.assertEqual(response.status_code, 404) + config.refresh_from_db() + self.assertEqual(config.status, 'deactivated') + def test_device_checksum(self): d = self._create_device_config() c = d.config @@ -137,7 +166,7 @@ def test_device_get_object_cached(self): self.assertEqual(obj.os, 'test_cache') with self.subTest('test cache invalidation on device delete'): - d.delete() + d.delete(check_deactivated=False) with self.assertNumQueries(1): with self.assertRaises(Http404): view.get_device() @@ -245,6 +274,9 @@ def test_device_config_download_requested_signal_is_emitted(self): request=response.wsgi_request, ) + def test_device_config_deactivated_checksum(self): + self._test_deactivating_deactivated_device_view('device_checksum') + @capture_any_output() def test_device_checksum_400(self): d = self._create_device_config() @@ -285,6 +317,9 @@ def test_device_download_config(self): self.assertIsNotNone(d.last_ip) self.assertIsNone(d.management_ip) + def test_deactivated_device_download_config(self): + self._test_deactivating_deactivated_device_view('device_download_config') + def test_device_download_config_bad_uuid(self): d = self._create_device_config() pk = '{}-wrong'.format(d.pk) @@ -772,6 +807,11 @@ def test_device_report_status_error(self): self.assertEqual(d.config.status, 'error') self.assertEqual(d.config.error_reason, error_reason) + def test_deactivated_device_report_status(self): + self._test_deactivating_deactivated_device_view( + 'device_report_status', method='post', data={'status': 'applied'} + ) + def test_device_report_status_bad_uuid(self): d = self._create_device_config() pk = '{}-wrong'.format(d.pk) @@ -822,6 +862,28 @@ def test_device_report_status_405(self): ) self.assertEqual(response.status_code, 405) + def test_device_report_status_applied_after_deactivating(self): + """ + Ensure that when a device sends a "applied" status while + it is in "deactivating" state, the configuration status + of the device changes to "deactivated". + """ + self._create_template(required=True) + device = self._create_device_config() + device.deactivate() + with catch_signal(config_status_changed) as handler: + response = self.client.post( + reverse('controller:device_report_status', args=[device.pk]), + {'key': device.key, 'status': 'applied'}, + ) + device.config.refresh_from_db() + handler.assert_called_once_with( + sender=Config, signal=config_status_changed, instance=device.config + ) + self._check_header(response) + device.config.refresh_from_db() + self.assertEqual(device.config.status, 'deactivated') + def test_device_update_info(self): d = self._create_device_config() url = reverse('controller:device_update_info', args=[d.pk]) @@ -856,6 +918,11 @@ def test_device_update_info(self): self.assertEqual(d.system, params['system']) 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={} + ) + def test_device_update_info_bad_uuid(self): d = self._create_device_config() pk = '{}-wrong'.format(d.pk) diff --git a/openwisp_controller/config/tests/test_device.py b/openwisp_controller/config/tests/test_device.py index 17e38a1f4..462e59106 100644 --- a/openwisp_controller/config/tests/test_device.py +++ b/openwisp_controller/config/tests/test_device.py @@ -2,13 +2,21 @@ from unittest import mock from django.core.exceptions import ValidationError -from django.test import TestCase +from django.test import TestCase, TransactionTestCase from swapper import load_model from openwisp_utils.tests import AssertNumQueriesSubTestMixin, catch_signal from .. import settings as app_settings -from ..signals import device_group_changed, device_name_changed, management_ip_changed +from ..signals import ( + config_deactivated, + config_deactivating, + config_modified, + device_activated, + device_group_changed, + device_name_changed, + management_ip_changed, +) from ..validators import device_name_validator, mac_address_validator from .utils import CreateConfigTemplateMixin, CreateDeviceGroupMixin @@ -589,3 +597,82 @@ def test_create_default_config_existing(self): device.config.refresh_from_db() self.assertEqual(device.config.context, {'ssid': 'test'}) self.assertEqual(device.config.config, {'general': {}}) + + +class TestTransactionDevice( + CreateConfigTemplateMixin, + AssertNumQueriesSubTestMixin, + CreateDeviceGroupMixin, + TransactionTestCase, +): + def test_deactivating_device_with_config(self): + self._create_template(required=True) + self._create_template(name='Default', default=True) + group_template = self._create_template(name='Group') + group = self._create_device_group() + group.templates.add(group_template) + device = self._create_device(organization=self._get_org(), group=group) + config = device.config + self.assertEqual(config.templates.count(), 3) + + device.deactivate() + device.refresh_from_db() + config.refresh_from_db() + self.assertEqual(device.is_deactivated(), True) + self.assertEqual(config.status, 'deactivating') + self.assertEqual(config.config, {}) + self.assertEqual(config.templates.count(), 0) + + with catch_signal(config_modified) as mocked_config_modified, catch_signal( + device_activated + ) as mocked_device_activated: + device.activate() + mocked_device_activated.assert_called_once_with( + sender=Device, instance=device, signal=device_activated + ) + mocked_config_modified.assert_called() + + device.refresh_from_db() + config.refresh_from_db() + self.assertEqual(device.is_deactivated(), False) + self.assertEqual(config.status, 'modified') + # Required, default and group templates should be added back + self.assertEqual(config.templates.count(), 3) + + def test_deactivating_device_empty_config(self): + device = self._create_device() + config = self._create_config(device=device) + + with catch_signal( + config_deactivating + ) as mocked_config_deactivating, catch_signal( + config_deactivated + ) as mocked_config_deactivated: + device.deactivate() + mocked_config_deactivating.assert_called_once() + mocked_config_deactivated.assert_called_once() + device.refresh_from_db() + config.refresh_from_db + self.assertEqual(device.is_deactivated(), True) + self.assertEqual(config.status, 'deactivated') + self.assertEqual(device.management_ip, None) + + device.activate() + device.refresh_from_db() + config.refresh_from_db() + self.assertEqual(device.is_deactivated(), False) + self.assertEqual(config.status, 'applied') + + def test_deactivating_device_without_config(self): + device = self._create_device(management_ip='10.8.0.1') + self.assertEqual(device._has_config(), False) + device.deactivate() + device.refresh_from_db() + self.assertEqual(device._has_config(), False) + self.assertEqual(device.is_deactivated(), True) + self.assertEqual(device.management_ip, None) + + device.activate() + device.refresh_from_db() + self.assertEqual(device._has_config(), False) + self.assertEqual(device.is_deactivated(), False) diff --git a/openwisp_controller/config/tests/test_template.py b/openwisp_controller/config/tests/test_template.py index 25e8e40ac..914499e7f 100644 --- a/openwisp_controller/config/tests/test_template.py +++ b/openwisp_controller/config/tests/test_template.py @@ -78,7 +78,7 @@ def test_default_template(self): org = self._get_org() c = self._create_config(organization=org) self.assertEqual(c.templates.count(), 0) - c.device.delete() + c.device.delete(check_deactivated=False) # create default templates for different backends t1 = self._create_template( name='default-openwrt', backend='netjsonconfig.OpenWrt', default=True diff --git a/openwisp_controller/config/tests/test_vpn.py b/openwisp_controller/config/tests/test_vpn.py index 8162f6ddb..ac67c5577 100644 --- a/openwisp_controller/config/tests/test_vpn.py +++ b/openwisp_controller/config/tests/test_vpn.py @@ -581,7 +581,7 @@ def test_ip_deleted_when_vpnclient_deleted(self): def test_ip_deleted_when_device_deleted(self): device, vpn, template = self._create_wireguard_vpn_template() self.assertEqual(device.config.vpnclient_set.count(), 1) - device.delete() + device.delete(check_deactivated=False) self.assertEqual(IpAddress.objects.count(), 1) def test_delete_vpnclient_ip(self): @@ -745,7 +745,7 @@ def test_auto_peer_configuration(self): self.assertEqual(len(vpn_config.get('peers', [])), 2) with self.subTest('cache updated when a new peer is deleted'): - device2.delete() + device2.delete(check_deactivated=False) # cache is invalidated but not updated # hence we expect queries to be generated with self.assertNumQueries(1): @@ -954,7 +954,7 @@ def test_auto_peer_configuration(self): self.assertEqual(len(peers), 2) with self.subTest('cache updated when a new peer is deleted'): - device2.delete() + device2.delete(check_deactivated=False) # cache is invalidated but not updated # hence we expect queries to be generated with self.assertNumQueries(2): @@ -1110,7 +1110,7 @@ def test_ip_deleted_when_device_deleted(self, mock_requests, mock_subprocess): self.assertEqual(mock_subprocess.run.call_count, 1) self.assertEqual(device.config.vpnclient_set.count(), 1) self.assertEqual(IpAddress.objects.count(), 2) - device.delete() + device.delete(check_deactivated=False) self.assertEqual(IpAddress.objects.count(), 1) @mock.patch(_ZT_GENERATE_IDENTITY_SUBPROCESS) diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 6e02cd895..812ebb3a9 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -14,7 +14,7 @@ from openwisp_utils.admin import TimeReadonlyAdminMixin from ..admin import MultitenantAdminMixin -from ..config.admin import DeviceAdmin +from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdmin from .schema import schema from .widgets import CommandSchemaWidget, CredentialsSchemaWidget @@ -73,7 +73,9 @@ def schema_view(self, request): return JsonResponse(schema) -class DeviceConnectionInline(MultitenantAdminMixin, admin.StackedInline): +class DeviceConnectionInline( + MultitenantAdminMixin, DeactivatedDeviceReadOnlyMixin, admin.StackedInline +): model = DeviceConnection verbose_name = _('Credentials') verbose_name_plural = verbose_name @@ -143,7 +145,7 @@ def has_change_permission(self, request, obj): return False -class CommandWritableInline(admin.StackedInline): +class CommandWritableInline(DeactivatedDeviceReadOnlyMixin, admin.StackedInline): model = Command extra = 1 form = CommandForm diff --git a/openwisp_controller/connection/api/views.py b/openwisp_controller/connection/api/views.py index e6931ff8d..3adf222e3 100644 --- a/openwisp_controller/connection/api/views.py +++ b/openwisp_controller/connection/api/views.py @@ -13,7 +13,11 @@ from openwisp_users.api.mixins import FilterByParentManaged from openwisp_users.api.mixins import ProtectedAPIMixin as BaseProtectedAPIMixin -from ...mixins import ProtectedAPIMixin +from ...mixins import ( + ProtectedAPIMixin, + RelatedDeviceModelPermission, + RelatedDeviceProtectedAPIMixin, +) from .serializer import ( CommandSerializer, CredentialSerializer, @@ -32,11 +36,17 @@ class ListViewPagination(pagination.PageNumberPagination): max_page_size = 100 -class BaseCommandView(FilterByParentManaged, BaseProtectedAPIMixin): +class BaseCommandView( + BaseProtectedAPIMixin, + FilterByParentManaged, +): model = Command queryset = Command.objects.prefetch_related('device') serializer_class = CommandSerializer + def get_permissions(self): + return super().get_permissions() + [RelatedDeviceModelPermission()] + def get_parent_queryset(self): return Device.objects.filter( pk=self.kwargs['id'], @@ -82,7 +92,7 @@ class CredentialDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView): serializer_class = CredentialSerializer -class BaseDeviceConection(ProtectedAPIMixin, GenericAPIView): +class BaseDeviceConnection(RelatedDeviceProtectedAPIMixin, GenericAPIView): model = DeviceConnection serializer_class = DeviceConnectionSerializer @@ -109,7 +119,7 @@ def get_parent_queryset(self): return Device.objects.filter(pk=self.kwargs['pk']) -class DeviceConnenctionListCreateView(BaseDeviceConection, ListCreateAPIView): +class DeviceConnenctionListCreateView(BaseDeviceConnection, ListCreateAPIView): pagination_class = ListViewPagination def get_queryset(self): @@ -121,7 +131,7 @@ def get_queryset(self): ) -class DeviceConnectionDetailView(BaseDeviceConection, RetrieveUpdateDestroyAPIView): +class DeviceConnectionDetailView(BaseDeviceConnection, RetrieveUpdateDestroyAPIView): def get_object(self): queryset = self.filter_queryset(self.get_queryset()) filter_kwargs = { diff --git a/openwisp_controller/connection/apps.py b/openwisp_controller/connection/apps.py index d35c9a332..c11a1dc95 100644 --- a/openwisp_controller/connection/apps.py +++ b/openwisp_controller/connection/apps.py @@ -10,7 +10,7 @@ from openwisp_utils.admin_theme.menu import register_menu_subitem -from ..config.signals import config_modified +from ..config.signals import config_deactivating, config_modified from .signals import is_working_changed @@ -41,6 +41,9 @@ def ready(self): config_modified.connect( self.config_modified_receiver, dispatch_uid='connection.update_config' ) + config_deactivating.connect( + self.config_modified_receiver, dispatch_uid='connection.update_config' + ) post_save.connect( Credentials.auto_add_credentials_to_device, diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index 7a8d5496b..7b3046ce9 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -262,6 +262,35 @@ def test_endpoints_for_non_existent_device(self): self.assertEqual(response.status_code, 404) self.assertDictEqual(response.data, device_not_found) + def test_endpoints_for_deactivated_device(self): + self.device_conn.device.deactivate() + + with self.subTest('Test listing commands'): + url = self._get_path('device_command_list', self.device_id) + response = self.client.get( + url, + ) + self.assertEqual(response.status_code, 200) + + with self.subTest('Test creating commands'): + url = self._get_path('device_command_list', self.device_id) + payload = { + 'type': 'custom', + 'input': {'command': 'echo test'}, + } + response = self.client.post( + url, data=payload, content_type='application/json' + ) + 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, + ) + self.assertEqual(response.status_code, 200) + def test_non_superuser(self): list_url = self._get_path('device_command_list', self.device_id) command = self._create_command(device_conn=self.device_conn) @@ -429,7 +458,7 @@ def test_post_deviceconnection_list(self): 'enabled': True, 'failure_reason': '', } - with self.assertNumQueries(12): + with self.assertNumQueries(13): response = self.client.post(path, data, content_type='application/json') self.assertEqual(response.status_code, 201) @@ -442,7 +471,7 @@ def test_post_deviceconenction_with_no_config_device(self): 'enabled': True, 'failure_reason': '', } - with self.assertNumQueries(12): + with self.assertNumQueries(13): response = self.client.post(path, data, content_type='application/json') error_msg = ''' the update strategy can be determined automatically only if @@ -474,7 +503,7 @@ def test_put_devceconnection_detail(self): 'enabled': False, 'failure_reason': '', } - with self.assertNumQueries(14): + with self.assertNumQueries(15): response = self.client.put(path, data, content_type='application/json') self.assertEqual(response.status_code, 200) self.assertEqual( @@ -488,7 +517,7 @@ def test_patch_deviceconnectoin_detail(self): path = reverse('connection_api:deviceconnection_detail', args=(d1, dc.pk)) self.assertEqual(dc.update_strategy, app_settings.UPDATE_STRATEGIES[0][0]) data = {'update_strategy': app_settings.UPDATE_STRATEGIES[1][0]} - with self.assertNumQueries(13): + with self.assertNumQueries(14): response = self.client.patch(path, data, content_type='application/json') self.assertEqual(response.status_code, 200) self.assertEqual( @@ -499,7 +528,7 @@ def test_delete_deviceconnection_detail(self): dc = self._create_device_connection() d1 = dc.device.id path = reverse('connection_api:deviceconnection_detail', args=(d1, dc.pk)) - with self.assertNumQueries(9): + with self.assertNumQueries(10): response = self.client.delete(path) self.assertEqual(response.status_code, 204) @@ -540,3 +569,65 @@ def test_bearer_authentication(self): HTTP_AUTHORIZATION=f'Bearer {token}', ) self.assertEqual(response.status_code, 200) + + def test_deactivated_device(self): + credentials = self._create_credentials(auto_add=True) + device = self._create_config(organization=credentials.organization).device + device_conn = device.deviceconnection_set.first() + create_api_path = reverse( + 'connection_api:deviceconnection_list', args=(device.pk,) + ) + detail_api_path = reverse( + 'connection_api:deviceconnection_detail', + args=[device.id, device_conn.id], + ) + device.deactivate() + + with self.subTest('Test creating DeviceConnection'): + response = self.client.post( + create_api_path, + data={ + 'credentials': credentials.pk, + 'update_strategy': app_settings.UPDATE_STRATEGIES[0][0], + 'enabled': True, + 'failure_reason': '', + }, + content_type='application/json', + ) + self.assertEqual(response.status_code, 403) + + with self.subTest('Test listing DeviceConnection'): + response = self.client.get( + create_api_path, + ) + self.assertEqual(response.status_code, 200) + + with self.subTest('Test retrieving DeviceConnection detail'): + response = self.client.get( + detail_api_path, + ) + self.assertEqual(response.status_code, 200) + + with self.subTest('Test updating DeviceConnection'): + response = self.client.put( + detail_api_path, + { + 'credentials': credentials.pk, + 'update_strategy': app_settings.UPDATE_STRATEGIES[1][0], + 'enabled': False, + 'failure_reason': '', + }, + content_type='application/json', + ) + self.assertEqual(response.status_code, 403) + + response = self.client.patch( + detail_api_path, {'enabled': False}, content_type='application/json' + ) + self.assertEqual(response.status_code, 403) + + with self.subTest('Test deleting DeviceConnection'): + response = self.client.delete( + detail_api_path, + ) + self.assertEqual(response.status_code, 403) diff --git a/openwisp_controller/geo/admin.py b/openwisp_controller/geo/admin.py index 2ca4b8c9d..cb283c6b2 100644 --- a/openwisp_controller/geo/admin.py +++ b/openwisp_controller/geo/admin.py @@ -15,7 +15,7 @@ from openwisp_users.multitenancy import MultitenantOrgFilter from ..admin import MultitenantAdminMixin -from ..config.admin import DeviceAdminExportable +from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdminExportable from .exportable import GeoDeviceResource DeviceLocation = load_model('geo', 'DeviceLocation') @@ -72,7 +72,9 @@ class LocationAdmin(MultitenantAdminMixin, AbstractLocationAdmin): LocationAdmin.list_filter.insert(0, MultitenantOrgFilter) -class DeviceLocationInline(ObjectLocationMixin, admin.StackedInline): +class DeviceLocationInline( + ObjectLocationMixin, DeactivatedDeviceReadOnlyMixin, admin.StackedInline +): model = DeviceLocation form = ObjectLocationForm verbose_name = _('Map') diff --git a/openwisp_controller/geo/api/views.py b/openwisp_controller/geo/api/views.py index 9d65d1fad..8c2cedaa4 100644 --- a/openwisp_controller/geo/api/views.py +++ b/openwisp_controller/geo/api/views.py @@ -3,7 +3,7 @@ from django.http import Http404 from django_filters import rest_framework as filters from rest_framework import generics, pagination, status -from rest_framework.exceptions import NotFound +from rest_framework.exceptions import NotFound, PermissionDenied from rest_framework.permissions import BasePermission from rest_framework.request import clone_request from rest_framework.response import Response @@ -14,7 +14,7 @@ from openwisp_users.api.filters import OrganizationManagedFilter from openwisp_users.api.mixins import FilterByOrganizationManaged, FilterByParentManaged -from ...mixins import ProtectedAPIMixin +from ...mixins import ProtectedAPIMixin, RelatedDeviceProtectedAPIMixin from .filters import DeviceListFilter from .serializers import ( DeviceCoordinatesSerializer, @@ -77,6 +77,8 @@ def get_location(self, device): def get_object(self, *args, **kwargs): device = super().get_object() + if self.request.method not in ('GET', 'HEAD') and device.is_deactivated(): + raise PermissionDenied location = self.get_location(device) if location: return location @@ -102,7 +104,7 @@ def create_location(self, device): class DeviceLocationView( - ProtectedAPIMixin, + RelatedDeviceProtectedAPIMixin, generics.RetrieveUpdateDestroyAPIView, ): serializer_class = DeviceLocationSerializer @@ -112,6 +114,7 @@ class DeviceLocationView( lookup_field = 'content_object' lookup_url_kwarg = 'pk' organization_field = 'content_object__organization' + _device_field = 'content_object' def get_queryset(self): qs = super().get_queryset() @@ -120,6 +123,11 @@ def get_queryset(self): except ValidationError: return qs.none() + def get_parent_queryset(self): + return Device.objects.filter( + pk=self.kwargs['pk'], + ) + def get_serializer_context(self): context = super().get_serializer_context() context.update({'device_id': self.kwargs['pk']}) diff --git a/openwisp_controller/geo/tests/test_api.py b/openwisp_controller/geo/tests/test_api.py index e52a2e431..ffc334ec2 100644 --- a/openwisp_controller/geo/tests/test_api.py +++ b/openwisp_controller/geo/tests/test_api.py @@ -157,6 +157,25 @@ def test_bearer_authentication(self): ) self.assertEqual(response.status_code, 200) + def test_deactivated_device(self): + device = self._create_object_location().device + url = '{0}?key={1}'.format(reverse(self.url_name, args=[device.pk]), device.key) + device.deactivate() + + with self.subTest('Test retrieving device co-ordinates'): + response = self.client.get( + url, + content_type='application/json', + ) + self.assertEqual(response.status_code, 200) + + with self.subTest('Test updating device co-ordinates'): + response = self.client.put( + url, + content_type='application/json', + ) + self.assertEqual(response.status_code, 403) + class TestMultitenantApi(TestGeoMixin, TestCase, CreateConfigTemplateMixin): object_location_model = DeviceLocation @@ -676,7 +695,7 @@ def test_create_devicelocation_using_related_ids(self): floorplan = self._create_floorplan() location = floorplan.location url = reverse('geo_api:device_location', args=[device.id]) - with self.assertNumQueries(13): + with self.assertNumQueries(15): response = self.client.put( url, data={ @@ -714,7 +733,7 @@ def test_create_devicelocation_location_floorplan(self): 'floorplan.image': self._get_simpleuploadedfile(), 'indoor': ['12.342,23.541'], } - with self.assertNumQueries(27): + with self.assertNumQueries(29): response = self.client.put( url, encode_multipart(BOUNDARY, data), content_type=MULTIPART_CONTENT ) @@ -781,7 +800,7 @@ def test_create_devicelocation_only_location(self): 'type': 'indoor', } } - with self.assertNumQueries(16): + with self.assertNumQueries(18): response = self.client.put(url, data=data, content_type='application/json') self.assertEqual(response.status_code, 201) self.assertEqual(self.location_model.objects.count(), 1) @@ -798,7 +817,7 @@ def test_create_devicelocation_only_floorplan(self): 'floorplan.floor': 1, 'floorplan.image': self._get_simpleuploadedfile(), } - with self.assertNumQueries(3): + with self.assertNumQueries(5): response = self.client.put( url, encode_multipart(BOUNDARY, data), content_type=MULTIPART_CONTENT ) @@ -821,7 +840,7 @@ def test_create_devicelocation_existing_location_new_floorplan(self): 'floorplan.image': self._get_simpleuploadedfile(), 'indoor': ['12.342,23.541'], } - with self.assertNumQueries(21): + with self.assertNumQueries(23): response = self.client.put( url, encode_multipart(BOUNDARY, data), content_type=MULTIPART_CONTENT ) @@ -844,7 +863,7 @@ def test_update_devicelocation_change_location_outdoor_to_indoor(self): } self.assertEqual(device_location.location.type, 'outdoor') self.assertEqual(device_location.floorplan, None) - with self.assertNumQueries(20): + with self.assertNumQueries(21): response = self.client.put( path, encode_multipart(BOUNDARY, data), content_type=MULTIPART_CONTENT ) @@ -863,7 +882,7 @@ def test_update_devicelocation_patch_indoor(self): 'indoor': '0,0', } self.assertEqual(device_location.indoor, '-140.38620,40.369227') - with self.assertNumQueries(9): + with self.assertNumQueries(10): response = self.client.patch(path, data, content_type='application/json') self.assertEqual(response.status_code, 200) device_location.refresh_from_db() @@ -880,7 +899,7 @@ def test_update_devicelocation_floorplan_related_id(self): data = { 'floorplan': str(floor2.id), } - with self.assertNumQueries(11): + with self.assertNumQueries(12): response = self.client.patch(path, data, content_type='application/json') self.assertEqual(response.status_code, 200) device_location.refresh_from_db() @@ -894,7 +913,7 @@ def test_update_devicelocation_location_related_id(self): data = { 'location': str(location2.id), } - with self.assertNumQueries(8): + with self.assertNumQueries(9): response = self.client.patch(path, data, content_type='application/json') self.assertEqual(response.status_code, 200) device_location.refresh_from_db() @@ -952,3 +971,38 @@ def _assert_device_list_with_geo_filter(response=None, device=None): # make sure device_b is in the api response r2 = self.client.get(f'{path}?with_geo=true') _assert_device_list_with_geo_filter(response=r2, device=device_b) + + def test_deactivated_device(self): + floorplan = self._create_floorplan() + device_location = self._create_object_location( + location=floorplan.location, floorplan=floorplan + ) + device_location.device.deactivate() + url = reverse('geo_api:device_location', args=[device_location.device.pk]) + + with self.subTest('Test retrieving DeviceLocation'): + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + with self.subTest('Test updating DeviceLocation'): + data = { + 'location': { + 'name': 'test-location', + 'address': 'Via del Corso, Roma, Italia', + 'geometry': 'SRID=4326;POINT (12.512124 41.898903)', + 'type': 'indoor', + } + } + response = self.client.put( + url, + data, + content_type='application/json', + ) + self.assertEqual(response.status_code, 403) + + response = self.client.patch(url, data, content_type='application/json') + self.assertEqual(response.status_code, 403) + + with self.subTest('Test deleting DeviceLocation'): + response = self.client.delete(url) + self.assertEqual(response.status_code, 403) diff --git a/openwisp_controller/mixins.py b/openwisp_controller/mixins.py index b028c5572..62e892884 100644 --- a/openwisp_controller/mixins.py +++ b/openwisp_controller/mixins.py @@ -1,5 +1,36 @@ from openwisp_users.api.mixins import FilterByOrganizationManaged from openwisp_users.api.mixins import ProtectedAPIMixin as BaseProtectedAPIMixin +from openwisp_users.api.permissions import DjangoModelPermissions, IsOrganizationManager + + +class RelatedDeviceModelPermission(DjangoModelPermissions): + _device_field = 'device' + + def _has_permissions(self, request, view, perm, obj=None): + if request.method in self.READ_ONLY_METHOD: + return perm + if obj: + device = getattr(obj, self._device_field) + else: + device = view.get_parent_queryset()[0] + return perm and not device.is_deactivated() + + def has_permission(self, request, view): + perm = super().has_permission(request, view) + return self._has_permissions(request, view, perm) + + def has_object_permission(self, request, view, obj): + perm = super().has_object_permission(request, view, obj) + return self._has_permissions(request, view, perm, obj) + + +class RelatedDeviceProtectedAPIMixin( + BaseProtectedAPIMixin, FilterByOrganizationManaged +): + permission_classes = [ + IsOrganizationManager, + RelatedDeviceModelPermission, + ] class ProtectedAPIMixin(BaseProtectedAPIMixin, FilterByOrganizationManaged): diff --git a/openwisp_controller/subnet_division/tests/test_admin.py b/openwisp_controller/subnet_division/tests/test_admin.py index 5fff4a2a3..31eeb987a 100644 --- a/openwisp_controller/subnet_division/tests/test_admin.py +++ b/openwisp_controller/subnet_division/tests/test_admin.py @@ -239,6 +239,8 @@ def test_subnet_filter_multitenancy(self): self.assertContains(response, config2.device.name) def test_delete_device(self): + self.config.device.deactivate() + self.config.set_status_deactivated() device_response = self.client.post( reverse( f'admin:{self.config_label}_device_delete', diff --git a/openwisp_controller/subnet_division/tests/test_models.py b/openwisp_controller/subnet_division/tests/test_models.py index fd7abf8d8..3c1b9e9ea 100644 --- a/openwisp_controller/subnet_division/tests/test_models.py +++ b/openwisp_controller/subnet_division/tests/test_models.py @@ -651,7 +651,7 @@ def test_device_deleted(self): rule.number_of_subnets, ) - self.config.device.delete() + self.config.device.delete(check_deactivated=False) self.assertEqual( subnet_query.count(), 0, @@ -731,7 +731,7 @@ def test_device_subnet_division_rule(self): self.assertIn(f'{rule.label}_subnet{subnet_id}_ip{ip_id}', context) # Verify working of delete handler - device.delete() + device.delete(check_deactivated=False) self.assertEqual( subnet_query.count(), 0, @@ -794,7 +794,7 @@ def test_device_rule_use_entire_subnet(self): self.assertIn(f'{rule.label}_subnet{subnet_id}_ip{ip_id}', context) # Verify working of delete handler - device.delete() + device.delete(check_deactivated=False) self.assertEqual( subnet_query.count(), 0, diff --git a/openwisp_controller/tests/test_selenium.py b/openwisp_controller/tests/test_selenium.py index 8e90773fb..fd451114f 100644 --- a/openwisp_controller/tests/test_selenium.py +++ b/openwisp_controller/tests/test_selenium.py @@ -38,7 +38,8 @@ def setUp(self): def test_restoring_deleted_device(self, *args): org = self._get_org() self._create_credentials(auto_add=True, organization=org) - device = self._create_config(organization=org).device + config = self._create_config(organization=org) + device = config.device self._create_object_location( location=self._create_location( organization=org, @@ -50,6 +51,8 @@ def test_restoring_deleted_device(self, *args): self.login() # Delete the device + device.deactivate() + config.set_status_deactivated() self.open( reverse(f'admin:{self.config_app_label}_device_delete', args=[device.id]) ) diff --git a/tests/openwisp2/sample_config/api/views.py b/tests/openwisp2/sample_config/api/views.py index 4804f1536..daa8e2feb 100644 --- a/tests/openwisp2/sample_config/api/views.py +++ b/tests/openwisp2/sample_config/api/views.py @@ -7,6 +7,12 @@ from openwisp_controller.config.api.download_views import ( DownloadVpnView as BaseDownloadVpnView, ) +from openwisp_controller.config.api.views import ( + DeviceActivateView as BaseDeviceActivateView, +) +from openwisp_controller.config.api.views import ( + DeviceDeactivateView as BaseDeviceDeactivateView, +) from openwisp_controller.config.api.views import ( DeviceDetailView as BaseDeviceDetailView, ) @@ -66,6 +72,14 @@ class DeviceDetailView(BaseDeviceDetailView): pass +class DeviceActivateView(BaseDeviceActivateView): + pass + + +class DeviceDeactivateView(BaseDeviceDeactivateView): + pass + + class DeviceGroupListCreateView(BaseDeviceGroupListCreateView): pass @@ -90,6 +104,8 @@ class DownloadDeviceView(BaseDownloadDeviceView): download_vpn_config = DownloadVpnView.as_view() device_list = DeviceListCreateView.as_view() device_detail = DeviceDetailView.as_view() +device_activate = DeviceActivateView.as_view() +device_deactivate = DeviceDeactivateView.as_view() download_device_config = DownloadDeviceView().as_view() devicegroup_list = DeviceGroupListCreateView.as_view() devicegroup_detail = DeviceGroupDetailView.as_view() diff --git a/tests/openwisp2/sample_config/migrations/0006_device__is_deactivated_alter_config_status.py b/tests/openwisp2/sample_config/migrations/0006_device__is_deactivated_alter_config_status.py new file mode 100644 index 000000000..767a15cfd --- /dev/null +++ b/tests/openwisp2/sample_config/migrations/0006_device__is_deactivated_alter_config_status.py @@ -0,0 +1,41 @@ +# Generated by Django 4.2.10 on 2024-03-01 16:37 + +import model_utils.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("sample_config", "0005_add_organizationalloweddevice"), + ] + + operations = [ + migrations.AddField( + model_name="device", + name="_is_deactivated", + field=models.BooleanField(default=False), + ), + migrations.AlterField( + model_name="config", + name="status", + field=model_utils.fields.StatusField( + choices=[ + ("modified", "modified"), + ("applied", "applied"), + ("error", "error"), + ("deactivating", "deactivating"), + ("deactivated", "deactivated"), + ], + default="modified", + help_text=( + '"modified" means the configuration is not applied yet;' + ' \n"applied" means the configuration is applied successfully;' + ' \n"error" means the configuration caused issues and it was' + ' rolled back;' + ), + max_length=100, + no_check_for_status=True, + verbose_name="configuration status", + ), + ), + ] diff --git a/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py b/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py new file mode 100644 index 000000000..cc1b0fa8c --- /dev/null +++ b/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py @@ -0,0 +1,39 @@ +# Generated by Django 4.2.11 on 2024-03-07 17:10 + +import model_utils.fields +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("sample_config", "0006_device__is_deactivated_alter_config_status"), + ] + + operations = [ + migrations.AlterField( + model_name="config", + name="status", + field=model_utils.fields.StatusField( + choices=[ + ("modified", "modified"), + ("applied", "applied"), + ("error", "error"), + ("deactivating", "deactivating"), + ("deactivated", "deactivated"), + ], + default="modified", + help_text=( + '"modified" means the configuration is not applied yet; \n' + '"applied" means the configuration is applied successfully; \n' + '"error" means the configuration caused issues and it was' + ' rolled back; \n"deactivating" means the device has been' + ' deactivated and the configuration is being removed; \n' + '"deactivated" means the configuration has been removed ' + 'from the device;' + ), + max_length=100, + no_check_for_status=True, + verbose_name="configuration status", + ), + ), + ]