From 05353a5fbe661d504abdede8875f450c4cc8dce5 Mon Sep 17 00:00:00 2001
From: Matthew Elwell <matthew.elwell@flagsmith.com>
Date: Thu, 18 Apr 2024 10:15:20 +0100
Subject: [PATCH] fix: prevent unauthorised remove-users access (#3791)

---
 api/organisations/permissions/permissions.py  | 13 +++-
 .../test_unit_organisations_views.py          | 63 +++++++++++++++++++
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/api/organisations/permissions/permissions.py b/api/organisations/permissions/permissions.py
index be39851d669c..c962d8eda77e 100644
--- a/api/organisations/permissions/permissions.py
+++ b/api/organisations/permissions/permissions.py
@@ -5,7 +5,7 @@
 from django.core.exceptions import ObjectDoesNotExist
 from django.db.models import Model
 from django.views import View
-from rest_framework.exceptions import PermissionDenied
+from rest_framework.exceptions import PermissionDenied, ValidationError
 from rest_framework.permissions import BasePermission, IsAuthenticated
 from rest_framework.request import Request
 
@@ -88,6 +88,17 @@ class OrganisationPermission(BasePermission):
     def has_permission(self, request, view):
         if view.action == "create" and settings.RESTRICT_ORG_CREATE_TO_SUPERUSERS:
             return request.user.is_superuser
+
+        organisation_id = view.kwargs.get("pk")
+        if organisation_id and not organisation_id.isnumeric():
+            raise ValidationError("Invalid organisation ID")
+
+        if view.action == "remove_users":
+            return request.user.is_organisation_admin(int(organisation_id))
+
+        if organisation_id:
+            return request.user.belongs_to(int(organisation_id))
+
         return True
 
     def has_object_permission(self, request, view, obj):
diff --git a/api/tests/unit/organisations/test_unit_organisations_views.py b/api/tests/unit/organisations/test_unit_organisations_views.py
index 87c9a27729cb..865a2396104f 100644
--- a/api/tests/unit/organisations/test_unit_organisations_views.py
+++ b/api/tests/unit/organisations/test_unit_organisations_views.py
@@ -1797,3 +1797,66 @@ def test_doesnt_retrieve_stale_api_usage_notifications(
     # Then
     assert response.status_code == status.HTTP_200_OK
     assert len(response.data["results"]) == 0
+
+
+def test_non_organisation_user_cannot_remove_user_from_organisation(
+    staff_user: FFAdminUser, organisation: Organisation, api_client: APIClient
+) -> None:
+    # Given
+    another_organisation = Organisation.objects.create(name="another organisation")
+    another_user = FFAdminUser.objects.create(email="another_user@example.com")
+    another_user.add_organisation(another_organisation)
+    api_client.force_authenticate(another_user)
+
+    url = reverse(
+        "api-v1:organisations:organisation-remove-users", args=[organisation.id]
+    )
+
+    data = [{"id": staff_user.id}]
+
+    # When
+    response = api_client.post(
+        url, data=json.dumps(data), content_type="application/json"
+    )
+
+    # Then
+    assert response.status_code == status.HTTP_403_FORBIDDEN
+
+
+def test_non_admin_user_cannot_remove_user_from_organisation(
+    staff_user: FFAdminUser,
+    organisation: Organisation,
+    staff_client: APIClient,
+    admin_user: FFAdminUser,
+) -> None:
+    # Given
+    url = reverse(
+        "api-v1:organisations:organisation-remove-users", args=[organisation.id]
+    )
+
+    data = [{"id": admin_user.id}]
+
+    # When
+    response = staff_client.post(
+        url, data=json.dumps(data), content_type="application/json"
+    )
+
+    # Then
+    assert response.status_code == status.HTTP_403_FORBIDDEN
+
+
+def test_validation_error_if_non_numeric_organisation_id(
+    staff_client: APIClient,
+) -> None:
+    # Given
+    url = reverse("api-v1:organisations:organisation-remove-users", args=["foo"])
+
+    data = []
+
+    # When
+    response = staff_client.post(
+        url, data=json.dumps(data), content_type="application/json"
+    )
+
+    # Then
+    assert response.status_code == status.HTTP_400_BAD_REQUEST