diff --git a/TODO.md b/TODO.md index 9d874cc6..2f43ed05 100644 --- a/TODO.md +++ b/TODO.md @@ -20,7 +20,7 @@ the owner's OS user and can never be granted. Indirection there really is accide ## Themes -**85 open, 24 closed**, across 29 themes. +**84 open, 25 closed**, across 29 themes. | Theme | Open | Closed | | --- | --- | --- | @@ -37,7 +37,7 @@ the owner's OS user and can never be granted. Indirection there really is accide | [Infra (alpha)](todo/infra/) | 6 | 0 | | [Known bugs](todo/known-bugs/) | 3 | 2 | | [Mobile apps](todo/mobile-apps/) | 3 | 0 | -| [Multi-user](todo/multi-user/) | 17 | 4 | +| [Multi-user](todo/multi-user/) | 16 | 5 | | [Notifications](todo/notifications/) | 1 | 0 | | [Plugin ecosystem — before it opens to third parties (2026-08-18)](todo/plugin-ecosystem/) | 3 | 0 | | [Plugin infrastructure](todo/plugin-infrastructure/) | 2 | 0 | diff --git a/src/servers/permissions/authorize.test.ts b/src/servers/permissions/authorize.test.ts new file mode 100644 index 00000000..7d2985b1 --- /dev/null +++ b/src/servers/permissions/authorize.test.ts @@ -0,0 +1,216 @@ +import { describe, expect, test } from 'bun:test'; +import type { UserRole } from 'officerdb'; +import type { EffectivePermissions } from './authorize'; +import { decideApiRequest, decideWsProvider, resolveEffectivePermissions } from './authorize'; +import type { PermissionLevel } from './registry'; +import { CORE_PERMISSIONS } from './registry'; + +/* + The resolver, which its own header calls the file standing between a Member and a shell — and which had no + automated tests at all until 2026-08-22. + + It had none because every path needed two database calls. The decisions are now split from the fetching + (`resolveEffectivePermissions`, `decideApiRequest`, `decideWsProvider` are pure), so this file needs no + mock, no database and no `officerdb` stub — which matters beyond convenience: the one file in this + repository that mocks `officerdb` leaks its stub into whatever runs after it. + + REAL registry keys throughout, deliberately. A suite built on invented permissions would keep passing + after a rename, which is the failure mode a test of an authorization table can least afford. If a key here + disappears, this file should go red. +*/ + +const member = (osUser: string | null = 'm1'): { role: UserRole; osUser: string | null } => ({ + role: 'Member', + osUser, +}); +const grants = (...pairs: Array<[string, PermissionLevel]>) => new Map(pairs); +const resolve = (user: { role: UserRole; osUser: string | null }, g = grants()) => resolveEffectivePermissions(user, g); + +/** No account at all — an unauthenticated or deleted caller, as the async wrapper reports it. */ +const NOBODY: EffectivePermissions = { isOwner: false, grants: new Map() }; + +describe('resolving what an account holds', () => { + test('the owner bypasses, and carries no grants to consult', () => { + const owner = resolve({ role: 'Super Admin', osUser: null }); + expect(owner.isOwner).toBe(true); + // Empty on purpose: an owner's grants are never read, and filling them would invite somebody to. + expect(owner.grants.size).toBe(0); + }); + + test('a member gets every core permission at write, granted nothing', () => { + const held = resolve(member()).grants; + for (const permission of CORE_PERMISSIONS) expect(held.get(permission.key)).toBe('write'); + }); + + test('core is write even though nothing granted it', () => { + // A read-only "change your own password" is not a coherent thing to offer, which is why core is not + // levelled. Pinned because a future levels-everywhere refactor would silently break it. + expect(resolve(member()).grants.get('account')).toBe('write'); + }); + + test('an app grant is honoured at the level given', () => { + expect(resolve(member(), grants(['calendar', 'read'])).grants.get('calendar')).toBe('read'); + expect(resolve(member(), grants(['calendar', 'write'])).grants.get('calendar')).toBe('write'); + }); + + // The rule that only exists because getOwnerHomeDir ignores its argument: a confined grant with no Linux + // user to confine to would resolve to the OWNER's home. + test('a confined grant needs a Linux account', () => { + expect(resolve(member('m1'), grants(['files', 'write'])).grants.get('files')).toBe('write'); + expect(resolve(member(null), grants(['files', 'write'])).grants.has('files')).toBe(false); + }); + + test('every confined permission is dropped together, not one by one', () => { + const held = resolve(member(null), grants(['files', 'write'], ['terminal', 'write'], ['chat', 'write'])); + for (const key of ['files', 'terminal', 'chat']) expect(held.grants.has(key)).toBe(false); + }); + + // The database has no CHECK stopping these rows; the API refuses to write one. This is the layer that + // has to hold if one ever exists. + test('an execution grant in the table is ignored', () => { + expect(resolve(member(), grants(['tasks', 'write'])).grants.has('tasks')).toBe(false); + }); + + test('an admin grant in the table is ignored', () => { + expect(resolve(member(), grants(['user-admin', 'write'])).grants.has('user-admin')).toBe(false); + expect(resolve(member(), grants(['server-admin', 'write'])).grants.has('server-admin')).toBe(false); + }); + + test('a grant naming a permission that no longer exists is ignored', () => { + const held = resolve(member(), grants(['cliamp', 'write'], ['a-plugin-uninstalled-last-year', 'read'])); + expect(held.grants.has('cliamp')).toBe(false); + expect(held.grants.has('a-plugin-uninstalled-last-year')).toBe(false); + // …and does not disturb what is legitimately held. + expect(held.grants.get('account')).toBe('write'); + }); +}); + +describe('deciding an HTTP request', () => { + const owner: EffectivePermissions = { isOwner: true, grants: new Map() }; + + test('the owner is allowed anything, including paths nothing serves', () => { + expect(decideApiRequest(owner, 'DELETE', '/api/users/9').allowed).toBe(true); + expect(decideApiRequest(owner, 'GET', '/api/nothing-serves-this').allowed).toBe(true); + }); + + test('a path no permission covers is refused, not allowed by default', () => { + const d = decideApiRequest(NOBODY, 'GET', '/api/nothing-serves-this'); + expect(d.allowed).toBe(false); + expect(d.reason).toContain('no permission covers this path'); + }); + + test('an execution permission is refused however it is granted', () => { + // Refused by KIND, before any grant lookup — so a row granting it changes nothing. + const forged = resolve(member(), grants(['tasks', 'write'])); + const d = decideApiRequest(forged, 'GET', '/api/tasks'); + expect(d.allowed).toBe(false); + expect(d.reason).toContain('runs as the server owner'); + }); + + test('an admin permission is refused', () => { + const d = decideApiRequest(resolve(member()), 'GET', '/api/users'); + expect(d.allowed).toBe(false); + expect(d.reason).toContain('restricted to the server owner'); + }); + + // The one self-service route in the registry: updating your own profile lives on the router that also + // holds account administration. + test('a self-service route is allowed with no grant at all', () => { + expect(decideApiRequest(resolve(member()), 'PUT', '/api/users').allowed).toBe(true); + }); + + test('self-service is exact — neither the method nor the path may drift', () => { + expect(decideApiRequest(resolve(member()), 'POST', '/api/users').allowed).toBe(false); + expect(decideApiRequest(resolve(member()), 'PUT', '/api/users/7').allowed).toBe(false); + }); + + test('no grant means no access, with the permission named', () => { + const d = decideApiRequest(resolve(member()), 'GET', '/api/file-browser'); + expect(d.allowed).toBe(false); + expect(d.reason).toContain('your role does not have access to'); + }); + + test('read grants reads and refuses writes', () => { + const held = resolve(member('m1'), grants(['files', 'read'])); + expect(decideApiRequest(held, 'GET', '/api/file-browser').allowed).toBe(true); + const write = decideApiRequest(held, 'POST', '/api/file-browser'); + expect(write.allowed).toBe(false); + expect(write.reason).toContain('read-only'); + }); + + test('write grants both', () => { + const held = resolve(member('m1'), grants(['files', 'write'])); + expect(decideApiRequest(held, 'GET', '/api/file-browser').allowed).toBe(true); + expect(decideApiRequest(held, 'POST', '/api/file-browser').allowed).toBe(true); + }); + + // A read that has to be a POST — the body is too big for a query string — must not need write. + // + // The path is `/api/file-browser/download`, not `/api/download`: readOnlyWrites entries are RELATIVE to + // the permission's own api prefix. Worth a comment because the absolute form looks right, is what I wrote + // first, and fails as "no permission covers this path" rather than as a level problem — a misleading + // error for a misleading path. + test('a readOnlyWrites path is allowed at read level', () => { + const held = resolve(member('m1'), grants(['files', 'read'])); + expect(decideApiRequest(held, 'POST', '/api/file-browser/download').allowed).toBe(true); + // And the same POST elsewhere under the prefix still needs write. + expect(decideApiRequest(held, 'POST', '/api/file-browser/move').allowed).toBe(false); + }); + + // The two rules composing: granted, but with no kernel boundary to be granted inside. + test('a granted confined permission is still refused without a Linux account', () => { + const held = resolve(member(null), grants(['files', 'write'])); + expect(decideApiRequest(held, 'GET', '/api/file-browser').allowed).toBe(false); + }); + + test('an unauthenticated caller is refused everything', () => { + for (const path of ['/api/file-browser', '/api/chat', '/api/users', '/api/tasks']) { + expect(decideApiRequest(NOBODY, 'GET', path).allowed).toBe(false); + } + }); +}); + +describe('deciding a websocket', () => { + test('the owner opens anything', () => { + expect(decideWsProvider({ isOwner: true, grants: new Map() }, 'terminal')).toBe(true); + }); + + test('a provider no permission claims is refused', () => { + expect(decideWsProvider(resolve(member()), 'not-a-provider')).toBe(false); + }); + + test('a confined socket needs the grant AND the Linux account', () => { + expect(decideWsProvider(resolve(member('m1'), grants(['terminal', 'read'])), 'terminal')).toBe(true); + expect(decideWsProvider(resolve(member(null), grants(['terminal', 'read'])), 'terminal')).toBe(false); + }); + + // No method to reason about, so any level opens it. Pinned because "read-only socket" is a tempting idea + // that would silently close terminals granted at read. + test('read is enough — a socket has no method', () => { + expect(decideWsProvider(resolve(member('m1'), grants(['chat', 'read'])), 'chat')).toBe(true); + expect(decideWsProvider(resolve(member('m1'), grants(['chat', 'read'])), 'chat-notify')).toBe(true); + }); + + test("an execution permission's socket is refused however it is granted", () => { + const forged = resolve(member('m1'), grants(['tasks', 'write'])); + expect(decideWsProvider(forged, 'task-runner')).toBe(false); + expect(decideWsProvider(forged, 'pipeline')).toBe(false); + }); + + test('an unauthenticated caller opens nothing', () => { + for (const p of ['terminal', 'chat', 'task-runner', 'sidecar']) expect(decideWsProvider(NOBODY, p)).toBe(false); + }); +}); + +/* + NOT covered here, and stated rather than left as a gap somebody has to notice: + + - `isOwnerOnlyRoute`. No permission in the registry declares `ownerOnly` yet — the field was added on + 2026-08-21 for plugin settings routes and has no user. It is checked before self-service and before + kind, which is the part worth a test, and it cannot get one until a real permission uses it. Covering it + with an invented permission would test this file's fixtures rather than the registry. + - The grant cache and `invalidateRoleGrants`. They live on the async side, around a database call, which + is exactly the half this split moved away from. Their contract — the one writer in + `api/users/permissions-routes.ts` calls the invalidator — is a claim about call sites, not about logic, + and a test here could only re-state it. +*/ diff --git a/src/servers/permissions/authorize.ts b/src/servers/permissions/authorize.ts index 0afd49d6..89fdbeb3 100644 --- a/src/servers/permissions/authorize.ts +++ b/src/servers/permissions/authorize.ts @@ -65,6 +65,57 @@ async function grantsForRole(role: UserRole): Promise, +): EffectivePermissions { + if (user.role === 'Super Admin') return { isOwner: true, grants: new Map() }; + + const grants = new Map(); + for (const permission of CORE_PERMISSIONS) grants.set(permission.key, 'write'); + + // Whether the kernel can enforce a boundary for this account. `confined` permissions are dropped + // without it — see below. + const hasOsAccount = !!user.osUser; + + for (const [key, level] of roleGrants) { + const permission = PERMISSION_BY_KEY.get(key); + // Unknown key: a permission that was renamed or removed while a grant survived. Ignore it — the + // alternative is honouring a name nothing defines. + if (!permission) continue; + + // A confined permission touches the filesystem or runs a process, and is safe only because the + // account has its own Linux user to be confined to. Without one there is no boundary, so the grant + // resolves to nothing rather than to the owner's home — which is what it WOULD resolve to, since + // `getOwnerHomeDir` ignores the email it is passed, always. + // + // Dropped here rather than refused per-router so that one rule covers the HTTP routes, the + // websocket doors and the dock all at once. A member with `files` granted but no OS account sees no + // Files icon, gets a 403 from /api/file-browser, and cannot open the terminal socket — from this. + if (permission.kind === 'confined') { + if (!hasOsAccount) continue; + grants.set(key, level); + continue; + } + + if (permission.kind !== 'app') continue; + grants.set(key, level); + } + + return { isOwner: false, grants }; +} + export async function getEffectivePermissions(userId: number | undefined): Promise { const empty: EffectivePermissions = { isOwner: false, grants: new Map() }; if (!userId) return empty; @@ -73,39 +124,7 @@ export async function getEffectivePermissions(userId: number | undefined): Promi const user = await getUserById(userId); if (!user) return empty; if (user.role === 'Super Admin') return { isOwner: true, grants: new Map() }; - - const grants = new Map(); - for (const permission of CORE_PERMISSIONS) grants.set(permission.key, 'write'); - - // Whether the kernel can enforce a boundary for this account. `confined` permissions are dropped - // without it — see below. - const hasOsAccount = !!user.osUser; - - for (const [key, level] of await grantsForRole(user.role)) { - const permission = PERMISSION_BY_KEY.get(key); - // Unknown key: a permission that was renamed or removed while a grant survived. Ignore it — the - // alternative is honouring a name nothing defines. - if (!permission) continue; - - // A confined permission touches the filesystem or runs a process, and is safe only because the - // account has its own Linux user to be confined to. Without one there is no boundary, so the grant - // resolves to nothing rather than to the owner's home — which is what it WOULD resolve to, since - // `getOwnerHomeDir` ignores the email it is passed, always. - // - // Dropped here rather than refused per-router so that one rule covers the HTTP routes, the - // websocket doors and the dock all at once. A member with `files` granted but no OS account sees no - // Files icon, gets a 403 from /api/file-browser, and cannot open the terminal socket — from this. - if (permission.kind === 'confined') { - if (!hasOsAccount) continue; - grants.set(key, level); - continue; - } - - if (permission.kind !== 'app') continue; - grants.set(key, level); - } - - return { isOwner: false, grants }; + return resolveEffectivePermissions(user, await grantsForRole(user.role)); } catch { // A transient database error must never grant anything. Deny, and let the next request retry. return empty; @@ -123,7 +142,16 @@ export async function isApiRequestAllowed( method: string, path: string, ): Promise<{ allowed: boolean; reason?: string }> { - const { isOwner, grants } = await getEffectivePermissions(userId); + return decideApiRequest(await getEffectivePermissions(userId), method, path); +} + +/** The decision, given an already-resolved account. Pure, and where every rule below actually lives. */ +export function decideApiRequest( + effective: EffectivePermissions, + method: string, + path: string, +): { allowed: boolean; reason?: string } { + const { isOwner, grants } = effective; if (isOwner) return { allowed: true }; const permission = permissionForApiPath(path); @@ -170,7 +198,12 @@ export async function isApiRequestAllowed( * rather than by being left off a list. The rule stays because the first plugin to own a socket needs it. */ export async function isWsProviderAllowed(userId: number | undefined, provider: string): Promise { - const { isOwner, grants } = await getEffectivePermissions(userId); + return decideWsProvider(await getEffectivePermissions(userId), provider); +} + +/** The decision, given an already-resolved account. See `isWsProviderAllowed` for why level is not read. */ +export function decideWsProvider(effective: EffectivePermissions, provider: string): boolean { + const { isOwner, grants } = effective; if (isOwner) return true; const permission = permissionForWsProvider(provider); diff --git a/todo/multi-user/permissions-authorize-ts-has-no-automated.md b/todo/multi-user/permissions-authorize-ts-has-no-automated.md index 5bfec8b2..9b714462 100644 --- a/todo/multi-user/permissions-authorize-ts-has-no-automated.md +++ b/todo/multi-user/permissions-authorize-ts-has-no-automated.md @@ -1,8 +1,8 @@ --- id: multi-user/permissions-authorize-ts-has-no-automated -title: "`permissions/authorize.ts` has no automated tests" +title: '`permissions/authorize.ts` has no automated tests' theme: multi-user -status: open +status: done created: 2026-08-20 date_source: assumed order: 11 @@ -12,3 +12,32 @@ order: 11 registry functions and the totality check; the resolver that does the owner bypass, the grant lookup, the role cache and the fail-closed catches is exercised only by hand. It is the file standing between a Member and a shell. + +--- + +**Done 2026-08-22. 27 tests, no mock and no database.** + +The reason it had none was structural: every path through the resolver needed two database calls, and the +only file in this repository that mocks `officerdb` leaks its stub into whatever runs after it — so the +obvious approach would have traded one untested file for a flaky suite. + +So the decisions were split from the fetching. `resolveEffectivePermissions(user, roleGrants)`, +`decideApiRequest(effective, method, path)` and `decideWsProvider(effective, provider)` are pure; the async +functions keep the caching, the fail-closed catches and the shape callers use. The tests use REAL registry +keys throughout, deliberately — a suite built on invented permissions keeps passing after a rename, which is +the failure an authorization table can least afford. + +What is pinned, beyond the happy paths: the owner carries no grants to consult; core is `write` +unconditionally; a confined grant is dropped without a Linux user, and all three confined permissions drop +together; execution and admin grants in the table are ignored even though no CHECK stops the row existing; +an unknown key is ignored without disturbing what is legitimately held; self-service is exact in both method +and path; `readOnlyWrites` is relative to the permission's prefix; a socket needs no level; and an +unauthenticated caller is refused every door. + +Two things deliberately NOT covered, and said so in the file rather than left to be noticed: `ownerOnly`, +because no permission declares it yet and covering it would test the fixtures rather than the registry; and +the grant cache, whose contract is a claim about call sites rather than logic. + +Found while writing them: `readOnlyWrites` paths are relative to the permission's api prefix, and getting +that wrong fails as "no permission covers this path" rather than as a level error — a misleading message for +a misleading path.