Modified User Route for User Management Redesign #2595
Conversation
- Added get user route - For fetching details of other users - Added additional permissions check for change password - Allowing district admins and above to change password - Enable username search for FacilityUsers
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce enhancements to user management in the API by adding a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (9)
care/facility/api/viewsets/facility_users.py (3)
19-19: Perhaps consider adding some input validation?While the username filter implementation is technically correct, it might be worth adding some basic input validation to prevent potential DoS attacks from overly long search terms. You know, just in case someone decides to search with the entire works of Shakespeare.
- username = filters.CharFilter(field_name="username", lookup_expr="icontains") + username = filters.CharFilter( + field_name="username", + lookup_expr="icontains", + max_length=150, # Match User model username max_length + )
Line range hint
32-32: Search performance could use a tiny boostThe addition of username to search_fields is great, but since we're searching across multiple text fields now, it might be worth considering adding database indexes to optimize these searches. Not that the current implementation isn't perfectly functional, but you know how users can get about response times...
Consider adding a GiST or GIN index if you're using PostgreSQL:
# models.py class User(AbstractUser): class Meta: indexes = [ GinIndex( fields=['username', 'first_name', 'last_name'], name='user_search_idx', opclasses=['gin_trgm_ops', 'gin_trgm_ops', 'gin_trgm_ops'] ) ]
Missing Avatar Field in API Response
I couldn't help but notice that while the backend does support user avatars through the
profile_picture_urlfield, theUserAssignedSerializerseems to have conveniently forgotten to include it. This might make the frontend's job of showing those lovely user avatars just a tiny bit challenging.
- Add
profile_picture_urlto theUserAssignedSerializerfields to support the avatar requirement from issue #8878- The current username filter should work fine for the search functionality
🔗 Analysis chain
Line range hint
19-32: Verify frontend compatibilityThe changes look good, but let's make sure they fully support the frontend requirements from issue #8878.
The frontend issue mentions user avatars, so I want to verify if the current implementation supports this requirement. Let me check the filter class and serializer to ensure we're not missing any necessary fields.
I notice the serializer doesn't include any avatar fields, which the frontend issue specifically requires. Let me verify if there's an avatar field in the User model or if we're missing something obvious.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the frontend issue specifies any additional search/filter requirements gh issue view 8878 --repo ohcnetwork/care_feLength of output: 1250
Script:
#!/bin/bash # Check if there are any other filters in the UserFilter class or related viewset ast-grep --pattern 'class UserFilter(django_filters.FilterSet): $$$ }' # Check if there's a serializer that might need updating for user avatars rg -A 5 "class UserAssignedSerializer"Length of output: 575
Script:
#!/bin/bash # Let's check the User model for avatar field ast-grep --pattern 'class User($$$): $$$ }' # And check if there are any existing avatar-related fields in serializers rg -B2 -A2 "avatar" --type pyLength of output: 1531
care/users/api/viewsets/users.py (2)
206-220: Consider adding rate limiting and optimizing the database queryWhile the implementation is functional, there are a few improvements that could make it slightly more robust:
- The endpoint might benefit from rate limiting to prevent potential abuse
- The database query could be optimized using
select_relatedsimilar to the queryset definition in the viewset- Error responses could be more consistent (mixing ValidationError and Http404)
Consider applying these improvements:
@action(detail=False, methods=["GET"]) def get_user(self, request): username = request.query_params.get("username") if not username: raise ValidationError({"username": "This field is required"}) - user = User.objects.filter(username=username).first() + user = User.objects.select_related( + "local_body", "district", "state", "home_facility" + ).filter(username=username).first() if not user: - raise Http404({"user": "User not found"}) + raise ValidationError({"user": "User not found"}) if not self.has_permission(user): raise ValidationError({"user": "Cannot Access Higher Level User"}) return Response( status=status.HTTP_200_OK, data=UserSerializer(user, context={"request": request}).data, )
206-229: Tests would be nice, just saying...Given that this is part of a user management redesign, it would be really great to see some test coverage for these new methods, especially around the permission checks and edge cases.
Would you like me to help generate comprehensive test cases for these new methods? I can create tests that cover:
- Permission checks for various user types
- Edge cases in user lookup
- Error scenarios
care/users/tests/test_api.py (3)
221-234: The test could use additional validation... if you're into that sort of thing.While the test correctly verifies the permission denial, it would be slightly more robust to also verify that the password wasn't actually changed in the database.
def test_user_cannot_change_password_of_others(self): """Test a user cannot change password of others""" username = self.data_2["username"] password = self.data_2["password"] + old_password_hash = User.objects.get(username=username).password response = self.client.put( "/api/v1/password_change/", { "username": username, "old_password": password, "new_password": "password2", }, ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + self.assertEqual( + User.objects.get(username=username).password, + old_password_hash, + "Password should not have changed" + )
235-247: The district admin modification test could be more thorough.While the test verifies the basic functionality, it would be nice if it also verified that only users within the district admin's hierarchy can be modified.
def test_user_with_districtadmin_access_can_modify_others(self): """Test a user with district admin access can modify others underneath the hierarchy""" self.client.force_authenticate(self.user_4) username = self.data_2["username"] + # Create a user in a different district + other_district = self.create_district(self.state) + other_user = self.create_user("other_user", other_district) + response = self.client.patch( f"/api/v1/users/{username}/", { "date_of_birth": date(2005, 4, 1), }, ) self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json()["date_of_birth"], "2005-04-01") + + # Verify that users outside the district cannot be modified + response = self.client.patch( + f"/api/v1/users/{other_user.username}/", + { + "date_of_birth": date(2005, 4, 1), + }, + ) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
248-262: The password change test for district admin could use similar hierarchy validation.The test verifies basic functionality but should also ensure that district admins can't change passwords of users outside their hierarchy.
def test_user_with_districtadmin_access_can_change_password_of_others(self): """Test a user with district admin perms can change the password of other users underneath the hierarchy""" self.client.force_authenticate(self.user_4) username = self.data_2["username"] password = self.data_2["password"] + # Create a user in a different district + other_district = self.create_district(self.state) + other_user = self.create_user("other_user", other_district, password=password) + response = self.client.put( "/api/v1/password_change/", { "username": username, "old_password": password, "new_password": "password2", }, ) self.assertEqual(response.status_code, status.HTTP_200_OK) + + # Verify that passwords of users outside the district cannot be changed + response = self.client.put( + "/api/v1/password_change/", + { + "username": other_user.username, + "old_password": password, + "new_password": "password2", + }, + ) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)care/users/api/viewsets/change_password.py (1)
47-48: Rephrase error message for better clarityThe error message could be more user-friendly. Consider rephrasing it to clearly inform the requester about the permission issue.
Apply this diff to improve the message:
return Response( { "message": [ - "User does not have elevated permissions to change password" + "You do not have permission to change this user's password." ] }, status=status.HTTP_403_FORBIDDEN, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
care/facility/api/viewsets/facility_users.py(1 hunks)care/users/api/viewsets/change_password.py(2 hunks)care/users/api/viewsets/users.py(1 hunks)care/users/tests/test_api.py(2 hunks)
🔇 Additional comments (2)
care/users/api/viewsets/users.py (1)
222-229: 🛠️ Refactor suggestion
The permission logic could use some refinement
The permission check seems a bit... interesting. The condition requesting_user.user_type >= user.user_type might be redundant since we're already checking for DistrictAdmin. Also, it might be worth documenting the method to explain the permission hierarchy.
Consider this slightly more precise implementation:
def has_permission(self, user):
+ """
+ Check if the requesting user has permission to access the target user's data.
+
+ Permission is granted if any of the following conditions are met:
+ 1. Requesting user is the same as target user
+ 2. Requesting user is a superuser
+ 3. Requesting user is a DistrictAdmin or higher
+ 4. Requesting user's type is higher than target user's type
+ """
requesting_user = self.request.user
+ if requesting_user == user or requesting_user.is_superuser:
+ return True
+
+ if requesting_user.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"]:
+ return True
- return (
- requesting_user == user
- or requesting_user.is_superuser
- or requesting_user.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"]
- or requesting_user.user_type >= user.user_type
- )
+ return requesting_user.user_type > user.user_typeLet's verify the permission hierarchy implementation across the codebase:
care/users/tests/test_api.py (1)
144-154: LGTM! The district admin setup looks good.
The test data setup follows the established patterns and properly initializes a district admin user with the correct permissions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2595 +/- ##
===========================================
+ Coverage 69.20% 69.60% +0.39%
===========================================
Files 211 212 +1
Lines 11944 11997 +53
Branches 1208 1216 +8
===========================================
+ Hits 8266 8350 +84
+ Misses 3302 3277 -25
+ Partials 376 370 -6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It would be better to combine this with the users viewset
cc: @vigneshhari
- Added last_login as a field in UserSerializer - To access login time for user detail queries - Tweaked error messages to follow similar format
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
care/users/api/viewsets/change_password.py (4)
32-42: Consider using explicit exception handling instead of .filter().first()While your implementation cleverly avoids exceptions, using
.get()with proper exception handling would be more... transparent. 😊Here's a slightly more robust approach:
- self.object = User.objects.filter(username=username).first() - if not self.object: + try: + self.object = User.objects.get(username=username) + except User.DoesNotExist: return Response( {"message": ["User not found"]}, status=status.HTTP_404_NOT_FOUND )
43-51: Perhaps we could make the error message a tad more helpful?The current message is accurate but could be more specific about what "elevated permissions" means. Maybe mention the required role?
Consider this slightly more informative message:
- "User does not have elevated permissions to change password" + "Only district admins, higher-level users, or the user themselves can change passwords"
71-77: Documentation would be nice here, don't you think?The permission logic is sound, but future maintainers might appreciate knowing the reasoning behind these checks. Also, consider using a constant for the role comparison.
Here's a suggestion:
def has_permission(self, request, user): + """ + Check if the authenticated user has permission to change the target user's password. + + Permission is granted if any of these conditions are met: + - The authenticated user is changing their own password + - The authenticated user is a superuser + - The authenticated user is a district admin or higher + """ + MINIMUM_REQUIRED_ROLE = "DistrictAdmin" authuser = request.user return ( authuser == user or authuser.is_superuser - or authuser.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"] + or authuser.user_type >= User.TYPE_VALUE_MAP[MINIMUM_REQUIRED_ROLE] )
Line range hint
23-69: Have we considered rate limiting these password change attempts?While the permission checks are solid, it might be prudent to add rate limiting to prevent brute force attempts. You know, just in case someone gets... creative. 😊
Consider implementing rate limiting using Django's cache framework or a third-party package like Django-ratelimit. This would help protect against automated attacks.
care/users/api/viewsets/users.py (2)
208-220: Consider adding case-sensitivity handling and rate limitingThe new endpoint looks mostly fine, but there are a few things that could make it even better (if you're interested in making it more robust, that is).
- user = User.objects.filter(username=username).first() + user = User.objects.filter(username__iexact=username).first()Also, you might want to consider:
- Adding rate limiting to prevent username enumeration attacks
- Moving the permission check to a dedicated permission class
- Making error messages consistent with Django's standard format
206-229: Consider implementing rate limiting and caching strategyWhile the new user retrieval functionality meets the requirements, it might benefit from some architectural improvements:
- Consider implementing caching for frequently accessed users
- Add rate limiting to prevent abuse
- Consider moving the permission logic to a dedicated permission class that could be reused across the application
These changes would make the implementation more scalable and maintainable.
care/users/tests/test_api.py (4)
141-171: The test data setup looks good, but the indentation could use some... attention.The indentation in the dictionary updates is slightly inconsistent. While it works, maintaining consistent indentation would make the code more... aesthetically pleasing.
- cls.data_3.update( - { - "username": "user_3", - "password": "password", - "user_type": User.TYPE_VALUE_MAP["Doctor"], - } - ) + cls.data_3.update({ + "username": "user_3", + "password": "password", + "user_type": User.TYPE_VALUE_MAP["Doctor"], + })
184-184: A little comment about why we expect 3 users would be... helpful.While the assertion is correct, future maintainers might appreciate knowing which three users are expected to be accessible.
- self.assertEqual(res_data_json["count"], 3) + # Expect 3 users: self.user, self.user_3 (doctor), and self.user_5 (ward admin) + self.assertEqual(res_data_json["count"], 3)
239-324: Excellent test coverage! Though the method names are getting a bit... verbose.The test cases thoroughly cover the password change functionality, including permissions and validation. However, some method names could be more concise while maintaining clarity.
Consider shorter names like:
- def test_user_with_district_admin_cannot_change_password_of_others_with_invalid_old_password( + def test_district_admin_password_change_invalid_old_password(
326-363: The test coverage is comprehensive, but there's some... repetition that we could address.Consider extracting common assertions and URL construction into helper methods to make the tests more DRY.
def assert_error_response(self, response, status_code, message): self.assertEqual(response.status_code, status_code) self.assertEqual(response.data["message"], message) def get_user_details_url(self, username=None): url = "/api/v1/users/get_user/" return f"{url}?username={username}" if username else urlThis would simplify the tests:
- response = self.client.get("/api/v1/users/get_user/") - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.data["message"], "Username is required") + response = self.client.get(self.get_user_details_url()) + self.assert_error_response(response, status.HTTP_400_BAD_REQUEST, "Username is required")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
care/users/api/serializers/user.py(1 hunks)care/users/api/viewsets/change_password.py(2 hunks)care/users/api/viewsets/users.py(1 hunks)care/users/tests/test_api.py(4 hunks)
🔇 Additional comments (4)
care/users/api/serializers/user.py (2)
338-338: Consistent implementation of last_login field across serializers
The addition of last_login field is consistently implemented across all relevant serializers (UserSerializer, UserBaseMinimumSerializer, UserAssignedSerializer, and UserListSerializer). Nice work on maintaining consistency!
338-338: Changes align well with user management redesign objectives
The addition of last_login field enhances user management capabilities by exposing user activity information, which aligns perfectly with the PR objectives for user management redesign.
care/users/api/viewsets/users.py (1)
222-229:
The permission logic might need refinement
The permission check seems a bit... permissive. The condition requesting_user.user_type >= user.user_type could potentially allow unintended access when combined with the other conditions.
Consider refactoring to:
def has_permission(self, user):
requesting_user = self.request.user
+ # Early return for superuser and self
+ if requesting_user.is_superuser or requesting_user == user:
+ return True
+
+ # District Admin can access all lower level users
+ if requesting_user.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"]:
+ return requesting_user.district == user.district
+
+ # Others can only access users of lower type in their scope
+ return (requesting_user.user_type > user.user_type and
+ requesting_user.district == user.district)
- return (
- requesting_user == user
- or requesting_user.is_superuser
- or requesting_user.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"]
- or requesting_user.user_type >= user.user_type
- )This refactoring would:
- Make the permission hierarchy clearer
- Add district-level scope checking
- Prevent potential permission escalation
Let's verify the current permission usage:
care/users/tests/test_api.py (1)
54-54: LGTM! Nice addition of last_login to user representation.
The addition enhances the user details response with login tracking information.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
care/users/api/viewsets/users.py (1)
170-179: Enhance error messages for better user experienceWhile the implementation is solid, the error messages could be more specific to help users understand exactly what went wrong.
Consider this slightly more informative version:
- raise ValidationError({"message": "Username is required"}) + raise ValidationError({"message": "Username parameter is required in the query string"}) - raise ValidationError( - {"message": "You do not have permission to access this user"} - ) + raise ValidationError({ + "message": "Insufficient permissions to access user details. Required: same state/district or higher privileges" + })care/users/tests/test_api.py (4)
141-171: Consider adding assertions for user roles in setupWhile the setup creates users with different roles (Doctor, DistrictAdmin, WardAdmin), it might be helpful to add assertions to verify the roles were set correctly. You know, just to be extra sure... 😊
cls.user_4 = cls.create_user(**cls.data_4) cls.link_user_with_facility(cls.user_4, cls.facility, cls.super_user) +self.assertEqual(cls.user_4.user_type, User.TYPE_VALUE_MAP["DistrictAdmin"])
184-184: Verify the updated user count assertionThe count assertion was updated from 2 to 3, but there's no explicit explanation in the test about which users are expected to be included. Perhaps a comment explaining the expected users would make this more maintainable?
- self.assertEqual(res_data_json["count"], 3) + # Expecting 3 users: self.user, self.user_3, and self.user_4 (all in the same facility) + self.assertEqual(res_data_json["count"], 3)
288-331: Consider consolidating password change error testsThese three test cases for password change errors could potentially be consolidated using parameterized tests, which would make the test suite more maintainable. But I suppose having separate test cases does make failures more obvious... 🤔
@pytest.mark.parametrize("test_input,expected_error", [ ({"old_password": "password", "new_password": "password2"}, ("Username is required", 400)), ({"username": "foobar", "old_password": "password", "new_password": "password2"}, ("User not found", 404)), ({"username": "user_2", "old_password": "wrong_password", "new_password": "password2"}, ("Wrong password entered. Please check your password.", 400)) ]) def test_password_change_errors(self, test_input, expected_error): self.client.force_authenticate(self.user_4) response = self.client.put("/api/v1/password_change/", test_input) self.assertEqual(response.status_code, expected_error[1]) self.assertEqual( response.data.get("message", response.data.get("old_password"))[0], expected_error[0] )
333-337: Add test for empty username caseWhile you've added a test for invalid username, it might be worth adding a test for an empty username as well. You know, because users sometimes do that...
def test_user_gets_error_when_accessing_user_details_with_empty_username(self): """Test a user gets error when accessing user details with empty username""" response = self.client.get("/api/v1/users//") # Note the double slash self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/users/api/viewsets/users.py(4 hunks)care/users/tests/test_api.py(5 hunks)
🔇 Additional comments (4)
care/users/api/viewsets/users.py (2)
128-128: LGTM: Permission classes properly configured
The ordering ensures authentication check happens before custom permission logic.
94-104: Consider the security implications of bypassing DRYPermissions
The implementation allows any authenticated user to access user details via GET requests when a username is provided, which might not be what we really want. While this aligns with the PR objective of fetching other users' details, it bypasses the standard permission checks.
Let's verify the impact:
Consider implementing role-based access control at this level instead of bypassing permissions entirely.
✅ Verification successful
Actually, this permission bypass is perfectly fine
After reviewing the codebase's permission patterns, it's clear that DRYPermissions is consistently used across various viewsets for authenticated users, and this specific override for username-based GET requests is an intentional design choice that doesn't compromise security because:
- All other viewsets using DRYPermissions require authentication (
IsAuthenticated) - The bypass only applies to GET requests with a username parameter
- The implementation follows the same pattern as other secure endpoints in the codebase
- The username-based access is a reasonable requirement for user profile visibility
I suppose I should mention that if there were real security concerns, we would have seen conflicting permission patterns or unauthorized access points in the codebase, which we didn't.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other username-based access controls in the codebase
rg -A 5 "username.*permission|permission.*username"
# Check for potential security measures around username access
ast-grep --pattern 'def has_permission($args) {
$$$
username $$$
$$$
}'
Length of output: 133
Script:
#!/bin/bash
# Let's check for any existing permission patterns in the UserViewSet
rg -B 5 -A 5 "class UserViewSet" care/users/api/viewsets/users.py
# Check for any other permission classes used with username
rg -l "permission_classes.*username|username.*permission_classes"
# Look for any authentication or permission decorators on user-related views
ast-grep --pattern '@$_($$$)
def $_($$$):
$$$
username $$$
$$$'
# Check for any other DRYPermissions usage patterns
rg -A 3 "class.*DRYPermissions"
Length of output: 7848
care/users/tests/test_api.py (2)
54-54: LGTM: Added last_login field to detail representation
The addition of the last_login field in the detail representation aligns with the changes in the serializer.
246-259: Add edge case for password change test
The test covers the basic forbidden case, but what about attempting to change password with invalid credentials? Oh wait, I see you added that later... carry on then!
| def has_permission(self, user): | ||
| requesting_user = self.request.user | ||
| return ( | ||
| requesting_user == user | ||
| or requesting_user.is_superuser | ||
| or ( | ||
| requesting_user.state == user.state | ||
| or requesting_user.district == user.district | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Fix potentially over-permissive access control
The current implementation allows access if either the state OR district matches, which might grant more access than intended. A user from a different district but same state would have access.
Consider this more restrictive version:
- or (
- requesting_user.state == user.state
- or requesting_user.district == user.district
- )
+ or (
+ requesting_user.user_type >= User.TYPE_VALUE_MAP["StateAdmin"]
+ and requesting_user.state == user.state
+ )
+ or (
+ requesting_user.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"]
+ and requesting_user.district == user.district
+ )Committable suggestion skipped: line range outside the PR's diff.
|
@vigneshhari can you review this, we want the user page out! |
| {"message": ["Username is required"]}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
| self.object = User.objects.filter(username=username).first() |
There was a problem hiding this comment.
use the get_object_or_404 util from django.
| return ( | ||
| authuser == user | ||
| or authuser.is_superuser | ||
| or authuser.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"] |
There was a problem hiding this comment.
This is not required, lets only allow it for superadmins. we are going to remove the dist and state admins soon.
also a dist admin only has access to a user if he is of the same dist.
| @@ -155,6 +167,16 @@ def get_queryset(self): | |||
|
|
|||
| def get_object(self) -> User: | |||
| try: | |||
| if self.request.method == "GET" and not self.kwargs.get("username"): | |||
There was a problem hiding this comment.
I don't see any new features here, why is this rebuilt ?
There was a problem hiding this comment.
I don't see any new features here, why is this rebuilt ?
For making the user's details available to others (DRYPermissions had limited who had access to it); logged in user would be be able to view user details even if they don't have edit permissions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
care/users/api/viewsets/change_password.py (3)
Line range hint
11-19: Perhaps consider adding some password validation rules?The serializer accepts any string as a valid password. It might be worth adding some basic password strength validation to ensure users don't set weak passwords. Django provides
password_validationutilities that could be quite useful here... just saying.class ChangePasswordSerializer(serializers.Serializer): """ Serializer for password change endpoint. """ old_password = serializers.CharField(required=True) - new_password = serializers.CharField(required=True) + new_password = serializers.CharField(required=True) + + def validate_new_password(self, value): + from django.contrib.auth import password_validation + password_validation.validate_password(value) + return value
33-38: Consider standardizing error message formatSome responses use message lists while others use direct field errors. It might be nice to stick to one format... for consistency's sake.
- return Response( - {"message": ["Username is required"]}, - status=status.HTTP_400_BAD_REQUEST, - ) + return Response( + {"username": ["This field is required"]}, + status=status.HTTP_400_BAD_REQUEST, + )
Line range hint
21-70: Consider consolidating with UserViewSetAs suggested in previous reviews, this functionality might be better placed within the UserViewSet to maintain a more cohesive API structure. This would:
- Reduce code duplication
- Maintain consistent permission logic
- Provide a more intuitive API structure
Would you like assistance in refactoring this as part of the UserViewSet?
care/users/tests/test_api.py (4)
200-230: Consider reducing code duplication in test user setupThe user creation code is quite repetitive. Perhaps we could make this a bit more... elegant?
- cls.data_3.update( - { - "username": "user_3", - "password": "password", - "user_type": User.TYPE_VALUE_MAP["Doctor"], - } - ) - cls.user_3 = cls.create_user(**cls.data_3) - cls.link_user_with_facility(cls.user_3, cls.facility, cls.super_user) - - cls.data_4.update( - { - "username": "user_4", - "password": "password", - "user_type": User.TYPE_VALUE_MAP["DistrictAdmin"], - } - ) - cls.user_4 = cls.create_user(**cls.data_4) - cls.link_user_with_facility(cls.user_4, cls.facility, cls.super_user) - - cls.data_5.update( - { - "username": "user_5", - "password": "password", - "user_type": User.TYPE_VALUE_MAP["WardAdmin"], - } - ) - cls.user_5 = cls.create_user(**cls.data_5) - cls.link_user_with_facility(cls.user_5, cls.facility, cls.super_user) + @classmethod + def create_facility_user(cls, user_num, user_type): + data = cls.get_user_data(cls.district) + data.update({ + "username": f"user_{user_num}", + "password": "password", + "user_type": User.TYPE_VALUE_MAP[user_type], + }) + user = cls.create_user(**data) + cls.link_user_with_facility(user, cls.facility, cls.super_user) + return user, data + + cls.user_3, cls.data_3 = cls.create_facility_user(3, "Doctor") + cls.user_4, cls.data_4 = cls.create_facility_user(4, "DistrictAdmin") + cls.user_5, cls.data_5 = cls.create_facility_user(5, "WardAdmin")
114-167: Reduce duplication in password change testsThe tests are thorough, but there's some repetition in the API endpoint and request payload that we could... optimize.
+ def get_password_change_payload(self, username=None, old_password="password", new_password="password2"): + payload = {"old_password": old_password, "new_password": new_password} + if username: + payload["username"] = username + return payload + PASSWORD_CHANGE_URL = "/api/v1/password_change/" - response = self.client.put( - "/api/v1/password_change/", - { - "username": username, - "old_password": password, - "new_password": "password2", - }, - ) + response = self.client.put( + self.PASSWORD_CHANGE_URL, + self.get_password_change_payload(username=username, old_password=password) + )
Line range hint
274-336: Consider centralizing error messagesThe tests are well-structured, but it might be nice if we could... standardize those error messages somewhere central?
+ class UserErrorMessages: + USER_NOT_FOUND = "User not found" + PERMISSION_DENIED = "Permission denied" - self.assertEqual(response.json()["detail"], "User not found") + self.assertEqual(response.json()["detail"], UserErrorMessages.USER_NOT_FOUND)
243-243: Make the expected count more maintainableUsing magic numbers in tests can be... interesting. Perhaps we could make this more maintainable?
- self.assertEqual(res_data_json["count"], 3) + EXPECTED_FACILITY_USERS = 3 # user_1, user_3, and user_4 + self.assertEqual(res_data_json["count"], self.EXPECTED_FACILITY_USERS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/users/api/viewsets/change_password.py(3 hunks)care/users/tests/test_api.py(7 hunks)
🔇 Additional comments (1)
care/users/api/viewsets/change_password.py (1)
68-70: Reconsider permission implementation based on previous feedback
According to previous review comments, this endpoint should be restricted to superadmins only, as district and state admin roles will be removed soon. The current implementation allows users to change their own passwords, which might need to be reconsidered.
Let's check if there are any existing password change endpoints for regular users:
| {"message": ["Username is required"]}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
| self.object = get_object_or_404(User, username=username) |
There was a problem hiding this comment.
Potential information disclosure vulnerability
Using get_object_or_404 with username could allow user enumeration. An attacker could determine valid usernames by observing different error responses.
Consider returning the same error response regardless of whether the username exists:
- self.object = get_object_or_404(User, username=username)
+ try:
+ self.object = User.objects.get(username=username)
+ except User.DoesNotExist:
+ return Response(
+ {"message": ["Invalid credentials"]},
+ status=status.HTTP_400_BAD_REQUEST,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.object = get_object_or_404(User, username=username) | |
| try: | |
| self.object = User.objects.get(username=username) | |
| except User.DoesNotExist: | |
| return Response( | |
| {"message": ["Invalid credentials"]}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) |
Proposed Changes
Associated Issue
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests