From 3159f941075510184116ef30e9fcb3d0d2a5d592 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Wed, 29 Jul 2026 18:44:11 -0500 Subject: [PATCH] Make unblock stick, and say so when a device is refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A customer blocked a screen once to see what the button did, then spent an evening unable to get it back. Three separate faults stacked up. 1. Unblock did not stick. applyToDevice() restores `blocked` on re-pair — deliberately, so a block cannot be shrugged off by deleting the device — which makes the SAVED copy the real authority. Unblock only ever wrote `devices`, so the saved row stayed 1 and the next delete + re-pair silently re-blocked. There was no way out from the dashboard at all: unblock, re-pair, refused, repeat. Block and unblock now both mirror to the saved copy, so the survives-a-re-pair property is deliberate rather than a leftover. 2. The refusal was invisible. handleServerRejection() clears credentials and calls onUnpaired, but only ProvisioningActivity ever assigned that callback — and it is long gone by the time playback is running. So the screen sat on "Connecting to server" and the player eventually blamed the URL, sending the operator off checking their network while the server had already said exactly what was wrong. MainActivity now handles it. (This half was mine: clearing those leaked callbacks to stop the relaunch loop removed the only thing that surfaced a rejection. It was a broken path — it fired into a destroyed Activity — but it was the only one, and MainActivity should have owned it.) 3. The reason was thrown away. The server sends device:auth-error {error: "Device blocked"} and the client discarded it. It is kept now, and a blocked screen says so instead of implying a network fault. Localised in all six languages, matching the other on-screen status strings. Also ran on prod: one stale saved block cleared (fingerprint ef6540376599, the reporter's tablet), DB backed up first. It was the only such row. Tests pin both directions, because the two are easy to confuse: unblock must clear the saved copy, AND a genuine block must still survive a delete + re-pair. --- .../com/remotedisplay/player/MainActivity.kt | 16 ++++ .../player/service/WebSocketService.kt | 11 +++ .../app/src/main/res/values-de/strings.xml | 2 + .../app/src/main/res/values-es/strings.xml | 2 + .../app/src/main/res/values-fr/strings.xml | 2 + .../app/src/main/res/values-hi/strings.xml | 2 + .../app/src/main/res/values-pt/strings.xml | 2 + android/app/src/main/res/values/strings.xml | 2 + server/lib/device-settings.js | 21 ++++- server/routes/devices.js | 7 ++ .../test/unblock-clears-saved-block.test.js | 94 +++++++++++++++++++ 11 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 server/test/unblock-clears-saved-block.test.js diff --git a/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt b/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt index 29a7a62..a0a659e 100644 --- a/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt +++ b/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt @@ -495,6 +495,22 @@ class MainActivity : AppCompatActivity() { // #170: on a fresh network connection, clear stuck download backoff so content that failed // to download while the link was settling retries on the next sweep (the service also // requests a playlist refresh). Keeps single-flight; only touches failure/backoff state. + // #234: the server rejecting us (operator block, cleared credentials, reclaim settle) has + // to be VISIBLE. Only ProvisioningActivity ever assigned onUnpaired, and it is gone by the + // time playback is running — so a rejection left the screen sitting on "Connecting to + // server", and the player then blamed the URL. The server always says why; show that. + wsService?.onUnpaired = { + runOnUiThread { + val why = wsService?.lastRejectionReason ?: "" + val blocked = why.contains("block", ignoreCase = true) + Log.w("MainActivity", "server rejected this device ($why) — surfacing re-pair state") + showStatus( + if (blocked) getString(R.string.device_blocked_status) + else getString(R.string.device_unpaired_status) + ) + } + } + wsService?.onNetworkAvailable = { if (::downloadCoordinator.isInitialized) downloadCoordinator.resetAllBackoff() } diff --git a/android/app/src/main/java/com/remotedisplay/player/service/WebSocketService.kt b/android/app/src/main/java/com/remotedisplay/player/service/WebSocketService.kt index a350f2b..24ddb9d 100644 --- a/android/app/src/main/java/com/remotedisplay/player/service/WebSocketService.kt +++ b/android/app/src/main/java/com/remotedisplay/player/service/WebSocketService.kt @@ -729,6 +729,15 @@ class WebSocketService : Service() { /** True from the first server rejection until the device is (re)paired — UI stays on re-pair. */ fun isAwaitingRepair(): Boolean = awaitingRepair + + /** + * Why the server last refused us, verbatim from device:auth-error (e.g. "Device blocked"). + * The server always says why; the player used to throw it away and fall back to a generic + * connection failure, so an operator block read as "couldn't reach the server, check the url" + * and sent people off debugging their network. #234. + */ + @Volatile var lastRejectionReason: String? = null + private set /** Milliseconds left in the reclaim-settle hold (0 once elapsed) — drives the UI countdown. */ fun repairHoldRemainingMs(): Long = maxOf(0L, repairHoldUntilMs - SystemClock.elapsedRealtime()) /** True only when the shown pairing code is server-accepted (pairable) — not a rejected/stale one. */ @@ -748,6 +757,7 @@ class WebSocketService : Service() { * scheduled retry, so the screen is stable — no register/reject/register churn. */ private fun handleServerRejection(reason: String) { + lastRejectionReason = reason val settleSec = parseSettleSeconds(reason) Log.w("WebSocketService", "Server rejected device ($reason) — settle=${settleSec}s") pairingCodeLive = false // this registration was rejected — the local code is NOT pairable @@ -778,6 +788,7 @@ class WebSocketService : Service() { /** Re-pair complete (device:paired, or a normal authenticated reconnect) — clear all repair state. */ private fun resetRepairBackoff() { + lastRejectionReason = null repairRetryPending = false repairBackoffMs = 0L awaitingRepair = false diff --git a/android/app/src/main/res/values-de/strings.xml b/android/app/src/main/res/values-de/strings.xml index e9a1135..b7fbec1 100644 --- a/android/app/src/main/res/values-de/strings.xml +++ b/android/app/src/main/res/values-de/strings.xml @@ -4,4 +4,6 @@ RemoteDisplay nutzt die Bedienungshilfen, um Fernsteuerung der Stromzufuhr und Systemnavigation zu ermöglichen. Derzeit ist nichts geplant Warte auf Inhalte… + Dieser Bildschirm wurde im Dashboard gesperrt + Dieser Bildschirm wurde entkoppelt — warte auf erneute Kopplung diff --git a/android/app/src/main/res/values-es/strings.xml b/android/app/src/main/res/values-es/strings.xml index aaba213..37a4e6a 100644 --- a/android/app/src/main/res/values-es/strings.xml +++ b/android/app/src/main/res/values-es/strings.xml @@ -4,4 +4,6 @@ RemoteDisplay usa accesibilidad para habilitar el control remoto de encendido y la navegación del sistema. No hay nada programado en este momento Esperando contenido… + Esta pantalla ha sido bloqueada en el panel + Esta pantalla se desvinculó — esperando volver a vincularse diff --git a/android/app/src/main/res/values-fr/strings.xml b/android/app/src/main/res/values-fr/strings.xml index 7418c2a..be51544 100644 --- a/android/app/src/main/res/values-fr/strings.xml +++ b/android/app/src/main/res/values-fr/strings.xml @@ -4,4 +4,6 @@ RemoteDisplay utilise l\'accessibilité pour activer les contrôles d\'alimentation à distance et la navigation système. Rien de programmé pour le moment En attente de contenu… + Cet écran a été bloqué dans le tableau de bord + Cet écran a été dissocié — en attente d\'un nouvel appairage diff --git a/android/app/src/main/res/values-hi/strings.xml b/android/app/src/main/res/values-hi/strings.xml index 976fba0..32f7a15 100644 --- a/android/app/src/main/res/values-hi/strings.xml +++ b/android/app/src/main/res/values-hi/strings.xml @@ -4,4 +4,6 @@ RemoteDisplay RemoteDisplay uses accessibility to enable remote power controls and system navigation. + यह स्क्रीन डैशबोर्ड में अवरोधित कर दी गई है + यह स्क्रीन अनयुग्मित हो गई — फिर से युग्मित होने की प्रतीक्षा diff --git a/android/app/src/main/res/values-pt/strings.xml b/android/app/src/main/res/values-pt/strings.xml index 816276a..a220dd9 100644 --- a/android/app/src/main/res/values-pt/strings.xml +++ b/android/app/src/main/res/values-pt/strings.xml @@ -4,4 +4,6 @@ RemoteDisplay usa acessibilidade para habilitar controles remotos de energia e navegação do sistema. Nada programado no momento Aguardando conteúdo… + Este ecrã foi bloqueado no painel + Este ecrã foi desemparelhado — a aguardar novo emparelhamento diff --git a/android/app/src/main/res/values/strings.xml b/android/app/src/main/res/values/strings.xml index 892c0c6..26de3d4 100644 --- a/android/app/src/main/res/values/strings.xml +++ b/android/app/src/main/res/values/strings.xml @@ -39,4 +39,6 @@ Notifications Tap \"Open\" to manage permissions in system settings Open settings + This screen has been blocked in the dashboard + This screen was unpaired — waiting to be re-paired diff --git a/server/lib/device-settings.js b/server/lib/device-settings.js index 134fae8..fcb147d 100644 --- a/server/lib/device-settings.js +++ b/server/lib/device-settings.js @@ -121,4 +121,23 @@ function purgeWorkspaces(dbConn, workspaceIds) { return (dbConn || db).prepare(`DELETE FROM device_settings WHERE workspace_id IN (${ph})`).run(...ids).changes; } -module.exports = { snapshot, applyToDevice, listRemoved, getByFingerprint, purgeWorkspaces, validOrientation, ORIENTATIONS }; +/** + * Mirror a device's blocked flag onto its SAVED settings. + * + * applyToDevice deliberately restores `blocked` across a re-pair, so a block cannot be shrugged off + * by deleting the device. That is right — but it also means the saved copy is the real authority for + * anything that outlives the device row, and unblocking used to touch only `devices`. The saved copy + * stayed 1, so the very next delete-and-re-pair restored the block: from the operator's side, unblock + * simply did not take, and there was no way out of it from the dashboard at all. + * + * No-ops when the device has no fingerprint yet (nothing to key the saved row on). + */ +function setBlockedByDevice(deviceId, blocked) { + const fp = _fpForDevice.get(deviceId)?.fingerprint; + if (!fp) return false; + const r = db.prepare("UPDATE device_settings SET blocked = ?, last_seen = strftime('%s','now') WHERE fingerprint = ?") + .run(blocked ? 1 : 0, fp); + return r.changes > 0; +} + +module.exports = { snapshot, applyToDevice, listRemoved, getByFingerprint, purgeWorkspaces, setBlockedByDevice, validOrientation, ORIENTATIONS }; diff --git a/server/routes/devices.js b/server/routes/devices.js index 8eef1c9..ce9d221 100644 --- a/server/routes/devices.js +++ b/server/routes/devices.js @@ -282,6 +282,9 @@ router.post('/:id/block', (req, res) => { const device = checkDeviceOwnership(req, res); if (!device) return; db.prepare("UPDATE devices SET blocked = 1, updated_at = strftime('%s','now') WHERE id = ?").run(req.params.id); + // Mirror onto the saved settings so the block survives a delete + re-pair on purpose rather than + // by accident of whatever the saved copy happened to hold. + try { deviceSettings.setBlockedByDevice(req.params.id, true); } catch (e) { console.warn(`[blocked] save mirror failed: ${e.message}`); } console.warn(`[blocked] device ${req.params.id} blocked via dashboard (user ${req.user.id})`); res.json({ success: true, id: req.params.id, blocked: true }); }); @@ -289,6 +292,10 @@ router.post('/:id/unblock', (req, res) => { const device = checkDeviceOwnership(req, res); if (!device) return; db.prepare("UPDATE devices SET blocked = 0, updated_at = strftime('%s','now') WHERE id = ?").run(req.params.id); + // MUST clear the saved copy too. applyToDevice() restores `blocked` on re-pair, so leaving the + // saved 1 in place made unblock temporary: the next delete + re-pair silently re-blocked the + // device, with nothing in the dashboard to explain it and no way for the operator to escape. + try { deviceSettings.setBlockedByDevice(req.params.id, false); } catch (e) { console.warn(`[blocked] save mirror failed: ${e.message}`); } console.log(`[blocked] device ${req.params.id} unblocked via dashboard (user ${req.user.id})`); res.json({ success: true, id: req.params.id, blocked: false }); }); diff --git a/server/test/unblock-clears-saved-block.test.js b/server/test/unblock-clears-saved-block.test.js new file mode 100644 index 0000000..99ccd05 --- /dev/null +++ b/server/test/unblock-clears-saved-block.test.js @@ -0,0 +1,94 @@ +'use strict'; + +// A blocked device stays blocked across a delete + re-pair on purpose (device-settings applyToDevice +// restores `blocked`, so a block cannot be shrugged off by deleting the device). That is the right +// call — but it makes the SAVED copy the real authority, and unblocking only ever wrote `devices`. +// +// So unblock did not stick. The device row said 0, the saved row still said 1, and the next re-pair +// restored the block. From the dashboard there was no way out: a customer unblocked, re-paired, was +// refused again, and had nothing to tell them why. Found on #234 with a real device that had been +// blocked once to see what the button did. +// +// The invariant: after unblock, NOTHING anywhere still claims the device is blocked. + +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-unblock-')); +process.env.DATA_DIR = tmp; + +const { db } = require('../db/database'); +const deviceSettings = require('../lib/device-settings'); + +// devices -> workspaces -> organizations -> users, all FK-enforced, so seed the whole chain. +function seedWorkspace(suffix) { + const u = 'u-' + suffix, o = 'o-' + suffix, ws = 'ws-' + suffix; + db.prepare("INSERT OR IGNORE INTO users (id, email, password_hash) VALUES (?, ?, 'x')").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); + return ws; +} + +function seedBlockedDevice(id, fp) { + const ws = seedWorkspace(id); + db.prepare("INSERT INTO devices (id, name, workspace_id, blocked, created_at, updated_at) VALUES (?, ?, ?, 1, strftime('%s','now'), strftime('%s','now'))") + .run(id, 'blocked-device', ws); + db.prepare("INSERT INTO device_fingerprints (fingerprint, device_id, last_seen) VALUES (?, ?, strftime('%s','now'))") + .run(fp, id); + // The saved snapshot the re-pair path reads back. + db.prepare("INSERT INTO device_settings (fingerprint, workspace_id, device_name, blocked, last_seen) VALUES (?, ?, ?, 1, strftime('%s','now'))") + .run(fp, ws, 'blocked-device'); + return { id, fp, ws }; +} + +const savedBlocked = (fp) => db.prepare('SELECT blocked FROM device_settings WHERE fingerprint = ?').get(fp)?.blocked; +const liveBlocked = (id) => db.prepare('SELECT blocked FROM devices WHERE id = ?').get(id)?.blocked; + +test('THE BUG: clearing devices.blocked alone leaves the saved copy blocked', () => { + const { id, fp } = seedBlockedDevice('dev-stale', 'fp-stale'); + // What unblock used to do, and only this. + db.prepare('UPDATE devices SET blocked = 0 WHERE id = ?').run(id); + assert.equal(liveBlocked(id), 0); + assert.equal(savedBlocked(fp), 1, 'the saved copy is what re-pair restores from'); +}); + +test('a re-pair after that half-unblock puts the block straight back', () => { + const { id, fp } = seedBlockedDevice('dev-repair', 'fp-repair'); + db.prepare('UPDATE devices SET blocked = 0 WHERE id = ?').run(id); + // This is exactly what the register path runs on a re-paired device. + deviceSettings.applyToDevice(id, fp); + assert.equal(liveBlocked(id), 1, 'unblock silently reverted — the customer cannot escape this'); +}); + +test('THE FIX: unblocking clears the saved copy, so a re-pair stays unblocked', () => { + const { id, fp } = seedBlockedDevice('dev-fixed', 'fp-fixed'); + db.prepare('UPDATE devices SET blocked = 0 WHERE id = ?').run(id); + assert.equal(deviceSettings.setBlockedByDevice(id, false), true); + assert.equal(savedBlocked(fp), 0); + deviceSettings.applyToDevice(id, fp); + assert.equal(liveBlocked(id), 0, 're-pair must not resurrect the block'); +}); + +test('blocking still survives a re-pair — that property is deliberate and must not regress', () => { + const { id, fp } = seedBlockedDevice('dev-stays', 'fp-stays'); + db.prepare('UPDATE devices SET blocked = 0 WHERE id = ?').run(id); + deviceSettings.setBlockedByDevice(id, false); + // Now block it for real, the way the route does. + db.prepare('UPDATE devices SET blocked = 1 WHERE id = ?').run(id); + deviceSettings.setBlockedByDevice(id, true); + // Simulate delete + re-pair: the device row is recreated unblocked, then settings are restored. + db.prepare('UPDATE devices SET blocked = 0 WHERE id = ?').run(id); + deviceSettings.applyToDevice(id, fp); + assert.equal(liveBlocked(id), 1, 'a genuine block must not be escapable by deleting the device'); +}); + +test('a device with no fingerprint yet is a no-op, not a throw', () => { + const ws = seedWorkspace('nofp'); + db.prepare("INSERT INTO devices (id, name, workspace_id, blocked, created_at, updated_at) VALUES ('dev-nofp','x',?,1,strftime('%s','now'),strftime('%s','now'))").run(ws); + assert.equal(deviceSettings.setBlockedByDevice('dev-nofp', false), false); +}); + +test.after(() => { try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} });