diff --git a/backend/backend/graphene/mutations/service_accounts.py b/backend/backend/graphene/mutations/service_accounts.py index c09e9a69b..41ed0ae33 100644 --- a/backend/backend/graphene/mutations/service_accounts.py +++ b/backend/backend/graphene/mutations/service_accounts.py @@ -332,6 +332,16 @@ def mutate(cls, root, info, service_account_id, name, role_id, identity_ids=None service_account.name = name service_account.role = role if identity_ids is not None: + # Binding an identity lets it mint tokens for this account, so + # it needs ExternalIdentities access on top of SA update — the + # same pair the UI gates the control on. + if not user_has_permission( + user, "read", "ExternalIdentities", service_account.organisation + ): + raise GraphQLError( + "You don't have permission to manage External Identities " + "in this organisation" + ) identities = Identity.objects.filter( id__in=identity_ids, organisation=service_account.organisation, diff --git a/backend/backend/graphene/types.py b/backend/backend/graphene/types.py index 6d65c35b0..79837cb1c 100644 --- a/backend/backend/graphene/types.py +++ b/backend/backend/graphene/types.py @@ -1011,6 +1011,13 @@ def resolve_network_policies(self, info): return list(chain(account_policies, global_policies)) def resolve_identities(self, info): + # Return an empty list instead of raising — a field error here would fail + # the whole Service Account query for members who can legitimately + # view the account, just without ExternalIdentities access. + if not user_has_permission( + info.context.user, "read", "ExternalIdentities", self.organisation + ): + return [] return self.identities.filter(deleted_at=None) diff --git a/backend/tests/api/test_service_account_identities_access.py b/backend/tests/api/test_service_account_identities_access.py new file mode 100644 index 000000000..31f6b1e92 --- /dev/null +++ b/backend/tests/api/test_service_account_identities_access.py @@ -0,0 +1,147 @@ +"""External Identity access checks on the Service Account detail path. + +A member who can reach a Service Account (org-level `ServiceAccounts.read` +or team-based access) but has no `ExternalIdentities` permission — the +default Developer role, or any custom role that leaves the resource empty +— was still served the account's linked identities, and the console fired +an ungated `identities` query at them, producing a permission error toast +on page load. + +The resolver now withholds the rows silently (an exception here would +fail the whole Service Account query), and the update mutation refuses to +bind identities without ExternalIdentities access — the one point where +the user genuinely has to be blocked. +""" + +from unittest.mock import MagicMock, patch + +import pytest +from graphql import GraphQLError + + +_MUTATIONS = "backend.graphene.mutations.service_accounts" + + +def _info(user): + info = MagicMock() + info.context.user = user + return info + + +def _make_sa(): + sa = MagicMock() + sa.organisation = MagicMock() + return sa + + +@patch("backend.graphene.types.user_has_permission", return_value=False) +def test_resolve_identities_returns_empty_without_permission(mock_perm): + """No ExternalIdentities.read → no identity rows, and no error, so + the rest of the Service Account query still resolves.""" + from backend.graphene.types import ServiceAccountType + + sa = _make_sa() + user = MagicMock() + + result = ServiceAccountType.resolve_identities(sa, _info(user)) + + assert result == [] + sa.identities.filter.assert_not_called() + + +@patch("backend.graphene.types.user_has_permission", return_value=True) +def test_resolve_identities_returns_rows_when_permitted(mock_perm): + from backend.graphene.types import ServiceAccountType + + sa = _make_sa() + user = MagicMock() + expected_qs = MagicMock() + sa.identities.filter.return_value = expected_qs + + result = ServiceAccountType.resolve_identities(sa, _info(user)) + + assert result is expected_qs + sa.identities.filter.assert_called_once_with(deleted_at=None) + # The gate must match the org-level `identities` query's gate. + args, _kwargs = mock_perm.call_args + assert args[0] is user + assert args[1] == "read" + assert args[2] == "ExternalIdentities" + assert args[3] is sa.organisation + + +def _run_update(identity_ids, has_identity_permission): + from backend.graphene.mutations.service_accounts import ( + UpdateServiceAccountMutation, + ) + + sa = _make_sa() + user = MagicMock() + + with patch(f"{_MUTATIONS}.ServiceAccount") as mock_sa_cls, patch( + f"{_MUTATIONS}.Role" + ) as mock_role_cls, patch(f"{_MUTATIONS}._check_sa_permission"), patch( + f"{_MUTATIONS}.role_has_global_access", return_value=False + ), patch( + f"{_MUTATIONS}.user_has_permission", return_value=has_identity_permission + ) as mock_perm, patch( + f"{_MUTATIONS}.Identity" + ) as mock_identity_cls: + mock_sa_cls.objects.get.return_value = sa + mock_role_cls.objects.get.return_value = MagicMock(name="role") + + try: + UpdateServiceAccountMutation.mutate( + None, + _info(user), + service_account_id="sa-1", + name="account", + role_id="role-1", + identity_ids=identity_ids, + ) + raised = None + except GraphQLError as e: + raised = e + + return sa, mock_perm, mock_identity_cls, raised + + +def test_update_rejects_identity_binding_without_permission(): + """The one place blocking is warranted — and nothing is persisted.""" + sa, _perm, mock_identity_cls, raised = _run_update( + identity_ids=["idn-1"], has_identity_permission=False + ) + + assert raised is not None + assert "External Identities" in str(raised) + mock_identity_cls.objects.filter.assert_not_called() + sa.identities.set.assert_not_called() + sa.save.assert_not_called() + + +def test_update_binds_identities_when_permitted(): + sa, mock_perm, mock_identity_cls, raised = _run_update( + identity_ids=["idn-1"], has_identity_permission=True + ) + + assert raised is None + sa.identities.set.assert_called_once_with( + mock_identity_cls.objects.filter.return_value + ) + sa.save.assert_called_once() + args, _kwargs = mock_perm.call_args + assert args[1] == "read" + assert args[2] == "ExternalIdentities" + + +def test_update_without_identity_ids_needs_no_identity_permission(): + """Renaming or re-roling an account must not start demanding + ExternalIdentities access.""" + sa, mock_perm, _identity_cls, raised = _run_update( + identity_ids=None, has_identity_permission=False + ) + + assert raised is None + mock_perm.assert_not_called() + sa.identities.set.assert_not_called() + sa.save.assert_called_once() diff --git a/frontend/app/[team]/access/service-accounts/[account]/_components/ServiceAccountIdentities.tsx b/frontend/app/[team]/access/service-accounts/[account]/_components/ServiceAccountIdentities.tsx index 07e73b158..e104189ab 100644 --- a/frontend/app/[team]/access/service-accounts/[account]/_components/ServiceAccountIdentities.tsx +++ b/frontend/app/[team]/access/service-accounts/[account]/_components/ServiceAccountIdentities.tsx @@ -2,6 +2,7 @@ import { ServiceAccountType } from '@/apollo/graphql' import { organisationContext } from '@/contexts/organisationContext' +import { userHasPermission } from '@/utils/access/permissions' import { useContext, useMemo, useRef, useState } from 'react' import { Button } from '@/components/common/Button' import { EmptyState } from '@/components/common/EmptyState' @@ -16,17 +17,37 @@ import { toast } from 'react-toastify' import { KeyManagementDialog } from '@/components/service-accounts/KeyManagementDialog' import UpdateServiceAccount from '@/graphql/mutations/service-accounts/updateServiceAccount.gql' import { TbLockShare } from 'react-icons/tb' -import { FaSearch, FaTimesCircle, FaServer } from 'react-icons/fa' +import { FaSearch, FaTimesCircle, FaServer, FaBan } from 'react-icons/fa' import clsx from 'clsx' import GenericDialog from '@/components/common/GenericDialog' import { MdSearchOff } from 'react-icons/md' import Link from 'next/link' -export const ServiceAccountIdentities = ({ account }: { account: ServiceAccountType }) => { +export const ServiceAccountIdentities = ({ + account, + canManageAccount = false, +}: { + account: ServiceAccountType + canManageAccount?: boolean +}) => { const { activeOrganisation: organisation } = useContext(organisationContext) + + // External Identities are an org-level resource, so read access comes from + // the org role rather than the SA's team-scoped role. + const userCanReadIdentities = organisation + ? userHasPermission(organisation.role!.permissions, 'ExternalIdentities', 'read') + : false + + // Attaching identities mutates the Service Account, so managing them needs + // both: visibility of the identities, and update access on this account. + const userCanManageIdentities = userCanReadIdentities && canManageAccount + + // Skip rather than let the resolver raise — an ungated query here throws a + // permission error toast at anyone who can reach this page without + // ExternalIdentities.read (e.g. a Developer with team-based SA access). const { data } = useQuery(GetOrganisationIdentities, { variables: { organisationId: organisation?.id }, - skip: !organisation, + skip: !organisation || !userCanReadIdentities, }) const dialogRef = useRef<{ openModal: () => void; closeModal: () => void }>(null) @@ -57,29 +78,64 @@ export const ServiceAccountIdentities = ({ account }: { account: ServiceAccountT ) const handleSave = async () => { - await updateAccount({ - variables: { - serviceAccountId: account.id, - name: account.name, - roleId: account.role!.id, - identityIds: Array.from(selected), - }, - refetchQueries: [ - { query: GetServiceAccountDetail, variables: { orgId: organisation?.id, id: account.id } }, - ], - }) + try { + await updateAccount({ + variables: { + serviceAccountId: account.id, + name: account.name, + roleId: account.role!.id, + identityIds: Array.from(selected), + }, + refetchQueries: [ + { + query: GetServiceAccountDetail, + variables: { orgId: organisation?.id, id: account.id }, + }, + ], + }) + } catch { + // Surfaced by the global Apollo error link + return + } closeManageIdentitiesDialog() toast.success('Updated identities for this account') } + const sectionHeader = ( + <> +