diff --git a/CHANGELOG.md b/CHANGELOG.md index 30b8fa77..a90c9f6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -253,6 +253,12 @@ Core: from every config load and could not `import web_interface.app` at all. `ensure_shared_group_ownership()` now returns immediately when `os.geteuid` or `os.chown` is missing. No behaviour change on the Pi. +- Restoring a backup on Windows no longer fails over files that already exist. + The restore carries each replaced file's owner across with `os.chown`, which + does not exist on Windows; the `AttributeError` escaped the per-file error + handling, so the restore stopped at `config.json` with nothing restored. The + ownership step is now skipped where `os.chown` is missing. No behaviour + change on the Pi. Automatic updates and Update Code: diff --git a/scripts/install/install_service.sh b/scripts/install/install_service.sh index 8b129503..f0d9de0a 100755 --- a/scripts/install/install_service.sh +++ b/scripts/install/install_service.sh @@ -105,7 +105,7 @@ echo "Installing LEDMatrix Web Interface service (ledmatrix-web.service)..." # Rendered from systemd/ledmatrix-web.service, the same template # install_web_service.sh uses. This was an inline heredoc until it drifted from # the template: it had lost Wants=network-online.target, RestartSec, -# SyslogIdentifier, CacheDirectory and Environment=USE_THREADING. Because +# SyslogIdentifier and Environment=USE_THREADING. Because # src/startup_validator.py compares the installed unit against the template, # every boot warned "re-run install_service.sh" -- and doing so reinstalled the # same stale copy, so the warning could never clear. diff --git a/scripts/install/install_web_service.sh b/scripts/install/install_web_service.sh index 40d5378c..0c4c9908 100755 --- a/scripts/install/install_web_service.sh +++ b/scripts/install/install_web_service.sh @@ -32,7 +32,7 @@ fi # Render the unit from systemd/ledmatrix-web.service. That template is the # only description of the unit; this script used to carry its own heredoc copy, # and install_service.sh a third, which is how the installed unit on real rigs -# ended up missing RestartSec, SyslogIdentifier and CacheDirectory while +# ended up missing RestartSec and SyslogIdentifier while # src/startup_validator.py warned about drift on every boot. TEMPLATE="$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" if [ ! -f "$TEMPLATE" ]; then @@ -62,31 +62,47 @@ for VERIFY_UNIT in ledmatrix-update-verify.service ledmatrix-update-verify.path; fi done -# Ensure cache directory exists with proper permissions -# This is a fallback for older systemd versions that don't support CacheDirectory -# Systemd 239+ will automatically create it via CacheDirectory directive +# Shared cache directory. The display service (root) and this web service both +# write here and read each other's files, which are created 0660, so the two +# share it through the directory's group: ledmatrix when the installing user +# is in it (first_time_install.sh / setup_cache.sh set that up), otherwise the +# user's own group. setgid makes new files inherit that group. +# +# An existing directory keeps its group whenever the web user can read through +# it -- ledmatrix, or the user's own group where systemd's old CacheDirectory= +# left it -- because re-grouping a working directory strands every file already +# in it on the old group. Only a group the user is not in (root's, or ledmatrix +# for a user outside it) is replaced. This used to force the user's group on +# every run, replacing the ledmatrix group setup_cache.sh had set. echo "Setting up cache directory..." CACHE_DIR="/var/cache/ledmatrix" +USER_GROUPS=$(id -nG "$ACTUAL_USER" 2>/dev/null | tr ' ' '\n') +if printf '%s\n' "$USER_GROUPS" | grep -qx ledmatrix; then + CACHE_GROUP="ledmatrix" +else + CACHE_GROUP=$(id -gn "$ACTUAL_USER" 2>/dev/null || echo root) +fi if [ ! -d "$CACHE_DIR" ]; then mkdir -p "$CACHE_DIR" - # Set group ownership to allow both root and web user access - # Try to use ACTUAL_USER's group, fallback to root if that fails - if getent group "$ACTUAL_USER" > /dev/null 2>&1; then - chown root:"$ACTUAL_USER" "$CACHE_DIR" 2>/dev/null || chown root:root "$CACHE_DIR" - else - chown root:root "$CACHE_DIR" - fi - chmod 775 "$CACHE_DIR" + chown root:"$CACHE_GROUP" "$CACHE_DIR" 2>/dev/null || true echo "✓ Cache directory created: $CACHE_DIR" else - # Ensure permissions are correct - chmod 775 "$CACHE_DIR" 2>/dev/null || true - # Try to set group ownership if possible - if getent group "$ACTUAL_USER" > /dev/null 2>&1; then - chown root:"$ACTUAL_USER" "$CACHE_DIR" 2>/dev/null || true + DIR_GROUP=$(stat -c %G "$CACHE_DIR" 2>/dev/null) + if ! printf '%s\n' "$USER_GROUPS" | grep -qx "$DIR_GROUP"; then + if chgrp "$CACHE_GROUP" "$CACHE_DIR" 2>/dev/null; then + echo "✓ Cache directory group changed from $DIR_GROUP to $CACHE_GROUP" + # Files already there keep the old group. The display service + # re-groups its own files when it starts (DiskCache.share_existing_files, + # which refuses symlinks and hard links); a recursive chgrp here + # would not. try-restart does nothing if the service is not running. + if find "$CACHE_DIR" -maxdepth 1 -name '*.json' -user root ! -group "$CACHE_GROUP" -print -quit 2>/dev/null | grep -q .; then + systemctl try-restart ledmatrix.service 2>/dev/null || true + fi + fi fi echo "✓ Cache directory exists: $CACHE_DIR" fi +chmod 2775 "$CACHE_DIR" 2>/dev/null || true # Reload systemd to recognize the new service echo "Reloading systemd..." diff --git a/src/backup_manager.py b/src/backup_manager.py index e8d03b18..13114114 100644 --- a/src/backup_manager.py +++ b/src/backup_manager.py @@ -530,12 +530,15 @@ def _copy_file(src: Path, dst: Path) -> None: os.chmod(tmp_path, existing_mode) else: shutil.copymode(src, tmp_path) - if existing_owner is not None: + if existing_owner is not None and hasattr(os, 'chown'): # Replacing a file creates a new inode owned by whoever is running, # which would silently move a root-owned config to the web user. # Carry the previous owner across when the OS permits it — only # root can hand a file to another user, so this is best-effort and # a plain restore as the web user simply keeps its own ownership. + # os.chown does not exist on Windows (where st_uid/st_gid are just + # 0); looking it up there raises AttributeError, which no caller + # catches, so every restore over an existing file aborted. try: os.chown(tmp_path, existing_owner[0], existing_owner[1]) except (OSError, PermissionError): diff --git a/src/cache/disk_cache.py b/src/cache/disk_cache.py index c64c8632..87c3d7a9 100644 --- a/src/cache/disk_cache.py +++ b/src/cache/disk_cache.py @@ -7,6 +7,7 @@ Handles persistent disk-based caching with atomic writes and error recovery. import json import math import os +import stat import time import tempfile import logging @@ -139,6 +140,65 @@ else: return json.loads(raw) +# SHARING CACHE FILES BETWEEN THE TWO SERVICES +# -------------------------------------------- +# The display service runs as root and the web interface as the installing +# user, and the web interface reads records only the display writes +# (display_current_state, display_on_demand_state, plugin_metrics:*). Files are +# written 0660, so the web interface can read one only through its group. +# +# The installers rely on the directory's setgid bit to set that group. That is +# not something the cache can count on: systemd's CacheDirectory=, which +# ledmatrix-web.service carried until Sept 2026, re-owns the directory and +# everything in it to the web user and its primary group whenever the +# directory's owner does not match, and the setgid layout never survives that. +# From then on every file root creates is root:root 0660, unreadable by the web +# interface. Measured on one rig: 365 such files, and the web UI's display +# status, on-demand state and plugin health all silently empty. +# +# So a cache file takes its group from the directory explicitly, whether or +# not setgid is set. Only a group-writable directory counts as shared: that +# group can already replace any file in it, so reading them grants nothing new. +# +# Everything here works on an open descriptor, never a path. The directory is +# writable by the web user, so between a path check and a path operation that +# user could put a symlink in the file's place, and root would then chown and +# chmod whatever it points at. + +_CACHE_FILE_MODE = 0o660 + + +def _shared_group(directory: str) -> Optional[int]: + """The group a cache file in ``directory`` should carry, if it is shared.""" + try: + st = os.stat(directory) + except OSError: + return None + if not st.st_mode & stat.S_IWGRP: + return None + return st.st_gid + + +def _share_open_file(fd: int, group: Optional[int]) -> None: + """Make an open cache file readable by the other service. Best effort.""" + fchmod = getattr(os, 'fchmod', None) # absent on Windows before 3.13 + if fchmod is not None: + try: + fchmod(fd, _CACHE_FILE_MODE) + except OSError: + pass + fchown = getattr(os, 'fchown', None) # absent on Windows + if fchown is None or group is None: + return + try: + if os.fstat(fd).st_gid != group: + fchown(fd, -1, group) + except OSError: + # Not a member of the directory's group and not root: nothing to do, + # and the file keeps the group it was created with. + pass + + class DiskCache: """Manages persistent disk-based cache.""" @@ -350,13 +410,12 @@ class DiskCache: try: with os.fdopen(fd, 'wb') as tmp_file: tmp_file.write(payload) + # Before the rename, not after: mkstemp + # creates the file 0600, and a reader that + # opened it in between was refused. + _share_open_file(tmp_file.fileno(), _shared_group(tmp_dir)) os.replace(tmp_path, cache_path) self._write_digests[key] = digest - # Set proper permissions: 660 (rw-rw----) for group-readable cache files - try: - os.chmod(cache_path, 0o660) # nosec B103 - intentional; web UI and service share a group - except OSError: - pass # Non-critical if chmod fails finally: if os.path.exists(tmp_path): try: @@ -368,12 +427,8 @@ class DiskCache: try: with open(cache_path, 'wb') as cache_file: cache_file.write(payload) + _share_open_file(cache_file.fileno(), _shared_group(tmp_dir)) self._write_digests[key] = digest - # Set proper permissions: 660 (rw-rw----) for group-readable cache files - try: - os.chmod(cache_path, 0o660) # nosec B103 - intentional; web UI and service share a group - except OSError: - pass # Non-critical if chmod fails self.logger.debug("Wrote cache for %s directly (non-atomic)", key) except (IOError, OSError, PermissionError) as write_error: # If direct write also fails, try fallback location @@ -398,11 +453,7 @@ class DiskCache: fallback_path = os.path.join(fallback_dir, os.path.basename(cache_path)) with open(fallback_path, 'wb') as tmp_file: tmp_file.write(payload) - # Set proper permissions: 660 (rw-rw----) for group-readable cache files - try: - os.chmod(fallback_path, 0o660) # nosec B103 - intentional; web UI and service share a group - except OSError: - pass # Non-critical if chmod fails + _share_open_file(tmp_file.fileno(), _shared_group(fallback_dir)) self.logger.debug("Cache wrote to fallback location: %s", fallback_path) return # Successfully wrote to fallback, exit gracefully except (IOError, OSError, PermissionError) as e2: @@ -459,6 +510,72 @@ class DiskCache: def get_cache_dir(self) -> Optional[str]: """Get the cache directory path.""" return self.cache_dir + + def share_existing_files(self) -> int: + """Give cache files already on disk the group set() now gives new ones. + + set() fixes every file it writes from now on; this repairs the ones an + older version left behind as root:root, which the web interface cannot + read until each key happens to be rewritten -- and some, like a + plugin's metrics, may not be for a long time. Meant to run once per + process, off the startup path. + + Only this process's own regular files are touched, and each one through + a descriptor opened with O_NOFOLLOW and checked for a single link: the + directory is writable by the web user, and a root process must not be + steered into changing a file outside it. + + Returns: + Number of files whose group or mode was changed. + """ + fchown = getattr(os, 'fchown', None) + geteuid = getattr(os, 'geteuid', None) + nofollow = getattr(os, 'O_NOFOLLOW', None) + if not self.cache_dir or fchown is None or geteuid is None or nofollow is None: + return 0 + group = _shared_group(self.cache_dir) + if group is None: + return 0 + euid = geteuid() + + changed = 0 + try: + entries = list(os.scandir(self.cache_dir)) + except OSError as e: + self.logger.debug("Could not scan %s to share cache files: %s", self.cache_dir, e) + return 0 + for entry in entries: + if not entry.name.endswith('.json'): + continue + try: + st = entry.stat(follow_symlinks=False) + except OSError: + continue + if (not stat.S_ISREG(st.st_mode) or st.st_uid != euid + or (st.st_gid == group and stat.S_IMODE(st.st_mode) == _CACHE_FILE_MODE)): + continue + try: + fd = os.open(entry.path, os.O_RDONLY | nofollow | getattr(os, 'O_NONBLOCK', 0)) + except OSError: + continue + try: + st = os.fstat(fd) + if not stat.S_ISREG(st.st_mode) or st.st_uid != euid or st.st_nlink != 1: + continue + _share_open_file(fd, group) + st = os.fstat(fd) + if st.st_gid == group and stat.S_IMODE(st.st_mode) == _CACHE_FILE_MODE: + changed += 1 + except OSError: + continue + finally: + os.close(fd) + if changed: + self.logger.info( + "Made %d cache file(s) in %s readable by the directory's group " + "(gid %d) so the web interface can read them", + changed, self.cache_dir, group) + return changed @staticmethod def _is_orphaned_temp(filename: str) -> bool: diff --git a/src/cache_manager.py b/src/cache_manager.py index 67624de1..dc7cebdc 100644 --- a/src/cache_manager.py +++ b/src/cache_manager.py @@ -762,6 +762,14 @@ class CacheManager: self.logger.info("Disk cache cleanup thread started (interval: %d hours)", self._disk_cleanup_interval_hours) + # Repair files an older version wrote unreadable by the web + # interface (see disk_cache.py, "SHARING CACHE FILES"). Once per + # directory per process, which is what this thread already is. + try: + self._disk_cache_component.share_existing_files() + except Exception as e: + self.logger.error("Error sharing existing cache files: %s", e, exc_info=True) + # Run initial cleanup on startup (deferred from __init__ to avoid blocking) try: self.logger.debug("Running initial disk cache cleanup") diff --git a/systemd/ledmatrix-web.service b/systemd/ledmatrix-web.service index 3b74b569..98178e71 100644 --- a/systemd/ledmatrix-web.service +++ b/systemd/ledmatrix-web.service @@ -25,9 +25,13 @@ RestartSec=10 StandardOutput=syslog StandardError=syslog SyslogIdentifier=ledmatrix-web -# Automatically create and manage cache directory -CacheDirectory=ledmatrix -CacheDirectoryMode=0775 +# No CacheDirectory=. /var/cache/ledmatrix is shared with ledmatrix.service, +# which runs as root, and the installers set it up as root:ledmatrix 2775 so +# both services can read what the other writes. CacheDirectory= makes systemd +# re-own that directory and everything in it to this unit's User= and that +# user's primary group whenever the owner differs -- which, for a directory +# root created, is on the first start. That erased the shared group, and from +# then on the web interface could not read a single file the display wrote. [Install] WantedBy=multi-user.target diff --git a/test/test_api_v3_lazy_plugin_discovery.py b/test/test_api_v3_lazy_plugin_discovery.py new file mode 100644 index 00000000..19c1e8fc --- /dev/null +++ b/test/test_api_v3_lazy_plugin_discovery.py @@ -0,0 +1,165 @@ +"""Tests that api_v3 routes find installed plugins before anything else has. + +The web process discovers plugins lazily (web_interface/app.py): nothing scans +the plugins directory at startup, and plugin_manifests stays empty until some +endpoint calls discover_plugins(). Routes that consulted plugin_manifests +without discovering therefore misbehaved for as long as nothing else had run. +Measured on a rig after restarting ledmatrix-web: + + POST /api/v3/display/on-demand/start {"plugin_id": "ledmatrix-stocks"} + -> 404 "Plugin ledmatrix-stocks not found", for over three minutes, + until GET /api/v3/plugins/installed happened to discover plugins. + +The browser UI loads the plugin list first, so people rarely saw it; API-only +callers (the Home Assistant MQTT bridge, scripts) saw it after every restart. +/plugins/toggle answered the same 404, and /config/main was worse: an +undiscovered plugin section skipped secret separation and wrote its API key +into config.json in plain text. + +A real PluginManager over a temporary plugins directory, so "empty until +discovered" is the real behaviour rather than a mock's. +""" + +import json +import sys +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from src.config_manager import ConfigManager # noqa: E402 +from src.plugin_system.plugin_manager import PluginManager # noqa: E402 +from src.plugin_system.schema_manager import SchemaManager # noqa: E402 +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + +PLUGIN_ID = 'ledmatrix-stocks' +MODE = 'stocks' + + +def _install(plugins_dir, plugin_id, modes, schema=None): + plugin_dir = plugins_dir / plugin_id + plugin_dir.mkdir(parents=True) + (plugin_dir / 'manifest.json').write_text(json.dumps({ + 'id': plugin_id, 'name': plugin_id, 'version': '1.0.0', + 'entry_point': 'manager.py', 'class_name': 'Plugin', + 'display_modes': modes, + })) + (plugin_dir / 'manager.py').write_text('') + if schema is not None: + (plugin_dir / 'config_schema.json').write_text(json.dumps(schema)) + + +@pytest.fixture +def plugins_dir(tmp_path): + path = tmp_path / 'plugin-repos' + path.mkdir() + _install(path, PLUGIN_ID, [MODE], schema={ + 'type': 'object', + 'properties': { + 'enabled': {'type': 'boolean'}, + 'symbols': {'type': 'string'}, + 'api_key': {'type': 'string', 'x-secret': True}, + }, + }) + return path + + +@pytest.fixture +def fresh_web_process(api_v3_module, plugins_dir): + """The web process right after a restart: nothing discovered yet.""" + manager = PluginManager(plugins_dir=str(plugins_dir)) + assert not manager.plugin_manifests + api_v3_module.api_v3.plugin_manager = manager + return manager + + +@pytest.fixture +def display_service(): + """Keep on-demand start away from systemctl and the real cache.""" + with patch('web_interface.blueprints.api_v3.display._ensure_cache_manager') as cache, \ + patch('web_interface.blueprints.api_v3.display._get_display_service_status') as status: + cache.return_value = MagicMock() + status.return_value = {'active': True} + yield cache.return_value + + +def _start(client, **body): + body.setdefault('start_service', False) + return client.post('/api/v3/display/on-demand/start', json=body) + + +class TestOnDemandStart: + def test_by_plugin_id(self, api_v3_client, fresh_web_process, display_service): + response = _start(api_v3_client, plugin_id=PLUGIN_ID) + + assert response.status_code == 200, response.get_json() + request = display_service.set.call_args.args[1] + assert (request['plugin_id'], request['mode']) == (PLUGIN_ID, MODE) + + def test_by_mode_alone(self, api_v3_client, fresh_web_process, display_service): + response = _start(api_v3_client, mode=MODE) + + assert response.status_code == 200, response.get_json() + assert display_service.set.call_args.args[1]['plugin_id'] == PLUGIN_ID + + def test_a_plugin_installed_since_the_last_scan( + self, api_v3_client, fresh_web_process, plugins_dir, display_service): + fresh_web_process.discover_plugins() + _install(plugins_dir, 'ledmatrix-weather', ['weather']) + + assert _start(api_v3_client, plugin_id='ledmatrix-weather').status_code == 200 + assert _start(api_v3_client, mode=MODE).status_code == 200 + + def test_a_mode_installed_since_the_last_scan( + self, api_v3_client, fresh_web_process, plugins_dir, display_service): + fresh_web_process.discover_plugins() + _install(plugins_dir, 'ledmatrix-weather', ['weather']) + + response = _start(api_v3_client, mode='weather') + + assert response.status_code == 200, response.get_json() + + def test_an_unknown_plugin_is_still_not_found( + self, api_v3_client, fresh_web_process, display_service): + assert _start(api_v3_client, plugin_id='no-such-plugin').status_code == 404 + assert _start(api_v3_client, mode='no-such-mode').status_code == 404 + display_service.set.assert_not_called() + + +def test_toggle_finds_the_plugin(api_v3_client, api_v3_module, fresh_web_process): + config_manager = api_v3_module.api_v3.config_manager + config_manager.load_config.return_value = {PLUGIN_ID: {'enabled': False}} + config_manager.save_config_atomic.return_value = MagicMock( + status=MagicMock(value='success')) + + response = api_v3_client.post('/api/v3/plugins/toggle', + json={'plugin_id': PLUGIN_ID, 'enabled': True}) + + assert response.status_code == 200, response.get_json() + saved = config_manager.save_config_atomic.call_args.args[0] + assert saved[PLUGIN_ID]['enabled'] is True + + +def test_main_config_save_keeps_a_plugin_secret_out_of_config_json( + api_v3_client, api_v3_module, fresh_web_process, plugins_dir, tmp_path): + # /config/main validates a plugin section like POST /plugins/config does, + # so it needs a real schema manager rather than the helper's mock. + api_v3_module.api_v3.schema_manager = SchemaManager(plugins_dir=plugins_dir) + config_file = tmp_path / 'config.json' + config_file.write_text('{}') + secrets_file = tmp_path / 'config_secrets.json' + config_manager = ConfigManager(config_path=str(config_file), + secrets_path=str(secrets_file)) + config_manager.template_path = str(tmp_path / 'no-template.json') + api_v3_module.api_v3.config_manager = config_manager + + response = api_v3_client.post('/api/v3/config/main', json={ + PLUGIN_ID: {'symbols': 'AAPL', 'api_key': 's3cret-key'}, + }) + + assert response.status_code == 200, response.get_json() + assert 's3cret' not in config_file.read_text() + assert json.loads(secrets_file.read_text())[PLUGIN_ID]['api_key'] == 's3cret-key' + assert json.loads(config_file.read_text())[PLUGIN_ID]['symbols'] == 'AAPL' diff --git a/test/test_backup_manager.py b/test/test_backup_manager.py index e6533d69..fb603d43 100644 --- a/test/test_backup_manager.py +++ b/test/test_backup_manager.py @@ -3,9 +3,12 @@ from __future__ import annotations import json +import os import stat +import sys import zipfile from pathlib import Path +from unittest.mock import MagicMock import pytest @@ -300,6 +303,11 @@ def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> N assert any("unsafe" in e.lower() for e in result.errors) +@pytest.mark.skipif( + sys.platform == "win32", + reason="simulates root-owned POSIX files with chmod 0o444, which on Windows sets the " + "read-only attribute, and Windows refuses to rename over a read-only file", +) def test_restore_over_a_file_the_user_cannot_write( project: Path, empty_project: Path, tmp_path: Path ) -> None: @@ -334,3 +342,53 @@ def test_restore_over_a_file_the_user_cannot_write( # The destination's mode is preserved rather than widened to the umask. assert stat.S_IMODE((empty_project / "config" / "config_secrets.json").stat().st_mode) == 0o444 + + +def _existing_config(empty_project: Path) -> None: + (empty_project / "config").mkdir(parents=True, exist_ok=True) + for name in ("config.json", "config_secrets.json", "wifi_config.json", "ytm_auth.json"): + (empty_project / "config" / name).write_text("{}", encoding="utf-8") + + +def test_restore_over_existing_files_without_os_chown( + project: Path, empty_project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Restore must work where the OS has no file ownership API (Windows). + + Replacing a file tries to carry its previous owner across with os.chown. + That name does not exist on Windows, and the AttributeError is not an + OSError, so it escaped every per-section handler: restoring over any + existing config aborted the whole restore and left the old files in place. + """ + zip_path = create_backup(project, output_dir=tmp_path / "exports") + _existing_config(empty_project) + monkeypatch.delattr(os, "chown", raising=False) + + result = restore_backup(zip_path, empty_project, RestoreOptions()) + + assert result.success, result.errors + for section in ("config", "secrets", "wifi", "ytm_auth"): + assert section in result.restored, f"{section} not restored: {result.errors}" + restored = json.loads((empty_project / "config" / "config.json").read_text()) + assert restored["my-plugin"]["favorites"] == ["A", "B"] + + +def test_restore_still_carries_the_previous_owner_across( + project: Path, empty_project: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Where os.chown exists, the replaced file keeps the old file's owner.""" + zip_path = create_backup(project, output_dir=tmp_path / "exports") + _existing_config(empty_project) + target = empty_project / "config" / "config.json" + old = target.stat() + chown = MagicMock() + monkeypatch.setattr(os, "chown", chown, raising=False) + + result = restore_backup(zip_path, empty_project, RestoreOptions( + restore_secrets=False, restore_wifi=False, + restore_fonts=False, restore_plugin_uploads=False, reinstall_plugins=False, + )) + + assert result.success, result.errors + owners = {(c.args[1], c.args[2]) for c in chown.call_args_list} + assert owners == {(old.st_uid, old.st_gid)} diff --git a/test/test_cache_shared_group.py b/test/test_cache_shared_group.py new file mode 100644 index 00000000..4d0d4a8c --- /dev/null +++ b/test/test_cache_shared_group.py @@ -0,0 +1,267 @@ +"""Tests that the web interface can read what the display service caches. + +The display service runs as root and the web interface as the installing user; +cache files are 0660, so the web interface reads them only through their group. +The installers made that the directory's group via setgid, but the web unit's +CacheDirectory= let systemd re-own /var/cache/ledmatrix to the web user and its +primary group on first start, erasing it. Every file root wrote afterwards was +root:root 0660: + + WARNING - Permission denied loading cache for display_current_state from + /var/cache/ledmatrix/display_current_state.json: [Errno 13] Permission denied + +Measured on one rig: 365 unreadable files, and the web UI's display status, +on-demand state and plugin health all silently empty. + +Most of these need POSIX ownership calls, so they skip on Windows. The ones +that only make sense as root (another user reading, another user's files) skip +without it; run them with `sudo python3 -m pytest test/test_cache_shared_group.py`. +""" + +import os +import shutil +import stat +import tempfile +import time +from pathlib import Path + +import pytest + +from src.cache import disk_cache as disk_cache_module +from src.cache.disk_cache import DiskCache + +posix_only = pytest.mark.skipif( + not hasattr(os, 'fchown') or not hasattr(os, 'geteuid'), + reason="needs POSIX file ownership") + +IS_ROOT = hasattr(os, 'geteuid') and os.geteuid() == 0 +root_only = pytest.mark.skipif(not IS_ROOT, reason="needs root") + +# Ids nothing on a test host is likely to use, for root-only scenarios. +OTHER_GID = 48213 +OTHER_UID = 48214 + + +def _another_group(): + """A group this process may chown its files to, other than its own.""" + if IS_ROOT: + return OTHER_GID + for gid in os.getgroups(): + if gid != os.getegid(): + return gid + pytest.skip("process belongs to no supplementary group") + + +def _shared_dir(path, gid, mode=0o775): + """The rig's broken layout: group-writable, no setgid, a foreign group.""" + os.chown(path, -1, gid) + os.chmod(path, mode) + assert not os.stat(path).st_mode & stat.S_ISGID + return path + + +@posix_only +class TestNewFilesTakeTheDirectoryGroup: + def test_without_setgid(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + + cache.set('display_current_state', {'mode': 'clock'}) + + st = os.stat(tmp_path / 'display_current_state.json') + assert st.st_gid == gid + assert stat.S_IMODE(st.st_mode) == 0o660 + + def test_the_file_is_never_visible_with_a_narrower_mode(self, tmp_path, monkeypatch): + """mkstemp creates 0600; the rename used to publish it before the chmod.""" + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + seen = [] + real_replace = os.replace + + def spy(src, dst): + st = os.stat(src) + seen.append((stat.S_IMODE(st.st_mode), st.st_gid)) + return real_replace(src, dst) + + monkeypatch.setattr(disk_cache_module.os, 'replace', spy) + cache.set('k', {'v': 1}) + + assert seen == [(0o660, gid)] + + def test_a_rewrite_fixes_a_file_left_with_the_wrong_group(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + path = tmp_path / 'k.json' + path.write_text('{}') + os.chown(path, -1, os.getegid()) + + cache.set('k', {'v': 2}) + + assert os.stat(path).st_gid == gid + + def test_the_direct_write_fallback_shares_too(self, tmp_path, monkeypatch): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + + def no_temp(*args, **kwargs): + raise OSError("no temp files") + + monkeypatch.setattr(disk_cache_module.tempfile, 'mkstemp', no_temp) + cache.set('k', {'v': 3}) + + st = os.stat(tmp_path / 'k.json') + assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o660, gid) + + def test_a_directory_not_shared_with_its_group_is_left_alone(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid, mode=0o755))) + + cache.set('k', {'v': 4}) + + assert os.stat(tmp_path / 'k.json').st_gid == os.getegid() + + +@posix_only +class TestExistingFilesAreRepaired: + def test_a_root_style_file_becomes_readable(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + path = tmp_path / 'display_on_demand_state.json' + path.write_text('{}') + os.chown(path, -1, os.getegid()) + os.chmod(path, 0o600) + + assert cache.share_existing_files() == 1 + + st = os.stat(path) + assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o660, gid) + + def test_files_already_shared_are_not_counted(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + cache.set('k', {'v': 1}) + + assert cache.share_existing_files() == 0 + + def test_only_json_files(self, tmp_path): + gid = _another_group() + cache = DiskCache(str(_shared_dir(tmp_path, gid))) + other = tmp_path / 'tile.png' + other.write_bytes(b'x') + os.chown(other, -1, os.getegid()) + + cache.share_existing_files() + + assert os.stat(other).st_gid == os.getegid() + + def test_a_symlink_is_not_followed(self, tmp_path): + """The directory is writable by the web user; root must not be aimed elsewhere.""" + gid = _another_group() + cache_dir = tmp_path / 'cache' + cache_dir.mkdir() + _shared_dir(cache_dir, gid) + outside = tmp_path / 'outside' + outside.write_text('secret') + os.chown(outside, -1, os.getegid()) + os.chmod(outside, 0o600) + (cache_dir / 'evil.json').symlink_to(outside) + + assert DiskCache(str(cache_dir)).share_existing_files() == 0 + + st = os.stat(outside) + assert (stat.S_IMODE(st.st_mode), st.st_gid) == (0o600, os.getegid()) + + def test_a_hard_linked_file_is_skipped(self, tmp_path): + gid = _another_group() + cache_dir = tmp_path / 'cache' + cache_dir.mkdir() + _shared_dir(cache_dir, gid) + outside = tmp_path / 'outside' + outside.write_text('secret') + os.chown(outside, -1, os.getegid()) + os.chmod(outside, 0o600) + os.link(outside, cache_dir / 'linked.json') + + assert DiskCache(str(cache_dir)).share_existing_files() == 0 + assert stat.S_IMODE(os.stat(outside).st_mode) == 0o600 + + @root_only + def test_another_users_file_is_skipped(self, tmp_path): + cache = DiskCache(str(_shared_dir(tmp_path, OTHER_GID))) + path = tmp_path / 'theirs.json' + path.write_text('{}') + os.chown(path, OTHER_UID, 0) + os.chmod(path, 0o600) + + assert cache.share_existing_files() == 0 + assert os.stat(path).st_gid == 0 + + +@posix_only +@root_only +def test_a_non_root_member_of_the_group_can_read_what_root_wrote(): + """The rig, end to end: root writes, the web user reads.""" + # Not tmp_path: pytest's root-owned base directory is 0700, which the + # reader could not traverse no matter what the cache file's group was. + base = tempfile.mkdtemp() + try: + _root_writes_web_user_reads(Path(base)) + finally: + shutil.rmtree(base, ignore_errors=True) + + +def _root_writes_web_user_reads(base): + os.chmod(base, 0o755) + cache_dir = base / 'cache' + cache_dir.mkdir() + # What systemd's CacheDirectory= left behind: the web user's own group. + os.chown(cache_dir, OTHER_UID, OTHER_GID) + os.chmod(cache_dir, 0o775) + DiskCache(str(cache_dir)).set('display_current_state', {'mode': 'clock'}) + + pid = os.fork() + if pid == 0: # pragma: no cover - child + code = 1 + try: + os.setgroups([]) + os.setgid(OTHER_GID) + os.setuid(OTHER_UID) + with open(cache_dir / 'display_current_state.json', 'rb') as f: + code = 0 if b'clock' in f.read() else 2 + except PermissionError: + code = 13 + finally: + os._exit(code) + _, status = os.waitpid(pid, 0) + assert os.WEXITSTATUS(status) == 0, "web user could not read the display's cache file" + + +def test_the_web_unit_does_not_let_systemd_re_own_the_cache(): + template = Path(__file__).resolve().parent.parent / 'systemd' / 'ledmatrix-web.service' + directives = [line.strip() for line in template.read_text(encoding='utf-8').splitlines() + if line.strip() and not line.strip().startswith('#')] + offending = [d for d in directives if d.startswith(('CacheDirectory', 'StateDirectory'))] + assert not offending, ( + f"{offending}: systemd re-owns that directory to the web user, and the " + "display service (root) shares it") + + +def test_the_cleanup_thread_repairs_existing_files(tmp_path, monkeypatch): + from src.cache_manager import CacheManager + + calls = [] + monkeypatch.setattr(CacheManager, '_get_writable_cache_dir', lambda self: str(tmp_path)) + monkeypatch.setattr(DiskCache, 'share_existing_files', + lambda self: calls.append(self.cache_dir) or 0) + CacheManager._cleanup_owners.clear() + manager = CacheManager() + try: + deadline = time.monotonic() + 5 + while not calls and time.monotonic() < deadline: + time.sleep(0.01) + finally: + manager.stop_cleanup_thread() + CacheManager._cleanup_owners.clear() + + assert calls == [str(tmp_path)] diff --git a/test/test_web_display_state_freshness.py b/test/test_web_display_state_freshness.py new file mode 100644 index 00000000..3349db00 --- /dev/null +++ b/test/test_web_display_state_freshness.py @@ -0,0 +1,62 @@ +"""Tests that the web UI reports the display's state as it is now. + +The display service writes display_on_demand_state and display_current_state; +the web interface is a different process and can only see them on disk. Its +reads went through the cache's memory tier, which keeps the first copy read +for the full max_age (120s). Measured on a rig once the web interface could +read these files at all: /display/on-demand/status said "active" for over 100 +seconds while the file on disk had said "idle" the whole time. +""" + +import json + +import pytest +from flask import Flask + +import web_interface.blueprints.api_v3 as api_pkg +from src.cache_manager import CacheManager + + +@pytest.fixture +def two_processes(tmp_path, monkeypatch): + """A display-side and a web-side cache over one directory, as on a rig.""" + monkeypatch.setattr(CacheManager, '_get_writable_cache_dir', lambda self: str(tmp_path)) + monkeypatch.setattr(CacheManager, 'start_cleanup_thread', lambda self: None) + display, web = CacheManager(), CacheManager() + monkeypatch.setattr(api_pkg, 'cache_manager', web) + monkeypatch.setattr(api_pkg, '_get_display_service_status', lambda: {'active': True}) + return display + + +@pytest.fixture +def client(): + app = Flask(__name__) + app.config['TESTING'] = True + if 'api_v3' not in app.blueprints: + app.register_blueprint(api_pkg.api_v3, url_prefix='/api/v3') + with app.test_client() as c: + yield c + + +def _get(client, url): + response = client.get(url) + assert response.status_code == 200 + return json.loads(response.data)['data'] + + +def test_on_demand_status_sees_the_stop_immediately(client, two_processes): + two_processes.set('display_on_demand_state', {'active': True, 'status': 'active'}) + assert _get(client, '/api/v3/display/on-demand/status')['state']['status'] == 'active' + + two_processes.set('display_on_demand_state', {'active': False, 'status': 'idle'}) + + assert _get(client, '/api/v3/display/on-demand/status')['state']['status'] == 'idle' + + +def test_current_status_sees_the_mode_change_immediately(client, two_processes): + two_processes.set('display_current_state', {'mode': 'odds_ticker'}) + assert _get(client, '/api/v3/display/current-status')['mode'] == 'odds_ticker' + + two_processes.set('display_current_state', {'mode': 'stocks'}) + + assert _get(client, '/api/v3/display/current-status')['mode'] == 'stocks' diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 4b0a3374..17ca4c3f 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -568,21 +568,41 @@ def _installed_plugin_ids(): enumerate the installed plugins and read each one's persisted summary by ID instead of relying on the tracker's in-memory `get_all_*` view. """ - pm = api_v3.plugin_manager - manifests = getattr(pm, 'plugin_manifests', None) - if not manifests: - # Only pay for a discovery scan when we haven't discovered anything yet; - # subsequent polls reuse the already-populated manifest map. - try: - pm.discover_plugins() - except Exception: - logger.debug('discover_plugins failed while listing plugin ids', exc_info=True) - manifests = getattr(pm, 'plugin_manifests', None) + manifests = _discovered_plugin_manifests() try: return list(manifests.keys()) if manifests else [] except Exception: logger.debug('listing plugin_manifests failed while building plugin ids', exc_info=True) return [] +def _discovered_plugin_manifests(plugin_id=None, rescan=False): + """The plugin manager's manifests, discovering plugins first if needed. + + The web process discovers plugins lazily (see app.py): nothing scans at + startup, so plugin_manifests is empty until some endpoint calls + discover_plugins(). A route that looks a plugin up without coming through + here answers "not found" for every installed plugin until something else + has run -- after a web restart, POST /display/on-demand/start returned 404 + for minutes on a real rig, and only API-only callers ever noticed. + + Scans when nothing is discovered yet, when ``plugin_id`` is given and not + among the manifests (it may have been installed since the last scan), or + when ``rescan`` is set (for lookups that are not by id, such as a mode). + Otherwise the existing map is reused, so a steady stream of requests + for known plugins costs nothing. + + Returns the manifest map, or {} when there is no plugin manager. + """ + pm = api_v3.plugin_manager + if pm is None: + return {} + manifests = getattr(pm, 'plugin_manifests', None) + if not manifests or rescan or (plugin_id is not None and plugin_id not in manifests): + try: + pm.discover_plugins() + except Exception: + logger.warning('Plugin discovery failed', exc_info=True) + manifests = getattr(pm, 'plugin_manifests', None) + return manifests or {} def _do_transactional_uninstall(plugin_id, preserve_config): """Execute an uninstall with snapshot-based rollback. diff --git a/web_interface/blueprints/api_v3/config.py b/web_interface/blueprints/api_v3/config.py index 3805b6fd..01c9b3d7 100644 --- a/web_interface/blueprints/api_v3/config.py +++ b/web_interface/blueprints/api_v3/config.py @@ -1001,9 +1001,14 @@ def save_main_config(): # flagged degraded at its next load). plugin_keys_to_remove = [] plugin_secrets_updates = {} + # Discovered first: a plugin key not recognised here skips secret + # separation below and falls through to the generic merge, which wrote + # the plugin's API key into config.json in plain text whenever nothing + # had yet discovered plugins in this process (any save after a restart). + plugin_manifests = _pkg._discovered_plugin_manifests() for key in data: # Check if this key is a plugin ID - if api_v3.plugin_manager and key in api_v3.plugin_manager.plugin_manifests: + if api_v3.plugin_manager and key in plugin_manifests: plugin_id = key submitted_config = data[key] if not isinstance(submitted_config, dict): diff --git a/web_interface/blueprints/api_v3/display.py b/web_interface/blueprints/api_v3/display.py index afb2f5d9..7b632c94 100644 --- a/web_interface/blueprints/api_v3/display.py +++ b/web_interface/blueprints/api_v3/display.py @@ -138,7 +138,11 @@ def get_on_demand_status(): """Return the current on-demand display state.""" try: cache = _ensure_cache_manager() - state = cache.get('display_on_demand_state', max_age=120) + # memory_ttl=0: the display service writes this key, so only the file + # is current. This process's memory tier would keep serving the first + # copy it read for the full max_age -- "active" for two minutes after + # the display had already stopped. + state = cache.get('display_on_demand_state', max_age=120, memory_ttl=0) if state is None: state = { 'active': False, @@ -174,14 +178,20 @@ def start_on_demand_display(): resolved_mode = mode if api_v3.plugin_manager: - if resolved_plugin and resolved_plugin not in api_v3.plugin_manager.plugin_manifests: + if resolved_plugin and resolved_plugin not in _pkg._discovered_plugin_manifests(resolved_plugin): return jsonify({'status': 'error', 'message': f'Plugin {resolved_plugin} not found'}), 404 if resolved_plugin and not resolved_mode: modes = api_v3.plugin_manager.get_plugin_display_modes(resolved_plugin) resolved_mode = modes[0] if modes else resolved_plugin elif resolved_mode and not resolved_plugin: + _pkg._discovered_plugin_manifests() resolved_plugin = api_v3.plugin_manager.find_plugin_for_mode(resolved_mode) + if not resolved_plugin: + # Not among what was discovered: the plugin that declares + # it may have been installed since. Scan once more. + _pkg._discovered_plugin_manifests(rescan=True) + resolved_plugin = api_v3.plugin_manager.find_plugin_for_mode(resolved_mode) if not resolved_plugin: return jsonify({'status': 'error', 'message': f'Mode {resolved_mode} not found'}), 404 @@ -304,7 +314,8 @@ def get_current_display_status(): """ try: cache = _ensure_cache_manager() - state = cache.get('display_current_state', max_age=120) + # memory_ttl=0: written by the display service; see get_on_demand_status. + state = cache.get('display_current_state', max_age=120, memory_ttl=0) if state is None: state = { 'mode': None, diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index ad483c4b..fbe62390 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -453,7 +453,7 @@ def toggle_plugin(): return _toggle_starlark_app(plugin_id[len('starlark:'):], enabled) # Check if plugin exists in manifests (discovered but may not be loaded) - if plugin_id not in api_v3.plugin_manager.plugin_manifests: + if plugin_id not in _pkg._discovered_plugin_manifests(plugin_id): return jsonify({'status': 'error', 'message': 'Plugin not found'}), 404 # Update config (this is what the display controller reads)