🛠️ PUT-1521: Cleanup puter js permissions api + backend routes - #3607
Merged
Salazareo merged 12 commits intoAug 21, 2026
Merged
Conversation
Fifteen request methods differed only by a folder name or an access level, so every new resource meant another method. Replace them with requestFolder, requestApps, requestSubdomains and requestAppRootDir, each taking the access level as an argument. The old names stay as @deprecated aliases: puter.js ships unpinned from js.puter.com/v2, so removing them would break live apps. They remain in the generated declarations because stripInternal has no effect on declarations emitted from JavaScript, and hand-omitting them would break TypeScript callers the runtime still serves. Also drops the user-to-user and user-to-group grant wrappers (groups.js and the grantUser/grantGroup half of grants.js) plus the req_ shim, none of which were documented or called. The app, origin and dev-app grants stay: the dashboard uses puter.perms.revokeApp() to clear grants on app uninstall.
Replace the twelve one-method-per-task pages with requestFolder, requestApps and requestSubdomains, and rewrite the Perms overview around the seven public methods. The deprecated aliases keep working but are no longer documented. Boy Scout: drops the long-dead commented-out grantUser/revokeOrigin sidebar block for pages that were never published.
…n routes Filesystem access is shared through /share, which records the grant so the owner can see and revoke it. The older direct-grant paths were left behind with no caller anywhere - not the GUI, not a doc, not an app: grant-user-user (already a 501 stub), revoke-user-user, grant/revoke-user-group, and the five /group/* CRUD routes. Removing them orphans PermissionService.grant/revokeUserGroupPermission and its group-members cache bump, the three PermissionStore group writers, and six GroupStore methods, so those go too. What stays, and why: - grant/revokeUserUserPermission - ACLService and ShareService power fs.share through them. - The group permission read path (#scanUserGroup, readUserGroupPerms) - a migration seeds the admin group unrestricted driver access, so it is load-bearing. - GroupStore getByUid/addUsers/removeUsers - signup, save_account, OIDC and the self-hosted default user assign group membership. No schema change: user_to_user_permissions, user_to_group_permissions and their audit tables are untouched. Group rows now come only from migrations, so tests that need one seed it with SQL the way a migration does.
Contributor
With the /group/* routes gone, `getByUid` had no production caller left — the routes were the only thing that read a group back. Removing it takes the row decoder and the GroupRow type with it, since they exist only to shape its result. What remains is `addUsers`/`removeUsers`: signup, save_account, OIDC and the self-hosted admin bootstrap all assign group membership. Permissions attached to a group are read through PermissionStore, which joins the junction table itself and never needed the store. Tests that wanted a group id now select it, which is all `getByUid` was doing for them.
freeai, experimental and dangerous exist in prod but in no migration — they were added by hand when hardcoded permissions were keyed by group name. That map is now a flat per-user floor (`default_user_permissions`), so a group nothing looks up grants nothing. Guarded rather than unconditional, because both tables the delete can reach cascade: dropping a group that still carries permissions or members would silently revoke them from every member. Only a group with neither goes. One that survives has dependents and needs a deliberate decision — query user_to_group_permissions by group_id to see what it holds. system, admin, user and temp are untouched: config names two of them and code names the others. Matches on `extra.name`, not `metadata.name` — `metadata` carries the display title and colour, and `critical: true` is set on all of these including freeai, so it does not discriminate.
jfcastro92
marked this pull request as ready for review
August 19, 2026 19:53
Contributor
Coverage Report for puter.js SDK
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Salazareo
requested changes
Aug 20, 2026
Salazareo
left a comment
Salazareo
left a comment
Member
There was a problem hiding this comment.
backend changes look good,
but I think the docs should wait until we expose a nicer way to request the permissions:
puter.perms.request(Enum, detailsObj)
…+ check()
One method per task meant a new method, doc page and sidebar entry for every
resource. `request` now takes the resource and a payload whose accepted fields
depend on it, and `check` answers the same question without prompting.
request('folder', { name: 'Documents', access: 'write' }) -> path
request('apps', { access: 'read' }) -> boolean
request('email') -> address
check('folder', { name: 'Documents', access: 'write' }) -> boolean
Returns stay per-resource: a folder gives its path, email the address, the rest
a boolean, and anything denied is falsy so one `if` covers both.
An array asks for several at once. Everything already held is settled first, so
the prompt covers only what is missing and does not appear when the whole set is
held - the user answers once for the lot. `check` answers per entry, in order,
so a caller can tell which parts are missing rather than only that some are.
Each resource declares four things in one registry entry: how to ask for it
alone, whether it is held, the strings a batch pools into a prompt, and the
value once held. The strings themselves are defined once in
lib/permissionStrings.js, so a request and its check cannot name them
differently. `check` is built on /auth/check-permissions, already live and
already used by UI.js, and it throws rather than answering false when the check
cannot run: a caller that cannot tell "denied" from "never ran" would prompt
someone who had already granted it.
Backward compatibility: all 22 older methods stay callable and typed, marked
@deprecated with the call that replaces them. A lone string still routes to the
raw-permission path - no resource name contains a `:` and every permission
string does, so the two forms cannot collide. The grant/revoke app methods are
untouched; the consent dialog and the dashboard's uninstall path use them.
Also drops three copies of the access-level assertion onto one shared
validator, and gives `appRootDir` a non-prompting server probe, since
`app-root-dir:` only resolves while a grant is being written and a permission
check on it always answers false.
Five per-method pages became one `request()` page carrying the resource table, the batch form and the raw-string escape hatch, plus a `check()` page. The overview is rewritten around the two methods. requestAppData's page is re-homed as /Perms/appData rather than deleted - its scope table, private-entry guidance and lifetime notes are not signature documentation and have nowhere else to live. Inbound links from KV/set.md and Objects/app.md follow it. Playground examples move to the new call form. They are not wired into examples.js, but an example demonstrating a deprecated method is worse than one nobody loads.
Dropping this route with the rest of the unused user-to-user plumbing went too far. The grant side is retired and stays retired - puter.fs.share() is the only way in - but access those grants left behind has to remain withdrawable, and a caller reaching the endpoint over HTTP directly had no replacement. Revoking can only ever narrow what someone can reach, so keeping it carries no risk. revokeUserUserPermission never left the permission service; it is load-bearing for puter.fs.share(). This only re-wires the handler to it, with the gates it always had. Nothing in this repo calls the route, which makes it exactly what a later cleanup reads as dead, so a test pins the registration and its gate alongside the restored 400 and grant/revoke round-trip cases. The 501 stub at grant-user-user and the never-called /group/* routes stay deleted, as does puter.perms.revokeUser - puter.fs.unshare() replaces it and falls back to live grants when no share row exists.
…ains
`apps-of-user:<uuid>:write` covers managing the user's apps, which includes
reading them, but nothing said so to the permission system. Prefix implication
only widens the other way — an `apps-of-user:<uuid>` grant covers both modes —
so a scan for `:read` missed a `:write` grant, and `puter.perms.check('apps')`
reported an app holding write as holding nothing. A batched request would then
prompt again for access already granted.
Adds the read-from-write exploder for both namespaces, mirroring
`fs-access-levels`. The widening runs one way only, and does not cross into
another user's namespace; both are covered by tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`/auth/request-app-root-dir` conflates two questions: may the caller claim its
root directory, and where is it. The second provisions `AppData/<uid>` on first
ask, so a caller that only wanted the first — `puter.perms.check('appRootDir')`
— created a directory by asking about it.
Adds `check: true`, which runs the same actor guard and stops at the answer.
A caller that may not claim it still gets the 403, so the flag can't widen
anything. Existing callers are unaffected: without it the route behaves exactly
as before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`request` dispatched to the old per-task methods while `check` asked the
permission tables, so the two answered different questions about the same
access. Concretely, before this: `request('folder', { access: 'write' })`,
`'apps'`, `'subdomains'`, `'appData'` and `'permission'` prompted every time,
whether or not the access was held — which the docs said they wouldn't;
`check('folder')` reported false for a folder the app could read through an ACL
grant that no `fs:` string names, so a batch prompted for it needlessly; a
batch entry for `'appRootDir'` skipped the post-grant retry the single call
does, resolving `undefined` after a grant that had in fact succeeded; and an
N-entry batch made N permission reads plus 2N `whoami` calls.
Both now run the same pipeline — resolve the permission strings, read what is
held once, prompt for the remainder, resolve each entry — with per-resource
hooks for the parts only that resource can answer. So a batch costs one
permission read and one `whoami`, a check reports exactly what a request would
skip the prompt for, and `'folder'` uses the same stat-or-permission reading in
both.
Also:
- A resource is looked up as an own property, so `request('constructor')` is
the permission string it always was rather than a TypeError.
- A permission read that fails no longer decides anything: `request` falls
through to the prompt it would have raised anyway, `check` throws. Before,
`check('appRootDir')` folded a failed check into "not granted", which is what
the documentation says must not happen.
- Drops `requestFolder`, `requestApps`, `requestSubdomains` and
`requestAppRootDir`. They were added in this branch and immediately deprecated
— never shipped, and `request()` no longer needs to route through them. The 22
methods that did ship keep their exact behaviour, prompting without consulting
what is held, which the suite now asserts alongside the new behaviour.
- Documents `'appRootDir'`, which was a supported resource in every overload and
in `PermsResource` but named in none of the docs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- `src/puter-js/test/perms.test.js` still called `requestApps`,
`requestFolder`, `requestSubdomains` and `requestAppRootDir`, which the
previous commit removed. Four cases in the interactive browser harness threw.
Pointed at `request(...)` instead.
- `request('appRootDir', …)` made two round trips where the shipped method
makes one: a read-only probe, then the call that names the directory. A
request is going to claim it either way, so the claim is now the check, and
the entry it returns carries through to the result. `check` keeps the
read-only mode, which is the reason that mode exists. Matters because the
route sits on the FS_SIGN bucket, shared with signed-URL minting.
- `requestPermission` is one of the shipped methods, and the previous commit's
message was wrong to say all 22 keep their exact behaviour: it forwards to
`request`, so it now settles a permission the caller already holds instead of
prompting for it. The value can differ, not just the prompt count — a user who
would have clicked Deny on a re-prompt used to get `false`. It is the more
honest answer (the app does hold the access, and denying a re-prompt never
took it away), but it is a change, and the suite assertion had been switched
to an unheld permission, which hid it. Asserted both ways instead, in the unit
tests and the API suite.
- An entry that names no permission no longer rides a grant given for the other
entries in the same call. Unreachable today — every resource either names one
or reports itself held — but nothing pinned it.
- Reverted three type-union reformats in `LegacyFSController.ts` that a
formatter had folded into the app-root-dir commit. That file was not
prettier-clean to begin with; reformatting it is somebody else's change.
- Docs and types: `Perms.md`'s `appRootDir` row now matches `request.md`'s,
`check.md` says that a `true` is per entry and a batch still prompts if any
one entry is missing, and `types.js` no longer names `requestFolder` /
`requestAppData` in prose that ships in the generated declarations.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Salazareo
approved these changes
Aug 21, 2026
Salazareo
left a comment
Salazareo
left a comment
Member
There was a problem hiding this comment.
made some changes, but should be good now
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
2 participants
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.
Two piles of unused surface had accumulated around permissions, and the
request*family had grown one method per task. This removes the former, and replaces the latter with a single front door rather than a shorter list of the same shape.1. User↔user and user↔group permission plumbing that nothing called. Filesystem sharing moved to
puter.fs.share(), which records a share row so the owner can see and revoke it. The older direct-grant paths were left behind:/auth/grant-user-userwas already a 501 stub, andgrant/revoke-user-groupplus the five/group/*CRUD routes had no caller anywhere — not the GUI, not a doc, not an app.puter.permslayered 8 undocumented wrappers on top of them.2. One
request*method per task. 15 methods differed only by a folder name or an access level (requestReadDesktop,requestWriteDesktop, …requestManageSubdomains). Every new resource meant another method, another doc page, another sidebar entry.Supported
puter.permssurface: 33 → 8. Two permission methods (request,check) plus the six app/origin grant methods. Eight backend routes gone, along with the service and store methods they alone fed.puter.js: one
request(), onecheck()The resource is the first argument and it decides which payload fields are accepted — the shape
emit(name, data)uses, so a caller writes one call regardless of what they are asking for:Returns stay per-resource, so a call gives back what the caller actually needed rather than a boolean plus a second call to go get it. Everything denied is falsy, so one
ifcovers both outcomes either way.PermsBatchEntryis a discriminated union and each resource gets its own@overload, sorequest('apps', { name: 'Desktop' })is a type error rather than a silently ignored field. The generated.d.tscarries all sixteen overloads.check()— the same question, without a promptTakes the same resource and details as
request, and never opens a dialog. This is what lets an app offer an opt-in only where one is needed instead of prompting on load. Built on/auth/check-permissions, which is already live and already used internally byUI.js, so no new backend surface.Two deliberate choices:
false. A caller that cannot tell "denied" from "the check never ran" would prompt someone who had already granted it.'appData'with several scopes, or'permission'with a list, answersfalsewhen partly granted — the prompt is still needed.'appRootDir'is the one resource that asks the server rather than checking a permission string:app-root-dir:…is a pseudo-permission that only resolves while a grant is being written, so a permission check on it always answers false.Batching
Either method takes an array, each entry naming its own resource:
Everything already held is settled before anything is asked, so the dialog lists only what is actually missing — and never appears at all when the whole batch is already granted. The set goes up as one request, which is the point: the user answers once and the answer covers all of it. A denial denies every entry that needed the prompt; entries that were already held keep their value, since nothing was asked about them.
check([...])answers per entry in the order asked, so a caller can tell which parts are missing rather than only that some part is.How the resources are wired
Each resource declares four things in one registry entry: how to ask for it alone, whether it is held, the strings a batch pools into a prompt, and the value once held. The strings themselves are defined once in
lib/permissionStrings.js, so a request and its matching check cannot name the same permission differently. Single-resourcerequestdelegates to the method that has always served that resource, so those paths behave exactly as before.Backward compatibility
All 22 older methods stay callable, typed, and unchanged — same arguments, return values and error codes. They carry
@deprecatednaming the call that replaces them:requestEmail()request('email')requestFolder(name, access)request('folder', { name, access })requestApps(access)/requestSubdomains(access)request('apps' | 'subdomains', { access })requestAppData(app, scopes)request('appData', { app, scopes })requestAppRootDir(app, access)request('appRootDir', { app, access })They stay in the generated
.d.tsrather than being hidden —stripInternalhas no effect on declarations emitted from JavaScript, and dropping them by hand would break TypeScript callers the runtime still serves. Editors show them struck through.request('fs:/path:read')still works: a lone string routes to the raw-permission path. No resource name contains a:and every permission string does, so the two forms cannot be confused.grantApp,revokeApp,grantOrigin,revokeOrigin,grantAppAnyUserandrevokeAppAnyUserare untouched and not deprecated — the consent dialog and the dashboard's uninstall path run through them.Eight methods are removed outright and this is a break, signed off after review:
grantUser,revokeUser,grantGroup,revokeGroup,createGroup,addUsersToGroup,removeUsersFromGroup,listGroups, plus thereq_shim. All were undocumented, absent from the sidebar, andgrantUseralready returned 501 from the server.puter.fs.unshare()replacesrevokeUserand covers the legacy case, since it falls back to live grants when no share row exists.New exported types:
PermsResource,PermsBatchEntry,PermsAccessRequest,PermsFolderRequest,PermsAppDataRequest,PermsAppRootDirRequest,PermsPermissionRequest,PermsRequestDetails. Validation is client-side and rejects with{ message, code: 'invalid_argument' }before any network call.Backend
Routes deleted:
grant-user-user,grant-user-group,revoke-user-group,/group/create,/group/add-users,/group/remove-users,/group/list,/group/public-groups.That orphaned, and so also removes:
PermissionService.grant/revokeUserGroupPermissionand its group-members cache bumpPermissionStoregroup writersGroupStoredown toaddUsers/removeUsers—getByUidhad no production caller once the routes went, which took the row decoder andGroupRowwith itrevoke-user-useris kept, deprecatedDeleting this one went too far and it has been restored. The grant side is retired and stays retired —
puter.fs.share()is the only way in — but access those grants left behind has to remain withdrawable, and a caller reaching the endpoint over HTTP directly had no replacement. Revoking can only ever narrow what someone can reach, so keeping it carries no risk.PermissionService.revokeUserUserPermissionnever left (it is load-bearing forputer.fs.share()), so this only re-wires the handler to it with the gates it always had. Nothing in this repo calls the route, which makes it exactly what a later cleanup reads as dead — so a test pins the registration and its gate alongside the restored 400 and grant/revoke round-trip cases.The SDK's
revokeUserwrapper is not restored:puter.fs.unshare()covers it, and re-adding the wrapper would re-grow the surface this PR exists to shrink.Direction this sets, per review: user-to-user access should be built as feature-specific APIs backed by the permission service — the way sharing was, where the grant and a listable, revocable share row are written together — rather than a catch-all string give/revoke API. That is the reasoning for deleting the rest rather than keeping them around.
Migration: three groups no code reads
freeai,experimentalanddangerousexist in prod but in no migration — added by hand when hardcoded permissions were keyed by group name. That map is now a flat per-user floor (default_user_permissionsindata/hardcoded-permissions.js), so a group nothing looks up grants nothing. Dropped for sqlite, mysql and postgres.Guarded, not unconditional.
user_to_group_permissions.group_idandjct_user_group.group_idare bothON DELETE CASCADE, so removing a group that still carries permissions or members would silently revoke them from every member. Only a group with neither is dropped, and four tests cover both branches.system,admin,userandtempare untouched — config names two, code names the others.Consequence worth knowing at deploy time: if a group survives the migration, the cleanup silently did not happen for it. Check which way it went:
Non-zero
permsormembersmeans those grants need a deliberate decision before the rows can go.Per AGENTS.md, this is a permission-related change. No permission check logic was modified. What deliberately stayed:
PermissionService.grant/revokeUserUserPermission—ACLServiceandShareServicepowerputer.fs.share()through them. The grant route is gone and the revoke route is deprecated-but-live; the service methods are load-bearing.#scanUserGroup,readUserGroupPerms,FSService) — kept because prod may hold group permission rows this repo can't see. Note the migration-seededsystem → admin group → driverrow is not the justification:default_user_permissionsalready gives every user actor baredriver, so that row is redundant.GroupStore.addUsers/removeUsers— signup,save_account, OIDC and the self-hosted default user all assign group membership.grant/revoke-user-appandgrant/revoke-dev-app— the consent-dialog path (UIPermissionDialog,UIDesktop), and the dashboard callsputer.perms.revokeApp(appUid, '*')to clear grants on app uninstall./auth/check-permissions— unchanged;check()is a new consumer of it, not a new endpoint.user_to_user_permissions,user_to_group_permissionsand both audit tables keep their rows, so no historical grant or audit record is lost.check()widens no access: it reports what the caller already holds, evaluated by the samecheckManypath the route already used.Docs
The review asked that the docs wait for a nicer request surface, so they now land on it rather than on the collapsed one-per-task names:
/Perms/request— the resource table, the details each takes, batching, and the raw-string escape hatch/Perms/check— new/Perms/appData—requestAppData.mdre-homed rather than deleted: its scope table, private-entry guidance (disableSharing) and grant-lifetime notes are not signature documentation and have nowhere else to live. Inbound links fromKV/set.mdandObjects/app.mdfollow it.requestEmail,requestFolder,requestApps,requestSubdomains) — signature plus one example, all of it now on therequest()pagePerms.mdoverview is rewritten around the two methods, and the playground examples move to the new call formrequestAppRootDirstays undocumented, matching its status today — this PR shrinks published surface rather than growing it.Testing
npm run test:backend— 6104 passed, 24 skippednpm run build:workerLib && npm run test:puterjs— 1813 passed across node, browser and workerd (was 1754 on this branch before the new API)npm run check:puterjs:types— declarations generate and type-check withoutskipLibChecknpm run typecheck— no new errorsrequest.test.js(perms directory now 60) covering dispatch, per-resource returns and permission strings, batch pooling into one prompt, batch validation before any prompt is raised, legacy string routing, and everyinvalid_argumentpathperms.suite.tsexercisesrequestandcheckin both forms on node/workerd, plus the batch that resolves with no prompt at all (both folders already readable, so it runs on every platform)/auth/revoke-user-user's registration and itsrequireUserActorgate are asserted from the route decorator metadata, so a future cleanup deleting it fails loudlyuser_to_group_permissionsrow, assertscheck()is true, deletes it, asserts falseNot re-run: the Playwright
requestPermission.spec.jssuite (33 passed earlier on this branch). It drives the GUI consent dialog throughputer.ui.requestPermissionand thegrant/revoke-user-approutes; it does not referenceputer.perms, and none of that changed here.Notes for reviewers
revoke-user-usercame back deliberately, for the backward-compat reason above; the other seven and the 501 stub stand.never queries group membership to resolve a user permissionspied onGroupStore.listGroupsWithMember, which this PR removes, so the test couldn't survive. The invariant now holds by construction: the onlyjct_user_groupreference left in production code is the inlineJOINinsidereadUserGroupPerms— one query, no membership enumeration — and the method it guarded no longer exists to call. Happy to add a replacement guard if reviewers want belt-and-braces.@internaldoes not do what AGENTS.md line 118 says. It claims tagged members are stripped from the generated declarations.stripInternalhas no effect on declarations emitted from JavaScript — verified with an isolated repro, and a pre-existing@internalmethod insrc/puter-js/src/index.jsalso survives intotypes/. That's why the aliases here are marked@deprecatedonly. Worth a separate docs fix.checkis the authority on what is held, and the batch consults it before prompting; single-resourcerequestkeeps its existing flow unchanged (requestFolder, for instance, always prompts forwrite). For an app-under-user actor — the audience for these calls — the two agree, since the app holds nothing implicitly. Happy to align the single path too if you'd rather, but that changes a shipped method's flow and felt out of scope here.