From e60236762519cd222a608bc160a074e7e793d323 Mon Sep 17 00:00:00 2001 From: Nimish Date: Tue, 25 Aug 2026 19:01:34 +0530 Subject: [PATCH] fix: lock the External Identities section instead of erroring at members without access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any member who could reach a Service Account detail page but had no ExternalIdentities permission got a "You don't have permission to read identities in this organisation" toast on page load, with nothing they were trying to do. ServiceAccountIdentities fired GetOrganisationIdentities unconditionally, the resolver raises on missing permission, and the global Apollo error link toasts every GraphQL error. The default Developer role hits this: no org-level ServiceAccounts permission, but team-based access to the account, and ExternalIdentities empty. Custom roles with ServiceAccounts.read and no identity access hit it too. The identities page and the network-policy component already gated their queries this way — this component was the outlier. Frontend: - Skip the query without ExternalIdentities.read and render an "Access restricted" state for that section only. The rest of the page loads normally and nothing is toasted. - Gate the manage controls on read access plus update-on-this-account (effectiveCanUpdateSA && hasTeamAccess), so no one is offered a control whose mutation would be rejected. - Don't report success from handleSave when the mutation was rejected. Backend, so the gate isn't only cosmetic: - ServiceAccountType.resolve_identities withholds the rows without permission. It returns [] rather than raising: a field error there would fail the whole Service Account query and put the toast back. - updateServiceAccount now requires ExternalIdentities access to bind identities — ServiceAccounts.update alone was enough before. The check only runs when identityIds is passed, so renames and role changes are unaffected. --- .../graphene/mutations/service_accounts.py | 10 ++ backend/backend/graphene/types.py | 7 + .../test_service_account_identities_access.py | 147 ++++++++++++++++++ .../_components/ServiceAccountIdentities.tsx | 116 ++++++++++---- .../service-accounts/[account]/page.tsx | 5 +- 5 files changed, 257 insertions(+), 28 deletions(-) create mode 100644 backend/tests/api/test_service_account_identities_access.py diff --git a/backend/backend/graphene/mutations/service_accounts.py b/backend/backend/graphene/mutations/service_accounts.py index 8c38da51a..dc77902cb 100644 --- a/backend/backend/graphene/mutations/service_accounts.py +++ b/backend/backend/graphene/mutations/service_accounts.py @@ -305,6 +305,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 dd18f6177..21a2489dc 100644 --- a/backend/backend/graphene/types.py +++ b/backend/backend/graphene/types.py @@ -908,6 +908,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 = ( + <> +
External Identities
+
+ Manage which external identities are trusted for this account +
+ + ) + + // Lock the section rather than block the page — this is a read the user + // simply isn't entitled to, not an action that needs an error. + if (!userCanReadIdentities) { + return ( +
+ {sectionHeader} + + +
+ } + > + <> + + + ) + } + if (!account.serverSideKeyManagementEnabled) { return (
{/* Server-side key management is required state */} -
External Identities
-
- Manage which external identities are trusted for this account -
+ {sectionHeader} } > - + {canManageAccount ? : <>}
) @@ -104,11 +160,13 @@ export const ServiceAccountIdentities = ({ account }: { account: ServiceAccountT
Manage which external identities are trusted for this account
- {(account as any).identities && (account as any).identities.length > 0 && ( - - )} + {userCanManageIdentities && + (account as any).identities && + (account as any).identities.length > 0 && ( + + )}
@@ -140,9 +198,13 @@ export const ServiceAccountIdentities = ({ account }: { account: ServiceAccountT
} > - + {userCanManageIdentities ? ( + + ) : ( + <> + )} )} diff --git a/frontend/app/[team]/access/service-accounts/[account]/page.tsx b/frontend/app/[team]/access/service-accounts/[account]/page.tsx index e86b5ab9a..f962c976d 100644 --- a/frontend/app/[team]/access/service-accounts/[account]/page.tsx +++ b/frontend/app/[team]/access/service-accounts/[account]/page.tsx @@ -456,7 +456,10 @@ export default function ServiceAccount({ params }: { params: { team: string; acc )} - + {userCanViewNetworkAccess && (