diff --git a/docs/user/rest-api.rst b/docs/user/rest-api.rst index 5fbe388c..580058fd 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/admin.py b/openwisp_radius/admin.py index ebd816de..9d82d9bf 100644 --- a/openwisp_radius/admin.py +++ b/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): diff --git a/openwisp_radius/api/serializers.py b/openwisp_radius/api/serializers.py index a9a96490..bf398ea6 100644 --- a/openwisp_radius/api/serializers.py +++ b/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 diff --git a/openwisp_radius/api/urls.py b/openwisp_radius/api/urls.py index 83837448..d7aa9d41 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 1bd77995..cf19fe9d 100644 --- a/openwisp_radius/api/views.py +++ b/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) 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 787dc73a..80a4cb76 100644 --- a/openwisp_radius/tests/test_api/test_api.py +++ b/openwisp_radius/tests/test_api/test_api.py @@ -1073,6 +1073,111 @@ def test_user_group_check_serializer_counter_does_not_exist(self): }, ) + 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 b8efb235..e8257c2d 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 @@ -98,6 +99,10 @@ class RadiusAccountingView(BaseRadiusAccountingView): pass +class BatchUpdateView(BaseBatchUpdateView): + pass + + authorize = AuthorizeView.as_view() postauth = PostAuthView.as_view() accounting = AccountingView.as_view() @@ -116,3 +121,4 @@ 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() 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