diff --git a/frontend/js/api.js b/frontend/js/api.js index 9ec7996..9fb8b82 100644 --- a/frontend/js/api.js +++ b/frontend/js/api.js @@ -193,6 +193,7 @@ export const api = { getItemSchedules: (id, itemId) => request(`/playlists/${id}/items/${itemId}/schedules`), setItemSchedules: (id, itemId, blocks) => request(`/playlists/${id}/items/${itemId}/schedules`, { method: 'PUT', body: JSON.stringify({ blocks }) }), assignPlaylistToDevice: (playlistId, device_id) => request(`/playlists/${playlistId}/assign`, { method: 'POST', body: JSON.stringify({ device_id }) }), + clearDevicePlaylist: (device_id) => request(`/devices/${device_id}/playlist`, { method: 'DELETE' }), publishPlaylist: (id) => request(`/playlists/${id}/publish`, { method: 'POST' }), discardPlaylistDraft: (id) => request(`/playlists/${id}/discard`, { method: 'POST' }), diff --git a/frontend/js/views/device-detail.js b/frontend/js/views/device-detail.js index 07df81b..53b6159 100644 --- a/frontend/js/views/device-detail.js +++ b/frontend/js/views/device-detail.js @@ -1039,10 +1039,15 @@ function setupActions(device) { playlistPicker.addEventListener('change', async () => { const newPlaylistId = playlistPicker.value; - if (!newPlaylistId) return; // Don't allow deselecting for now try { - await api.assignPlaylistToDevice(newPlaylistId, device.id); - device.playlist_id = newPlaylistId; + // Empty value is the "No playlist" option. It used to be discarded right here, so the + // option was offered, selecting it did nothing, and nothing said so (#234). + if (newPlaylistId) { + await api.assignPlaylistToDevice(newPlaylistId, device.id); + } else { + await api.clearDevicePlaylist(device.id); + } + device.playlist_id = newPlaylistId || null; const assignments = await api.getAssignments(device.id); const pc = document.getElementById('playlistContainer'); pc.innerHTML = renderPlaylist(assignments); diff --git a/server/routes/devices.js b/server/routes/devices.js index 30e56b5..e098d59 100644 --- a/server/routes/devices.js +++ b/server/routes/devices.js @@ -215,6 +215,39 @@ router.get('/:id/preview-payload', (req, res) => { }); // Update device +// Clear a device's playlist — the "No playlist" option in the dashboard picker. +// +// There was no way to do this. PUT /devices/:id ignores playlist_id (it always has), and +// POST /playlists/:id/assign can only ever SET one, so the picker carried a guard that +// silently discarded the selection: `if (!newPlaylistId) return; // Don't allow deselecting`. +// The option was offered, selecting it did nothing, and no error said so — reported on #234 +// as "I selected No playlist and it still showed the same video". It did. +// +// Device-scoped rather than playlist-scoped because there is no playlist to authorize +// against when clearing; ownership is checked the same way every other device mutation +// checks it. Clearing an already-clear device is a no-op success, so the button is safe to +// press twice. +router.delete('/:id/playlist', (req, res) => { + const device = checkDeviceOwnership(req, res); + if (!device) return; + + db.prepare('UPDATE devices SET playlist_id = NULL, updated_at = ? WHERE id = ?') + .run(Math.floor(Date.now() / 1000), req.params.id); + + // Push the now-empty playlist so the screen stops, rather than leaving the old content up + // until something else happens to update it. + try { + const io = req.app.get('io'); + if (io) { + const { buildPlaylistPayload } = require('../ws/deviceSocket'); + const commandQueue = require('../lib/command-queue'); + commandQueue.queueOrEmitPlaylistUpdate(io.of('/device'), req.params.id, buildPlaylistPayload); + } + } catch (e) { /* silent — the DB is the source of truth, the push is best-effort */ } + + res.json({ success: true }); +}); + router.put('/:id', (req, res) => { const device = checkDeviceOwnership(req, res); if (!device) return; diff --git a/server/test/clear-device-playlist.test.js b/server/test/clear-device-playlist.test.js new file mode 100644 index 0000000..b03b82b --- /dev/null +++ b/server/test/clear-device-playlist.test.js @@ -0,0 +1,107 @@ +'use strict'; + +// "No playlist" was an option you could select that did nothing. +// +// The dashboard picker offered ``, and its change handler +// opened with `if (!newPlaylistId) return; // Don't allow deselecting for now` — so choosing it +// sent no request, changed nothing, and raised no error. The guard was honest about why: there was +// no way to do it. PUT /devices/:id has never read playlist_id (it returns 200 and ignores it), and +// POST /playlists/:id/assign can only set one. +// +// Reported on #234 as "I also selected No playlist ... it still showed the same video". It did. +// +// The invariant: clearing a display's playlist actually clears it, and only the people allowed to +// change that display can do it. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'st-clearpl-')); +process.env.DATA_DIR = tmp; +process.env.JWT_SECRET = 'test-secret-clear-playlist'; + +const express = require('express'); +const { db } = require('../db/database'); +const { requireAuth, generateToken } = require('../middleware/auth'); + +// devices -> workspaces -> organizations -> users, FK-enforced, so seed the whole chain. +function seed(suffix) { + const u = 'u-' + suffix, o = 'o-' + suffix, ws = 'ws-' + suffix; + const dev = 'd-' + suffix, pl = 'p-' + suffix; + db.prepare("INSERT OR IGNORE INTO users (id, email, password_hash, role) VALUES (?, ?, 'x', 'user')") + .run(u, suffix + '@test.local'); + db.prepare('INSERT OR IGNORE INTO organizations (id, name, owner_user_id) VALUES (?, ?, ?)').run(o, 'org ' + suffix, u); + db.prepare('INSERT OR IGNORE INTO workspaces (id, organization_id, name) VALUES (?, ?, ?)').run(ws, o, 'ws ' + suffix); + // accessContext resolves through the MEMBERSHIP tables, not organizations.owner_user_id — + // seeding only the owner column gets a legitimate owner a 403 and looks like an authz bug. + db.prepare("INSERT OR IGNORE INTO organization_members (organization_id, user_id, role) VALUES (?, ?, 'org_owner')").run(o, u); + db.prepare("INSERT INTO playlists (id, name, workspace_id, user_id) VALUES (?, 'PL', ?, ?)").run(pl, ws, u); + db.prepare(`INSERT INTO devices (id, name, workspace_id, user_id, playlist_id, created_at, updated_at) + VALUES (?, 'Screen', ?, ?, ?, strftime('%s','now'), strftime('%s','now'))`).run(dev, ws, u, pl); + return { u, ws, dev, pl }; +} + +const mine = seed('mine'); +const theirs = seed('theirs'); + +const app = express(); +app.use(express.json()); +app.use('/api/devices', requireAuth, require('../routes/devices')); +const server = app.listen(0); + +const userRow = (id) => db.prepare('SELECT id, email, role FROM users WHERE id = ?').get(id); +const tokenFor = (u, ws) => generateToken(userRow(u), ws); + +async function del(deviceId, token) { + await new Promise(r => (server.listening ? r() : server.once('listening', r))); + const res = await fetch(`http://127.0.0.1:${server.address().port}/api/devices/${deviceId}/playlist`, { + method: 'DELETE', + headers: token ? { Authorization: `Bearer ${token}` } : {}, + }); + return res.status; +} + +const playlistOf = (id) => db.prepare('SELECT playlist_id FROM devices WHERE id = ?').get(id).playlist_id; + +test('THE BUG: PUT /devices/:id silently ignores playlist_id, so it could not clear one', async () => { + // Pinned so nobody "fixes" the picker by pointing it back at PUT and re-creating the silence. + const src = fs.readFileSync(path.join(__dirname, '..', 'routes', 'devices.js'), 'utf8'); + const put = src.slice(src.indexOf("router.put('/:id'")); + const body = put.slice(0, put.indexOf('\nrouter.')); + assert.ok(!/playlist_id\s*[,=]/.test(body), 'PUT now touches playlist_id — update this test and the picker'); +}); + +test('THE FIX: clearing a playlist actually clears it', async () => { + assert.equal(playlistOf(mine.dev), mine.pl, 'precondition: a playlist is assigned'); + assert.equal(await del(mine.dev, tokenFor(mine.u, mine.ws)), 200); + assert.equal(playlistOf(mine.dev), null, 'the display must end up with no playlist'); +}); + +test('clearing an already-clear display is a harmless no-op', async () => { + // The button is in a dropdown a person can pick twice; it must not 404 or 500 on the second go. + assert.equal(await del(mine.dev, tokenFor(mine.u, mine.ws)), 200); + assert.equal(playlistOf(mine.dev), null); +}); + +test('someone else\'s display cannot be cleared', async () => { + const before = playlistOf(theirs.dev); + const status = await del(theirs.dev, tokenFor(mine.u, mine.ws)); + assert.ok(status === 403 || status === 404, `expected refusal, got ${status}`); + assert.equal(playlistOf(theirs.dev), before, 'a refused call must not have changed anything'); +}); + +test('an unauthenticated caller cannot clear a playlist', async () => { + const before = playlistOf(theirs.dev); + assert.equal(await del(theirs.dev), 401); + assert.equal(playlistOf(theirs.dev), before); +}); + +test('a display that does not exist is refused, not invented', async () => { + const status = await del('no-such-device', tokenFor(mine.u, mine.ws)); + assert.ok(status === 403 || status === 404, `expected refusal, got ${status}`); +}); + +test.after(() => { server.close(); try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} });