27 tests on the file standing between a member and a shell
authorize.ts had none, and its own header is what says why that mattered: every deny path in the platform ends there, and the failure mode of a permissive default is not a bug report, it is someone else's session. It had none for a structural reason rather than an oversight. Every path needed two database calls, and the only file here 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 are split from the fetching. resolveEffectivePermissions, decideApiRequest and decideWsProvider are pure functions of their arguments; the async wrappers keep the role cache, the fail-closed catches and the shape callers use. No behaviour change, and the live gate still answers 401 on file-browser, users and tasks. 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. If a key here disappears this file goes red. 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 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 method AND path; a socket needs no level; an unauthenticated caller is refused every door. Two gaps are stated in the file rather than left to be found: ownerOnly has no declaring permission yet, so testing it would test the fixtures; and the grant cache's contract is a claim about call sites, not 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, now commented at the test that hit it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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<string, PermissionLevel>(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.
|
||||
*/
|
||||
@@ -65,6 +65,57 @@ async function grantsForRole(role: UserRole): Promise<Map<string, PermissionLeve
|
||||
* write such a row, but this is the layer that has to hold if one ever exists — a constraint the database
|
||||
* does not enforce is a constraint the reader must.
|
||||
*/
|
||||
/**
|
||||
* The resolution itself, with no database in it.
|
||||
*
|
||||
* Split out on 2026-08-22 so it can be tested. `authorize.ts` is described in its own header as the file
|
||||
* standing between a Member and a shell, and it had no automated tests at all — because every path through
|
||||
* it needed two database calls, and the only file in this repository that mocks `officerdb` has a stub that
|
||||
* leaks into whatever runs after it. Pulling the decision away from the fetching removes the need for a
|
||||
* mock rather than working around one: everything below this line is a pure function of two arguments.
|
||||
*
|
||||
* The async wrapper keeps the caching, the error handling and the single-question shape callers rely on.
|
||||
*/
|
||||
export function resolveEffectivePermissions(
|
||||
user: { role: UserRole; osUser: string | null },
|
||||
roleGrants: Map<string, PermissionLevel>,
|
||||
): EffectivePermissions {
|
||||
if (user.role === 'Super Admin') return { isOwner: true, grants: new Map() };
|
||||
|
||||
const grants = new Map<string, PermissionLevel>();
|
||||
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<EffectivePermissions> {
|
||||
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<string, PermissionLevel>();
|
||||
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<boolean> {
|
||||
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);
|
||||
|
||||
Reference in New Issue
Block a user