[change] Notification preferences: respected Django permissions #312

Ensured only users with permission to change notification settings
are allowed to change the preferences of other users in the organizations
they manage. 

Closes #312

Author: Dhanus3133

docs/user/notification-preferences.rst

@@ -52,13 +52,14 @@ A notification setting can therefore either:
 
 Notification settings are automatically generated for notification types
 and organizations associated with a user. Effective notification behavior
-is resolved dynamically using inherited defaults. Superusers have the
-ability to manage notification settings for all users, including adding or
-deleting them. Meanwhile, staff users can modify their preferred
-notification delivery methods, choosing between receiving notifications
-via web, email, or both. Additionally, users have the option to disable
-notifications entirely by turning off both web and email notification
-settings.
+is resolved dynamically using inherited defaults. Staff users who have the
+``change_notificationsetting`` permission can manage notification settings
+for users in organizations they manage. Superusers can manage any user's
+notification settings without additional permissions. Meanwhile, users can
+modify their own preferred notification delivery methods, choosing between
+receiving notifications via web, email, or both. Additionally, users have
+the option to disable notifications entirely by turning off both web and
+email notification settings.
 
 .. note::
 

openwisp_notifications/api/permissions.py

@@ -1,16 +1,71 @@
-from rest_framework.permissions import BasePermission
+from rest_framework.permissions import SAFE_METHODS, BasePermission
+
+from openwisp_notifications.swapper import load_model, swapper_load_model
+
+NotificationSetting = load_model("NotificationSetting")
+OrganizationUser = swapper_load_model("openwisp_users", "OrganizationUser")
 
 
 class PreferencesPermission(BasePermission):
     """
     Permission class for the notification preferences.
 
-    Permission is granted only in these two cases:
+    Permission is granted in these cases:
     1. Superusers can change the notification preferences of any user.
-    2. Regular users can only change their own preferences.
+    2. Users with the required NotificationSetting permission can access
+       the notification preferences of another user in a managed organization.
+    3. All other authenticated users can only change their own preferences.
+       This includes deprecated routes without a user_id URL parameter.
     """
 
+    def _get_perm(self, action):
+        """Return the active NotificationSetting model permission."""
+        return (
+            f"{NotificationSetting._meta.app_label}."
+            f"{action}_{NotificationSetting._meta.model_name}"
+        )
+
+    def _has_required_perm(self, request):
+        """
+        Match Django model permission semantics for preferences API methods.
+
+        Read requests accept view or change permission because users with
+        change permission should be able to inspect settings before editing.
+        Write requests require change permission.
+        """
+        if request.method in SAFE_METHODS:
+            return request.user.has_perm(
+                self._get_perm("view")
+            ) or request.user.has_perm(self._get_perm("change"))
+        return request.user.has_perm(self._get_perm("change"))
+
+    def _can_access_target_user(self, request, view):
+        """
+        Check whether the requested user belongs to a managed organization.
+
+        This is used only for other-user access. Own preferences and
+        superuser access are handled separately in ``has_permission``.
+        """
+        user_id = view.kwargs.get("user_id")
+        queryset = OrganizationUser.objects.filter(
+            user_id=user_id,
+            organization_id__in=request.user.organizations_managed,
+        )
+        organization_id = view.kwargs.get("organization_id")
+        # Bulk organization updates must target an organization the requester
+        # manages and the target user belongs to.
+        if organization_id is not None:
+            queryset = queryset.filter(organization_id=organization_id)
+        return queryset.exists()
+
     def has_permission(self, request, view):
