mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 14:25:08 +00:00
refactor(api-v3): split the 10,469-line blueprint into a package (#553)
* refactor(api-v3): split the 10,469-line blueprint into a package web_interface/blueprints/api_v3.py held 111 routes, 56 helpers and 181 functions in one module -- 9% of the core by line count and three times the next largest file. It becomes a package of nine route modules grouped by path segment, plus __init__.py for the shared imports, constants, Blueprint and helpers. Every route module decorates the SAME api_v3 Blueprint object, so endpoint names stay api_v3.<function>, the URL map is unchanged and app.py is untouched. Verified: 111 routes before, 111 after, byte-identical rules, endpoints and methods, and every endpoint still on the one blueprint. plugins 3,867 config 1,178 starlark 692 system 619 fonts 452 misc 398 wifi 361 display 326 backup 212 __init__ 1,787 (imports, constants, Blueprint, 56 helpers) Two things the URL-map check could not catch, both found by running the suite: 1. PROJECT_ROOT = Path(__file__).parent.parent.parent. Moving the code one directory deeper made that resolve to web_interface/ instead of the project root. Nothing failed at import; it surfaced as ~110 tests failing with 404s and "installation script not found", because every path built from it was one level too shallow. Now parents[3], and test_api_v3_url_map.py asserts PROJECT_ROOT/run.py exists so the next move cannot repeat it. 2. Module-attribute patching. Tests do monkeypatch.setattr(api_v3_module, "_BACKUP_EXPORT_DIR", ...) and a route module that binds such a name by value never sees the patch. The shared code therefore stays in __init__.py rather than moving to a _common submodule -- it has to live on the module the tests patch -- and the eleven names tests patch are read back through the package (_pkg.X) instead of bound by value. Those eleven were found by AST-scanning every setattr in the test tree, not by guessing; "time" is among them, used to drive a fake clock through the second-resolution credential-backup filenames. Test changes are confined to what genuinely moved: patch targets that now name the owning route module, imports of helpers, and six tests that scan the api_v3 source as a file and now read the package directory. Full suite: 4,278 passed, 68 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(api-v3): address CodeRabbit findings from the blueprint-split review Fixes to the api_v3 package split (PR #553), one per finding verified against the actual code: - __init__.py: _redact_credentials only blanked scalar values under a credential-named key; a bare list of secrets under such a key (e.g. tokens: ["a", "b"]) passed through untouched, since the list branch recursed with no memory that its key looked like a credential. Nested dicts still walk normally (a documented, tested behaviour -- a container like secrets: {api_key: ..., note: ...} is a section name, not a value to blank outright), but any value reached under a credential-shaped key is now actually blanked. - __init__.py: the OAuth helper script's raw stderr/stdout went to logger.error unredacted (CWE-532) right next to a comment claiming this was deliberate; the HTTP response already used the existing redact_text helper. Routed the log line through the same helper. - __init__.py / starlark.py: the standalone Starlark manifest fallback (used when the plugin instance isn't loaded) read-modified-wrote manifest.json with no lock, unlike StarlarkAppsPlugin._update_manifest_safe (plugin-repos/starlark-apps/manager.py), which already holds an flock for the same file when the plugin is loaded. Added _starlark_manifest_lock, mirroring that pattern, and wrapped every standalone read-modify-write call site in it. The app-config update route also wrote config.json and the manifest as two separate, non-transactional writes (a second, distinct finding at the same call site); config.json is now rolled back if the manifest write that follows it fails. - backup.py: restore options used bare bool() on values from the request, so {"restore_secrets": "false"} restored secrets anyway (bool("false") is True). Switched to the existing _coerce_to_bool helper already used for this exact purpose elsewhere in the package. - config.py: an automated import-rewrite mangled four user-facing validation strings and their neighbouring comments -- "Invalid start time" had become "Invalid start _pkg.time" (and likewise for "end time") in both the schedule and dim-schedule per-day validation paths. - display.py: `import _pkg.time as time_module` -- _pkg is a local alias for the package, not a real importable module, so this raised ModuleNotFoundError whenever a caller restarted an already-running display service via /display/on-demand/start, after the on-demand request was already written to cache. Fixed to `import time`. Audited the rest of the package for the same `_pkg.<module>` import mistake; every other `_pkg.` reference is a legitimate attribute read-through (`_pkg.time.time()`, `_pkg._get_starlark_plugin()`, ...), not a broken import statement. - fonts.py: validate_file_upload's max_size_mb parameter is silently unused by that helper (it only checks filename/extension) -- the font upload route saved arbitrarily large files as a result. Added the same seek-and-check pattern already used for the sibling .star upload. - wifi.py: two ad hoc, inconsistent bool coercions. POST /wifi/ap/auto-enable used bare bool(), so a JSON string "false" enabled it. POST /wifi/radio's enabled/force parsing recognized real bool and some strings but not int 1/0 (1 is True is False in Python). Factored one small _parse_bool_ish helper local to this file and used it at all three sites. Not changed: the "unknown/misspelled restore option keys default to True" half of the backup.py finding -- the file's own comment documents that a missing key deliberately means "restore everything," matching the already-existing JSON-parse-failure guard a few lines above it; only the bool-coercion defect was a real bug. Added or extended regression tests for every fix, following each area's existing test conventions. Full suite: 4328 passed, 62 skipped, 2 failed on both this branch and origin/main (missing tzdata package breaks two timezone-alias tests in test_onboarding_checklist.py, unrelated to this change) -- no new failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5 * fix(api-v3): reject unknown restore option keys CodeRabbit's review of the blueprint split (#553) asked that POST /backup/restore reject option keys outside RestoreOptions' known set. The follow-up commit fixed the bool("false")-is-True bug with _coerce_to_bool but never added the key check: a typo'd or renamed key (e.g. "restoreSecrets") is silently ignored by opts_dict.get(key, True), so the flag stays at its True default and secrets get restored despite the caller's request saying otherwise -- with no indication anything was wrong. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vmcwf5vMgYqdt8bJTZtiwb * fix(api-v3): address CodeRabbit findings on the blueprint split - _redact_credentials: blank scalar descendants of objects reached through a credential-owned list (e.g. tokens: [{"value": "secret"}]) regardless of field name -- the existing name-based walk only protected direct dict values under a credential key, not list items. - wifi.py: reject enabled/force/auto_enable_ap_mode values _parse_bool_ish can't recognize (400) instead of silently treating them as False, which could disable Wi-Fi or the radio itself. - Starlark manifest locking: lock a stable manifest.json.lock sidecar instead of manifest.json itself, in both the standalone route path (_starlark_manifest_lock) and the plugin path (StarlarkAppsPlugin._save_manifest / _update_manifest_safe). manifest.json is replaced by an atomic rename on every write, which swaps in a fresh inode; a lock held on the old inode does not exclude a second locker that opens the path afresh right after the rename and gets the new inode, so two writers could race despite each holding "a lock". A sidecar that no write ever touches always resolves to the same inode for every locker. Skipped as stale: the "serialize the complete manifest read-modify-write" finding at api_v3/__init__.py -- every standalone handler that calls _write_starlark_manifest is already wrapped in _starlark_manifest_lock() on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(api-v3): re-check reconciliation findings by the reconciler's own rules Both CodeRabbit findings on the merge commit, verified against the code first. Major, plugins.py: the stale-findings filter derived its own notion of "in config" and "on disk", and both were looser than the reconciliation module's. set(load_config()) also contains system keys, the secrets-file keys load_config() merges in, and non-dict values; and any directory holding a manifest.json counted as installed even when that manifest does not parse. Either looseness clears a finding that is still true -- and a secrets key read as a plugin is the precise bug the filter exists to stop reporting, so reintroducing that asymmetry while re-checking was the wrong way round. The two extractions now live in state_reconciliation.py as config_plugin_ids() and disk_plugin_ids(), with ignored_config_keys() and secrets_top_level_keys() alongside. _get_config_state() and _get_disk_state() use them too, so there is one definition rather than two that can drift. _get_disk_state() re-reads each manifest for version/name after taking membership from the shared extractor; that costs one extra small read per plugin on a path that runs once per boot. Minor, the new test: the fixture assigned api_v3.config_manager and api_v3.plugin_manager directly. Those live on a module-level blueprint singleton, so the mocks leaked into every later test that imports api_v3 -- pointing at a tmp_path already deleted. Both now go through monkeypatch.setattr, which restores them. This is the same pollution class that made an earlier test in this session break seven unrelated ones, so it is worth getting right. Five cases added for the parity itself: a secrets key, a system key and a non-dict value must not clear an "installed but missing from config" finding, and neither an unparseable manifest nor a .standalone-backup- directory may count as installed. All five fail against the looser version. Linux CI on the preceding commit: Core unit tests, plugin harness, CodeQL and CodeRabbit all pass. Codacy reads action_required on every commit of this branch including the first, so it is pre-existing and not from this work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chuck
@@ -139,6 +139,58 @@ class TestRequestValidation:
|
||||
assert post(client, options="{}").status_code == 200
|
||||
assert restore.call_args[0][2].restore_config is True
|
||||
|
||||
def test_unknown_option_key_is_refused(self, client, restore):
|
||||
# Regression: opts_dict.get('restore_secrets', True) silently
|
||||
# ignores a typo'd/renamed key like "restoreSecrets" and keeps the
|
||||
# True default, restoring secrets a caller's request clearly meant
|
||||
# to exclude -- with no indication anything was wrong.
|
||||
response = post(client, options=json.dumps({"restoreSecrets": False}))
|
||||
assert response.status_code == 400
|
||||
assert "Unknown restore option" in response.get_json()["message"]
|
||||
assert "restoreSecrets" in response.get_json()["message"]
|
||||
restore.assert_not_called()
|
||||
|
||||
def test_known_and_unknown_keys_together_are_refused(self, client, restore):
|
||||
response = post(client, options=json.dumps({
|
||||
"restore_secrets": False, "restore_everything": True}))
|
||||
assert response.status_code == 400
|
||||
restore.assert_not_called()
|
||||
|
||||
|
||||
class TestOptionsAreBooleanAware:
|
||||
"""Regression: bool("false") is True in Python.
|
||||
|
||||
Every restore flag used bare bool() coercion, so a caller that sends its
|
||||
options as JSON strings rather than real booleans -- a form field, a
|
||||
hand-built request -- had `{"restore_secrets": "false"}` restore secrets
|
||||
anyway, the opposite of what was asked. Fixed with the same
|
||||
string-aware `_coerce_to_bool` already used for checkbox-style config
|
||||
fields elsewhere in this package (config.py, plugins.py).
|
||||
"""
|
||||
|
||||
@pytest.mark.parametrize("raw,expected", [
|
||||
("false", False), ("False", False), ("FALSE", False),
|
||||
("0", False),
|
||||
("true", True), ("True", True), ("1", True),
|
||||
])
|
||||
def test_string_valued_flags_are_parsed_not_just_truthy(
|
||||
self, client, restore, raw, expected):
|
||||
post(client, options=json.dumps({"restore_secrets": raw}))
|
||||
assert restore.call_args[0][2].restore_secrets is expected
|
||||
|
||||
def test_a_string_false_does_not_restore_secrets(self, client, restore):
|
||||
# The exact shape of the bug: a truthy non-empty string coerced by
|
||||
# bare bool() to True regardless of its contents.
|
||||
post(client, options=json.dumps({"restore_secrets": "false"}))
|
||||
assert restore.call_args[0][2].restore_secrets is False
|
||||
|
||||
def test_real_json_booleans_still_work(self, client, restore):
|
||||
post(client, options=json.dumps({"restore_secrets": False,
|
||||
"restore_config": True}))
|
||||
options = restore.call_args[0][2]
|
||||
assert options.restore_secrets is False
|
||||
assert options.restore_config is True
|
||||
|
||||
|
||||
class TestSuccess:
|
||||
def test_success_returns_the_result(self, client, restore):
|
||||
@@ -230,7 +282,7 @@ class TestPluginReinstall:
|
||||
def test_missing_store_manager_is_reported_per_plugin(self, client, restore):
|
||||
restore.return_value = FakeResult(plugins_to_install=[{"plugin_id": "clock"}])
|
||||
api_v3.plugin_store_manager = None
|
||||
with patch("web_interface.blueprints.api_v3.plugin_store_manager", None):
|
||||
with patch("web_interface.blueprints.api_v3.backup.plugin_store_manager", None):
|
||||
body = post(client).get_json()
|
||||
assert body["data"]["plugins_failed"][0]["error"] == "Store manager unavailable"
|
||||
|
||||
|
||||
@@ -77,7 +77,10 @@ class TestTheRoutesExistAtAll:
|
||||
oauth = Path(project_root) / 'web_interface/static/v3/js/widgets/google-oauth.js'
|
||||
assert '/api/v3/plugins/calendar/list-calendars' in picker.read_text(encoding='utf-8')
|
||||
assert '/api/v3/plugins/calendar/authenticate' in oauth.read_text(encoding='utf-8')
|
||||
source = (Path(project_root) / 'web_interface/blueprints/api_v3.py').read_text(encoding='utf-8')
|
||||
# api_v3 is a package now, so the route strings are spread across its
|
||||
# modules; read the whole directory rather than one file.
|
||||
pkg = Path(project_root) / 'web_interface/blueprints/api_v3'
|
||||
source = "\n".join(f.read_text(encoding='utf-8') for f in sorted(pkg.glob('*.py')))
|
||||
assert "'/plugins/calendar/list-calendars'" in source
|
||||
assert "'/plugins/calendar/authenticate'" in source
|
||||
|
||||
@@ -360,6 +363,22 @@ class TestDiagnosticsAreRedacted:
|
||||
assert 'hunter2' not in error, error
|
||||
assert '<redacted>' in error, error
|
||||
|
||||
def test_script_stderr_is_redacted_in_the_log_too(self, tmp_path, caplog):
|
||||
# Regression: the return value went through redact_text (asserted
|
||||
# above), but the logger.error call right next to it logged `raw`
|
||||
# verbatim -- a script that handles OAuth client secrets and can
|
||||
# quote them in its stderr, landing unredacted in the log (CWE-532).
|
||||
script = tmp_path / 'calendar_registration.py'
|
||||
script.write_text(
|
||||
'import sys\n'
|
||||
'sys.stderr.write("boom client_secret=hunter2 more\\n")\n',
|
||||
encoding='utf-8')
|
||||
with caplog.at_level('ERROR', logger=mod.logger.name):
|
||||
mod._run_calendar_registration(tmp_path, '')
|
||||
logged = '\n'.join(r.getMessage() for r in caplog.records)
|
||||
assert 'hunter2' not in logged, logged
|
||||
assert '<redacted>' in logged, logged
|
||||
|
||||
def test_a_failing_script_payload_is_redacted(self, client):
|
||||
(client.plugin_dir / 'credentials.json').write_text('{}', encoding='utf-8')
|
||||
(client.plugin_dir / 'calendar_registration.py').write_text(
|
||||
|
||||
@@ -10,8 +10,8 @@ from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
SOURCE = (Path(__file__).resolve().parents[2]
|
||||
/ "web_interface" / "blueprints" / "api_v3.py")
|
||||
API_V3_PKG = (Path(__file__).resolve().parents[2]
|
||||
/ "web_interface" / "blueprints" / "api_v3")
|
||||
|
||||
#: Objects that still hold submitted secret values at the point these log
|
||||
#: calls run. Interpolating one whole into a log message leaks credentials.
|
||||
@@ -19,7 +19,7 @@ UNREDACTED = ("plugin_config", "secrets_config", "current_secrets")
|
||||
|
||||
|
||||
def _logging_lines():
|
||||
for number, line in enumerate(SOURCE.read_text(encoding="utf-8").splitlines(), 1):
|
||||
for number, line in enumerate("\n".join(p.read_text(encoding="utf-8") for p in sorted(API_V3_PKG.glob("*.py"))).splitlines(), 1):
|
||||
stripped = line.strip()
|
||||
if stripped.startswith("#"):
|
||||
continue
|
||||
|
||||
@@ -15,8 +15,18 @@ from pathlib import Path
|
||||
|
||||
from src.web_interface.secret_helpers import find_secret_fields, separate_secrets
|
||||
|
||||
API_V3_PATH = (Path(__file__).resolve().parents[2]
|
||||
/ "web_interface" / "blueprints" / "api_v3.py")
|
||||
API_V3_PKG = (Path(__file__).resolve().parents[2]
|
||||
/ "web_interface" / "blueprints" / "api_v3")
|
||||
|
||||
|
||||
def _api_v3_source() -> str:
|
||||
"""Every module of the api_v3 package as one string.
|
||||
|
||||
It used to be a single file; the inline copies this guards against could
|
||||
now reappear in any module of the package.
|
||||
"""
|
||||
return "\n".join(p.read_text(encoding="utf-8")
|
||||
for p in sorted(API_V3_PKG.glob("*.py")))
|
||||
|
||||
# The migration is complete: any inline reimplementation is a regression.
|
||||
EXPECTED_INLINE_COPIES = 0
|
||||
@@ -24,7 +34,7 @@ EXPECTED_INLINE_COPIES = 0
|
||||
|
||||
class TestNoInlineCopies:
|
||||
def _count(self, name: str) -> int:
|
||||
source = API_V3_PATH.read_text(encoding="utf-8")
|
||||
source = _api_v3_source()
|
||||
return len(re.findall(rf"^\s*def {name}\(", source, flags=re.MULTILINE))
|
||||
|
||||
def test_no_inline_find_secret_fields(self):
|
||||
@@ -46,7 +56,7 @@ class TestNoInlineCopies:
|
||||
def test_canonical_import_present(self):
|
||||
# Tripwire: the endpoints still need the helpers, so removing the
|
||||
# import means either dead secret handling or a new local copy.
|
||||
source = API_V3_PATH.read_text(encoding="utf-8")
|
||||
source = _api_v3_source()
|
||||
assert re.search(
|
||||
r"from src\.web_interface\.secret_helpers import .*find_secret_fields",
|
||||
source,
|
||||
|
||||
@@ -63,14 +63,14 @@ class TestRoutesAreRegistered:
|
||||
|
||||
class TestInstallPixlet:
|
||||
def test_it_does_not_404(self, client):
|
||||
with patch('web_interface.blueprints.api_v3.subprocess.run') as run:
|
||||
with patch('web_interface.blueprints.api_v3.starlark.subprocess.run') as run:
|
||||
run.return_value = MagicMock(returncode=0, stdout="ok", stderr="")
|
||||
resp = client.post('/api/v3/starlark/install-pixlet')
|
||||
assert resp.status_code != 404, "the route is still missing"
|
||||
assert resp.get_json().get('message') != 'Resource not found'
|
||||
|
||||
def test_success_is_reported_in_the_shape_the_button_reads(self, client):
|
||||
with patch('web_interface.blueprints.api_v3.subprocess.run') as run:
|
||||
with patch('web_interface.blueprints.api_v3.starlark.subprocess.run') as run:
|
||||
run.return_value = MagicMock(returncode=0, stdout="done", stderr="")
|
||||
resp = client.post('/api/v3/starlark/install-pixlet')
|
||||
body = resp.get_json()
|
||||
@@ -78,7 +78,7 @@ class TestInstallPixlet:
|
||||
assert 'message' in body, "the JS shows data.message on success"
|
||||
|
||||
def test_a_failed_download_says_why(self, client):
|
||||
with patch('web_interface.blueprints.api_v3.subprocess.run') as run:
|
||||
with patch('web_interface.blueprints.api_v3.starlark.subprocess.run') as run:
|
||||
run.return_value = MagicMock(returncode=1, stdout="", stderr="no such release")
|
||||
resp = client.post('/api/v3/starlark/install-pixlet')
|
||||
body = resp.get_json()
|
||||
@@ -88,7 +88,7 @@ class TestInstallPixlet:
|
||||
|
||||
def test_a_timeout_is_reported_rather_than_hanging(self, client):
|
||||
import subprocess as sp
|
||||
with patch('web_interface.blueprints.api_v3.subprocess.run',
|
||||
with patch('web_interface.blueprints.api_v3.starlark.subprocess.run',
|
||||
side_effect=sp.TimeoutExpired(cmd='x', timeout=300)):
|
||||
resp = client.post('/api/v3/starlark/install-pixlet')
|
||||
assert resp.get_json()['status'] == 'error'
|
||||
@@ -395,6 +395,164 @@ class TestTheManifestStaysRelocatable:
|
||||
assert self._install(tmp_path)['star_file'] == 'demo.star'
|
||||
|
||||
|
||||
class TestManifestLockPreventsLostUpdates:
|
||||
"""The standalone manifest fallback (no plugin instance loaded) reads,
|
||||
mutates and writes manifest.json with no coordination across requests.
|
||||
Each write is atomic on its own (temp file + rename), but two concurrent
|
||||
read-modify-write cycles can still race: both read the same starting
|
||||
manifest, and the second write silently discards whatever the first one
|
||||
added. _starlark_manifest_lock closes that window -- mirrors
|
||||
StarlarkAppsPlugin._update_manifest_safe, which already does this when
|
||||
the plugin instance is loaded.
|
||||
"""
|
||||
|
||||
@pytest.fixture
|
||||
def starlark_dir(self, tmp_path, monkeypatch):
|
||||
from web_interface.blueprints import api_v3 as module
|
||||
apps_dir = tmp_path / "starlark-apps"
|
||||
apps_dir.mkdir()
|
||||
monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir)
|
||||
monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json')
|
||||
monkeypatch.setattr(module, '_STARLARK_MANIFEST_LOCK_FILE', apps_dir / 'manifest.json.lock')
|
||||
module._write_starlark_manifest({'apps': {}})
|
||||
return apps_dir
|
||||
|
||||
def test_the_locked_file_survives_a_manifest_write(self, starlark_dir):
|
||||
"""_write_starlark_manifest replaces manifest.json with a fresh inode
|
||||
on every write (temp file + rename). If the lock were taken on that
|
||||
same file, a second locker's fresh os.open() right after the rename
|
||||
would land on the new inode -- unguarded, because only the old,
|
||||
now-orphaned inode was ever locked -- and two writers could race
|
||||
despite each believing it "held the lock" (see the docstring on
|
||||
_starlark_manifest_lock). Locking a sidecar path that no write ever
|
||||
touches or renames over closes that: the inode identity of what gets
|
||||
locked must not change across writes.
|
||||
"""
|
||||
import os
|
||||
|
||||
from web_interface.blueprints import api_v3 as module
|
||||
|
||||
with module._starlark_manifest_lock():
|
||||
manifest = module._read_starlark_manifest()
|
||||
lock_ino_before = os.stat(module._STARLARK_MANIFEST_LOCK_FILE).st_ino
|
||||
|
||||
for app_id in ('one', 'two', 'three'):
|
||||
with module._starlark_manifest_lock():
|
||||
manifest = module._read_starlark_manifest()
|
||||
manifest.setdefault('apps', {})[app_id] = {'enabled': True}
|
||||
assert module._write_starlark_manifest(manifest)
|
||||
|
||||
lock_ino_after = os.stat(module._STARLARK_MANIFEST_LOCK_FILE).st_ino
|
||||
assert lock_ino_after == lock_ino_before, (
|
||||
"the locked file's inode changed across writes -- a locker that "
|
||||
"opened it before this write and one that opens it after would "
|
||||
"no longer contend for the same lock")
|
||||
|
||||
def test_two_concurrent_updates_are_both_kept(self, starlark_dir):
|
||||
import threading
|
||||
import time as _time
|
||||
|
||||
from web_interface.blueprints import api_v3 as module
|
||||
|
||||
def add_app(app_id):
|
||||
with module._starlark_manifest_lock():
|
||||
manifest = module._read_starlark_manifest()
|
||||
# Widen the window between read and write. Without the lock
|
||||
# both threads read here before either writes, and whichever
|
||||
# writes second overwrites the other's addition; with the
|
||||
# lock, the second thread cannot even start its read until
|
||||
# the first has written and released.
|
||||
_time.sleep(0.05)
|
||||
manifest.setdefault('apps', {})[app_id] = {'enabled': True}
|
||||
module._write_starlark_manifest(manifest)
|
||||
|
||||
threads = [threading.Thread(target=add_app, args=(app_id,))
|
||||
for app_id in ('a', 'b')]
|
||||
for t in threads:
|
||||
t.start()
|
||||
for t in threads:
|
||||
t.join(timeout=5)
|
||||
|
||||
manifest = module._read_starlark_manifest()
|
||||
assert set(manifest['apps']) == {'a', 'b'}, (
|
||||
"a concurrent update was lost: %r" % (manifest,))
|
||||
|
||||
def test_the_lock_is_reentrant_safe_across_sequential_calls(self, starlark_dir):
|
||||
"""Not reentrant within one thread -- just that using it twice in a
|
||||
row (the ordinary case: one request, then the next) works cleanly
|
||||
and does not leak the lock file descriptor or leave it locked."""
|
||||
from web_interface.blueprints import api_v3 as module
|
||||
|
||||
for app_id in ('first', 'second'):
|
||||
with module._starlark_manifest_lock():
|
||||
manifest = module._read_starlark_manifest()
|
||||
manifest.setdefault('apps', {})[app_id] = {'enabled': True}
|
||||
module._write_starlark_manifest(manifest)
|
||||
|
||||
manifest = module._read_starlark_manifest()
|
||||
assert set(manifest['apps']) == {'first', 'second'}
|
||||
|
||||
|
||||
class TestConfigAndManifestStayInSync:
|
||||
"""Standalone-mode PUT /starlark/apps/<id>/config (no plugin instance
|
||||
loaded) writes config.json and then the manifest. If the manifest write
|
||||
fails after config.json was already written, the two disagree about
|
||||
what was saved unless config.json is rolled back.
|
||||
"""
|
||||
|
||||
@pytest.fixture
|
||||
def app_dir(self, tmp_path, monkeypatch):
|
||||
from web_interface.blueprints import api_v3 as module
|
||||
apps_dir = tmp_path / "starlark-apps"
|
||||
apps_dir.mkdir()
|
||||
monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir)
|
||||
monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json')
|
||||
one_app_dir = apps_dir / 'demo'
|
||||
one_app_dir.mkdir()
|
||||
module._write_starlark_manifest({'apps': {'demo': {'name': 'Demo', 'enabled': True}}})
|
||||
return one_app_dir
|
||||
|
||||
def test_manifest_write_failure_rolls_back_an_existing_config_json(self, client, app_dir):
|
||||
config_file = app_dir / 'config.json'
|
||||
config_file.write_text(json.dumps({'existing': 'value'}))
|
||||
|
||||
with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
|
||||
patch('web_interface.blueprints.api_v3._write_starlark_manifest', return_value=False):
|
||||
resp = client.put('/api/v3/starlark/apps/demo/config',
|
||||
json={'new_field': 'x'})
|
||||
|
||||
assert resp.status_code == 500
|
||||
assert json.loads(config_file.read_text()) == {'existing': 'value'}, (
|
||||
"config.json kept the new value even though the manifest write "
|
||||
"that was supposed to follow it failed")
|
||||
|
||||
def test_manifest_write_failure_removes_a_freshly_created_config_json(self, client, app_dir):
|
||||
config_file = app_dir / 'config.json'
|
||||
assert not config_file.exists()
|
||||
|
||||
with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
|
||||
patch('web_interface.blueprints.api_v3._write_starlark_manifest', return_value=False):
|
||||
resp = client.put('/api/v3/starlark/apps/demo/config',
|
||||
json={'new_field': 'x'})
|
||||
|
||||
assert resp.status_code == 500
|
||||
assert not config_file.exists(), (
|
||||
"config.json was left behind even though the manifest write "
|
||||
"that was supposed to follow it failed")
|
||||
|
||||
def test_success_updates_both_config_and_manifest(self, client, app_dir):
|
||||
from web_interface.blueprints import api_v3 as module
|
||||
|
||||
with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None):
|
||||
resp = client.put('/api/v3/starlark/apps/demo/config',
|
||||
json={'new_field': 'x'})
|
||||
|
||||
assert resp.status_code == 200, resp.get_json()
|
||||
assert json.loads((app_dir / 'config.json').read_text())['new_field'] == 'x'
|
||||
manifest = json.loads(module._STARLARK_MANIFEST_FILE.read_text())
|
||||
assert manifest['apps']['demo']['config']['new_field'] == 'x'
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The store loaded, then stopped loading, and nothing anywhere said why.
|
||||
#
|
||||
|
||||
@@ -19,7 +19,12 @@ import re
|
||||
from pathlib import Path
|
||||
|
||||
PROJECT_ROOT = Path(__file__).resolve().parents[2]
|
||||
API_V3 = PROJECT_ROOT / "web_interface" / "blueprints" / "api_v3.py"
|
||||
# api_v3 is a package; a sudo systemctl call can live in any of its modules.
|
||||
API_V3_PKG = PROJECT_ROOT / "web_interface" / "blueprints" / "api_v3"
|
||||
|
||||
|
||||
def _api_v3_source() -> str:
|
||||
return "\n".join(p.read_text() for p in sorted(API_V3_PKG.glob("*.py")))
|
||||
SUDOERS_SCRIPT = PROJECT_ROOT / "scripts" / "install" / "configure_web_sudo.sh"
|
||||
|
||||
|
||||
@@ -51,7 +56,7 @@ def _granted_systemctl_rules(script: str) -> set[tuple[str, str]]:
|
||||
|
||||
|
||||
def test_every_sudo_systemctl_call_is_granted() -> None:
|
||||
calls = _sudo_systemctl_calls(API_V3.read_text())
|
||||
calls = _sudo_systemctl_calls(_api_v3_source())
|
||||
rules = _granted_systemctl_rules(SUDOERS_SCRIPT.read_text())
|
||||
|
||||
assert calls, "expected to find sudo systemctl calls in api_v3.py"
|
||||
@@ -68,7 +73,7 @@ def test_every_sudo_systemctl_call_is_granted() -> None:
|
||||
def test_units_are_fully_qualified() -> None:
|
||||
"""Privileged systemctl calls must name the unit as <name>.service so they
|
||||
match the sudoers grants, which use the fully-qualified unit name."""
|
||||
calls = _sudo_systemctl_calls(API_V3.read_text())
|
||||
calls = _sudo_systemctl_calls(_api_v3_source())
|
||||
unqualified = {(v, u) for v, u in calls if not u.endswith(".service")}
|
||||
assert not unqualified, (
|
||||
"sudo systemctl calls must use fully-qualified .service unit names: "
|
||||
|
||||
Reference in New Issue
Block a user