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/api/freeradius_views.py b/openwisp_radius/api/freeradius_views.py index acf10621..63214d55 100644 --- a/openwisp_radius/api/freeradius_views.py +++ b/openwisp_radius/api/freeradius_views.py @@ -106,7 +106,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 54f6cdfb..b11a3827 100644 --- a/openwisp_radius/base/models.py +++ b/openwisp_radius/base/models.py @@ -1446,9 +1446,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 @@ -1492,13 +1494,15 @@ 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( { "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 55ea5e49..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 @@ -2519,6 +2560,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)