mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 06:15:09 +00:00
fix(cache): web UI can read what the display service caches again (#593)
* fix(cache): web UI can read what the display service caches again ledmatrix-web.service carried CacheDirectory=ledmatrix. With User= set to the installing user, systemd re-owns /var/cache/ledmatrix and everything in it to that user and its primary group whenever the directory's owner differs -- for a directory root created, on the first start. That erased the root:ledmatrix setgid layout the installers set up, so every file the display service (root) wrote afterwards was root:root 0660 and unreadable by the web interface: WARNING - Permission denied loading cache for display_current_state ... Since #547 install_service.sh renders the web unit from the template, so every fresh install hit this. Measured on one rig: 392 unreadable files, and the web UI's display status, on-demand state and plugin health empty. Existing installs only receive `git pull`, never a reinstalled unit, so the fix for them is in the code the root display service runs: - DiskCache.set gives each file the directory's group (when the directory is group-writable) and 0660 on the open descriptor before the rename, independent of setgid. This also closes a window where a fresh file was visible as mkstemp's 0600. - DiskCache.share_existing_files repairs files an older version left behind, once per process from the cleanup thread. It works through O_NOFOLLOW descriptors and skips hard links and other users' files: the directory is writable by the web user, and root must not be steered into changing a file outside it. For new installs, the web unit drops CacheDirectory=/CacheDirectoryMode=, and install_web_service.sh stops replacing an existing directory's ledmatrix group with the user's group. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): on-demand and current-display status read the display's latest state Found testing the cache-permission fix on a rig: once the web interface could read display_on_demand_state at all, /display/on-demand/status kept answering "active" for over 100 seconds while the file on disk said "idle". Both status routes read the display service's keys through the web process's memory tier, which serves the first copy it read for the full max_age (120s). Read them with memory_ttl=0, as every other cross-process reader (plugin health/metrics, the on-demand mailbox) already does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(install): re-group the cache dir whenever the web user is outside its group install_web_service.sh replaced an existing cache directory's group only when it was root's. A directory in any other group the web user is not a member of -- root:ledmatrix, for a user who is not in ledmatrix -- was left alone, and every file root wrote there stayed unreadable to the web interface. Replace the group whenever the installing user is not in it. A directory whose group the user is already in (ledmatrix, or the user's own group where CacheDirectory= left it) is still left as it is: re-grouping a working directory strands the files already in it on the old group. When the group does change and root-owned JSON files carrying the old group are present, try-restart ledmatrix.service so DiskCache.share_existing_files re-groups them through its symlink- and hard-link-safe path, rather than a recursive chgrp. Verified under WSL's systemd for seven directory states (user group, ledmatrix member, ledmatrix non-member with and without root files, root:root, missing, unnamed gid); the previous version left the non-member case unchanged. Addresses CodeRabbit review on #593. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Chuck
@@ -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)]
|
||||
@@ -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'
|
||||
Reference in New Issue
Block a user