[change] Limit controller operations on disabled organizations #1393

.github/workflows/ci.yml

@@ -72,6 +72,8 @@ jobs:
           pip install -U pip wheel setuptools
           pip install -U -r requirements-test.txt
           pip install -U -e .
+          pip install --upgrade --force-reinstall --no-deps --no-cache-dir https://github.com/openwisp/openwisp-users/tarball/issues/522-disabled-org
+          pip install --upgrade --force-reinstall --no-deps --no-cache-dir https://github.com/openwisp/openwisp-ipam/tarball/fix-disabled-org-handling
           pip install ${{ matrix.django-version }}
 
       - name: Start redis

AGENTS.md

@@ -78,6 +78,8 @@ If instructions conflict (please let us know!), repository config and CI workflo
 
 - Watch for cross-organization data leaks, command execution issues, unsafe file paths, unsafe redirects, insecure credentials, and secrets.
 - Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data.
+- Objects belonging to a disabled organization must be readable and deletable; creation and updates must be blocked across all relevant write paths. This applies to objects with either a direct or chained/nested relationship to the organization. No other operations should be permitted, except for ordinary cleanup operations.
+- Operations on deactivated devices must be blocked, except for read-only access and cleanup operations required to maintain consistency. Creation, updates, provisioning, configuration, and other mutating operations must not be performed for deactivated devices.
 
 ## Troubleshooting
 

docs/user/device-config-status.rst

@@ -30,6 +30,8 @@ the device to revert to its previous working configuration.
 The device is in the process of being deactivated. The configuration is
 scheduled to be removed from the device.
 
+.. _controller_deactivated_config_status:
+
 ``deactivated``
 ---------------
 

openwisp_controller/config/admin.py

@@ -571,6 +571,7 @@ class DeviceAdmin(MultitenantAdminMixin, BaseConfigAdmin, CopyableFieldsAdmin):
         "activate_device",
         "delete_selected",
     ]
+    disabled_organization_action_exclusions = ("deactivate_device",)
     org_position = 1 if not app_settings.HARDWARE_ID_ENABLED else 2
     list_display.insert(org_position, "organization")
     _state_adding = False
@@ -601,7 +602,7 @@ def media(self):
         return super().media + forms.Media(js=js, css={"all": css})
 
     def has_change_permission(self, request, obj=None):
-        perm = super().has_change_permission(request)
+        perm = super().has_change_permission(request, obj)
         if not obj or getattr(request, "_recover_view", False):
             return perm
         return perm and not obj.is_deactivated()
@@ -726,8 +727,9 @@ def change_group(self, request, queryset):
         # Validate all selected devices belong to the same organization
         # which is managed by the user.
         org_id = None
-        if queryset:
-            org_id = queryset[0].organization_id
+        first_device = queryset.select_related("organization").first()
+        if first_device:
+            org_id = first_device.organization_id
         if not request.user.is_superuser and not request.user.is_manager(org_id):
             logger.warning(f'{request.user} does not manage "{org_id}" organization.')
             return HttpResponseForbidden()
@@ -738,6 +740,13 @@ def change_group(self, request, queryset):
                 messages.ERROR,
             )
             return HttpResponseRedirect(request.get_full_path())
+        if first_device and not first_device.organization.is_active:
+            self.message_user(
+                request,
+                _("Selected organization is disabled."),
+                messages.ERROR,
+            )
+            return HttpResponseRedirect(request.get_full_path())
 
         if "apply" in request.POST:
             form = ChangeDeviceGroupForm(data=request.POST, org_id=org_id)
@@ -868,7 +877,24 @@ def deactivate_device(self, request, queryset):
 
     @admin.action(description=_("Activate selected devices"), permissions=["change"])
     def activate_device(self, request, queryset):
-        self._change_device_status(request, queryset, "activate")
+        disabled_org_devices = list(queryset.filter(organization__is_active=False))
+        if disabled_org_devices:
+            devices_html = ", ".join(
+                self._get_device_path(device) for device in disabled_org_devices
+            )
+            self.message_user(
+                request,
+                mark_safe(
+                    _("Cannot activate devices of a disabled organization: %(devices)s")
+                    % {"devices": devices_html}
+                ),
+                messages.ERROR,
+            )
+        self._change_device_status(
+            request,
+            queryset.filter(organization__is_active=True),
+            "activate",
+        )
 
     @admin.action(description=delete_selected.short_description, permissions=["delete"])
     def delete_selected(self, request, queryset):
@@ -976,15 +1002,18 @@ def get_urls(self):
     def get_extra_context(self, pk=None):
         ctx = super().get_extra_context(pk)
         if pk:
-            device = self.model.objects.select_related("config").get(id=pk)
+            device = self.model.objects.select_related("config", "organization").get(
+                id=pk
+            )
             ctx.update(
                 {
                     "show_deactivate": not device.is_deactivated(),
-                    "show_activate": device.is_deactivated(),
+                    "show_activate": device.is_deactivated()
+                    and device.organization.is_active,
                     "action_checkbox_name": helpers.ACTION_CHECKBOX_NAME,
                 }
             )
