[feature] Added CIDR and subnet support to FREERADIUS_ALLOWED_HOSTS #229#694
[feature] Added CIDR and subnet support to FREERADIUS_ALLOWED_HOSTS #229#694UltraBot05 wants to merge 1 commit intoopenwisp:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (4)
📝 WalkthroughWalkthroughThe changes add CIDR/range support and whitespace-tolerant parsing for FreeRADIUS allowed hosts. Model properties and validators now split comma-separated entries, trim each entry, ignore empties, and validate networks using ip_network(..., strict=False). The API view constructs ip_network from the stripped client IP when checking membership against allowed networks. Three tests were added to cover CIDR ranges, entries with spaces/host bits, and rejection of IPs outside allowed ranges. Sequence Diagram(s)sequenceDiagram
participant Client
participant API_View as Freeradius API View
participant Settings as RadiusSettings Parser
participant Validator as IP Network Validator
participant Cache as Cache/DB
Client->>API_View: request from client IP
API_View->>Settings: freeradius_allowed_hosts_list (split & trim)
Settings->>Cache: read/write cached hosts list
API_View->>Validator: construct ip_network(stripped_client_ip, strict=False)
Validator->>Settings: check if client IP in any allowed network
alt IP allowed
API_View-->>Client: 200 OK (allowed)
else IP not allowed
API_View-->>Client: 403 Forbidden (detail: IP not allowed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/base/models.py`:
- Around line 1388-1390: The loop over allowed_hosts_set redundantly calls
str(...).strip() before validating with ipaddress.ip_network; since
allowed_hosts_set is built from freeradius_allowed_hosts_list which already
strips and yields strings, replace ipaddress.ip_network(str(ip_address).strip(),
strict=False) with ipaddress.ip_network(ip_address, strict=False) in the
validation loop (keep strict=False).
In `@openwisp_radius/tests/test_api/test_freeradius_api.py`:
- Around line 2388-2447: Add parallel CIDR-related tests to the TestClientIpApi
class mirroring the new tests in TestTransactionClientIpApi: create tests named
like test_ip_from_radsetting_cidr_range_valid,
test_ip_from_radsetting_spaces_and_host_bits_valid, and
test_ip_outside_cidr_range_rejected that manipulate
OrganizationRadiusSettings.freeradius_allowed_hosts, patch
openwisp_radius.api.freeradius_views.get_client_ip, and assert the same
responses as in the Transaction tests; also add edge-case tests in
TestClientIpApi for IPv6 CIDR ranges (e.g., ::1/128 and 2001:db8::/32) and a
regression test for a single IP without CIDR to ensure existing behavior from
test_ip_from_radsetting_invalid is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ea97a6c-6e92-4dc2-8af3-995aba4b7baa
📒 Files selected for processing (3)
openwisp_radius/api/freeradius_views.pyopenwisp_radius/base/models.pyopenwisp_radius/tests/test_api/test_freeradius_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (5)
openwisp_radius/base/models.py (1)
1342-1346: LGTM!The list comprehension correctly strips whitespace from each entry and filters out empty strings, ensuring robust parsing of comma-separated IP addresses with accidental spaces.
openwisp_radius/api/freeradius_views.py (1)
106-118: LGTM! Correct implementation of CIDR range matching.The logic correctly:
- Uses
ip_network(str(ip).strip(), strict=False)to parse allowed hosts as networks, supporting both single IPs and CIDR ranges.- Checks if the client IP falls within any allowed network using the
inoperator withip_address()andip_network().- Preserves the existing error handling for invalid IP configurations.
The
str(ip).strip()is slightly redundant (values should already be stripped strings), but serves as defensive coding and doesn't impact functionality.openwisp_radius/tests/test_api/test_freeradius_api.py (3)
2388-2403: LGTM! Good test coverage for CIDR range matching.This test correctly validates that an IP address within a CIDR range (172.18.0.10 in 172.18.0.0/16) is accepted for authorization.
2405-2425: LGTM! Comprehensive test for whitespace and host bits handling.This test effectively validates:
- Whitespace trimming in allowed hosts parsing (space after comma).
- Host bits in CIDR notation (172.18.0.5/16 instead of canonical 172.18.0.0/16) working with
strict=False.- Cache correctness after save.
- Client IP within the range is accepted.
2427-2447: LGTM! Good negative test case for CIDR rejection.This test correctly validates that an IP address outside the allowed CIDR range (10.0.0.5 not in 172.18.0.0/16) is rejected with the appropriate 403 status and error message.
…isp#229 Closes openwisp#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.
UltraBot05
force-pushed
the
fix-allowed-hosts-cidr
branch
from
e2ee205 to
f31bb3a
Compare
UltraBot05
changed the title
|
Hi, some information on this: On CodeRabbitAI's input: CIs have passed, please check. |
2 participants
Added
strict=Falsetoipaddress.ip_networkparsing andstrip() to string inputs to allow networks with host bits and spaces to be correctly parsed and authenticated.Checklist
Reference to Existing Issue
Closes #229.
Description of Changes
This PR enables CIDR and subnet support for
FREERADIUS_ALLOWED_HOSTSby addingstrict=Falsetoipaddress.ip_network()parsing in base/models.py and api/freeradius_views.py.This change prevents
ValueErrorcrashes when host bits are set and allows authentication for any client IP within a defined network range. Additionally,.strip()was added to handle accidental whitespace in input strings. New test cases intest_freeradius_api.pyverify correct range-based authentication and resilient string parsing.