fix: improve _is_user_object_admin query performance (#6537)

Author: gagantrivedi

api/permissions/permission_service.py

@@ -233,18 +233,53 @@ def master_api_key_has_organisation_permission(
 def _is_user_object_admin(
     user: "FFAdminUser", object_: Union[Project, Environment]
 ) -> bool:
-    ModelClass = type(object_)
-    base_filter = get_base_permission_filter(user, ModelClass)  # type: ignore[arg-type]
-    filter_ = base_filter & Q(id=object_.id)
-
-    if ModelClass is Project:
-        filter_ = filter_ & Q(organisation__users=user)
-    elif ModelClass is Environment:
-        filter_ = filter_ & Q(project__organisation__users=user)
+    """
+    Check if user has admin permission on a project or environment.
+
+    Runs separate queries with early returns:
+    1. Organisation membership - check to prevent orphaned permission
+       records from granting access.
+    2. Direct user permission - checks UserProjectPermission/UserEnvironmentPermission.
+    3. Group permission - checks via user's group memberships.
+    4. Role permission - RBAC check, only if enabled.
+
+    Returns as soon as permission is found, avoiding unnecessary subsequent queries.
+
+    """
+    model_class = type(object_)
+    object_id = object_.id
+
+    # Check: verify user belongs to the organisation that owns this object
+    if model_class is Project:
+        if not Project.objects.filter(id=object_id, organisation__users=user).exists():
+            return False
+    elif model_class is Environment:
+        if not Environment.objects.filter(
+            id=object_id, project__organisation__users=user
+        ).exists():
+            return False
     else:  # pragma: no cover
-        raise ValueError(f"Unexpected object type {type(object_)}")
+        raise ValueError(f"Unexpected object type {model_class}")
+
+    # Check direct permission
+    user_filter = get_user_permission_filter(user, allow_admin=True)
+    if model_class.objects.filter(user_filter & Q(id=object_id)).exists():
+        return True
 
-    return ModelClass.objects.filter(filter_).exists()
+    # Check group permission
+    group_filter = get_group_permission_filter(user, allow_admin=True)
+    if model_class.objects.filter(group_filter & Q(id=object_id)).exists():
+        return True
+
+    # Check role permission (only if RBAC installed)
+    if settings.IS_RBAC_INSTALLED:  # pragma: no cover
+        role_filter = get_role_permission_filter(
+            user, model_class, permission_key=None, allow_admin=True, tag_ids=None
+        )
+        if model_class.objects.filter(role_filter & Q(id=object_id)).exists():
+            return True
+
+    return False
 
 
 def get_base_permission_filter(  # type: ignore[no-untyped-def]

api/tests/unit/environments/test_unit_environments_views.py

@@ -717,7 +717,7 @@ def test_view_environment_with_staff__query_count_is_expected_without_rbac(
         environment_metadata_b,
         required_a_environment_metadata_field,
         environment_content_type,
-        expected_query_count=9,
+        expected_query_count=11,
     )
 
 
@@ -746,7 +746,7 @@ def test_view_environment_with_staff__query_count_is_expected_with_rbac(
         environment_metadata_b,
         required_a_environment_metadata_field,
         environment_content_type,
-        expected_query_count=10,
+        expected_query_count=12,
     )
 
 

api/tests/unit/permissions/permission_service/conftest.py

@@ -1,5 +1,6 @@
 import pytest
 
+from environments.models import Environment
 from environments.permissions.models import (
     UserEnvironmentPermission,
     UserPermissionGroupEnvironmentPermission,
@@ -43,7 +44,9 @@ def project_admin_via_user_permission_group(  # type: ignore[no-untyped-def]
 
 @pytest.fixture
 def environment_admin_via_user_permission(
-    staff_user: FFAdminUser, user_environment_permission: UserEnvironmentPermission
+    staff_user: FFAdminUser,
+    environment: Environment,  # Explicitly depend on environment to ensure same instance
+    user_environment_permission: UserEnvironmentPermission,
 ) -> UserEnvironmentPermission:
     user_environment_permission.admin = True
     user_environment_permission.save()
@@ -61,6 +64,7 @@ def environment_admin_via_user_permission_group(
     user_environment_permission_group: UserPermissionGroupEnvironmentPermission,
     staff_user: FFAdminUser,
     user_permission_group: UserPermissionGroup,
+    environment: Environment,  # Explicitly depend on environment to ensure same instance
 ) -> UserPermissionGroupEnvironmentPermission:
     user_permission_group.users.add(staff_user)
 

api/tests/unit/permissions/permission_service/test_is_user_environment_admin.py

@@ -134,3 +134,38 @@ def test_is_user_environment_admin__does_not_return_environment_for_orphan_group
 
     # Then
     assert not is_user_environment_admin(user=staff_user, environment=environment)
+
+
+def test_is_user_environment_admin__short_circuits_on_direct_permission(
+    staff_user: FFAdminUser,
+    environment: Environment,
+    environment_admin_via_user_permission: UserEnvironmentPermission,
+    django_assert_num_queries: typing.Any,
+) -> None:
+    # When/Then - should take only 6 queries:
+    # 1. Check if user is org admin (is_user_organisation_admin)
+    # 2. Check organisation membership for project (_is_user_object_admin)
+    # 3. Check direct user permission on project (not found)
+    # 4. Check group permission on project (not found)
+    # 5. Check organisation membership for environment (_is_user_object_admin)
+    # 6. Check direct user permission on environment (short-circuits here)
+    with django_assert_num_queries(6):
+        assert is_user_environment_admin(staff_user, environment) is True
+
+
+def test_is_user_environment_admin__short_circuits_on_group_permission(
+    staff_user: FFAdminUser,
+    environment: Environment,
+    environment_admin_via_user_permission_group: UserPermissionGroupEnvironmentPermission,
+    django_assert_num_queries: typing.Any,
+) -> None:
+    # When/Then - should take only 7 queries:
+    # 1. Check if user is org admin (is_user_organisation_admin)
+    # 2. Check organisation membership for project (_is_user_object_admin)
+    # 3. Check direct user permission on project (not found)
+    # 4. Check group permission on project (not found)
+    # 5. Check organisation membership for environment (_is_user_object_admin)
+    # 6. Check direct user permission on environment (not found)
+    # 7. Check group permission on environment (short-circuits here)
+    with django_assert_num_queries(7):
+        assert is_user_environment_admin(staff_user, environment) is True

api/tests/unit/permissions/permission_service/test_is_user_project_admin.py

@@ -96,3 +96,32 @@ def test_is_user_project_admin__does_not_return_project_for_orphan_group_permiss
 
     # Then
     assert not is_user_project_admin(user=staff_user, project=project)
+
+
+def test_is_user_project_admin__short_circuits_on_direct_permission(
+    staff_user: FFAdminUser,
+    project: Project,
+    project_admin_via_user_permission: UserProjectPermission,
+    django_assert_num_queries: typing.Any,
+) -> None:
+    # When/Then - should take only 3 queries:
+    # 1. Check if user is org admin (is_user_organisation_admin)
+    # 2. Check organisation membership (_is_user_object_admin)
+    # 3. Check direct user permission (short-circuits here)
+    with django_assert_num_queries(3):
+        assert is_user_project_admin(staff_user, project) is True
+
+
+def test_is_user_project_admin__short_circuits_on_group_permission(
+    staff_user: FFAdminUser,
+    project: Project,
+    project_admin_via_user_permission_group: UserPermissionGroupProjectPermission,
+    django_assert_num_queries: typing.Any,
+) -> None:
+    # When/Then - should take only 4 queries:
+    # 1. Check if user is org admin (is_user_organisation_admin)
+    # 2. Check organisation membership (_is_user_object_admin)
+    # 3. Check direct user permission (not found)
+    # 4. Check group permission (short-circuits here)
+    with django_assert_num_queries(4):
+        assert is_user_project_admin(staff_user, project) is True