-            if device.is_deactivated():
+            if ctx["show_activate"]:
                 ctx["additional_buttons"].append(
                     {
                         "raw_html": mark_safe(
@@ -993,7 +1022,7 @@ def get_extra_context(self, pk=None):
                         )
                     }
                 )
-            else:
+            elif not device.is_deactivated():
                 ctx["additional_buttons"].append(
                     {
                         "raw_html": mark_safe(
@@ -1178,10 +1207,19 @@ def save_clones(view, user, queryset, organization=None):
             # validate organization
             if organization:
                 try:
-                    validated_org = Organization.objects.get(pk=organization)
+                    validated_org = Organization.active.get(pk=organization)
                 except (ValidationError, Organization.DoesNotExist) as e:
                     logger.warning(
-                        f"Detected tampering in clone template form by user {user}: {e}"
+                        "Cannot clone template: the organization selected by "
+                        f"user {user} does not exist or is disabled: {e}"
+                    )
+                    view.message_user(
+                        request,
+                        _(
+                            "Cannot clone templates: the selected organization"
+                            " does not exist or is disabled."
+                        ),
+                        messages.ERROR,
                     )
                     return
                 if not user.is_superuser and not user.is_manager(organization):
@@ -1236,11 +1274,11 @@ def save_clones(view, user, queryset, organization=None):
                 )
 
         if user.is_superuser:
-            all_orgs = Organization.objects.all()
+            all_orgs = Organization.active.all()
             if all_orgs.count() > 1:
                 selectable_orgs = all_orgs
         elif len(user.organizations_managed) > 1:
-            selectable_orgs = Organization.objects.filter(
+            selectable_orgs = Organization.active.filter(
                 pk__in=user.organizations_managed
             )
         if selectable_orgs:

openwisp_controller/config/api/serializers.py

@@ -112,10 +112,15 @@ class FilterTemplatesByOrganization(serializers.PrimaryKeyRelatedField):
     def get_queryset(self):
         user = self.context["request"].user
         if user.is_superuser:
-            queryset = Template.objects.all()
+            queryset = Template.objects.filter(
+                Q(organization__is_active=True) | Q(organization__isnull=True)
+            )
         else:
             queryset = Template.objects.filter(
-                Q(organization__in=user.organizations_managed)
+                Q(
+                    organization__in=user.organizations_managed,
+                    organization__is_active=True,
+                )
                 | Q(organization__isnull=True)
             )
         return queryset

openwisp_controller/config/api/views.py

@@ -3,6 +3,7 @@
 from django.db.models import F, Q
 from django.http import Http404
 from django.urls.base import reverse
+from django.utils.translation import gettext_lazy as _
 from django_filters.rest_framework import DjangoFilterBackend
 from rest_framework import serializers, status
 from rest_framework.generics import (
@@ -98,7 +99,7 @@ class DeviceDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView):
     """
 
     serializer_class = DeviceDetailSerializer
-    queryset = Device.objects.select_related("config", "group", "organization")
+    queryset = Device.objects.select_related("config", "group")
     permission_classes = ProtectedAPIMixin.permission_classes + (DevicePermission,)
 
     def perform_destroy(self, instance):
@@ -124,6 +125,11 @@ class DeviceActivateView(ProtectedAPIMixin, GenericAPIView):
 
     def post(self, request, *args, **kwargs):
         device = self.get_object()
+        if not device.organization.is_active:
+            return Response(
+                {"detail": _("Cannot activate a device of a disabled organization.")},
+                status=status.HTTP_403_FORBIDDEN,
+            )
         device.activate()
         serializer = DeviceDetailSerializer(
             device, context=self.get_serializer_context()
@@ -134,6 +140,10 @@ def post(self, request, *args, **kwargs):
 class DeviceDeactivateView(ProtectedAPIMixin, GenericAPIView):
     serializer_class = serializers.Serializer
     queryset = Device.objects.filter(_is_deactivated=False)
+    # Deactivation stays allowed even when the organization is disabled:
+    # it's the remediation an operator needs if the org-wide deactivation
+    # task failed for this device (see deactivate_organization_devices).
+    allow_disabled_organization_writes = True
 
     def post(self, request, *args, **kwargs):
         device = self.get_object()
@@ -154,7 +164,7 @@ class DeviceGroupListCreateView(ProtectedAPIMixin, ListCreateAPIView):
 
 class DeviceGroupDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView):
     serializer_class = DeviceGroupSerializer
-    queryset = DeviceGroup.objects.select_related("organization").order_by("-created")
+    queryset = DeviceGroup.objects.order_by("-created")
 
 
 def get_cached_devicegroup_args_rewrite(cls, org_slugs, common_name):
@@ -168,7 +178,7 @@ def get_cached_devicegroup_args_rewrite(cls, org_slugs, common_name):
 
 class DeviceGroupCommonName(ProtectedAPIMixin, RetrieveAPIView):
     serializer_class = DeviceGroupSerializer
-    queryset = DeviceGroup.objects.select_related("organization").order_by("-created")
+    queryset = DeviceGroup.objects.order_by("-created")
     # Not setting lookup_field makes DRF raise error. but it is not used
     lookup_field = "pk"
 
@@ -190,7 +200,7 @@ def get_device_group(cls, org_slugs, common_name):
             )
             vpnclient = VpnClient.objects.only("config_id").get(cert_id=cert.id)
             group = (
-                Device.objects.select_related("group")
+                Device.objects.select_related("group", "group__organization")
                 .only("group")
                 .get(config=vpnclient.config_id)
                 .group

openwisp_controller/config/apps.py

@@ -11,6 +11,7 @@
 )
 from swapper import get_model_name, load_model
 
+from openwisp_users.signals import organization_disabled
 from openwisp_utils.admin_theme import register_dashboard_chart
 from openwisp_utils.admin_theme.menu import register_menu_group
 
@@ -105,11 +106,11 @@ def connect_cache_dependencies(self):
                     DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated
                 ),
             ),
-            # When an organization is disabled, all its devices are deactivated,
-            # so we need to invalidate the controller view caches for all objects.
             CacheDependency(
-                source=self.org_model,
-                signal="pre_save",
+                signal_obj=organization_disabled,
+                name="organization_disabled",
+                # organization_disabled signal is fired when the transaction
+                # is committed to the database.
                 on_commit=False,
                 target=organization_disabled_handler,
             ),

openwisp_controller/config/base/config.py

@@ -486,6 +486,10 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
             ).delete()
 
         if action == "post_add":
+            if not instance.device.organization.is_active:
+                # Do not create VPN clients (and their certs) for a
+                # disabled organization
+                return
             # A single template change can trigger multiple m2m_changed events.
             # Use the full current template set instead of this event's pk_set so
             # every attached VPN template has its VpnClient.

openwisp_controller/config/base/device.py

@@ -485,9 +485,10 @@ def create_default_config(self, **options):
         creates a new config instance to apply group templates
         if group has templates.
         """
-        if self.is_deactivated():
-            # All modification operations are blocked on deactivated devices.
-            # Hence, default config should not be created for deactivated devices.
+        if self.is_deactivated() or not self.organization.is_active:
+            # All modification operations are blocked on deactivated devices
+            # and on devices belonging to a disabled organization. Hence,
+            # default config should not be created in either case.
             return
         if not (self.group and self.group.templates.exists()):
             return
@@ -508,11 +509,12 @@ def manage_devices_group_templates(cls, device_ids, old_group_ids, group_id):
             device_ids = [device_ids]
             old_group_ids = [old_group_ids]
         for device_id, old_group_id in zip(device_ids, old_group_ids):
-            device = Device.objects.get(pk=device_id)
-            if device.is_deactivated():
+            device = Device.objects.select_related("organization").get(pk=device_id)
+            if device.is_deactivated() or not device.organization.is_active:
                 # Skip deactivated devices: their configuration is intentionally
                 # emptied during deactivation, so re-applying group templates
                 # would break that state and trigger a push to the device.
+                # Also skip devices of a disabled organization.
                 continue
             if not hasattr(device, "config"):
                 device.create_default_config()

openwisp_controller/config/base/device_group.py

@@ -94,6 +94,12 @@ def manage_group_templates(cls, group_id, old_template_ids, template_ids):
         DeviceGroup = load_model("config", "DeviceGroup")
         Template = load_model("config", "Template")
         device_group = DeviceGroup.objects.get(id=group_id)
+        if not device_group.organization.is_active:
+            # Do not push template changes to a disabled organization's
+            # devices; closes the race window between an organization
+            # being disabled and the async deactivate_organization_devices
+            # task deactivating each of its devices.
+            return
         templates = Template.objects.filter(pk__in=template_ids)
         old_templates = Template.objects.filter(pk__in=old_template_ids)
         for device in device_group.device_set.exclude(_is_deactivated=True).iterator():

openwisp_controller/config/base/vpn.py

@@ -252,6 +252,8 @@ def save(self, *args, **kwargs):
         """
         Calls _auto_create_cert() if cert is not set.
         """
+        if self.organization_id and not self.organization.is_active:
+            return super().save(*args, **kwargs)
         config = {}
         created = self._state.adding
         if not created:
@@ -958,6 +960,8 @@ def _get_unique_checks(self, exclude=None, include_meta_constraints=False):
 
     def save(self, *args, **kwargs):
         """Performs automatic provisioning if ``auto_cert`` is True."""
+        if not self.config.device.organization.is_active:
+            return super().save(*args, **kwargs)
         if self.auto_cert:
             self._auto_x509()
             self._auto_ip()
@@ -970,7 +974,11 @@ def _auto_x509(self):
         """
         Automatically creates an x509 certificate.
         """
-        if not self.vpn._is_backend_type("openvpn") or self.cert:
+        if (
+            not self.vpn._is_backend_type("openvpn")
+            or self.cert
+            or not self.config.device.organization.is_active
+        ):
             return
         cn = self._get_common_name()
         self._auto_create_cert(name=self.config.device.name, common_name=cn)