Skip to content

fix: lock the External Identities section instead of erroring at members without access - #985

Open
nimish-ks wants to merge 2 commits into
mainfrom
fix/service-account-external-identities-access
Open

fix: lock the External Identities section instead of erroring at members without access#985
nimish-ks wants to merge 2 commits into
mainfrom
fix/service-account-external-identities-access

Conversation

@nimish-ks

Copy link
Copy Markdown
Member

🔍 Overview

Members who could legitimately open a Service Account detail page, but who have no ExternalIdentities permission, got an error toast on page load:

You don't have permission to read identities in this organisation

They weren't trying to do anything — it fired on render, and it fires again on every visit.

The cause is an ungated query. ServiceAccountIdentities fired GetOrganisationIdentities unconditionally (skip: !organisation), resolve_identities raises on missing permission, and the global Apollo error link toasts every GraphQL error.

The Service Account detail query itself never raises — resolve_service_accounts returns 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-level ServiceAccounts permission, team-based access to the account, ExternalIdentities: []. Custom roles with ServiceAccounts.read and no identity access hit it too.

The identities page and UpdateAccountNetworkPolicies already gate their queries this way. This component was the outlier.

💡 Proposed Changes

Frontend

  • Skip the query without ExternalIdentities.read, and render an "Access restricted" empty state for that section only. The rest of the page loads normally; nothing is toasted.
  • Gate the manage controls on read access and update-on-this-account (effectiveCanUpdateSA && hasTeamAccess, passed down from the page as canManageAccount), 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.
  • handleSave no longer reports success 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 straight back. This matches the existing resolve_tokens / resolve_handlers gating pattern.
  • updateServiceAccount now requires ExternalIdentities access to bind identities. ServiceAccounts.update alone 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 when identityIds is 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

  • The updateServiceAccount change is a tightening. Any custom role with ServiceAccounts.update but no ExternalIdentities permission loses the ability to attach identities. I believe that's the correct authz boundary, but it is a behaviour change for such roles.
  • ExternalIdentities.read is 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_identities returns [] and never touches the queryset without permission
  • resolve_identities returns rows when permitted, gated on the right action/resource/org
  • updateServiceAccount rejects identity binding without permission, and persists nothing
  • updateServiceAccount binds identities when permitted
  • updateServiceAccount with no identityIds requires no identity permission (guards renames/role changes)

Verified:

  • Backend suite: 1408 passed, 1 failed. The failure is tests/utils/test_secret.py::test_file_read_permission_error, which fails identically on untouched main — it chmods a file to 000 and expects a read failure, which doesn't happen as root in the container. Unrelated to this change.
  • tsc --noEmit clean; next lint clean 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 — the skip on the query and the early return are the actual fix. Then types.py: resolve_identities for the return-[]-not-raise decision, and service_accounts.py: UpdateServiceAccountMutation for 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:

  • The identities section's key-management CTA shows for team-owned Service Accounts, while the page header hides key management for those (!account.team?.id). Harmless — the action works — but inconsistent.
  • Cancelling the manage dialog leaves stale toggles on reopen, since selected is only seeded at mount.

✨ How to Test the Changes Locally

  1. As an Owner, create an External Identity (Access → Identities) and a team-owned Service Account with server-side key management enabled.
  2. Add a member with the Developer role to that team.
  3. As that Developer, open the Service Account detail page.
  4. Before: a permission error toast on load. After: no toast; the External Identities section shows "Access restricted" and the rest of the page works.
  5. As an Owner, confirm the section still lists identities and the "Manage External Identities" dialog saves.

Backend tests:

cd backend && pytest tests/api/test_service_account_identities_access.py -v

💚 Did You...

  • Ensure linting passes (code style checks)? — next lint clean on the changed files, tsc --noEmit clean
  • Update dependencies and lockfiles (if required) — not required
  • Update migrations (if required) — not required
  • Regenerate graphql schema and types (if required) — not required, schema shape unchanged
  • Verify the app builds locally? — not required for this change
  • Manually test the changes on different browsers/devices? — not run

…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
nimish-ks force-pushed the fix/service-account-external-identities-access branch from 830f399 to e602367 Compare August 25, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants