fix: lock the External Identities section instead of erroring at members without access - #985
Open
nimish-ks wants to merge 2 commits into
Open
fix: lock the External Identities section instead of erroring at members without access#985nimish-ks wants to merge 2 commits into
nimish-ks wants to merge 2 commits into
Conversation
…ers without access 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.
nimish-ks
force-pushed
the
fix/service-account-external-identities-access
branch
from
August 25, 2026 14:07
830f399 to
e602367
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔍 Overview
Members who could legitimately open a Service Account detail page, but who have no
ExternalIdentitiespermission, got an error toast on page load:They weren't trying to do anything — it fired on render, and it fires again on every visit.
The cause is an ungated query.
ServiceAccountIdentitiesfiredGetOrganisationIdentitiesunconditionally (skip: !organisation),resolve_identitiesraises on missing permission, and the global Apollo error link toasts every GraphQL error.The Service Account detail query itself never raises —
resolve_service_accountsreturns an empty queryset instead — so anyone with team-based access to the account loads the page fine and then eats the toast. The default Developer role is exactly this shape: no org-levelServiceAccountspermission, team-based access to the account,ExternalIdentities: []. Custom roles withServiceAccounts.readand no identity access hit it too.The identities page and
UpdateAccountNetworkPoliciesalready gate their queries this way. This component was the outlier.💡 Proposed Changes
Frontend
ExternalIdentities.read, and render an "Access restricted" empty state for that section only. The rest of the page loads normally; nothing is toasted.effectiveCanUpdateSA && hasTeamAccess, passed down from the page ascanManageAccount), so nobody is offered a control whose mutation would be rejected. The "Enable server-side key management" CTA in that section is gated the same way.handleSaveno longer reports success when the mutation was rejected.Backend — so the gate isn't only cosmetic:
ServiceAccountType.resolve_identitieswithholds the rows without permission. It returns[]rather than raising: a field error there would fail the whole Service Account query and put the toast straight back. This matches the existingresolve_tokens/resolve_handlersgating pattern.updateServiceAccountnow requiresExternalIdentitiesaccess to bind identities.ServiceAccounts.updatealone was enough before, which meant a custom role could attach an identity — and so grant it token-minting power for the account — without any identity permission. The check only runs whenidentityIdsis actually passed, so renames and role changes are unaffected.No schema, migration, or dependency changes.
📝 Release Notes
Fixed a permission error toast that appeared when opening a Service Account that you can access but whose External Identities you aren't permitted to see. That section now shows an "Access restricted" state instead, and the rest of the page works as normal.
❓ Open Questions
updateServiceAccountchange is a tightening. Any custom role withServiceAccounts.updatebut noExternalIdentitiespermission loses the ability to attach identities. I believe that's the correct authz boundary, but it is a behaviour change for such roles.ExternalIdentities.readis checked against the org role, not the SA's team-scoped role, since identities are an org-level resource — same as the identities page. Flagging in case team roles should override here.🧪 Testing
New:
backend/tests/api/test_service_account_identities_access.py— 5 tests.resolve_identitiesreturns[]and never touches the queryset without permissionresolve_identitiesreturns rows when permitted, gated on the right action/resource/orgupdateServiceAccountrejects identity binding without permission, and persists nothingupdateServiceAccountbinds identities when permittedupdateServiceAccountwith noidentityIdsrequires no identity permission (guards renames/role changes)Verified:
tests/utils/test_secret.py::test_file_read_permission_error, which fails identically on untouchedmain— it chmods a file to000and expects a read failure, which doesn't happen as root in the container. Unrelated to this change.tsc --noEmitclean;next lintclean on both changed files.Gap: no frontend component tests — the permission states were reasoned through rather than rendered in a test.
🎯 Reviewer Focus
Start at
ServiceAccountIdentities.tsx— theskipon the query and the early return are the actual fix. Thentypes.py: resolve_identitiesfor the return-[]-not-raise decision, andservice_accounts.py: UpdateServiceAccountMutationfor the tightened mutation, which is the only behaviour change that can block an existing user.➕ Additional Context
Two adjacent things noticed and deliberately left alone:
!account.team?.id). Harmless — the action works — but inconsistent.selectedis only seeded at mount.✨ How to Test the Changes Locally
Backend tests:
💚 Did You...
next lintclean on the changed files,tsc --noEmitclean