fix(get_permitted_projects): get rid of distinct (#4320)

api/features/permissions.py

@@ -45,6 +45,10 @@ def has_permission(self, request, view):
             # handled by has_object_permission
             return True
 
+        if view.action in ["list"]:
+            # handled by the view
+            return True
+
         try:
             project_id = view.kwargs.get("project_pk") or request.data.get("project")
             project = Project.objects.get(id=project_id)

api/permissions/permission_service.py

@@ -1,5 +1,6 @@
-from typing import TYPE_CHECKING, List, Union
+from typing import TYPE_CHECKING, List, Set, Union
 
+from django.conf import settings
 from django.db.models import Q, QuerySet
 
 from environments.models import Environment
@@ -76,16 +77,20 @@ def get_permitted_projects_for_user(
         - If `tag_ids` is a list of tag IDs, only project with one of those tags will
         be returned
     """
-    base_filter = get_base_permission_filter(
+    project_ids_from_base_filter = get_object_id_from_base_permission_filter(
         user, Project, permission_key, tag_ids=tag_ids
     )
 
     organisation_filter = Q(
         organisation__userorganisation__user=user,
         organisation__userorganisation__role=OrganisationRole.ADMIN.name,
     )
-    filter_ = base_filter | organisation_filter
-    return Project.objects.filter(filter_).distinct()
+    project_ids_from_organisation = Project.objects.filter(
+        organisation_filter
+    ).values_list("id", flat=True)
+
+    project_ids = project_ids_from_base_filter | set(project_ids_from_organisation)
+    return Project.objects.filter(id__in=project_ids)
 
 
 def get_permitted_projects_for_master_api_key(
@@ -220,6 +225,34 @@ def get_base_permission_filter(
     return user_filter | group_filter | role_filter
 
 
+def get_object_id_from_base_permission_filter(
+    user: "FFAdminUser",
+    for_model: Union[Organisation, Project, Environment] = None,
+    permission_key: str = None,
+    allow_admin: bool = True,
+    tag_ids=None,
+) -> Set[int]:
+    object_ids = set()
+    user_filter = get_user_permission_filter(user, permission_key, allow_admin)
+    object_ids.update(
+        list(for_model.objects.filter(user_filter).values_list("id", flat=True))
+    )
+
+    group_filter = get_group_permission_filter(user, permission_key, allow_admin)
+
+    object_ids.update(
+        list(for_model.objects.filter(group_filter).values_list("id", flat=True))
+    )
+    if settings.IS_RBAC_INSTALLED:  # pragma: no cover
+        role_filter = get_role_permission_filter(
+            user, for_model, permission_key, allow_admin, tag_ids
+        )
+        object_ids.update(
+            list(for_model.objects.filter(role_filter).values_list("id", flat=True))
+        )
+    return object_ids
+
+
 def get_user_permission_filter(
     user: "FFAdminUser", permission_key: str = None, allow_admin: bool = True
 ) -> Q:

api/tests/unit/features/feature_segments/test_unit_feature_segments_views.py

@@ -32,8 +32,8 @@
     [
         (
             lazy_fixture("admin_client"),
-            3,
-        ),  # 1 for paging, 1 for result, 1 for getting the current live version
+            6,
+        ),  # 1 for paging, 3 for permissions, 1 for result, 1 for getting the current live version
         (
             lazy_fixture("admin_master_api_key_client"),
             4,

api/tests/unit/features/test_unit_features_permissions.py

@@ -85,26 +85,6 @@ def test_project_user_with_read_access_can_list_features(
     assert result is True
 
 
-def test_user_with_no_project_permissions_cannot_list_features(
-    staff_user: FFAdminUser,
-    project: Project,
-) -> None:
-    # Given
-    feature_permissions = FeaturePermissions()
-    mock_view = mock.MagicMock(
-        kwargs={"project_pk": project.id},
-        detail=False,
-        action="list",
-    )
-    mock_request = mock.MagicMock(data={}, user=staff_user)
-
-    # When
-    result = feature_permissions.has_permission(mock_request, mock_view)
-
-    # Then
-    assert result is False
-
-
 def test_organisation_admin_can_create_feature(
     admin_user: FFAdminUser,
     project: Project,

api/tests/unit/features/test_unit_features_views.py

@@ -2600,13 +2600,32 @@ def test_list_features_n_plus_1(
         v1_feature_state.clone(env=environment, version=i, live_from=timezone.now())
 
     # When
-    with django_assert_num_queries(17):
+    with django_assert_num_queries(16):
         response = staff_client.get(url)
 
     # Then
     assert response.status_code == status.HTTP_200_OK
 
 
+def test_list_features_from_different_project_returns_404(
+    staff_client: APIClient,
+    organisation_two_project_two: Project,
+    with_project_permissions: WithProjectPermissionsCallable,
+) -> None:
+    # Given
+    with_project_permissions([VIEW_PROJECT])
+
+    url = reverse(
+        "api-v1:projects:project-features-list", args=[organisation_two_project_two.id]
+    )
+
+    # When
+    response = staff_client.get(url)
+
+    # Then
+    assert response.status_code == status.HTTP_404_NOT_FOUND
+
+
 def test_list_features_with_union_tag(
     staff_client: APIClient,
     project: Project,
@@ -2808,7 +2827,7 @@ def test_list_features_with_feature_state(
     url = f"{base_url}?environment={environment.id}"
 
     # When
-    with django_assert_num_queries(17):
+    with django_assert_num_queries(16):
         response = staff_client.get(url)
 
     # Then
@@ -3102,7 +3121,7 @@ def test_feature_list_last_modified_values(
         Feature.objects.create(name=f"feature_{i}", project=project)
 
     # When
-    with django_assert_num_queries(19):  # TODO: reduce this number of queries!
+    with django_assert_num_queries(18):  # TODO: reduce this number of queries!
         response = staff_client.get(url)
 
     # Then

api/tests/unit/segments/test_unit_segments_views.py

@@ -366,7 +366,7 @@ def test_get_segment_by_uuid(client, project, segment):
     "client, num_queries",
     [
         (lazy_fixture("admin_master_api_key_client"), 12),
-        (lazy_fixture("admin_client"), 11),
+        (lazy_fixture("admin_client"), 14),
     ],
 )
 def test_list_segments(