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 introduced in this pull request enhance user management functionalities by adding a case-insensitive 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 (
|
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.58% 69.63% +0.05%
===========================================
Files 211 211
Lines 11875 11879 +4
Branches 1201 1202 +1
===========================================
+ Hits 8263 8272 +9
+ Misses 3246 3240 -6
- Partials 366 367 +1 ☔ View full report in Codecov by Sentry. |
- 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!
|
@vigneshhari can you review this, we want the user page out! |
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:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
care/facility/tests/test_facilityuser_api.py (2)
23-25: Perhaps consider a more meaningful test user name?While "dummystaff" works, a name like "searchable_user" or "test_search_user" might better indicate this user's purpose in the search functionality tests. Just a thought...
38-46: Documentation seems to be playing hide and seek.Would be lovely to have docstrings for the test methods explaining the test scenarios and expected behaviors. This would make it easier for future contributors to understand the test coverage... just saying.
363f57c to
c38cfc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
care/users/api/viewsets/change_password.py (2)
2-2: Remove unused importThe
get_object_or_404import is not being used anywhere in the code. I mean, we could keep it as a souvenir, but...-from django.shortcuts import get_object_or_404🧰 Tools
🪛 Ruff (0.8.0)
2-2:
django.shortcuts.get_object_or_404imported but unusedRemove unused import:
django.shortcuts.get_object_or_404(F401)
Line range hint
31-42: Consider enhancing password validationThe current password validation only checks if the old password is correct. Perhaps we could add some basic password strength requirements? Just a thought...
if serializer.is_valid(): check = self.object.check_password(request.data.get("old_password")) if not check: return Response( { "old_password": [ "Wrong password entered. Please check your password." ] }, status=status.HTTP_400_BAD_REQUEST, ) + new_password = serializer.data.get("new_password") + if len(new_password) < 8: + return Response( + {"new_password": ["Password must be at least 8 characters long"]}, + status=status.HTTP_400_BAD_REQUEST, + ) self.object.set_password(serializer.data.get("new_password"))🧰 Tools
🪛 Ruff (0.8.0)
2-2:
django.shortcuts.get_object_or_404imported but unusedRemove unused import:
django.shortcuts.get_object_or_404(F401)
care/users/models.py (1)
Line range hint
371-379: Consider adding error handling for user type validationThe
has_write_permissionmethod might raise KeyError or TypeError. While it's handled by returning True, it might be worth being more explicit about the fallback behavior.@staticmethod def has_write_permission(request): + if not request.data: + return True try: return int(request.data["user_type"]) <= User.TYPE_VALUE_MAP["Volunteer"] except TypeError: return ( User.TYPE_VALUE_MAP[request.data["user_type"]] <= User.TYPE_VALUE_MAP["Volunteer"] ) except KeyError: - # No user_type passed, the view shall raise a 400 + # No user_type passed in request data, defaulting to True + # The view will handle validation and raise 400 if needed return Truecare/users/tests/test_api.py (2)
244-257: Add edge cases to password change testWhile testing that users can't change others' passwords is good, we might want to test some edge cases. You know, just in case someone tries something creative...
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"] + # Test with empty username + response = self.client.put( + "/api/v1/password_change/", + { + "username": "", + "old_password": password, + "new_password": "password2", + }, + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + # Test with non-existent username response = self.client.put( "/api/v1/password_change/", { - "username": username, + "username": "nonexistent_user", "old_password": password, "new_password": "password2", }, ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
271-275: Consider testing more invalid username patternsThe test for invalid usernames could be more thorough. After all, users can be quite... creative with their input.
def test_user_gets_error_when_accessing_user_details_with_invalid_username(self): """Test a user gets error when accessing user details with invalid username""" + invalid_usernames = ['', ' ', 'foo bar', 'admin;', '<script>'] + for username in invalid_usernames: + response = self.client.get(f"/api/v1/users/{username}/") + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - response = self.client.get("/api/v1/users/foobar/") - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
care/facility/api/viewsets/facility_users.py(1 hunks)care/users/api/viewsets/change_password.py(1 hunks)care/users/api/viewsets/users.py(1 hunks)care/users/models.py(1 hunks)care/users/tests/test_api.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- care/users/api/viewsets/users.py
- care/facility/api/viewsets/facility_users.py
🧰 Additional context used
🪛 Ruff (0.8.0)
care/users/api/viewsets/change_password.py
2-2: django.shortcuts.get_object_or_404 imported but unused
Remove unused import: django.shortcuts.get_object_or_404
(F401)
🔇 Additional comments (1)
care/users/models.py (1)
367-367: Verify security implications of universal read permission
Always returning True for has_object_read_permission means everyone can read everything. I'm sure that's exactly what we want... right?
✅ Verification successful
Actually, this universal read permission is perfectly fine
Looking at the codebase's permission structure, the User model's has_object_read_permission is just one layer in a rather sophisticated permission system. The real access control happens through:
- Base permission mixins that check superuser status and ownership
- Facility-specific permissions that verify user roles and relationships
- Patient-related models that enforce minimum role requirements (NurseReadOnly)
- Query-level filtering that pre-filters accessible records
So while it might seem concerning at first glance that we're returning True, the actual authorization is handled quite thoroughly elsewhere. But you know, thanks for keeping us on our toes about security!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any sensitive fields in the User model that might be exposed
rg -A 5 "^class User\(" | rg -A 20 "models\.(Char|Text|Email|JSON)Field"
Length of output: 217
Script:
#!/bin/bash
# Let's get a better view of the User model and its fields
ast-grep --pattern 'class User($$$) {
$$$
}'
# Also check the has_object_read_permission implementation
ast-grep --pattern 'def has_object_read_permission($$$) {
$$$
}'
# And look for any permission checks or filters using this method
rg -B 2 -A 2 "has_object_read_permission"
Length of output: 8912
e3deae8 to
5b6397c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
care/users/tests/test_api.py (3)
25-27: LGTM, but consider using a more descriptive variable name.The variable name
data_2could be more descriptive of its purpose, perhaps something likeuser_2_data? Just a thought...
147-177: Test data setup could be more DRY.The repeated pattern of creating users with different roles could be refactored into a helper method. I mean, if you're into that sort of thing...
@classmethod + def create_user_with_role(cls, username, user_type, district, facility=None): + data = cls.get_user_data(district) + data.update({ + "username": username, + "password": "password", + "user_type": User.TYPE_VALUE_MAP[user_type], + }) + user = cls.create_user(**data) + if facility: + cls.link_user_with_facility(user, facility, cls.super_user) + return user, data - cls.data_3.update(...) - cls.user_3 = cls.create_user(**cls.data_3) + cls.user_3, cls.data_3 = cls.create_user_with_role("user_3", "Doctor", cls.district, cls.facility)
272-275: LGTM, but consider additional edge cases.While testing invalid username is good, we might want to test other edge cases like empty string or special characters... but I suppose that's not critical right now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
care/facility/api/viewsets/facility_users.py(1 hunks)care/users/models.py(1 hunks)care/users/tests/test_api.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- care/facility/api/viewsets/facility_users.py
- care/users/models.py
🔇 Additional comments (1)
care/users/tests/test_api.py (1)
58-58: Ensure consistent serializer field updates.
The addition of last_login field to the user representation should be reflected in all relevant serializer tests.
✅ Verification successful
Oh, look who's being thorough with their serializers
I see that last_login is consistently included across all relevant user serializers in the codebase, and there's even a test case that explicitly verifies the presence of last_login in the response. The field is properly handled in:
- UserSerializer
- UserBaseMinimumSerializer
- UserAssignedSerializer
- UserListSerializer
And there's a test in test_api.py that specifically sets up and verifies last_login scenarios. How... refreshingly complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all user serializer tests include last_login field
# Search for serializer test files and check for last_login field
rg -l "UserSerializer|UserBaseMinimumSerializer|UserAssignedSerializer|UserListSerializer" | xargs rg "last_login"
Length of output: 1028
Script:
#!/bin/bash
# Let's check the actual test files to see if they're testing the last_login field
rg -A 5 -B 5 "test.*UserSerializer" care/users/tests/
Length of output: 55
Script:
#!/bin/bash
# Let's look more broadly for test files and last_login in tests
rg -l "test.*User" care/users/tests/ | xargs rg -A 5 -B 5 "last_login"
Length of output: 30827
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/users/api/viewsets/users.py(1 hunks)
🔇 Additional comments (1)
care/users/api/viewsets/users.py (1)
158-160: Consider adding permission checks in get_object method
Based on the previous discussions in the PR comments, it might be worth adding permission checks to ensure users can only retrieve details of users within their jurisdiction. You know, just to keep things... secure. 😊
Let's verify the current permission implementation:
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
New Features
usernamefilter for user searches.last_loginfield to user data representations.Bug Fixes
Tests