From d0ee32414c9fb778ed680e71c0bd42fae7c89388 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 29 Feb 2024 17:27:17 +0530 Subject: [PATCH 01/52] [feature] Implemented device deactivation and reactivation #625 Closes #625 --- openwisp_controller/config/admin.py | 55 +++++++++++++++++-- openwisp_controller/config/base/config.py | 30 +++++++++- openwisp_controller/config/base/device.py | 33 ++++++++++- .../migrations/0054_device__is_deactivated.py | 18 ++++++ .../config/tests/test_device.py | 45 ++++++++++++++- openwisp_controller/connection/admin.py | 6 +- openwisp_controller/geo/admin.py | 10 +++- 7 files changed, 183 insertions(+), 14 deletions(-) create mode 100644 openwisp_controller/config/migrations/0054_device__is_deactivated.py diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index f51194af5..b6aad80ae 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -73,6 +73,20 @@ class BaseAdmin(TimeReadonlyAdminMixin, ModelAdmin): history_latest_first = True +class DeactivatedDeviceReadOnlyMixin(object): + def has_add_permission(self, request, obj): + perm = super().has_add_permission(request, obj) + if not obj: + return perm + return perm and not obj.is_deactivated() + + def has_change_permission(self, request, obj=None): + perm = super().has_change_permission(request) + if not obj: + return perm + return perm and not obj.is_deactivated() + + class BaseConfigAdmin(BaseAdmin): change_form_template = 'admin/config/change_form.html' preview_template = None @@ -390,6 +404,7 @@ class Meta(BaseForm.Meta): class ConfigInline( + DeactivatedDeviceReadOnlyMixin, MultitenantAdminMixin, TimeReadonlyAdminMixin, SystemDefinedVariableMixin, @@ -425,10 +440,6 @@ def _error_reason_field_conditional(self, obj, fields): fields.insert(fields.index('status') + 1, 'error_reason') return fields - def get_readonly_fields(self, request, obj): - fields = super().get_readonly_fields(request, obj) - return self._error_reason_field_conditional(obj, fields) - def get_fields(self, request, obj): fields = super().get_fields(request, obj) return self._error_reason_field_conditional(obj, fields) @@ -499,7 +510,7 @@ class DeviceAdmin(MultitenantAdminMixin, BaseConfigAdmin, UUIDAdmin): ] inlines = [ConfigInline] conditional_inlines = [] - actions = ['change_group'] + actions = ['deactivate_device', 'change_group', 'activate_device'] org_position = 1 if not app_settings.HARDWARE_ID_ENABLED else 2 list_display.insert(org_position, 'organization') _state_adding = False @@ -520,6 +531,12 @@ 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: + return perm + return perm and not 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 +641,34 @@ def change_group(self, request, queryset): request, 'admin/config/change_device_group.html', context ) + def _change_device_status(self, request, queryset, method): + """ + This helper method provides re-usability of code for + device activation and deactivation actions. + """ + # Validate selected devices can be managed by the user + if not request.user.is_superuser: + # There could be multiple devices selected by the user. + # Validate that all the devices can be managed by the user. + for org_id in set(queryset.values_list('organization_id', flat=True)): + if not request.user.is_manager(str(org_id)): + logger.warning( + f'{request.user} attempted to deactivate device of "{org_id}"' + ' organization which they do not manage.' + ' The operation was rejected.' + ) + return HttpResponseForbidden() + for device in queryset.iterator(): + getattr(device, method)() + + @admin.actions(description=_('Deactivate selected devices'), permissions=['change']) + def deactivate_device(self, request, queryset): + self._change_device_status(request, queryset, 'deactivate') + + @admin.actions(description=_('Activate selected devices'), permissions=['change']) + def activate_device(self, request, queryset): + self._change_device_status(request, queryset, 'activate') + def get_fields(self, request, obj=None): """ Do not show readonly fields in add form diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 181cc566a..132f35e7f 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -62,7 +62,7 @@ class AbstractConfig(BaseConfig): blank=True, ) - STATUS = Choices('modified', 'applied', 'error') + STATUS = Choices('modified', 'applied', 'error', 'deactivating', 'deactivated') status = StatusField( _('configuration status'), help_text=_( @@ -331,6 +331,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 @@ -483,6 +485,15 @@ def save(self, *args, **kwargs): self._initial_status = self.status return result + 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' @@ -539,9 +550,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 +575,18 @@ 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.config = {} + self._set_status('deactivating', save, extra_update_fields=['config']) + self.templates.clear() + + def set_status_deactivated(self, save=True): + self._set_status('deactivated', save) + 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 95cf065f9..8042767d9 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.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 @@ -96,6 +96,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 +167,33 @@ 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.set_status_deactivating() + self._is_deactivated = True + self.save() + + 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.set_status_modified() + # Trigger enforcing of required templates + self.config.templates.clear() + self._is_deactivated = False + self.save() + def get_context(self): config = self._get_config() return config.get_context() 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..2becdbfd9 --- /dev/null +++ b/openwisp_controller/config/migrations/0054_device__is_deactivated.py @@ -0,0 +1,18 @@ +# 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/tests/test_device.py b/openwisp_controller/config/tests/test_device.py index 17e38a1f4..675ef331d 100644 --- a/openwisp_controller/config/tests/test_device.py +++ b/openwisp_controller/config/tests/test_device.py @@ -2,7 +2,7 @@ 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 @@ -589,3 +589,46 @@ 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, + TestOrganizationMixin, + AssertNumQueriesSubTestMixin, + CreateDeviceGroupMixin, + TransactionTestCase, +): + def test_deactivating_device_with_config(self): + self._create_template(required=True) + config = self._create_config(organization=self._get_org()) + device = config.device + self.assertEqual(config.templates.count(), 1) + + 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) + + device.activate() + device.refresh_from_db() + config.refresh_from_db() + self.assertEqual(device.is_deactivated(), False) + self.assertEqual(config.status, 'modified') + # Required templates are automatically added + self.assertEqual(config.templates.count(), 1) + + def test_deactivating_device_without_config(self): + device = self._create_device() + self.assertEqual(device._has_config(), False) + device.deactivate() + device.refresh_from_db() + self.assertEqual(device._has_config(), False) + self.assertEqual(device.is_deactivated(), True) + + device.activate() + device.refresh_from_db() + self.assertEqual(device._has_config(), False) + self.assertEqual(device.is_deactivated(), False) diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 6e02cd895..f2114657f 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 diff --git a/openwisp_controller/geo/admin.py b/openwisp_controller/geo/admin.py index 2ca4b8c9d..da58e2e55 100644 --- a/openwisp_controller/geo/admin.py +++ b/openwisp_controller/geo/admin.py @@ -15,7 +15,11 @@ from openwisp_users.multitenancy import MultitenantOrgFilter from ..admin import MultitenantAdminMixin -from ..config.admin import DeviceAdminExportable +from ..config.admin import ( + DeactivatedDeviceReadOnlyMixin, + DeviceAdmin, + DeviceAdminExportable, +) from .exportable import GeoDeviceResource DeviceLocation = load_model('geo', 'DeviceLocation') @@ -72,7 +76,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') From a8aa0b577f93270b5df83bbceef41b33e7ff2aeb Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 29 Feb 2024 18:23:09 +0530 Subject: [PATCH 02/52] [feature] Do not delete related Certs when device is deactivated --- openwisp_controller/config/base/config.py | 14 ++++++++++++-- openwisp_controller/config/tests/test_config.py | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 132f35e7f..9267caab0 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -236,8 +236,13 @@ 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']) + and not ( + action == 'post_clear' and instance.is_deactivating_or_deactivated() + ) + ): return vpn_client_model = cls.vpn.through # coming from signal @@ -252,6 +257,11 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs): # coming from admin ModelForm else: templates = pk_set + if action == 'post_clear' and instance.is_deactivating_or_deactivated(): + # If the config is being deactivting or deactivated, then + # delete all vpn clients and return. + instance.vpnclient_set.all().delete() + return # delete VPN clients which have been cleared # by sortedm2m and have not been added back if action == 'post_add': diff --git a/openwisp_controller/config/tests/test_config.py b/openwisp_controller/config/tests/test_config.py index 1f9059649..3a7c007be 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.set_status_deactivating() + 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') From 6bdffdc4a71282c0cc5249993a8169d8c071c4e5 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 29 Feb 2024 19:38:08 +0530 Subject: [PATCH 03/52] [feature] Set device status to deactivated if current status is deactivating --- .../config/controller/views.py | 5 +++++ .../config/tests/test_controller.py | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/openwisp_controller/config/controller/views.py b/openwisp_controller/config/controller/views.py index dfbcc40ec..6dd5552c8 100644 --- a/openwisp_controller/config/controller/views.py +++ b/openwisp_controller/config/controller/views.py @@ -247,6 +247,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/tests/test_controller.py b/openwisp_controller/config/tests/test_controller.py index 05443e5f3..aed074a9c 100644 --- a/openwisp_controller/config/tests/test_controller.py +++ b/openwisp_controller/config/tests/test_controller.py @@ -822,6 +822,27 @@ 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". + """ + 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]) From 259f31f7e8c29978acb169bd2933e828c262f121 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 29 Feb 2024 20:06:20 +0530 Subject: [PATCH 04/52] [fix] Fixed readonlyfields for ConfigInline --- openwisp_controller/config/admin.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index b6aad80ae..7ba0b48ef 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -440,6 +440,10 @@ def _error_reason_field_conditional(self, obj, fields): fields.insert(fields.index('status') + 1, 'error_reason') return fields + def get_readonly_fields(self, request, obj): + fields = super().get_readonly_fields(request, obj) + return self._error_reason_field_conditional(obj, fields) + def get_fields(self, request, obj): fields = super().get_fields(request, obj) return self._error_reason_field_conditional(obj, fields) From f47edd29055ea76f70e77f96c1e7b8f756f41ab3 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 29 Feb 2024 20:55:44 +0530 Subject: [PATCH 05/52] [feature] Return 404 checksum for deactivated devices --- openwisp_controller/config/apps.py | 5 ++++ openwisp_controller/config/base/config.py | 21 +++++++++++++++- .../config/controller/views.py | 14 +++++++++-- openwisp_controller/config/signals.py | 4 +++ .../config/tests/test_controller.py | 25 +++++++++++++++++++ 5 files changed, 66 insertions(+), 3 deletions(-) diff --git a/openwisp_controller/config/apps.py b/openwisp_controller/config/apps.py index 846725c2e..2e7e4add9 100644 --- a/openwisp_controller/config/apps.py +++ b/openwisp_controller/config/apps.py @@ -22,6 +22,7 @@ from . import settings as app_settings from .signals import ( config_backend_changed, + config_deactivated, config_modified, device_group_changed, device_name_changed, @@ -305,6 +306,10 @@ def enable_cache_invalidation(self): sender=self.device_model, dispatch_uid='invalidate_get_device_cache', ) + config_deactivated.connect( + DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated, + dispatch_uid='config_deactivated_invalidate_get_device_cache', + ) config_modified.connect( DeviceChecksumView.invalidate_checksum_cache, dispatch_uid='invalidate_checksum_cache', diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 9267caab0..4824c0c16 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -14,7 +14,12 @@ 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_modified, + config_status_changed, +) from ..sortedm2m.fields import SortedManyToManyField from ..utils import get_default_templates_queryset from .base import BaseConfig @@ -105,6 +110,7 @@ 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_status_changed = False def __str__(self): @@ -492,6 +498,8 @@ 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_deactivated and self.is_deactivated(): + self._send_config_deactivated_signal() self._initial_status = self.status return result @@ -541,6 +549,16 @@ def _send_config_modified_signal(self, action): device=self.device, ) + 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. @@ -595,6 +613,7 @@ def set_status_deactivating(self, save=True): self.templates.clear() def set_status_deactivated(self, save=True): + self._send_config_deactivated = True self._set_status('deactivated', save) def _has_device(self): diff --git a/openwisp_controller/config/controller/views.py b/openwisp_controller/config/controller/views.py index 6dd5552c8..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): """ diff --git a/openwisp_controller/config/signals.py b/openwisp_controller/config/signals.py index 57a661d76..f75ff293d 100644 --- a/openwisp_controller/config/signals.py +++ b/openwisp_controller/config/signals.py @@ -17,6 +17,10 @@ config_modified.__doc__ = """ Providing arguments: ['instance', 'device', 'config', 'previous_status', 'action'] """ +config_deactivated = Signal() +config_deactivated.__doc__ = """ +Providing arguments: ['instance', 'previous_status'] +""" device_registered = Signal() device_registered.__doc__ = """ Providing arguments: ['instance', 'is_new'] diff --git a/openwisp_controller/config/tests/test_controller.py b/openwisp_controller/config/tests/test_controller.py index aed074a9c..17afc2c6f 100644 --- a/openwisp_controller/config/tests/test_controller.py +++ b/openwisp_controller/config/tests/test_controller.py @@ -245,6 +245,31 @@ def test_device_config_download_requested_signal_is_emitted(self): request=response.wsgi_request, ) + def test_device_config_deactivated_checksum(self): + device = self._create_device_config() + config = device.config + # The endpoint returns 200 when config.status is modified + config.set_status_modified() + path = reverse('controller:device_checksum', args=[device.pk]) + response = self.client.get(path, {'key': device.key}) + 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') + @capture_any_output() def test_device_checksum_400(self): d = self._create_device_config() From 8c60f7d77ca6d5fb2a4717d8e17d89fb93c230de Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 1 Mar 2024 21:59:53 +0530 Subject: [PATCH 06/52] [tests] Added test for device controller views --- .../config/tests/test_controller.py | 84 ++++++++++++++----- 1 file changed, 61 insertions(+), 23 deletions(-) diff --git a/openwisp_controller/config/tests/test_controller.py b/openwisp_controller/config/tests/test_controller.py index 17afc2c6f..fbdaffe12 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 @@ -246,29 +275,7 @@ def test_device_config_download_requested_signal_is_emitted(self): ) def test_device_config_deactivated_checksum(self): - device = self._create_device_config() - config = device.config - # The endpoint returns 200 when config.status is modified - config.set_status_modified() - path = reverse('controller:device_checksum', args=[device.pk]) - response = self.client.get(path, {'key': device.key}) - 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') + self._test_deactivating_deactivated_device_view('device_checksum') @capture_any_output() def test_device_checksum_400(self): @@ -310,6 +317,27 @@ def test_device_download_config(self): self.assertIsNotNone(d.last_ip) self.assertIsNone(d.management_ip) + def test_deactivated_device_download_config(self): + device = self._create_device_config() + config = device.config + url = reverse('controller:device_download_config', args=[device.pk]) + + response = self.client.get(url, {'key': device.key}) + self.assertEqual(response.status_code, 200) + self.assertEqual(self.cn) + + config.set_status_deactivating() + response = self.client.get(url, {'key': device.key}) + self.assertEqual(response.status_code, 200) + config.refresh_from_db() + self.assertEqual(config.status, 'deactivating') + + config.set_status_deactivated() + response = self.client.get(url, {'key': device.key}) + self.assertEqual(response.status_code, 404) + config.refresh_from_db() + self.assertEqual(config.status, 'deactivated') + def test_device_download_config_bad_uuid(self): d = self._create_device_config() pk = '{}-wrong'.format(d.pk) @@ -797,6 +825,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) @@ -902,6 +935,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) From 778a72942cc039fc0df48b7dd9102ccbcfe02ee2 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 1 Mar 2024 22:00:11 +0530 Subject: [PATCH 07/52] [feature] Added activate and deactivate button on the device page --- openwisp_controller/config/admin.py | 35 ++++++++++++++----- .../admin/config/device/submit_line.html | 19 ++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) create mode 100644 openwisp_controller/config/templates/admin/config/device/submit_line.html diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 7ba0b48ef..591a47322 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -101,7 +101,7 @@ class Media: for file_ in ('preview.js', 'unsaved_changes.js', 'switcher.js') ] - def get_extra_context(self, pk=None): + def get_extra_context(self, request, pk=None): prefix = 'admin:{0}_{1}'.format( self.opts.app_label, self.model.__name__.lower() ) @@ -136,7 +136,7 @@ def get_extra_context(self, pk=None): def add_view(self, request, form_url='', extra_context=None): extra_context = extra_context or {} - extra_context.update(self.get_extra_context()) + extra_context.update(self.get_extra_context(request)) instance = self.model() if hasattr(instance, 'get_default_templates'): templates = instance.get_default_templates() @@ -145,7 +145,7 @@ def add_view(self, request, form_url='', extra_context=None): return super().add_view(request, form_url, extra_context) def change_view(self, request, object_id, form_url='', extra_context=None): - extra_context = self.get_extra_context(object_id) + extra_context = self.get_extra_context(request, object_id) return super().change_view(request, object_id, form_url, extra_context) def get_urls(self): @@ -541,6 +541,14 @@ def has_change_permission(self, request, obj=None): 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) @@ -734,8 +742,17 @@ def get_urls(self): pass return urls - def get_extra_context(self, pk=None): - ctx = super().get_extra_context(pk) + def get_extra_context(self, request, pk=None): + ctx = super().get_extra_context(request, pk) + if pk: + device = self.get_object(request, pk) + ctx.update( + { + 'show_deactivate': not device.is_deactivated(), + 'show_activate': device.is_deactivated(), + 'action_checkbox_name': helpers.ACTION_CHECKBOX_NAME, + } + ) ctx.update( { 'relevant_template_url': reverse( @@ -750,7 +767,7 @@ def get_extra_context(self, pk=None): return ctx def add_view(self, request, form_url='', extra_context=None): - extra_context = self.get_extra_context() + extra_context = self.get_extra_context(request) return super().add_view(request, form_url, extra_context) def get_inlines(self, request, obj): @@ -1124,14 +1141,14 @@ def schema_view(self, request): def add_view(self, request, form_url='', extra_context=None): extra_context = extra_context or {} - extra_context.update(self.get_extra_context()) + extra_context.update(self.get_extra_context(request)) return super().add_view(request, form_url, extra_context) def change_view(self, request, object_id, form_url='', extra_context=None): - extra_context = self.get_extra_context(object_id) + extra_context = self.get_extra_context(request, object_id) return super().change_view(request, object_id, form_url, extra_context) - def get_extra_context(self, pk=None): + def get_extra_context(self, request, pk=None): ctx = { 'relevant_template_url': reverse( 'admin:get_relevant_templates', args=['org_id'] diff --git a/openwisp_controller/config/templates/admin/config/device/submit_line.html b/openwisp_controller/config/templates/admin/config/device/submit_line.html new file mode 100644 index 000000000..3ff0831d9 --- /dev/null +++ b/openwisp_controller/config/templates/admin/config/device/submit_line.html @@ -0,0 +1,19 @@ +{% extends "admin/submit_line.html" %} +{% load i18n l10n admin_urls %} + +{% block submit-row %} +{% url opts|admin_urlname:'changelist' as changelist_url %} +
+ {% csrf_token %} + + {% if show_activate %} + + + {% endif %} + {% if show_deactivate %} + + + {% endif %} +
+{{ block.super }} +{% endblock %} From a08c85e4d7971bbc68f81f6f2196ae956f2f0cd6 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 1 Mar 2024 22:06:20 +0530 Subject: [PATCH 08/52] [chores] Added migrations for Config.status --- .../migrations/0055_alter_config_status.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 openwisp_controller/config/migrations/0055_alter_config_status.py 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..1e877dec7 --- /dev/null +++ b/openwisp_controller/config/migrations/0055_alter_config_status.py @@ -0,0 +1,32 @@ +# 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;', + max_length=100, + no_check_for_status=True, + verbose_name="configuration status", + ), + ), + ] From 0af9d2dcf8c7f4ef360e54c7c32ba9341d517f8e Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 1 Mar 2024 22:07:30 +0530 Subject: [PATCH 09/52] [chores] Added migrations for sample app --- ...ice__is_deactivated_alter_config_status.py | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 tests/openwisp2/sample_config/migrations/0006_device__is_deactivated_alter_config_status.py 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..8d3db4a20 --- /dev/null +++ b/tests/openwisp2/sample_config/migrations/0006_device__is_deactivated_alter_config_status.py @@ -0,0 +1,37 @@ +# 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", + ), + ), + ] From 834c8228215d6d9e4b81aa5a9239c5d6b4d0e5a9 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 1 Mar 2024 22:20:41 +0530 Subject: [PATCH 10/52] [tests] Fixed tests --- .../config/tests/test_controller.py | 20 +------------------ .../subnet_division/tests/test_admin.py | 2 ++ 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/openwisp_controller/config/tests/test_controller.py b/openwisp_controller/config/tests/test_controller.py index fbdaffe12..b54f5fc3c 100644 --- a/openwisp_controller/config/tests/test_controller.py +++ b/openwisp_controller/config/tests/test_controller.py @@ -318,25 +318,7 @@ def test_device_download_config(self): self.assertIsNone(d.management_ip) def test_deactivated_device_download_config(self): - device = self._create_device_config() - config = device.config - url = reverse('controller:device_download_config', args=[device.pk]) - - response = self.client.get(url, {'key': device.key}) - self.assertEqual(response.status_code, 200) - self.assertEqual(self.cn) - - config.set_status_deactivating() - response = self.client.get(url, {'key': device.key}) - self.assertEqual(response.status_code, 200) - config.refresh_from_db() - self.assertEqual(config.status, 'deactivating') - - config.set_status_deactivated() - response = self.client.get(url, {'key': device.key}) - self.assertEqual(response.status_code, 404) - config.refresh_from_db() - self.assertEqual(config.status, 'deactivated') + self._test_deactivating_deactivated_device_view('device_download_config') def test_device_download_config_bad_uuid(self): d = self._create_device_config() 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', From df822ce18a916e6e3cf135b11b197385ee45cc33 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 1 Mar 2024 22:32:04 +0530 Subject: [PATCH 11/52] [qa] Fixed QA issues --- .../config/migrations/0055_alter_config_status.py | 7 ++++++- .../0006_device__is_deactivated_alter_config_status.py | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/migrations/0055_alter_config_status.py b/openwisp_controller/config/migrations/0055_alter_config_status.py index 1e877dec7..bd8399a1a 100644 --- a/openwisp_controller/config/migrations/0055_alter_config_status.py +++ b/openwisp_controller/config/migrations/0055_alter_config_status.py @@ -23,7 +23,12 @@ class Migration(migrations.Migration): ("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;', + 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/0006_device__is_deactivated_alter_config_status.py b/tests/openwisp2/sample_config/migrations/0006_device__is_deactivated_alter_config_status.py index 8d3db4a20..dc120260d 100644 --- 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 @@ -28,7 +28,12 @@ class Migration(migrations.Migration): ("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;', + 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", From 5f2a0a3c1fdab5db3a1ccec59e0d927df352a30b Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 1 Mar 2024 23:00:45 +0530 Subject: [PATCH 12/52] [fix] Fixed issues with DeviceAdmin.get_extra_context --- openwisp_controller/config/admin.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 591a47322..6cf809e53 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -101,7 +101,7 @@ class Media: for file_ in ('preview.js', 'unsaved_changes.js', 'switcher.js') ] - def get_extra_context(self, request, pk=None): + def get_extra_context(self, pk=None): prefix = 'admin:{0}_{1}'.format( self.opts.app_label, self.model.__name__.lower() ) @@ -136,7 +136,7 @@ def get_extra_context(self, request, pk=None): def add_view(self, request, form_url='', extra_context=None): extra_context = extra_context or {} - extra_context.update(self.get_extra_context(request)) + extra_context.update(self.get_extra_context()) instance = self.model() if hasattr(instance, 'get_default_templates'): templates = instance.get_default_templates() @@ -145,7 +145,7 @@ def add_view(self, request, form_url='', extra_context=None): return super().add_view(request, form_url, extra_context) def change_view(self, request, object_id, form_url='', extra_context=None): - extra_context = self.get_extra_context(request, object_id) + extra_context = self.get_extra_context(object_id) return super().change_view(request, object_id, form_url, extra_context) def get_urls(self): @@ -742,10 +742,10 @@ def get_urls(self): pass return urls - def get_extra_context(self, request, pk=None): - ctx = super().get_extra_context(request, pk) + def get_extra_context(self, pk=None): + ctx = super().get_extra_context(pk) if pk: - device = self.get_object(request, pk) + device = self.model.objects.select_related('config').get(id=pk) ctx.update( { 'show_deactivate': not device.is_deactivated(), @@ -767,7 +767,7 @@ def get_extra_context(self, request, pk=None): return ctx def add_view(self, request, form_url='', extra_context=None): - extra_context = self.get_extra_context(request) + extra_context = self.get_extra_context() return super().add_view(request, form_url, extra_context) def get_inlines(self, request, obj): @@ -1141,14 +1141,14 @@ def schema_view(self, request): def add_view(self, request, form_url='', extra_context=None): extra_context = extra_context or {} - extra_context.update(self.get_extra_context(request)) + extra_context.update(self.get_extra_context()) return super().add_view(request, form_url, extra_context) def change_view(self, request, object_id, form_url='', extra_context=None): - extra_context = self.get_extra_context(request, object_id) + extra_context = self.get_extra_context(object_id) return super().change_view(request, object_id, form_url, extra_context) - def get_extra_context(self, request, pk=None): + def get_extra_context(self, pk=None): ctx = { 'relevant_template_url': reverse( 'admin:get_relevant_templates', args=['org_id'] From 318b16910034c25405a9a93514580ebc2f1ea353 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 1 Mar 2024 23:01:05 +0530 Subject: [PATCH 13/52] [feature] Added "config_deactivating" signal --- openwisp_controller/config/apps.py | 5 +++++ openwisp_controller/config/base/config.py | 16 ++++++++++++++++ openwisp_controller/config/signals.py | 4 ++++ openwisp_controller/connection/apps.py | 5 ++++- 4 files changed, 29 insertions(+), 1 deletion(-) diff --git a/openwisp_controller/config/apps.py b/openwisp_controller/config/apps.py index 2e7e4add9..2d2961080 100644 --- a/openwisp_controller/config/apps.py +++ b/openwisp_controller/config/apps.py @@ -23,6 +23,7 @@ from .signals import ( config_backend_changed, config_deactivated, + config_deactivating, config_modified, device_group_changed, device_name_changed, @@ -310,6 +311,10 @@ def enable_cache_invalidation(self): 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', diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 4824c0c16..47a8d48ef 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -17,6 +17,7 @@ from ..signals import ( config_backend_changed, config_deactivated, + config_deactivating, config_modified, config_status_changed, ) @@ -111,6 +112,7 @@ def __init__(self, *args, **kwargs): 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): @@ -498,6 +500,8 @@ 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 @@ -549,6 +553,17 @@ 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. @@ -609,6 +624,7 @@ def set_status_deactivating(self, save=True): clears configuration and templates. """ self.config = {} + self._send_config_deactivating = True self._set_status('deactivating', save, extra_update_fields=['config']) self.templates.clear() diff --git a/openwisp_controller/config/signals.py b/openwisp_controller/config/signals.py index f75ff293d..eac00ad0c 100644 --- a/openwisp_controller/config/signals.py +++ b/openwisp_controller/config/signals.py @@ -21,6 +21,10 @@ 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'] 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, From 02395be34643e99b3ee728a38a579564fe5b590c Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Mon, 4 Mar 2024 22:58:23 +0530 Subject: [PATCH 14/52] [admin] Added error handling to activate and deactivate actions --- openwisp_controller/config/admin.py | 51 ++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 6cf809e53..8e2073143 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -19,6 +19,7 @@ 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 flat_json_widget.widgets import FlatJsonWidget from import_export.admin import ImportExportMixin @@ -653,6 +654,18 @@ 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, + ) + def _change_device_status(self, request, queryset, method): """ This helper method provides re-usability of code for @@ -670,8 +683,44 @@ def _change_device_status(self, request, queryset, method): ' The operation was rejected.' ) return HttpResponseForbidden() + success_devices = [] for device in queryset.iterator(): - getattr(device, method)() + try: + getattr(device, method)() + except Exception: + self.message_user( + request, + _('An error occurred while trying to "%(method)s" "%(device)s.') + % {'method': method, 'device': device}, + messages.ERROR, + ) + else: + success_devices.append(self._get_device_path(device)) + if not success_devices: + # There were zero successful devices, return + return + if len(success_devices) == 1: + self.message_user( + request, + format_html( + _('The device {device} was {method}d successfully'), + device=self._get_device_path(device), + method=method, + ), + messages.SUCCESS, + ) + else: + devices_html = ', '.join(success_devices[0:-1]) + devices_html = f'{devices_html} and {success_devices[-1]}' + self.message_user( + request, + format_html( + _('The following devices were {method}d successfully: {devices}'), + method=method, + devices=mark_safe(devices_html), + ), + messages.SUCCESS, + ) @admin.actions(description=_('Deactivate selected devices'), permissions=['change']) def deactivate_device(self, request, queryset): From 332cf502f3907b87199aede6f8157489d6b90723 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 5 Mar 2024 19:39:40 +0530 Subject: [PATCH 15/52] [change] Show delete action after deactivate action --- openwisp_controller/config/admin.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 8e2073143..d6470f71f 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -515,7 +515,12 @@ class DeviceAdmin(MultitenantAdminMixin, BaseConfigAdmin, UUIDAdmin): ] inlines = [ConfigInline] conditional_inlines = [] - actions = ['deactivate_device', 'change_group', 'activate_device'] + 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 From 8bc75f164b825ecddde0468586fa8292d09bb714 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 5 Mar 2024 19:40:01 +0530 Subject: [PATCH 16/52] [feature] Emit device_deactivated signal when device is deactivated --- openwisp_controller/config/base/device.py | 8 +++++++- openwisp_controller/config/signals.py | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py index 8042767d9..701d7fab5 100644 --- a/openwisp_controller/config/base/device.py +++ b/openwisp_controller/config/base/device.py @@ -10,7 +10,12 @@ 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_deactivated, + device_group_changed, + device_name_changed, + management_ip_changed, +) from ..validators import device_name_validator, mac_address_validator from .base import BaseModel @@ -180,6 +185,7 @@ def deactivate(self): self.config.set_status_deactivating() self._is_deactivated = True self.save() + device_deactivated.send(sender=self.__class__, instance=self) def activate(self): if not self.is_deactivated(): diff --git a/openwisp_controller/config/signals.py b/openwisp_controller/config/signals.py index eac00ad0c..7224c9c6b 100644 --- a/openwisp_controller/config/signals.py +++ b/openwisp_controller/config/signals.py @@ -29,6 +29,10 @@ device_registered.__doc__ = """ Providing arguments: ['instance', 'is_new'] """ +device_deactivated = Signal() +device_deactivated.__doc__ = """ +Providing arguments: ['instance'] +""" management_ip_changed = Signal() management_ip_changed.__doc__ = """ Providing arguments: ['instance', 'management_ip', 'old_management_ip'] From eb363d1aed129834049e4380b584268d1ab103d7 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 5 Mar 2024 20:51:38 +0530 Subject: [PATCH 17/52] [fix] Fixed activate button on top submitting device-form --- openwisp_controller/config/admin.py | 1 + .../admin/config/device/change_form.html | 23 +++++++++++++++ .../admin/config/device/submit_line.html | 29 ++++++++++--------- 3 files changed, 39 insertions(+), 14 deletions(-) create mode 100644 openwisp_controller/config/templates/admin/config/device/change_form.html diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index d6470f71f..385d41980 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -468,6 +468,7 @@ def __init__(self, org_id, **kwargs): class DeviceAdmin(MultitenantAdminMixin, BaseConfigAdmin, UUIDAdmin): + change_form_template = None list_display = [ 'name', 'backend', 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..c52942b46 --- /dev/null +++ b/openwisp_controller/config/templates/admin/config/device/change_form.html @@ -0,0 +1,23 @@ +{% extends "admin/config/change_form.html" %} +{% load admin_urls %} + +{% 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") + 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/submit_line.html b/openwisp_controller/config/templates/admin/config/device/submit_line.html index 3ff0831d9..2bc6b3dd7 100644 --- a/openwisp_controller/config/templates/admin/config/device/submit_line.html +++ b/openwisp_controller/config/templates/admin/config/device/submit_line.html @@ -1,19 +1,20 @@ {% extends "admin/submit_line.html" %} -{% load i18n l10n admin_urls %} +{% load i18n l10n %} {% block submit-row %} -{% url opts|admin_urlname:'changelist' as changelist_url %} -
- {% csrf_token %} - - {% if show_activate %} - - - {% endif %} - {% if show_deactivate %} - - - {% endif %} -
+{% comment %} +The form element "act_deact_device" for these elements is present +in admin/config/device/change_form.html. Refer this file for +explanation. +{% endcomment %} + +{% if show_activate %} + + +{% endif %} +{% if show_deactivate %} + + +{% endif %} {{ block.super }} {% endblock %} From b2e83254be697e5eb10c65d9f64ef57d0be144c9 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 16:01:50 +0530 Subject: [PATCH 18/52] [req-changes] Added deactivated warning message to device's change page --- .../templates/admin/config/device/change_form.html | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/openwisp_controller/config/templates/admin/config/device/change_form.html b/openwisp_controller/config/templates/admin/config/device/change_form.html index c52942b46..c0814934b 100644 --- a/openwisp_controller/config/templates/admin/config/device/change_form.html +++ b/openwisp_controller/config/templates/admin/config/device/change_form.html @@ -1,5 +1,14 @@ {% extends "admin/config/change_form.html" %} -{% load admin_urls %} +{% load admin_urls i18n %} + +{% block messages %} + {{ block.super }} + {% if original and original.is_deactivated %} +
    +
  • {% trans "This device has been deactivated." %}
  • +
