diff --git a/docs/developer/admin-utils.rst b/docs/developer/admin-utils.rst index ea7516c0d..f8fb0f52b 100644 --- a/docs/developer/admin-utils.rst +++ b/docs/developer/admin-utils.rst @@ -31,6 +31,39 @@ This class has two important attributes: `_ for a real-world example. +Disabled Organization Write Protection +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +``MultitenantAdminMixin`` also blocks changes to any object belonging to a +:ref:`disabled organization `, while still +allowing that object to be viewed and deleted. This applies to superusers +too: there is no per-user bypass (the only way to opt out is the +class-level attribute described below). For models whose organization is +reached through a parent (via ``multitenant_parent``), the mixin traverses +the parent to find the organization, so those child objects are protected +as well. + +This is controlled by the ``disabled_organization_write_protection`` class +attribute, which defaults to ``True``. Set it to ``False`` on a specific +``ModelAdmin`` to opt out: + +.. code-block:: python + + from django.contrib import admin + from openwisp_users.multitenancy import MultitenantAdminMixin + + + class BookAdmin(MultitenantAdminMixin, admin.ModelAdmin): + disabled_organization_write_protection = False + # other attributes + +The ``organization`` form field's queryset also excludes disabled +organizations for everyone, superusers included, so a disabled +organization can never be *selected* for a new object. The one exception +is an object that already belongs to a disabled organization on a +``ModelAdmin`` with the opt-out set: its own (disabled) organization stays +selectable in the field so the existing value can still be saved. + ``MultitenantOrgFilter`` ------------------------ diff --git a/docs/developer/django-rest-framework-utils.rst b/docs/developer/django-rest-framework-utils.rst index 80137c27d..c4738f9dc 100644 --- a/docs/developer/django-rest-framework-utils.rst +++ b/docs/developer/django-rest-framework-utils.rst @@ -131,13 +131,69 @@ organization managers or owners to view shared objects in read-only mode. Standard users will not be able to view or list shared objects. +``DisabledOrgReadOnly`` +~~~~~~~~~~~~~~~~~~~~~~~ + +**Full python path**: +``openwisp_users.api.permissions.DisabledOrgReadOnly``. + +This object-level permission class blocks updating an object that belongs +to a :ref:`disabled organization `. Read (safe +methods) and ``DELETE`` remain allowed. + +.. important:: + + ``DisabledOrgReadOnly`` guards **updates only**. It implements + ``has_object_permission``, which DRF does not call on ``POST``, so it + does **not** block *creating* a new object for a disabled + organization. Create protection instead relies on the organization + field excluding disabled organizations: use one of the + ``FilterSerializerByOrganization`` mixins (or a related field backed + by ``Organization.active``) on the serializer. A plain + ``ModelSerializer`` whose organization field defaults to + ``Organization.objects`` will happily create objects for a disabled + organization even under ``ProtectedAPIMixin``. + +A view can opt out of this guard by setting +``allow_disabled_organization_writes = True``: + +.. code-block:: python + + from openwisp_users.api.permissions import DisabledOrgReadOnly + from rest_framework.generics import RetrieveUpdateDestroyAPIView + + + class SubnetView(RetrieveUpdateDestroyAPIView): + permission_classes = (DisabledOrgReadOnly,) + allow_disabled_organization_writes = True + # other attributes + +``DisabledOrgReadOnly`` is already included in ``ProtectedAPIMixin``'s +default ``permission_classes`` (see below), so views that use +``ProtectedAPIMixin`` get this guard automatically without any extra +configuration. + +.. note:: + + ``Organization.active`` (django-organizations' ``ActiveOrgManager``) + is the canonical queryset for active organizations: use + ``Organization.active.all()`` when writing custom code that needs to + select from or filter active organizations, instead of filtering + ``Organization.objects`` manually. + ``ProtectedAPIMixin`` --------------------- **Full python path**: ``openwisp_users.api.mixins.ProtectedAPIMixin``. This mixin provides a set of authentication and permission classes that -are commonly used across various OpenWISP modules API views. +are commonly used across various OpenWISP modules API views, including +``DisabledOrgReadOnly`` (see above). + +If a view overrides ``permission_classes`` entirely instead of extending +``ProtectedAPIMixin.permission_classes``, it will not inherit +``DisabledOrgReadOnly`` (or any future addition to the mixin's defaults) +automatically, and must re-declare it explicitly if the guard is needed. Usage example: @@ -255,6 +311,15 @@ and ``FilterSerializerByOrgOwned`` can be used to solve this issue. These serializers do not allow non-superusers to create shared objects. +.. _multi_tenant_serializers_disabled_org: + +The ``organization`` field's queryset also excludes :ref:`disabled +organizations `, for everyone, superusers +included, so a disabled organization can never be selected when creating +or updating an object. Submitting the primary key of a disabled +organization returns a validation error explaining that the organization +does not exist or is disabled. + Usage example: .. code-block:: python diff --git a/docs/user/basic-concepts.rst b/docs/user/basic-concepts.rst index 70e0266d7..4c7539b6f 100644 --- a/docs/user/basic-concepts.rst +++ b/docs/user/basic-concepts.rst @@ -149,6 +149,56 @@ instance of the platform. `django-organizations `_ third-party app. +.. _disabling_an_organization: + +Disabling an Organization +------------------------- + +Superusers and managers of the organization can disable it, by unchecking +its **Is active** flag on the "Change organization" page or via the REST +API (subject to the usual permission requirements for editing an +organization). + +Disabling an organization does not delete anything: all of its data, +including users, memberships, and related objects, remains fully +**readable** and **deletable**. What changes is: + +- **No new object can be created for a disabled organization**, and + **existing objects belonging to it cannot be modified**, superusers + included. This applies to the organization's own record too: once + disabled, only its **Is active** flag can be changed (to re-enable it) + or its owner unassigned; everything else is locked until it is + re-enabled. +- Deleting objects, including the organization itself, is always allowed, + so cleanup is never blocked. +- The organization stops appearing in **organization selection widgets** + (e.g. when creating a new object), so it can no longer be picked for new + data. It still appears in admin **list filters**, so its existing data + remains easy to find for auditing purposes. +- Re-enabling a disabled organization is allowed **only for superusers**. + Once an organization is disabled, its managers lose access to it (a + disabled organization is no longer part of the organizations they + manage), so they can no longer edit it, including re-enabling it. A + superuser must re-enable the organization before its managers regain + access. + +.. note:: + + In the REST API, attempting to update an object belonging to a + disabled organization returns an HTTP 400 or 403 response with a clear + error message, instead of failing silently or being blocked without + explanation. + +.. note:: + + Re-enabling an organization and editing its other fields must be done + in **two separate steps**, matching the admin interface (which locks + every field except **Is active** while the organization is disabled). + First re-enable the organization (change only **Is active**), then + edit its other fields or assign an owner. A single request that both + re-enables the organization and changes another field (or assigns an + owner) is rejected. + Organization Membership and Roles --------------------------------- diff --git a/openwisp_users/admin.py b/openwisp_users/admin.py index af504df38..4225c7eda 100644 --- a/openwisp_users/admin.py +++ b/openwisp_users/admin.py @@ -38,6 +38,7 @@ from . import settings as app_settings from .multitenancy import MultitenantAdminMixin, MultitenantOrgFilter from .utils import BaseAdmin +from .widgets import OrganizationAutocompleteSelect Group = load_model("openwisp_users", "Group") Organization = load_model("openwisp_users", "Organization") @@ -98,34 +99,100 @@ class OrganizationOwnerInline(admin.StackedInline): extra = 0 autocomplete_fields = ("organization_user",) + def has_add_permission(self, request, obj=None): + # obj is the parent Organization here + if obj is not None and not obj.is_active: + return False + return super().has_add_permission(request, obj) + def has_change_permission(self, request, obj=None): + if obj is not None and not obj.is_active: + return False if obj and not request.user.is_superuser and not request.user.is_owner(obj): return False return super().has_change_permission(request, obj) +class OrganizationUserInlineFormSet(RequiredInlineFormSet): + """ + Renders existing memberships of a disabled organization as read-only so + the row survives a no-op save (the disabled organization is not part of + the field queryset otherwise) and its select widget shows the disabled + organization instead of rendering empty. Deleting the row stays possible. + """ + + def add_fields(self, form, index): + super().add_fields(form, index) + instance = getattr(form, "instance", None) + if ( + instance + and instance.pk + and instance.organization_id + and not instance.organization.is_active + ): + org_field = form.fields.get("organization") + if org_field is not None: + org_field.disabled = True + org_field.queryset = Organization.objects.filter( + pk=instance.organization_id + ) + if "is_admin" in form.fields: + form.fields["is_admin"].disabled = True + + class OrganizationUserInline(admin.StackedInline): model = OrganizationUser - formset = RequiredInlineFormSet + formset = OrganizationUserInlineFormSet view_on_site = False fields = ("organization", "is_admin") autocomplete_fields = ("organization",) + def get_queryset(self, request): + # OrganizationUserInlineFormSet.add_fields() reads + # instance.organization.is_active for every row; select_related + # folds that per-row query into this one. + return super().get_queryset(request).select_related("organization") + def get_formset(self, request, obj=None, **kwargs): """ - In form dropdowns, display only organizations - in which operator `is_admin` and for superusers - display all organizations + In form dropdowns, display only active organizations; + non-superusers additionally only see organizations + in which they are `is_admin`. """ formset = super().get_formset(request, obj=obj, **kwargs) + org_field = formset.form.base_fields["organization"] + org_field.queryset = org_field.queryset.filter(is_active=True) if request.user.is_superuser: return formset - if not request.user.is_superuser: - formset.form.base_fields["organization"].queryset = ( - Organization.objects.filter(pk__in=request.user.organizations_managed) - ) + org_field.queryset = org_field.queryset.filter( + pk__in=request.user.organizations_managed + ) return formset + def formfield_for_foreignkey(self, db_field, request, **kwargs): + """ + Route the organization picker through the ``ow-auto-filter`` endpoint + so disabled organizations are excluded from the dropdown for everyone, + superusers included (the stock ``admin:autocomplete`` endpoint does not + filter them). Only replaces the widget when the field is actually an + autocomplete field, so that disabling ``autocomplete_fields`` keeps + rendering a plain select. + """ + if db_field.name == "organization" and db_field.name in ( + self.get_autocomplete_fields(request) + ): + kwargs["widget"] = OrganizationAutocompleteSelect( + db_field, self.admin_site, using=kwargs.get("using") + ) + return super().formfield_for_foreignkey(db_field, request, **kwargs) + + def has_add_permission(self, request, obj=None): + # an operator who manages no active organization cannot pick one, so + # the add row would be unusable: hide it + if not request.user.is_superuser and not request.user.organizations_managed: + return False + return super().has_add_permission(request, obj) + def get_extra(self, request, obj=None, **kwargs): if not obj: return 1 @@ -583,6 +650,29 @@ def has_change_permission(self, request, obj=None): return False return super().has_change_permission(request, obj) + def get_readonly_fields(self, request, obj=None): + """ + A disabled organization can only be re-enabled: every other + field becomes readonly (owner unassignment is still possible + via the inline's delete action, which does not go through here). + """ + fields = super().get_readonly_fields(request, obj) + if obj and not obj.is_active: + editable_fields = [ + f.name + for f in self.model._meta.local_fields + if f.editable and f.name != "is_active" + ] + fields = list(fields) + [f for f in editable_fields if f not in fields] + return fields + + def get_prepopulated_fields(self, request, obj=None): + # prepopulated_fields cannot reference a field that is also + # readonly, which is the case for "slug" on a disabled organization + if obj and not obj.is_active: + return {} + return super().get_prepopulated_fields(request, obj) + class Media(CopyableFieldsAdmin.Media): css = {"all": ("openwisp-users/css/admin.css",)} diff --git a/openwisp_users/api/mixins.py b/openwisp_users/api/mixins.py index 0eb32756f..b98212aa2 100644 --- a/openwisp_users/api/mixins.py +++ b/openwisp_users/api/mixins.py @@ -1,6 +1,7 @@ import swapper from django.core.exceptions import ValidationError from django.db.models import ForeignKey, ManyToManyField, Q +from django.utils.translation import gettext_lazy as _ from django_filters import rest_framework as filters from django_filters.filters import QuerySetRequestMixin as BaseQuerySetRequestMixin from rest_framework.authentication import SessionAuthentication @@ -8,10 +9,18 @@ from rest_framework.permissions import IsAuthenticated from .authentication import BearerAuthentication -from .permissions import DjangoModelPermissions, IsOrganizationManager +from .permissions import ( + DisabledOrgReadOnly, + DjangoModelPermissions, + IsOrganizationManager, +) Organization = swapper.load_model("openwisp_users", "Organization") +DISABLED_ORGANIZATION_ERROR_MESSAGE = _( + 'Organization with pk "{pk_value}" does not exist or is disabled.' +) + class OrgLookup: @property @@ -159,18 +168,28 @@ def _user_attr(self): def filter_fields(self): user = self.context["request"].user - # superuser can see everything - if user.is_superuser or user.is_anonymous: - return - # non superusers can see only items of organizations they're related to - organization_filter = getattr(user, self._user_attr) + # superuser can see everything, except disabled organizations + # The anonymouse use case exist so we don't run into errors with swagger + is_superuser_or_anonymous = user.is_superuser or user.is_anonymous + if not is_superuser_or_anonymous: + # non superusers can see only items of organizations + # they're related to + organization_filter = getattr(user, self._user_attr) for field in self.fields: if field == "organization" and not self.fields[field].read_only: # queryset attribute will not be present if set to read_only - self.fields[field].allow_null = False - self.fields[field].queryset = self.fields[field].queryset.filter( - pk__in=organization_filter - ) + # disabled organizations are excluded for everyone, superusers + # included, since they can only be re-enabled, not written to + queryset = self.fields[field].queryset.filter(is_active=True) + self.fields[field].error_messages[ + "does_not_exist" + ] = DISABLED_ORGANIZATION_ERROR_MESSAGE + if not is_superuser_or_anonymous: + self.fields[field].allow_null = False + queryset = queryset.filter(pk__in=organization_filter) + self.fields[field].queryset = queryset + continue + if is_superuser_or_anonymous: continue conditions = Q(**{self.organization_lookup: organization_filter}) if self.include_shared: @@ -308,4 +327,5 @@ class ProtectedAPIMixin(object): permission_classes = ( IsOrganizationManager, DjangoModelPermissions, + DisabledOrgReadOnly, ) diff --git a/openwisp_users/api/permissions.py b/openwisp_users/api/permissions.py index 4177229f3..af5b7389f 100644 --- a/openwisp_users/api/permissions.py +++ b/openwisp_users/api/permissions.py @@ -1,5 +1,5 @@ from django.utils.translation import gettext_lazy as _ -from rest_framework.permissions import BasePermission +from rest_framework.permissions import SAFE_METHODS, BasePermission from rest_framework.permissions import ( DjangoModelPermissions as BaseDjangoModelPermissions, ) @@ -95,6 +95,31 @@ def validate_membership(self, user, org): return org and (user.is_superuser or user.is_owner(org)) +class DisabledOrgReadOnly(ObjectOrganizationMixin, BasePermission): + """ + Blocks update of objects belonging to a disabled organization. + Read and delete remain allowed. Applies to superusers as well. + Views can opt out with `allow_disabled_organization_writes = True`. + """ + + message = _( + "This object belongs to a disabled organization: " + "it can be viewed or deleted, but not modified." + ) + + def has_object_permission(self, request, view, obj): + if getattr(view, "allow_disabled_organization_writes", False): + return True + if request.method in SAFE_METHODS or request.method == "DELETE": + return True + try: + organization = self.get_object_organization(view, obj) + except AttributeError: + # object has no organization field, rule not applicable + return True + return organization is None or organization.is_active + + class DjangoModelPermissions(ObjectOrganizationMixin, BaseDjangoModelPermissions): perms_map = { "GET": ["%(app_label)s.view_%(model_name)s"], diff --git a/openwisp_users/api/serializers.py b/openwisp_users/api/serializers.py index 062ae0e8b..5938e16f4 100644 --- a/openwisp_users/api/serializers.py +++ b/openwisp_users/api/serializers.py @@ -4,6 +4,7 @@ from allauth.account.models import EmailAddress from django.contrib.auth import get_user_model from django.contrib.auth.models import Permission +from django.core.exceptions import ValidationError as DjangoValidationError from django.db import transaction from django.db.models import Q from django.utils.translation import gettext_lazy as _ @@ -12,6 +13,8 @@ from openwisp_utils.api.serializers import ValidatedModelSerializer +from .mixins import DISABLED_ORGANIZATION_ERROR_MESSAGE + Group = load_model("openwisp_users", "Group") Organization = load_model("openwisp_users", "Organization") User = get_user_model() @@ -20,6 +23,19 @@ OrganizationOwner = load_model("openwisp_users", "OrganizationOwner") +def _full_clean_or_raise(instance): + """ + Django's ValidationError raised by full_clean() is not caught by DRF + unless it happens inside a serializer's validate(); these call sites + call full_clean() from create()/update(), so it must be converted + manually or it propagates as an unhandled 500. + """ + try: + instance.full_clean() + except DjangoValidationError as e: + raise serializers.ValidationError(serializers.as_serializer_error(e)) + + class OrganizationSerializer(ValidatedModelSerializer): class Meta: model = Organization @@ -66,7 +82,7 @@ def get_queryset(self): queryset = OrganizationUser.objects.filter( Q(organization__in=user.organizations_managed) ) - return queryset.select_related() + return queryset.filter(organization__is_active=True).select_related() class OrganizationOwnerSerializer(serializers.ModelSerializer): @@ -96,6 +112,45 @@ class Meta: "modified", ) + def validate(self, data): + if self.instance and not self.instance.is_active: + owner_data = data.get("owner") or {} + owner_present = "owner" in data + is_owner_unassignment = ( + owner_present and owner_data.get("organization_user") is None + ) + reenabling = data.get("is_active") is True + # A key whose submitted value matches the value already stored is + # not a change, so a read-modify-write PUT that resends every + # field unchanged except is_active must not be rejected just + # because e.g. "name" is present in the payload. + changed_keys = { + key + for key in data + if key != "owner" and getattr(self.instance, key) != data[key] + } + if owner_present: + changed_keys.add("owner") + # While disabled, only re-enabling (Is active) and/or unassigning + # the owner are allowed, and neither can be combined with any other + # change (editing a field or assigning an owner). This matches the + # admin interface, which locks every field except Is active, so the + # admin, the API and the docs tell the same story. + allowed = ( + changed_keys <= {"is_active", "owner"} + and (not owner_present or is_owner_unassignment) + and (reenabling or is_owner_unassignment) + ) + if not allowed: + raise serializers.ValidationError( + _( + "This organization is disabled: only re-enabling it, " + "unassigning its owner, or deleting it is allowed. Edit " + "other fields or assign an owner after re-enabling it." + ) + ) + return super().validate(data) + def update(self, instance, validated_data): if validated_data.get("owner"): org_owner = validated_data.pop("owner") @@ -110,7 +165,7 @@ def update(self, instance, validated_data): org_owner = OrganizationOwner.objects.create( organization=instance, organization_user=org_user ) - org_owner.full_clean() + _full_clean_or_raise(org_owner) org_owner.save() return super().update(instance, validated_data) @@ -127,7 +182,7 @@ def update(self, instance, validated_data): org_owner = OrganizationOwner.objects.create( organization=instance, organization_user=org_user ) - org_owner.full_clean() + _full_clean_or_raise(org_owner) org_owner.save() instance = self.instance or self.Meta.model(**validated_data) @@ -174,12 +229,16 @@ def update(self, instance, validated_data): class OrgUserCustomPrimarykeyRelatedField(serializers.PrimaryKeyRelatedField): + default_error_messages = { + "does_not_exist": DISABLED_ORGANIZATION_ERROR_MESSAGE, + } + def get_queryset(self): user = self.context["request"].user if user.is_superuser: - queryset = Organization.objects.all() + queryset = Organization.active.all() else: - queryset = Organization.objects.filter(pk__in=user.organizations_managed) + queryset = Organization.active.filter(pk__in=user.organizations_managed) return queryset @@ -270,20 +329,24 @@ def create(self, validated_data): password = validated_data.pop("password") email_verified = validated_data.pop("email_verified", False) - instance = self.instance or self.Meta.model(**validated_data) - instance.set_password(password) - instance.full_clean() - instance.save() - - if group_data: - instance.groups.add(*group_data) - - if org_user_data: - if org_user_data.get("organization") is not None: - org_user_data["user"] = instance - org_user_instance = OrganizationUser(**org_user_data) - org_user_instance.full_clean() - org_user_instance.save() + # Keep user and membership creation in a single transaction so a + # membership validation failure does not leave a half-created user + # behind while _full_clean_or_raise returns a 400. + with transaction.atomic(): + instance = self.instance or self.Meta.model(**validated_data) + instance.set_password(password) + instance.full_clean() + instance.save() + + if group_data: + instance.groups.add(*group_data) + + if org_user_data: + if org_user_data.get("organization") is not None: + org_user_data["user"] = instance + org_user_instance = OrganizationUser(**org_user_data) + _full_clean_or_raise(org_user_instance) + org_user_instance.save() if instance.email: try: @@ -354,20 +417,16 @@ def update(self, instance, validated_data): except OrganizationUser.DoesNotExist: pass if org_user: - if ( - str(org_user_data["organization"].id) - in instance.organizations_dict.keys() - ): - if org_user.is_admin != org_user_data.get("is_admin"): - org_user.is_admin = org_user_data["is_admin"] - org_user.full_clean() - org_user.save() - else: - org_user.delete() + if org_user.is_admin != org_user_data.get("is_admin"): + org_user.is_admin = org_user_data["is_admin"] + _full_clean_or_raise(org_user) + org_user.save() + else: + org_user.delete() else: org_user_data["user"] = instance org_user_instance = OrganizationUser(**org_user_data) - org_user_instance.full_clean() + _full_clean_or_raise(org_user_instance) org_user_instance.save() return super().update(instance, validated_data) diff --git a/openwisp_users/apps.py b/openwisp_users/apps.py index 6fb0f1187..12d1b291a 100644 --- a/openwisp_users/apps.py +++ b/openwisp_users/apps.py @@ -197,7 +197,7 @@ def update_organizations_dict(cls, instance, signal, **kwargs): @classmethod def create_organization_owner(cls, instance, created, **kwargs): - if not created or not instance.is_admin: + if not created or not instance.is_admin or not instance.organization.is_active: return OrganizationOwner = load_model("openwisp_users", "OrganizationOwner") org_owner_exist = OrganizationOwner.objects.filter( diff --git a/openwisp_users/base/models.py b/openwisp_users/base/models.py index 52ff71ce5..8e365b64b 100644 --- a/openwisp_users/base/models.py +++ b/openwisp_users/base/models.py @@ -469,14 +469,14 @@ def add_user(self, user, is_admin=False, **kwargs): automatically via a signal receiver. Without this change, the add_user method would throw IntegrityError. """ - if not self.users.all().exists(): is_admin = True OrganizationUser = load_model("openwisp_users", "OrganizationUser") - return OrganizationUser.objects.create( - user=user, organization=self, is_admin=is_admin - ) + org_user = OrganizationUser(user=user, organization=self, is_admin=is_admin) + org_user.full_clean() + org_user.save() + return org_user class BaseOrganizationUser(models.Model): @@ -491,6 +491,27 @@ class Meta: abstract = True def clean(self): + if self.organization_id and not self.organization.is_active: + if self._state.adding: + raise ValidationError( + {"organization": _("Cannot add users to a disabled organization.")} + ) + # Only block real modifications: Django re-runs full_clean() on + # untouched inline rows, so a no-op save of a user who belongs to a + # disabled organization must not fail. + db_values = ( + self._meta.model.objects.filter(pk=self.pk) + .values("organization_id", "is_admin", "user_id") + .first() + ) + if db_values is None or ( + db_values["organization_id"] != self.organization_id + or db_values["is_admin"] != self.is_admin + or db_values["user_id"] != self.user_id + ): + raise ValidationError( + _("Memberships of a disabled organization cannot be modified.") + ) if ( not self._state.adding and self.user.is_owner(self.organization_id) @@ -521,6 +542,22 @@ class BaseOrganizationOwner(models.Model): id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) def clean(self): + if self.organization_id and not self.organization.is_active: + # Only block assigning or changing an owner: an untouched owner row + # is re-validated when its organization is disabled, and that must + # not prevent disabling the organization. + db_values = ( + self._meta.model.objects.filter(pk=self.pk) + .values("organization_id", "organization_user_id") + .first() + ) + if db_values is None or ( + db_values["organization_id"] != self.organization_id + or db_values["organization_user_id"] != self.organization_user_id + ): + raise ValidationError( + _("Cannot assign an owner to a disabled organization.") + ) if self.organization_user.organization.pk != self.organization.pk: raise ValidationError( { diff --git a/openwisp_users/multitenancy.py b/openwisp_users/multitenancy.py index b6557fb23..51b3fa5de 100644 --- a/openwisp_users/multitenancy.py +++ b/openwisp_users/multitenancy.py @@ -20,6 +20,9 @@ class MultitenantAdminMixin(object): multitenant_shared_relations = None multitenant_parent = None + # opt-out hook: set to False on subclasses that should allow writes + # to objects belonging to a disabled organization + disabled_organization_write_protection = True def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -58,7 +61,60 @@ def get_queryset(self, request): qsarg = "{0}__organization__in".format(self.multitenant_parent) return qs.filter(**{qsarg: user.organizations_managed}) - def _edit_form(self, request, form): + def _get_object_organization(self, obj): + """ + Returns the organization an object belongs to, traversing + ``multitenant_parent`` for models whose organization is reached + through a parent (e.g. a Book through its Shelf). + """ + organization = getattr(obj, "organization", None) + if organization is None and self.multitenant_parent: + parent = obj + for attr in self.multitenant_parent.split("__"): + parent = getattr(parent, attr, None) + if parent is None: + break + organization = getattr(parent, "organization", None) + return organization + + def has_change_permission(self, request, obj=None): + """ + Objects belonging to a disabled organization stay readable and + deletable, but cannot be changed, regardless of the user being a + superuser. Subclasses can opt out with + ``disabled_organization_write_protection = False``. + """ + if self.disabled_organization_write_protection and obj is not None: + organization = self._get_object_organization(obj) + if organization is not None and not organization.is_active: + return False + return super().has_change_permission(request, obj) + + def has_add_permission(self, request, *args, **kwargs): + """ + Hide the Add button from admins who manage no active organization: + the organization dropdown would be empty and the form could never be + submitted. Does not apply to the user admin or to models without an + organization (directly or through ``multitenant_parent``). + + ``*args`` keeps this compatible with both ``ModelAdmin`` + (``request``) and ``InlineModelAdmin`` (``request, obj``), since this + mixin is used on inlines too. + """ + if ( + not request.user.is_superuser + and self.model != User + and not request.user.organizations_managed + ): + # Any model with an organization field (directly, or reached + # through multitenant_parent) is blocked: _edit_form() makes the + # field required for non-superusers, so the form could not be + # submitted without an active organization to pick anyway. + if hasattr(self.model, "organization") or self.multitenant_parent: + return False + return super().has_add_permission(request, *args, **kwargs) + + def _edit_form(self, request, form, obj=None): """ Modifies the form querysets as follows; if current user is not superuser: @@ -67,10 +123,25 @@ def _edit_form(self, request, form): or shared relations * do not allow organization field to be empty (shared org) else show everything + Organization choices always exclude disabled organizations, + superusers included, except an admin that opted out of write + protection (``disabled_organization_write_protection = False``) + keeps the edited object's own disabled organization selectable, + or the form could never be saved. """ fields = form.base_fields user = request.user org_field = fields.get("organization") + keep_disabled_org_pk = None + if not self.disabled_organization_write_protection and obj is not None: + organization = self._get_object_organization(obj) + if organization is not None and not organization.is_active: + keep_disabled_org_pk = organization.pk + if org_field: + allowed = Q(is_active=True) + if keep_disabled_org_pk is not None: + allowed |= Q(pk=keep_disabled_org_pk) + org_field.queryset = org_field.queryset.filter(allowed) if user.is_superuser and org_field and not org_field.required: org_field.empty_label = SHARED_SYSTEMWIDE_LABEL elif not user.is_superuser: @@ -78,7 +149,10 @@ def _edit_form(self, request, form): # organizations relation; # may be readonly and not present in field list if org_field: - org_field.queryset = org_field.queryset.filter(pk__in=orgs_pk) + managed = Q(pk__in=orgs_pk) + if keep_disabled_org_pk is not None: + managed |= Q(pk=keep_disabled_org_pk) + org_field.queryset = org_field.queryset.filter(managed) org_field.empty_label = None org_field.required = True # other relations @@ -93,7 +167,7 @@ def _edit_form(self, request, form): def get_form(self, request, obj=None, **kwargs): form = super().get_form(request, obj, **kwargs) - self._edit_form(request, form) + self._edit_form(request, form, obj) return form def get_formset(self, request, obj=None, **kwargs): diff --git a/openwisp_users/tests/test_admin.py b/openwisp_users/tests/test_admin.py index ab0ad5cda..ed0005f49 100644 --- a/openwisp_users/tests/test_admin.py +++ b/openwisp_users/tests/test_admin.py @@ -26,6 +26,7 @@ from ..admin import OrganizationAdmin, OrganizationOwnerAdmin from ..apps import logger as apps_logger from ..multitenancy import MultitenantAdminMixin +from ..widgets import OrganizationAutocompleteSelect from .utils import ( TestMultitenantAdminMixin, TestOrganizationMixin, @@ -1864,6 +1865,299 @@ def test_only_superuser_can_delete_inline_org_owner(self): self.assertEqual(r.status_code, 200) self.assertContains(r, '-DELETE">Delete') + def test_disabled_organization_change_form(self): + admin = self._get_admin() + self.client.force_login(admin) + org = self._create_org(name="disabled-admin-org", is_active=False) + path = reverse(f"admin:{self.app_label}_organization_change", args=[org.pk]) + + with self.subTest("Fields readonly except is_active, no 500"): + response = self.client.get(path) + self.assertEqual(response.status_code, 200) + self.assertNotContains( + response, f'/", + views.template_disabled_org_write_allowed_detail, + name="test_template_disabled_org_write_allowed_detail", + ), + path( + "protected_template//", + views.protected_template_detail, + name="test_protected_template_detail", + ), path( "library/", views.library_list, diff --git a/tests/testapp/views.py b/tests/testapp/views.py index 77d2a7994..795eb899f 100644 --- a/tests/testapp/views.py +++ b/tests/testapp/views.py @@ -22,9 +22,11 @@ FilterByParentMembership, FilterByParentOwned, FilterDjangoByOrgManaged, + ProtectedAPIMixin, ) from openwisp_users.api.permissions import ( BaseOrganizationPermission, + DisabledOrgReadOnly, DjangoModelPermissions, IsOrganizationManager, IsOrganizationMember, @@ -211,6 +213,7 @@ class TemplateListCreateView(FilterByOrganizationManaged, ListCreateAPIView): permission_classes = ( IsOrganizationMember, DjangoModelPermissions, + DisabledOrgReadOnly, ) queryset = Template.objects.all() @@ -221,10 +224,28 @@ class TemplateDetailView(FilterByOrganizationManaged, RetrieveUpdateDestroyAPIVi permission_classes = ( IsOrganizationMember, DjangoModelPermissions, + DisabledOrgReadOnly, ) queryset = Template.objects.all() +class TemplateDisabledOrgWriteAllowedDetailView(TemplateDetailView): + allow_disabled_organization_writes = True + + +class ProtectedTemplateDetailView( + ProtectedAPIMixin, FilterByOrganizationManaged, RetrieveUpdateDestroyAPIView +): + """ + Uses ProtectedAPIMixin directly, with no permission_classes/ + authentication_classes override, to prove the disabled-organization + guard is inherited automatically rather than manually re-declared. + """ + + serializer_class = TemplateSerializer + queryset = Template.objects.all() + + class LibraryListFilter(FilterDjangoByOrgManaged): class Meta: model = Library @@ -285,6 +306,10 @@ class ShelfWithReadOnlyOrgListCreateView( shelf_list_owner_view = ShelfListOwnerView.as_view() template_list = TemplateListCreateView.as_view() template_detail = TemplateDetailView.as_view() +template_disabled_org_write_allowed_detail = ( + TemplateDisabledOrgWriteAllowedDetailView.as_view() +) +protected_template_detail = ProtectedTemplateDetailView.as_view() library_list = LibraryListCreateView.as_view() library_detail = LibraryDetailView.as_view() book_nested_shelf = BookNestedShelfListCreateView.as_view()