[change!] Moved password reset to openwisp-users & prevent user enumeration #754 - #757
[change!] Moved password reset to openwisp-users & prevent user enumeration #754#757pandafy wants to merge 14 commits into
Conversation
…ration #754 - Password reset now always returns HTTP 200 to prevent user enumeration. Previously, unknown users returned HTTP 404. - Password reset is now handled by openwisp-users. The local PasswordResetSerializer is deprecated and will be removed in 1.4.0. - SAML sessions are now marked as externally authenticated, preventing PasswordExpirationMiddleware from redirecting SAML users to the password change page. Closes #754
|
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:
📝 WalkthroughWalkthroughPassword reset handling now delegates to OpenWISP Users, with organization-scoped active-user filtering, configurable reset URLs, throttling, and membership validation. Deprecated local form and serializer wrappers remain for compatibility. Password, SAML, and captive-page authentication flows now record their authentication methods, and password expiration reporting distinguishes external from password-based authentication. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PasswordResetView
participant OrganizationMembers
participant CurrentSite
Client->>PasswordResetView: Submit reset identifier
PasswordResetView->>OrganizationMembers: Find active organization members
PasswordResetView->>CurrentSite: Read reset URL domain
PasswordResetView-->>Client: Return reset response
sequenceDiagram
participant PasswordLogin
participant SAMLLogin
participant CaptiveRedirect
participant AuthenticationState
participant PasswordExpirationMiddleware
PasswordLogin->>AuthenticationState: Record PASSWORD
SAMLLogin->>AuthenticationState: Record EXTERNAL
CaptiveRedirect->>AuthenticationState: Record EXTERNAL
PasswordExpirationMiddleware->>AuthenticationState: Check authentication method
AuthenticationState-->>PasswordExpirationMiddleware: Return PASSWORD or EXTERNAL
Possibly related issues
Possibly related PRs
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 4 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Dependency Conflict During Package InstallationHello @pandafy, There is a dependency conflict between Fix: The (
"openwisp-users @"
"https://github.com/openwisp/openwisp-users/archive/refs/heads/"
"issues/511-passowrd-expiration-sso.tar.gz"
),to use a version that aligns with the other (
"openwisp-users @"
"https://github.com/openwisp/openwisp-users/archive/refs/heads/"
"1.3.tar.gz"
),This change will ensure that both |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_radius/api/views.py`:
- Around line 564-580: Extend tests for get_password_reset_url to verify the
organization’s radius_settings.password_reset_url takes precedence over
PASSWORD_RESET_URLS, PASSWORD_RESET_URLS is used when no organization override
exists, and the default URL is used when neither is configured. Assert each
generated URL interpolates organization, uid, token, and current-site domain
values.
- Around line 564-566: Remove the unnecessary blank line immediately after the
get_password_reset_url method declaration, keeping uid =
user_pk_to_url_str(user) as the first statement in the method body.
In `@openwisp_radius/tests/test_saml/test_views.py`:
- Line 366: Remove the commented-out `@capture_any_output`() decorator near the
affected test; do not restore it unless that test explicitly requires output
capture.
- Line 375: In the test setup around the user-under-test flow, replace the broad
User.objects.update call with a direct update on the user instance so only that
user’s password_updated value is changed; leave unrelated users and fixtures
untouched.
- Around line 395-398: Strengthen the follow-up request assertion in the session
redirect test by verifying that additional_info_response redirects to the
expected destination rather than only checking status 302. Assert its Location
target is not the password-change URL, preserving the existing successful
redirect behavior.
In `@setup.py`:
- Around line 45-46: Update the OpenWISP Users dependency URL in setup.py to
reference an immutable commit SHA or a release archive instead of the mutable
issues/511-passowrd-expiration-sso branch archive, preserving the dependency’s
intended source.
🪄 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: 7faea65b-c4ff-4db5-8295-c4206a1b2ba3
📒 Files selected for processing (9)
openwisp_radius/api/serializers.pyopenwisp_radius/api/views.pyopenwisp_radius/base/forms.pyopenwisp_radius/saml/views.pyopenwisp_radius/templates/custom_password_reset_email.htmlopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_saml/test_views.pysetup.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (2)
- openwisp_radius/templates/custom_password_reset_email.html
- openwisp_radius/base/forms.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.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.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.
Files:
openwisp_radius/saml/views.pytests/openwisp2/settings.pysetup.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_saml/test_views.pyopenwisp_radius/api/views.pyopenwisp_radius/api/serializers.py
🔇 Additional comments (6)
openwisp_radius/saml/views.py (1)
24-24: LGTM!Also applies to: 66-66
openwisp_radius/tests/test_saml/test_views.py (1)
2-3: LGTM!Also applies to: 12-22
openwisp_radius/api/serializers.py (1)
28-30: LGTM!Also applies to: 59-65
tests/openwisp2/settings.py (1)
302-302: LGTM!openwisp_radius/api/views.py (1)
56-59: LGTM!Also applies to: 540-562, 604-605
openwisp_radius/tests/test_api/test_api.py (1)
918-933: LGTM!Also applies to: 967-987, 1158-1165
| "https://github.com/openwisp/openwisp-users/archive/refs/heads/" | ||
| "issues/511-passowrd-expiration-sso.tar.gz" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-remote \
https://github.com/openwisp/openwisp-users.git \
refs/heads/issues/511-passowrd-expiration-ssoRepository: openwisp/openwisp-radius
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '35,55p' setup.py | cat -nRepository: openwisp/openwisp-radius
Length of output: 1041
Pin the OpenWISP Users dependency to a commit SHA. This branch archive is still mutable, so builds can drift; use an immutable commit pin or release archive instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@setup.py` around lines 45 - 46, Update the OpenWISP Users dependency URL in
setup.py to reference an immutable commit SHA or a release archive instead of
the mutable issues/511-passowrd-expiration-sso branch archive, preserving the
dependency’s intended source.
5a2df1d to
57bd2ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openwisp_radius/api/views.py (1)
569-575: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRead the raw organization override before applying the settings fallback.
FallbackCharFieldreturns its fallback when the stored value isNULL; therefore Line 570 is always truthy and the PK-keyedPASSWORD_RESET_URLSlookup is unreachable. Organizations configured only throughPASSWORD_RESET_URLS[str(org.pk)]receive the global default URL instead. (openwisp.io)
openwisp_radius/api/views.py#L569-L575: distinguish the storedpassword_reset_urlvalue from its fallback, then use the PK-keyed mapping when no explicit organization URL is stored.openwisp_radius/tests/test_api/test_api.py#L1043-L1086: add a regression test for a PK-keyedPASSWORD_RESET_URLSentry and assert its fully interpolated URL is emailed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_radius/api/views.py` around lines 569 - 575, In openwisp_radius/api/views.py lines 569-575, update the password-reset URL selection to inspect the raw stored organization value rather than the FallbackCharField-resolved value, using the PK-keyed PASSWORD_RESET_URLS entry when no explicit override is stored; preserve the global fallback otherwise. In openwisp_radius/tests/test_api/test_api.py lines 1043-1086, add a regression test configuring a PK-keyed PASSWORD_RESET_URLS entry and assert that the emailed URL is fully interpolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_radius/tests/test_api/test_rest_token.py`:
- Around line 306-307: Merge test_password_login_triggers_password_expiration
into test_user_auth_token_password_expired, using the latter as the single
coverage point for this endpoint and password_expired=True behavior. Move the
targeted-user setup and precondition assertion into
test_user_auth_token_password_expired, preserve the USER_PASSWORD_EXPIRATION
patch, and remove the redundant test.
- Line 315: Remove the redundant “Login via password” comment immediately before
the password-login call in the affected test, leaving the test behavior and
surrounding code unchanged.
---
Duplicate comments:
In `@openwisp_radius/api/views.py`:
- Around line 569-575: In openwisp_radius/api/views.py lines 569-575, update the
password-reset URL selection to inspect the raw stored organization value rather
than the FallbackCharField-resolved value, using the PK-keyed
PASSWORD_RESET_URLS entry when no explicit override is stored; preserve the
global fallback otherwise. In openwisp_radius/tests/test_api/test_api.py lines
1043-1086, add a regression test configuring a PK-keyed PASSWORD_RESET_URLS
entry and assert that the emailed URL is fully interpolated.
🪄 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: 1baceba9-4a9a-4c43-8665-eef79b49879b
📒 Files selected for processing (6)
.github/workflows/ci.ymlopenwisp_radius/api/serializers.pyopenwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_saml/test_views.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.
Files:
openwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_saml/test_views.pyopenwisp_radius/api/views.pyopenwisp_radius/api/serializers.pyopenwisp_radius/tests/test_api/test_api.py
🧠 Learnings (2)
📚 Learning: 2026-03-06T08:47:54.428Z
Learnt from: stktyagi
Repo: openwisp/openwisp-radius PR: 689
File: .github/workflows/bot-ci-failure.yml:60-60
Timestamp: 2026-03-06T08:47:54.428Z
Learning: In openwisp/openwisp-radius and similar OpenWISP repos, reusable workflow caller files intentionally reference upstream reusable workflows with master (e.g., openwisp/openwisp-utils/.github/workflows/reusable-bot-ci-failure.ymlmaster). This is by design to ensure callers always use the latest changes from the master branch of the upstream project. Do not flag such references as security or pinning issues during code reviews. If applying this pattern broadly, treat any workflow call that uses a remote reusable workflow from a different repository with master as an intentional design choice rather than a vulnerability.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-04-17T13:53:45.430Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 698
File: .github/workflows/ci.yml:77-77
Timestamp: 2026-04-17T13:53:45.430Z
Learning: In .github/workflows/ci.yml, the CI step that runs `pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"` is an intentionally temporary blocker dependency (to use a pre-release branch until openwisp-users PR `#498` is merged). Do not flag this pip install as an issue in future reviews as long as it appears in this workflow with the same purpose.
Applied to files:
.github/workflows/ci.yml
🔇 Additional comments (5)
openwisp_radius/tests/test_saml/test_views.py (1)
364-365: LGTM!Also applies to: 374-376, 399-402
openwisp_radius/api/serializers.py (1)
2-2: LGTM!Also applies to: 29-35, 60-75
.github/workflows/ci.yml (1)
78-78: LGTM!openwisp_radius/api/views.py (1)
4-4: LGTM!Also applies to: 26-26, 44-44, 523-540, 591-606
openwisp_radius/tests/test_api/test_api.py (1)
918-987: LGTM!Also applies to: 1201-1209
| @mock.patch("openwisp_users.settings.USER_PASSWORD_EXPIRATION", 30) | ||
| def test_password_login_triggers_password_expiration(self): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Merge this with the existing expiration test.
test_user_auth_token_password_expired already exercises the same endpoint and asserts password_expired=True. Move the targeted-user setup and precondition assertion into that test instead of retaining duplicate coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_radius/tests/test_api/test_rest_token.py` around lines 306 - 307,
Merge test_password_login_triggers_password_expiration into
test_user_auth_token_password_expired, using the latter as the single coverage
point for this endpoint and password_expired=True behavior. Move the
targeted-user setup and precondition assertion into
test_user_auth_token_password_expired, preserve the USER_PASSWORD_EXPIRATION
patch, and remove the redundant test.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous Review Summaries (8 snapshots, latest commit 107dc46)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 107dc46)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 7ea7f74)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit cd18b7f)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 14fe03a)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 7f6b779)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 8301db2)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 69a947b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit baa7724)Status: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Reviewed by laguna-s-2.1:free · Input: 541.1K · Output: 52.9K · Cached: 1.8M |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_radius/api/views.py`:
- Line 526: Update the password-reset view’s throttle_classes declaration to
explicitly include ScopedRateThrottle alongside the configured
DEFAULT_THROTTLE_CLASSES, preserving the inherited “others” scope limit even
when global defaults are empty or omit it.
🪄 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: f15b27da-f58f-4996-9a09-0dab333234ab
📒 Files selected for processing (1)
openwisp_radius/api/views.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.
Files:
openwisp_radius/api/views.py
🔇 Additional comments (1)
openwisp_radius/api/views.py (1)
4-6: LGTM!Also applies to: 26-26, 43-45, 57-60, 528-533, 542-587, 609-612
|
|
||
| class PasswordResetView(ThrottledAPIMixin, DispatchOrgMixin, BasePasswordResetView): | ||
| authentication_classes = tuple() | ||
| throttle_classes = drf_api_settings.DEFAULT_THROTTLE_CLASSES |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep password-reset throttling explicit.
DEFAULT_THROTTLE_CLASSES can be empty or omit ScopedRateThrottle, which removes the inherited "others" limit from these public endpoints. Keep ScopedRateThrottle on the view, or guarantee it is always appended globally, so password reset stays rate-limited.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_radius/api/views.py` at line 526, Update the password-reset view’s
throttle_classes declaration to explicitly include ScopedRateThrottle alongside
the configured DEFAULT_THROTTLE_CLASSES, preserving the inherited “others” scope
limit even when global defaults are empty or omit it.
Flake8 F401 Unused ImportHello @pandafy, The CI failed because of an unused import detected by Flake8. Failure:
Fix: |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Please address the two compatibility regressions below. I pushed ace3c59 with a failing regression test for the superuser case.
| and preventing user enumeration. | ||
| """ | ||
| app_label = User._meta.app_config.label | ||
| return ( |
There was a problem hiding this comment.
Please preserve the existing superuser behavior here. The previous flow called validate_membership(), which allows superusers, but this relation filter excludes a superuser without an OrganizationUser record and silently sends no email. I pushed ace3c59 with a regression test. Include active superusers in this queryset and keep the membership filter for other users.
| self.fields["csvfile"].help_text = help_text | ||
|
|
||
|
|
||
| class PasswordResetForm(BasePasswordResetForm): |
There was a problem hiding this comment.
Please keep this import path as a deprecated alias or thin subclass of openwisp_users.base.forms.PasswordResetForm. Issue #511 requires preserving RADIUS extension points and import paths, but extensions importing or subclassing openwisp_radius.base.forms.PasswordResetForm will now fail immediately.
|
@coderabbitai full review! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/openwisp2/settings.py`:
- Line 310: Shorten the PASSWORD_RESET_SERIALIZER setting line in the settings
configuration so it satisfies the repository’s configured flake8 maximum line
length, while preserving the same serializer path and setting key.
🪄 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: f25f6847-8a25-47fd-8afb-622454737d22
📒 Files selected for processing (13)
.github/workflows/ci.ymlopenwisp_radius/api/serializers.pyopenwisp_radius/api/views.pyopenwisp_radius/base/forms.pyopenwisp_radius/saml/views.pyopenwisp_radius/social/views.pyopenwisp_radius/templates/custom_password_reset_email.htmlopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_saml/test_views.pyopenwisp_radius/tests/test_social.pytests/openwisp2/sample_users/migrations/0001_initial.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (1)
- openwisp_radius/templates/custom_password_reset_email.html
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.
Files:
tests/openwisp2/settings.pytests/openwisp2/sample_users/migrations/0001_initial.pyopenwisp_radius/social/views.pyopenwisp_radius/base/forms.pyopenwisp_radius/saml/views.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_social.pyopenwisp_radius/tests/test_saml/test_views.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/api/views.pyopenwisp_radius/api/serializers.py
🧠 Learnings (2)
📚 Learning: 2026-03-06T08:47:54.428Z
Learnt from: stktyagi
Repo: openwisp/openwisp-radius PR: 689
File: .github/workflows/bot-ci-failure.yml:60-60
Timestamp: 2026-03-06T08:47:54.428Z
Learning: In openwisp/openwisp-radius and similar OpenWISP repos, reusable workflow caller files intentionally reference upstream reusable workflows with master (e.g., openwisp/openwisp-utils/.github/workflows/reusable-bot-ci-failure.ymlmaster). This is by design to ensure callers always use the latest changes from the master branch of the upstream project. Do not flag such references as security or pinning issues during code reviews. If applying this pattern broadly, treat any workflow call that uses a remote reusable workflow from a different repository with master as an intentional design choice rather than a vulnerability.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-04-17T13:53:45.430Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 698
File: .github/workflows/ci.yml:77-77
Timestamp: 2026-04-17T13:53:45.430Z
Learning: In .github/workflows/ci.yml, the CI step that runs `pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"` is an intentionally temporary blocker dependency (to use a pre-release branch until openwisp-users PR `#498` is merged). Do not flag this pip install as an issue in future reviews as long as it appears in this workflow with the same purpose.
Applied to files:
.github/workflows/ci.yml
🔇 Additional comments (21)
openwisp_radius/base/forms.py (1)
1-7: LGTM!Also applies to: 55-69
openwisp_radius/api/serializers.py (2)
61-75: LGTM!
850-855: 🩺 Stability & AvailabilityConfirm
is_password_authenticatedsemantics whenrequestisNone.
RadiusUserSerializeris instantiated without a view/request in several places (e.g.RadiusUserSerializer(admin)in tests), sorequestisNonehere. The reportedpassword_expiredvalue for those callers depends entirely on howopenwisp_users.auth.is_password_authenticatedtreats a missing request (fall back touser.last_login_methodvs. returnFalse). Worth verifying so token/validate endpoints don't silently stop reporting expired passwords.#!/bin/bash fd -t f 'auth.py' -p 'openwisp_users' --exec ast-grep outline {} --items all python - <<'PY' import inspect try: from openwisp_users import auth print(inspect.getsource(auth.is_password_authenticated)) except Exception as e: print("unavailable:", e) PY.github/workflows/ci.yml (1)
78-78: LGTM!openwisp_radius/tests/test_api/test_rest_token.py (2)
312-327: Remove the duplicate password-expiration test.
test_user_auth_token_password_expireddirectly above covers the same expired-password login and response assertion; retain that test and remove this duplicate.
4-4: LGTM!Also applies to: 13-13, 34-34, 303-310, 494-506
openwisp_radius/saml/views.py (1)
24-29: LGTM!Also applies to: 70-70, 153-156
openwisp_radius/social/views.py (1)
10-11: LGTM!Also applies to: 73-76
tests/openwisp2/sample_users/migrations/0001_initial.py (1)
37-51: LGTM!openwisp_radius/tests/test_social.py (1)
1-16: LGTM!Also applies to: 139-176
openwisp_radius/tests/test_saml/test_views.py (1)
2-2: LGTM!Also applies to: 11-21, 361-403
openwisp_radius/api/views.py (5)
531-531: Password-reset throttling still depends entirely on global defaults.
DEFAULT_THROTTLE_CLASSESmay be empty or omitScopedRateThrottle, dropping the inherited"others"scope limit on this public endpoint. Previously flagged and still unaddressed.
4-4: LGTM!Also applies to: 26-26, 43-43, 56-60
319-323: LGTM!
549-579: LGTM!
581-601: LGTM!Also applies to: 611-611, 625-626
openwisp_radius/tests/test_api/test_api.py (5)
18-18: LGTM!Also applies to: 30-41
925-940: LGTM!
974-994: LGTM!
1050-1191: LGTM!
1307-1314: LGTM!
| REST_AUTH = { | ||
| "SESSION_LOGIN": False, | ||
| "PASSWORD_RESET_SERIALIZER": "openwisp_radius.api.serializers.PasswordResetSerializer", | ||
| "PASSWORD_RESET_SERIALIZER": "openwisp_users.api.serializers.PasswordResetSerializer", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Check line length against the QA config.
This line is ~92 characters; if flake8 is configured with the usual 88-char limit, run-qa-checks will fail with E501 (black won't split the string literal for you).
Proposed fix
- "PASSWORD_RESET_SERIALIZER": "openwisp_users.api.serializers.PasswordResetSerializer",
+ "PASSWORD_RESET_SERIALIZER": (
+ "openwisp_users.api.serializers.PasswordResetSerializer"
+ ),#!/bin/bash
fd -H -t f -e cfg -e toml -e ini . -d 2 --exec grep -Hn -i -A3 'flake8\|max-line-length' {}
awk 'NR==310 {print length($0)": "$0}' tests/openwisp2/settings.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/openwisp2/settings.py` at line 310, Shorten the
PASSWORD_RESET_SERIALIZER setting line in the settings configuration so it
satisfies the repository’s configured flake8 maximum line length, while
preserving the same serializer path and setting key.
2 participants
Checklist
Reference to Existing Issue
Closes #754
Description of Changes
Blockers