mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-11 01:26:37 +00:00
Reported after a fresh install: installing an app from the Starlark tab
failed with "install failed: Failed to install from repository", and so did
uploading a .star file and installing from a GitHub directory. The reporter
found the cause only by reading service logs, and fixed it with
sudo chown -R ledpi:ledpi /home/ledpi/LEDMatrix/starlark-apps
starlark-apps is gitignored, so it is never checked out -- it is created
lazily by whichever process reaches it first. Those processes run as
different users. systemd/ledmatrix.service is User=root and constructs this
plugin at startup, which is where _get_apps_directory() is called from;
systemd/ledmatrix-web.service runs as the login user and is what actually
installs apps.
The documented first step is to install pixlet and reboot, so on a fresh
machine the display service usually wins that race and mkdir() leaves the
directory root-owned. The web process then fails in _install_star_file() on
app_dir.mkdir(), which catches nothing, so PermissionError reaches the
route's outer `except Exception` and becomes the generic message the user
saw. All three install paths write to the same directory, which is why all
three failed.
The web user cannot repair this -- chown needs root. So root does it, on
every startup, which also heals machines already broken by this without the
owner having to find the chown themselves. It is a no-op when not root, when
the platform has no POSIX ownership, and when the checkout genuinely belongs
to root; a chown that fails warns rather than killing startup.
Also made the failure legible if the handover is ever prevented: a
PermissionError now names the directory, the automatic repair, and the
manual chown, instead of a message that names neither path nor cause.
Verified by mutation: dropping the handover call, chowning a genuinely
root-owned checkout, and letting a non-root process chown each fail their
own test. 121 starlark tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
265 lines
9.7 KiB
Python
265 lines
9.7 KiB
Python
"""starlark-apps: the root display service must not lock the web UI out.
|
|
|
|
Reported by a user after a fresh install: installing an app from the Starlark
|
|
tab failed with "install failed: Failed to install from repository", and so did
|
|
uploading a .star file and installing from a GitHub directory. The cause was
|
|
ownership, which the error named nowhere -- they found it only by reading the
|
|
service logs, and fixed it with
|
|
|
|
sudo chown -R ledpi:ledpi /home/ledpi/LEDMatrix/starlark-apps
|
|
|
|
The starlark-apps directory is not in the repository, so it is created lazily
|
|
by whichever process reaches it first. Those processes run as different users:
|
|
systemd/ledmatrix.service is `User=root` and constructs this plugin at startup
|
|
(which is what calls _get_apps_directory), while systemd/ledmatrix-web.service
|
|
runs as the login user and is what actually installs apps. The documented
|
|
first step is to install pixlet and reboot, so on a fresh machine the display
|
|
service usually wins the race and the directory lands root-owned.
|
|
|
|
The web user cannot repair that -- chown needs root. So root hands the
|
|
directory over itself, every startup, which also heals machines already broken
|
|
by this.
|
|
"""
|
|
|
|
import importlib.util
|
|
import sys
|
|
import types
|
|
from pathlib import Path
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
PLUGIN_DIR = Path(__file__).resolve().parent.parent / "plugin-repos" / "starlark-apps"
|
|
|
|
|
|
@pytest.fixture(scope="module")
|
|
def manager_module():
|
|
if not PLUGIN_DIR.exists():
|
|
pytest.skip("starlark-apps plugin is not checked out")
|
|
sys.path.insert(0, str(PLUGIN_DIR))
|
|
injected_fcntl = "fcntl" not in sys.modules
|
|
if injected_fcntl:
|
|
stub = types.ModuleType("fcntl")
|
|
stub.LOCK_EX, stub.LOCK_UN = 2, 8
|
|
stub.flock = lambda *a, **kw: None
|
|
sys.modules["fcntl"] = stub
|
|
try:
|
|
spec = importlib.util.spec_from_file_location(
|
|
"starlark_manager_ownership", PLUGIN_DIR / "manager.py")
|
|
module = importlib.util.module_from_spec(spec)
|
|
spec.loader.exec_module(module)
|
|
return module
|
|
except Exception as e: # noqa: BLE001 - optional deps may be absent
|
|
pytest.skip(f"starlark-apps manager is not importable here: {e}")
|
|
finally:
|
|
sys.path.remove(str(PLUGIN_DIR))
|
|
if injected_fcntl:
|
|
sys.modules.pop("fcntl", None)
|
|
|
|
|
|
def _plugin(manager_module):
|
|
"""A manager with __init__ bypassed -- only ownership paths are tested."""
|
|
cls = manager_module.StarlarkAppsPlugin
|
|
inst = cls.__new__(cls)
|
|
inst.logger = MagicMock()
|
|
return inst
|
|
|
|
|
|
class _Stat:
|
|
"""Only st_uid/st_gid are read by the code under test."""
|
|
|
|
def __init__(self, uid, gid):
|
|
self.st_uid = uid
|
|
self.st_gid = gid
|
|
|
|
|
|
@pytest.fixture
|
|
def owned(monkeypatch):
|
|
"""Let a test declare a fake uid/gid for specific paths.
|
|
|
|
Real ownership cannot be faked without root, and these tests must run as
|
|
an ordinary user in CI.
|
|
"""
|
|
fake = {}
|
|
real_stat = Path.stat
|
|
|
|
def patched(self, *args, **kwargs):
|
|
key = str(self)
|
|
if key in fake:
|
|
return _Stat(*fake[key])
|
|
return real_stat(self, *args, **kwargs)
|
|
|
|
monkeypatch.setattr(Path, "stat", patched)
|
|
return fake
|
|
|
|
|
|
@pytest.fixture
|
|
def as_root(monkeypatch):
|
|
"""Run the handover as root, recording chowns instead of performing them."""
|
|
calls = []
|
|
monkeypatch.setattr("os.geteuid", lambda: 0)
|
|
monkeypatch.setattr("os.chown", lambda p, uid, gid: calls.append((str(p), uid, gid)))
|
|
return calls
|
|
|
|
|
|
def _tree(tmp_path):
|
|
"""A project root with an apps directory holding one installed app."""
|
|
apps = tmp_path / "starlark-apps"
|
|
(apps / "analogclock").mkdir(parents=True)
|
|
(apps / "analogclock" / "analog_clock.star").write_text("# app")
|
|
(apps / "manifest.json").write_text("{}")
|
|
return apps
|
|
|
|
|
|
class TestRootHandsTheDirectoryOver:
|
|
def test_root_created_directory_is_given_to_the_checkout_owner(
|
|
self, manager_module, tmp_path, owned, as_root):
|
|
apps = _tree(tmp_path)
|
|
owned[str(tmp_path)] = (1000, 1000) # checkout belongs to the login user
|
|
owned[str(apps)] = (0, 0) # but root got there first
|
|
|
|
_plugin(manager_module)._hand_apps_dir_to_checkout_owner(apps, tmp_path)
|
|
|
|
assert (str(apps), 1000, 1000) in as_root
|
|
|
|
def test_contents_are_repaired_not_just_the_directory(
|
|
self, manager_module, tmp_path, owned, as_root):
|
|
"""An install broken by this leaves root-owned files inside it too."""
|
|
apps = _tree(tmp_path)
|
|
owned[str(tmp_path)] = (1000, 1000)
|
|
for p in (apps, apps / "analogclock",
|
|
apps / "analogclock" / "analog_clock.star",
|
|
apps / "manifest.json"):
|
|
owned[str(p)] = (0, 0)
|
|
|
|
_plugin(manager_module)._hand_apps_dir_to_checkout_owner(apps, tmp_path)
|
|
|
|
chowned = {c[0] for c in as_root}
|
|
assert str(apps / "analogclock" / "analog_clock.star") in chowned
|
|
assert str(apps / "manifest.json") in chowned
|
|
|
|
def test_nothing_is_touched_when_ownership_is_already_right(
|
|
self, manager_module, tmp_path, owned, as_root):
|
|
apps = _tree(tmp_path)
|
|
owned[str(tmp_path)] = (1000, 1000)
|
|
for p in (apps, apps / "analogclock",
|
|
apps / "analogclock" / "analog_clock.star",
|
|
apps / "manifest.json"):
|
|
owned[str(p)] = (1000, 1000)
|
|
|
|
_plugin(manager_module)._hand_apps_dir_to_checkout_owner(apps, tmp_path)
|
|
|
|
assert as_root == []
|
|
|
|
|
|
class TestItDoesNotOverreach:
|
|
def test_a_non_root_process_changes_nothing(
|
|
self, manager_module, tmp_path, owned, monkeypatch):
|
|
"""The web service also calls this. It has no right to chown anything."""
|
|
apps = _tree(tmp_path)
|
|
owned[str(tmp_path)] = (1000, 1000)
|
|
owned[str(apps)] = (0, 0)
|
|
calls = []
|
|
monkeypatch.setattr("os.geteuid", lambda: 1000)
|
|
monkeypatch.setattr("os.chown", lambda p, u, g: calls.append(p))
|
|
|
|
_plugin(manager_module)._hand_apps_dir_to_checkout_owner(apps, tmp_path)
|
|
|
|
assert calls == []
|
|
|
|
def test_a_genuinely_root_owned_checkout_is_left_alone(
|
|
self, manager_module, tmp_path, owned, as_root):
|
|
"""Installed as root on purpose: there is nobody to hand it to."""
|
|
apps = _tree(tmp_path)
|
|
owned[str(tmp_path)] = (0, 0)
|
|
owned[str(apps)] = (0, 0)
|
|
|
|
_plugin(manager_module)._hand_apps_dir_to_checkout_owner(apps, tmp_path)
|
|
|
|
assert as_root == []
|
|
|
|
def test_a_failed_chown_warns_and_does_not_raise(
|
|
self, manager_module, tmp_path, owned, monkeypatch):
|
|
"""Startup must not die because one file could not be handed over."""
|
|
apps = _tree(tmp_path)
|
|
owned[str(tmp_path)] = (1000, 1000)
|
|
owned[str(apps)] = (0, 0)
|
|
monkeypatch.setattr("os.geteuid", lambda: 0)
|
|
|
|
def boom(*_a, **_kw):
|
|
raise OSError("read-only file system")
|
|
|
|
monkeypatch.setattr("os.chown", boom)
|
|
plugin = _plugin(manager_module)
|
|
|
|
plugin._hand_apps_dir_to_checkout_owner(apps, tmp_path) # must not raise
|
|
|
|
assert plugin.logger.warning.called
|
|
|
|
|
|
class TestTheDirectoryGetterUsesIt:
|
|
def test_get_apps_directory_performs_the_handover(
|
|
self, manager_module, monkeypatch):
|
|
"""Pins the wiring: creating the directory without handing it over is
|
|
exactly the bug.
|
|
|
|
This calls the real getter, which resolves to the checkout's own
|
|
starlark-apps directory, so it removes the directory again when the
|
|
test was what created it.
|
|
"""
|
|
plugin = _plugin(manager_module)
|
|
seen = []
|
|
monkeypatch.setattr(
|
|
type(plugin), "_hand_apps_dir_to_checkout_owner",
|
|
lambda self, apps, root: seen.append((apps, root)))
|
|
expected = PLUGIN_DIR.parent.parent / "starlark-apps"
|
|
pre_existing = expected.exists()
|
|
|
|
try:
|
|
result = plugin._get_apps_directory()
|
|
|
|
assert result == expected and result.exists()
|
|
assert seen and seen[0] == (expected, expected.parent)
|
|
finally:
|
|
if not pre_existing and expected.exists():
|
|
try:
|
|
expected.rmdir()
|
|
except OSError:
|
|
pass
|
|
|
|
|
|
class TestTheErrorNamesTheCause:
|
|
"""The reporter saw only "Failed to install from repository".
|
|
|
|
That message names no path and no cause, so the only way to the answer was
|
|
reading the service logs. The handover above should stop the failure
|
|
happening at all; this makes the failure legible if it ever does.
|
|
"""
|
|
|
|
@pytest.fixture(scope="class")
|
|
def hint(self):
|
|
try:
|
|
from web_interface.blueprints.api_v3.starlark import _ownership_hint
|
|
except Exception as e: # noqa: BLE001 - Flask/web deps may be absent
|
|
pytest.skip(f"web_interface is not importable here: {e}")
|
|
return _ownership_hint
|
|
|
|
def test_a_permission_error_explains_itself(self, hint):
|
|
message = hint(PermissionError(13, "Permission denied"))
|
|
assert message
|
|
assert "chown" in message
|
|
assert "starlark-apps" in message
|
|
|
|
def test_it_points_at_the_automatic_repair_first(self, hint):
|
|
"""Restarting the display service is the fix that needs no root user
|
|
to understand it -- the manual chown is the fallback."""
|
|
message = hint(PermissionError(13, "Permission denied"))
|
|
assert "systemctl restart ledmatrix" in message
|
|
assert message.index("systemctl") < message.index("chown")
|
|
|
|
def test_other_failures_are_not_mislabelled(self, hint):
|
|
"""A network error must not be reported as an ownership problem."""
|
|
for err in (ValueError("bad json"), OSError(28, "No space left on device"),
|
|
TimeoutError("github timed out")):
|
|
assert hint(err) is None
|