mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 13:53:12 -06:00
Make unblock stick, and say so when a device is refused
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.
This commit is contained in:
parent
39c4ec8af8
commit
3159f94107
|
|
@ -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()
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -4,4 +4,6 @@
|
|||
<string name="accessibility_description">RemoteDisplay nutzt die Bedienungshilfen, um Fernsteuerung der Stromzufuhr und Systemnavigation zu ermöglichen.</string>
|
||||
<string name="nothing_scheduled">Derzeit ist nichts geplant</string>
|
||||
<string name="waiting_for_content">Warte auf Inhalte…</string>
|
||||
<string name="device_blocked_status">Dieser Bildschirm wurde im Dashboard gesperrt</string>
|
||||
<string name="device_unpaired_status">Dieser Bildschirm wurde entkoppelt — warte auf erneute Kopplung</string>
|
||||
</resources>
|
||||
|
|
|
|||
|
|
@ -4,4 +4,6 @@
|
|||
<string name="accessibility_description">RemoteDisplay usa accesibilidad para habilitar el control remoto de encendido y la navegación del sistema.</string>
|
||||
<string name="nothing_scheduled">No hay nada programado en este momento</string>
|
||||
<string name="waiting_for_content">Esperando contenido…</string>
|
||||
<string name="device_blocked_status">Esta pantalla ha sido bloqueada en el panel</string>
|
||||
<string name="device_unpaired_status">Esta pantalla se desvinculó — esperando volver a vincularse</string>
|
||||
</resources>
|
||||
|
|
|
|||
|
|
@ -4,4 +4,6 @@
|
|||
<string name="accessibility_description">RemoteDisplay utilise l\'accessibilité pour activer les contrôles d\'alimentation à distance et la navigation système.</string>
|
||||
<string name="nothing_scheduled">Rien de programmé pour le moment</string>
|
||||
<string name="waiting_for_content">En attente de contenu…</string>
|
||||
<string name="device_blocked_status">Cet écran a été bloqué dans le tableau de bord</string>
|
||||
<string name="device_unpaired_status">Cet écran a été dissocié — en attente d\'un nouvel appairage</string>
|
||||
</resources>
|
||||
|
|
|
|||
|
|
@ -4,4 +4,6 @@
|
|||
<resources>
|
||||
<string name="app_name">RemoteDisplay</string>
|
||||
<string name="accessibility_description">RemoteDisplay uses accessibility to enable remote power controls and system navigation.</string>
|
||||
<string name="device_blocked_status">यह स्क्रीन डैशबोर्ड में अवरोधित कर दी गई है</string>
|
||||
<string name="device_unpaired_status">यह स्क्रीन अनयुग्मित हो गई — फिर से युग्मित होने की प्रतीक्षा</string>
|
||||
</resources>
|
||||
|
|
|
|||
|
|
@ -4,4 +4,6 @@
|
|||
<string name="accessibility_description">RemoteDisplay usa acessibilidade para habilitar controles remotos de energia e navegação do sistema.</string>
|
||||
<string name="nothing_scheduled">Nada programado no momento</string>
|
||||
<string name="waiting_for_content">Aguardando conteúdo…</string>
|
||||
<string name="device_blocked_status">Este ecrã foi bloqueado no painel</string>
|
||||
<string name="device_unpaired_status">Este ecrã foi desemparelhado — a aguardar novo emparelhamento</string>
|
||||
</resources>
|
||||
|
|
|
|||
|
|
@ -39,4 +39,6 @@
|
|||
<string name="settings_perm_notifications">Notifications</string>
|
||||
<string name="settings_perm_hint">Tap \"Open\" to manage permissions in system settings</string>
|
||||
<string name="settings_perm_open">Open settings</string>
|
||||
<string name="device_blocked_status">This screen has been blocked in the dashboard</string>
|
||||
<string name="device_unpaired_status">This screen was unpaired — waiting to be re-paired</string>
|
||||
</resources>
|
||||
|
|
|
|||
|
|
@ -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 };
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
});
|
||||
|
|
|
|||
94
server/test/unblock-clears-saved-block.test.js
Normal file
94
server/test/unblock-clears-saved-block.test.js
Normal file
|
|
@ -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 (_) {} });
|
||||
Loading…
Reference in a new issue