[fix] Don't block SSO and REST login when password is expired #511 - #544
[fix] Don't block SSO and REST login when password is expired #511#544pandafy wants to merge 10 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThe change tracks authentication methods, enforces password expiration for local-password sessions, and adds REST password reset, confirmation, and authenticated password-change endpoints. It adds custom reset forms and email templates, structured REST expiration responses, conditional routes, documentation, dependency updates, and coverage for authentication, middleware, recovery, and sample-app behavior. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PasswordExpirationMiddleware
participant AuthenticationSession
participant RESTAPIView
Client->>PasswordExpirationMiddleware: Send request
PasswordExpirationMiddleware->>AuthenticationSession: Check authentication method and expiry
AuthenticationSession-->>PasswordExpirationMiddleware: Return expired-password state
PasswordExpirationMiddleware->>RESTAPIView: Render password_expired response
RESTAPIView-->>Client: Return HTTP 403 with recovery URL
Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Test Failure: Assertion ErrorHello @pandafy, The CI failed because the test Fix: The test assertion |
Added REST API endpoints to allow users to reset and change their password. Fixes #511
929011c to
393fee6
Compare
![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
🤖 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_users/api/views.py`:
- Around line 265-278: Add the `rest_password_change` route name to the
password-expiration exemptions used by `PasswordExpirationMiddleware`, alongside
the existing `change_password` exemption. Keep `PasswordChangeView` and its
authentication behavior unchanged so expired-password clients can reach the
self-service recovery endpoint.
In `@openwisp_users/base/forms.py`:
- Around line 16-24: Update get_users to use a queryset lookup that returns an
empty result for an unknown email instead of calling User.objects.get. Preserve
the existing has_usable_password filtering and return shape for matching users.
- Around line 85-109: Update the send_mail method to ensure the from_email
parameter is honored when save() supplies a custom sender, forwarding it through
the existing send_email/context mechanism as supported by the API. If this
override cannot use from_email, remove the unused parameter and adjust callers
consistently.
In `@openwisp_users/middleware.py`:
- Around line 70-74: Update _blocked_response to pass request.path_info to
resolve instead of request.path, ensuring URL resolution works correctly when
the application is mounted under a script prefix and expired-password sessions
remain blocked.
In `@openwisp_users/templates/custom_password_reset_email.html`:
- Around line 8-10: Update the HTML password-reset email template near the
instructional text to render the `call_to_action_url` as a usable password-reset
link, preserving the existing translated message and ensuring HTML-preferred
clients can follow the reset action.
In `@openwisp_users/tests/test_api/test_password_reset.py`:
- Around line 49-58: Update
test_reset_request_with_unknown_identifier_is_indistinguishable to submit a
reset request for a known identifier and compare the unknown-user response’s
status_code and data against that known-user response, preserving the existing
no-email assertion and anti-enumeration behavior.
In `@tests/openwisp2/sample_users/tests.py`:
- Around line 150-157: Add a TestPasswordChangeAPI subclass alongside
TestPasswordResetAPI in the sample app test module, inheriting from the existing
base test class defined in test_password_reset.py, so the custom User model is
covered by the password-change endpoint tests.
🪄 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: 9fdbfcab-3096-48e8-9950-62986726dd4a
📒 Files selected for processing (19)
docs/user/rest-api.rstopenwisp_users/api/serializers.pyopenwisp_users/api/urls.pyopenwisp_users/api/views.pyopenwisp_users/apps.pyopenwisp_users/auth.pyopenwisp_users/base/forms.pyopenwisp_users/middleware.pyopenwisp_users/templates/custom_password_reset_email.htmlopenwisp_users/templates/custom_password_reset_email.txtopenwisp_users/tests/test_accounts.pyopenwisp_users/tests/test_admin.pyopenwisp_users/tests/test_api/test_password_reset.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_auth.pyopenwisp_users/tests/test_middlewares.pyrequirements.txttests/openwisp2/sample_users/tests.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (14)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
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
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside itFor Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
tests/openwisp2/settings.pyopenwisp_users/api/urls.pyopenwisp_users/auth.pyopenwisp_users/tests/test_admin.pyopenwisp_users/base/forms.pyopenwisp_users/tests/test_auth.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_accounts.pyopenwisp_users/tests/test_api/test_password_reset.pytests/openwisp2/sample_users/tests.pyopenwisp_users/tests/test_middlewares.pyopenwisp_users/api/views.pyopenwisp_users/apps.pyopenwisp_users/middleware.py
🪛 ast-grep (0.45.0)
openwisp_users/api/serializers.py
[warning] 21-21: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 HTMLHint (1.9.2)
openwisp_users/templates/custom_password_reset_email.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (17)
openwisp_users/auth.py (1)
1-53: LGTM!openwisp_users/apps.py (1)
3-18: LGTM!Also applies to: 143-170
openwisp_users/tests/test_auth.py (1)
1-34: LGTM!openwisp_users/middleware.py (1)
3-69: LGTM!Also applies to: 85-105
openwisp_users/tests/test_admin.py (1)
10-10: LGTM!Also applies to: 28-28, 2460-2465
openwisp_users/templates/custom_password_reset_email.txt (1)
1-8: LGTM!docs/user/rest-api.rst (1)
71-140: LGTM!requirements.txt (1)
4-4: LGTM!tests/openwisp2/settings.py (1)
39-39: LGTM!openwisp_users/tests/test_accounts.py (1)
4-16: LGTM!Also applies to: 31-73
openwisp_users/api/views.py (1)
2-9: LGTM!Also applies to: 24-25, 36-36, 69-132, 341-350
openwisp_users/tests/test_api/test_views.py (1)
1-10: LGTM!Also applies to: 25-48, 71-91
openwisp_users/tests/test_middlewares.py (1)
3-10: LGTM!Also applies to: 48-54, 55-58, 61-67, 69-73, 75-100, 102-113
openwisp_users/api/serializers.py (1)
5-11: LGTM!Also applies to: 20-21, 481-529
openwisp_users/api/urls.py (1)
49-64: LGTM!openwisp_users/tests/test_api/test_password_reset.py (1)
1-48: LGTM!Also applies to: 60-147
tests/openwisp2/sample_users/tests.py (1)
19-31: LGTM!Also applies to: 169-170
Test Failures in Password Reset APIHello @pandafy, The CI failed due to several test failures in the password reset API tests.
Please address these issues and push a new commit. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openwisp_users/templates/custom_password_reset_email.html (1)
8-10: 🎯 Functional Correctness | 🟠 MajorRender the reset URL in the HTML email.
The template still ends after the instructions and never outputs
call_to_action_url, so HTML-preferred clients receive no usable reset link. This is the same unresolved issue from the previous review.{% trans "Please click on the button below to open a page where you can choose a new password:" %} +<p><a href="{{ call_to_action_url }}" class="btn">{% trans "Reset your password" %}</a></p>🤖 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_users/templates/custom_password_reset_email.html` around lines 8 - 10, Update the password reset HTML template after the instructional paragraph to render the call_to_action_url as the reset link, ensuring HTML email clients receive a usable URL while preserving the existing translated text.openwisp_users/middleware.py (1)
72-75: 🔒 Security & Privacy | 🟠 MajorResolve against
request.path_info, notrequest.path.Django documents that
request.pathmay include the deployment script prefix whilerequest.path_infoexcludes it. Under a prefix such as/app/,resolve(request.path)can raiseResolver404; this method then returnsNone, allowing expired-password sessions to reach protected views. (docs.djangoproject.com)- resolver_match = resolve(request.path) + resolver_match = resolve(request.path_info)This is the same unresolved finding from the previous review.
🤖 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_users/middleware.py` around lines 72 - 75, Update _blocked_response to pass request.path_info to resolve instead of request.path, ensuring URL resolution works when the application is deployed under a script prefix and preserving the existing Resolver404 handling.
🤖 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_users/apps.py`:
- Around line 168-170: Update handle_allauth_login to also call
set_authentication_method(request, EXTERNAL) for passwordless login-by-code
sessions, while preserving the existing sociallogin behavior. Add a regression
test verifying code-based logins are marked EXTERNAL and expired users are not
blocked.
---
Duplicate comments:
In `@openwisp_users/middleware.py`:
- Around line 72-75: Update _blocked_response to pass request.path_info to
resolve instead of request.path, ensuring URL resolution works when the
application is deployed under a script prefix and preserving the existing
Resolver404 handling.
In `@openwisp_users/templates/custom_password_reset_email.html`:
- Around line 8-10: Update the password reset HTML template after the
instructional paragraph to render the call_to_action_url as the reset link,
ensuring HTML email clients receive a usable URL while preserving the existing
translated text.
🪄 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: 71fe4911-2b24-4066-a1a3-1ad9743a77d1
📒 Files selected for processing (19)
docs/user/rest-api.rstopenwisp_users/api/serializers.pyopenwisp_users/api/urls.pyopenwisp_users/api/views.pyopenwisp_users/apps.pyopenwisp_users/auth.pyopenwisp_users/base/forms.pyopenwisp_users/middleware.pyopenwisp_users/templates/custom_password_reset_email.htmlopenwisp_users/templates/custom_password_reset_email.txtopenwisp_users/tests/test_accounts.pyopenwisp_users/tests/test_admin.pyopenwisp_users/tests/test_api/test_password_reset.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_auth.pyopenwisp_users/tests/test_middlewares.pyrequirements.txttests/openwisp2/sample_users/tests.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.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.12 | django~=5.0.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
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
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside itFor Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_users/api/urls.pyopenwisp_users/tests/test_auth.pytests/openwisp2/settings.pyopenwisp_users/tests/test_admin.pytests/openwisp2/sample_users/tests.pyopenwisp_users/auth.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_middlewares.pyopenwisp_users/base/forms.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_api/test_password_reset.pyopenwisp_users/apps.pyopenwisp_users/tests/test_accounts.pyopenwisp_users/middleware.pyopenwisp_users/api/views.py
🪛 ast-grep (0.45.0)
openwisp_users/api/serializers.py
[warning] 21-21: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 HTMLHint (1.9.2)
openwisp_users/templates/custom_password_reset_email.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (21)
openwisp_users/auth.py (1)
1-14: LGTM!Also applies to: 17-30, 33-53
openwisp_users/apps.py (1)
3-7: LGTM!Also applies to: 18-18, 143-167
openwisp_users/tests/test_auth.py (1)
1-35: LGTM!openwisp_users/middleware.py (1)
3-70: LGTM!Also applies to: 87-108
openwisp_users/api/urls.py (1)
49-64: LGTM!openwisp_users/templates/custom_password_reset_email.txt (1)
1-8: LGTM!docs/user/rest-api.rst (1)
71-140: LGTM!requirements.txt (1)
4-4: LGTM!tests/openwisp2/settings.py (1)
39-39: LGTM!openwisp_users/api/views.py (2)
264-277: Past exemption gap now resolved.Earlier review flagged
rest_password_changeas missing fromPasswordExpirationMiddleware.exempted_url_names. The current middleware context already includesAPI_PASSWORD_CHANGE_URL_NAMEin that list (checked against bothurl_nameandview_name), andtest_expired_password_session_can_use_rest_password_changeexercises this path successfully. No further action needed here.
68-79: LGTM!Also applies to: 81-131, 340-349
openwisp_users/base/forms.py (2)
16-24: 🩺 Stability & Availability | ⚡ Quick win
.get()still raisesDoesNotExistfor an unmatched email.Previously flagged:
User.objects.get(email=email)turns a normal reset request for an unrecognized address into a 500 instead of returning no users. This is a public extension point per the docstring ("subclasses can customize"), so a caller supplying an arbitrary/unresolved email will crash here.🐛 Proposed fix
- user = User.objects.get(email=email) - return [user] if user.has_usable_password() else [] + return [ + user + for user in User.objects.filter(email=email) + if user.has_usable_password() + ]
85-109: 🗄️ Data Integrity & Integration | ⚡ Quick win
from_emailis still silently dropped.Previously flagged:
send_mailacceptsfrom_emailas a parameter but never passes it tosend_email(...).PasswordResetSerializer.save()explicitly setsopts["from_email"] = getattr(settings, "DEFAULT_FROM_EMAIL"), but that value is discarded here, so reset emails always useopenwisp_utils's default sender regardless ofDEFAULT_FROM_EMAIL.openwisp_utils admin_theme email send_email function signature from_email argumentopenwisp_users/tests/test_api/test_password_reset.py (2)
49-58: 🎯 Functional Correctness | ⚡ Quick winTautological self-comparison doesn't verify indistinguishability.
Still comparing
unknown_user_responseto itself, so this can never fail even if the unknown-identifier response differed from a known-identifier one. Compare against a response for a known identifier instead, per the previous suggestion.🧪 Proposed fix
def test_reset_request_with_unknown_identifier_is_indistinguishable(self): + known_user_response = self.client.post( + self.request_url, {"input": "tester"} + ) + mail.outbox.clear() unknown_user_response = self.client.post( self.request_url, {"input": "does-not-exist"} ) self.assertEqual(unknown_user_response.status_code, 200) self.assertEqual(len(mail.outbox), 0) self.assertEqual( - unknown_user_response.status_code, unknown_user_response.status_code + unknown_user_response.status_code, known_user_response.status_code ) - self.assertEqual(unknown_user_response.data, unknown_user_response.data) + self.assertEqual(unknown_user_response.data, known_user_response.data)
1-47: LGTM!Also applies to: 60-147
tests/openwisp2/sample_users/tests.py (1)
19-31: 📐 Maintainability & Code Quality | ⚡ Quick win
TestPasswordChangeAPIstill isn't wired into the sample app's test suite.
TestPasswordResetAPIis imported/subclassed here, butTestPasswordChangeAPIfrom the same module is not, so the sample app's customUsermodel isn't exercised against the new password-change endpoint.✅ Proposed fix
from openwisp_users.tests.test_api.test_password_reset import ( TestPasswordResetAPI as BaseTestPasswordResetAPI, + TestPasswordChangeAPI as BaseTestPasswordChangeAPI, )class TestPasswordResetAPI(BaseTestPasswordResetAPI): pass + + +class TestPasswordChangeAPI(BaseTestPasswordChangeAPI): + passdel BaseTestPasswordExpirationMiddleware del BaseTestPasswordResetAPI +del BaseTestPasswordChangeAPIAlso applies to: 150-156, 169-170
openwisp_users/tests/test_accounts.py (1)
4-16: LGTM!Also applies to: 31-73
openwisp_users/tests/test_admin.py (1)
10-10: LGTM!Also applies to: 28-28, 2460-2465
openwisp_users/tests/test_api/test_views.py (1)
1-10: LGTM!Also applies to: 25-48, 71-91
openwisp_users/tests/test_middlewares.py (1)
3-10: LGTM!Also applies to: 48-130
openwisp_users/api/serializers.py (1)
5-11: LGTM!Also applies to: 20-21, 481-528
| @classmethod | ||
| def handle_password_login(cls, request, **kwargs): | ||
| backend = getattr(kwargs.get("user"), "backend", "") | ||
| if backend == "sesame.backends.ModelBackend": |
There was a problem hiding this comment.
We need explicit handling for Sesame backend so we can except users logging in with magic links.
CI Failures: Code Style and ImportsHello @pandafy, There are multiple code style and import sorting issues detected by
To fix these issues, please run the following command in your local environment: openwisp-qa-formatThis command will automatically format your code and sort the imports according to the project's standards. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openwisp_users/base/forms.py (1)
46-51: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove the settings import to module scope.
django.conf.settingsdoes not require deferred importing here; keeping it insidesave()violates the repository’s import-placement guideline.🤖 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_users/base/forms.py` around lines 46 - 51, Move the django.conf.settings import from the save() method to module scope, alongside the other imports, and continue using settings.INSTALLED_APPS in the existing conditional without changing the token-generator selection.Source: Coding guidelines
openwisp_users/tests/test_middlewares.py (1)
121-133: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise bearer authentication alongside an expired session.
This test uses an unauthenticated client, so it never verifies the middleware behavior when an expired password session and a Bearer token are both present. Log the expired admin in before sending the token request, or explicitly document that mixed credentials are unsupported.
Suggested coverage change
- admin = self._create_admin() + admin = self._login_expired_admin() token = Token.objects.create(user=admin) - admin.password_updated = now().date() - timedelta(days=180) - admin.save()🤖 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_users/tests/test_middlewares.py` around lines 121 - 133, Update test_expired_password_session_bearer_request_not_blocked_for_existing_token to authenticate the expired admin in the client before issuing the Bearer-token request, while preserving the existing token setup and expected 200 response.openwisp_users/api/views.py (1)
68-69: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReinstate the password-expiration check before minting auth tokens.
openwisp_users/api/views.py#L68-L69:super().post()now issues a token even when the password is expired; return a403withpassword_expiredbefore delegating.openwisp_users/tests/test_api/test_views.py#L25-L36: update the regression test to expect rejection and no token.🤖 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_users/api/views.py` around lines 68 - 69, The post method in openwisp_users/api/views.py (lines 68-69) must check for an expired password and return HTTP 403 with password_expired before calling super().post(); update openwisp_users/tests/test_api/test_views.py (lines 25-36) to assert rejection and confirm no token is issued.
🤖 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_users/base/forms.py`:
- Around line 23-24: Update the user query in the password-usable user lookup to
use the case-insensitive email lookup `email__iexact` instead of exact `email`,
while preserving the existing filtering and return behavior.
In `@openwisp_users/tests/test_accounts.py`:
- Line 3: Update the account tests to use settings.SESSION_COOKIE_NAME instead
of the hardcoded “sessionid” wherever session cookies are referenced, preserving
the existing authentication and follow-up request behavior.
---
Outside diff comments:
In `@openwisp_users/api/views.py`:
- Around line 68-69: The post method in openwisp_users/api/views.py (lines
68-69) must check for an expired password and return HTTP 403 with
password_expired before calling super().post(); update
openwisp_users/tests/test_api/test_views.py (lines 25-36) to assert rejection
and confirm no token is issued.
In `@openwisp_users/base/forms.py`:
- Around line 46-51: Move the django.conf.settings import from the save() method
to module scope, alongside the other imports, and continue using
settings.INSTALLED_APPS in the existing conditional without changing the
token-generator selection.
In `@openwisp_users/tests/test_middlewares.py`:
- Around line 121-133: Update
test_expired_password_session_bearer_request_not_blocked_for_existing_token to
authenticate the expired admin in the client before issuing the Bearer-token
request, while preserving the existing token setup and expected 200 response.
🪄 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: af6efe59-259f-4421-860b-29bc0bf75593
📒 Files selected for processing (9)
openwisp_users/api/views.pyopenwisp_users/apps.pyopenwisp_users/base/forms.pyopenwisp_users/middleware.pyopenwisp_users/tests/test_accounts.pyopenwisp_users/tests/test_api/test_password_reset.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_middlewares.pytests/openwisp2/sample_users/tests.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
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
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside itFor Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
tests/openwisp2/sample_users/tests.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_accounts.pyopenwisp_users/base/forms.pyopenwisp_users/apps.pyopenwisp_users/api/views.pyopenwisp_users/tests/test_middlewares.pyopenwisp_users/tests/test_api/test_password_reset.pyopenwisp_users/middleware.py
🔇 Additional comments (12)
openwisp_users/apps.py (2)
172-174: Passwordless login-by-code is still not markedEXTERNAL(duplicate).
handle_allauth_loginonly checkssociallogin; passwordless code flows can remain classified asPASSWORDand be blocked after local-password expiration. Add coverage or disregard this only if login-by-code is disabled.
3-7: LGTM!Also applies to: 18-18, 143-170
openwisp_users/base/forms.py (3)
85-109:from_emailis still discarded (duplicate of the previous finding).
save()supplies a custom sender, butsend_mail()never forwards it tosend_email(). Preserve the sender through the supported email utility mechanism or remove the unused parameter.
99-109: 🎯 Functional CorrectnessVerify the
html_email_template_namecontract.The argument is accepted but ignored, so callers providing separate text and HTML templates will have
email_template_namerendered as HTML. Renderhtml_email_template_name or email_template_name, or remove the parameter if this behavior is intentional.
1-7: LGTM!Also applies to: 26-45, 52-84
openwisp_users/api/views.py (1)
2-9: LGTM!Also applies to: 24-25, 36-36, 72-124, 255-268, 331-340
openwisp_users/middleware.py (1)
3-30: LGTM!Also applies to: 39-46, 50-65, 67-72, 74-88, 89-102, 104-110
openwisp_users/tests/test_api/test_views.py (1)
1-10: LGTM!Also applies to: 60-80
openwisp_users/tests/test_middlewares.py (1)
1-13: LGTM!Also applies to: 51-76, 78-120, 135-172
openwisp_users/tests/test_api/test_password_reset.py (1)
1-11: LGTM!Also applies to: 14-33, 34-70, 71-81, 82-97, 98-133
tests/openwisp2/sample_users/tests.py (1)
19-21: LGTM!Also applies to: 161-162, 178-178
openwisp_users/tests/test_accounts.py (1)
4-16: LGTM!Also applies to: 32-46, 49-70, 75-90, 92-98
2b2f53c to
16feb3d
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
There was a problem hiding this comment.
CRITICAL: Password expiration check removed from ObtainAuthTokenView without replacement enforcement point
The if user.has_password_expired(): return Response(..., status=403) check was removed from ObtainAuthTokenView.post (line 69) and replaced with return super().post(...) which delegates to dj-rest-auth's serializer. The serializer has no password expiration awareness.
This creates a vulnerability for token-based (Bearer) authentication. The middleware only blocks session-authenticated requests (AUTH_SESSION_KEY in request.session), but token auth is stateless and never sets a session key. As a result, expired-password users can obtain new API tokens via the token endpoint.
The test test_obtain_auth_token_expired_password_rejected passes only because the Django test client uses session auth, hitting the middleware blocker — not because the view itself enforces the check.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -61,6 +69,59 @@ def post(self, request, *args, **kwargs): | |||
| return super().post(request, *args, **kwargs) | |||
There was a problem hiding this comment.
CRITICAL: Password expiration check removed without replacement enforcement point
The if user.has_password_expired(): return Response(..., status=403) check was removed from ObtainAuthTokenView.post and replaced with return super().post(...). The dj-rest-auth serializer has no password expiration awareness.
This creates a vulnerability for token-based (Bearer) authentication. The middleware only blocks session-authenticated requests (AUTH_SESSION_KEY in request.session), but token auth is stateless and never sets a session key. Expired-password users can obtain new API tokens via the token endpoint.
The test test_obtain_auth_token_expired_password_rejected passes only because the Django test client uses session auth, hitting the middleware blocker — not because the view itself enforces the check.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
This no longer applies. The latest issue #511 discussion explicitly dropped rejection at token issuance because downstream clients need to obtain a token to complete password recovery. Allowing issuance is intentional. The remaining bug is that an expired session cookie causes the middleware to reject this endpoint before it validates the submitted credentials.
AI-assisted (GPT-5.6 Sol / high), not manually validated.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Review Summaries (10 snapshots, latest commit 852e290)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 852e290)Status: No Issues Found | Recommendation: Merge OverviewThe incremental diff includes a single change: adding Files Reviewed (1 file)
Previous review (commit 02684ef)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit c613dce)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 6ea4272)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous review (commit f1b41cf)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous review (commit e43df9b)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous review (commit a9baca3)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 5e55d66)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 2076e43)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous review (commit 16feb3d)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (9 files)
Reviewed by step-3.7-flash · Input: 69.7K · Output: 2.4K · Cached: 254.7K |
Flake8 F401 Unused ImportsHello @pandafy, The CI failed because of unused imports detected by Flake8.
Fix: Remove these unused imports from the specified files. Test Failure:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_users/tests/test_api/test_views.py (1)
24-35: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore rejection of token issuance for expired passwords.
This test now codifies the opposite of the PR requirement: an expired password must not obtain a REST token. Assert the structured
403/password_expiredresponse and verify that no token is returned.Proposed test correction
- def test_obtain_auth_token_expired_password_success(self): + def test_obtain_auth_token_expired_password_rejected(self): self._create_user( username="tester", password="tester", password_updated=now().date() - timedelta(days=180), ) params = {"username": "tester", "password": "tester"} url = reverse("users:user_auth_token") response = self.client.post(url, params) - self.assertEqual(response.status_code, 200) - self.assertIn("token", response.data) + self.assertEqual(response.status_code, 403) + self.assertEqual(response.data["code"], "password_expired") + self.assertNotIn("token", response.data)🤖 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_users/tests/test_api/test_views.py` around lines 24 - 35, Update test_obtain_auth_token_expired_password_success to assert the expired-password request is rejected with HTTP 403 and the structured password_expired response, and verify the response does not contain a token. Rename the test to reflect rejection rather than successful token issuance.openwisp_users/tests/test_api/test_password_reset.py (1)
74-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompare unknown and known reset responses.
This test only checks a fixed response for the unknown identifier; it never proves that the response matches a real account request. This repeats the previous-review finding. Create a known user, capture its response, clear the outbox, then compare both status and response data.
🤖 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_users/tests/test_api/test_password_reset.py` around lines 74 - 83, Create a known user within test_reset_request_with_unknown_identifier_is_indistinguishable, submit a password-reset request for that user, and capture its status and response data. Clear mail.outbox afterward, submit the unknown-identifier request, and assert that both status code and response data match the known request while confirming no email is sent for the unknown identifier.
♻️ Duplicate comments (1)
openwisp_users/base/forms.py (1)
80-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not silently discard
from_email.
save(..., from_email=...)is still accepted through**kwargsbut is never forwarded, so callers lose their requested sender without an error. This repeats the unresolved previous-review finding; preserve the sender contract or reject/handle the argument explicitly.🤖 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_users/base/forms.py` around lines 80 - 94, Update the password-reset form’s save/send_mail flow so an explicitly supplied from_email is not silently ignored: either forward it through the email-sending path and preserve the requested sender, or explicitly reject/handle the argument with a clear error. Use the visible send_mail method and its caller’s **kwargs handling as the change points.
🤖 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 `@docs/user/account-and-password-expiration.rst`:
- Around line 88-102: Update the password-expiration documentation and JSON
example to state that api_password_change_url and api_password_reset_url are
included only when the authentication API is enabled and their corresponding
routes are available; show these fields as conditional rather than universally
present.
In `@docs/user/rest-api.rst`:
- Around line 124-139: The self-service password-change documentation lists
unsupported PUT and PATCH methods. Update the endpoint description and curl
example for the self-service password-change endpoint to use POST, matching its
implemented handler and existing tests.
---
Outside diff comments:
In `@openwisp_users/tests/test_api/test_password_reset.py`:
- Around line 74-83: Create a known user within
test_reset_request_with_unknown_identifier_is_indistinguishable, submit a
password-reset request for that user, and capture its status and response data.
Clear mail.outbox afterward, submit the unknown-identifier request, and assert
that both status code and response data match the known request while confirming
no email is sent for the unknown identifier.
In `@openwisp_users/tests/test_api/test_views.py`:
- Around line 24-35: Update test_obtain_auth_token_expired_password_success to
assert the expired-password request is rejected with HTTP 403 and the structured
password_expired response, and verify the response does not contain a token.
Rename the test to reflect rejection rather than successful token issuance.
---
Duplicate comments:
In `@openwisp_users/base/forms.py`:
- Around line 80-94: Update the password-reset form’s save/send_mail flow so an
explicitly supplied from_email is not silently ignored: either forward it
through the email-sending path and preserve the requested sender, or explicitly
reject/handle the argument with a clear error. Use the visible send_mail method
and its caller’s **kwargs handling as the change points.
🪄 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: b34bab24-f900-417b-bf03-9881d40d6558
📒 Files selected for processing (15)
docs/developer/misc-utils.rstdocs/user/account-and-password-expiration.rstdocs/user/rest-api.rstdocs/user/settings.rstopenwisp_users/api/serializers.pyopenwisp_users/api/views.pyopenwisp_users/auth.pyopenwisp_users/backends.pyopenwisp_users/base/forms.pyopenwisp_users/tests/test_api/test_password_reset.pyopenwisp_users/tests/test_api/test_throttling.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_auth.pyopenwisp_users/tests/test_middlewares.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (1)
- openwisp_users/api/serializers.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
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
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside itFor Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_users/backends.pyopenwisp_users/auth.pyopenwisp_users/tests/test_api/test_throttling.pytests/openwisp2/settings.pyopenwisp_users/base/forms.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_auth.pyopenwisp_users/tests/test_middlewares.pyopenwisp_users/api/views.pyopenwisp_users/tests/test_api/test_password_reset.py
🔇 Additional comments (17)
openwisp_users/auth.py (1)
1-16: LGTM!Also applies to: 17-31, 33-64
openwisp_users/backends.py (1)
24-24: LGTM!openwisp_users/tests/test_auth.py (1)
1-11: LGTM!Also applies to: 14-23, 24-34
openwisp_users/tests/test_middlewares.py (1)
1-13: LGTM!Also applies to: 51-53, 58-76, 78-103, 105-120, 121-134, 136-145, 147-172, 174-183, 184-194
openwisp_users/tests/test_api/test_views.py (1)
1-9: LGTM!Also applies to: 63-79
openwisp_users/tests/test_api/test_throttling.py (1)
13-16: LGTM!docs/developer/misc-utils.rst (2)
166-185: LGTM!
198-207: 🎯 Functional CorrectnessBear in mind the Bearer exception may not hold for mixed auth requests. If this middleware runs before DRF authentication and only checks the session, a request with an expired session cookie plus a valid
Bearertoken can still be rejected with403. Narrow the doc to token-only requests or add an explicit Bearer/session bypass.tests/openwisp2/settings.py (1)
39-39: LGTM!Also applies to: 115-119
openwisp_users/api/views.py (3)
70-77: 🗄️ Data Integrity & IntegrationVerify the custom reset serializer is configured in production.
This view no longer declares
serializer_class, while the suppliedREST_AUTH["PASSWORD_RESET_SERIALIZER"]override exists only intests/openwisp2/settings.py. The dj-rest-auth base view resolves its serializer from that setting; without the production equivalent, the endpoint can fall back to a serializer that does not support the documentedinputidentifier. (raw.githubusercontent.com) Restore the explicit binding or ship/document a package-level default and test without the test-only override.
24-24: LGTM!Also applies to: 95-121, 252-262, 325-334
2-7: 🩺 Stability & AvailabilityMake optional allauth support consistent across both modules.
Both modules import
allauth.account.utilsunconditionally, so deployments without allauth can fail during module import before the non-allauth fallback is reached.
openwisp_users/api/views.py#L2-L7: guarduser_pk_to_url_stror make allauth mandatory.openwisp_users/base/forms.py#L1-L1: apply the same optional-dependency strategy.openwisp_users/base/forms.py (2)
12-26: LGTM!Also applies to: 57-78
54-56: 🎯 Functional CorrectnessNo change needed here. The only in-repo password-reset caller passes a request, so this fallback doesn’t affect the shipped reset flow.
> Likely an incorrect or invalid review comment.openwisp_users/tests/test_api/test_password_reset.py (1)
3-27: LGTM!Also applies to: 29-72, 85-136, 137-176, 179-212
docs/user/rest-api.rst (1)
71-113: LGTM!docs/user/settings.rst (1)
76-78: LGTM!
5e55d66 to
a9baca3
Compare
ReStructuredText Formatting FailureHello @pandafy, The CI failed because of a ReStructuredText formatting issue in the documentation. Failure:
Fix: Please reformat the file |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
AI-assisted review (GPT-5.6 Sol / high). The findings have not been manually validated.
I left the outstanding findings inline. The CodeRabbit observation about the test-only password-reset serializer is correct and remains important. The Kilo and CodeRabbit requests to reject token issuance for expired passwords are stale because the latest issue #511 discussion explicitly dropped that requirement. The login-by-code suggestion is too broad for this PR because Sesame magic links are the supported passwordless path here and are explicitly covered. The HTML CTA comment was correctly withdrawn, but the CTA target still returns 405. Other automated comments addressed by the current head are not repeated.
| return super().post(request, *args, **kwargs) | ||
|
|
||
|
|
||
| class PasswordResetView(BasePasswordResetView): |
There was a problem hiding this comment.
High: This endpoint only uses PasswordResetSerializer because the test project overrides REST_AUTH["PASSWORD_RESET_SERIALIZER"]. A default installation gets dj-rest-auth's serializer, accepts only email, and bypasses the documented username and phone lookup. I would set serializer_class = PasswordResetSerializer directly on this view and test it without the test-only override.
There was a problem hiding this comment.
@nemesifier this is how it's done in openwisp-radius right now.
And, this is something which will be configured by either ansible-openwisp2 and docker-openwisp
| ) | ||
|
|
||
|
|
||
| class PasswordChangeView(BasePasswordChangeView): |
There was a problem hiding this comment.
High: The old password is not always verified as the docstring claims. dj-rest-auth defaults OLD_PASSWORD_FIELD_ENABLED to False, removes old_password, and allows an authenticated session or Bearer token to replace the password. The tests hide this with a test-only setting. I would use an OpenWISP-owned serializer which always requires and verifies old_password.
| if method is not None: | ||
| return method | ||
| user = user if user is not None else getattr(request, "user", None) | ||
| return getattr(user, "last_login_method", "") or PASSWORD |
There was a problem hiding this comment.
High: An unmarked session inherits the user's global last_login_method. An external login in another browser can therefore make an older password-authenticated session bypass expiration, while a later password login can misclassify an external session. When a request has no session marker, I would default that request to PASSWORD and reserve the persisted fallback for callers with no request. Please cover two independent sessions.
| touching token generation or email sending. | ||
| """ | ||
| uid = user_pk_to_url_str(user) | ||
| confirm_path = reverse("users:rest_password_reset_confirm") |
There was a problem hiding this comment.
High: The email tells the user to click a reset button, but this target only accepts POST. Clicking the generated URL performs GET and returns 405, so the default recovery flow is not usable. Please point the CTA to a GET-capable password selection page, or provide one, and make the test follow the emailed URL.
| "account_reset_password_done", | ||
| "account_reset_password_from_key", | ||
| "account_reset_password_from_key_done", | ||
| "change_password", |
There was a problem hiding this comment.
High: I do not think this endpoint can be exempted. It lets a superuser or organization manager change another user's password without presenting their own old password, so an expired privileged session can continue account takeover operations. Please remove this exemption and keep only the self-service recovery route exempt.
| local login. | ||
| """ | ||
|
|
||
| throttle_classes = [AuthRateThrottle] |
There was a problem hiding this comment.
Medium: AuthRateThrottle subclasses AnonRateThrottle, which returns no cache key for authenticated requests. Any logged-in user can bypass this limit and send unlimited reset emails. The same ineffective throttle is used by confirm and password change. Please use throttles that cover authenticated requests and add authenticated-request coverage.
|
|
||
| def __call__(self, request): | ||
| session_authenticated_before = AUTH_SESSION_KEY in request.session | ||
| if session_authenticated_before and self._is_expired_password_session(request): |
There was a problem hiding this comment.
Medium: This check runs before DRF authenticates an accompanying Bearer token. A request with an expired session cookie and a valid existing key is rejected even though protected API views prioritize BearerAuthentication. The current test has no session and misses this case. Please cover mixed credentials and ensure only a successfully authenticated Bearer request bypasses the session restriction.
There was a problem hiding this comment.
@nemesifier I don't think this is a real concern.
| API_PASSWORD_CHANGE_URL_NAME, | ||
| "users:rest_password_reset", | ||
| "users:rest_password_reset_confirm", | ||
| ] |
There was a problem hiding this comment.
Medium: The final issue discussion intentionally allows token issuance with an expired password, but an expired session cookie still blocks users:user_auth_token here before submitted credentials reach the view. I would exempt that route and add a regression test where the client already has an expired session.
There was a problem hiding this comment.
@nemesifier, I think the current implementation is sane. If we can block token issuance with an expired password we should block it.
In openwisp-radius, it is more of a problem because the downstream application (WiFi Login Pages) will use the Bearer Token to change the password and it wouldn't be able to do so without obtaining token.
However, the approach is contradicting and make the maintenance harder.
A user that has once gained the Token can perform all operations with the REST API even with an expired password.
| {"token": "7a2e1d3d008253c123c61d56741003db5a194256"} | ||
|
|
||
| .. _authenticating_rest_api: | ||
| .. _rest_password_reset: |
There was a problem hiding this comment.
Medium: This replaced the existing authenticating_rest_api anchor and Bearer-token usage section instead of adding the recovery documentation after it. docs/developer/django-rest-framework-utils.rst still references the removed anchor. Please restore that section and keep the new material alongside it.
|
|
||
| .. code-block:: text | ||
|
|
||
| PUT /api/v1/users/user/password/change/ |
There was a problem hiding this comment.
Low: This summary still advertises PUT and PATCH, but the view only implements POST. Please make it match the corrected detailed section.
[
{"line": 2026, "col": 0, "message": "Imports are incorrectly sorted and/or formatted."},
{"line": 2026, "col": 0, "message": "Isort check failed! Hint: did you forget to run openwisp-qa-format?"}
]CI Failure AnalysisHello @pandafy, The CI has failed due to an import sorting and formatting issue in the Failure: Code Style/QA (Isort) Fix: |
f1b41cf to
6ea4272
Compare
Instead of storing which method the user has logged in with, we now only store if the user has logged in with password.
|
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). |
|
@coderabbitai full review! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_users/middleware.py`:
- Around line 39-43: Ensure expired-password users cannot obtain REST
authentication tokens: either remove users:user_auth_token from the
password-change exemption list or add a has_password_expired() check in
ObtainAuthTokenView.post before token creation. Update the related success test
to cover the expired-password rejection while preserving normal token issuance
for valid credentials.
In `@openwisp_users/tests/test_api/test_views.py`:
- Around line 25-36: Update test_obtain_auth_token_expired_password_success to
expect rejection for the expired password instead of a successful token
response: assert the documented non-200 status and verify response.data contains
the password-recovery link, removing the token assertion.
In `@openwisp_users/tests/test_middlewares.py`:
- Around line 135-146: Update
test_expired_password_session_can_obtain_auth_token and the underlying
users:user_auth_token flow so password-authenticated users with expired
passwords cannot receive new tokens: expect HTTP 403 and a response code of
"password_expired". Preserve validity for existing Bearer tokens while enforcing
the expiration response at token issuance.
In `@tests/openwisp2/sample_users/migrations/0001_initial.py`:
- Around line 36-52: The sample_users migration history must remain unchanged:
restore last_login_method in 0001_initial.py and add a new append-only migration
after 0006_alter_organizationuser_is_admin.py that transitions the schema to
password_based_token, preserving compatibility for databases with
already-applied migrations.
🪄 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: 3aaebcbc-20db-4981-9a52-6f6a403c607d
📒 Files selected for processing (30)
docs/developer/misc-utils.rstdocs/user/account-and-password-expiration.rstdocs/user/rest-api.rstdocs/user/settings.rstopenwisp_users/admin.pyopenwisp_users/api/serializers.pyopenwisp_users/api/urls.pyopenwisp_users/api/views.pyopenwisp_users/apps.pyopenwisp_users/auth.pyopenwisp_users/backends.pyopenwisp_users/base/forms.pyopenwisp_users/base/models.pyopenwisp_users/middleware.pyopenwisp_users/migrations/0026_user_password_based_token.pyopenwisp_users/settings.pyopenwisp_users/templates/custom_password_reset_email.htmlopenwisp_users/templates/custom_password_reset_email.txtopenwisp_users/tests/test_accounts.pyopenwisp_users/tests/test_admin.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_api/test_password_reset.pyopenwisp_users/tests/test_api/test_throttling.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/tests/test_auth.pyopenwisp_users/tests/test_middlewares.pyrequirements.txttests/openwisp2/sample_users/migrations/0001_initial.pytests/openwisp2/sample_users/tests.pytests/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
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
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside itFor Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_users/migrations/0026_user_password_based_token.pyopenwisp_users/tests/test_api/test_throttling.pyopenwisp_users/admin.pyopenwisp_users/backends.pyopenwisp_users/base/models.pytests/openwisp2/sample_users/migrations/0001_initial.pyopenwisp_users/base/forms.pyopenwisp_users/tests/test_auth.pyopenwisp_users/api/urls.pyopenwisp_users/tests/test_api/test_views.pyopenwisp_users/apps.pyopenwisp_users/auth.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_accounts.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_middlewares.pytests/openwisp2/settings.pyopenwisp_users/middleware.pyopenwisp_users/tests/test_admin.pyopenwisp_users/api/views.pyopenwisp_users/tests/test_api/test_password_reset.pytests/openwisp2/sample_users/tests.pyopenwisp_users/settings.py
🧠 Learnings (2)
📚 Learning: 2026-06-19T00:08:14.892Z
Learnt from: nemesifier
Repo: openwisp/openwisp-users PR: 530
File: openwisp_users/migrations/0023_add_auth_token_permissions_to_admins.py:9-12
Timestamp: 2026-06-19T00:08:14.892Z
Learning: When working on migrations in openwisp_users that grant permissions relying on Django REST Framework’s authtoken app, treat `rest_framework.authtoken` as a required dependency: keep explicit migration dependencies (e.g., depend on `('authtoken', '0004_alter_tokenproxy_options')`) so the permission-grant migration runs only after the relevant authtoken migration has been applied. Any `apps.is_installed('rest_framework.authtoken')` checks elsewhere should be reviewed as defensive guards only (to avoid crashes in unusual environments), not as evidence that authtoken is optional.
Applied to files:
openwisp_users/migrations/0026_user_password_based_token.py
📚 Learning: 2026-06-19T00:08:15.922Z
Learnt from: nemesifier
Repo: openwisp/openwisp-users PR: 530
File: tests/openwisp2/sample_users/migrations/0004_default_groups.py:19-22
Timestamp: 2026-06-19T00:08:15.922Z
Learning: When reviewing Django migrations in these openwisp-users migration directories, treat `rest_framework.authtoken` as a required dependency (not optional). If a migration declares a hard `dependencies` entry on `("authtoken", "0004_alter_tokenproxy_options")`, consider it intentional to guarantee correct migration ordering—do not suggest making the dependency conditional or optional. Likewise, if corresponding code uses `apps.is_installed("rest_framework.authtoken")` in models/admin, treat that as defensive guarding only (not as support for running without `authtoken`).
Applied to files:
openwisp_users/migrations/0026_user_password_based_token.pytests/openwisp2/sample_users/migrations/0001_initial.py
🪛 ast-grep (0.45.0)
openwisp_users/api/serializers.py
[warning] 23-23: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 HTMLHint (1.9.2)
openwisp_users/templates/custom_password_reset_email.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (33)
openwisp_users/admin.py (1)
228-228: LGTM!openwisp_users/tests/test_api/test_views.py (3)
1-10: LGTM!
38-47: LGTM!
71-91: LGTM!openwisp_users/auth.py (1)
1-90: LGTM!openwisp_users/apps.py (1)
3-18: LGTM!Also applies to: 143-176
openwisp_users/tests/test_admin.py (1)
10-28: LGTM!Also applies to: 182-182, 393-393, 429-429, 454-454, 498-498, 847-847, 891-891, 923-923, 951-951, 1019-1019, 2061-2061, 2152-2152, 2192-2192, 2218-2218, 2474-2479
openwisp_users/tests/test_auth.py (1)
1-104: LGTM!openwisp_users/tests/test_middlewares.py (1)
1-133: LGTM!Also applies to: 148-209
openwisp_users/settings.py (1)
62-73: LGTM!openwisp_users/templates/custom_password_reset_email.txt (1)
1-8: LGTM!docs/user/rest-api.rst (1)
92-160: LGTM!Also applies to: 177-197
docs/user/settings.rst (1)
76-78: LGTM!openwisp_users/base/models.py (1)
79-94: LGTM!openwisp_users/migrations/0026_user_password_based_token.py (1)
1-31: LGTM!openwisp_users/tests/test_accounts.py (1)
4-19: LGTM!Also applies to: 34-99
openwisp_users/middleware.py (1)
3-30: LGTM!Also applies to: 44-110
docs/developer/misc-utils.rst (1)
166-235: LGTM!Also applies to: 248-264
docs/user/account-and-password-expiration.rst (1)
77-104: LGTM!openwisp_users/api/urls.py (1)
49-70: LGTM!tests/openwisp2/sample_users/tests.py (1)
19-34: LGTM!Also applies to: 153-164, 176-178
openwisp_users/api/views.py (2)
86-96:serializer_classstill relies on globalREST_AUTHsettings, not an explicit binding.Neither
PasswordResetViewnorPasswordChangeViewsetsserializer_classexplicitly. Both endpoints only pick upPasswordResetSerializer/PasswordChangeSerializerbecause the test project configuresREST_AUTH["PASSWORD_RESET_SERIALIZER"]/REST_AUTH["PASSWORD_CHANGE_SERIALIZER"]. A default installation without that override falls back to dj-rest-auth's stock serializers: an email-only reset form, and a password-change form whereold_passwordis unverified by default (OLD_PASSWORD_FIELD_ENABLEDdefaults toFalse).A maintainer previously acknowledged this pattern (matching openwisp-radius) and stated it is meant to be configured downstream by ansible-openwisp2/docker-openwisp. Re-flagging since the underlying code is unchanged from the earlier review; downstream projects that don't apply that configuration silently lose the multi-identifier reset and old-password verification.
Also applies to: 268-275
93-93:AuthRateThrottledoes not throttle authenticated requests on these endpoints.
AuthRateThrottlesubclassesAnonRateThrottle, whoseget_cache_keyreturnsNonefor authenticated requests.PasswordChangeView(line 273) only accepts authenticated requests, so its throttle is inert for every request. The same gap applies toPasswordResetView(line 93) andPasswordResetConfirmView(line 121) when hit by a logged-in user, letting any authenticated account send unlimited reset emails or brute-force confirm tokens without rate limiting.Re-flagging a previously raised, still-unresolved concern.
Also applies to: 121-121, 273-273
openwisp_users/backends.py (1)
24-24: LGTM!requirements.txt (1)
4-4: LGTM!tests/openwisp2/settings.py (1)
39-39: LGTM!openwisp_users/api/serializers.py (2)
483-528: LGTM!
531-544: LGTM!openwisp_users/base/forms.py (1)
1-109: LGTM!openwisp_users/templates/custom_password_reset_email.html (1)
1-11: LGTM!openwisp_users/tests/test_api/test_password_reset.py (1)
1-241: LGTM!openwisp_users/tests/test_api/test_throttling.py (1)
13-16: LGTM!openwisp_users/tests/test_api/test_api.py (1)
439-461: 🗄️ Data Integrity & IntegrationNo target-user check is needed in this route.
PasswordExpirationMiddlewareexemptsusers:rest_password_change, notusers:change_password; this test remains subject to expiration blocking.> Likely an incorrect or invalid review comment.
| API_PASSWORD_CHANGE_URL_NAME, | ||
| "users:rest_password_reset", | ||
| "users:rest_password_reset_confirm", | ||
| "users:user_auth_token", | ||
| ] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline openwisp_users/api/views.py --items all
rg -n -C 12 \
'def obtain_auth_token\b|has_password_expired|password_expired|record_password_based_token' \
openwisp_users/api/views.py openwisp_users/tests/test_apiRepository: openwisp/openwisp-users
Length of output: 14613
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- openwisp_users/api/views.py ---'
sed -n '51,85p' openwisp_users/api/views.py
printf '%s\n' '--- openwisp_users/tests/test_api/test_views.py ---'
sed -n '14,50p' openwisp_users/tests/test_api/test_views.py
printf '%s\n' '--- expiration-related implementations and tests ---'
rg -n -C 10 \
'password_updated|password_expired|password_expiration|PASSWORD_EXPIRATION|record_password_based_token|authenticate\(' \
openwisp_users --glob '*.py'Repository: openwisp/openwisp-users
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
view = Path("openwisp_users/api/views.py").read_text()
tests = Path("openwisp_users/tests/test_api/test_views.py").read_text()
post = re.search(
r"class ObtainAuthTokenView\b.*?\n(?=class |\Z)", view, re.S
)
assert post, "ObtainAuthTokenView was not found"
post_text = post.group(0)
expired_test = re.search(
r"def test_obtain_auth_token_expired_password_success\(self\):.*?(?=\n def |\Z)",
tests,
re.S,
)
assert expired_test, "Expired-password token test was not found"
print("ObtainAuthTokenView.post calls serializer.is_valid before Token.objects.get_or_create.")
print("ObtainAuthTokenView.post checks has_password_expired:", "has_password_expired" in post_text)
print("Expired-password test expects status 200:", "assertEqual(response.status_code, 200)" in expired_test.group(0))
print("Expired-password test expects a token:", '"token" in response.data' in expired_test.group(0))
PYRepository: openwisp/openwisp-users
Length of output: 400
Reject expired passwords before issuing REST tokens. ObtainAuthTokenView.post has no has_password_expired() check and creates a token after credential validation. Remove users:user_auth_token from the exemption or add the check in the view, and update the success test.
🤖 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_users/middleware.py` around lines 39 - 43, Ensure expired-password
users cannot obtain REST authentication tokens: either remove
users:user_auth_token from the password-change exemption list or add a
has_password_expired() check in ObtainAuthTokenView.post before token creation.
Update the related success test to cover the expired-password rejection while
preserving normal token issuance for valid credentials.
| @patch.object(app_settings, "USER_PASSWORD_EXPIRATION", 10) | ||
| def test_obtain_auth_token_expired_password_success(self): | ||
| self._create_user( | ||
| username="tester", | ||
| password="tester", | ||
| password_updated=now().date() - timedelta(days=180), | ||
| ) | ||
| params = {"username": "tester", "password": "tester"} | ||
| url = reverse("users:user_auth_token") | ||
| response = self.client.post(url, params) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertIn("token", response.data) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert rejection for an expired local-password token request.
This test backdates password_updated by 180 days while USER_PASSWORD_EXPIRATION is 10 days. It then submits username/password credentials and requires a 200 response with a token. The PR objective requires REST token issuance to be rejected when the password is expired. Change this test to assert the documented rejection response and recovery link.
🤖 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_users/tests/test_api/test_views.py` around lines 25 - 36, Update
test_obtain_auth_token_expired_password_success to expect rejection for the
expired password instead of a successful token response: assert the documented
non-200 status and verify response.data contains the password-recovery link,
removing the token assertion.
| @patch.object(app_settings, "STAFF_USER_PASSWORD_EXPIRATION", 10) | ||
| def test_expired_password_session_can_obtain_auth_token(self): | ||
| # the final #511 decision allows token issuance even with an | ||
| # expired password, so a browser holding an expired-password | ||
| # session must still be able to reach the token endpoint. | ||
| self._login_expired_admin() | ||
| response = self.client.post( | ||
| reverse("users:user_auth_token"), | ||
| data={"username": "admin", "password": "tester"}, | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertIn("token", response.json()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Reject token issuance for an expired password.
This test accepts a new token after password expiration. A newly issued Bearer token bypasses the expiration restriction for a password-authenticated user.
Make the token endpoint return the password-expired response. Update this test to expect HTTP 403 and code == "password_expired". Existing Bearer tokens can remain valid.
🤖 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_users/tests/test_middlewares.py` around lines 135 - 146, Update
test_expired_password_session_can_obtain_auth_token and the underlying
users:user_auth_token flow so password-authenticated users with expired
passwords cannot receive new tokens: expect HTTP 403 and a response code of
"password_expired". Preserve validity for existing Bearer tokens while enforcing
the expiration response at token issuance.
| ( | ||
| "password_based_token", | ||
| models.BooleanField( | ||
| blank=True, | ||
| default=None, | ||
| help_text=( | ||
| "Indicates whether the last authentication token was" | ||
| " obtained using the local password. When false, the" | ||
| " token came from an external method (eg: SSO, SAML)" | ||
| " and password expiration is not enforced for it. None" | ||
| " means no token has been issued for this user since" | ||
| " this feature was introduced." | ||
| ), | ||
| null=True, | ||
| verbose_name="password based token", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f . tests/openwisp2/sample_users/migrations
rg -n -C 4 'password_based_token|dependencies\s*=' tests/openwisp2/sample_users/migrationsRepository: openwisp/openwisp-users
Length of output: 4473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration files ---'
git ls-files tests/openwisp2/sample_users/migrations
printf '%s\n' '--- targeted diff ---'
git diff -- tests/openwisp2/sample_users/migrations/0001_initial.py tests/openwisp2/sample_users/migrations
printf '%s\n' '--- field definitions and migration references ---'
rg -n -C 5 'password_based_token|class Migration|dependencies|operations' tests openwisp_users openwisp_users 2>/dev/null || true
printf '%s\n' '--- latest sample_users migration dependencies ---'
for f in tests/openwisp2/sample_users/migrations/*.py; do
echo "### $f"
sed -n '1,80p' "$f"
doneRepository: openwisp/openwisp-users
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository state ---'
git status --short
git log -2 --oneline -- tests/openwisp2/sample_users/migrations/0001_initial.py
printf '%s\n' '--- current migration field ---'
sed -n '18,65p' tests/openwisp2/sample_users/migrations/0001_initial.py
printf '%s\n' '--- parent migration field ---'
if git rev-parse --verify HEAD^ >/dev/null 2>&1; then
git show HEAD^:tests/openwisp2/sample_users/migrations/0001_initial.py 2>/dev/null |
sed -n '18,65p' || true
fi
printf '%s\n' '--- sample_users migration graph ---'
for f in tests/openwisp2/sample_users/migrations/*.py; do
printf '### %s\n' "$f"
rg -n '^(class Migration| dependencies| \("sample_users"| operations| migrations\.(AddField|CreateModel|AlterField)| name=)' "$f" || true
doneRepository: openwisp/openwisp-users
Length of output: 5479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for rev in e43df9b 02684ef; do
printf '%s\n' "--- $rev summary ---"
git show --stat --oneline --no-renames "$rev"
printf '%s\n' "--- $rev affected paths ---"
git diff-tree --no-commit-id --name-status -r "$rev"
printf '%s\n' "--- $rev migration diff ---"
git show --format= --no-ext-diff -- tests/openwisp2/sample_users/migrations/0001_initial.py "$rev" 2>/dev/null || true
done
printf '%s\n' '--- migration history for sample_users ---'
git log --all --oneline --follow -- tests/openwisp2/sample_users/migrations/0001_initial.pyRepository: openwisp/openwisp-users
Length of output: 4434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for rev in e43df9b 02684ef; do
printf '%s\n' "--- $rev sample migration diff ---"
git show --format= --no-ext-diff "$rev" -- tests/openwisp2/sample_users/migrations/0001_initial.py
printf '%s\n' "--- $rev user migration diff ---"
git show --format= --no-ext-diff "$rev" -- openwisp_users/migrations
done
printf '%s\n' '--- sample_users usage and migration configuration ---'
rg -n -C 3 'sample_users|MIGRATION|MIGRATIONS|migrate|migration' tests/openwisp2 tests -g '*.py' -g '*.ini' -g '*.cfg' -g '*.toml' -g '*.yml' -g '*.yaml' | head -n 300Repository: openwisp/openwisp-users
Length of output: 32564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
import pathlib
import subprocess
root = pathlib.Path("tests/openwisp2/sample_users/migrations")
def text_at(revision, path):
if revision == "WORKTREE":
return pathlib.Path(path).read_text()
return subprocess.check_output(["git", "show", f"{revision}:{path}"], text=True)
def user_fields(source):
tree = ast.parse(source)
for node in ast.walk(tree):
if isinstance(node, ast.Call) and getattr(node.func, "attr", None) == "CreateModel":
for kw in node.keywords:
if kw.arg == "name" and isinstance(kw.value, ast.Constant) and kw.value.value == "User":
fields = next(k.value for k in node.keywords if k.arg == "fields")
return [item.elts[0].value for item in fields if isinstance(item, ast.Tuple)]
raise RuntimeError("User CreateModel not found")
before = user_fields(text_at("e43df9b", "tests/openwisp2/sample_users/migrations/0001_initial.py"))
after = user_fields(text_at("WORKTREE", "tests/openwisp2/sample_users/migrations/0001_initial.py"))
migration_names = sorted(p.stem for p in root.glob("[0-9][0-9][0-9][0-9]_*.py"))
print("0001 at e43df9b:", {name: name in before for name in ("last_login_method", "password_based_token")})
print("0001 at current:", {name: name in after for name in ("last_login_method", "password_based_token")})
print("sample_users migrations:", migration_names)
print("post-0001 migration contains password_based_token:",
any("password_based_token" in p.read_text() for p in root.glob("[0-9][0-9][0-9][0-9]_*.py") if p.name != "0001_initial.py"))
PYRepository: openwisp/openwisp-users
Length of output: 318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
import pathlib
import subprocess
root = pathlib.Path("tests/openwisp2/sample_users/migrations")
def text_at(revision, path):
if revision == "WORKTREE":
return pathlib.Path(path).read_text()
return subprocess.check_output(["git", "show", f"{revision}:{path}"], text=True)
def user_fields(source):
tree = ast.parse(source)
for node in ast.walk(tree):
if not (isinstance(node, ast.Call) and getattr(node.func, "attr", None) == "CreateModel"):
continue
name_kw = next((k for k in node.keywords if k.arg == "name"), None)
fields_kw = next((k for k in node.keywords if k.arg == "fields"), None)
if (
name_kw
and isinstance(name_kw.value, ast.Constant)
and name_kw.value.value == "User"
and fields_kw
and isinstance(fields_kw.value, ast.List)
):
return [
item.elts[0].value
for item in fields_kw.value.elts
if isinstance(item, ast.Tuple)
and item.elts
and isinstance(item.elts[0], ast.Constant)
]
raise RuntimeError("User CreateModel not found")
before = user_fields(text_at("e43df9b", "tests/openwisp2/sample_users/migrations/0001_initial.py"))
after = user_fields(text_at("WORKTREE", "tests/openwisp2/sample_users/migrations/0001_initial.py"))
migration_names = sorted(p.stem for p in root.glob("[0-9][0-9][0-9][0-9]_*.py"))
print("0001 at e43df9b:", {name: name in before for name in ("last_login_method", "password_based_token")})
print("0001 at current:", {name: name in after for name in ("last_login_method", "password_based_token")})
print("sample_users migrations:", migration_names)
print(
"post-0001 migration contains password_based_token:",
any(
"password_based_token" in p.read_text()
for p in root.glob("[0-9][0-9][0-9][0-9]_*.py")
if p.name != "0001_initial.py"
),
)
PYRepository: openwisp/openwisp-users
Length of output: 490
Preserve the sample_users migration history.
0001_initial.py replaces last_login_method with password_based_token. Django does not rerun an applied migration, so existing databases can lack password_based_token. Keep 0001_initial.py unchanged and add an append-only migration after 0006_alter_organizationuser_is_admin.py to transition the field.
🤖 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/sample_users/migrations/0001_initial.py` around lines 36 -
52, The sample_users migration history must remain unchanged: restore
last_login_method in 0001_initial.py and add a new append-only migration after
0006_alter_organizationuser_is_admin.py that transitions the schema to
password_based_token, preserving compatibility for databases with
already-applied migrations.
2 participants
Checklist
Reference to Existing Issue
Fixes #511
Description of Changes
Added REST API endpoints to allow users to reset and change their password.