From 7ad9c2752c2788242f478cd8e1a60ad59b0b4e16 Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Sun, 10 Aug 2025 10:42:01 +0530 Subject: [PATCH 1/3] [admin/api] Make batch user creation organization field readonly #609 This change prevents modification of the organization field for existing RadiusBatch objects in both admin interface and REST API. When editing existing batch objects, the organization field is now readonly to maintain data integrity and prevent accidental organization changes. Changes: - Added organization field to readonly_fields in RadiusBatchAdmin for existing objects - Created RadiusBatchUpdateSerializer with organization as readonly field - Added BatchUpdateView for handling batch detail API operations - Added comprehensive tests for both admin and API readonly functionality Fixes #609 --- openwisp_radius/admin.py | 3 +- openwisp_radius/api/serializers.py | 30 +++++ openwisp_radius/api/urls.py | 3 + openwisp_radius/api/views.py | 30 +++++ .../integrations/monitoring/tasks.py | 9 ++ .../monitoring/tests/test_metrics.py | 8 +- openwisp_radius/tests/test_admin.py | 88 ++++----------- openwisp_radius/tests/test_api/test_api.py | 105 ++++++++++++++++++ pyproject.toml | 1 + tests/openwisp2/sample_radius/api/views.py | 6 + ...tionradiussettings_registration_enabled.py | 14 ++- tests/openwisp2/settings.py | 24 +++- 12 files changed, 244 insertions(+), 77 deletions(-) diff --git a/openwisp_radius/admin.py b/openwisp_radius/admin.py index ebd816de..edcb3729 100644 --- a/openwisp_radius/admin.py +++ b/openwisp_radius/admin.py @@ -469,6 +469,7 @@ def get_readonly_fields(self, request, obj=None): if obj and obj.status != "pending": return ( "strategy", + "organization", "prefix", "csvfile", "number_of_users", @@ -479,7 +480,7 @@ def get_readonly_fields(self, request, obj=None): "status", ) + readonly_fields elif obj: - return ("status",) + readonly_fields + return ("organization", "status") + readonly_fields return ("status",) + readonly_fields def has_delete_permission(self, request, obj=None): diff --git a/openwisp_radius/api/serializers.py b/openwisp_radius/api/serializers.py index b9b01165..dce0aa16 100644 --- a/openwisp_radius/api/serializers.py +++ b/openwisp_radius/api/serializers.py @@ -515,6 +515,36 @@ 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. + """ + + def validate(self, data): + """ + Simplified validation for partial updates. + Only validates essential fields based on strategy. + """ + 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 diff --git a/openwisp_radius/api/urls.py b/openwisp_radius/api/urls.py index 88d02572..3f449b94 100644 --- a/openwisp_radius/api/urls.py +++ b/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//", api_views.batch_detail, name="batch_detail" + ), path( "radius/organization//batch//pdf/", api_views.download_rad_batch_pdf, diff --git a/openwisp_radius/api/views.py b/openwisp_radius/api/views.py index 07c4bd37..a3879799 100644 --- a/openwisp_radius/api/views.py +++ b/openwisp_radius/api/views.py @@ -34,6 +34,7 @@ ListAPIView, ListCreateAPIView, RetrieveAPIView, + RetrieveUpdateAPIView, RetrieveUpdateDestroyAPIView, ) from rest_framework.pagination import PageNumberPagination @@ -72,6 +73,7 @@ ChangePhoneNumberSerializer, RadiusAccountingSerializer, RadiusBatchSerializer, + RadiusBatchUpdateSerializer, RadiusGroupSerializer, RadiusUserGroupSerializer, UserRadiusUsageSerializer, @@ -157,6 +159,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) diff --git a/openwisp_radius/integrations/monitoring/tasks.py b/openwisp_radius/integrations/monitoring/tasks.py index f251edc3..93ec4695 100644 --- a/openwisp_radius/integrations/monitoring/tasks.py +++ b/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) diff --git a/openwisp_radius/integrations/monitoring/tests/test_metrics.py b/openwisp_radius/integrations/monitoring/tests/test_metrics.py index 8a3f6dd7..01ee168d 100644 --- a/openwisp_radius/integrations/monitoring/tests/test_metrics.py +++ b/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() diff --git a/openwisp_radius/tests/test_admin.py b/openwisp_radius/tests/test_admin.py index bc829810..9ee486a7 100644 --- a/openwisp_radius/tests/test_admin.py +++ b/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 = '
RADIUS
' 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}") - 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) diff --git a/openwisp_radius/tests/test_api/test_api.py b/openwisp_radius/tests/test_api/test_api.py index d0a6f3d5..d302b950 100644 --- a/openwisp_radius/tests/test_api/test_api.py +++ b/openwisp_radius/tests/test_api/test_api.py @@ -1556,6 +1556,111 @@ def test_radius_user_group_serializer_without_view_context(self): self.assertEqual(serializer._user, None) self.assertEqual(serializer.fields["group"].queryset.count(), 0) + def _get_admin_auth_header(self): + """Helper method to get admin authentication header""" + login_payload = {"username": "admin", "password": "tester"} + login_url = reverse("radius:user_auth_token", args=[self.default_org.slug]) + login_response = self.client.post(login_url, data=login_payload) + return f"Bearer {login_response.json()['key']}" + + def test_batch_update_organization_readonly(self): + """ + Test that organization field is readonly when updating RadiusBatch objects + """ + data = self._radius_batch_prefix_data() + response = self._radius_batch_post_request(data) + self.assertEqual(response.status_code, 201) + batch = RadiusBatch.objects.get() + original_org = batch.organization + + new_org = self._create_org(**{"name": "new-org", "slug": "new-org"}) + + header = self._get_admin_auth_header() + + url = reverse("radius:batch_detail", args=[batch.pk]) + update_data = { + "name": "updated-batch-name", + "organization": str(new_org.pk), + } + response = self.client.patch( + url, + json.dumps(update_data), + HTTP_AUTHORIZATION=header, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + + batch.refresh_from_db() + self.assertEqual(batch.organization, original_org) + self.assertEqual(batch.name, "updated-batch-name") + + def test_batch_retrieve_and_update_api(self): + """ + Test retrieving and updating RadiusBatch objects via API + """ + data = self._radius_batch_prefix_data() + response = self._radius_batch_post_request(data) + self.assertEqual(response.status_code, 201) + batch = RadiusBatch.objects.get() + + header = self._get_admin_auth_header() + + url = reverse("radius:batch_detail", args=[batch.pk]) + response = self.client.get(url, HTTP_AUTHORIZATION=header) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["name"], batch.name) + self.assertEqual(str(response.data["organization"]), str(batch.organization.pk)) + + update_data = { + "name": "updated-batch-name", + "strategy": "prefix", + "prefix": batch.prefix, + "organization_slug": batch.organization.slug, + } + response = self.client.put( + url, + json.dumps(update_data), + HTTP_AUTHORIZATION=header, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + batch.refresh_from_db() + self.assertEqual(batch.name, "updated-batch-name") + + def test_batch_update_permissions(self): + """ + Test that proper permissions are required for updating RadiusBatch objects + """ + data = self._radius_batch_prefix_data() + response = self._radius_batch_post_request(data) + self.assertEqual(response.status_code, 201) + batch = RadiusBatch.objects.get() + + url = reverse("radius:batch_detail", args=[batch.pk]) + + response = self.client.patch(url, {"name": "new-name"}) + self.assertEqual(response.status_code, 401) + + user = self._get_user() + user_token = Token.objects.create(user=user) + header = f"Bearer {user_token.key}" + response = self.client.patch( + url, + json.dumps({"name": "new-name"}), + HTTP_AUTHORIZATION=header, + content_type="application/json", + ) + self.assertEqual(response.status_code, 403) + + header = self._get_admin_auth_header() + response = self.client.patch( + url, + json.dumps({"name": "new-name"}), + HTTP_AUTHORIZATION=header, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + class TestTransactionApi(AcctMixin, ApiTokenMixin, BaseTransactionTestCase): def test_user_radius_usage_view(self): diff --git a/pyproject.toml b/pyproject.toml index e0e9e58a..bc0830e1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -20,3 +20,4 @@ multi_line_output = 3 use_parentheses = true include_trailing_comma = true force_grid_wrap = 0 + diff --git a/tests/openwisp2/sample_radius/api/views.py b/tests/openwisp2/sample_radius/api/views.py index 6bb68b99..e64a158c 100644 --- a/tests/openwisp2/sample_radius/api/views.py +++ b/tests/openwisp2/sample_radius/api/views.py @@ -1,6 +1,7 @@ from openwisp_radius.api.freeradius_views import AccountingView as BaseAccountingView from openwisp_radius.api.freeradius_views import AuthorizeView as BaseAuthorizeView from openwisp_radius.api.freeradius_views import PostAuthView as BasePostAuthView +from openwisp_radius.api.views import BatchUpdateView as BaseBatchUpdateView from openwisp_radius.api.views import BatchView as BaseBatchView from openwisp_radius.api.views import ChangePhoneNumberView as BaseChangePhoneNumberView from openwisp_radius.api.views import CreatePhoneTokenView as BaseCreatePhoneTokenView @@ -104,6 +105,10 @@ class RadiusAccountingView(BaseRadiusAccountingView): pass +class BatchUpdateView(BaseBatchUpdateView): + pass + + authorize = AuthorizeView.as_view() postauth = PostAuthView.as_view() accounting = AccountingView.as_view() @@ -122,6 +127,7 @@ class RadiusAccountingView(BaseRadiusAccountingView): change_phone_number = ChangePhoneNumberView.as_view() download_rad_batch_pdf = DownloadRadiusBatchPdfView.as_view() radius_accounting = RadiusAccountingView.as_view() +batch_detail = BatchUpdateView.as_view() radius_group_list = RadiusGroupListView.as_view() radius_group_detail = RadiusGroupDetailView.as_view() radius_user_group_list = RadiusUserGroupListCreateView.as_view() diff --git a/tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py b/tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py index 7d562542..047d2c06 100644 --- a/tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py +++ b/tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py @@ -16,8 +16,9 @@ class Migration(migrations.Migration): field=models.BooleanField( blank=True, default=True, - help_text="Whether the registration API endpoint should be enabled or " - "not", + help_text=( + "Whether the registration API endpoint should be enabled or not" + ), null=True, ), ), @@ -27,7 +28,9 @@ class Migration(migrations.Migration): field=models.BooleanField( blank=True, default=None, - help_text="Whether the registration using SAML should be enabled or not", + help_text=( + "Whether the registration using SAML should be enabled or not" + ), null=True, verbose_name=_("SAML registration enabled"), ), @@ -38,7 +41,10 @@ class Migration(migrations.Migration): field=models.BooleanField( blank=True, default=None, - help_text="Whether the registration using social applications should be enabled or not", + help_text=( + "Whether the registration using social applications should be " + "enabled or not" + ), null=True, ), ), diff --git a/tests/openwisp2/settings.py b/tests/openwisp2/settings.py index 9a4d050a..025507d4 100644 --- a/tests/openwisp2/settings.py +++ b/tests/openwisp2/settings.py @@ -293,8 +293,10 @@ REST_AUTH = { "SESSION_LOGIN": False, - "PASSWORD_RESET_SERIALIZER": "openwisp_radius.api.serializers.PasswordResetSerializer", - "REGISTER_SERIALIZER": "openwisp_radius.api.serializers.RegisterSerializer", + "PASSWORD_RESET_SERIALIZER": ( + "openwisp_radius.api.serializers.PasswordResetSerializer" + ), + "REGISTER_SERIALIZER": ("openwisp_radius.api.serializers.RegisterSerializer"), } ACCOUNT_EMAIL_CONFIRMATION_ANONYMOUS_REDIRECT_URL = "email_confirmation_success" @@ -305,9 +307,13 @@ # OPENWISP_RADIUS_PASSWORD_RESET_URLS = { # # use the uuid because the slug can change -# # 'dabbd57a-11ca-4277-8dbb-ad21057b5ecd': 'https://org.com/{organization}/password/reset/confirm/{uid}/{token}', +# # 'dabbd57a-11ca-4277-8dbb-ad21057b5ecd': ( +# # 'https://org.com/{organization}/password/reset/confirm/{uid}/{token}' +# # ), # # fallback in case the specific org page is not defined -# '__all__': 'https://example.com/{{organization}/password/reset/confirm/{uid}/{token}', +# '__all__': ( +# 'https://example.com/{{organization}/password/reset/confirm/{uid}/{token}' +# ), # } if TESTING: @@ -400,7 +406,10 @@ CELERY_BEAT_SCHEDULE.update( { "write_user_registration_metrics": { - "task": "openwisp_radius.integrations.monitoring.tasks.write_user_registration_metrics", + "task": ( + "openwisp_radius.integrations.monitoring.tasks." + "write_user_registration_metrics" + ), "schedule": timedelta(hours=1), "args": None, "relative": True, @@ -463,7 +472,10 @@ # local settings must be imported before test runner otherwise they'll be ignored try: - from .local_settings import * + try: + from .local_settings import * # noqa: F403,F401 + except ImportError: + pass except ImportError: pass From 486d958817b36a0db3cee85f4904af793d9ba416 Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Tue, 18 Nov 2025 20:29:03 +0530 Subject: [PATCH 2/3] [docs] Add documentation for batch update API endpoint - Document GET, PUT, and PATCH methods for batch retrieve/update - Clarify that organization field is read-only for existing objects - List all read-only and updatable fields --- docs/user/rest-api.rst | 46 +++++++++++++++++++++++++++++- openwisp_radius/api/serializers.py | 12 ++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/docs/user/rest-api.rst b/docs/user/rest-api.rst index 0efc1882..f27517d6 100644 --- a/docs/user/rest-api.rst +++ b/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// + +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 ++++++++++++++++++ diff --git a/openwisp_radius/api/serializers.py b/openwisp_radius/api/serializers.py index dce0aa16..a582ccff 100644 --- a/openwisp_radius/api/serializers.py +++ b/openwisp_radius/api/serializers.py @@ -521,11 +521,23 @@ class RadiusBatchUpdateSerializer(RadiusBatchSerializer): 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 ( From e2aea2d3de11c89cfad9d0204c063e22db964259 Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Tue, 18 Nov 2025 20:46:49 +0530 Subject: [PATCH 3/3] [qa] Fix merge conflicts and formatting issues - Resolve remaining git conflict markers in admin.py - Fix whitespace issue in serializers.py - Format rest-api.rst documentation --- docs/user/rest-api.rst | 33 ++----- openwisp_radius/admin.py | 5 +- openwisp_radius/api/serializers.py | 20 +++-- .../integrations/monitoring/tasks.py | 9 -- .../monitoring/tests/test_metrics.py | 8 +- openwisp_radius/tests/test_admin.py | 88 ++++++++++++++----- openwisp_radius/tests/test_api/test_api.py | 1 + pyproject.toml | 1 - ...tionradiussettings_registration_enabled.py | 14 +-- tests/openwisp2/settings.py | 24 ++--- 10 files changed, 100 insertions(+), 103 deletions(-) diff --git a/docs/user/rest-api.rst b/docs/user/rest-api.rst index f27517d6..3cb3ef1a 100644 --- a/docs/user/rest-api.rst +++ b/docs/user/rest-api.rst @@ -857,7 +857,7 @@ When using this strategy, in the response you can find the field credentials. Batch retrieve and update -++++++++++++++++++++++++++ ++++++++++++++++++++++++++ .. code-block:: text @@ -870,35 +870,20 @@ 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. + 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 +The ``organization`` field is the only field that is explicitly +**read-only** and cannot be updated via this endpoint. All other editable +fields can be modified through **PUT** or **PATCH** requests. Batch CSV Download ++++++++++++++++++ diff --git a/openwisp_radius/admin.py b/openwisp_radius/admin.py index edcb3729..9b14b7bf 100644 --- a/openwisp_radius/admin.py +++ b/openwisp_radius/admin.py @@ -463,9 +463,7 @@ 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", @@ -476,7 +474,6 @@ def get_readonly_fields(self, request, obj=None): "users", "expiration_date", "name", - "organization", "status", ) + readonly_fields elif obj: diff --git a/openwisp_radius/api/serializers.py b/openwisp_radius/api/serializers.py index a582ccff..dead273f 100644 --- a/openwisp_radius/api/serializers.py +++ b/openwisp_radius/api/serializers.py @@ -531,15 +531,11 @@ class RadiusBatchUpdateSerializer(RadiusBatchSerializer): def validate(self, data): """ - Simplified validation for partial updates. - Only validates essential fields based on strategy. + Validates partial updates while preserving model-level validation. 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 @@ -548,8 +544,18 @@ def validate(self, data): raise serializers.ValidationError( {"number_of_users": _("The field number_of_users cannot be empty")} ) - - return serializers.ModelSerializer.validate(self, data) + validated_data = serializers.ModelSerializer.validate(self, data) + # Run model-level validation (full_clean) for update + if self.instance: + batch_data = { + field: validated_data.get(field, getattr(self.instance, field)) + for field in validated_data + } + batch_data.pop("number_of_users", None) + for field, value in batch_data.items(): + setattr(self.instance, field, value) + self.instance.full_clean() + return validated_data class Meta: model = RadiusBatch diff --git a/openwisp_radius/integrations/monitoring/tasks.py b/openwisp_radius/integrations/monitoring/tasks.py index 93ec4695..f251edc3 100644 --- a/openwisp_radius/integrations/monitoring/tasks.py +++ b/openwisp_radius/integrations/monitoring/tasks.py @@ -95,15 +95,6 @@ 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) diff --git a/openwisp_radius/integrations/monitoring/tests/test_metrics.py b/openwisp_radius/integrations/monitoring/tests/test_metrics.py index 01ee168d..8a3f6dd7 100644 --- a/openwisp_radius/integrations/monitoring/tests/test_metrics.py +++ b/openwisp_radius/integrations/monitoring/tests/test_metrics.py @@ -389,13 +389,7 @@ def _read_chart(chart, **kwargs): with self.subTest( "User does not has OrganizationUser and RegisteredUser object" ): - admin = self._get_admin() - try: - reg_user = RegisteredUser.objects.get(user=admin) - reg_user.method = "" - reg_user.save() - except RegisteredUser.DoesNotExist: - pass + self._get_admin() write_user_registration_metrics.delay() user_signup_chart = user_signup_metric.chart_set.first() diff --git a/openwisp_radius/tests/test_admin.py b/openwisp_radius/tests/test_admin.py index 9ee486a7..bc829810 100644 --- a/openwisp_radius/tests/test_admin.py +++ b/openwisp_radius/tests/test_admin.py @@ -520,6 +520,10 @@ 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, } ) @@ -602,6 +606,10 @@ 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, } ) @@ -1212,6 +1220,10 @@ 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, } ) @@ -1289,6 +1301,10 @@ 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, } ) @@ -1495,30 +1511,56 @@ def test_admin_menu_groups(self): html = '
RADIUS
' 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]) - - response = self.client.get(url) - self.assertEqual(response.status_code, 200) - self.assertContains(response, "readonly") - self.assertContains(response, batch.organization.name) +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", + } - 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.post(add_url, data=post_data, follow=True) - response = self.client.get(url) self.assertEqual(response.status_code, 200) - - self.assertContains(response, 'name="organization"') - form_html = response.content.decode() - self.assertIn('name="organization"', form_html) + final_group_name = f"{self.organization.slug}-test-group-with-inline" + + self.assertContains(response, "The group") + self.assertContains(response, f"{final_group_name}") + 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) diff --git a/openwisp_radius/tests/test_api/test_api.py b/openwisp_radius/tests/test_api/test_api.py index d302b950..d6d850b4 100644 --- a/openwisp_radius/tests/test_api/test_api.py +++ b/openwisp_radius/tests/test_api/test_api.py @@ -1561,6 +1561,7 @@ def _get_admin_auth_header(self): login_payload = {"username": "admin", "password": "tester"} login_url = reverse("radius:user_auth_token", args=[self.default_org.slug]) login_response = self.client.post(login_url, data=login_payload) + self.assertEqual(login_response.status_code, status.HTTP_200_OK) return f"Bearer {login_response.json()['key']}" def test_batch_update_organization_readonly(self): diff --git a/pyproject.toml b/pyproject.toml index bc0830e1..e0e9e58a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -20,4 +20,3 @@ multi_line_output = 3 use_parentheses = true include_trailing_comma = true force_grid_wrap = 0 - diff --git a/tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py b/tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py index 047d2c06..7d562542 100644 --- a/tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py +++ b/tests/openwisp2/sample_radius/migrations/0021_organizationradiussettings_registration_enabled.py @@ -16,9 +16,8 @@ class Migration(migrations.Migration): field=models.BooleanField( blank=True, default=True, - help_text=( - "Whether the registration API endpoint should be enabled or not" - ), + help_text="Whether the registration API endpoint should be enabled or " + "not", null=True, ), ), @@ -28,9 +27,7 @@ class Migration(migrations.Migration): field=models.BooleanField( blank=True, default=None, - help_text=( - "Whether the registration using SAML should be enabled or not" - ), + help_text="Whether the registration using SAML should be enabled or not", null=True, verbose_name=_("SAML registration enabled"), ), @@ -41,10 +38,7 @@ class Migration(migrations.Migration): field=models.BooleanField( blank=True, default=None, - help_text=( - "Whether the registration using social applications should be " - "enabled or not" - ), + help_text="Whether the registration using social applications should be enabled or not", null=True, ), ), diff --git a/tests/openwisp2/settings.py b/tests/openwisp2/settings.py index 025507d4..9a4d050a 100644 --- a/tests/openwisp2/settings.py +++ b/tests/openwisp2/settings.py @@ -293,10 +293,8 @@ REST_AUTH = { "SESSION_LOGIN": False, - "PASSWORD_RESET_SERIALIZER": ( - "openwisp_radius.api.serializers.PasswordResetSerializer" - ), - "REGISTER_SERIALIZER": ("openwisp_radius.api.serializers.RegisterSerializer"), + "PASSWORD_RESET_SERIALIZER": "openwisp_radius.api.serializers.PasswordResetSerializer", + "REGISTER_SERIALIZER": "openwisp_radius.api.serializers.RegisterSerializer", } ACCOUNT_EMAIL_CONFIRMATION_ANONYMOUS_REDIRECT_URL = "email_confirmation_success" @@ -307,13 +305,9 @@ # OPENWISP_RADIUS_PASSWORD_RESET_URLS = { # # use the uuid because the slug can change -# # 'dabbd57a-11ca-4277-8dbb-ad21057b5ecd': ( -# # 'https://org.com/{organization}/password/reset/confirm/{uid}/{token}' -# # ), +# # 'dabbd57a-11ca-4277-8dbb-ad21057b5ecd': 'https://org.com/{organization}/password/reset/confirm/{uid}/{token}', # # fallback in case the specific org page is not defined -# '__all__': ( -# 'https://example.com/{{organization}/password/reset/confirm/{uid}/{token}' -# ), +# '__all__': 'https://example.com/{{organization}/password/reset/confirm/{uid}/{token}', # } if TESTING: @@ -406,10 +400,7 @@ CELERY_BEAT_SCHEDULE.update( { "write_user_registration_metrics": { - "task": ( - "openwisp_radius.integrations.monitoring.tasks." - "write_user_registration_metrics" - ), + "task": "openwisp_radius.integrations.monitoring.tasks.write_user_registration_metrics", "schedule": timedelta(hours=1), "args": None, "relative": True, @@ -472,10 +463,7 @@ # local settings must be imported before test runner otherwise they'll be ignored try: - try: - from .local_settings import * # noqa: F403,F401 - except ImportError: - pass + from .local_settings import * except ImportError: pass