diff --git a/api/features/permissions.py b/api/features/permissions.py index 3ddd9fcf1812..e116eee86657 100644 --- a/api/features/permissions.py +++ b/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) diff --git a/api/permissions/permission_service.py b/api/permissions/permission_service.py index 774a09fe89d6..aa938401f228 100644 --- a/api/permissions/permission_service.py +++ b/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,7 +77,7 @@ 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 ) @@ -84,8 +85,12 @@ def get_permitted_projects_for_user( 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: diff --git a/api/tests/unit/features/feature_segments/test_unit_feature_segments_views.py b/api/tests/unit/features/feature_segments/test_unit_feature_segments_views.py index 0bfc0a895375..b2b55e3d303f 100644 --- a/api/tests/unit/features/feature_segments/test_unit_feature_segments_views.py +++ b/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, diff --git a/api/tests/unit/features/test_unit_features_permissions.py b/api/tests/unit/features/test_unit_features_permissions.py index 95812efc4770..e1dc4f864e64 100644 --- a/api/tests/unit/features/test_unit_features_permissions.py +++ b/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, diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 20f15cf00b6a..c0496b26f497 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/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 diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index a5b67952ecc7..a59d5f39d82d 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/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(