-        return request.user.is_superuser or request.user.id == view.kwargs.get(
-            "user_id"
+        if request.user.is_superuser:
+            return True
+        user_id = view.kwargs.get("user_id")
+        if user_id is None:
+            return True
+        if str(request.user.id) == str(user_id):
+            return True
+        return self._has_required_perm(request) and self._can_access_target_user(
+            request, view
         )

openwisp_notifications/api/views.py

@@ -125,14 +125,24 @@ def get_queryset(self):
         if getattr(self, "swagger_fake_view", False):
             return NotificationSetting.objects.none()  # pragma: no cover
         user_id = self.kwargs.get("user_id", self.request.user.id)
-        return (
+        queryset = (
             NotificationSetting.objects.exclude(
                 Q(organization__is_active=False)
                 | Q(type__in=app_settings.DISALLOW_PREFERENCES_CHANGE_TYPE)
             )
             .filter(user_id=user_id, deleted=False)
             .select_related("organization__notification_settings")
         )
+        # Other-user reads should never expose organization settings outside
+        # the organizations managed by the requester.
+        if not self.request.user.is_superuser and str(self.request.user.pk) != str(
+            user_id
+        ):
+            queryset = queryset.filter(
+                Q(organization__isnull=True)
+                | Q(organization_id__in=self.request.user.organizations_managed)
+            )
+        return queryset
 
 
 class NotificationSettingListView(BaseNotificationSettingView, ListModelMixin):

openwisp_notifications/templates/admin/openwisp_users/user/change_form_object_tools.html

@@ -1,11 +1,12 @@
 {% extends "admin/change_form_object_tools.html" %}
 
-{% load i18n admin_urls %}
+{% load i18n admin_urls notification_tags %}
 
 {% block object-tools-items %}
-  {% if request.user.is_staff and original.is_staff %}
+  {% has_notification_setting_permission request.user original as has_perm %}
+  {% if original.is_staff and has_perm %}
     <li>
-      <a class="button" href="{% url 'notifications:user_notification_preference' object_id %}">Notification Preferences</a>
+      <a class="button" href="{% url 'notifications:user_notification_preference' object_id %}">{% trans "Notification Preferences" %}</a>
     </li>
   {% endif %}
   {{ block.super }}

openwisp_notifications/templatetags/notification_tags.py

@@ -3,10 +3,11 @@
 from django.template import Library
 from django.utils.html import format_html
 
-from openwisp_notifications.swapper import load_model
+from openwisp_notifications.swapper import load_model, swapper_load_model
 from openwisp_notifications.utils import normalize_unread_count
 
 Notification = load_model("Notification")
+OrganizationUser = swapper_load_model("openwisp_users", "OrganizationUser")
 
 register = Library()
 
@@ -30,8 +31,7 @@ def unread_notifications(context):
     count = get_notifications_count(context)
     output = ""
     if count:
-        output = '<span id="ow-notification-count">{0}</span>'
-        output = format_html(output.format(count))
+        output = format_html('<span id="ow-notification-count">{0}</span>', count)
     return output
 
 
@@ -44,4 +44,20 @@ def should_load_notifications_widget(request):
     )
 
 
+@register.simple_tag
+def has_notification_setting_permission(user, target_user=None):
+    if not user or not user.is_authenticated:
+        return False
+    NotificationSetting = load_model("NotificationSetting")
+    perm = f"{NotificationSetting._meta.app_label}.change_{NotificationSetting._meta.model_name}"
+    if not user.has_perm(perm):
+        return False
+    if user.is_superuser or target_user is None:
+        return True
+    return OrganizationUser.objects.filter(
+        user=target_user,
+        organization_id__in=user.organizations_managed,
+    ).exists()
+
+
 register.simple_tag(takes_context=True)(unread_notifications)

openwisp_notifications/tests/test_admin.py

@@ -12,16 +12,20 @@
 from openwisp_notifications import settings as app_settings
 from openwisp_notifications.signals import notify
 from openwisp_notifications.swapper import load_model, swapper_load_model
+from openwisp_notifications.templatetags.notification_tags import (
+    has_notification_setting_permission,
+)
 from openwisp_notifications.widgets import _add_object_notification_widget
 from openwisp_users.admin import UserAdmin
 from openwisp_users.tests.utils import TestMultitenantAdminMixin
 
-from .test_helpers import MessagingRequest
+from .test_helpers import MessagingRequest, get_notification_setting_permission
 
 Notification = load_model("Notification")
 NotificationSetting = load_model("NotificationSetting")
 notification_queryset = Notification.objects.order_by("-timestamp")
 Group = swapper_load_model("openwisp_users", "Group")
+OrganizationUser = swapper_load_model("openwisp_users", "OrganizationUser")
 
 
 class MockUser:
@@ -164,7 +168,9 @@ def test_ignore_notification_widget_add_view(self):
         self.assertNotContains(response, "owIsChangeForm")
 
     def test_notification_preferences_button_staff_user(self):
-        user = self._create_user(is_staff=True)
+        user = self._create_user(
+            username="btn_tester", email="btn_tester@test.com", is_staff=True
+        )
         user_admin_page = reverse(
             f"admin:{self.users_app_label}_user_change", args=(user.pk,)
         )
@@ -180,14 +186,124 @@ def test_notification_preferences_button_staff_user(self):
             response = self.client.get(user_admin_page)
             self.assertContains(response, expected_html, html=True)
 
-        # Button does not appear for non-staff user
-        with self.subTest("Button should not appear for non-staff user"):
+        with self.subTest("Superuser does not see button for non-staff user"):
             user.is_staff = False
             user.full_clean()
             user.save()
             response = self.client.get(user_admin_page)
             self.assertNotContains(response, expected_html, html=True)
 
+    def test_notification_preferences_button_with_permission(self):
+        perm = get_notification_setting_permission("change")
+        org = self._get_org()
+        staff_perm = self._create_administrator(
+            username="perm_viewer",
+            email="perm_viewer@test.com",
+            organizations=[org],
+        )
+        staff_perm.user_permissions.add(perm)
+        target_staff = self._create_user(
+            username="staff_target_with_perm",
+            email="staff_target_with_perm@test.com",
+            is_staff=True,
+        )
+        OrganizationUser.objects.create(
+            user=target_staff, organization=org, is_admin=False
+        )
+        target_staff_page = reverse(
+            f"admin:{self.users_app_label}_user_change", args=(target_staff.pk,)
+        )
+        expected_url = reverse(
+            "notifications:user_notification_preference", args=(target_staff.pk,)
+        )
+        expected_html = (
+            f'<a class="button" href="{expected_url}">Notification Preferences</a>'
+        )
+        self.client.force_login(staff_perm)
+        response = self.client.get(target_staff_page)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, expected_html, html=True)
+
+        non_staff = self._create_user(
+            username="non_staff_target",
+            email="non_staff_target@test.com",
+            is_staff=False,
+        )
+        OrganizationUser.objects.create(
+            user=non_staff, organization=org, is_admin=False
+        )
+        non_staff_page = reverse(
+            f"admin:{self.users_app_label}_user_change", args=(non_staff.pk,)
+        )
+        expected_url = reverse(
+            "notifications:user_notification_preference", args=(non_staff.pk,)
+        )
+        expected_html = (
+            f'<a class="button" href="{expected_url}">Notification Preferences</a>'
+        )
+        response = self.client.get(non_staff_page)
+        self.assertEqual(response.status_code, 200)
+        self.assertNotContains(response, expected_html, html=True)
+
+    def test_notification_preferences_button_without_permission(self):
+        org = self._get_org()
+        staff_noperm = self._create_administrator(
+            username="no_perm_viewer",
+            email="no_perm_viewer@test.com",
+            organizations=[org],
+        )
+        perm = get_notification_setting_permission("change")
+        staff_noperm.user_permissions.remove(perm)
+        for group in staff_noperm.groups.all():
+            group.permissions.remove(perm)
+        target_staff = self._create_user(
+            username="staff_target",
+            email="staff_target@test.com",
+            is_staff=True,
+        )
+        OrganizationUser.objects.create(
+            user=target_staff, organization=org, is_admin=False
+        )
+        target_staff_page = reverse(
+            f"admin:{self.users_app_label}_user_change", args=(target_staff.pk,)
+        )
+        expected_url = reverse(
+            "notifications:user_notification_preference", args=(target_staff.pk,)
+        )
+        expected_html = (
+            f'<a class="button" href="{expected_url}">Notification Preferences</a>'
+        )
+        self.client.force_login(staff_noperm)
+        response = self.client.get(target_staff_page)
+        self.assertEqual(response.status_code, 200)
+        self.assertNotContains(response, expected_html, html=True)
+
+    def test_notification_preferences_button_outside_managed_org(self):
+        org_b = self._create_org(name="other org", slug="other-org")
+        perm = get_notification_setting_permission("change")
+        requester = self._create_administrator(
+            username="requester",
+            email="requester@test.com",
+            organizations=[self._get_org()],
+        )
+        requester.user_permissions.add(perm)
+        target_outside = self._create_user(
+            username="outside_target",
+            email="outside_target@test.com",
+            is_staff=True,
+        )
+        OrganizationUser.objects.create(
+            user=target_outside, organization=org_b, is_admin=False
+        )
+
+        with self.subTest("Tag returns False for target outside managed orgs"):
+            self.assertFalse(
+                has_notification_setting_permission(requester, target_outside)
+            )
+
+        with self.subTest("Tag returns True without target user"):
+            self.assertTrue(has_notification_setting_permission(requester))
+
 
 class TestOrganizationNotificationsSettingsAdmin(BaseTestAdmin):
     app_label = "openwisp_notifications"