Enforce Django password validation on password change #3423
Conversation
📝 WalkthroughWalkthroughRemoved the serializer docstring, constrained Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (2)
care/users/api/viewsets/change_password.py (2)
10-10: Restore the removed docstring for better maintainability.The class docstring was removed in this change. Documentation helps maintain code clarity, especially for serializers that handle security-sensitive operations.
🔎 Add a docstring to the serializer:
class ChangePasswordSerializer(serializers.Serializer): + """ + Serializer for changing user password with validation. + """ old_password = serializers.CharField(required=True)
31-50: Verify the validation works as expected and add tests.The PR objectives mention that tests were not added for this security-related change. Please ensure that:
- Passwords shorter than 8 characters are properly rejected with clear error messages
- Passwords exactly 8 characters long are accepted
- Edge cases like empty strings, whitespace, and special characters are handled correctly
- The API response format remains consistent with existing patterns
Would you like me to help generate test cases for this validation change?
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/users/api/viewsets/change_password.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/users/api/viewsets/change_password.py
|
Hi maintainers 👋 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/users/api/viewsets/change_password.py (1)
14-14: Consider adding help_text for better API documentation.While the field definition is technically correct, adding a
help_text(e.g., "Password must meet security requirements.") would make the API more self-documenting for consumers.🔎 View suggested enhancement:
- new_password = serializers.CharField(required=True, max_length=128) + new_password = serializers.CharField( + required=True, + max_length=128, + help_text="Password must meet security requirements.", + )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/users/api/viewsets/change_password.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/users/api/viewsets/change_password.py
🪛 Ruff (0.14.8)
care/users/api/viewsets/change_password.py
20-20: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
care/users/api/viewsets/change_password.py (1)
2-3: LGTM!The imports are correctly added and the aliasing of
DjangoValidationErrorproperly avoids conflicts with DRF'sValidationError.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
care/users/api/viewsets/change_password.py (3)
14-14: Consider adding explicitmin_length=8for clarity.While Django's
MinimumLengthValidator(called invalidate_new_password) enforces the 8-character minimum by default, addingmin_length=8to the field definition would make the constraint immediately visible and provide a faster fail path before the more expensive Django validation runs. It also documents the requirement at the field level, which is, you know, kind of helpful for anyone reading the code.🔎 Proposed enhancement
- new_password = serializers.CharField(required=True, max_length=128) + new_password = serializers.CharField(required=True, min_length=8, max_length=128)
16-22: Add defensive context access to prevent KeyError.Line 17 uses direct dictionary access
self.context["request"]which will raise aKeyErrorif the serializer is instantiated without request context (e.g., in tests, management commands, or other contexts). While it works fine in the currentUpdateAPIViewusage, it makes the serializer unnecessarily fragile for reuse.🔎 Proposed fix with defensive access
def validate_new_password(self, value): - user = self.context["request"].user + request = self.context.get("request") + user = request.user if request else None try: validate_password(value, user=user) except DjangoValidationError as e: raise serializers.ValidationError(e.messages) from e return value
16-22: Add tests for the new password validation logic.The PR objectives mention that tests were "not added in this PR," but password validation is security-critical functionality that really should have test coverage. Without tests, there's no verification that the Django validators are working as expected or that the error messages are properly formatted for the API response.
Would you like me to generate test cases covering various scenarios (weak passwords, user attribute similarity, numeric-only passwords, etc.)?
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/users/api/viewsets/change_password.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/users/api/viewsets/change_password.py
🧬 Code graph analysis (1)
care/users/api/viewsets/change_password.py (1)
care/emr/resources/user/spec.py (1)
validate_password(110-117)
🔇 Additional comments (2)
care/users/api/viewsets/change_password.py (2)
2-3: LGTM! Clean import structure.The imports are well-organized and the aliasing of
ValidationErrorasDjangoValidationErrorproperly avoids naming conflicts with DRF's validation error.
52-52: Excellent fix usingvalidated_datainstead ofdata.This correctly uses the cleaned and validated password from
validated_datarather than the raw input fromdata. This ensures all validation (including the new Django password validators) is properly applied before the password is set.
19vip
changed the title
|
Thanks for the review @sainak! |
|
While adding tests for this, I noticed a few issues in the validation flow. I’ve fixed them and added full test coverage in PR #3434, which replaces this one. Please check that PR instead. Thanks! |
2 participants
Summary
This PR replaces the custom password length check in the change password endpoint with Django’s built-in password validation framework.
Problem:
The change password API previously enforced only a minimal custom length check, allowing weak passwords and ignoring project-configured password rules.
Solution:
Uses Django’s validate_password with user context to apply all configured validators.
Returns proper DRF validation errors when password rules are violated.
Preserves existing API behavior without breaking changes.
Impact:
Improves password security by aligning with Django standards.
Ensures consistent validation across the application.
Scope:
Backend only
No database changes
No API breaking changes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.