From f31bb3ae8e4f7717976327efce0dcd9cb88b8772 Mon Sep 17 00:00:00 2001 From: UltraBot05 <170253496+UltraBot05@users.noreply.github.com> Date: Mon, 16 Mar 2026 08:43:09 +0000 Subject: [PATCH 1/2] [fix] Added CIDR and subnet support to FREERADIUS_ALLOWED_HOSTS #229 Closes #229 Added strict=False to ipaddress.ip_network parsing and strip() to string inputs to allow networks with host bits and spaces to be correctly parsed and authenticated. --- openwisp_radius/api/freeradius_views.py | 4 +- openwisp_radius/base/models.py | 11 +++- .../tests/test_api/test_freeradius_api.py | 61 +++++++++++++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/openwisp_radius/api/freeradius_views.py b/openwisp_radius/api/freeradius_views.py index b69232e5..0ff5614e 100644 --- a/openwisp_radius/api/freeradius_views.py +++ b/openwisp_radius/api/freeradius_views.py @@ -105,7 +105,9 @@ def _check_client_ip_and_return(self, request, uuid): for ip in ip_list: try: - if ipaddress.ip_address(client_ip) in ipaddress.ip_network(ip): + if ipaddress.ip_address(client_ip) in ipaddress.ip_network( + (ip or "").strip(), strict=False + ): return (AnonymousUser(), uuid) except ValueError: invalid_addr_message = _( diff --git a/openwisp_radius/base/models.py b/openwisp_radius/base/models.py index 808b8640..909426fa 100644 --- a/openwisp_radius/base/models.py +++ b/openwisp_radius/base/models.py @@ -1338,8 +1338,11 @@ def __str__(self): @property def freeradius_allowed_hosts_list(self): addresses = [] - if self.freeradius_allowed_hosts: - addresses = self.freeradius_allowed_hosts.split(",") + addresses = [ + (ip or "").strip() + for ip in (self.freeradius_allowed_hosts or "").split(",") + if (ip or "").strip() + ] return addresses @property @@ -1383,7 +1386,9 @@ def _clean_freeradius_allowed_hosts(self): else: try: for ip_address in allowed_hosts_set: - ipaddress.ip_network(ip_address) + ip_str = (ip_address or "").strip() + if ip_str: + ipaddress.ip_network(ip_str, strict=False) except ValueError: raise ValidationError( { diff --git a/openwisp_radius/tests/test_api/test_freeradius_api.py b/openwisp_radius/tests/test_api/test_freeradius_api.py index 64968edd..82d022f5 100644 --- a/openwisp_radius/tests/test_api/test_freeradius_api.py +++ b/openwisp_radius/tests/test_api/test_freeradius_api.py @@ -2385,6 +2385,67 @@ def test_ip_from_radsetting_valid(self): self.assertEqual(response.status_code, 200) self.assertEqual(response.data, _AUTH_TYPE_ACCEPT_RESPONSE) + def test_ip_from_radsetting_cidr_range_valid(self): + with mock.patch(self.freeradius_hosts_path, []): + radsetting = OrganizationRadiusSettings.objects.get( + organization=self._get_org() + ) + radsetting.freeradius_allowed_hosts = "172.18.0.0/16" + radsetting.save() + + with mock.patch( + "openwisp_radius.api.freeradius_views.get_client_ip", + return_value=("172.18.0.10", True), + ): + response = self.client.post(reverse("radius:authorize"), self.params) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data, _AUTH_TYPE_ACCEPT_RESPONSE) + + def test_ip_from_radsetting_spaces_and_host_bits_valid(self): + org = self._get_org() + with mock.patch(self.freeradius_hosts_path, []): + radsetting = OrganizationRadiusSettings.objects.get(organization=org) + + radsetting.freeradius_allowed_hosts = "127.0.0.1, 172.18.0.5/16" + radsetting.save() + + self.assertEqual( + cache.get(f"ip-{org.pk}"), + ["127.0.0.1", "172.18.0.5/16"], + ) + + with mock.patch( + "openwisp_radius.api.freeradius_views.get_client_ip", + return_value=("172.18.0.10", True), + ): + response = self.client.post(reverse("radius:authorize"), self.params) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data, _AUTH_TYPE_ACCEPT_RESPONSE) + + @capture_any_output() + def test_ip_outside_cidr_range_rejected(self): + with mock.patch(self.freeradius_hosts_path, []): + radsetting = OrganizationRadiusSettings.objects.get( + organization=self._get_org() + ) + radsetting.freeradius_allowed_hosts = "172.18.0.0/16" + radsetting.save() + + with mock.patch( + "openwisp_radius.api.freeradius_views.get_client_ip", + return_value=("10.0.0.5", True), + ): + response = self.client.post(reverse("radius:authorize"), self.params) + + self.assertEqual(response.status_code, 403) + self.assertEqual( + response.data["detail"], + "Request rejected: Client IP address (10.0.0.5) is not in " + "the list of IP addresses allowed to consume the freeradius API.", + ) + def test_ip_from_setting_valid(self): response = self.client.post(reverse("radius:authorize"), self.params) self.assertEqual(response.status_code, 200) From 07342950d551bdaa75f46112ac7f8a5734ef665e Mon Sep 17 00:00:00 2001 From: UltraBot05 <170253496+UltraBot05@users.noreply.github.com> Date: Mon, 29 Jun 2026 07:00:05 +0000 Subject: [PATCH 2/2] [feature] Drop dead assignment, add docs note, add non-tx CIDR tests #229 freeradius_allowed_hosts_list had a dead `addresses = []` immediately overwritten by the comprehension; removed it. Also removed the stale "(no spaces)" hint from the validation error message since whitespace is now accepted. Updated OPENWISP_RADIUS_FREERADIUS_ALLOWED_HOSTS documentation to note that entries with host bits set and extra whitespace are handled gracefully. Added CIDR accept and reject tests to TestClientIpApi mirroring the existing tests in TestTransactionClientIpApi. Fixes #229 --- docs/user/settings.rst | 4 ++ openwisp_radius/base/models.py | 3 +- openwisp_radius/tests/test_admin.py | 2 +- .../tests/test_api/test_freeradius_api.py | 41 +++++++++++++++++++ 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/docs/user/settings.rst b/docs/user/settings.rst index 67091e91..9c4126c5 100644 --- a/docs/user/settings.rst +++ b/docs/user/settings.rst @@ -190,6 +190,10 @@ You can use subnets when freeradius is hosted on a variable IP, e.g.: - ``198.168.0.0/24`` to allow the entire LAN. - ``0.0.0.0/0`` to allow any address (useful for development / testing). +Entries with host bits set (e.g. ``172.18.0.5/16``) and extra whitespace +around comma-separated entries in the organization settings are handled +gracefully. + This value can be overridden per organization in the organization change page. You can skip setting this option if you intend to set it from organization change page for each organization. diff --git a/openwisp_radius/base/models.py b/openwisp_radius/base/models.py index 099f16d9..66c1178e 100644 --- a/openwisp_radius/base/models.py +++ b/openwisp_radius/base/models.py @@ -1372,7 +1372,6 @@ def __str__(self): @property def freeradius_allowed_hosts_list(self): - addresses = [] addresses = [ (ip or "").strip() for ip in (self.freeradius_allowed_hosts or "").split(",") @@ -1429,7 +1428,7 @@ def _clean_freeradius_allowed_hosts(self): { "freeradius_allowed_hosts": _( "Invalid input. Please enter valid ip addresses " - "or subnets separated by comma. (no spaces)" + "or subnets separated by comma." ) } ) diff --git a/openwisp_radius/tests/test_admin.py b/openwisp_radius/tests/test_admin.py index a6f47a6d..6ace02b0 100644 --- a/openwisp_radius/tests/test_admin.py +++ b/openwisp_radius/tests/test_admin.py @@ -582,7 +582,7 @@ def test_organization_radsettings_freeradius_allowed_hosts(self): response, _( "Invalid input. Please enter valid ip addresses " - "or subnets separated by comma. (no spaces)" + "or subnets separated by comma." ), ) diff --git a/openwisp_radius/tests/test_api/test_freeradius_api.py b/openwisp_radius/tests/test_api/test_freeradius_api.py index 69024817..27c3fcf9 100644 --- a/openwisp_radius/tests/test_api/test_freeradius_api.py +++ b/openwisp_radius/tests/test_api/test_freeradius_api.py @@ -2480,6 +2480,47 @@ def test_ip_from_radsetting_invalid(self): self.assertEqual(response.status_code, 403) self.assertEqual(response.data["detail"], test_fail_msg) + def test_ip_from_radsetting_cidr_range_valid(self): + with mock.patch(self.freeradius_hosts_path, []): + radsetting = OrganizationRadiusSettings.objects.get( + organization=self._get_org() + ) + radsetting.freeradius_allowed_hosts = "172.18.0.0/16" + radsetting.save() + + with mock.patch( + "openwisp_radius.api.freeradius_views.get_client_ip", + return_value=("172.18.0.10", True), + ): + response = self.client.post(reverse("radius:authorize"), self.params) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 200) + self.assertIn("control:Auth-Type", response.data) + self.assertEqual(response.data["control:Auth-Type"], "Accept") + + @capture_any_output() + def test_ip_outside_cidr_range_rejected(self): + with mock.patch(self.freeradius_hosts_path, []): + radsetting = OrganizationRadiusSettings.objects.get( + organization=self._get_org() + ) + radsetting.freeradius_allowed_hosts = "172.18.0.0/16" + radsetting.save() + + with mock.patch( + "openwisp_radius.api.freeradius_views.get_client_ip", + return_value=("10.0.0.5", True), + ): + response = self.client.post(reverse("radius:authorize"), self.params) + + self.assertEqual(response.status_code, 403) + self.assertEqual( + response.data["detail"], + "Request rejected: Client IP address (10.0.0.5) is not in " + "the list of IP addresses allowed to consume the freeradius API.", + ) + class TestTransactionClientIpApi( TestClientIpApiMixin, ApiTokenMixin, BaseTransactionTestCase