From 5502a3eaa83f7d3737d0a49790ee613031cbf4e6 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Fri, 5 Jun 2026 12:44:39 -0500 Subject: [PATCH] fix(roles): make platform_operator assignable + add deny/assign regression tests The bug: #13 added 'platform_operator' to the frontend role dropdown (PLATFORM_ROLE_OPTIONS) but #14's PUT /api/auth/users/:id/role whitelist (ASSIGNABLE_PLATFORM_ROLES) only listed ['user','platform_admin'], so selecting "Platform operator" returned 400 "Invalid role" - the role was unassignable via the UI. Fix: add 'platform_operator' to ASSIGNABLE_PLATFORM_ROLES. One line; the self-demote guard is intentionally left untouched (a platform_admin still cannot self-assign the non-owner operator role and lock themselves out). Tests (node:test, isolated in-memory DB injection - no DB_PATH change): - admin-users.test.js: platform_admin can PUT role=platform_operator on a target user -> 200 and the row persists as platform_operator (regression guard for the whitelist gap). - operator-permissions.test.js (new): verify-then-test of the highest-blast -radius deny. Operator CAN update/delete a workspace-scoped content row (cross-org write works) but is denied (403) updating or deleting a shared (workspace_id IS NULL) row - proving the separate PLATFORM_ROLES gate in content.js's checkContentWrite still holds after canWrite was broadened to isPlatformStaff. Verified read-only (no leak): the other shared-asset write sites keep their PLATFORM_ROLES gate that excludes operator - kiosk.js:57, widgets.js:110, folders.js:31, layouts.js:59/117/133. cd server && npm test -> 12 pass / 0 fail. Co-Authored-By: Claude Opus 4.8 (1M context) --- server/routes/auth.js | 2 +- server/test/admin-users.test.js | 14 +++ server/test/operator-permissions.test.js | 129 +++++++++++++++++++++++ 3 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 server/test/operator-permissions.test.js diff --git a/server/routes/auth.js b/server/routes/auth.js index ff64cda..dab360e 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -480,7 +480,7 @@ router.delete('/users/:id', requireAuth, requireSuperAdmin, (req, res) => { // org roles are managed in the members views. Whitelist is the current model: // 'user' and 'platform_admin' (the legacy 'admin'/'superadmin' strings are gone // after normalization and are no longer accepted here). -const ASSIGNABLE_PLATFORM_ROLES = ['user', 'platform_admin']; +const ASSIGNABLE_PLATFORM_ROLES = ['user', 'platform_operator', 'platform_admin']; router.put('/users/:id/role', requireAuth, requireSuperAdmin, (req, res) => { const { role } = req.body; if (!ASSIGNABLE_PLATFORM_ROLES.includes(role)) return res.status(400).json({ error: 'Invalid role' }); diff --git a/server/test/admin-users.test.js b/server/test/admin-users.test.js index 44f0555..a0c6240 100644 --- a/server/test/admin-users.test.js +++ b/server/test/admin-users.test.js @@ -112,6 +112,9 @@ const orgAdminA = seedUser({ id: 'u-orgadmin-a', email: 'orgadmin-a@test.local', db.prepare("INSERT INTO organization_members (organization_id, user_id, role) VALUES ('org-a','u-orgadmin-a','org_admin')").run(); const operator = seedUser({ id: 'u-operator', email: 'operator@test.local', role: 'platform_operator' }); const regular = seedUser({ id: 'u-regular', email: 'regular@test.local', role: 'user' }); +// Dedicated target for the role-assignment regression test (kept separate so it +// can't perturb the non-admin/operator tokens used by the deny tests above). +seedUser({ id: 'u-role-target', email: 'role-target@test.local', role: 'user' }); const tokens = { admin: generateToken(adminUser, null), @@ -224,3 +227,14 @@ test('must_change_password lifecycle: set on create, surfaced on login, cleared const row = db.prepare('SELECT must_change_password FROM users WHERE email=?').get('created@test.local'); assert.equal(row.must_change_password, 0, 'flag cleared in the DB'); }); + +test('platform_operator is assignable via PUT /users/:id/role (regression for #13/#14 whitelist gap)', async () => { + const res = await fetch(base + '/api/auth/users/u-role-target/role', { + method: 'PUT', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${tokens.admin}` }, + body: JSON.stringify({ role: 'platform_operator' }), + }); + assert.equal(res.status, 200); + const dbRole = db.prepare('SELECT role FROM users WHERE id = ?').get('u-role-target').role; + assert.equal(dbRole, 'platform_operator', 'role actually persisted as platform_operator'); +}); diff --git a/server/test/operator-permissions.test.js b/server/test/operator-permissions.test.js new file mode 100644 index 0000000..514253f --- /dev/null +++ b/server/test/operator-permissions.test.js @@ -0,0 +1,129 @@ +'use strict'; + +// #13 regression: platform_operator gets cross-org workspace read/write (via the +// canWrite broadening to isPlatformStaff) but must STILL be denied writing +// shared/global assets (workspace_id IS NULL), which carry a SEPARATE +// PLATFORM_ROLES gate on top of canWrite. This is the highest-blast-radius deny +// (operator editing platform-wide content), so we prove both halves: +// (a) operator CAN update/delete a workspace-scoped content row, and +// (b) operator CANNOT update/delete a shared (workspace_id IS NULL) row. +// +// Same isolated-in-memory-DB harness as admin-users.test.js: inject the DB into +// the require cache before any module that pulls ../db/database loads. Node v20 +// built-ins only. (node --test runs each file in its own process, so this +// injection does not collide with the other suite's.) + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const Database = require('better-sqlite3'); +const { v4: uuidv4 } = require('uuid'); + +process.env.JWT_SECRET = 'test-secret-operator-perms'; + +const db = new Database(':memory:'); +db.pragma('foreign_keys = ON'); +db.exec(` + CREATE TABLE users ( + id TEXT PRIMARY KEY, email TEXT UNIQUE NOT NULL, name TEXT NOT NULL DEFAULT '', + password_hash TEXT, auth_provider TEXT NOT NULL DEFAULT 'local', avatar_url TEXT, + role TEXT NOT NULL DEFAULT 'user', plan_id TEXT DEFAULT 'free', email_alerts INTEGER DEFAULT 1, + must_change_password INTEGER NOT NULL DEFAULT 0, + created_at INTEGER NOT NULL DEFAULT (strftime('%s','now')) + ); + CREATE TABLE workspaces ( + id TEXT PRIMARY KEY, organization_id TEXT NOT NULL, name TEXT NOT NULL, slug TEXT, + created_at INTEGER NOT NULL DEFAULT (strftime('%s','now')) + ); + CREATE TABLE organization_members ( + id INTEGER PRIMARY KEY AUTOINCREMENT, organization_id TEXT NOT NULL, user_id TEXT NOT NULL, + role TEXT NOT NULL, UNIQUE(organization_id, user_id) + ); + CREATE TABLE workspace_members ( + id INTEGER PRIMARY KEY AUTOINCREMENT, workspace_id TEXT NOT NULL, user_id TEXT NOT NULL, + role TEXT NOT NULL, joined_at INTEGER NOT NULL DEFAULT (strftime('%s','now')), + UNIQUE(workspace_id, user_id) + ); + CREATE TABLE content ( + id TEXT PRIMARY KEY, filename TEXT NOT NULL, filepath TEXT NOT NULL, mime_type TEXT NOT NULL, + file_size INTEGER NOT NULL, duration_sec REAL, thumbnail_path TEXT, width INTEGER, height INTEGER, + remote_url TEXT, user_id TEXT, folder TEXT, folder_id TEXT, workspace_id TEXT, + created_at INTEGER NOT NULL DEFAULT (strftime('%s','now')) + ); + -- Empty, but the DELETE handler queries these for playlist cleanup. + CREATE TABLE devices (id TEXT PRIMARY KEY, playlist_id TEXT); + CREATE TABLE playlists (id TEXT PRIMARY KEY, workspace_id TEXT, published_snapshot TEXT); + CREATE TABLE playlist_items (id INTEGER PRIMARY KEY AUTOINCREMENT, playlist_id TEXT, content_id TEXT); +`); + +const dbModulePath = require.resolve('../db/database'); +require.cache[dbModulePath] = { + id: dbModulePath, filename: dbModulePath, loaded: true, + exports: { db, pruneTelemetry() {}, pruneScreenshots() {} }, +}; + +const express = require('express'); +const { generateToken, requireAuth } = require('../middleware/auth'); +const { resolveTenancy } = require('../lib/tenancy'); +const contentRouter = require('../routes/content'); + +// Seed: org + workspace, a platform_operator user, and two content rows. +db.prepare("INSERT INTO workspaces (id, organization_id, name) VALUES ('ws-a','org-a','Workspace A')").run(); +db.prepare("INSERT INTO users (id, email, role) VALUES ('u-op','op@test.local','platform_operator')").run(); +const operator = { id: 'u-op', email: 'op@test.local', role: 'platform_operator' }; +// JWT carries current_workspace_id so resolveTenancy lands the operator (acting-as) in ws-a. +const opToken = generateToken(operator, 'ws-a'); + +const wsContentId = uuidv4(); +const sharedContentId = uuidv4(); +function seedContent(id, workspaceId) { + db.prepare(`INSERT INTO content (id, filename, filepath, mime_type, file_size, workspace_id) + VALUES (?, 'orig.png', '/does/not/exist.png', 'image/png', 123, ?)`).run(id, workspaceId); +} +seedContent(wsContentId, 'ws-a'); // workspace-scoped +seedContent(sharedContentId, null); // shared / platform-global (workspace_id IS NULL) + +const app = express(); +app.use(express.json()); +app.use('/api/content', requireAuth, resolveTenancy, contentRouter); +const server = app.listen(0); +let base; +test.before(async () => { + await new Promise(r => server.listening ? r() : server.once('listening', r)); + base = `http://127.0.0.1:${server.address().port}`; +}); +test.after(() => { server.close(); db.close(); }); + +const op = (method, id, body) => fetch(`${base}/api/content/${id}`, { + method, + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${opToken}` }, + ...(body ? { body: JSON.stringify(body) } : {}), +}); + +// (a) Proves the operator's cross-org write actually works (so (b) isn't just +// "operator can't write anything"). +test('operator CAN update a workspace-scoped content row', async () => { + const res = await op('PUT', wsContentId, { filename: 'renamed.png' }); + assert.equal(res.status, 200); + assert.equal(db.prepare('SELECT filename FROM content WHERE id=?').get(wsContentId).filename, 'renamed.png'); +}); + +// (b) The separate PLATFORM_ROLES gate on workspace_id IS NULL must deny operator. +test('operator CANNOT update a shared (workspace_id IS NULL) content row -> 403', async () => { + const res = await op('PUT', sharedContentId, { filename: 'hijacked.png' }); + assert.equal(res.status, 403); + assert.equal(db.prepare('SELECT filename FROM content WHERE id=?').get(sharedContentId).filename, 'orig.png', + 'shared row must be unchanged'); +}); + +test('operator CANNOT delete a shared (workspace_id IS NULL) content row -> 403', async () => { + const res = await op('DELETE', sharedContentId); + assert.equal(res.status, 403); + assert.ok(db.prepare('SELECT 1 FROM content WHERE id=?').get(sharedContentId), 'shared row must still exist'); +}); + +// Delete last so the workspace-scoped row survives the update assertion above. +test('operator CAN delete a workspace-scoped content row', async () => { + const res = await op('DELETE', wsContentId); + assert.equal(res.status, 200); + assert.ok(!db.prepare('SELECT 1 FROM content WHERE id=?').get(wsContentId), 'workspace row deleted'); +});