From 7fc9ca36b8b53c83eb8b1054c9096af872088e5d Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Mon, 11 Nov 2024 15:37:42 +0530 Subject: [PATCH 01/12] Added get user route - 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 --- care/facility/api/viewsets/facility_users.py | 1 + care/users/api/viewsets/change_password.py | 29 +++++++++++++++++++- care/users/api/viewsets/users.py | 25 +++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/care/facility/api/viewsets/facility_users.py b/care/facility/api/viewsets/facility_users.py index fb2cf25916..ce4cf71c91 100644 --- a/care/facility/api/viewsets/facility_users.py +++ b/care/facility/api/viewsets/facility_users.py @@ -16,6 +16,7 @@ class UserFilter(filters.FilterSet): choices=[(key, key) for key in User.TYPE_VALUE_MAP], coerce=lambda role: User.TYPE_VALUE_MAP[role], ) + username = filters.CharFilter(field_name="username", lookup_expr="icontains") class Meta: model = User diff --git a/care/users/api/viewsets/change_password.py b/care/users/api/viewsets/change_password.py index 805bebddd9..f5fc473fd2 100644 --- a/care/users/api/viewsets/change_password.py +++ b/care/users/api/viewsets/change_password.py @@ -29,7 +29,26 @@ class ChangePasswordView(UpdateAPIView): model = User def update(self, request, *args, **kwargs): - self.object = self.request.user + username = request.data.get("username") + if not username: + return Response( + {"message": ["Username is required"]}, + status=status.HTTP_400_BAD_REQUEST, + ) + self.object = User.objects.get(username=username) + if not self.object: + return Response( + {"message": ["User not found"]}, status=status.HTTP_404_NOT_FOUND + ) + if not self.has_permission(request, self.object): + return Response( + { + "message": [ + "User does not have elevated permissions to change password" + ] + }, + status=status.HTTP_403_FORBIDDEN, + ) serializer = self.get_serializer(data=request.data) if serializer.is_valid(): @@ -48,3 +67,11 @@ def update(self, request, *args, **kwargs): return Response({"message": "Password updated successfully"}) return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) + + def has_permission(self, request, user): + authuser = request.user + return ( + authuser == user + or authuser.is_superuser + or authuser.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"] + ) diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index a70241c855..913c76abe3 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -203,6 +203,31 @@ def destroy(self, request, *args, **kwargs): user.save(update_fields=["is_active"]) return Response(status=status.HTTP_204_NO_CONTENT) + @extend_schema(tags=["users"]) + @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() + if not user: + raise Http404({"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, + ) + + def has_permission(self, user): + requesting_user = self.request.user + 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 + ) + @extend_schema(tags=["users"]) @action(detail=False, methods=["POST"]) def add_user(self, request, *args, **kwargs): From 601ab9b0835cbf07b94ca17bd342d30c805a780e Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Mon, 11 Nov 2024 22:03:23 +0530 Subject: [PATCH 02/12] added tests --- care/users/tests/test_api.py | 53 ++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/care/users/tests/test_api.py b/care/users/tests/test_api.py index 109120d71b..75b81de7c3 100644 --- a/care/users/tests/test_api.py +++ b/care/users/tests/test_api.py @@ -141,6 +141,17 @@ def setUpTestData(cls) -> None: 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 = cls.get_user_data(cls.district) + 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) + def test_user_can_access_url(self): """Test user can access the url by location""" username = self.user.username @@ -207,6 +218,48 @@ def test_user_cannot_delete_others(self): User.objects.get(username=self.data_2[field]).username, ) + 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"] + 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) + + 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"] + 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") + + 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"] + 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) + class TestUserFilter(TestUtils, APITestCase): @classmethod From 537e6b28a16e7a4aba36a001041c43c59511fe11 Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Thu, 14 Nov 2024 17:36:47 +0530 Subject: [PATCH 03/12] Added additional tests - Added last_login as a field in UserSerializer - To access login time for user detail queries - Tweaked error messages to follow similar format --- care/users/api/serializers/user.py | 1 + care/users/api/viewsets/change_password.py | 2 +- care/users/api/viewsets/users.py | 6 +- care/users/tests/test_api.py | 105 ++++++++++++++++++++- 4 files changed, 108 insertions(+), 6 deletions(-) diff --git a/care/users/api/serializers/user.py b/care/users/api/serializers/user.py index c33a004bdf..332b6ab5b3 100644 --- a/care/users/api/serializers/user.py +++ b/care/users/api/serializers/user.py @@ -335,6 +335,7 @@ class Meta: "pf_auth", "read_profile_picture_url", "user_flags", + "last_login", ) read_only_fields = ( "is_superuser", diff --git a/care/users/api/viewsets/change_password.py b/care/users/api/viewsets/change_password.py index f5fc473fd2..e287818470 100644 --- a/care/users/api/viewsets/change_password.py +++ b/care/users/api/viewsets/change_password.py @@ -35,7 +35,7 @@ def update(self, request, *args, **kwargs): {"message": ["Username is required"]}, status=status.HTTP_400_BAD_REQUEST, ) - self.object = User.objects.get(username=username) + self.object = User.objects.filter(username=username).first() if not self.object: return Response( {"message": ["User not found"]}, status=status.HTTP_404_NOT_FOUND diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index 913c76abe3..e3b757e635 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -208,12 +208,12 @@ def destroy(self, request, *args, **kwargs): def get_user(self, request): username = request.query_params.get("username") if not username: - raise ValidationError({"username": "This field is required"}) + raise ValidationError({"message": "Username is required"}) user = User.objects.filter(username=username).first() if not user: - raise Http404({"user": "User not found"}) + raise Http404({"message": "User not found"}) if not self.has_permission(user): - raise ValidationError({"user": "Cannot Access Higher Level User"}) + raise ValidationError({"message": "User cannot access higher level user"}) return Response( status=status.HTTP_200_OK, data=UserSerializer(user, context={"request": request}).data, diff --git a/care/users/tests/test_api.py b/care/users/tests/test_api.py index 75b81de7c3..daad41381a 100644 --- a/care/users/tests/test_api.py +++ b/care/users/tests/test_api.py @@ -51,6 +51,7 @@ def get_detail_representation(self, obj=None) -> dict: "video_connect_link": obj.video_connect_link, "read_profile_picture_url": obj.profile_picture_url, "user_flags": [], + "last_login": obj.last_login, **self.get_local_body_district_state_representation(obj), } @@ -137,7 +138,13 @@ def setUpTestData(cls) -> None: cls.user_2 = cls.create_user(**cls.data_2) cls.data_3 = cls.get_user_data(cls.district) - cls.data_3.update({"username": "user_3", "password": "password"}) + 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) @@ -152,6 +159,17 @@ def setUpTestData(cls) -> None: 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 = cls.get_user_data(cls.district) + 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) + def test_user_can_access_url(self): """Test user can access the url by location""" username = self.user.username @@ -163,7 +181,7 @@ def test_user_can_read_all_users_within_accessible_facility(self): response = self.client.get("/api/v1/users/") self.assertEqual(response.status_code, status.HTTP_200_OK) res_data_json = response.json() - self.assertEqual(res_data_json["count"], 2) + self.assertEqual(res_data_json["count"], 3) results = res_data_json["results"] self.assertIn(self.user.id, {r["id"] for r in results}) self.assertIn(self.user_3.id, {r["id"] for r in results}) @@ -260,6 +278,89 @@ def test_user_with_districtadmin_access_can_change_password_of_others(self): ) self.assertEqual(response.status_code, status.HTTP_200_OK) + def test_user_with_districtadmin_access_cannot_change_password_of_others_without_username( + self, + ): + """Test a user with district admin access cannot change the password of other users without username""" + self.client.force_authenticate(self.user_4) + response = self.client.put( + "/api/v1/password_change/", + {"old_password": "password", "new_password": "password2"}, + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.data["message"][0], "Username is required") + + def test_user_with_district_admin_cannot_change_password_of_non_existing_user(self): + """Test a user with district admin access cannot change the password of a non existing user""" + self.client.force_authenticate(self.user_4) + response = self.client.put( + "/api/v1/password_change/", + { + "username": "foobar", + "old_password": "password", + "new_password": "password2", + }, + ) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.data["message"][0], "User not found") + + def test_user_with_district_admin_cannot_change_password_of_others_with_invalid_old_password( + self, + ): + """Test a user with district admin access cannot change the password of other users with invalid old password""" + self.client.force_authenticate(self.user_4) + response = self.client.put( + "/api/v1/password_change/", + { + "username": self.data_2["username"], + "old_password": "wrong_password", + "new_password": "password2", + }, + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual( + response.data["old_password"][0], + "Wrong password entered. Please check your password.", + ) + + def test_user_gets_error_when_accessing_user_details_without_username(self): + """Test a user gets error when accessing user details without username""" + 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") + + 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""" + response = self.client.get("/api/v1/users/get_user/", {"username": "foobar"}) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.data["message"], "User not found") + + def test_user_can_access_their_own_user_details(self): + """Test a user can access their own user details""" + response = self.client.get( + "/api/v1/users/get_user/", {"username": self.user.username} + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data["username"], self.user.username) + + def test_user_cannot_access_user_details_of_others(self): + """Test a user cannot access the details of other users above their hierarchy""" + self.client.force_authenticate(self.user_2) + username = self.data_3["username"] + response = self.client.get("/api/v1/users/get_user/", {"username": username}) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual( + response.data["message"], "User cannot access higher level user" + ) + + def test_user_with_higher_role_can_access_user_details_of_others(self): + """Test a user with higher role can access the details of other users below their hierarchy""" + self.client.force_authenticate(self.user_5) + username = self.data_2["username"] + response = self.client.get("/api/v1/users/get_user/", {"username": username}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data["username"], username) + class TestUserFilter(TestUtils, APITestCase): @classmethod From 9ff35103563eb725f31eea1c9155bbed2011f255 Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Thu, 14 Nov 2024 20:49:20 +0530 Subject: [PATCH 04/12] added last_login to read_only as well --- care/users/api/serializers/user.py | 1 + 1 file changed, 1 insertion(+) diff --git a/care/users/api/serializers/user.py b/care/users/api/serializers/user.py index 332b6ab5b3..96483c25c5 100644 --- a/care/users/api/serializers/user.py +++ b/care/users/api/serializers/user.py @@ -348,6 +348,7 @@ class Meta: "pf_endpoint", "pf_p256dh", "pf_auth", + "last_login", ) extra_kwargs = {"url": {"lookup_field": "username"}} From 8a0983a428c74a18598d3e17831d07483f8ae804 Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Mon, 18 Nov 2024 10:02:23 +0530 Subject: [PATCH 05/12] switched to using UserListView's get route --- care/users/api/viewsets/users.py | 46 +++++++++++++++++++------------- care/users/tests/test_api.py | 44 +++++++----------------------- 2 files changed, 36 insertions(+), 54 deletions(-) diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index e3b757e635..b53e2af1d8 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -91,6 +91,18 @@ def last_active_after(self, queryset, name, value): return queryset.filter(last_login__gte=date) +class UserViewSetPermission(DRYPermissions): + def has_permission(self, request, view): + if request.method == "GET" and "username" in view.kwargs: + return True + return super().has_permission(request, view) + + def has_object_permission(self, request, view, obj): + if request.method == "GET" and "username" in view.kwargs: + return True + return super().has_object_permission(request, view, obj) + + class UserViewSet( mixins.RetrieveModelMixin, mixins.UpdateModelMixin, @@ -113,7 +125,7 @@ class UserViewSet( queryset = queryset.filter(Q(asset__isnull=True)) lookup_field = "username" lookup_value_regex = "[^/]+" - permission_classes = (IsAuthenticated, DRYPermissions) + permission_classes = (IsAuthenticated, UserViewSetPermission) filter_backends = ( filters.DjangoFilterBackend, rest_framework_filters.OrderingFilter, @@ -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"): + username = self.request.query_params.get("username") + if not username: + raise ValidationError({"message": "Username is required"}) + user = get_object_or_404(self.get_queryset(), username=username) + if not self.has_permission(user): + raise ValidationError( + {"message": "You do not have permission to access this user"} + ) + return user return super().get_object() except Http404 as e: error = "User not found" @@ -203,29 +225,15 @@ def destroy(self, request, *args, **kwargs): user.save(update_fields=["is_active"]) return Response(status=status.HTTP_204_NO_CONTENT) - @extend_schema(tags=["users"]) - @action(detail=False, methods=["GET"]) - def get_user(self, request): - username = request.query_params.get("username") - if not username: - raise ValidationError({"message": "Username is required"}) - user = User.objects.filter(username=username).first() - if not user: - raise Http404({"message": "User not found"}) - if not self.has_permission(user): - raise ValidationError({"message": "User cannot access higher level user"}) - return Response( - status=status.HTTP_200_OK, - data=UserSerializer(user, context={"request": request}).data, - ) - def has_permission(self, user): requesting_user = self.request.user 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 + or ( + requesting_user.state == user.state + or requesting_user.district == user.district + ) ) @extend_schema(tags=["users"]) diff --git a/care/users/tests/test_api.py b/care/users/tests/test_api.py index daad41381a..5ac6e0a083 100644 --- a/care/users/tests/test_api.py +++ b/care/users/tests/test_api.py @@ -206,12 +206,19 @@ def test_user_can_modify_themselves(self): ) def test_user_cannot_read_others(self): - """Test 1 user can read the attributes of the other user""" + """Test 1 user can read the attributes of the other user not in the same ditrict/state""" username = self.data_2["username"] response = self.client.get(f"/api/v1/users/{username}/") self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) self.assertEqual(response.json()["detail"], "User not found") + def test_user_can_read_others_in_same_district_or_state(self): + """Test 1 user can read the attributes of the other user in the same district or state""" + username = self.user_3.username + response = self.client.get(f"/api/v1/users/{username}/") + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json()["first_name"], self.user_3.first_name) + def test_user_cannot_modify_others(self): """Test a user can't modify others""" username = self.data_2["username"] @@ -323,43 +330,10 @@ def test_user_with_district_admin_cannot_change_password_of_others_with_invalid_ "Wrong password entered. Please check your password.", ) - def test_user_gets_error_when_accessing_user_details_without_username(self): - """Test a user gets error when accessing user details without username""" - 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") - 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""" - response = self.client.get("/api/v1/users/get_user/", {"username": "foobar"}) + response = self.client.get("/api/v1/users/foobar/") self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - self.assertEqual(response.data["message"], "User not found") - - def test_user_can_access_their_own_user_details(self): - """Test a user can access their own user details""" - response = self.client.get( - "/api/v1/users/get_user/", {"username": self.user.username} - ) - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data["username"], self.user.username) - - def test_user_cannot_access_user_details_of_others(self): - """Test a user cannot access the details of other users above their hierarchy""" - self.client.force_authenticate(self.user_2) - username = self.data_3["username"] - response = self.client.get("/api/v1/users/get_user/", {"username": username}) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual( - response.data["message"], "User cannot access higher level user" - ) - - def test_user_with_higher_role_can_access_user_details_of_others(self): - """Test a user with higher role can access the details of other users below their hierarchy""" - self.client.force_authenticate(self.user_5) - username = self.data_2["username"] - response = self.client.get("/api/v1/users/get_user/", {"username": username}) - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data["username"], username) class TestUserFilter(TestUtils, APITestCase): From 7c6ee37ea1edf5e683641d146fa8416645c45916 Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Mon, 25 Nov 2024 20:53:14 +0530 Subject: [PATCH 06/12] changed password perms --- care/users/api/viewsets/change_password.py | 13 +-- care/users/tests/test_api.py | 119 ++++++++++----------- 2 files changed, 62 insertions(+), 70 deletions(-) diff --git a/care/users/api/viewsets/change_password.py b/care/users/api/viewsets/change_password.py index e287818470..febd15cc16 100644 --- a/care/users/api/viewsets/change_password.py +++ b/care/users/api/viewsets/change_password.py @@ -1,4 +1,5 @@ from django.contrib.auth import get_user_model +from django.shortcuts import get_object_or_404 from drf_spectacular.utils import extend_schema, extend_schema_view from rest_framework import serializers, status from rest_framework.generics import UpdateAPIView @@ -35,11 +36,7 @@ def update(self, request, *args, **kwargs): {"message": ["Username is required"]}, status=status.HTTP_400_BAD_REQUEST, ) - self.object = User.objects.filter(username=username).first() - if not self.object: - return Response( - {"message": ["User not found"]}, status=status.HTTP_404_NOT_FOUND - ) + self.object = get_object_or_404(User, username=username) if not self.has_permission(request, self.object): return Response( { @@ -70,8 +67,4 @@ def update(self, request, *args, **kwargs): def has_permission(self, request, user): authuser = request.user - return ( - authuser == user - or authuser.is_superuser - or authuser.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"] - ) + return authuser == user or authuser.is_superuser diff --git a/care/users/tests/test_api.py b/care/users/tests/test_api.py index 5ac6e0a083..b52a72e7a1 100644 --- a/care/users/tests/test_api.py +++ b/care/users/tests/test_api.py @@ -22,6 +22,10 @@ def setUpTestData(cls) -> None: cls.user = cls.create_user("staff1", cls.district) cls.user_data = cls.get_user_data(cls.district, 40) + cls.data_2 = cls.get_user_data(cls.district) + cls.data_2.update({"username": "user_2", "password": "password"}) + cls.user_2 = cls.create_user(**cls.data_2) + def setUp(self): self.client.force_authenticate(self.super_user) @@ -107,6 +111,61 @@ def test_superuser_can_delete(self): deleted=False, ) + def test_superuser_can_change_password_of_others(self): + """Test a user with superuser access can change the password of other users underneath the hierarchy""" + username = self.data_2["username"] + password = self.data_2["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) + + def test_superuser_cannot_change_password_of_others_without_username( + self, + ): + """Test a user with superuser access cannot change the password of other users without username""" + response = self.client.put( + "/api/v1/password_change/", + {"old_password": "password", "new_password": "password2"}, + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.data["message"][0], "Username is required") + + def test_superuser_cannot_change_password_of_non_existing_user(self): + """Test a user with superuser access cannot change the password of a non existing user""" + response = self.client.put( + "/api/v1/password_change/", + { + "username": "foobar", + "old_password": "password", + "new_password": "password2", + }, + ) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + + def test_superuser_cannot_change_password_of_others_with_invalid_old_password( + self, + ): + """Test a user with superuser access cannot change the password of other users with invalid old password""" + response = self.client.put( + "/api/v1/password_change/", + { + "username": self.data_2["username"], + "old_password": "wrong_password", + "new_password": "password2", + }, + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual( + response.data["old_password"][0], + "Wrong password entered. Please check your password.", + ) + class TestUser(TestUtils, APITestCase): def get_detail_representation(self, obj=None) -> dict: @@ -270,66 +329,6 @@ def test_user_with_districtadmin_access_can_modify_others(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json()["date_of_birth"], "2005-04-01") - 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"] - 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) - - def test_user_with_districtadmin_access_cannot_change_password_of_others_without_username( - self, - ): - """Test a user with district admin access cannot change the password of other users without username""" - self.client.force_authenticate(self.user_4) - response = self.client.put( - "/api/v1/password_change/", - {"old_password": "password", "new_password": "password2"}, - ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.data["message"][0], "Username is required") - - def test_user_with_district_admin_cannot_change_password_of_non_existing_user(self): - """Test a user with district admin access cannot change the password of a non existing user""" - self.client.force_authenticate(self.user_4) - response = self.client.put( - "/api/v1/password_change/", - { - "username": "foobar", - "old_password": "password", - "new_password": "password2", - }, - ) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - self.assertEqual(response.data["message"][0], "User not found") - - def test_user_with_district_admin_cannot_change_password_of_others_with_invalid_old_password( - self, - ): - """Test a user with district admin access cannot change the password of other users with invalid old password""" - self.client.force_authenticate(self.user_4) - response = self.client.put( - "/api/v1/password_change/", - { - "username": self.data_2["username"], - "old_password": "wrong_password", - "new_password": "password2", - }, - ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual( - response.data["old_password"][0], - "Wrong password entered. Please check your password.", - ) - 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""" response = self.client.get("/api/v1/users/foobar/") From b0067fdd5d60cf5bcc9730084dd113c0621c4570 Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Wed, 27 Nov 2024 23:30:48 +0530 Subject: [PATCH 07/12] filter facility users by search fields --- care/facility/api/viewsets/facility_users.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/care/facility/api/viewsets/facility_users.py b/care/facility/api/viewsets/facility_users.py index ce4cf71c91..09e2f6ba08 100644 --- a/care/facility/api/viewsets/facility_users.py +++ b/care/facility/api/viewsets/facility_users.py @@ -16,7 +16,6 @@ class UserFilter(filters.FilterSet): choices=[(key, key) for key in User.TYPE_VALUE_MAP], coerce=lambda role: User.TYPE_VALUE_MAP[role], ) - username = filters.CharFilter(field_name="username", lookup_expr="icontains") class Meta: model = User @@ -36,12 +35,21 @@ class FacilityUserViewSet(GenericViewSet, mixins.ListModelMixin): def get_queryset(self): try: + search_fields = { + key: self.request.query_params.get(key) + for key in self.search_fields + if self.request.query_params.get(key) + } facility = Facility.objects.get( external_id=self.kwargs.get("facility_external_id"), ) queryset = facility.users.filter( deleted=False, ).order_by("-last_login") + if search_fields: + queryset = queryset.filter( + **{key: value for key, value in search_fields.items() if value} + ) return queryset.prefetch_related( Prefetch( "skills", From 41563b683bbf71e06404d4516dae53cdc472fcb9 Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Fri, 29 Nov 2024 00:40:02 +0530 Subject: [PATCH 08/12] add test for coverage --- care/facility/tests/test_facilityuser_api.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/care/facility/tests/test_facilityuser_api.py b/care/facility/tests/test_facilityuser_api.py index 9ade78e875..5958ee9f39 100644 --- a/care/facility/tests/test_facilityuser_api.py +++ b/care/facility/tests/test_facilityuser_api.py @@ -20,6 +20,9 @@ def setUpTestData(cls) -> None: cls.facility2 = cls.create_facility( cls.super_user, cls.district, cls.local_body ) + cls.user2 = cls.create_user( + "dummystaff", cls.district, home_facility=cls.facility2 + ) def setUp(self) -> None: self.client.force_authenticate(self.super_user) @@ -32,6 +35,15 @@ def test_get_queryset_with_prefetching(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertNumQueries(2) + def test_get_queryset_with_search(self): + response = self.client.get( + f"/api/v1/facility/{self.facility2.external_id}/get_users/?username={self.user2.username}" + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(response.data["results"]), 1) + self.assertEqual(response.data["results"][0]["username"], self.user2.username) + def test_link_new_facility(self): response = self.client.get("/api/v1/facility/") From c38cfc3c585cd41e771d8ee1580a3b249d0b9837 Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Tue, 3 Dec 2024 01:42:28 +0530 Subject: [PATCH 09/12] simplified perms for get --- care/users/api/viewsets/users.py | 28 +++++----------------------- care/users/tests/test_api.py | 19 +++++++------------ care/utils/tests/test_utils.py | 6 +++--- 3 files changed, 15 insertions(+), 38 deletions(-) diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index b53e2af1d8..a036057a62 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -93,12 +93,12 @@ def last_active_after(self, queryset, name, value): class UserViewSetPermission(DRYPermissions): def has_permission(self, request, view): - if request.method == "GET" and "username" in view.kwargs: + if request.method == "GET" and view.action == "retrieve": return True return super().has_permission(request, view) def has_object_permission(self, request, view, obj): - if request.method == "GET" and "username" in view.kwargs: + if request.method == "GET" and view.action == "retrieve": return True return super().has_object_permission(request, view, obj) @@ -167,16 +167,9 @@ def get_queryset(self): def get_object(self) -> User: try: - if self.request.method == "GET" and not self.kwargs.get("username"): - username = self.request.query_params.get("username") - if not username: - raise ValidationError({"message": "Username is required"}) - user = get_object_or_404(self.get_queryset(), username=username) - if not self.has_permission(user): - raise ValidationError( - {"message": "You do not have permission to access this user"} - ) - return user + if self.request.method == "GET" and self.action == "retrieve": + username = self.kwargs.get("username") + return get_object_or_404(User, username=username) return super().get_object() except Http404 as e: error = "User not found" @@ -225,17 +218,6 @@ def destroy(self, request, *args, **kwargs): user.save(update_fields=["is_active"]) return Response(status=status.HTTP_204_NO_CONTENT) - 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 - ) - ) - @extend_schema(tags=["users"]) @action(detail=False, methods=["POST"]) def add_user(self, request, *args, **kwargs): diff --git a/care/users/tests/test_api.py b/care/users/tests/test_api.py index b52a72e7a1..1de09c69eb 100644 --- a/care/users/tests/test_api.py +++ b/care/users/tests/test_api.py @@ -72,9 +72,11 @@ def test_superuser_can_view(self): data = self.user_data.copy() data["date_of_birth"] = str(data["date_of_birth"]) data.pop("password") + user_data = self.get_detail_representation(self.user) + user_data.pop("created_by") self.assertDictEqual( res_data_json, - self.get_detail_representation(self.user), + user_data, ) def test_superuser_can_modify(self): @@ -264,19 +266,12 @@ def test_user_can_modify_themselves(self): User.objects.get(username=username).date_of_birth, date(2005, 4, 1) ) - def test_user_cannot_read_others(self): - """Test 1 user can read the attributes of the other user not in the same ditrict/state""" - username = self.data_2["username"] - response = self.client.get(f"/api/v1/users/{username}/") - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - self.assertEqual(response.json()["detail"], "User not found") - - def test_user_can_read_others_in_same_district_or_state(self): - """Test 1 user can read the attributes of the other user in the same district or state""" - username = self.user_3.username + def test_user_can_read_others(self): + """Test 1 user can read the attributes of any other user""" + username = self.user_2.username response = self.client.get(f"/api/v1/users/{username}/") self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.json()["first_name"], self.user_3.first_name) + self.assertEqual(response.json()["first_name"], self.user_2.first_name) def test_user_cannot_modify_others(self): """Test a user can't modify others""" diff --git a/care/utils/tests/test_utils.py b/care/utils/tests/test_utils.py index fbc286a337..99faed645b 100644 --- a/care/utils/tests/test_utils.py +++ b/care/utils/tests/test_utils.py @@ -764,7 +764,7 @@ def get_local_body_district_state_representation(self, obj): response.update(self.get_state_representation(getattr(obj, "state", None))) return response - def get_local_body_representation(self, local_body: LocalBody): + def get_local_body_representation(self, local_body: LocalBody | None): if local_body is None: return {"local_body": None, "local_body_object": None} return { @@ -778,7 +778,7 @@ def get_local_body_representation(self, local_body: LocalBody): }, } - def get_district_representation(self, district: District): + def get_district_representation(self, district: District | None): if district is None: return {"district": None, "district_object": None} return { @@ -790,7 +790,7 @@ def get_district_representation(self, district: District): }, } - def get_state_representation(self, state: State): + def get_state_representation(self, state: State | None): if state is None: return {"state": None, "state_object": None} return {"state": state.id, "state_object": {"id": state.id, "name": state.name}} From 5b6397c0570f23830adda611d7dcd6359230dcad Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Mon, 9 Dec 2024 14:57:11 +0530 Subject: [PATCH 10/12] reverting unneeded changes --- care/facility/api/viewsets/facility_users.py | 10 +--- care/users/api/viewsets/change_password.py | 22 +------- care/users/api/viewsets/users.py | 17 +----- care/users/models.py | 2 +- care/users/tests/test_api.py | 57 +------------------- 5 files changed, 5 insertions(+), 103 deletions(-) diff --git a/care/facility/api/viewsets/facility_users.py b/care/facility/api/viewsets/facility_users.py index 09e2f6ba08..ce4cf71c91 100644 --- a/care/facility/api/viewsets/facility_users.py +++ b/care/facility/api/viewsets/facility_users.py @@ -16,6 +16,7 @@ class UserFilter(filters.FilterSet): choices=[(key, key) for key in User.TYPE_VALUE_MAP], coerce=lambda role: User.TYPE_VALUE_MAP[role], ) + username = filters.CharFilter(field_name="username", lookup_expr="icontains") class Meta: model = User @@ -35,21 +36,12 @@ class FacilityUserViewSet(GenericViewSet, mixins.ListModelMixin): def get_queryset(self): try: - search_fields = { - key: self.request.query_params.get(key) - for key in self.search_fields - if self.request.query_params.get(key) - } facility = Facility.objects.get( external_id=self.kwargs.get("facility_external_id"), ) queryset = facility.users.filter( deleted=False, ).order_by("-last_login") - if search_fields: - queryset = queryset.filter( - **{key: value for key, value in search_fields.items() if value} - ) return queryset.prefetch_related( Prefetch( "skills", diff --git a/care/users/api/viewsets/change_password.py b/care/users/api/viewsets/change_password.py index febd15cc16..805bebddd9 100644 --- a/care/users/api/viewsets/change_password.py +++ b/care/users/api/viewsets/change_password.py @@ -1,5 +1,4 @@ from django.contrib.auth import get_user_model -from django.shortcuts import get_object_or_404 from drf_spectacular.utils import extend_schema, extend_schema_view from rest_framework import serializers, status from rest_framework.generics import UpdateAPIView @@ -30,22 +29,7 @@ class ChangePasswordView(UpdateAPIView): model = User def update(self, request, *args, **kwargs): - username = request.data.get("username") - if not username: - return Response( - {"message": ["Username is required"]}, - status=status.HTTP_400_BAD_REQUEST, - ) - self.object = get_object_or_404(User, username=username) - if not self.has_permission(request, self.object): - return Response( - { - "message": [ - "User does not have elevated permissions to change password" - ] - }, - status=status.HTTP_403_FORBIDDEN, - ) + self.object = self.request.user serializer = self.get_serializer(data=request.data) if serializer.is_valid(): @@ -64,7 +48,3 @@ def update(self, request, *args, **kwargs): return Response({"message": "Password updated successfully"}) return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) - - def has_permission(self, request, user): - authuser = request.user - return authuser == user or authuser.is_superuser diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index a036057a62..a70241c855 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -91,18 +91,6 @@ def last_active_after(self, queryset, name, value): return queryset.filter(last_login__gte=date) -class UserViewSetPermission(DRYPermissions): - def has_permission(self, request, view): - if request.method == "GET" and view.action == "retrieve": - return True - return super().has_permission(request, view) - - def has_object_permission(self, request, view, obj): - if request.method == "GET" and view.action == "retrieve": - return True - return super().has_object_permission(request, view, obj) - - class UserViewSet( mixins.RetrieveModelMixin, mixins.UpdateModelMixin, @@ -125,7 +113,7 @@ class UserViewSet( queryset = queryset.filter(Q(asset__isnull=True)) lookup_field = "username" lookup_value_regex = "[^/]+" - permission_classes = (IsAuthenticated, UserViewSetPermission) + permission_classes = (IsAuthenticated, DRYPermissions) filter_backends = ( filters.DjangoFilterBackend, rest_framework_filters.OrderingFilter, @@ -167,9 +155,6 @@ def get_queryset(self): def get_object(self) -> User: try: - if self.request.method == "GET" and self.action == "retrieve": - username = self.kwargs.get("username") - return get_object_or_404(User, username=username) return super().get_object() except Http404 as e: error = "User not found" diff --git a/care/users/models.py b/care/users/models.py index bf0d47c284..9ea5d0cf43 100644 --- a/care/users/models.py +++ b/care/users/models.py @@ -364,7 +364,7 @@ def has_read_permission(request): return True def has_object_read_permission(self, request): - return request.user.is_superuser or self == request.user + return True @staticmethod def has_write_permission(request): diff --git a/care/users/tests/test_api.py b/care/users/tests/test_api.py index 1de09c69eb..5411c8f1d4 100644 --- a/care/users/tests/test_api.py +++ b/care/users/tests/test_api.py @@ -113,61 +113,6 @@ def test_superuser_can_delete(self): deleted=False, ) - def test_superuser_can_change_password_of_others(self): - """Test a user with superuser access can change the password of other users underneath the hierarchy""" - username = self.data_2["username"] - password = self.data_2["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) - - def test_superuser_cannot_change_password_of_others_without_username( - self, - ): - """Test a user with superuser access cannot change the password of other users without username""" - response = self.client.put( - "/api/v1/password_change/", - {"old_password": "password", "new_password": "password2"}, - ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.data["message"][0], "Username is required") - - def test_superuser_cannot_change_password_of_non_existing_user(self): - """Test a user with superuser access cannot change the password of a non existing user""" - response = self.client.put( - "/api/v1/password_change/", - { - "username": "foobar", - "old_password": "password", - "new_password": "password2", - }, - ) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - - def test_superuser_cannot_change_password_of_others_with_invalid_old_password( - self, - ): - """Test a user with superuser access cannot change the password of other users with invalid old password""" - response = self.client.put( - "/api/v1/password_change/", - { - "username": self.data_2["username"], - "old_password": "wrong_password", - "new_password": "password2", - }, - ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual( - response.data["old_password"][0], - "Wrong password entered. Please check your password.", - ) - class TestUser(TestUtils, APITestCase): def get_detail_representation(self, obj=None) -> dict: @@ -309,7 +254,7 @@ def test_user_cannot_change_password_of_others(self): "new_password": "password2", }, ) - self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) def test_user_with_districtadmin_access_can_modify_others(self): """Test a user with district admin access can modify others underneath the hierarchy""" From 5291acdfeb4e181fdfde50caf919bcddc978ada4 Mon Sep 17 00:00:00 2001 From: Jacobjohnjeevan Date: Mon, 9 Dec 2024 15:23:21 +0530 Subject: [PATCH 11/12] fix --- care/users/api/viewsets/users.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index a70241c855..72303fbc25 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -155,6 +155,9 @@ def get_queryset(self): def get_object(self) -> User: try: + if self.request.method == "GET" and self.action == "retrieve": + username = self.kwargs.get("username") + return get_object_or_404(User, username=username) return super().get_object() except Http404 as e: error = "User not found" From 107e547ecf122717c7fa9d7c2263aca43c34b55f Mon Sep 17 00:00:00 2001 From: Vignesh Hari <14056798+vigneshhari@users.noreply.github.com> Date: Tue, 10 Dec 2024 12:22:34 +0530 Subject: [PATCH 12/12] Remove redundant check --- care/users/api/viewsets/users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index 72303fbc25..d6c434decb 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -155,7 +155,7 @@ def get_queryset(self): def get_object(self) -> User: try: - if self.request.method == "GET" and self.action == "retrieve": + if self.action == "retrieve": username = self.kwargs.get("username") return get_object_or_404(User, username=username) return super().get_object()