+ {% endif %} +{% endblock messages %} {% block content %} {% comment %} From 091de5e7c464e460c3ba1571adb3084725ba517c Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 16:02:54 +0530 Subject: [PATCH 19/52] [change] Upadted Config.status helptext --- openwisp_controller/config/base/config.py | 5 ++++- .../config/migrations/0055_alter_config_status.py | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 47a8d48ef..1f7de4e7a 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -74,7 +74,10 @@ class AbstractConfig(BaseConfig): 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 all the' + ' configuration is being removed; \n' + '"deactivated" means the configuration has been removed from the device;' ), ) error_reason = models.CharField( diff --git a/openwisp_controller/config/migrations/0055_alter_config_status.py b/openwisp_controller/config/migrations/0055_alter_config_status.py index bd8399a1a..3c01c69c7 100644 --- a/openwisp_controller/config/migrations/0055_alter_config_status.py +++ b/openwisp_controller/config/migrations/0055_alter_config_status.py @@ -28,6 +28,10 @@ class Migration(migrations.Migration): ' \n"applied" means the configuration is applied successfully;' ' \n"error" means the configuration caused issues and it was' ' rolled back;' + '"deactivating" means the device has been deactivated and all the' + ' configuration is being removed; \n' + '"deactivated" means the configuration has been removed from' + ' the device;' ), max_length=100, no_check_for_status=True, From 6da3756c7b73819cb85b462230cf342420d76f06 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 16:04:26 +0530 Subject: [PATCH 20/52] [fix] Don't show any extra form on deactivated devices --- openwisp_controller/config/admin.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 385d41980..cf1d3b469 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -87,6 +87,11 @@ def has_change_permission(self, request, obj=None): return perm return perm and not obj.is_deactivated() + 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' @@ -754,6 +759,7 @@ def ip(self, obj): ip.short_description = _('IP address') def config_status(self, obj): + # if obj return obj.config.status config_status.short_description = _('config status') From eb6c36dd24b683bc1b9c6dc5c8deda6ac8896945 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 16:10:25 +0530 Subject: [PATCH 21/52] [tests] Added admin tests for device change page --- .../config/tests/test_admin.py | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 737a5e8ad..54ba45dc8 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -1951,6 +1951,124 @@ def tearDownClass(cls): devnull.close() +class TestTransactionAdmin( + CreateConfigTemplateMixin, + TestAdminMixin, + TestOrganizationMixin, + 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 ( + '' + ) + + 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 + """ + 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=3) + # Verify deleting existing Inline objects is not allowed + self.assertNotContains(response, ' Date: Thu, 7 Mar 2024 16:30:45 +0530 Subject: [PATCH 22/52] [refactor] Refactored logic for sending activate/deactivate message to user --- openwisp_controller/config/admin.py | 69 ++++++++++++++++------------- 1 file changed, 38 insertions(+), 31 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index cf1d3b469..8ccdc29b0 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -677,6 +677,38 @@ def _get_device_path(self, device): device, ) + def _message_user_device_status(self, request, devices, method, message_level): + if not devices: + return + if len(devices) == 1: + devices_html = devices[0] + if message_level == messages.SUCCESS: + message = _('The device {devices_html} was {method}ed successfully') + elif message_level == messages.ERROR: + message = _( + 'An error occurred while {method}ing the device {devices_html}' + ) + else: + devices_html = mark_safe(', '.join(devices[:-1]) + ' and ' + devices[-1]) + if message_level == messages.SUCCESS: + message = _( + 'The following devices were {method}ed successfully: {devices_html}' + ) + elif message_level == messages.ERROR: + message = _( + 'An error occurred while {method}ing the following' + ' devices: {devices_html}' + ) + self.message_user( + request, + format_html( + message, + devices_html=devices_html, + method=method[:-1], + ), + message_level, + ) + def _change_device_status(self, request, queryset, method): """ This helper method provides re-usability of code for @@ -695,43 +727,18 @@ def _change_device_status(self, request, queryset, method): ) return HttpResponseForbidden() success_devices = [] + error_devices = [] for device in queryset.iterator(): try: getattr(device, method)() except Exception: - self.message_user( - request, - _('An error occurred while trying to "%(method)s" "%(device)s.') - % {'method': method, 'device': device}, - messages.ERROR, - ) + error_devices.append(self._get_device_path(device)) else: success_devices.append(self._get_device_path(device)) - if not success_devices: - # There were zero successful devices, return - return - if len(success_devices) == 1: - self.message_user( - request, - format_html( - _('The device {device} was {method}d successfully'), - device=self._get_device_path(device), - method=method, - ), - messages.SUCCESS, - ) - else: - devices_html = ', '.join(success_devices[0:-1]) - devices_html = f'{devices_html} and {success_devices[-1]}' - self.message_user( - request, - format_html( - _('The following devices were {method}d successfully: {devices}'), - method=method, - devices=mark_safe(devices_html), - ), - messages.SUCCESS, - ) + self._message_user_device_status( + request, success_devices, method, messages.SUCCESS + ) + self._message_user_device_status(request, error_devices, method, messages.ERROR) @admin.actions(description=_('Deactivate selected devices'), permissions=['change']) def deactivate_device(self, request, queryset): From 8dc6096882c474eee75154ea28edc34364b917d4 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 18:31:42 +0530 Subject: [PATCH 23/52] [tests] Added test for device changelist admin action --- openwisp_controller/config/admin.py | 16 +- .../config/tests/test_admin.py | 227 ++++++++++++++++++ 2 files changed, 229 insertions(+), 14 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 8ccdc29b0..8a857fc82 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -683,10 +683,10 @@ def _message_user_device_status(self, request, devices, method, message_level): if len(devices) == 1: devices_html = devices[0] if message_level == messages.SUCCESS: - message = _('The device {devices_html} was {method}ed successfully') + message = _('The device {devices_html} was {method}ed successfully.') elif message_level == messages.ERROR: message = _( - 'An error occurred while {method}ing the device {devices_html}' + 'An error occurred while {method}ing the device {devices_html}.' ) else: devices_html = mark_safe(', '.join(devices[:-1]) + ' and ' + devices[-1]) @@ -714,18 +714,6 @@ def _change_device_status(self, request, queryset, method): This helper method provides re-usability of code for device activation and deactivation actions. """ - # Validate selected devices can be managed by the user - if not request.user.is_superuser: - # There could be multiple devices selected by the user. - # Validate that all the devices can be managed by the user. - for org_id in set(queryset.values_list('organization_id', flat=True)): - if not request.user.is_manager(str(org_id)): - logger.warning( - f'{request.user} attempted to deactivate device of "{org_id}"' - ' organization which they do not manage.' - ' The operation was rejected.' - ) - return HttpResponseForbidden() success_devices = [] error_devices = [] for device in queryset.iterator(): diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 54ba45dc8..54029063c 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 @@ -1950,6 +1951,58 @@ 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, @@ -2068,6 +2121,180 @@ def test_device_without_config_change_activate_deactivate(self): # Verify adding a new inline objects is not allowed self.assertContains(response, '-TOTAL_FORMS" value="0"', count=4) + def _test_device_changelist_activate_deactivate_admin_action( + self, method='activate', is_initially_deactivated=True + ): + """ + This helper function is used by + test_device_changelist_activate_admin_action and + test_device_changelist_deactivate_admin_action test cases. + It verifies that activate/deactivate operation works as expected. + It also verifies the success/error operation for the operation. + """ + org = self._get_org() + device1 = self._create_device( + organization=org, _is_deactivated=is_initially_deactivated + ) + device2 = self._create_device( + name='device2', + mac_address='11:22:33:44:55:77', + organization=org, + _is_deactivated=is_initially_deactivated, + ) + device3 = self._create_device( + name='device3', + mac_address='11:22:33:44:55:88', + organization=org, + _is_deactivated=is_initially_deactivated, + ) + html_method = method[:-1] + single_success_message_html = ( + f'
  • 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, From acde7046d360562d75084b92e512218a0d7feaf3 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 22:35:55 +0530 Subject: [PATCH 24/52] [temp] Upgraded openwisp-utils --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 0c0918816..771030bba 100644 --- a/requirements.txt +++ b/requirements.txt @@ -6,7 +6,7 @@ django-x509 @ https://github.com/openwisp/django-x509/tarball/master django-loci @ https://github.com/openwisp/django-loci/tarball/master django-flat-json-widget @ https://github.com/openwisp/django-flat-json-widget/tarball/master openwisp-users @ https://github.com/openwisp/openwisp-users/tarball/master -openwisp-utils[celery] @ https://github.com/openwisp/openwisp-utils/tarball/master +openwisp-utils[celery] @ https://github.com/openwisp/openwisp-utils/tarball/extendable-submit-line openwisp-notifications @ https://github.com/openwisp/openwisp-notifications/tarball/master openwisp-ipam @ https://github.com/openwisp/openwisp-ipam/tarball/master djangorestframework-gis~=1.0 From 8df3e0bae03778a612e568f8130e47109c1b48f8 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 22:41:04 +0530 Subject: [PATCH 25/52] [chores] Added migrations --- .../migrations/0056_alter_config_status.py | 32 +++++++++++++++++++ .../migrations/0007_alter_config_status.py | 32 +++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 openwisp_controller/config/migrations/0056_alter_config_status.py create mode 100644 tests/openwisp2/sample_config/migrations/0007_alter_config_status.py diff --git a/openwisp_controller/config/migrations/0056_alter_config_status.py b/openwisp_controller/config/migrations/0056_alter_config_status.py new file mode 100644 index 000000000..a8b557821 --- /dev/null +++ b/openwisp_controller/config/migrations/0056_alter_config_status.py @@ -0,0 +1,32 @@ +# Generated by Django 4.2.11 on 2024-03-07 17:09 + +from django.db import migrations +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ("config", "0055_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 all 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/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..1ddab9bff --- /dev/null +++ b/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py @@ -0,0 +1,32 @@ +# Generated by Django 4.2.11 on 2024-03-07 17:10 + +from django.db import migrations +import model_utils.fields + + +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 all 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", + ), + ), + ] From a6148b4abaadc272655723be465c559045b04bcf Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 22:43:05 +0530 Subject: [PATCH 26/52] [chores] Upgraded openwisp-utils --- .github/workflows/ci.yml | 1 + requirements.txt | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c94dbedec..b208dbec4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,6 +78,7 @@ jobs: pip install -U pip wheel setuptools pip install -U -r requirements-test.txt pip install -U -e . + pip install -U --force-reinstall --no-deps https://github.com/openwisp/openwisp-utils/tarball/extendable-submit-line pip install ${{ matrix.django-version }} - name: QA checks diff --git a/requirements.txt b/requirements.txt index 771030bba..0c0918816 100644 --- a/requirements.txt +++ b/requirements.txt @@ -6,7 +6,7 @@ django-x509 @ https://github.com/openwisp/django-x509/tarball/master django-loci @ https://github.com/openwisp/django-loci/tarball/master django-flat-json-widget @ https://github.com/openwisp/django-flat-json-widget/tarball/master openwisp-users @ https://github.com/openwisp/openwisp-users/tarball/master -openwisp-utils[celery] @ https://github.com/openwisp/openwisp-utils/tarball/extendable-submit-line +openwisp-utils[celery] @ https://github.com/openwisp/openwisp-utils/tarball/master openwisp-notifications @ https://github.com/openwisp/openwisp-notifications/tarball/master openwisp-ipam @ https://github.com/openwisp/openwisp-ipam/tarball/master djangorestframework-gis~=1.0 From 5339254a5c94a2f94f9726907905c83aade0a62f Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 7 Mar 2024 22:53:01 +0530 Subject: [PATCH 27/52] [chores] Fixed formatting --- .../config/migrations/0056_alter_config_status.py | 14 ++++++++++++-- .../migrations/0007_alter_config_status.py | 14 ++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/openwisp_controller/config/migrations/0056_alter_config_status.py b/openwisp_controller/config/migrations/0056_alter_config_status.py index a8b557821..565a7a680 100644 --- a/openwisp_controller/config/migrations/0056_alter_config_status.py +++ b/openwisp_controller/config/migrations/0056_alter_config_status.py @@ -1,7 +1,7 @@ # Generated by Django 4.2.11 on 2024-03-07 17:09 -from django.db import migrations import model_utils.fields +from django.db import migrations class Migration(migrations.Migration): @@ -23,7 +23,17 @@ class Migration(migrations.Migration): ("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 all the configuration is being removed; \n"deactivated" means the configuration has been removed from the device;', + 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' + ' all 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/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py b/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py index 1ddab9bff..cab185d0b 100644 --- a/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py +++ b/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py @@ -1,7 +1,7 @@ # Generated by Django 4.2.11 on 2024-03-07 17:10 -from django.db import migrations import model_utils.fields +from django.db import migrations class Migration(migrations.Migration): @@ -23,7 +23,17 @@ class Migration(migrations.Migration): ("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 all the configuration is being removed; \n"deactivated" means the configuration has been removed from the device;', + 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' + ' all 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", From 2710b46d79f460038f345b9da1736189773f7bb4 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 4 Apr 2024 21:08:42 +0530 Subject: [PATCH 28/52] [req-changes] Updated config status on device admin --- openwisp_controller/config/admin.py | 8 +++-- .../config/tests/test_admin.py | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 8a857fc82..819aca1dc 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -754,8 +754,12 @@ def ip(self, obj): ip.short_description = _('IP address') def config_status(self, obj): - # if 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') diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 54029063c..a37182f4c 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -1468,6 +1468,40 @@ 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_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.set_status_deactivating() + 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) From b6b3facba843e001b2bca1e8f21bc70493c67744 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 4 Apr 2024 22:22:53 +0530 Subject: [PATCH 29/52] [req-changes] Refactored code for device status message --- openwisp_controller/config/admin.py | 61 +++++++++++++------ .../config/tests/test_admin.py | 4 +- 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 819aca1dc..3feca012a 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -21,6 +21,7 @@ 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 @@ -677,34 +678,54 @@ def _get_device_path(self, device): 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] - if message_level == messages.SUCCESS: - message = _('The device {devices_html} was {method}ed successfully.') - elif message_level == messages.ERROR: - message = _( - 'An error occurred while {method}ing the device {devices_html}.' - ) else: - devices_html = mark_safe(', '.join(devices[:-1]) + ' and ' + devices[-1]) - if message_level == messages.SUCCESS: - message = _( - 'The following devices were {method}ed successfully: {devices_html}' - ) - elif message_level == messages.ERROR: - message = _( - 'An error occurred while {method}ing the following' - ' devices: {devices_html}' - ) + devices_html = ', '.join(devices[:-1]) + ' and ' + devices[-1] + message = self._device_status_messages[method][message_level] self.message_user( request, - format_html( - message, - devices_html=devices_html, - method=method[:-1], + mark_safe( + message % {'devices_html': devices_html, 'devices': len(devices)} ), message_level, ) diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index a37182f4c..177ca811c 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -2192,7 +2192,7 @@ def _test_device_changelist_activate_deactivate_admin_action( f' successfully: ' f'{device1.name}, ' f'{device2.name} and {device3.name}' + f'change/">{device3.name}.' ) single_error_message_html = ( f'
  • An error occurred while {html_method}ing the device' @@ -2204,7 +2204,7 @@ def _test_device_changelist_activate_deactivate_admin_action( f' devices: ' f'{device1.name}, {device2.name} and {device3.name}
  • ' + f'{device3.id}/change/">{device3.name}.' ) path = reverse(f'admin:{self.app_label}_device_changelist') From 6942cac5f3174175bc31108067024995dbbb2f6d Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Sat, 6 Apr 2024 01:30:05 +0530 Subject: [PATCH 30/52] [req-changes] Display warning when user delete active devices from admin action --- openwisp_controller/config/admin.py | 17 +++++++++ .../device/delete_selected_confirmation.html | 36 +++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 openwisp_controller/config/templates/admin/config/device/delete_selected_confirmation.html diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 3feca012a..1c449d165 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -757,6 +757,23 @@ def deactivate_device(self, request, queryset): 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 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 %} From 3e8bcf7d6802b6cfd253bc895e6810e01b28b26c Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 1 Aug 2024 21:25:42 +0530 Subject: [PATCH 31/52] [chores] Miscellaneous uppdates --- openwisp_controller/config/admin.py | 4 ++-- openwisp_controller/config/tests/test_admin.py | 5 ++--- openwisp_controller/config/tests/test_device.py | 1 - openwisp_controller/connection/admin.py | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 1c449d165..c51848767 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -749,11 +749,11 @@ def _change_device_status(self, request, queryset, method): ) self._message_user_device_status(request, error_devices, method, messages.ERROR) - @admin.actions(description=_('Deactivate selected devices'), permissions=['change']) + @admin.action(description=_('Deactivate selected devices'), permissions=['change']) def deactivate_device(self, request, queryset): self._change_device_status(request, queryset, 'deactivate') - @admin.actions(description=_('Activate selected devices'), permissions=['change']) + @admin.action(description=_('Activate selected devices'), permissions=['change']) def activate_device(self, request, queryset): self._change_device_status(request, queryset, 'activate') diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 177ca811c..be22d2f95 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -2041,7 +2041,6 @@ def test_device_changelist_activate_deactivate_admin_action_security( class TestTransactionAdmin( CreateConfigTemplateMixin, TestAdminMixin, - TestOrganizationMixin, TransactionTestCase, ): app_label = 'config' @@ -2115,7 +2114,7 @@ def test_device_with_config_change_deactivate_deactivate(self): 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=3) + self.assertContains(response, '-TOTAL_FORMS" value="0"', count=2) # Verify deleting existing Inline objects is not allowed self.assertNotContains(response, ' Date: Thu, 1 Aug 2024 23:55:01 +0530 Subject: [PATCH 32/52] [fix] Fixed selenium test --- openwisp_controller/config/admin.py | 8 ++++++-- openwisp_controller/tests/test_selenium.py | 7 +++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index c51848767..ba0e9702e 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -84,7 +84,7 @@ def has_add_permission(self, request, obj): def has_change_permission(self, request, obj=None): perm = super().has_change_permission(request) - if not obj: + if not obj or getattr(request, '_recover_view', False): return perm return perm and not obj.is_deactivated() @@ -550,7 +550,7 @@ class Media(BaseConfigAdmin.Media): def has_change_permission(self, request, obj=None): perm = super().has_change_permission(request) - if not obj: + if not obj or getattr(request, '_recover_view', False): return perm return perm and not obj.is_deactivated() @@ -868,6 +868,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/tests/test_selenium.py b/openwisp_controller/tests/test_selenium.py index 8e90773fb..62f5b506a 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]) ) @@ -79,7 +82,7 @@ def test_restoring_deleted_device(self, *args): for error in self.web_driver.get_log('browser'): self.assertNotEqual(error['level'], 'WARNING') self.web_driver.find_element( - by=By.XPATH, value='//*[@id="device_form"]/div/div[1]/input[1]' + by=By.XPATH, value='//*[@id="device_form"]/div/div[1]/input[2]' ).click() try: WebDriverWait(self.web_driver, 5).until( From de5a90ac2636a23a3933dcb1398d1b87b92d7344 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 2 Aug 2024 16:02:03 +0530 Subject: [PATCH 33/52] WIP: 6eea7bc2 [fix] Fixed selenium test --- openwisp_controller/config/admin.py | 2 +- openwisp_controller/config/base/config.py | 25 ++++++++++++----------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index ba0e9702e..05977178b 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -78,7 +78,7 @@ class BaseAdmin(TimeReadonlyAdminMixin, ModelAdmin): class DeactivatedDeviceReadOnlyMixin(object): def has_add_permission(self, request, obj): perm = super().has_add_permission(request, obj) - if not obj: + if not obj or getattr(request, '_recover_view', False): return perm return perm and not obj.is_deactivated() diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 1f7de4e7a..87d298b94 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -247,14 +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 """ - if ( - instance._state.adding - or (action not in ['post_add', 'post_remove']) - and not ( - action == 'post_clear' and instance.is_deactivating_or_deactivated() - ) - ): + 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 config is being deactivating or deactivated, 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): @@ -268,11 +274,6 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs): # coming from admin ModelForm else: templates = pk_set - if action == 'post_clear' and instance.is_deactivating_or_deactivated(): - # If the config is being deactivting or deactivated, then - # delete all vpn clients and return. - instance.vpnclient_set.all().delete() - return # delete VPN clients which have been cleared # by sortedm2m and have not been added back if action == 'post_add': From bdb4485c0c8e80d4d280bdcba06ef968ffd6dd0c Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 2 Aug 2024 18:27:10 +0530 Subject: [PATCH 34/52] [fix] Fixed implementation of submit inline buttons --- openwisp_controller/config/admin.py | 25 ++++++++++++++++++- .../admin/config/device/change_form.html | 8 +++--- .../admin/config/device/submit_line.html | 20 --------------- .../config/tests/test_admin.py | 6 ++--- 4 files changed, 31 insertions(+), 28 deletions(-) delete mode 100644 openwisp_controller/config/templates/admin/config/device/submit_line.html diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 05977178b..3da8d47e0 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -474,7 +474,10 @@ def __init__(self, org_id, **kwargs): class DeviceAdmin(MultitenantAdminMixin, BaseConfigAdmin, UUIDAdmin): - change_form_template = None + change_form_template = 'admin/config/device/change_form.html' + delete_selected_confirmation_template = ( + 'admin/config/device/delete_selected_confirmation.html' + ) list_display = [ 'name', 'backend', @@ -851,6 +854,26 @@ def get_extra_context(self, pk=None): 'action_checkbox_name': helpers.ACTION_CHECKBOX_NAME, } ) + if device.is_deactivated(): + ctx['additional_buttons'].append( + { + 'html': mark_safe( + '' + ) + } + ) + else: + ctx['additional_buttons'].append( + { + 'html': mark_safe( + '' + ) + } + ) ctx.update( { 'relevant_template_url': reverse( diff --git a/openwisp_controller/config/templates/admin/config/device/change_form.html b/openwisp_controller/config/templates/admin/config/device/change_form.html index c0814934b..e97999f75 100644 --- a/openwisp_controller/config/templates/admin/config/device/change_form.html +++ b/openwisp_controller/config/templates/admin/config/device/change_form.html @@ -1,5 +1,5 @@ {% extends "admin/config/change_form.html" %} -{% load admin_urls i18n %} +{% load admin_urls i18n l10n %} {% block messages %} {{ block.super }} @@ -16,7 +16,7 @@ workaround for activating and deactivating device operations within the change form. - We utilize a distinct form element (id="act_deact_device") + 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 @@ -25,8 +25,10 @@ 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/submit_line.html b/openwisp_controller/config/templates/admin/config/device/submit_line.html deleted file mode 100644 index 2bc6b3dd7..000000000 --- a/openwisp_controller/config/templates/admin/config/device/submit_line.html +++ /dev/null @@ -1,20 +0,0 @@ -{% extends "admin/submit_line.html" %} -{% load i18n l10n %} - -{% block submit-row %} -{% comment %} -The form element "act_deact_device" for these elements is present -in admin/config/device/change_form.html. Refer this file for -explanation. -{% endcomment %} - -{% if show_activate %} - - -{% endif %} -{% if show_deactivate %} - - -{% endif %} -{{ block.super }} -{% endblock %} diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index be22d2f95..311fb2660 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -2049,11 +2049,9 @@ class TestTransactionAdmin( ) _deactivate_btn_html = ( '' - ) - _activate_btn_html = ( - '' + ' type="submit" value="Deactivate" form="act_deact_device_form">

    ' ) + _activate_btn_html = '' _save_btn_html = '' def setUp(self): From 26f058aa83769a3fb1604036334cb3360f891fef Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 2 Aug 2024 22:06:27 +0530 Subject: [PATCH 35/52] [tests] Fixed tests --- openwisp_controller/tests/test_selenium.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openwisp_controller/tests/test_selenium.py b/openwisp_controller/tests/test_selenium.py index 62f5b506a..fd451114f 100644 --- a/openwisp_controller/tests/test_selenium.py +++ b/openwisp_controller/tests/test_selenium.py @@ -82,7 +82,7 @@ def test_restoring_deleted_device(self, *args): for error in self.web_driver.get_log('browser'): self.assertNotEqual(error['level'], 'WARNING') self.web_driver.find_element( - by=By.XPATH, value='//*[@id="device_form"]/div/div[1]/input[2]' + by=By.XPATH, value='//*[@id="device_form"]/div/div[1]/input[1]' ).click() try: WebDriverWait(self.web_driver, 5).until( From e2edfb6004a38a14d6ad03157cd9c9c9ae8939d2 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 8 Aug 2024 15:13:29 +0530 Subject: [PATCH 36/52] [req-changes] Formatted code and updated docs --- docs/developer/utils.rst | 37 +++++++++++++++++++ .../migrations/0054_device__is_deactivated.py | 1 - .../migrations/0055_alter_config_status.py | 1 - .../migrations/0056_alter_config_status.py | 1 - .../config/tests/test_admin.py | 5 ++- openwisp_controller/geo/admin.py | 6 +-- ...ice__is_deactivated_alter_config_status.py | 1 - .../migrations/0007_alter_config_status.py | 1 - 8 files changed, 42 insertions(+), 11 deletions(-) diff --git a/docs/developer/utils.rst b/docs/developer/utils.rst index 996fe4fd8..9e91944bd 100644 --- a/docs/developer/utils.rst +++ b/docs/developer/utils.rst @@ -146,6 +146,43 @@ 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 deactivated. + .. _config_backend_changed: ``config_backend_changed`` diff --git a/openwisp_controller/config/migrations/0054_device__is_deactivated.py b/openwisp_controller/config/migrations/0054_device__is_deactivated.py index 2becdbfd9..3cb2739f6 100644 --- a/openwisp_controller/config/migrations/0054_device__is_deactivated.py +++ b/openwisp_controller/config/migrations/0054_device__is_deactivated.py @@ -4,7 +4,6 @@ class Migration(migrations.Migration): - dependencies = [ ('config', '0053_vpnclient_secret'), ] diff --git a/openwisp_controller/config/migrations/0055_alter_config_status.py b/openwisp_controller/config/migrations/0055_alter_config_status.py index 3c01c69c7..b8c8c04cf 100644 --- a/openwisp_controller/config/migrations/0055_alter_config_status.py +++ b/openwisp_controller/config/migrations/0055_alter_config_status.py @@ -5,7 +5,6 @@ class Migration(migrations.Migration): - dependencies = [ ("config", "0054_device__is_deactivated"), ] diff --git a/openwisp_controller/config/migrations/0056_alter_config_status.py b/openwisp_controller/config/migrations/0056_alter_config_status.py index 565a7a680..e65777880 100644 --- a/openwisp_controller/config/migrations/0056_alter_config_status.py +++ b/openwisp_controller/config/migrations/0056_alter_config_status.py @@ -5,7 +5,6 @@ class Migration(migrations.Migration): - dependencies = [ ("config", "0055_alter_config_status"), ] diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 311fb2660..fe6c2a942 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -2051,7 +2051,10 @@ class TestTransactionAdmin( '' ) - _activate_btn_html = '' + _activate_btn_html = ( + '' + ) _save_btn_html = '' def setUp(self): diff --git a/openwisp_controller/geo/admin.py b/openwisp_controller/geo/admin.py index da58e2e55..cb283c6b2 100644 --- a/openwisp_controller/geo/admin.py +++ b/openwisp_controller/geo/admin.py @@ -15,11 +15,7 @@ from openwisp_users.multitenancy import MultitenantOrgFilter from ..admin import MultitenantAdminMixin -from ..config.admin import ( - DeactivatedDeviceReadOnlyMixin, - DeviceAdmin, - DeviceAdminExportable, -) +from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdminExportable from .exportable import GeoDeviceResource DeviceLocation = load_model('geo', 'DeviceLocation') 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 index dc120260d..767a15cfd 100644 --- 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 @@ -5,7 +5,6 @@ class Migration(migrations.Migration): - dependencies = [ ("sample_config", "0005_add_organizationalloweddevice"), ] diff --git a/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py b/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py index cab185d0b..c00a6faf9 100644 --- a/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py +++ b/tests/openwisp2/sample_config/migrations/0007_alter_config_status.py @@ -5,7 +5,6 @@ class Migration(migrations.Migration): - dependencies = [ ("sample_config", "0006_device__is_deactivated_alter_config_status"), ] From ecccf77c29167cbc99de37626faa71bb165d3111 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 8 Aug 2024 23:48:41 +0530 Subject: [PATCH 37/52] [change] Clear management IP when device is deactivated --- openwisp_controller/config/admin.py | 17 +++++++++++------ openwisp_controller/config/base/device.py | 1 + openwisp_controller/config/tests/test_device.py | 3 ++- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 3da8d47e0..3860dedd6 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -76,17 +76,22 @@ class BaseAdmin(TimeReadonlyAdminMixin, ModelAdmin): class DeactivatedDeviceReadOnlyMixin(object): - def has_add_permission(self, request, obj): - perm = super().has_add_permission(request, obj) + 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) - if not obj or getattr(request, '_recover_view', False): - return perm - return perm and not obj.is_deactivated() + 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(): diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py index 701d7fab5..cf13d6c06 100644 --- a/openwisp_controller/config/base/device.py +++ b/openwisp_controller/config/base/device.py @@ -184,6 +184,7 @@ def deactivate(self): if self._has_config(): self.config.set_status_deactivating() self._is_deactivated = True + self.management_ip = '' self.save() device_deactivated.send(sender=self.__class__, instance=self) diff --git a/openwisp_controller/config/tests/test_device.py b/openwisp_controller/config/tests/test_device.py index c4e8dff6e..9fb53171a 100644 --- a/openwisp_controller/config/tests/test_device.py +++ b/openwisp_controller/config/tests/test_device.py @@ -620,12 +620,13 @@ def test_deactivating_device_with_config(self): self.assertEqual(config.templates.count(), 1) def test_deactivating_device_without_config(self): - device = self._create_device() + 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() From 1d43fb58377c2fe3b615d1bacf520f3381b27286 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 9 Aug 2024 00:33:50 +0530 Subject: [PATCH 38/52] [change] Added API endpoints for activating/deactivating device --- openwisp_controller/config/api/serializers.py | 2 ++ openwisp_controller/config/api/urls.py | 10 ++++++ openwisp_controller/config/api/views.py | 32 ++++++++++++++++++- openwisp_controller/config/tests/test_api.py | 18 +++++++++++ 4 files changed, 61 insertions(+), 1 deletion(-) 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..a77e6dcb4 100644 --- a/openwisp_controller/config/api/views.py +++ b/openwisp_controller/config/api/views.py @@ -4,12 +4,14 @@ 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 ...mixins import ProtectedAPIMixin @@ -95,6 +97,32 @@ class DeviceDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView): queryset = Device.objects.select_related('config', 'group', 'organization') +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): serializer_class = DeviceGroupSerializer queryset = DeviceGroup.objects.prefetch_related('templates').order_by('-created') @@ -240,6 +268,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/tests/test_api.py b/openwisp_controller/config/tests/test_api.py index 2327175be..55f8c8270 100644 --- a/openwisp_controller/config/tests/test_api.py +++ b/openwisp_controller/config/tests/test_api.py @@ -482,6 +482,24 @@ def test_device_download_api(self): r = self.client.get(path) self.assertEqual(r.status_code, 200) + 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) From 56e801c81eeadc3564c8d4af603a1ef5d923ca3b Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 9 Aug 2024 01:10:36 +0530 Subject: [PATCH 39/52] [change] Updated device delete API endpoint --- openwisp_controller/config/base/device.py | 9 +++++- openwisp_controller/config/tests/test_api.py | 33 ++++++++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py index cf13d6c06..4b40440c7 100644 --- a/openwisp_controller/config/base/device.py +++ b/openwisp_controller/config/base/device.py @@ -1,6 +1,6 @@ from hashlib import md5 -from django.core.exceptions import ObjectDoesNotExist, ValidationError +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 _ @@ -285,6 +285,13 @@ def save(self, *args, **kwargs): if not state_adding: self._check_changed_fields() + def delete(self, using=None, keep_parents=False): + if not self.is_deactivated() or ( + self._has_config() and not self.config.is_deactivated() + ): + raise PermissionDenied('The device should be deactivated before deleting') + 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 diff --git a/openwisp_controller/config/tests/test_api.py b/openwisp_controller/config/tests/test_api.py index 55f8c8270..0f2727334 100644 --- a/openwisp_controller/config/tests/test_api.py +++ b/openwisp_controller/config/tests/test_api.py @@ -501,12 +501,33 @@ def test_device_activate_api(self): 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) + 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) From 5d7f122ea6337cab1dcc931c3741c2e6b18c07bc Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 9 Aug 2024 20:17:30 +0530 Subject: [PATCH 40/52] [change] Disable API operations on deactivated devices --- openwisp_controller/config/base/device.py | 7 +- .../config/tests/test_admin.py | 9 ++ .../config/tests/test_config.py | 2 +- .../config/tests/test_controller.py | 2 +- .../config/tests/test_template.py | 2 +- openwisp_controller/config/tests/test_vpn.py | 8 +- openwisp_controller/connection/api/views.py | 20 +++- .../connection/tests/test_api.py | 101 +++++++++++++++++- openwisp_controller/geo/api/views.py | 16 ++- openwisp_controller/geo/tests/test_api.py | 72 +++++++++++-- openwisp_controller/mixins.py | 25 +++++ .../subnet_division/tests/test_models.py | 6 +- 12 files changed, 235 insertions(+), 35 deletions(-) diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py index 4b40440c7..2c7527f25 100644 --- a/openwisp_controller/config/base/device.py +++ b/openwisp_controller/config/base/device.py @@ -285,9 +285,10 @@ def save(self, *args, **kwargs): if not state_adding: self._check_changed_fields() - def delete(self, using=None, keep_parents=False): - if not self.is_deactivated() or ( - self._has_config() and not self.config.is_deactivated() + 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 should be deactivated before deleting') return super().delete(using, keep_parents) diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index fe6c2a942..7e9a1e878 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -1164,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]) diff --git a/openwisp_controller/config/tests/test_config.py b/openwisp_controller/config/tests/test_config.py index 3a7c007be..57f771d7f 100644 --- a/openwisp_controller/config/tests/test_config.py +++ b/openwisp_controller/config/tests/test_config.py @@ -681,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 b54f5fc3c..d2481f2ff 100644 --- a/openwisp_controller/config/tests/test_controller.py +++ b/openwisp_controller/config/tests/test_controller.py @@ -166,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() 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/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/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index 4499ddfe3..30b41e55d 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -260,6 +260,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) @@ -424,7 +453,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) @@ -437,7 +466,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 @@ -469,7 +498,7 @@ def test_put_devceconnection_detail(self): 'enabled': False, 'failure_reason': '', } - with self.assertNumQueries(14): + with self.assertNumQueries(16): response = self.client.put(path, data, content_type='application/json') self.assertEqual(response.status_code, 200) self.assertEqual( @@ -483,7 +512,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(15): response = self.client.patch(path, data, content_type='application/json') self.assertEqual(response.status_code, 200) self.assertEqual( @@ -494,7 +523,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(11): response = self.client.delete(path) self.assertEqual(response.status_code, 204) @@ -535,3 +564,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/api/views.py b/openwisp_controller/geo/api/views.py index 9d65d1fad..d7878a796 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,10 @@ 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 +80,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 +107,7 @@ def create_location(self, device): class DeviceLocationView( - ProtectedAPIMixin, + RelatedDeviceProtectedAPIMixin, generics.RetrieveUpdateDestroyAPIView, ): serializer_class = DeviceLocationSerializer @@ -120,6 +125,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..be5021063 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(22): 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(11): 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(13): 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(10): 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..767a50735 100644 --- a/openwisp_controller/mixins.py +++ b/openwisp_controller/mixins.py @@ -1,5 +1,30 @@ 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): + def _has_permissions(self, request, view, perm): + if request.method in self.READ_ONLY_METHOD: + return perm + return perm and not view.get_parent_queryset()[0].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) + + +class RelatedDeviceProtectedAPIMixin( + BaseProtectedAPIMixin, FilterByOrganizationManaged +): + permission_classes = [ + IsOrganizationManager, + RelatedDeviceModelPermission, + ] class ProtectedAPIMixin(BaseProtectedAPIMixin, FilterByOrganizationManaged): diff --git a/openwisp_controller/subnet_division/tests/test_models.py b/openwisp_controller/subnet_division/tests/test_models.py index cdd266656..eb9b13eeb 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, From 13c678336a0daca20372b6c4b2ea29e5cd39f691 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 9 Aug 2024 20:52:28 +0530 Subject: [PATCH 41/52] [tests] Fixed tests --- openwisp_controller/geo/api/views.py | 5 +---- tests/openwisp2/sample_config/api/views.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/openwisp_controller/geo/api/views.py b/openwisp_controller/geo/api/views.py index d7878a796..4adbaed0f 100644 --- a/openwisp_controller/geo/api/views.py +++ b/openwisp_controller/geo/api/views.py @@ -14,10 +14,7 @@ from openwisp_users.api.filters import OrganizationManagedFilter from openwisp_users.api.mixins import FilterByOrganizationManaged, FilterByParentManaged -from ...mixins import ( - ProtectedAPIMixin, - RelatedDeviceProtectedAPIMixin, -) +from ...mixins import ProtectedAPIMixin, RelatedDeviceProtectedAPIMixin from .filters import DeviceListFilter from .serializers import ( DeviceCoordinatesSerializer, 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() From 1afd75a038e871c5665f5c5a646d87d297556056 Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Thu, 7 Nov 2024 18:06:18 -0300 Subject: [PATCH 42/52] [ci] Removed branched openwisp-utils --- .github/workflows/ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b208dbec4..c94dbedec 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,7 +78,6 @@ jobs: pip install -U pip wheel setuptools pip install -U -r requirements-test.txt pip install -U -e . - pip install -U --force-reinstall --no-deps https://github.com/openwisp/openwisp-utils/tarball/extendable-submit-line pip install ${{ matrix.django-version }} - name: QA checks From 924c1d3b26d7d9d691c3aa7b8d43750ee3fdec8b Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 8 Nov 2024 23:17:03 +0530 Subject: [PATCH 43/52] [tests] Fixed failing tests --- openwisp_controller/config/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 3860dedd6..ddc3c84b5 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -862,7 +862,7 @@ def get_extra_context(self, pk=None): if device.is_deactivated(): ctx['additional_buttons'].append( { - 'html': mark_safe( + 'raw_html': mark_safe( '' ) @@ -871,7 +871,7 @@ def get_extra_context(self, pk=None): else: ctx['additional_buttons'].append( { - 'html': mark_safe( + 'raw_html': mark_safe( '