feat: make personal API access keys rotatable - #2926
Conversation
The personal access key is the bearer token for the external API v2 and the MCP server, but it was generated once at account creation with no way to change it, so a leaked key could only be dealt with by deleting the user. Adds PATCH /me/accessKey plus a Rotate Access Key control on the Personal API Access Key card, mirroring the existing team ingestion key flow. The route takes no user identifier (the id comes from the session), so it can only rotate the caller's own key, and it is deliberately not exposed on the bearer-authed external API v2. Rotation takes effect immediately and leaves the browser session signed in. Also standardizes APIKeyCopyButton on data-testid, which replaces data-test-id that neither Playwright nor Testing Library queries by default, and splits the duplicated "api-key" value into ingestion-api-key and personal-access-key.
🦋 Changeset detectedLatest commit: 9480389 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Greptile SummaryThis PR adds immediate personal API access-key rotation through a session-authenticated endpoint and Team Settings UI.
Confidence Score: 4/5The PR is not yet safe to merge because rotation controls can still launch overlapping requests and leave the UI displaying a revoked key. The shared confirmation prevents duplicate resolution of one dialog but does not prevent opening another dialog while the first mutation is pending; concurrent personal-key responses can therefore update the cache out of order. Files Needing Attention: packages/app/src/components/TeamSettings/ApiKeysSection.tsx and packages/app/src/api.ts
|
| Filename | Overview |
|---|---|
| packages/api/src/controllers/user.ts | Adds an atomic user access-key replacement using a newly generated UUID. |
| packages/api/src/routers/api/me.ts | Adds a session-scoped endpoint that rotates only the authenticated user's access key. |
| packages/app/src/api.ts | Adds the rotation mutation and authoritative cache update, but concurrent successful responses remain unordered. |
| packages/app/src/components/TeamSettings/ApiKeysSection.tsx | Adds personal-key rotation and shared confirmations, but the enabled triggers still permit overlapping rotations. |
| packages/common-utils/src/types.ts | Adds the shared response schema and type for personal access-key rotation. |
| packages/api/src/routers/api/tests/me.int.test.ts | Covers authentication, persistence, revocation, session continuity, and user scoping for the new endpoint. |
Sequence Diagram
sequenceDiagram
participant User
participant UI as Team Settings
participant API as PATCH /me/accessKey
participant DB as MongoDB
participant Cache as React Query me cache
User->>UI: Confirm rotation
UI->>API: Session-authenticated PATCH
API->>DB: Replace User.accessKey
DB-->>API: Updated user
API-->>UI: newAccessKey
UI->>Cache: Update accessKey
Cache-->>User: Display new key
Reviews (5): Last reviewed commit: "Merge branch 'main' into tom/rotatable-a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Deep Review✅ No critical issues found. This is a small, well-tested change: the rotation route takes the user id from the session (never the request), key generation and CSRF posture match the pre-existing 🟡 P2 -- recommended
🔵 P3 nitpicks (3)
Reviewers (9): correctness, security, adversarial, api-contract, kieran-typescript, reliability, testing, maintainability, project-standards. Testing gaps: The confirm (accept) path of rotation is covered only at the integration layer, not E2E, by deliberate design (a confirmed rotation in a parallel worker would 401 the shared-account bearer specs) -- acceptable given the documented constraint. |
E2E Test Results✅ All tests passed • 300 passed • 1 skipped • 1122s
Tests ran across 4 shards in parallel. |
…tion Two review findings on the new personal access key flow, plus the sentence-case convention the card was not following. The success handler discarded the newAccessKey from the response and relied on refetchMe() to pick it up. If that refetch failed, every useMe consumer kept rendering the old key, which the rotation had already revoked, so the user could copy a dead credential with no way to reach the working one short of a reload. The mutation now seeds the me cache from the response instead, which needs no network round trip and removes the failure window. Confirming closed the modal but left the controls enabled, and Mantine keeps modal content mounted through the exit transition, so a fast double click sent two PATCHes and the second revoked the key the first had just generated. The shared confirm modal now takes confirmDisabled, wired to isPending for both the ingestion and personal flows. Applies sentence case to every label in the card per agent_docs/code_style.md, including the pre-existing ingestion key strings, so the two halves match. Acronyms keep their casing, so "Rotate API key" and "Rotate personal API access key". The E2E label assertions and the MCP.md references move with them.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
LGTM with a question/suggestion
| ); | ||
| } | ||
|
|
||
| function RotateKeyConfirmModal({ |
There was a problem hiding this comment.
nit: Are we able to use useConfirm here instead of a new component? I do see this is just an extraction of what was there previously for the ingestion key, so not a huge deal.
There was a problem hiding this comment.
Good call. Will change
| Rotating your personal access key immediately revokes the | ||
| current one and generates a new one. This action is{' '} | ||
| <b>not reversible</b>. |
There was a problem hiding this comment.
Not a blocker here, just a callout before EE merge: If I understand this correctly, the access key is not scoped per-team, so will be revoked across teams. This might not be an issue in OSS (no cross-team users), nor Cloud (no access keys), but if anyone is using multiple teams in the private image, this could unexpectedly rotate their key in a different team than the one they're viewing. It could be worth updating this copy in the EE repo to reflect that.
Review feedback: use the existing useConfirm rather than a bespoke modal
component. Both the ingestion and personal key flows now go through it, so
RotateKeyConfirmModal is gone.
This also removes the confirmDisabled/isPending plumbing added for the
double-rotation guard. useConfirm resolves its promise exactly once, so a
double click on Confirm during the modal's exit transition cannot fire a second
PATCH; the guarantee is structural rather than a prop each caller has to wire.
Tradeoff: useConfirm passes no title to the Modal and renders its body at
size="sm" opacity={0.7}, so the ingestion dialog loses its heading and both
warnings are muted. That matches the other four consumers, which are all
destructive delete confirmations. Adding an optional title to useConfirm would
be a separate change across all of them.
The per-flow confirm and cancel testids collapse into the shared confirm-*
ones, so the page object drops its duplicated locators.
Adds a required-pattern section for useConfirm to code_style.md, next to the other mandated components. Beyond the usage example it records the parts that are not obvious from the source: the promise resolves exactly once so double click protection is free, the confirm/cancel test ids are shared and must not be duplicated per flow, and component tests have to mock it because ConfirmProvider pulls in next/router. It also documents the missing title and the muted body, with the instruction to extend useConfirm rather than fork a one-off modal. The AGENTS.md pointer said to read code_style.md "only when actively coding", which invites deferring it during planning and then never returning. This PR shipped title-case labels and a hand-rolled modal for exactly that reason, with both rules already written down. It now says to read it before writing or planning any packages/app UI change, and calls out that these patterns are invisible from the surrounding file, so matching the component you are editing is not sufficient.
2 participants
Summary
Personal API access keys can now be rotated from Team Settings → API & Agents.
The key is the bearer token for the external API v2 and the MCP server, but it was generated once at account creation and could never be changed. A leaked key could only be dealt with by deleting the user.
What's new
PATCH /me/accessKey, plus a Rotate access key button on the Personal API access key card, mirroring the ingestion key flow directly above it.Two deliberate limits
Also in this diff
useConfirmdialog instead of a bespoke modal (review feedback). The ingestion flow loses its modal title as a result, and its confirm/cancel testids become the sharedconfirm-*ones.APIKeyCopyButtonemitteddata-test-id, which neither Playwright nor Testing Library queries by default, and both key displays shared the valueapi-key. Nowdata-testid, split intoingestion-api-keyandpersonal-access-key.agent_docs/code_style.md. Half-converting it would have looked like an oversight.MCP.mdreferences the same labels, so it moves too.useConfirm, and a fix to theAGENTS.mdpointer that said to readcode_style.md"only when actively coding". This PR shipped title-case labels and a hand-rolled modal because that invited deferring the file past planning, with both rules already written down.How to test on Vercel preview
N/A. Not reachable on the preview build.
useMe()returnsnullunder LOCAL_MODE and the Personal API access key card only renders whenme != null, so neither the card nor its Rotate button exists there. Covered by integration, component and E2E tests instead.References