diff --git a/src/databases/officer_db/src/permissions/index.ts b/src/databases/officer_db/src/permissions/index.ts index f6a692b0..2cde4c64 100644 --- a/src/databases/officer_db/src/permissions/index.ts +++ b/src/databases/officer_db/src/permissions/index.ts @@ -1,4 +1,11 @@ -export { getAllRoleGrants, getRoleGrants, setRoleGrant, revokeRoleGrant, replaceRoleGrants } from './queries'; +export { + getAllRoleGrants, + getRoleGrants, + setRoleGrant, + revokeRoleGrant, + replaceRoleGrants, + revokeGrantsForPermissions, +} from './queries'; export type { RoleGrant } from './queries'; diff --git a/src/databases/officer_db/src/permissions/queries.ts b/src/databases/officer_db/src/permissions/queries.ts index 0bef401c..d5c7fcc8 100644 --- a/src/databases/officer_db/src/permissions/queries.ts +++ b/src/databases/officer_db/src/permissions/queries.ts @@ -1,4 +1,4 @@ -import { eq, and } from 'drizzle-orm'; +import { eq, and, inArray } from 'drizzle-orm'; import { db } from '../db'; import { rolePermissions } from './schema'; import type { UserRole } from '../auth/schema'; @@ -75,3 +75,23 @@ export async function replaceRoleGrants( } }); } + +/** + * Drop every role's grants for these permission keys. + * + * Called when a plugin is uninstalled: its keys stop existing, and a grant naming one is a row nobody + * can see, edit or remove through the UI. Left in place they made the whole permissions screen + * unsaveable — the page loads every grant, submits them back, and the write refused a key it no longer + * recognised. + * + * Across ALL roles, not one: an uninstall is not per-role, and leaving another role's copy behind would + * reproduce the same dead screen for whoever opened that tab. + */ +export async function revokeGrantsForPermissions(permissions: string[]): Promise { + if (!permissions.length) return 0; + const removed = await db + .delete(rolePermissions) + .where(inArray(rolePermissions.permission, permissions)) + .returning({ id: rolePermissions.id }); + return removed.length; +} diff --git a/src/servers/api/users/permissions-routes.ts b/src/servers/api/users/permissions-routes.ts index 1802b8ba..e6401c42 100644 --- a/src/servers/api/users/permissions-routes.ts +++ b/src/servers/api/users/permissions-routes.ts @@ -125,6 +125,8 @@ permissionAdminRouter.put('/permissions/:role', ownerGate, async (ctx) => { if (!Array.isArray(raw)) throw errors.BAD_REQUEST('Expected { grants: [{ permission, level }] }'); const grants: { permission: string; level: 'read' | 'write' }[] = []; + /** Keys nothing defines — almost always a plugin that has since been uninstalled. */ + const dropped: string[] = []; for (const entry of raw) { const { permission, level } = (entry ?? {}) as { permission?: unknown; level?: unknown }; if (typeof permission !== 'string') throw errors.BAD_REQUEST('Each grant needs a permission key'); @@ -133,8 +135,21 @@ permissionAdminRouter.put('/permissions/:role', ownerGate, async (ctx) => { // The registry is the authority on what a permission key means, which is why the column has no CHECK. // This is where that authority is applied — rejecting a name nothing defines, and refusing to store a // grant the resolver would drop on read anyway. + // An unknown key is DROPPED, not refused. + // + // It used to reject the whole request, and that turned an uninstall into a dead settings screen: the + // grants for a removed plugin are still in `role_permissions`, the page loads them, and submitting + // them back fails with "Unknown permission 'music'" — for a row the owner never touched and cannot + // see. Nothing could be saved again until somebody edited the database. + // + // Dropping is also what the resolver already does on read, so this only makes the write agree with + // it. The cost is that a typo in a hand-made API call is silently ignored rather than reported; + // that is worth less than a settings page that cannot be used. const known = PERMISSION_BY_KEY.get(permission); - if (!known) throw errors.BAD_REQUEST(`Unknown permission '${permission}'`); + if (!known) { + dropped.push(permission); + continue; + } if (known.kind !== 'app' && known.kind !== 'confined') { throw errors.BAD_REQUEST( known.kind === 'execution' @@ -145,10 +160,14 @@ permissionAdminRouter.put('/permissions/:role', ownerGate, async (ctx) => { grants.push({ permission, level }); } + if (dropped.length) console.log(`[permissions] ${role}: ignored unknown key(s) ${dropped.join(', ')}`); + await replaceRoleGrants(role, grants); // The cache's entire invalidation contract, discharged here. Adding a second writer means adding a // second call to this — see the note on grantCache in permissions/authorize.ts. invalidateRoleGrants(role); - return ctx.json({ role, grants }); + // `dropped` is returned so the screen can say what it quietly discarded, rather than the owner + // finding out later that a grant they thought they saved is gone. + return ctx.json({ role, grants, dropped }); }); diff --git a/src/servers/api/users/stale-grants.test.ts b/src/servers/api/users/stale-grants.test.ts new file mode 100644 index 00000000..a7d5556a --- /dev/null +++ b/src/servers/api/users/stale-grants.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, test } from 'bun:test'; +import { PERMISSION_BY_KEY } from '@@/permissions/registry'; + +// A plugin's permission keys stop existing the moment it is uninstalled, but its grants stay in +// `role_permissions`. The settings page loads every grant and submits them all back, so one orphan made +// the entire screen unsaveable — for a row nobody could see, edit or remove through the UI. +// +// Two things now hold. Uninstall takes the grants with it, and the write drops a key it does not know +// instead of refusing the request. + +const partition = (submitted: string[]) => { + const kept: string[] = []; + const dropped: string[] = []; + for (const key of submitted) (PERMISSION_BY_KEY.get(key) ? kept : dropped).push(key); + return { kept, dropped }; +}; + +describe('a grant naming a permission that no longer exists', () => { + test('is separated from the ones that do, rather than failing the batch', () => { + // `music` is the real case: a plugin, uninstalled, its grants left behind. + const { kept, dropped } = partition(['chat', 'music', 'terminal']); + expect(dropped).toEqual(['music']); + expect(kept).toEqual(['chat', 'terminal']); + }); + + test('a batch of nothing but unknown keys still leaves the known set empty, not undefined', () => { + expect(partition(['music', 'photos-that-never-existed']).kept).toEqual([]); + }); + + test('core keys are never treated as stale', () => { + // The failure mode to avoid is over-correcting: dropping a key the registry does define would + // silently revoke access the owner asked for. + for (const key of ['chat', 'terminal', 'files']) { + expect(partition([key]).dropped).toEqual([]); + } + }); +}); diff --git a/src/servers/plugins/install.ts b/src/servers/plugins/install.ts index f4778735..b2f996dd 100644 --- a/src/servers/plugins/install.ts +++ b/src/servers/plugins/install.ts @@ -14,6 +14,7 @@ import { deleteProcess, processStatus, startProcess, stopProcess } from '../app- import { addPluginToEcosystem, pluginProcessName, removePluginFromEcosystem } from './ecosystem'; import { discoverPlugins } from './discover'; import { fetchPlugin } from './fetch'; +import { revokeGrantsForPermissions } from 'officerdb'; import { publishAssets, unpublishAssets } from '../app-store/assets'; import { publishAffordances, unpublishAffordances } from '../affordances/publish'; import { refreshPluginMounts, snapshotPlugins } from './mount'; @@ -430,6 +431,20 @@ export async function uninstallPlugin(appName: string, onStep?: OnStep, onLog?: // The one thing uninstall IS allowed to delete: published assets are copies, and the originals are // still in the plugin's own directory. Nothing a user made is in there. + // Its grants go too. The keys stop existing when the plugin is unmounted, and a grant naming one is a + // row nobody can see, edit or remove through the UI — which made the whole permissions screen + // unsaveable, because the page submits every grant it loaded and the write refused a key it no longer + // recognised. Reported on a real server after uninstalling music. + // + // This is a deliberate exception to "uninstall deletes nothing": a grant is the PLATFORM's record of + // who may reach the plugin, not anything the plugin stored. Reinstalling means granting again, which + // is the honest behaviour anyway — the alternative is access silently returning. + if (plugin) { + const keys = plugin.manifest.permissions.map((p) => p.key); + const revoked = await revokeGrantsForPermissions(keys).catch(() => 0); + if (revoked) await step(steps, onStep, `grants: ${revoked} removed for ${keys.join(', ')}`); + } + await unpublishAssets(appName); // Same reasoning: a copy, with the original still in the plugin's directory. Removed so an agent stops // being told about a surface that is no longer there — an affordance for an uninstalled plugin is