From a0eefdd3223cb0e684eb4aed36f21b9ea4f9d370 Mon Sep 17 00:00:00 2001
From: Zach Aysan <zachaysan@gmail.com>
Date: Wed, 8 Nov 2023 09:55:08 -0500
Subject: [PATCH] fix: Check that feature owners are able to view the project
 of a feature (#2931)

---
 api/features/views.py                         | 10 +++-
 .../unit/features/test_unit_features_views.py | 51 ++++++++++++++-----
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/api/features/views.py b/api/features/views.py
index cbfe227f9f0b..f53beca91f9b 100644
--- a/api/features/views.py
+++ b/api/features/views.py
@@ -35,7 +35,7 @@
 )
 from projects.models import Project
 from projects.permissions import VIEW_PROJECT
-from users.models import UserPermissionGroup
+from users.models import FFAdminUser, UserPermissionGroup
 from webhooks.webhooks import WebhookEventType
 
 from .models import Feature, FeatureState
@@ -202,8 +202,14 @@ def remove_group_owners(self, request, *args, **kwargs):
     def add_owners(self, request, *args, **kwargs):
         serializer = FeatureOwnerInputSerializer(data=request.data)
         serializer.is_valid(raise_exception=True)
-
         feature = self.get_object()
+
+        for user in FFAdminUser.objects.filter(
+            id__in=serializer.validated_data["user_ids"]
+        ):
+            if not user.has_project_permission(VIEW_PROJECT, feature.project):
+                raise serializers.ValidationError("Some users not found")
+
         serializer.add_owners(feature)
         return Response(self.get_serializer(instance=feature).data)
 
diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py
index e51813c9f45f..08ce52da849f 100644
--- a/api/tests/unit/features/test_unit_features_views.py
+++ b/api/tests/unit/features/test_unit_features_views.py
@@ -16,6 +16,7 @@
 from features.multivariate.models import MultivariateFeatureOption
 from organisations.models import Organisation, OrganisationRole
 from projects.models import Project, UserProjectPermission
+from projects.permissions import VIEW_PROJECT
 from segments.models import Segment
 from users.models import FFAdminUser, UserPermissionGroup
 
@@ -529,9 +530,11 @@ def test_should_create_tags_when_feature_created(client, project, tag_one, tag_t
     "client",
     [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")],
 )
-def test_add_owners_adds_owner(client, project):
+def test_add_owners_fails_if_user_not_found(client, project):
     # Given
     feature = Feature.objects.create(name="Test Feature", project=project)
+
+    # Users have no association to the project or organisation.
     user_1 = FFAdminUser.objects.create_user(email="user1@mail.com")
     user_2 = FFAdminUser.objects.create_user(email="user2@mail.com")
     url = reverse(
@@ -541,24 +544,48 @@ def test_add_owners_adds_owner(client, project):
     data = {"user_ids": [user_1.id, user_2.id]}
 
     # When
-    json_response = client.post(
-        url, data=json.dumps(data), content_type="application/json"
-    ).json()
+    response = client.post(url, data=json.dumps(data), content_type="application/json")
+    # Then
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert response.data == ["Some users not found"]
+    assert feature.owners.filter(id__in=[user_1.id, user_2.id]).count() == 0
+
+
+@pytest.mark.parametrize(
+    "client",
+    [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")],
+)
+def test_add_owners_adds_owner(staff_user, admin_user, client, project):
+    # Given
+    feature = Feature.objects.create(name="Test Feature", project=project)
+    UserProjectPermission.objects.create(
+        user=staff_user, project=project
+    ).add_permission(VIEW_PROJECT)
+
+    url = reverse(
+        "api-v1:projects:project-features-add-owners",
+        args=[project.id, feature.id],
+    )
+    data = {"user_ids": [staff_user.id, admin_user.id]}
+
+    # When
+    response = client.post(url, data=json.dumps(data), content_type="application/json")
 
+    json_response = response.json()
     # Then
     assert len(json_response["owners"]) == 2
     assert json_response["owners"][0] == {
-        "id": user_1.id,
-        "email": user_1.email,
-        "first_name": user_1.first_name,
-        "last_name": user_1.last_name,
+        "id": staff_user.id,
+        "email": staff_user.email,
+        "first_name": staff_user.first_name,
+        "last_name": staff_user.last_name,
         "last_login": None,
     }
     assert json_response["owners"][1] == {
-        "id": user_2.id,
-        "email": user_2.email,
-        "first_name": user_2.first_name,
-        "last_name": user_2.last_name,
+        "id": admin_user.id,
+        "email": admin_user.email,
+        "first_name": admin_user.first_name,
+        "last_name": admin_user.last_name,
         "last_login": None,
     }