diff --git a/api/permissions/permission_service.py b/api/permissions/permission_service.py index 07b25b4124c4..ad1ab41cb86f 100644 --- a/api/permissions/permission_service.py +++ b/api/permissions/permission_service.py @@ -199,26 +199,47 @@ def get_permitted_environments_for_master_api_key( def user_has_organisation_permission( user: "FFAdminUser", organisation: Organisation, permission_key: str ) -> bool: + """ + Check if user has the given permission on an organisation. + + Runs separate queries with early returns: + 1. Organisation admin - admins hold every organisation permission. + 2. Organisation membership - check to prevent orphaned permission + records from granting access. + 3. Direct user permission - checks UserOrganisationPermission. + 4. Group permission - checks via user's group memberships. + 5. Role permission - RBAC check, only if enabled. + """ if is_user_organisation_admin(user, organisation): return True + # Check: verify user belongs to the organisation + if not Organisation.objects.filter(id=organisation.id, users=user).exists(): + return False + # NOTE: since we store organisation admin slightly differently - # compared to project and environment `get_base_permission_filter` - # with allow_admin=True will not work for organisation - base_filter = get_base_permission_filter( - user, - Organisation, # type: ignore[arg-type] - permission_key, - allow_admin=False, - ) - filter_ = base_filter & Q(id=organisation.id) + # compared to project and environment, allow_admin=True will not + # work for organisation + + # Check direct permission + user_filter = get_user_permission_filter(user, permission_key, allow_admin=False) + if Organisation.objects.filter(user_filter & Q(id=organisation.id)).exists(): + return True - queryset = Organisation.objects.filter(filter_) + # Check group permission + group_filter = get_group_permission_filter(user, permission_key, allow_admin=False) + if Organisation.objects.filter(group_filter & Q(id=organisation.id)).exists(): + return True - # Final check to verify that user belongs to organisation - queryset = queryset.filter(users=user) + # Check role permission (only if RBAC installed) + if settings.IS_RBAC_INSTALLED: # pragma: no cover + role_filter = get_role_permission_filter( + user, Organisation, permission_key, allow_admin=False + ) + if Organisation.objects.filter(role_filter & Q(id=organisation.id)).exists(): + return True - return queryset.exists() # type: ignore[no-any-return] + return False def master_api_key_has_organisation_permission( diff --git a/api/tests/unit/permissions/permission_service/test_user_has_organisation_permissions.py b/api/tests/unit/permissions/permission_service/test_user_has_organisation_permissions.py index 1798892cc473..1706fbe64420 100644 --- a/api/tests/unit/permissions/permission_service/test_user_has_organisation_permissions.py +++ b/api/tests/unit/permissions/permission_service/test_user_has_organisation_permissions.py @@ -1,3 +1,5 @@ +import typing + from organisations.models import Organisation, UserOrganisation from organisations.permissions.models import ( OrganisationPermissionModel, @@ -156,3 +158,97 @@ def test_user_has_organisation_permission__user_removed_from_organisation__retur organisation=organisation, permission_key=CREATE_PROJECT, ) + + +def test_user_has_organisation_permission__direct_permission__short_circuits_in_three_queries( + staff_user: FFAdminUser, + organisation: Organisation, + django_assert_num_queries: typing.Any, +) -> None: + # Given + user_org_permission = UserOrganisationPermission.objects.create( + user=staff_user, organisation=organisation + ) + user_org_permission.permissions.add(CREATE_PROJECT) # type: ignore[arg-type] + + # When + # Should take only 3 queries: + # 1. Check if user is org admin (is_user_organisation_admin) + # 2. Check organisation membership + # 3. Check direct user permission (short-circuits here) + with django_assert_num_queries(3): + result = user_has_organisation_permission( + staff_user, organisation, CREATE_PROJECT + ) + + # Then + assert result is True + + +def test_user_has_organisation_permission__group_permission__short_circuits_in_four_queries( + staff_user: FFAdminUser, + organisation: Organisation, + user_permission_group: UserPermissionGroup, + django_assert_num_queries: typing.Any, +) -> None: + # Given + user_permission_group.users.add(staff_user) + group_org_permission = UserPermissionGroupOrganisationPermission.objects.create( + group=user_permission_group, organisation=organisation + ) + group_org_permission.permissions.add(CREATE_PROJECT) # type: ignore[arg-type] + + # When + # Should take only 4 queries: + # 1. Check if user is org admin (is_user_organisation_admin) + # 2. Check organisation membership + # 3. Check direct user permission (not found) + # 4. Check group permission (short-circuits here) + with django_assert_num_queries(4): + result = user_has_organisation_permission( + staff_user, organisation, CREATE_PROJECT + ) + + # Then + assert result is True + + +def test_user_has_organisation_permission__no_permissions_assigned__checks_each_source_in_four_queries( + staff_user: FFAdminUser, + organisation: Organisation, + django_assert_num_queries: typing.Any, +) -> None: + # Given / When + # Should take exactly 4 queries, one per permission source — never a + # single combined query joining the user and group permission tables: + # 1. Check if user is org admin (is_user_organisation_admin) + # 2. Check organisation membership + # 3. Check direct user permission (not found) + # 4. Check group permission (not found; role check skipped without RBAC) + with django_assert_num_queries(4): + result = user_has_organisation_permission( + staff_user, organisation, MANAGE_USER_GROUPS + ) + + # Then + assert result is False + + +def test_user_has_organisation_permission__user_not_in_organisation__short_circuits_in_two_queries( + organisation: Organisation, + django_assert_num_queries: typing.Any, +) -> None: + # Given + user = FFAdminUser.objects.create(email="not-a-member@example.com") + + # When + # Should take only 2 queries: + # 1. Check if user is org admin (is_user_organisation_admin) + # 2. Check organisation membership (short-circuits here) + with django_assert_num_queries(2): + result = user_has_organisation_permission( + user, organisation, MANAGE_USER_GROUPS + ) + + # Then + assert result is False diff --git a/api/tests/unit/users/test_unit_users_models.py b/api/tests/unit/users/test_unit_users_models.py index 475a28af9647..e95e622d62dc 100644 --- a/api/tests/unit/users/test_unit_users_models.py +++ b/api/tests/unit/users/test_unit_users_models.py @@ -9,7 +9,10 @@ from organisations.permissions.permissions import ORGANISATION_PERMISSIONS from projects.models import Project from tests.types import WithProjectPermissionsCallable -from users.models import FFAdminUser, UserPermissionGroup +from users.models import ( + FFAdminUser, + UserPermissionGroup, +) def test_belongs_to__user_in_organisation__returns_true(