[fix] Use UUID converters for users API detail routes #487#492
[fix] Use UUID converters for users API detail routes #487#492czarflix wants to merge 2 commits intoopenwisp:masterfrom
Conversation
User and organization API detail routes currently use string path converters even though the underlying resources are UUID-backed. This allows malformed IDs to resolve to DRF views and return 401 instead of being rejected at the routing boundary with 404. Update the five affected routes to use uuid converters and add a regression test covering malformed UUID paths. Closes openwisp#487
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates several API URL patterns to use UUID path converters ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_users/api/urls.py`:
- Around line 29-32: ChangePasswordView.get_permissions currently compares
str(self.request.user.id) to self.kwargs["pk"] which is a uuid.UUID (because the
URL uses "<uuid:pk>") causing the check to always fail; update the comparison in
ChangePasswordView.get_permissions to compare the UUID objects directly (i.e.,
remove the str() conversion so it compares self.request.user.id to
self.kwargs["pk"]) so the self-check correctly allows IsAuthenticated for a user
changing their own password.
🪄 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: 5c961879-2f6c-43b0-a6c8-3146e978c126
📒 Files selected for processing (2)
openwisp_users/api/urls.pyopenwisp_users/tests/test_api/test_views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- 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.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (2)
openwisp_users/api/urls.py (1)
18-22: URL converter changes align with model definitions.The changes correctly enforce UUID validation at the routing level, matching the
UUIDFieldprimary key definition in the User model (openwisp_users/base/models.py:52). Django's ORM handles UUID objects properly in filter queries (e.g.,User.objects.filter(pk=self.kwargs["pk"])), so database lookups will work correctly.Also applies to: 28-28, 34-42
openwisp_users/tests/test_api/test_views.py (1)
28-40: Good regression test coverage for UUID routing validation.The test correctly validates that malformed UUIDs are rejected at the routing level with 404. Using hardcoded paths is appropriate here since
reverse()requires valid parameters. The use ofsubTestprovides good isolation for debugging failures.One minor consideration: the hardcoded
/api/v1/prefix could become brittle if the API version or URL configuration changes. Consider using Django's test client URL prefix configuration or extracting the prefix to a constant if this becomes a maintenance concern.
After switching the users API detail routes to uuid converters, ChangePasswordView receives a UUID object in kwargs['pk'] instead of a string. The self-password permission check was still comparing str(self.request.user.id) to that UUID, which broke self password changes and caused API test regressions. Compare UUID values directly so self-password requests keep using the intended IsAuthenticated permission path. Related to openwisp#487
czarflix
force-pushed
the
codex/issue-487-uuid-route-validation
branch
from
a42ea59 to
3bb4ee5
Compare
|
I pushed a follow-up update to fix the branch-specific regression caused by the UUID route converter change and to satisfy commit validation. Local verification on the current branch:
If the remaining CI failure is still the full-suite selenium error
that appears to be preexisting: I reproduced the same failure on a clean |
|
@nemesifier I investigated the failing CI job on this PR and the failure appears to be outside the UUID route change itself. |
|
I have seen the same CI failure in other PRs. Help is welcome to fix it (in a separate PR). |
alright, I will get to it. Thanks for your time. |
I opened the separate upstream fix PR here: openwisp/openwisp-utils#620. From my testing, it preserves the serial skip behavior added in #570 and fixes the parallel runner crash. I also added focused regression coverage for both result types. |
2 participants
Checklist
Reference to Existing Issue
Closes #487.
Description of Changes
This fixes a router-to-model PK mismatch in the users API.
The user and organization detail routes were defined with
strconverters even though the underlying resources are UUID-backed, which allowed malformed values likenot-a-uuidto resolve to DRF views and return401instead of failing at the routing boundary with404.This change updates the five affected routes to use
uuidconverters and adds a focused regression test to assert malformed UUID paths return404.Because the route converter change makes
kwargs["pk"]a UUID object, this also updates the self-password permission check inChangePasswordViewto compare UUID values directly. Without that follow-up, self password changes would incorrectly take the stricter permission path and fail existing API tests.Manual validation performed:
python tests/manage.py test openwisp_users.tests.test_api.test_views --parallel 2python tests/manage.py test openwisp_users.tests.test_api --parallel 2python tests/manage.py test openwisp_users.tests.test_api.test_api.TestUsersApi.test_organization_detail_api openwisp_users.tests.test_api.test_api.TestUsersApi.test_get_user_detail_api openwisp_users.tests.test_api.test_api.TestUsersApi.test_change_password_of_superuser_by_superuser openwisp_users.tests.test_api.test_api.TestUsersApi.test_get_email_list_api openwisp_users.tests.test_api.test_api.TestUsersApi.test_put_email_update_api --parallel 2./run-qa-checksNo manual documentation update was needed because this change corrects the route definition itself, which is the source of truth for the generated API schema/docs.
Screenshot
N/A (backend/API change)