Repository navigation
feat(api): edit every account with PATCH /accounts/{account_id} - #674
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Claude finished @alukach's task in 15s —— View job ✅ No blocking issues — safe to merge. I read the diff for
DocsThe description names Simplify (ponytail)
💰 Estimated review cost: $0.13 · 0m14s · 5 turns |
Base automatically changed from
feat/api-accounts-and-remaining-routes
to
main
October 16, 2026 02:29
Accounts are edited the way products are: PATCH on the account, answering the account. It takes the profile fields PATCH /accounts/{id}/profile took, which it replaces, and disabled; editing needs PutAccountProfile and disabled needs DisableAccount, so an organization's maintainers can edit it but only its owners can disable it. A service account takes only a name. The write carries only what changed. GET /accounts/{id}/profile stays as the public read.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alukach
force-pushed
the
feat/api-patch-account
branch
from
October 16, 2026 02:31
2fbecad to
d455f8d
Compare
An account edit wrote `name` on every update, since accountsTable.update always set it, so a disable or profile-only edit could revert a concurrent rename; the table now sets `name` only when given. A `disabled` equal to the account's current value is no longer written either, so a caller without DisableAccount who resubmits it can't undo a concurrent disable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alukach
added a commit
that referenced
this pull request
Oct 16, 2026
Follows #667, now merged. Writes down the API conventions this round of PRs settled on, so the next endpoint follows them without anyone reconstructing them from the code. ## What - **`API.md`** at the repository root, beside `CONTRIBUTING.md`: - **One operation, thin adapters:** what an operation decides, what a route and a server action may do, and the exceptions for routes no session can call. - **Documented where it's defined:** `registerPath` beside the handler, the coverage test and its `UNREGISTERED` allowlist, published prose, explicit `HEAD`, `bearer` and `optionalBearer`, `internal`, and `deprecated`. - **Resources and methods:** URL and field naming, and a table of what each method means and answers. Deactivating is `PATCH { "disabled": true }`, never `DELETE`, and one resource has one edit endpoint. - **Lists:** `{items, next_cursor}`, never a bare array. - **Errors:** the shared shape and when each status applies, the 500 `internal` error and the trust exchange's `{trusted: false}` 403, authorizing before validating where the permission doesn't depend on the input, and answering 404 for things the caller may not know of. - **Writes:** carry only the fields that changed, and what `accountsTable.update` always writes. - **Compatibility:** the data proxy is pinned by a test, the CLI follows the reference, and anyone else gets a traffic check, then deprecation rather than removal. - **`CLAUDE.md`** (and `AGENTS.md`, which links to it) gets a short **API** section telling an agent to read `API.md` before touching `/api/v1`. - **`CONTRIBUTING.md`**'s "Server Actions & API Routes" paragraph said both call `isAuthorized()` themselves. It now describes them as adapters over one operation and points at `API.md`. ## Where it lives A root `API.md` rather than more of `CLAUDE.md`: these are conventions for whoever writes an endpoint, person or agent, and much of it is visible in the code once you know where to look, which is what `CLAUDE.md` says it isn't for. `CLAUDE.md` carries the pointer, so agents still load it at the right moment. `CONTRIBUTING.md` links to it. ## Merge order The guide describes the API as it stands once the open PRs from this round merge, and names things they add: - `optionalBearer`, `internal` and `deprecated` endpoints (#667) - `data-proxy-contract.test.ts` (#671) - explicit `HEAD` (#672) - `pageSchema` and `onePage` (#673) - one `PATCH` per account (#674) - `DELETE` that only deletes (#675) Merge it after them, or trim the line that names whichever of them doesn't land. ## Where the code doesn't follow the guide yet The guide states the conventions, not the code's progress toward them. Known gaps, tracked outside the docs: - The `/data-connections` routes and the data-connection actions still read tables and call `isAuthorized()` directly: #593. - The credentials, profile-image and `createAccount` actions still call `isAuthorized()` directly: #592 and #594. - `updateProduct`, `deleteProduct` and `updateMembership` validate the body before authorizing, so a caller who may not act sees field errors instead of a 403. - `accountsTable.update` always writes `name`, `type` and whole metadata maps, so `updateAccount` can undo a concurrent edit. ## Docs, ADRs and `llms.txt` Repository-internal guidance only: no user-facing flow changes, so docs.source.coop and `llms.txt` are unaffected, and it records no decision shared with the data proxy beyond the existing contract test, so no data.source.coop ADR changes. ## Testing Documentation only; no code changed. Each claim in `API.md` was checked against the code after a review pass found several the code didn't hold to (`route-coverage.test.ts`, `accountsTable.update`, `withApiSession`'s error handling, the revocations and trust-exchange routes). Checked that `AGENTS.md` still resolves to `CLAUDE.md` and that the prose in `API.md` wraps at 80, as the repository's Markdown does. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
alukach
marked this pull request as ready for review
October 16, 2026 03:20
alukach
added a commit
that referenced
this pull request
Oct 16, 2026
… flags fold into /accounts/{account_id} (#675)
Follows #674. Items 3 and on from the follow-ups discussed on #667:
`/accounts/{account_id}` becomes the one endpoint for an account.
`DELETE` only deletes, and the `/profile` and `/flags` sub-routes fold
into its `GET` and `PATCH`. #665 needs to rebase onto it (see below).
## What
| Endpoint | Before | Now |
|---|---|---|
| `GET /accounts/{account_id}` | the whole record to whoever may read
it; 401/403 to everyone else | the whole record to whoever may read it,
**its `PublicAccount` view to everyone else**, signed in or not |
| `GET /accounts/{account_id}/profile` | the public profile |
**removed**; `GET /accounts/{account_id}` answers it |
| `GET /accounts/{account_id}/flags` | the account's flags |
**removed**; they're in the record `GET /accounts/{account_id}` answers,
to the same readers |
| `PUT /accounts/{account_id}/flags` | replaces the flags (admin) |
**removed**; `PATCH /accounts/{account_id}` takes `flags` (admin) |
| `DELETE /accounts/{account_id}`, service account | disabled it |
**deletes it**, through `deleteAccount` |
| `DELETE /accounts/{account_id}`, person or organization | disabled it
| **409** to a caller who may disable it, saying to `PATCH { "disabled":
true }`; a person or an organization is never deleted |
### `GET` answers two shapes
`getAccount` absorbs `getAccountProfile`: `GetAccount` gets the
`Account` record, else `GetAccountProfile` gets a `PublicAccount`, else
a disabled or service account is a 404 and anything else is refused. The
reference documents the 200 as `anyOf` the two, and the route declares
`optionalBearer`, since it answers anonymous callers. Who sees a 404 is
unchanged from the profile route.
`PublicAccount` is the record without its private fields: `account_id`,
`type`, `name`, `disabled` and `metadata_public`, plus a computed
`profile_image`. It replaces the flattened `AccountProfileResponse`,
which had no `account_id`, so it couldn't serve as the account embedded
in a product or a list.
Products embed the same view. `shown()` already cut a product's account
down for a caller who may not read it, but cast the result to `Account`,
so the reference promised `flags`, `created_at` and `updated_at` that
weren't sent. `ProductResponseSchema` now carries the `Product` name in
the reference and declares `account` as `Account | PublicAccount`. The
`Product` type the app reads from the database is unchanged.
### `flags` on `PATCH`
`flags` is one more field on `UpdateAccount`, and takes
`PutAccountFlags` the way `disabled` takes `DisableAccount`: a caller
may hold any of the three permissions alone, and is refused only for the
fields they send. A service account still can't hold `admin` (400,
naming `flags`). Only the flags are written, so a concurrent profile
edit survives. The account-flags form's `updateAccountFlags` server
action keeps its name and calls `updateAccount`. `API.md` already said
one resource is edited through one endpoint, which the flags route
contradicted. Its methods table now cites `PUT /data-connections/{id}`
as the `PUT` example in place of `/flags`.
### Deleting a service account
`deleteAccount`:
1. **disables the account first**, writing only `disabled`, so no key
exchanges and nothing new can be granted, issued or trusted while it's
torn down (granting, issuing a key and adding a trust each already
refuse a disabled service account);
2. deletes every membership and **every key row** it held, in parallel;
3. deletes the account row, whose trusts go with it.
Keys are deleted rather than revoked, so a service account that later
takes the same ID starts with no keys, instead of listing its
predecessor's. As a backstop, **the key exchange refuses a key whose
`created_at` is earlier than its account's**. That ties a key to the
account it was issued to, so a key the cleanup misses (a race, GSI lag,
a delete path that skips `deleteAccount`) can't sign in as a re-created
account with the same ID. A key minted with an account at creation is
never older than it: the account's timestamp is taken first.
**Known gap:** disabling first only stops requests that read the account
after the disable. A grant or trust from a request that read it just
before can still land after the cleanup lists memberships, and survive
the delete. A service account re-created with the same ID would then
inherit it. Keys have the "older than its account" backstop, but
memberships and trusts have none yet. Closing that needs conditional
writes or the same check on memberships and trusts, which is a
follow-up.
Authorization comes before anything an unauthorized caller shouldn't
learn: a service account the caller can't manage is a 404, as `GET`
answers, and the 409 pointing at `PATCH` goes only to a caller who may
disable that person or organization.
The service-account settings page's `deleteServiceAccount` action goes
through the same operation, so deleting from the UI cleans up keys too;
on `main` it left them live. On a refusal it shows its own message
rather than the API's.
## #665
#665 makes the same `DELETE` change through its own
`deleteServiceAccount` operation, but still disables any other account.
Rebased onto this, it can drop that operation and its `DELETE` route,
and keep the rest of its service-account and trust endpoints.
## Nothing calls these yet
No caller in the app's API clients, the CLI, the data proxy or the other
repos in the org (code search for `/profile` and `/flags`). The
account-flags form uses its server action, not the route. No production
traffic in the last day for `DELETE /accounts/{account_id}`.
## UI
The settings page's delete button and the account-flags form call the
same server actions as before, which now adapt `deleteAccount` and
`updateAccount`; their stories stub the actions, so they are unchanged.
One visual change: a signed-out reader of `/products` gets each
product's account as a `PublicAccount`, which has no emails, so the
hover card couldn't find an individual's Gravatar. The public view
carries the `profile_image` worked out before the emails were dropped,
`avatarUrl` uses it, and the account links, hover card and avatar accept
`Account | PublicAccount`. New story: [AccountLinks › Public
View](https://source-coop-ui-git-feat-api-delete-only-deletes-radiantearth.vercel.app/?path=/story/components-accounts-accountlinks--public-view).
No screenshot: headless Chromium didn't get past Storybook's loader
locally.
## Docs, ADRs and `llms.txt`
- `public/llms.txt`'s public-profile line now points at
`/api/v1/accounts/{account_id}`, which answers anonymously.
- `API.md`'s methods table cites `PUT /data-connections/{id}` instead of
the removed `/flags` route.
- `API.md`'s rule for published `summary` and `description` text now
says how to write it: lead with a verb, don't drop words, one rule per
sentence, name who gets what actively, 25 words or fewer a sentence, and
address the caller as "you". These are ASD-STE100's writing rules
without its controlled vocabulary, which would read stiffly in developer
docs and which nothing here could enforce. The `GET`, `PATCH` and
`DELETE` descriptions on `/accounts/{account_id}` are rewritten to it.
Descriptions on other routes keep the older style until someone touches
them.
- Checked docs.source.coop:
`using-source/upload/with-service-account.md` names only the
key-revocation endpoint and says a service account can be deleted
without saying what goes with it, so it still holds. No page names the
profile or flags endpoints.
- Checked the data-proxy ADRs: 013 records that a revoked key stops new
exchanges within the proxy's cache TTL. A deleted key, or one older than
its account, answers inactive the same way, so 013 and 014 still hold.
## Testing
At `b06dbf94`: `npm run type-check` passes, and the full `npx jest`
passes (94 suites, 1,017 tests).
- `operations/accounts.test.ts`: `getAccount` answering the record or a
`PublicAccount` without emails, flags or private metadata, and 404 for a
hidden service or disabled account; `flags` on `updateAccount` (takes
`PutAccountFlags`, no `admin` for a service account, writes only the
flags, rejects an unknown flag); `deleteAccount` disabling first and
deleting memberships and keys, the 409 only to a caller who may disable,
the 404 to whoever may not manage a service account.
- `exchanges/route.test.ts`: a key older than its account answers
inactive.
- `actions/service-accounts.test.ts`: the action redirects on success
and shows its own message on refusal; `setProductAccess` grants nothing
to a disabled service account but still revokes its grant.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
alukach
added a commit
that referenced
this pull request
Oct 16, 2026
main's updateAccount and deleteAccount (#674, #675) now edit and delete service accounts, so this branch drops its own updateServiceAccount, deleteServiceAccount and UpdateServiceAccountSchema, and PATCH/DELETE /accounts/{account_id} are main's. The rename and disable actions call updateAccount. Grants use product_id (#680). setProductAccess keeps going through the membership operations, which refuse a disabled service account through serviceAccountGrantProblem (#666). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alukach
added a commit
that referenced
this pull request
Oct 17, 2026
…665) Closes #594. Part of #588. ## What Service accounts and their API keys over `/api/v1`, documented at `/api/docs`: | Endpoint | Operation | |---|---| | `GET /accounts/{owner}/service-accounts` | `listServiceAccounts` → `{items, next_cursor: null}` | | `POST /accounts/{owner}/service-accounts` | `createServiceAccount` — `local_id`, `name`, `github_subjects[]`, `grants[{product_id, role}]`, optional `key{label, expires_at}`; the response carries the issued key when one is asked for | | `GET /accounts/{sa}/trusts` | `listTrusts` → `{items, next_cursor: null}` | | `POST /accounts/{sa}/trusts` | `addGithubTrust` — `{subject}` | | `DELETE /accounts/{sa}/trusts?issuer=&subject=` | `removeTrust` — 404 for a trust the account doesn't have; returns the trust as it was | | `GET /accounts/{sa}/keys` | `listKeys` → `{items, next_cursor: null}`, revoked and expired keys included, never a hash | | `POST /accounts/{sa}/keys` | `issueKey` — `{label, expires_at}` → 201 with `key`, the only time it appears | | `PATCH /accounts/{sa}/keys/{key_id}` | `updateKey` — `{expires_at?, revoked?: true}` | `src/lib/operations/service-accounts.ts` holds the rules the issue names: `GITHUB_ACTIONS_SUBJECT_REGEX` (as a Zod schema shared with the OpenAPI doc), the read/write-only grant roles, `managedServiceAccount` (every "not yours / missing / not a service account" answers 404, keeping the single answer it gave before), and the rule that a service account can't own another. The server actions in `src/lib/actions/service-accounts.ts` now adapt FormData into those operations, and the service-accounts settings page lists through `listServiceAccounts`. Editing and deleting a service account are main's `PATCH` and `DELETE /accounts/{account_id}` (#674, #675): rename and disable call `updateAccount`, and delete calls `deleteAccount`, so this PR adds no operation of its own for them. Trusts are returned through `AccountTrustSchema`, so the table's `identity` sort key stays out of responses. `setProductAccess` now calls `inviteMember` / `updateMembership` / `revokeMembership`, and creation grants each product through `inviteMember` too, so every grant passes the same authorization, duplicate check and audit log; so product access over the API is the product members endpoints from #589 with role `read_data` or `write_data`, and gets no endpoint of its own. API keys were first left to #570 and #580–#582, which built the keys and their settings UI but no API; with those merged, this PR adds the API for them (scope noted on #594). `issueApiKey`, `revokeApiKey`, `setApiKeyExpiry` and the create action now map form fields onto `issueKey`, `updateKey` and `createServiceAccount`, so no action touches the keys table, and the key rules (hash-only storage, the six-character hint, refusing a disabled account, the expiry range) are tested once, on the operations. ## Worth a look - Revoking is `PATCH {"revoked": true}`, not `DELETE`: the record stays, with `revoked_at`, `revoked_via` and `revoked_by`, as disabling an account is a `PATCH`. `revoked: false` is refused with 400, since a revoked key can't be restored; revoking one twice is 409, and a PATCH with neither field is 400. - `expires_at` is an ISO time, at most 3650 days away, or `null` for a key that lasts until revoked. It is required on issue, so a caller chooses no-expiry deliberately. The settings form still takes days and converts them. - A key requested with `POST /service-accounts` is validated with the rest of the body, before anything is written, and stored after the grants and trusts. - `createServiceAccount` checks every grant against `InviteMembership` before it writes the account, so a grant the caller may not make leaves nothing behind and a retry with the same `local_id` succeeds. `inviteMember` can refuse nothing else afterwards: the account is new and enabled, and the grants are deduplicated first. - Deleting a service account is main's `deleteAccount` on both the API and the settings page. It deletes the account's API keys rather than revoking them, and the key exchange also refuses a key older than its account, so a key from a deleted account can't sign in as the next account to take its id. ADR 013 still holds. - `setProductAccess` refuses a new grant to a disabled service account (#666) through `inviteMember` and `updateMembership`, which both check `serviceAccountGrantProblem`. Revoking still works while the account is disabled. - Service-account lists use the `{items, next_cursor}` page shape, as products and data connections do, so pagination can arrive later without a breaking change. ## Docs, ADRs, llms.txt - docs.source.coop `docs/using-source/upload/with-service-account.md` walks through the settings UI, which is unchanged, so it still holds; it could later mention issuing a key over the API, which #595's reference covers meanwhile. The API reference is #595's to publish. - data.source.coop ADRs: I checked 005 (authorization), 010 (account-owned roles) and 013 (API keys). ADR 013 has source.coop generate the key, store only its hash, show it once, refuse a disabled service account and keep expiry editable, with `key_id` as the public handle; the key endpoints do exactly that, so this PR implements existing decisions and moves none. - `public/llms.txt`: no change. These endpoints all need auth, so they can't go on the file's no-auth URL list, and its existing "API reference" line points at `/api/openapi`, which now includes them. ## Storybook No component changed. The settings page now gets its list from the operation, and the key actions keep their form fields and state shapes, so `IssueApiKeyDialog` and the rest are untouched. ## Testing - `npm run type-check`: clean. - `npx jest`: 96 suites, 1068 tests pass. That includes the service-account action tests, now run through the operations; the key action tests, now checking only the form mapping; new operation tests (list authz, JSON create, the 404 on an unknown trust, a refused grant or bad key writing nothing, issuing, listing, expiring and revoking keys); and the OpenAPI validity and route-coverage tests. - Not exercised against a running server. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This branch was successfully deployed
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.
Follows #667, now merged. Item 2 of the follow-ups discussed there; #665 needs to rebase onto it (see below).
What
Products are edited with
PATCH /products/{account_id}/{product_id}, which answers the product. Accounts had two ways to change: #667'sPATCH /accounts/{account_id}/profilefor people and organizations, and #665'sPATCH /accounts/{account_id}for service accounts. This PR makes accounts work the way products do: onePATCH /accounts/{account_id}for every kind of account, answering theAccount.PATCH /accounts/{account_id}updateAccount: profile fields anddisabled, for any accountPATCH /accounts/{account_id}/profileGET /accounts/{account_id}/profilePATCH /accounts/{account_id}updateAccounttakesUpdateAccount: everythingUpdateAccountProfiletook (name,bio,location,orcid,ror_id,websites,email) plusdisabled. Editing and disabling are separate permissions, as they already were inauthz.ts:disabledneedPutAccountProfile: the person themselves, an organization's owners and maintainers, or whoever manages a service account's owner.disabledneedsDisableAccount: an organization's owners, a service account's managers, or an admin. Resubmitting the current value changes nothing and needs nothing.name(anddisabled); a profile field is a 400 naming it.PutAccountProfilerefuses other edits to it until then, except by an admin.The write carries only what changed:
nameonly when given (accountsTable.updateno longer sets it unconditionally),emailsonly when the address changes,metadata_publiconly when a profile field is given,disabledonly when it differs from the stored value. A rename can't write back a stale profile ordisabled, a disable can't write back a stale profile or name, and a caller withoutDisableAccountwho resubmits the currentdisabledcan't undo a concurrent disable.The edit-profile form's server action,
updateAccountProfile, keeps its name and now callsupdateAccount.#665
#665's
updateServiceAccountand itsPATCH /accounts/{account_id}do for service accounts whatupdateAccountnow does for every account,nameanddisabledwith the same permissions. When #665 rebases onto this, it can drop both and point its service-account settings atupdateAccount. Itsauditentry can move intoupdateAccount's log line, which records the fields changed and who changed them. ItsDELETEchange is item 3, stacked on this PR.Nothing calls these yet
No caller in the app's API clients, the CLI or the data proxy, and no production traffic in the last day (the window Vercel's metrics allow) for
PATCH /accounts/{id}/profileorPATCH /accounts/{id}. The web form goes through the server action, not the route.Docs, ADRs and
llms.txtpublic/llms.txtnamesGET /accounts/{account_id}/profile(from feat(api): accounts, whoami and the internal routes on the ops layer #667), which is unchanged.using-source/create-an-account.mddescribes setting up a profile in the UI, andusing-source/upload/with-service-account.mddescribes the service account's Disable button; neither names an endpoint, and both flows still work as described.Testing
npm run type-checkpasses atbafc5340.npx jestpassed (93 suites, 1,003 tests) beforebafc5340; atbafc5340,operations/accounts.test.tsandclients/database/pass (9 suites, 63 tests).operations/accounts.test.tscovers the split permissions (disable withDisableAccountalone; edit without it, which refuses a change todisabledbut accepts resubmitting it unchanged), a service account's rename and refused profile field, and the narrowed write (noemailskey when the address is unchanged, nonamekey when it isn't given, nodisabledkey when it's resubmitted unchanged).🤖 Generated with Claude Code