From e69a5b4f773acd7e0f5f762f34560f9fead7e928 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 19 Oct 2023 17:19:07 +0100 Subject: [PATCH 1/3] Add endpoint to list group summaries --- api/users/views.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/api/users/views.py b/api/users/views.py index 3e79ebab8db8..8b3c107ebcad 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -162,14 +162,16 @@ def get_queryset(self): organisation = Organisation.objects.get(id=organisation_pk) qs = UserPermissionGroup.objects.filter(organisation=organisation) - if not self.request.user.has_organisation_permission( - organisation, MANAGE_USER_GROUPS + if ( + self.action != "summaries" + and not self.request.user.has_organisation_permission( + organisation, MANAGE_USER_GROUPS + ) ): + # my-groups and summaries return a very cut down set of data, we can safely allow all users + # of the groups / organisation to retrieve them in this case, otherwise they must be a group admin. q = Q(userpermissiongroupmembership__ffadminuser=self.request.user) if self.action != "my_groups": - # my-groups returns a very cut down set of data, we can safely allow all users - # of the groups to retrieve them in this case, otherwise they must be a group - # admin. q = q & Q(userpermissiongroupmembership__group_admin=True) qs = qs.filter(q) @@ -178,7 +180,7 @@ def get_queryset(self): def get_serializer_class(self): if self.action == "retrieve": return UserPermissionGroupSerializerDetail - elif self.action == "my_groups": + elif self.action in ("my_groups", "summaries"): return MyUserPermissionGroupsSerializer return ListUserPermissionGroupSerializer @@ -244,6 +246,13 @@ def my_groups(self, request: Request, organisation_pk: int) -> Response: """ return self.list(request, organisation_pk) + @action(detail=False, methods=["GET"], url_path="summaries") + def summaries(self, request: Request, organisation_pk: int) -> Response: + """ + Returns a list of summary group objects for all groups in the organisation. + """ + return self.list(request, organisation_pk) + @permission_classes([IsAuthenticated(), NestedIsOrganisationAdminPermission()]) @api_view(["POST"]) From a9bd765906c62f3a8a7ebc52a92009a018c4399d Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 20 Oct 2023 17:07:40 +0100 Subject: [PATCH 2/3] Add tests --- api/organisations/tests/test_views.py | 55 +++++++++++++++++++++++++++ api/users/views.py | 4 +- 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/api/organisations/tests/test_views.py b/api/organisations/tests/test_views.py index 5aefb206c8d6..be9ec66d16cc 100644 --- a/api/organisations/tests/test_views.py +++ b/api/organisations/tests/test_views.py @@ -1284,3 +1284,58 @@ def test_list_my_groups(organisation, api_client): "id": user_permission_group_1.id, "name": user_permission_group_1.name, } + + +def test_list_group_summaries( + organisation: Organisation, admin_client: APIClient +) -> None: + # Given + user_permission_group_1 = UserPermissionGroup.objects.create( + organisation=organisation, name="group1" + ) + user_permission_group_2 = UserPermissionGroup.objects.create( + organisation=organisation, name="group2" + ) + + url = reverse( + "api-v1:organisations:organisation-groups-summaries", args=[organisation.id] + ) + + # When + response = admin_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["count"] == 2 + assert response_json["results"][0] == { + "id": user_permission_group_1.id, + "name": user_permission_group_1.name, + } + assert response_json["results"][1] == { + "id": user_permission_group_2.id, + "name": user_permission_group_2.name, + } + + +def test_user_from_another_organisation_cannot_list_group_summaries( + organisation: Organisation, api_client: APIClient +) -> None: + # Given + UserPermissionGroup.objects.create(organisation=organisation, name="group1") + + organisation_2 = Organisation.objects.create(name="org2") + org2_user = FFAdminUser.objects.create(email="org2user@example.com") + org2_user.add_organisation(organisation_2) + api_client.force_authenticate(org2_user) + + url = reverse( + "api-v1:organisations:organisation-groups-summaries", args=[organisation.id] + ) + + # When + response = api_client.get(url) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN diff --git a/api/users/views.py b/api/users/views.py index 8b3c107ebcad..844c0abe0bb9 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -168,7 +168,7 @@ def get_queryset(self): organisation, MANAGE_USER_GROUPS ) ): - # my-groups and summaries return a very cut down set of data, we can safely allow all users + # my_groups and summaries return a very cut down set of data, we can safely allow all users # of the groups / organisation to retrieve them in this case, otherwise they must be a group admin. q = Q(userpermissiongroupmembership__ffadminuser=self.request.user) if self.action != "my_groups": @@ -246,7 +246,7 @@ def my_groups(self, request: Request, organisation_pk: int) -> Response: """ return self.list(request, organisation_pk) - @action(detail=False, methods=["GET"], url_path="summaries") + @action(detail=False, methods=["GET"]) def summaries(self, request: Request, organisation_pk: int) -> Response: """ Returns a list of summary group objects for all groups in the organisation. From 3980872231c8924624b4ba81e8ef26ff45e19b52 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 30 Nov 2023 18:51:01 +0000 Subject: [PATCH 3/3] Fix permissions issue --- api/organisations/permissions/permissions.py | 2 +- api/tests/unit/organisations/test_unit_organisations_views.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/organisations/permissions/permissions.py b/api/organisations/permissions/permissions.py index 7207c4cba286..82e42bf32420 100644 --- a/api/organisations/permissions/permissions.py +++ b/api/organisations/permissions/permissions.py @@ -127,7 +127,7 @@ def has_permission(self, request, view): return False return ( - view.action in ("list", "my_groups") + view.action in ("list", "my_groups", "summaries") and request.user.belongs_to(organisation.id) or view.detail is True # delegate to has_object_permission / get_queryset or request.user.has_organisation_permission( diff --git a/api/tests/unit/organisations/test_unit_organisations_views.py b/api/tests/unit/organisations/test_unit_organisations_views.py index db62b151b398..6b4a5f3d82eb 100644 --- a/api/tests/unit/organisations/test_unit_organisations_views.py +++ b/api/tests/unit/organisations/test_unit_organisations_views.py @@ -1624,7 +1624,7 @@ def test_list_organisations_shows_dunning( def test_list_group_summaries( - organisation: Organisation, admin_client: APIClient + organisation: Organisation, staff_client: APIClient ) -> None: # Given user_permission_group_1 = UserPermissionGroup.objects.create( @@ -1639,7 +1639,7 @@ def test_list_group_summaries( ) # When - response = admin_client.get(url) + response = staff_client.get(url) # Then assert response.status_code == status.HTTP_200_OK