From 2b4f1dd03857d19e4966bf3e9eaa6b69697e1d33 Mon Sep 17 00:00:00 2001 From: Jacob John Jeevan <40040905+Jacobjeevan@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:24:50 +0530 Subject: [PATCH] Modified User Route for User Management Redesign (#2595) * 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 * added tests * 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 * added last_login to read_only as well * switched to using UserListView's get route * changed password perms * filter facility users by search fields * add test for coverage * simplified perms for get * reverting unneeded changes * fix * Remove redundant check --------- Co-authored-by: Vignesh Hari <14056798+vigneshhari@users.noreply.github.com> --- care/facility/api/viewsets/facility_users.py | 1 + care/facility/tests/test_facilityuser_api.py | 12 +++ care/users/api/serializers/user.py | 2 + care/users/api/viewsets/users.py | 3 + care/users/models.py | 2 +- care/users/tests/test_api.py | 83 ++++++++++++++++++-- care/utils/tests/test_utils.py | 6 +- 7 files changed, 97 insertions(+), 12 deletions(-) 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/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/") diff --git a/care/users/api/serializers/user.py b/care/users/api/serializers/user.py index c33a004bdf..96483c25c5 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", @@ -347,6 +348,7 @@ class Meta: "pf_endpoint", "pf_p256dh", "pf_auth", + "last_login", ) extra_kwargs = {"url": {"lookup_field": "username"}} diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index a70241c855..d6c434decb 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.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 109120d71b..5411c8f1d4 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) @@ -51,6 +55,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), } @@ -67,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): @@ -137,10 +144,38 @@ 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) + 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) + + 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 @@ -152,7 +187,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}) @@ -176,12 +211,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""" - username = self.data_2["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_404_NOT_FOUND) - self.assertEqual(response.json()["detail"], "User not found") + self.assertEqual(response.status_code, status.HTTP_200_OK) + 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""" @@ -207,6 +242,38 @@ 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_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""" + 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_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/") + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + class TestUserFilter(TestUtils, APITestCase): @classmethod 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}}