From 3732e67f2dc1c0010ad5d4796960af2ddedf90c9 Mon Sep 17 00:00:00 2001
From: Zach Aysan <zachaysan@gmail.com>
Date: Mon, 6 Nov 2023 15:37:31 -0500
Subject: [PATCH] fix: Tighten ACL for user routes (#2929)

---
 api/organisations/tests/test_views.py | 42 +++++++++++++++++++++++++++
 api/users/views.py                    |  8 ++---
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/api/organisations/tests/test_views.py b/api/organisations/tests/test_views.py
index 5aefb206c8d6..edd8b4d1608d 100644
--- a/api/organisations/tests/test_views.py
+++ b/api/organisations/tests/test_views.py
@@ -1157,6 +1157,27 @@ def test_make_user_group_admin_success(
     )
 
 
+def test_make_user_group_admin_forbidden(
+    staff_client: FFAdminUser,
+    organisation: Organisation,
+    user_permission_group: UserPermissionGroup,
+):
+    # Given
+    another_user = FFAdminUser.objects.create(email="another_user@example.com")
+    another_user.add_organisation(organisation)
+    another_user.permission_groups.add(user_permission_group)
+    url = reverse(
+        "api-v1:organisations:make-user-group-admin",
+        args=[organisation.id, user_permission_group.id, another_user.id],
+    )
+
+    # When
+    response = staff_client.post(url)
+
+    # Then
+    assert response.status_code == status.HTTP_403_FORBIDDEN
+
+
 def test_remove_user_as_group_admin_user_does_not_belong_to_group(
     admin_client, admin_user, organisation, user_permission_group
 ):
@@ -1202,6 +1223,27 @@ def test_remove_user_as_group_admin_success(
     )
 
 
+def test_remove_user_as_group_admin_forbidden(
+    staff_client: FFAdminUser,
+    organisation: Organisation,
+    user_permission_group: UserPermissionGroup,
+):
+    # Given
+    another_user = FFAdminUser.objects.create(email="another_user@example.com")
+    another_user.add_organisation(organisation)
+    another_user.permission_groups.add(user_permission_group)
+    another_user.make_group_admin(user_permission_group.id)
+    url = reverse(
+        "api-v1:organisations:remove-user-group-admin",
+        args=[organisation.id, user_permission_group.id, another_user.id],
+    )
+
+    # When
+    response = staff_client.post(url)
+    # Then
+    assert response.status_code == status.HTTP_403_FORBIDDEN
+
+
 def test_list_user_groups_as_group_admin(organisation, api_client):
     # Given
     user1 = FFAdminUser.objects.create(email="user1@example.com")
diff --git a/api/users/views.py b/api/users/views.py
index aa3b0ef1b95e..00bc2030288f 100644
--- a/api/users/views.py
+++ b/api/users/views.py
@@ -245,11 +245,11 @@ def my_groups(self, request: Request, organisation_pk: int) -> Response:
         return self.list(request, organisation_pk)
 
 
-@permission_classes([IsAuthenticated(), NestedIsOrganisationAdminPermission()])
 @api_view(["POST"])
+@permission_classes([IsAuthenticated, NestedIsOrganisationAdminPermission])
 def make_user_group_admin(
     request: Request, organisation_pk: int, group_pk: int, user_pk: int
-):
+) -> Response:
     user = get_object_or_404(
         FFAdminUser,
         userorganisation__organisation_id=organisation_pk,
@@ -260,11 +260,11 @@ def make_user_group_admin(
     return Response()
 
 
-@permission_classes([IsAuthenticated(), NestedIsOrganisationAdminPermission()])
 @api_view(["POST"])
+@permission_classes([IsAuthenticated, NestedIsOrganisationAdminPermission])
 def remove_user_as_group_admin(
     request: Request, organisation_pk: int, group_pk: int, user_pk: int
-):
+) -> Response:
     user = get_object_or_404(
         FFAdminUser,
         userorganisation__organisation_id=organisation_pk,