[models] Sanitize MAC addresses in called_station_id field#632
[models] Sanitize MAC addresses in called_station_id field#632Eeshu-Yadav wants to merge 2 commits intoopenwisp:masterfrom
Conversation
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
2 times, most recently
from
d02b555 to
df45e2a
Compare
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
3 times, most recently
from
b60a905 to
ba92950
Compare
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
5 times, most recently
from
b241cf3 to
ab72c26
Compare
|
@nemesifier kindly review this |
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
from
ab72c26 to
fd62691
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces MAC address sanitization and normalization across the codebase. Adds a module-level sanitize_mac_address(mac) and applies it when saving RadiusAccounting.called_station_id, when filtering/excluding open sessions in FreeRADIUS API views, and during the convert_called_station_id management command’s routing/session updates. Tests and fixtures updated to expect lowercase colon-separated MACs. Also includes minor migration literal fixes and new empty merge migrations; no public API signatures were changed. Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
from
fd62691 to
d106161
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav when you do UI changes, please always add a screenshot and/or a short screencast demonstrating the result really fixes the issue.
| if sys.argv[-1] == "publish": | ||
| # delete any *.pyc, *.pyo and __pycache__ | ||
| os.system('find . | grep -E "(__pycache__|\.pyc|\.pyo$)" | xargs rm -rf') | ||
| os.system(r'find . | grep -E "(__pycache__|\.pyc|\.pyo$)" | xargs rm -rf') |
There was a problem hiding this comment.
why this change? Seems unrelated AI slop
There was a problem hiding this comment.
will remove that
There was a problem hiding this comment.
actually during qa check this is showing , not major thing to raise a separate so just updated here ,
suggest me that should i keep this or remove
will add the ui ss also |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@openwisp_radius/base/models.py`:
- Around line 183-223: The sanitize_mac_address function currently only
preserves IPv4 by regex and may rewrite IPv6 or other IP literals; update
sanitize_mac_address to first detect and preserve any IP address by using the
ipaddress module (e.g., attempt ipaddress.ip_address(mac) and return mac if it
parses), ensure the ipaddress import is added, and keep the rest of the
mac_patterns/EUI-based extraction logic unchanged so only true MAC candidates
are normalized.
In `@openwisp_radius/management/commands/base/convert_called_station_id.py`:
- Around line 151-176: The fallback lookup logic in
BaseConvertCalledStationIdCommand incorrectly always logs a warning and
continues, even when alt_key is found; change the if/else so that when alt_key
is in routing_dict you assign routing_dict[lookup_key] = routing_dict[alt_key]
and proceed normally, but only log the warning and execute continue inside the
else branch (i.e., when no variant was found). Ensure the local helper
_strip_leading_zeros, the variables lookup_key, alt_key, routing_dict and
radius_session.session_id are used exactly as shown to locate and fix the
condition flow.
- Around line 151-153: Wrap the EUI conversion that builds lookup_key from
radius_session.calling_station_id in a try-except that catches AddrFormatError,
ValueError, and TypeError, mirroring the pattern used in
openwisp_radius/base/models.py; on exception emit a warning (including the
offending calling_station_id and session id/context) and skip processing that
session instead of aborting, leaving the rest of convert_called_station_id's
loop unchanged so only invalid entries are ignored.
In `@openwisp_radius/tests/test_api/test_freeradius_api.py`:
- Around line 2364-2375: The patch context for freeradius_hosts_path in
test_ip_from_radsetting_valid is too narrow (it currently ends before
radsetting.save() and the client.post), so expand the mock.patch block to
encompass the modification and usage of radsetting: obtain the
OrganizationRadiusSettings (radsetting), set radsetting.freeradius_allowed_hosts
= "127.0.0.1", call radsetting.save(), and then perform the
client.post(reverse("radius:authorize"), self.params) all inside the same
mock.patch context for self.freeradius_hosts_path so the test actually exercises
the non-empty hosts scenario; update references to
test_ip_from_radsetting_valid, freeradius_hosts_path,
radsetting.freeradius_allowed_hosts, save(), and the client.post call
accordingly.
In `@openwisp_radius/tests/test_commands.py`:
- Around line 438-440: Remove the debug print statements that were added to the
logger mocks (e.g., the lambda assigned to mocked_logger.side_effect that calls
print) and any other stray print(...) calls in the tests; instead rely on the
mock's call assertions or have the mock record messages (e.g., leave
mocked_logger as a MagicMock and assert mocked_logger.assert_called_with(...) or
inspect mocked_logger.call_args_list). Specifically, delete the lambda print
assignments and direct print(...) calls referenced (mocked_logger.side_effect =
lambda msg: print(...)) and update tests to assert on mocked_logger calls rather
than printing messages.
- Around line 414-448: Two subtests share the identical name "Test client common
name does not contain a MAC address", which makes test output ambiguous; rename
one of the subTests to a distinct, descriptive name (e.g., "Test client common
name does not contain a MAC address - search raises IndexError" or similar) or
remove the redundant test if it covers the same scenario, and update the
corresponding expected_msg/assertion (the
mocked_logger.assert_called_once_with(...) and any references to
dummy_routing_obj.common_name or the call to
call_command("convert_called_station_id")) to reflect the new name so the
assertion still matches the log output.
- Around line 449-477: The duplicate subTest title "Test routing information
does not contain all addresses" causes ambiguity; update the two subTest names
to be unique and descriptive (e.g., include the scenario or expected outcome)
where they are declared in openwisp_radius/tests/test_commands.py around the
tests that patch BaseConvertCalledStationIdCommand._get_openvpn_routing_info and
assert logger.warning calls; rename one subTest to something like "Test routing
information missing expected MAC" and the other to "Test routing information
missing all addresses" (or similar) so each subTest in the
convert_called_station_id tests is distinguishable in test output.
🧹 Nitpick comments (2)
openwisp_radius/tests/test_commands.py (2)
151-154: Misleading comment about unused variable.The comment on line 151 stating "Unused variable removed" is confusing since
radiusbatchis actually needed and immediately reassigned on line 153 to accessexpiration_date. Consider removing this comment or clarifying it.Suggested fix
- # radiusbatch = RadiusBatch.objects.first() # Unused variable removed self.assertEqual(get_user_model().objects.all().count(), 3) radiusbatch = RadiusBatch.objects.first()
453-455: Consider usingself.assertNotEqualinstead of bareassert.Using bare
assertstatements in unittest-based tests can be problematic because they may be stripped when running Python with optimization flags (-O). While these are for test setup validation,self.assertNotEqualprovides better error messages and consistency.Suggested fix
- assert ( - wrong_key != routing_key - ), f"Test setup error: wrong_key matches routing_key ({wrong_key})" + self.assertNotEqual( + wrong_key, routing_key, + f"Test setup error: wrong_key matches routing_key ({wrong_key})" + )Also applies to: 627-629
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
openwisp_radius/api/freeradius_views.pyopenwisp_radius/base/models.pyopenwisp_radius/management/commands/base/convert_called_station_id.pyopenwisp_radius/migrations/0002_initial_openwisp_radius.pyopenwisp_radius/migrations/0041_alter_organizationradiussettings_token.pyopenwisp_radius/migrations/0043_merge_20251222_0406.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_freeradius_api.pyopenwisp_radius/tests/test_commands.pyopenwisp_radius/tests/test_models.pyopenwisp_radius/tests/test_tasks.pysetup.pytests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.pytests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.pytests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py
🧰 Additional context used
🧬 Code graph analysis (7)
openwisp_radius/tests/test_api/test_api.py (2)
openwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.js (1)
called_station_id(53-54)openwisp_radius/api/freeradius_views.py (1)
get(498-503)
openwisp_radius/migrations/0043_merge_20251222_0406.py (3)
openwisp_radius/migrations/0041_alter_organizationradiussettings_token.py (1)
Migration(12-35)tests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.py (1)
Migration(12-35)tests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py (1)
Migration(6-13)
openwisp_radius/tests/test_models.py (2)
openwisp_radius/tests/__init__.py (1)
_create_radius_accounting(81-86)openwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.js (1)
called_station_id(53-54)
openwisp_radius/api/freeradius_views.py (1)
openwisp_radius/base/models.py (1)
sanitize_mac_address(183-223)
openwisp_radius/management/commands/base/convert_called_station_id.py (3)
openwisp_radius/utils.py (1)
load_model(33-34)openwisp_radius/base/models.py (1)
sanitize_mac_address(183-223)tests/openwisp2/sample_users/models.py (1)
Organization(32-34)
openwisp_radius/migrations/0041_alter_organizationradiussettings_token.py (2)
openwisp_radius/migrations/0002_initial_openwisp_radius.py (1)
Migration(21-554)tests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.py (1)
Migration(12-35)
openwisp_radius/tests/test_api/test_freeradius_api.py (3)
openwisp_radius/base/models.py (1)
sanitize_mac_address(183-223)openwisp_radius/utils.py (1)
load_model(33-34)tests/openwisp2/sample_radius/models.py (1)
OrganizationRadiusSettings(83-85)
🪛 Ruff (0.14.14)
openwisp_radius/migrations/0043_merge_20251222_0406.py
[warning] 8-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 13-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
setup.py
[error] 11-11: Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without shell
(S605)
[error] 11-11: Starting a process with a partial executable path
(S607)
tests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.py
[warning] 14-16: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 18-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py
[warning] 8-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 13-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_radius/management/commands/base/convert_called_station_id.py
[warning] 28-28: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 64-64: Do not catch blind exception: Exception
(BLE001)
[warning] 71-71: Consider moving this statement to an else block
(TRY300)
[warning] 136-136: Do not catch blind exception: Exception
(BLE001)
openwisp_radius/migrations/0041_alter_organizationradiussettings_token.py
[warning] 14-16: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 18-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (29)
setup.py (1)
11-11: LGTM - raw string is cleaner for regex patterns.The change to a raw string literal is a minor improvement for readability. The static analysis warnings (S605, S607) about shell command usage are pre-existing concerns with this publish script and not introduced by this change.
openwisp_radius/migrations/0002_initial_openwisp_radius.py (1)
251-251: LGTM - raw string for regex pattern is more idiomatic.The conversion to a raw string literal produces an equivalent regex pattern while improving readability.
tests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.py (1)
508-508: LGTM - consistent with main migration file.Same raw string improvement applied consistently to the sample app migration.
openwisp_radius/tests/test_tasks.py (2)
204-204: LGTM - raw string is appropriate for regex patterns.The raw string literal is the idiomatic choice for regex patterns. The
</a>doesn't require escaping in regex, so removing the backslash is correct.
244-244: LGTM - consistent with the previous regex change.openwisp_radius/api/freeradius_views.py (2)
31-31: LGTM - import correctly added.The import is appropriately placed and the function will be used to normalize MAC addresses for consistent comparisons.
374-380: LGTM - correct sanitization for consistent MAC address comparison.Sanitizing both
called_station_idandcalling_station_idbefore the exclusion filter ensures consistent comparison against the normalized values stored in the database. The implementation is correct since thesanitize_mac_addressfunction (fromopenwisp_radius/base/models.py) gracefully handles non-MAC values by returning them unchanged, and handlesNoneor empty inputs by returning them as-is.openwisp_radius/tests/test_models.py (3)
93-101: LGTM - test data updated to reflect MAC sanitization.The test data and assertions correctly use the lowercase colon-separated format that
sanitize_mac_addressproduces.
112-120: LGTM - consistent MAC format expectations.
129-140: LGTM - test assertions aligned with normalized MAC format.openwisp_radius/tests/test_api/test_api.py (5)
815-815: LGTM - raw string for regex pattern.
975-975: LGTM - assertion updated to expect sanitized MAC format.The expected value reflects the lowercase colon-separated format produced by
sanitize_mac_addressduring model save.
1270-1284: LGTM - test data uses normalized MAC format.The
called_station_idvalues correctly use the lowercase colon-separated format.
1333-1343: LGTM - filter queries and assertions use consistent MAC format.The tests correctly verify filtering with the normalized MAC address format stored in the database.
1345-1368: Note:calling_station_idremains unsanitized - this is intentional.The tests for
calling_station_idfiltering continue to use mixed formats (both11-22-33-44-55-66and11:22:33:44:55:66), and the assertions show the values are stored as-is. This correctly reflects that onlycalled_station_idis sanitized per the PR scope, whilecalling_station_idpreserves its original format.openwisp_radius/migrations/0043_merge_20251222_0406.py (1)
1-13: LGTM merge migration.
Straightforward dependency-only merge.tests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py (1)
1-13: LGTM merge migration.
No issues detected.openwisp_radius/base/models.py (2)
575-578: Good: sanitize called_station_id on save.
Keeps persistence normalized at the model layer.
621-630: Good: sanitize called_station_id for stale-session closure.
Ensures lookups align with stored normalized values.openwisp_radius/migrations/0041_alter_organizationradiussettings_token.py (1)
1-35: LGTM migration.
No concerns.tests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.py (1)
1-35: LGTM migration.
Looks consistent with main app migration.openwisp_radius/tests/test_api/test_freeradius_api.py (4)
6-6: LGTM: explicit swapper aliasing and sanitize import.
Keeps model loading explicit and avoids namespace ambiguity.Also applies to: 22-22, 38-39
405-411: Good: sanitize both sides in test comparisons.
Prevents false negatives when inputs come in mixed formats.
664-665: Updated expectations for normalized MAC values look consistent.
Test assertions now align with canonical lowercase colon format.Also applies to: 681-682, 1004-1005, 1041-1048, 2017-2029, 2108-2110
1463-1465: Nice: stronger assertions on Auth‑Type responses.
Improves test clarity without over‑constraining payloads.Also applies to: 1473-1475, 1700-1702, 1718-1721, 1770-1772, 1884-1886
openwisp_radius/tests/test_commands.py (4)
5-5: LGTM!The addition of
swapperimport andOrganizationmodel loading is appropriate for enabling organization-aware test setup. This aligns with the broader changes to support MAC address sanitization at the model level.Also applies to: 21-21
29-33: LGTM!The
_get_defaultsmethod correctly handles the case when anOrganizationmodel instance is already provided inopts, avoiding redundant processing by the parent class. Thehasattrcheck for_metais a reliable way to detect Django model instances.
395-396: LGTM - MAC address format expectations updated correctly.The test expectations are correctly updated to expect lowercase colon-separated MAC addresses (
aa:aa:aa:aa:aa:0ainstead ofAA-AA-AA-AA-AA-0A), aligning with the sanitization feature being implemented.Also applies to: 654-654, 687-687, 729-729, 739-739, 768-768
378-379: Good cleanup of undefined reference.The removal of
mocked_logger.call_count, 2is correct - this was an undefined reference that would have caused issues.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
from
d106161 to
1a46516
Compare
There was a problem hiding this comment.
Pull request overview
This pull request implements MAC address sanitization for the called_station_id field in RadiusAccounting to ensure consistent formatting across different input formats. MAC addresses are automatically converted to lowercase colon-separated format (aa:bb:cc:dd:ee:ff), addressing issue #624 where different MAC address formats (colon, dash, dot, no separators) were being treated as distinct values, impacting statistics in the monitoring integration.
Changes:
- Added
sanitize_mac_address()function in base/models.py using netaddr.EUI for MAC address normalization - Integrated automatic MAC sanitization in
AbstractRadiusAccounting.save()method - Updated management command
convert_called_station_idto use sanitized MAC addresses - Updated test expectations throughout the codebase to reflect lowercase colon-separated format
- Fixed regex string formats to use raw strings in some test files
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| openwisp_radius/base/models.py | Adds sanitize_mac_address function and integrates it into AbstractRadiusAccounting.save() |
| openwisp_radius/api/freeradius_views.py | Updates _check_simultaneous_use to sanitize MAC addresses before comparison |
| openwisp_radius/management/commands/base/convert_called_station_id.py | Refactors command to use sanitized MAC addresses and normalize routing dictionary keys |
| openwisp_radius/tests/test_models.py | Updates test expectations to use lowercase colon-separated format |
| openwisp_radius/tests/test_commands.py | Extensive updates to convert_called_station_id command tests with sanitized MAC format |
| openwisp_radius/tests/test_api/test_freeradius_api.py | Updates API test expectations for sanitized MAC addresses |
| openwisp_radius/tests/test_api/test_api.py | Updates accounting API test expectations |
| openwisp_radius/tests/test_tasks.py | Fixes regex strings to use raw string format |
| openwisp_radius/migrations/0043_merge_20251222_0406.py | Merge migration for parallel development branches |
| openwisp_radius/migrations/0041_alter_organizationradiussettings_token.py | Token field validator update migration |
| openwisp_radius/migrations/0002_initial_openwisp_radius.py | Updates regex to use raw string format |
| tests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py | Merge migration for test app |
| tests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.py | Token field validator update for test app |
| tests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.py | Updates regex to use raw string format in test app |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for pattern in mac_patterns: | ||
| match = re.search(pattern, mac) | ||
| if match: | ||
| mac_candidate = match.group(0) | ||
| try: | ||
| eui = EUI(mac_candidate) | ||
| return ":".join(["%02x" % x for x in eui.words]).lower() | ||
| except (AddrFormatError, ValueError, TypeError): | ||
| continue | ||
| return mac |
There was a problem hiding this comment.
The sanitize_mac_address function uses re.search() to extract MAC addresses from strings, which will strip any additional text (e.g., "00-27-22-F3-FA-F1:hostname" becomes "00:27:22:f3:fa:f1"). While this appears to be intentional based on test expectations, this is a breaking change that could cause data loss for deployments that store additional information in the called_station_id field alongside the MAC address. Consider documenting this behavior in the PR description and ensuring it aligns with the RFC compliance mentioned in the PR description, which states "preserving non-MAC values unchanged."
| def sanitize_mac_address(mac): | ||
| """ | ||
| Sanitize a MAC address string to the colon-separated lowercase format. | ||
| If the input is not a valid MAC address, return it unchanged. | ||
|
|
||
| Handles various MAC address formats: | ||
| - 00:1A:2B:3C:4D:5E -> 00:1a:2b:3c:4d:5e | ||
| - 00-1A-2B-3C-4D-5E -> 00:1a:2b:3c:4d:5e | ||
| - 001A.2B3C.4D5E -> 00:1a:2b:3c:4d:5e | ||
| - 001A2B3C4D5E -> 00:1a:2b:3c:4d:5e | ||
| """ | ||
| if not mac or not isinstance(mac, str): | ||
| return mac | ||
| try: | ||
| ipaddress.ip_address(mac) | ||
| return mac | ||
| except ValueError: | ||
| pass | ||
| mac_patterns = [ | ||
| r"([0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}", | ||
| r"([0-9A-Fa-f]{4}\.){2}[0-9A-Fa-f]{4}", | ||
| r"[0-9A-Fa-f]{12}", | ||
| ] | ||
| for pattern in mac_patterns: | ||
| match = re.search(pattern, mac) | ||
| if match: | ||
| mac_candidate = match.group(0) | ||
| try: | ||
| eui = EUI(mac_candidate) | ||
| return ":".join(["%02x" % x for x in eui.words]).lower() | ||
| except (AddrFormatError, ValueError, TypeError): | ||
| continue | ||
| return mac | ||
|
|
There was a problem hiding this comment.
The sanitize_mac_address function lacks dedicated test coverage. While the function is used in tests, there are no explicit tests verifying it handles all documented MAC address formats (colon-separated, dash-separated, dot-separated, and no separators) correctly, or that it properly returns non-MAC values unchanged. Consider adding a dedicated test method to verify these format conversions work as documented in the function's docstring.
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
2 times, most recently
from
083dcdc to
022a1b7
Compare
This change implements MAC address sanitization in the called_station_id field of RadiusAccounting to ensure consistent formatting across different input formats. MAC addresses are now automatically converted to lowercase colon-separated format (aa:bb:cc:dd:ee:ff) while preserving non-MAC values unchanged for RFC compliance. Changes: - Added sanitize_mac_address function in base/models.py using netaddr.EUI - Integrated automatic MAC sanitization in AbstractRadiusAccounting.save() - Updated test imports to use sanitize_mac_address from base.models - Supports multiple MAC formats: colon, dash, dot, and no separators Fixes openwisp#624
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
from
022a1b7 to
9986fe9
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/api/freeradius_views.py`:
- Around line 375-377: The code sanitizes calling_station_id when querying
open_sessions but only canonicalizes called_station_id on write, causing
mismatches; fix by updating AbstractRadiusAccounting.save() to canonicalize
calling_station_id as well (use sanitize_mac_address on the calling_station_id
value before storing) so stored rows and the open_sessions.exclude(...) call
(which uses sanitize_mac_address for calling_station_id) match; update any
related tests or validations to assert calling_station_id is normalized on save.
In `@openwisp_radius/base/models.py`:
- Around line 567-569: The save() change only normalizes future writes but
existing radacct.called_station_id rows must be canonicalized first; add a data
backfill migration that updates all existing RadAcct records to
sanitize_mac_address(called_station_id) (and similarly handle the other block
referenced at the 619-623 range), so _close_stale_sessions_on_nas_boot() will
match stale sessions; reference the RadAcct model's save method, the
called_station_id field, sanitize_mac_address, and the
_close_stale_sessions_on_nas_boot function when implementing the
migration/update query to normalize existing rows before deploying the equality
change.
- Around line 201-214: The code currently uses re.search on mac_patterns which
can match MAC-like substrings inside larger identifiers; change to full-string
matching (e.g., re.fullmatch or anchor patterns '^...$') when testing mac
against each pattern, and only attempt normalization with EUI(mac_candidate)
when the entire input matches; if no full match is found, return the original
mac unchanged. Update references in this block (mac_patterns, mac,
mac_candidate, EUI, AddrFormatError) accordingly so truncated substrings are not
normalized.
In `@openwisp_radius/management/commands/base/convert_called_station_id.py`:
- Around line 26-30: The _search_mac_address function is too narrow; replace the
current use of RE_VIRTUAL_ADDR_MAC.search with the project’s shared MAC
extraction/sanitizer (the same regex or helper used elsewhere to accept colon,
dash, dot, no-separator and prefixed forms) so it matches all supported MAC
formats in common_name, use a global/any-position match (not just
start-of-string), return the normalized match as before, and raise IndexError
only if that shared extractor finds nothing.
In `@openwisp_radius/migrations/0002_initial_openwisp_radius.py`:
- Line 251: The regex in the migration's re.compile call is double-escaped
(re.compile(r"^[^\\s/\.]+$")), changing semantics; update the pattern to use a
single backslash for the \s class so it reads the intended whitespace character
class (e.g., change the pattern in the re.compile(...) expression to use
r"^[^\s/\.]+$") to correctly forbid spaces and the characters "/" and "." while
allowing other characters; locate the re.compile invocation in the migration
(the exact call shown) and replace the string literal accordingly.
In `@openwisp_radius/tests/test_api/test_api.py`:
- Around line 1816-1826: The two consecutive subtests call self.client.get(path,
{"called_station_id": "aa:bb:cc:dd:ee:ff"}) twice, duplicating the same
canonical MAC check and not exercising normalization; replace the second call
with an alternate-format MAC (for example a dashed or uppercase MAC like
"AA-BB-CC-DD-EE-FF" or "AA:BB:CC:DD:EE:FF" in uppercase) so the test asserts
that normalization occurs (keep the same assertions on status_code, length and
that both returned entries' called_station_id equal the canonical lowercase
"aa:bb:cc:dd:ee:ff"), or remove the duplicate block if you prefer.
In `@openwisp_radius/tests/test_api/test_freeradius_api.py`:
- Around line 1700-1701: Tests currently weaken assertions by only checking
response.data["control:Auth-Type"] instead of the full expected structure;
update the test methods that touch response.data (the assertions using
self.assertIn("control:Auth-Type", response.data) and
self.assertEqual(response.data["control:Auth-Type"], "Accept")) to either (a)
restore the original equality check against the full _AUTH_TYPE_ACCEPT_RESPONSE
constant (use self.assertEqual(response.data, _AUTH_TYPE_ACCEPT_RESPONSE) or a
deep-equality/assertDictEqual) or (b) if non-deterministic fields are expected,
add a clarifying comment and explicitly assert the presence of all expected keys
(e.g., assertIn for "control:Auth-Type", "Auth-Type", "control:Response-Auth",
any counter fields) and their deterministic values rather than only the single
field; apply the same change to the other similar tests (the ones around the
repeated assertions) so they consistently validate the full expected shape or
document why only key-level checks are used.
- Around line 1463-1464: The test weakened the assertion by only checking
response.data["control:Auth-Type"] instead of comparing the whole response to
the expected _AUTH_TYPE_ACCEPT_RESPONSE; restore stronger checks by replacing
the partial assertions with a full equality assertion
(assertEqual(response.data, _AUTH_TYPE_ACCEPT_RESPONSE)) or, if variability is
expected, add explicit asserts for the other required keys/values (e.g.,
assertIn "CoovaChilli-Max-Total-Octets" and "Session-Timeout" in response.data
and assertEqual their values to the expected ones) so the test verifies the full
response structure used by the code handling authentication.
In `@openwisp_radius/tests/test_commands.py`:
- Around line 395-396: _unconverted_ids passed into _get_unconverted_sessions()
must be normalized to the same lowercase colon-separated MAC format that
RadiusAccounting.save() stores; modify _get_unconverted_sessions() to
map/normalize every value in its unconverted_ids input (convert to lowercase and
replace separators to colon-separated octets, e.g., "AA-AA-..." -> "aa:aa:...")
before using them in the queryset filter called_station_id__in, and update any
test expectation (the value around line 738) to use the normalized form or rely
on the same normalization helper to ensure matches.
In `@tests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.py`:
- Line 508: The regex used in the re.compile call is double-escaping the
backslash (r"^[^\\s/\.]+$") so `\\s` is treated literally instead of matching
whitespace; update the pattern passed to re.compile to use a single backslash in
the raw string (r"^[^\s/\.]+$") so \s correctly matches whitespace and enforces
"no spaces, dots or slashes" validation where this regex is used in the
migration.
🪄 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: 7bb659cd-9e65-4103-80c3-22d73568603e
📒 Files selected for processing (15)
openwisp_radius/api/freeradius_views.pyopenwisp_radius/base/models.pyopenwisp_radius/management/commands/base/convert_called_station_id.pyopenwisp_radius/migrations/0002_initial_openwisp_radius.pyopenwisp_radius/migrations/0041_alter_organizationradiussettings_token.pyopenwisp_radius/migrations/0043_merge_20251222_0406.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_freeradius_api.pyopenwisp_radius/tests/test_commands.pyopenwisp_radius/tests/test_models.pyopenwisp_radius/tests/test_tasks.pysetup.pytests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.pytests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.pytests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Code formatting must be compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Applied to files:
openwisp_radius/base/models.py
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Flag unused or redundant code
Applied to files:
openwisp_radius/base/models.py
🔇 Additional comments (22)
openwisp_radius/tests/test_tasks.py (1)
204-205: Good cleanup: raw regex literals are clearer and equivalent.Switching these patterns to raw strings removes unnecessary escaping without changing matching behavior.
Also applies to: 243-245
setup.py (1)
11-11: Good change: raw string avoids escape-sequence warnings without changing behavior.This keeps the publish cleanup command semantics intact while preventing string-literal escape warnings.
tests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.py (1)
19-34: Migration definition looks correct.
AlterFieldconfiguration and dependency wiring are valid.openwisp_radius/migrations/0041_alter_organizationradiussettings_token.py (1)
19-34: Migration looks valid and internally consistent.No correctness issues in dependency or
AlterFieldpayload.tests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py (1)
8-13: Merge migration is correctly defined.Dependencies and empty
operationsare appropriate for this branch merge point.openwisp_radius/migrations/0043_merge_20251222_0406.py (1)
8-13: Merge migration wiring looks correct.This properly reconciles the parallel migration branches with no extra operations.
openwisp_radius/tests/test_api/test_freeradius_api.py (8)
6-6: LGTM: Import aliasing for swapper.The aliasing of
swapperto_swapperand using it forOrganizationandOrganizationUsermodel loading is a clean approach to distinguish these imports from the project's internal model loading pattern.Also applies to: 38-39
22-22: LGTM: Import of sanitize_mac_address.This import supports the new MAC address normalization behavior being tested.
405-415: LGTM: MAC address sanitization in assertAcctData.The special handling for
called_station_idcorrectly sanitizes both the model value and the test data value before comparison, ensuring the assertion passes regardless of the input MAC format. The null-safety checks are appropriate.
664-664: LGTM: Test expectations updated for MAC normalization.Correctly updated to expect the normalized lowercase colon-separated MAC format (
00:00:11:11:22:22,00:00:22:22:33:33) after the model'ssave()sanitization.Also applies to: 681-681
1004-1004: LGTM: Filter and assertion updated for normalized MAC format.The query parameter and expected value are both using the normalized format, which aligns with the sanitization behavior in the model and views.
Also applies to: 1041-1047
2017-2020: LGTM: Roaming test assertions updated for MAC normalization.The assertions correctly use sanitized MAC format for
called_station_idcomparisons. Usingsanitize_mac_address()for the comparison at line 2026-2028 ensures the test handles any format variations in the test data.Also applies to: 2026-2031
2108-2112: Good addition: nas_ip_address filter in roaming test assertion.Adding
nas_ip_address=payload["nas_ip_address"]makes the filter more specific, reducing the chance of false positives when multiple sessions exist.
2364-2401: LGTM: New IP allowlist test coverage.The new test methods provide comprehensive coverage for IP allowlist validation:
test_ip_from_radsetting_valid: Tests org-specific allowlist with global list emptytest_ip_from_setting_valid: Tests default global allowlisttest_ip_from_radsetting_not_exist: Tests fallback behavior and 403 when no allowlist matchesThe patch scope is correct - the
mock.patchcontext encompasses both the setup and the request.openwisp_radius/tests/test_commands.py (8)
29-33: LGTM: Override handles pre-resolved organization objects.The
_get_defaultsoverride correctly returns early whenopts["organization"]is already an ORM model instance (detected viahasattr(..., "_meta")). Since the parent's_get_defaultsonly addsorganizationas a default (per context snippet 1), this early return safely preserves other fields passed inopts.
36-119: LGTM: Test updated to use explicit organization.The
test_cleanup_stale_radacct_commandcorrectly creates a dedicated organization and passes it to each test session. Usingsub_optionsfor the "does not affect closed session" subtest avoids mutating the sharedoptionsdict.
151-154: LGTM: Cleaned up unused variable and repositioned assignment.The commented line explains the removal, and the
radiusbatchassignment is now positioned where it's actually needed.
378-379: LGTM: Removed undefined reference.The stray
mocked_logger.call_count, 2line was correctly removed asmocked_loggerwas not defined in this scope.
414-444: LGTM: Subtests renamed to be distinct.The subtests "Test common name without MAC - with org_id" (line 414) and "Test common name without MAC - with unique_id" (line 571) now have distinct names, addressing the past review concern about duplicate subtest names.
Also applies to: 571-606
446-471: LGTM: Subtests renamed to be distinct.The subtests "Test missing routing info - with org_id" (line 446) and "Test missing routing info - with unique_id" (line 608) now have distinct names.
Also applies to: 608-636
638-642: LGTM: Test expectations updated for MAC normalization.All assertions correctly expect the normalized lowercase colon-separated MAC format (
"aa:aa:aa:aa:aa:0a","cc:cc:cc:cc:cc:0c") after the command processes the sessions.Also applies to: 717-717, 727-727, 756-756
669-718: LGTM: "Test update only session with unique_id" properly verifies selective updates.The test creates two sessions but only updates one (via
unique_id), and the mock routing returns a MAC different from the originalcalled_station_id, allowing verification that the conversion actually changes the value.
Commit Message Format FailureHello @Eeshu-Yadav, The CI failed because the commit message does not follow the required format. Fix: Here's an example of the correct format: |
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
6 times, most recently
from
d971ef7 to
8e119ba
Compare
Multiple Test Failures in OpenWISP RadiusHello @Eeshu-Yadav, There are multiple test failures in your CI run. Here's a breakdown:
|
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
from
8e119ba to
32837f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
setup.py (1)
11-11:⚠️ Potential issue | 🟡 MinorRemove this unrelated change to keep the PR focused.
This raw string literal change is cosmetic and unrelated to MAC address sanitization (the PR's objective). While harmless, it adds noise and scope creep. As discussed in previous comments, this should be removed to maintain a clean, focused PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.py` at line 11, Remove the unrelated cosmetic change by deleting the added os.system(r'find . | grep -E "(__pycache__|\.pyc|\.pyo$)" | xargs rm -rf') call from setup.py so the PR stays focused on MAC address sanitization; locate the os.system invocation and revert that line (or restore the original setup.py content) so no file-cleanup shell command is included in this change.openwisp_radius/base/models.py (1)
201-213:⚠️ Potential issue | 🟠 MajorMatch the whole value before normalizing.
Line 207 uses
re.search(), so identifiers like00-03-7F-12-34-56:AP1are truncated to00:03:7f:12:34:56instead of being preserved. That breaks the RFC-compliance promise to leave non-MAC values unchanged.🛠️ Suggested fix
for pattern in mac_patterns: - match = re.search(pattern, mac) - if match: - try: - eui = EUI(match.group(0)) + if re.fullmatch(pattern, mac): + try: + eui = EUI(mac) return ":".join([f"{x:02x}" for x in eui.words]).lower() except (AddrFormatError, ValueError, TypeError): continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/base/models.py` around lines 201 - 213, The current logic uses re.search on mac_patterns, so inputs like "00-03-7F-12-34-56:AP1" get partially matched and normalized; change the matching to require the entire value to match (use re.fullmatch or anchor the patterns) when iterating mac_patterns, e.g., replace re.search(pattern, mac) with a full-string match, retain the existing EUI(match.group(0)) normalization and exception handling (AddrFormatError, ValueError, TypeError), and ensure that if no full match is found the original mac value is returned unchanged.
🤖 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/tests/test_api/test_api.py`:
- Line 1753: The test currently passes already-normalized called_station_id
values at the two spots (the called_station_id arguments inserted at the
locations around the accounting endpoint test), which removes coverage for MAC
normalization; change one of those inputs (either the one at the earlier
occurrence or the later one) to a non-canonical format such as
"AA-BB-CC-DD-EE-FF" or "AA:BB:CC:DD:EE:FF" (uppercase/dashed) so the accounting
write path exercises sanitization in the code that handles called_station_id,
and keep the existing assertions expecting the normalized stored value (i.e.,
assert that the persisted value equals the canonical lowercase colon-separated
form).
In `@openwisp_radius/tests/test_api/test_freeradius_api.py`:
- Line 6: Replace the aliased import "import swapper as _swapper" with a direct
import and update usages: either use "import swapper" and change all
"_swapper.load_model(...)" calls to "swapper.load_model(...)" or import the
function directly with "from swapper import load_model" and replace
"_swapper.load_model(...)" with "load_model(...)"; update the two occurrences
(previously using _swapper.load_model) accordingly to remove the unnecessary
alias.
In `@openwisp_radius/tests/test_tasks.py`:
- Around line 241-245: The test is asserting HTML from a stale `email` variable
instead of the newly queued message; after calling
tasks.send_login_email.delay(accounting_data) (or immediately after the action
that should queue the Italian email), pop the latest message from the mail
outbox (e.g., call mail.outbox.pop() and assign it to `email`) before performing
the assertRegex on email.alternatives[0][0], ensuring you reference the newly
queued email rather than the previous subtest's message.
---
Duplicate comments:
In `@openwisp_radius/base/models.py`:
- Around line 201-213: The current logic uses re.search on mac_patterns, so
inputs like "00-03-7F-12-34-56:AP1" get partially matched and normalized; change
the matching to require the entire value to match (use re.fullmatch or anchor
the patterns) when iterating mac_patterns, e.g., replace re.search(pattern, mac)
with a full-string match, retain the existing EUI(match.group(0)) normalization
and exception handling (AddrFormatError, ValueError, TypeError), and ensure that
if no full match is found the original mac value is returned unchanged.
In `@setup.py`:
- Line 11: Remove the unrelated cosmetic change by deleting the added
os.system(r'find . | grep -E "(__pycache__|\.pyc|\.pyo$)" | xargs rm -rf') call
from setup.py so the PR stays focused on MAC address sanitization; locate the
os.system invocation and revert that line (or restore the original setup.py
content) so no file-cleanup shell command is included in this change.
🪄 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: d5e62cdd-bf15-405d-9316-6685f8fbd96e
📒 Files selected for processing (14)
openwisp_radius/api/freeradius_views.pyopenwisp_radius/base/models.pyopenwisp_radius/management/commands/base/convert_called_station_id.pyopenwisp_radius/migrations/0002_initial_openwisp_radius.pyopenwisp_radius/migrations/0041_alter_organizationradiussettings_token.pyopenwisp_radius/migrations/0043_merge_20251222_0406.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_freeradius_api.pyopenwisp_radius/tests/test_commands.pyopenwisp_radius/tests/test_tasks.pysetup.pytests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.pytests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.pytests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.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.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Code formatting must be compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Applied to files:
openwisp_radius/base/models.py
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Flag unused or redundant code
Applied to files:
openwisp_radius/base/models.py
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_radius/tests/test_api/test_freeradius_api.py
🔇 Additional comments (24)
tests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.py (1)
506-515: LGTM! Regex escaping fix correctly applied.The pattern
r"^[^\s/\.]+$"now correctly uses a single backslash in the raw string, ensuring\smatches whitespace as intended by the validator message.tests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.py (1)
1-35: LGTM! Migration correctly alters token field with proper validator.The migration properly uses the corrected regex pattern and follows Django migration conventions. The dependency on migration
0030is correctly specified.tests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py (1)
1-13: LGTM! Merge migration correctly resolves diamond dependency.The merge migration properly consolidates the two
0031_*migrations that diverged from0030. Emptyoperationsis appropriate for a pure merge point.openwisp_radius/migrations/0041_alter_organizationradiussettings_token.py (1)
1-35: LGTM! Migration correctly captures full field state.The migration properly uses the corrected regex pattern. The explicit
default,help_text, andvalidatorsare likely captured fromKeyField's internal defaults, which is standard Django migration behavior.openwisp_radius/migrations/0002_initial_openwisp_radius.py (1)
249-258: LGTM! Regex escaping fix correctly applied.The pattern
r"^[^\s/\.]+$"now correctly uses a single backslash in the raw string, consistent with the fix applied across all related migrations.openwisp_radius/migrations/0043_merge_20251222_0406.py (1)
1-13: LGTM! Merge migration correctly resolves parallel migration branches.The merge migration properly consolidates the two migration branches:
0041_alter_organizationradiussettings_tokenand the chain0041_radiusbatch_status → 0042_set_existing_batches_completed. Emptyoperationsis appropriate.openwisp_radius/base/models.py (1)
566-568: Verify there is a backfill for legacyradacct.called_station_idrows.Lines 567-568 normalize only future saves, while Line 620 now matches the canonical form only. Existing dashed/dotted rows will be missed by
Accounting-Oncleanup unless an omitted migration rewrites them first.#!/bin/bash # Look for a data migration/backfill that normalizes existing radacct.called_station_id values. fd -i 'migrations' -td rg -n --type=py "sanitize_mac_address|called_station_id|RadiusAccounting|radacct" -g '**/migrations/*.py'Expected result: at least one migration or backfill path that updates existing
radacct.called_station_idvalues before deployment.Also applies to: 618-620
openwisp_radius/tests/test_api/test_freeradius_api.py (8)
405-413: LGTM!The sanitization logic correctly handles both the expected and actual
called_station_idvalues, including properNonechecks. This ensures test assertions work regardless of input format variations.
662-662: LGTM!Test assertions correctly expect the normalized lowercase colon-separated MAC format after the model's
save()method applies sanitization.Also applies to: 679-679
1002-1002: LGTM!The test expectations correctly reflect the MAC sanitization behavior:
- Line 1002: Normalized from
"00-27-22-F3-FA-F1:hostname"to"00:27:22:f3:fa:f1"(hostname suffix stripped as expected when parsing MAC)- Lines 1039/1045: Filter and assertion use consistent normalized format
Also applies to: 1039-1045
1713-1714: LGTM!The assertion correctly verifies the full
_AUTH_TYPE_ACCEPT_RESPONSEstructure, ensuring complete response validation.
2009-2009: LGTM!The MAC address roaming tests correctly use normalized formats and the
sanitize_mac_addresshelper for dynamic comparisons, ensuring robust assertions regardless of input format variations.Also applies to: 2018-2020
2100-2100: LGTM!Adding
nas_ip_addressto the filter improves test specificity and reduces potential false positives when multiple sessions exist.
2356-2393: LGTM!The new IP allowlist tests provide good coverage:
test_ip_from_radsetting_valid: Correctly verifies org-levelfreeradius_allowed_hostsworks when global setting is emptytest_ip_from_setting_valid: Verifies default global setting allows the test clienttest_ip_from_radsetting_not_exist: Properly tests fallback behavior with subtests for both allowed and denied scenariosThe partial assertion pattern (
assertIn+assertEqualoncontrol:Auth-Type) is appropriate here since these tests focus on IP allowlist behavior, not counter/quota response fields.
2453-2454: LGTM!The assertion pattern is consistent with the base test class and appropriate for IP allowlist verification.
openwisp_radius/tests/test_commands.py (9)
5-5: LGTM!Standard swapper usage for loading the Organization model.
Also applies to: 21-21
29-33: LGTM!The
_get_defaultsoverride correctly handles pre-resolved Organization instances, avoiding redundant processing while properly falling back to the parent implementation when needed.
37-119: LGTM!The test refactoring properly:
- Uses
get_or_createto handle existing organizations- Consistently passes the Organization instance to each subtest
- Uses a separate
sub_optionsdict in the final subtest to avoid state mutation
152-152: LGTM!Minor cleanup changes: consistent variable naming, updated CSV fixture path, and simplified command invocation.
Also applies to: 287-291, 377-377
393-468: LGTM!The test restructuring correctly:
- Uses normalized MAC format in
unconverted_idsto match the sanitized database values- Adds unique subtest names with "- with org_id" suffix
- Properly calculates lowercased
routing_keyfor routing lookups- Patches the correct logger path (
BaseConvertCalledStationIdCommand.logger.warning)This addresses the past review concerns about normalizing
unconverted_idsand duplicate subtest names.
470-566: LGTM!The telnet error handling subtests correctly patch the command's internal methods and verify appropriate warning messages for various failure scenarios.
568-631: LGTM!The subtests have unique names ("- with unique_id" suffix) and properly reset session state before each test scenario. This addresses the past review concerns about duplicate subtest names.
633-722: LGTM!Test expectations consistently use the normalized lowercase colon-separated MAC format, correctly reflecting the
sanitize_mac_addressbehavior applied during model save.
724-751: LGTM!The slug-based test correctly mirrors the org_id test with normalized MAC formats in both configuration and assertions.
| from unittest import mock | ||
|
|
||
| import swapper | ||
| import swapper as _swapper |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Consider using swapper directly without aliasing.
The alias _swapper adds indirection without clear benefit. Since swapper.load_model is only used twice, you could import it directly or use the standard swapper name.
♻️ Optional simplification
-import swapper as _swapper
+import swapper
...
-Organization = _swapper.load_model("openwisp_users", "Organization")
-OrganizationUser = _swapper.load_model("openwisp_users", "OrganizationUser")
+Organization = swapper.load_model("openwisp_users", "Organization")
+OrganizationUser = swapper.load_model("openwisp_users", "OrganizationUser")Also applies to: 38-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/tests/test_api/test_freeradius_api.py` at line 6, Replace the
aliased import "import swapper as _swapper" with a direct import and update
usages: either use "import swapper" and change all "_swapper.load_model(...)"
calls to "swapper.load_model(...)" or import the function directly with "from
swapper import load_model" and replace "_swapper.load_model(...)" with
"load_model(...)"; update the two occurrences (previously using
_swapper.load_model) accordingly to remove the unnecessary alias.
f9a98a4 to
6d6b8c6
Compare
Test Failure in
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
setup.py (1)
11-11: 🧹 Nitpick | 🔵 TrivialDrop this unrelated literal-only churn.
This raw-string change is behaviorally equivalent and not tied to the MAC sanitization objective; consider reverting to keep the PR focused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.py` at line 11, Revert the unrelated literal-only change in the os.system invocation: restore the original string quoting for the cleanup command used in the os.system(...) call instead of the new raw-string form (the line containing os.system(r'find . | grep -E "(__pycache__|\.pyc|\.pyo$)" | xargs rm -rf')). Leave the cleanup command behavior unchanged but remove this cosmetic/raw-string edit so the PR remains focused on MAC sanitization.openwisp_radius/tests/test_tasks.py (1)
241-245:⚠️ Potential issue | 🟡 MinorUse the newly queued email before asserting HTML.
At Line 243, the test still asserts against a stale
💡 Proposed fix
with self.subTest("it should send mail in user language preference"): user.language = "it" user.save(update_fields=["language"]) tasks.send_login_email.delay(accounting_data) + self.assertEqual(len(mail.outbox), total_mails + 1) + email = mail.outbox.pop() self.assertRegex( "".join(email.alternatives[0][0].splitlines()), r'<a href=".*?sesame=.*">.*Manage Session.*</a>', )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/tests/test_tasks.py` around lines 241 - 245, The test is asserting against a stale `email` object after calling tasks.send_login_email.delay(accounting_data); update the test to first retrieve/pop the newly queued message (e.g., pop from the email outbox) into the `email` variable and then run the HTML assertion using "".join(email.alternatives[0][0].splitlines()); this ensures the assertion checks the Italian-language email produced by tasks.send_login_email.delay rather than a previous subtest's email.openwisp_radius/tests/test_api/test_api.py (1)
1753-1767:⚠️ Potential issue | 🟡 MinorNormalization coverage regressed for
called_station_idinput formats.Both fixtures (Lines 1753 and 1767) and the filter input (Line 1816) are canonical now, so this test path no longer verifies alternate-format normalization.
🧪 Suggested adjustment
data2.update( dict( session_id="40111116", unique_id="12234f69", input_octets=3000909, output_octets=1613176609, username="tester", organization=org1, calling_station_id="11-22-33-44-55-66", - called_station_id="aa:bb:cc:dd:ee:ff", + called_station_id="AA-BB-CC-DD-EE-FF", ) ) @@ - response = self.client.get(path, {"called_station_id": "aa:bb:cc:dd:ee:ff"}) + response = self.client.get(path, {"called_station_id": "AA-BB-CC-DD-EE-FF"})Also applies to: 1816-1820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/tests/test_api/test_api.py` around lines 1753 - 1767, The test lost coverage for normalization of called_station_id because both fixtures and the filter now use canonical MAC formats; change one of the inputs used in the path (e.g., the data passed into self._create_radius_accounting via data1 or data2 / acct_post_data) to an alternate MAC format (such as dashed "11-22-33-44-55-66" or uppercase without colons) so the normalization logic is exercised; update the test case that asserts filtering/normalization to use that alternate-format called_station_id while keeping the filter/query using the canonical format to verify normalization succeeds.openwisp_radius/base/models.py (2)
201-214:⚠️ Potential issue | 🟠 MajorUse full-string MAC matching to keep non-MAC values unchanged.
Using
re.searchat Line 207 still normalizes MAC-like substrings inside larger identifiers, which is destructive.🛠️ Proposed fix
for pattern in mac_patterns: - match = re.search(pattern, mac) - if match: + if re.fullmatch(pattern, mac): try: - eui = EUI(match.group(0)) + eui = EUI(mac) return ":".join([f"{x:02x}" for x in eui.words]).lower() except (AddrFormatError, ValueError, TypeError): continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/base/models.py` around lines 201 - 214, The current MAC normalization loop in mac_patterns uses re.search which finds MAC-like substrings inside larger strings and alters non-MAC values; update the matching to require the entire input be a MAC by using full-string matching (e.g., re.fullmatch or anchoring patterns with ^...$ / \A...\Z) when iterating mac_patterns, so that only inputs that wholly match a MAC pattern are passed to EUI and converted; keep the existing error handling around EUI(match.group(0)) and the exception tuple (AddrFormatError, ValueError, TypeError) in the same function where mac_patterns and the EUI conversion occur.
618-621:⚠️ Potential issue | 🟠 MajorBackfill legacy
called_station_idrows before relying on canonical equality.Filtering only by sanitized
called_station_idat Line 620 can miss pre-existing non-canonical records after upgrade.A data migration should canonicalize historical
radacct.called_station_idvalues withsanitize_mac_addressbefore this behavior is fully relied upon.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/base/models.py` around lines 618 - 621, Add a data migration to canonicalize historical radacct.called_station_id values by running sanitize_mac_address over existing rows before relying on canonical equality: create a migration that loads the Radacct model (the same model using cls in the code), iterates over records where called_station_id is not null and sanitize_mac_address(called_station_id) != called_station_id, and updates called_station_id to sanitize_mac_address(called_station_id) (apply to all rows, not only active/stale ones, to avoid missing legacy non-canonical records); after running this migration you can keep the current filter using sanitize_mac_address and cls.objects.filter(...) safely.openwisp_radius/tests/test_api/test_freeradius_api.py (1)
2364-2366:⚠️ Potential issue | 🟡 MinorRestore the full accept-payload assertion in these allowlist tests.
These branches now only prove that authorization succeeded. Regressions in
Session-TimeoutorCoovaChilli-Max-Total-Octetswould go unnoticed.🔧 Suggested fix
- self.assertIn("control:Auth-Type", response.data) - self.assertEqual(response.data["control:Auth-Type"], "Accept") + self.assertEqual(response.data, _AUTH_TYPE_ACCEPT_RESPONSE)Also applies to: 2370-2372, 2387-2389, 2453-2454
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/tests/test_api/test_freeradius_api.py` around lines 2364 - 2366, The test currently only asserts authorization succeeded; restore the full accept-payload checks by re-adding assertions that the expected accept attributes are present and have correct values on the response payload (e.g. assertEqual(response.status_code, 200); assertIn("control:Auth-Type", response.data); assertEqual(response.data["control:Auth-Type"], "Accept"); and also assertIn/ assertEqual for the other expected keys such as "Session-Timeout" and "CoovaChilli-Max-Total-Octets" (and any other accept attributes your test originally validated) using the same response variable so regressions in those fields are caught.
🤖 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/api/serializers.py`:
- Around line 275-277: The code indexes acct_data["called_station_id"] and calls
sanitize_mac_address without checking the key exists, which can raise KeyError;
modify the logic in the serializer so you only compute sanitized and compare
instance.called_station_id when "called_station_id" is present in acct_data
(e.g. if "called_station_id" in acct_data: sanitized =
sanitize_mac_address(acct_data["called_station_id"]); if not ids or
instance.called_station_id == sanitized: return acct_data), otherwise skip
sanitization and proceed without accessing acct_data["called_station_id"].
In `@openwisp_radius/management/commands/base/convert_called_station_id.py`:
- Around line 31-39: The mac matcher in _search_mac_address currently uses
re.search over mac_patterns and will match any 12-hex substring; update
mac_patterns to enforce hex boundaries so you only match standalone MAC tokens.
Wrap each pattern with negative lookbehind/lookahead like
(?<![0-9A-Fa-f])...pattern...(?![0-9A-Fa-f]) (e.g.
(?<![0-9A-Fa-f])([0-9A-Fa-f]{12})(?![0-9A-Fa-f])) and apply the same boundary
wrapping to the colon/period variants so the for-loop in _search_mac_address
returns only whole MAC matches.
In `@openwisp_radius/tests/test_api/test_freeradius_api.py`:
- Around line 1039-1045: Update the test query to pass a non-canonical MAC
string (e.g., with dashes, dots, or no separators) to self._acct_url (instead of
already normalized "e0:ca:40:ee:d1:0d") so the API's request-side normalization
is exercised; after calling the endpoint (response), assert the returned list
length and status as before, then assert that item["called_station_id"] equals
the normalized canonical form "e0:ca:40:ee:d1:0d" to verify the API normalizes
incoming MAC formats (references: self._acct_url, called_station_id, response,
item).
- Around line 405-409: The test helper currently normalizes both the expected
input and the stored value for the called_station_id check, which masks
persistence bugs; update the assertion logic in the block where key ==
"called_station_id" to only sanitize the expected input (data_value) using
sanitize_mac_address if present, but do not alter ra_value (the value read back
from RadiusAccounting) so the assertion compares the persisted field verbatim;
adjust the code around sanitize_mac_address, ra_value, data_value and the key
check accordingly.
In `@openwisp_radius/tests/test_commands.py`:
- Around line 393-394: The tests seed "unconverted_ids" with already-normalized
MACs so _get_unconverted_sessions() never exercises its sanitize_mac_address
step; update the test fixtures referenced by the failing tests to include at
least one unsanitized MAC (e.g., mixed case, missing leading zero, or different
separators) in the "unconverted_ids" list used by the tests that exercise
_get_unconverted_sessions(), so sanitize_mac_address is invoked and tested;
ensure the unsanitized value still maps to the expected normalized value in
assertions.
---
Duplicate comments:
In `@openwisp_radius/base/models.py`:
- Around line 201-214: The current MAC normalization loop in mac_patterns uses
re.search which finds MAC-like substrings inside larger strings and alters
non-MAC values; update the matching to require the entire input be a MAC by
using full-string matching (e.g., re.fullmatch or anchoring patterns with ^...$
/ \A...\Z) when iterating mac_patterns, so that only inputs that wholly match a
MAC pattern are passed to EUI and converted; keep the existing error handling
around EUI(match.group(0)) and the exception tuple (AddrFormatError, ValueError,
TypeError) in the same function where mac_patterns and the EUI conversion occur.
- Around line 618-621: Add a data migration to canonicalize historical
radacct.called_station_id values by running sanitize_mac_address over existing
rows before relying on canonical equality: create a migration that loads the
Radacct model (the same model using cls in the code), iterates over records
where called_station_id is not null and sanitize_mac_address(called_station_id)
!= called_station_id, and updates called_station_id to
sanitize_mac_address(called_station_id) (apply to all rows, not only
active/stale ones, to avoid missing legacy non-canonical records); after running
this migration you can keep the current filter using sanitize_mac_address and
cls.objects.filter(...) safely.
In `@openwisp_radius/tests/test_api/test_api.py`:
- Around line 1753-1767: The test lost coverage for normalization of
called_station_id because both fixtures and the filter now use canonical MAC
formats; change one of the inputs used in the path (e.g., the data passed into
self._create_radius_accounting via data1 or data2 / acct_post_data) to an
alternate MAC format (such as dashed "11-22-33-44-55-66" or uppercase without
colons) so the normalization logic is exercised; update the test case that
asserts filtering/normalization to use that alternate-format called_station_id
while keeping the filter/query using the canonical format to verify
normalization succeeds.
In `@openwisp_radius/tests/test_api/test_freeradius_api.py`:
- Around line 2364-2366: The test currently only asserts authorization
succeeded; restore the full accept-payload checks by re-adding assertions that
the expected accept attributes are present and have correct values on the
response payload (e.g. assertEqual(response.status_code, 200);
assertIn("control:Auth-Type", response.data);
assertEqual(response.data["control:Auth-Type"], "Accept"); and also assertIn/
assertEqual for the other expected keys such as "Session-Timeout" and
"CoovaChilli-Max-Total-Octets" (and any other accept attributes your test
originally validated) using the same response variable so regressions in those
fields are caught.
In `@openwisp_radius/tests/test_tasks.py`:
- Around line 241-245: The test is asserting against a stale `email` object
after calling tasks.send_login_email.delay(accounting_data); update the test to
first retrieve/pop the newly queued message (e.g., pop from the email outbox)
into the `email` variable and then run the HTML assertion using
"".join(email.alternatives[0][0].splitlines()); this ensures the assertion
checks the Italian-language email produced by tasks.send_login_email.delay
rather than a previous subtest's email.
In `@setup.py`:
- Line 11: Revert the unrelated literal-only change in the os.system invocation:
restore the original string quoting for the cleanup command used in the
os.system(...) call instead of the new raw-string form (the line containing
os.system(r'find . | grep -E "(__pycache__|\.pyc|\.pyo$)" | xargs rm -rf')).
Leave the cleanup command behavior unchanged but remove this cosmetic/raw-string
edit so the PR remains focused on MAC sanitization.
🪄 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: 55ab430d-ebd3-4cd6-b0af-498ec1397651
📒 Files selected for processing (15)
openwisp_radius/api/freeradius_views.pyopenwisp_radius/api/serializers.pyopenwisp_radius/base/models.pyopenwisp_radius/management/commands/base/convert_called_station_id.pyopenwisp_radius/migrations/0002_initial_openwisp_radius.pyopenwisp_radius/migrations/0041_alter_organizationradiussettings_token.pyopenwisp_radius/migrations/0043_merge_20251222_0406.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_freeradius_api.pyopenwisp_radius/tests/test_commands.pyopenwisp_radius/tests/test_tasks.pysetup.pytests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.pytests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.pytests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.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). (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Code formatting must be compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Applied to files:
openwisp_radius/base/models.py
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Flag unused or redundant code
Applied to files:
openwisp_radius/base/models.py
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_radius/tests/test_api/test_freeradius_api.py
🔇 Additional comments (9)
tests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.py (1)
508-508: Looks good.This update is consistent with the intended validator behavior and migration alignment.
tests/openwisp2/sample_radius/migrations/0031_alter_organizationradiussettings_token.py (1)
19-35: LGTM.Migration is clear and internally consistent.
tests/openwisp2/sample_radius/migrations/0032_merge_20251222_0407.py (1)
8-13: Merge migration is correct.Dependency stitching is explicit and
operations = []is appropriate here.openwisp_radius/migrations/0002_initial_openwisp_radius.py (1)
251-251: Looks good.The updated pattern is consistent with the validator intent.
openwisp_radius/migrations/0043_merge_20251222_0406.py (1)
8-13: LGTM.Clean merge-point migration; no action needed.
openwisp_radius/migrations/0041_alter_organizationradiussettings_token.py (1)
19-35: LGTM.Field alteration is well-formed and consistent with the migration chain.
openwisp_radius/tests/test_tasks.py (1)
202-205: Regex literal cleanup looks correct.Using a raw regex with
</a>at Line 204 is cleaner and equivalent for this assertion.openwisp_radius/api/freeradius_views.py (1)
375-378: Good canonical comparison in simultaneous-use exclusion.Sanitizing
called_station_idat Line 376 improves consistency with normalized accounting values.openwisp_radius/tests/test_api/test_api.py (1)
816-819: Regex update is correct.The raw-string anchor assertion at Line 818 is fine and keeps the intent intact.
| mac_patterns = [ | ||
| r"([0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}", | ||
| r"([0-9A-Fa-f]{4}\.){2}[0-9A-Fa-f]{4}", | ||
| r"[0-9A-Fa-f]{12}", | ||
| ] | ||
| for pattern in mac_patterns: | ||
| match = re.search(pattern, common_name) | ||
| if match: | ||
| return match[0] |
There was a problem hiding this comment.
The plain-hex matcher will accept arbitrary 12-hex substrings.
Because _search_mac_address() uses re.search, a common_name like client-deadbeefcafe-token will be treated as a MAC and persisted. Add hex-boundaries around these patterns before returning a match.
🔧 Suggested fix
mac_patterns = [
- r"([0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}",
- r"([0-9A-Fa-f]{4}\.){2}[0-9A-Fa-f]{4}",
- r"[0-9A-Fa-f]{12}",
+ r"(?<![0-9A-Fa-f])(?:[0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}(?![0-9A-Fa-f])",
+ r"(?<![0-9A-Fa-f])(?:[0-9A-Fa-f]{4}\.){2}[0-9A-Fa-f]{4}(?![0-9A-Fa-f])",
+ r"(?<![0-9A-Fa-f])[0-9A-Fa-f]{12}(?![0-9A-Fa-f])",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mac_patterns = [ | |
| r"([0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}", | |
| r"([0-9A-Fa-f]{4}\.){2}[0-9A-Fa-f]{4}", | |
| r"[0-9A-Fa-f]{12}", | |
| ] | |
| for pattern in mac_patterns: | |
| match = re.search(pattern, common_name) | |
| if match: | |
| return match[0] | |
| mac_patterns = [ | |
| r"(?<![0-9A-Fa-f])(?:[0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}(?![0-9A-Fa-f])", | |
| r"(?<![0-9A-Fa-f])(?:[0-9A-Fa-f]{4}\.){2}[0-9A-Fa-f]{4}(?![0-9A-Fa-f])", | |
| r"(?<![0-9A-Fa-f])[0-9A-Fa-f]{12}(?![0-9A-Fa-f])", | |
| ] | |
| for pattern in mac_patterns: | |
| match = re.search(pattern, common_name) | |
| if match: | |
| return match[0] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/management/commands/base/convert_called_station_id.py` around
lines 31 - 39, The mac matcher in _search_mac_address currently uses re.search
over mac_patterns and will match any 12-hex substring; update mac_patterns to
enforce hex boundaries so you only match standalone MAC tokens. Wrap each
pattern with negative lookbehind/lookahead like
(?<![0-9A-Fa-f])...pattern...(?![0-9A-Fa-f]) (e.g.
(?<![0-9A-Fa-f])([0-9A-Fa-f]{12})(?![0-9A-Fa-f])) and apply the same boundary
wrapping to the colon/period variants so the for-loop in _search_mac_address
returns only whole MAC matches.
| f"{self._acct_url}?called_station_id=e0:ca:40:ee:d1:0d", | ||
| HTTP_AUTHORIZATION=self.auth_header, | ||
| ) | ||
| self.assertEqual(len(response.json()), 1) | ||
| self.assertEqual(response.status_code, 200) | ||
| item = response.data[0] | ||
| self.assertEqual(item["called_station_id"], "E0-CA-40-EE-D1-0D") | ||
| self.assertEqual(item["called_station_id"], "e0:ca:40:ee:d1:0d") |
There was a problem hiding this comment.
This filter test no longer exercises request-side MAC normalization.
The request now uses an already normalized called_station_id, so this passes even if the API stops sanitizing dash/dot/unseparated inputs. Keep the query in a non-canonical format and assert the response is normalized.
🔧 Suggested fix
response = self.client.get(
- f"{self._acct_url}?called_station_id=e0:ca:40:ee:d1:0d",
+ f"{self._acct_url}?called_station_id=E0-CA-40-EE-D1-0D",
HTTP_AUTHORIZATION=self.auth_header,
)
self.assertEqual(len(response.json()), 1)
self.assertEqual(response.status_code, 200)
item = response.data[0]
- self.assertEqual(item["called_station_id"], "e0:ca:40:ee:d1:0d")
+ self.assertEqual(
+ item["called_station_id"],
+ sanitize_mac_address(data1["called_station_id"]),
+ )Based on learnings: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/tests/test_api/test_freeradius_api.py` around lines 1039 -
1045, Update the test query to pass a non-canonical MAC string (e.g., with
dashes, dots, or no separators) to self._acct_url (instead of already normalized
"e0:ca:40:ee:d1:0d") so the API's request-side normalization is exercised; after
calling the endpoint (response), assert the returned list length and status as
before, then assert that item["called_station_id"] equals the normalized
canonical form "e0:ca:40:ee:d1:0d" to verify the API normalizes incoming MAC
formats (references: self._acct_url, called_station_id, response, item).
| "unconverted_ids": ["aa:aa:aa:aa:aa:0a"], | ||
| } |
There was a problem hiding this comment.
Keep at least one unconverted_ids fixture unsanitized.
Both command tests now seed unconverted_ids with already normalized values, so _get_unconverted_sessions()'s new sanitize_mac_address step is never exercised. If that normalization is removed, these tests still pass.
🔧 Suggested fix
- "unconverted_ids": ["aa:aa:aa:aa:aa:0a"],
+ "unconverted_ids": ["AA-AA-AA-AA-AA-0A"],- "unconverted_ids": ["aa:aa:aa:aa:aa:0a"],
+ "unconverted_ids": ["AA-AA-AA-AA-AA-0A"],Also applies to: 733-734
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/tests/test_commands.py` around lines 393 - 394, The tests
seed "unconverted_ids" with already-normalized MACs so
_get_unconverted_sessions() never exercises its sanitize_mac_address step;
update the test fixtures referenced by the failing tests to include at least one
unsanitized MAC (e.g., mixed case, missing leading zero, or different
separators) in the "unconverted_ids" list used by the tests that exercise
_get_unconverted_sessions(), so sanitize_mac_address is invoked and tested;
ensure the unsanitized value still maps to the expected normalized value in
assertions.
dadc0e1 to
183c688
Compare
Test Failure in User Registration MetricsHello @Eeshu-Yadav, The CI build failed due to a test failure in Failure: The assertion Fix: Review the logic in There were also several slow tests reported, which might indicate performance issues that could be addressed separately. |
Eeshu-Yadav
force-pushed
the
issues/624-sanitize-called-station-id-mac-address
branch
from
183c688 to
2d59e5b
Compare
Checklist
Reference to Existing Issue
Closes #624
Description of Changes
This change implements MAC address sanitization in the called_station_id
field of RadiusAccounting to ensure consistent formatting across different
input formats. MAC addresses are now automatically converted to lowercase
colon-separated format (aa:bb:cc:dd:ee:ff) while preserving non-MAC values
unchanged for RFC compliance.