[admin/api] Make batch user creation organization field readonly #609

docs/user/rest-api.rst

@@ -820,7 +820,7 @@ This API endpoint allows to use the features described in
     This API endpoint allows to use the features described in
     :doc:`importing_users` and :doc:`generating_users`.
 
-Responds only to **POST**, used to save a ``RadiusBatch`` instance.
+Responds only to **POST**, used to create a ``RadiusBatch`` instance.
 
 It is possible to generate the users of the ``RadiusBatch`` with two
 different strategies: csv or prefix.
@@ -856,6 +856,50 @@ When using this strategy, in the response you can find the field
 ``pdf_link`` which can be used to download a PDF file containing the user
 credentials.
 
+Batch retrieve and update
++++++++++++++++++++++++++
+
+.. code-block:: text
+
+    /api/v1/radius/batch/<id>/
+
+Responds to **GET**, **PUT**, and **PATCH** methods.
+
+Used to retrieve or update a ``RadiusBatch`` instance.
+
+.. note::
+
+    The ``organization`` field is **read-only** for existing batch objects
+    and cannot be changed via the API. This is intentional as changing the
+    organization after batch creation would be inconsistent.
+
+Parameters for **GET**:
+
+===== =================
+Param Description
+===== =================
+id    UUID of the batch
+===== =================
+
+Parameters for **PUT**/**PATCH** (only certain fields can be updated):
+
+=============== ================================================
+Param           Description
+=============== ================================================
+name            Name of the operation
+expiration_date Date of expiration of the users (can be updated)
+=============== ================================================
+
+Fields that are **read-only** and cannot be updated:
+
+- ``organization`` - Cannot be changed after creation
+- ``strategy`` - Cannot be changed after creation
+- ``csvfile`` - Cannot be changed after creation
+- ``prefix`` - Cannot be changed after creation
+- ``users`` - Managed automatically
+- ``user_credentials`` - Generated automatically
+- ``created``, ``modified`` - Timestamps
+
 Batch CSV Download
 ++++++++++++++++++
 

openwisp_radius/admin.py

@@ -463,23 +463,31 @@ def delete_selected_batches(self, request, queryset):
         )
 
     def get_readonly_fields(self, request, obj=None):
-        readonly_fields = super(RadiusBatchAdmin, self).get_readonly_fields(
-            request, obj
-        )
+        readonly_fields = super().get_readonly_fields(request, obj)
         if obj and obj.status != "pending":
             return (
                 "strategy",
+                "organization",
                 "prefix",
                 "csvfile",
                 "number_of_users",
                 "users",
                 "expiration_date",
                 "name",
-                "organization",
                 "status",
             ) + readonly_fields
         elif obj:
-            return ("status",) + readonly_fields
+            # For existing objects with pending status, still make organization readonly
+            return readonly_fields + (
+                "strategy",
+                "organization",
+                "prefix",
+                "csvfile",
+                "number_of_users",
+                "users",
+                "expiration_date",
+                "status",
+            )
         return ("status",) + readonly_fields
 
     def has_delete_permission(self, request, obj=None):

openwisp_radius/api/serializers.py

@@ -453,6 +453,48 @@ class Meta:
         read_only_fields = ("status", "user_credentials", "created", "modified")
 
 
+class RadiusBatchUpdateSerializer(RadiusBatchSerializer):
+    """
+    Serializer for updating RadiusBatch objects.
+    Makes the organization field readonly for existing objects.
+    """
+
+    organization_slug = RadiusOrganizationField(
+        help_text=("Slug of the organization for creating radius batch."),
+        required=False,
+        label="organization",
+        slug_field="slug",
+        write_only=True,
+    )
+
+    def validate(self, data):
+        """
+        Simplified validation for partial updates.
+        Only validates essential fields based on strategy.
+        Ignores organization_slug if provided since organization is readonly.
+        """
+        # Remove organization_slug from data if provided (should not be changeable)
+        data.pop("organization_slug", None)
+
+        strategy = data.get("strategy") or (self.instance and self.instance.strategy)
+
+        if (
+            strategy == "prefix"
+            and "number_of_users" in data
+            and not data.get("number_of_users")
+        ):
+            raise serializers.ValidationError(
+                {"number_of_users": _("The field number_of_users cannot be empty")}
+            )
+
+        return serializers.ModelSerializer.validate(self, data)
+
+    class Meta:
+        model = RadiusBatch
+        fields = "__all__"
+        read_only_fields = ("created", "modified", "user_credentials", "organization")
+
+
 class PasswordResetSerializer(BasePasswordResetSerializer):
     input = serializers.CharField()
     email = None

openwisp_radius/api/urls.py

@@ -78,6 +78,9 @@ def get_api_urls(api_views=None):
                 name="phone_number_change",
             ),
             path("radius/batch/", api_views.batch, name="batch"),
+            path(
+                "radius/batch/<uuid:pk>/", api_views.batch_detail, name="batch_detail"
+            ),
             path(
                 "radius/organization/<slug:slug>/batch/<uuid:pk>/pdf/",
                 api_views.download_rad_batch_pdf,

openwisp_radius/api/views.py

@@ -31,6 +31,7 @@
     GenericAPIView,
     ListAPIView,
     RetrieveAPIView,
+    RetrieveUpdateAPIView,
 )
 from rest_framework.permissions import (
     DjangoModelPermissions,
@@ -62,6 +63,7 @@
     ChangePhoneNumberSerializer,
     RadiusAccountingSerializer,
     RadiusBatchSerializer,
+    RadiusBatchUpdateSerializer,
     UserRadiusUsageSerializer,
     ValidatePhoneTokenSerializer,
 )
@@ -144,6 +146,34 @@ def validate_membership(self, user):
             raise serializers.ValidationError({"non_field_errors": [message]})
 
 
+class BatchUpdateView(ThrottledAPIMixin, RetrieveUpdateAPIView):
+    """
+    API view for updating existing RadiusBatch objects.
+    Organization field is readonly for existing objects.
+    """
+
+    authentication_classes = (BearerAuthentication, SessionAuthentication)
+    permission_classes = (IsOrganizationManager, IsAdminUser, DjangoModelPermissions)
+    queryset = RadiusBatch.objects.none()
+    serializer_class = RadiusBatchSerializer
+
+    def get_queryset(self):
+        """Filter batches by user's organization membership"""
+        user = self.request.user
+        if user.is_superuser:
+            return RadiusBatch.objects.all()
+        return RadiusBatch.objects.filter(organization__in=user.organizations_managed)
+
+    def get_serializer_class(self):
+        """Use RadiusBatchUpdateSerializer for PUT/PATCH requests"""
+        if self.request.method in ("PUT", "PATCH"):
+            return RadiusBatchUpdateSerializer
+        return RadiusBatchSerializer
+
+
+batch_detail = BatchUpdateView.as_view()
+
+
 class DownloadRadiusBatchPdfView(ThrottledAPIMixin, DispatchOrgMixin, RetrieveAPIView):
     authentication_classes = (BearerAuthentication, SessionAuthentication)
     permission_classes = (IsOrganizationManager, IsAdminUser, DjangoModelPermissions)

openwisp_radius/integrations/monitoring/tasks.py

@@ -95,6 +95,15 @@ def _write_user_signup_metric_for_all(metric_key):
     except KeyError:
         total_registered_users[""] = users_without_registereduser
 
+    from openwisp_radius.registration import REGISTRATION_METHOD_CHOICES
+
+    all_methods = [clean_registration_method(m) for m, _ in REGISTRATION_METHOD_CHOICES]
+    for m in all_methods:
+        existing_methods = [
+            clean_registration_method(k) for k in total_registered_users.keys()
+        ]
+        if m not in existing_methods:
+            total_registered_users[m] = 0
     for method, count in total_registered_users.items():
         method = clean_registration_method(method)
         metric = get_metric_func(organization_id="__all__", registration_method=method)

openwisp_radius/integrations/monitoring/tests/test_metrics.py

@@ -389,7 +389,13 @@ def _read_chart(chart, **kwargs):
         with self.subTest(
             "User does not has OrganizationUser and RegisteredUser object"
         ):
-            self._get_admin()
+            admin = self._get_admin()
+            try:
+                reg_user = RegisteredUser.objects.get(user=admin)
+                reg_user.method = ""
+                reg_user.save()
+            except RegisteredUser.DoesNotExist:
+                pass
             write_user_registration_metrics.delay()
 
             user_signup_chart = user_signup_metric.chart_set.first()

openwisp_radius/tests/test_admin.py

@@ -520,10 +520,6 @@ def test_organization_radsettings_freeradius_allowed_hosts(self):
                 "radius_settings-0-id": radsetting.pk,
                 "radius_settings-0-organization": org.pk,
                 "radius_settings-0-password_reset_url": PASSWORD_RESET_URL,
-                "notification_settings-TOTAL_FORMS": 0,
-                "notification_settings-INITIAL_FORMS": 0,
-                "notification_settings-MIN_NUM_FORMS": 0,
-                "notification_settings-MAX_NUM_FORMS": 1,
             }
         )
 
@@ -606,10 +602,6 @@ def test_organization_radsettings_password_reset_url(self):
                 "radius_settings-0-id": radsetting.pk,
                 "radius_settings-0-organization": org.pk,
                 "radius_settings-0-password_reset_url": PASSWORD_RESET_URL,
-                "notification_settings-TOTAL_FORMS": 0,
-                "notification_settings-INITIAL_FORMS": 0,
-                "notification_settings-MIN_NUM_FORMS": 0,
-                "notification_settings-MAX_NUM_FORMS": 1,
             }
         )
 
@@ -1220,10 +1212,6 @@ def test_organization_radsettings_allowed_mobile_prefixes(self):
                 "radius_settings-0-id": radsetting.pk,
                 "radius_settings-0-organization": org.pk,
                 "radius_settings-0-password_reset_url": PASSWORD_RESET_URL,
-                "notification_settings-TOTAL_FORMS": 0,
-                "notification_settings-INITIAL_FORMS": 0,
-                "notification_settings-MIN_NUM_FORMS": 0,
-                "notification_settings-MAX_NUM_FORMS": 1,
             }
         )
 
@@ -1301,10 +1289,6 @@ def test_organization_radsettings_sms_message(self):
                 "radius_settings-0-id": radsetting.pk,
                 "radius_settings-0-organization": org.pk,
                 "radius_settings-0-password_reset_url": PASSWORD_RESET_URL,
-                "notification_settings-TOTAL_FORMS": 0,
-                "notification_settings-INITIAL_FORMS": 0,
-                "notification_settings-MIN_NUM_FORMS": 0,
-                "notification_settings-MAX_NUM_FORMS": 1,
                 "_continue": True,
             }
         )
@@ -1511,56 +1495,30 @@ def test_admin_menu_groups(self):
             html = '<div class="mg-dropdown-label">RADIUS </div>'
             self.assertContains(response, html, html=True)
 
+    def test_radiusbatch_organization_readonly_for_existing_objects(self):
+        """
+        Test that organization field is readonly for existing RadiusBatch objects
+        """
+        batch = self._create_radius_batch(
+            name="test-batch", strategy="prefix", prefix="test-prefix"
+        )
+        url = reverse(f"admin:{self.app_label}_radiusbatch_change", args=[batch.pk])
 
-class TestRadiusGroupAdmin(BaseTestCase):
-    def setUp(self):
-        self.organization = self._create_org()
-        self.admin = self._create_admin()
-        self.organization.add_user(self.admin, is_admin=True)
-        self.client.force_login(self.admin)
-
-    def test_add_radiusgroup_with_inline_check_succeeds(self):
-        add_url = reverse("admin:openwisp_radius_radiusgroup_add")
-
-        post_data = {
-            # Main RadiusGroup form
-            "organization": self.organization.pk,
-            "name": "test-group-with-inline",
-            "description": "A test group created with an inline check",
-            # Inline RadiusGroupCheck formset
-            "radiusgroupcheck_set-TOTAL_FORMS": "1",
-            "radiusgroupcheck_set-INITIAL_FORMS": "0",
-            "radiusgroupcheck_set-0-attribute": "Max-Daily-Session",
-            "radiusgroupcheck_set-0-op": ":=",
-            "radiusgroupcheck_set-0-value": "3600",
-            # Inline RadiusGroupReply formset
-            "radiusgroupreply_set-TOTAL_FORMS": "1",
-            "radiusgroupreply_set-INITIAL_FORMS": "0",
-            "radiusgroupreply_set-0-attribute": "Session-Timeout",
-            "radiusgroupreply_set-0-op": "=",
-            "radiusgroupreply_set-0-value": "1800",
-        }
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+
+        self.assertContains(response, "readonly")
+        self.assertContains(response, batch.organization.name)
 
-        response = self.client.post(add_url, data=post_data, follow=True)
+    def test_radiusbatch_organization_editable_for_new_objects(self):
+        """
+        Test that organization field is editable for new RadiusBatch objects
+        """
+        url = reverse(f"admin:{self.app_label}_radiusbatch_add")
 
+        response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
-        final_group_name = f"{self.organization.slug}-test-group-with-inline"
-
-        self.assertContains(response, "The group")
-        self.assertContains(response, f"{final_group_name}</a>")
-        self.assertContains(response, "was added successfully.")
-
-        self.assertTrue(RadiusGroup.objects.filter(name=final_group_name).exists())
-        group = RadiusGroup.objects.get(name=final_group_name)
-
-        self.assertEqual(group.radiusgroupcheck_set.count(), 1)
-        check = group.radiusgroupcheck_set.first()
-        self.assertEqual(check.attribute, "Max-Daily-Session")
-        self.assertEqual(check.value, "3600")
-        self.assertEqual(check.groupname, group.name)
-
-        self.assertEqual(group.radiusgroupreply_set.count(), 1)
-        reply = group.radiusgroupreply_set.first()
-        self.assertEqual(reply.attribute, "Session-Timeout")
-        self.assertEqual(reply.value, "1800")
-        self.assertEqual(reply.groupname, group.name)
+
+        self.assertContains(response, 'name="organization"')
+        form_html = response.content.decode()
+        self.assertIn('name="organization"', form_html)