diff --git a/CHANGELOG.md b/CHANGELOG.md index a9e7a23b..68ee3d9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -722,6 +722,45 @@ policies are unchanged. screen showed first and the game came after it. Each check also asks each plugin `has_live_content()` once, where a plugin registered under several modes used to be asked once per mode. +- A plugin action whose params hold `true`, `false` or `null` runs again. + `/api/v3/plugins/action` wrote the params into the source of the wrapper + that runs the plugin's script, and those JSON words are not Python, so the + wrapper stopped with a NameError and the action answered "Action failed". + The plugin file manager's category toggle sends `"enabled": true`, so + turning a category on or off in of-the-day always failed. The params now + reach the wrapper on its stdin; the script still receives them as JSON on + its own stdin, as before. +- An on-demand request that `/api/v3/display/on-demand/start` refuses no + longer runs later. With the display stopped the request goes to the + display's mailbox, and the display reads that mailbox for an hour without + looking at a request's age. So with "Start display service" unticked, the + answer was "Display service is not running", yet the next time the + display was started it ran that plugin, pinned if the request said so. + The same happened after "Failed to start display service". On either + refusal the route now takes its request back out of the mailbox, unless a + newer one has replaced it. A request the display acknowledges over the + control socket is now a success whatever systemd reports: a display run + by hand or in the emulator was told "not running" for a request it had + already taken, and with "Start display service" ticked the route tried to + start the service beside it. +- `/api/v3/plugins/operation/` reports a queued operation as `pending` + instead of answering 500. The queue keeps an operation's callback among + its parameters until it runs, and the status route tried to send that + function as JSON. An install queued behind another plugin's install + failed every status poll until the first one finished. Parameters whose + name starts with `_` are internal and are no longer in the answer. +- A second click on Install while that plugin is still installing, or an + Uninstall during its install, now answers 409 "already has an install, + update or uninstall in progress" instead of 500 "An error occurred". The + first operation carried on either way. The uninstall route also stopped + recording a failed uninstall in the operation history for an uninstall + that never started. +- `/api/v3/plugins//static/` serves images and other + binary files. It opened every file as UTF-8 text, so a plugin's icon or + preview image answered 500 `UnicodeDecodeError`. Files are now sent as + they are on disk, an image with its own content type; HTML, JavaScript, + CSS, JSON and other text keep the types they had. The path checks are + unchanged. - The display schedule turns the panel off at exactly the end time. A window now runs from its start time up to, but not including, its end time: with 07:00-23:00 the panel is on at 07:00 and off at 23:00. Before, the end diff --git a/src/plugin_system/operation_types.py b/src/plugin_system/operation_types.py index 7181263b..950b0fd3 100644 --- a/src/plugin_system/operation_types.py +++ b/src/plugin_system/operation_types.py @@ -48,12 +48,19 @@ class PluginOperation: completed_at: Optional[datetime] = None def to_dict(self) -> Dict[str, Any]: - """Convert operation to dictionary for serialization.""" + """Convert operation to dictionary for serialization. + + Parameters whose name starts with ``_`` are internal and left out: + PluginOperationQueue keeps the operation's callback there as + ``_callback`` until its worker runs it, and a pending operation's + status answered 500 because that function cannot be serialized. + """ return { 'operation_id': self.operation_id, 'operation_type': self.operation_type.value, 'plugin_id': self.plugin_id, - 'parameters': self.parameters, + 'parameters': {key: value for key, value in self.parameters.items() + if not str(key).startswith('_')}, 'status': self.status.value, 'progress': self.progress, 'message': self.message, diff --git a/test/test_api_v3_on_demand_restart.py b/test/test_api_v3_on_demand_restart.py index 17d31edd..9b889f83 100644 --- a/test/test_api_v3_on_demand_restart.py +++ b/test/test_api_v3_on_demand_restart.py @@ -37,6 +37,7 @@ from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401 START_URL = "/api/v3/display/on-demand/start" STOP_URL = "/api/v3/display/on-demand/stop" MAILBOX = "display_on_demand_request" +DISPLAY = "web_interface.blueprints.api_v3.display" @pytest.fixture @@ -150,6 +151,102 @@ class TestStartWhileTheServiceIsStopped: assert response.get_json()["status"] == "error" +class _Mailbox: + """The CacheManager calls the routes make, over a dict.""" + + def __init__(self): + self.entries = {} + + def set(self, key, value, ttl=None): + self.entries[key] = value + + def get(self, key, max_age=300, memory_ttl=None): + return self.entries.get(key) + + def delete(self, key): + self.entries.pop(key, None) + + +class TestARefusedStartLeavesNoRequestBehind: + """A start the route answers with an error must not run later. + + The request was posted (to the mailbox, with the display stopped) before + the route refused it, and the display reads the mailbox for an hour + without looking at a request's age. So "Display service is not running" + (start_service off) or "Failed to start display service" left the + request waiting, and the next time the display started -- minutes later, + by hand -- it ran that plugin, pinned if the request said so. + + A socket acknowledgement is the other side of it: the display answered, + so it is running and has the request, whatever systemd says (a display + run by hand or in the emulator has no active unit). That is a success, + not "not running", and no unit is started beside it. + """ + + @pytest.fixture + def mailbox(self, api_v3_module, service): + box = _Mailbox() + api_v3_module.api_v3.cache_manager = box + service["state"]["active"] = False + return box + + @pytest.mark.parametrize("body", [ + {"plugin_id": "weather", "start_service": False}, + {"plugin_id": "weather"}, # start_service defaults on + ]) + def test_a_socket_ack_is_a_success_whatever_systemd_says( + self, api_v3_client, service, mailbox, body): + with patch(f"{DISPLAY}.control_client.on_demand_start", + side_effect=lambda request_id, *a: {"accepted": True}): + response = api_v3_client.post(START_URL, json=body) + assert response.status_code == 200, response.get_json() + assert response.get_json()["data"]["transport"] == "socket" + assert MAILBOX not in mailbox.entries + assert _systemctl_verbs(service["systemctl"]) == [], ( + "a unit was started beside a display that answered the socket") + + def test_without_start_service_the_request_is_taken_back( + self, api_v3_client, service, mailbox): + response = api_v3_client.post(START_URL, json={ + "plugin_id": "weather", "pinned": True, "start_service": False}) + assert response.status_code == 400 + assert response.get_json()["status"] == "error" + assert MAILBOX not in mailbox.entries + + def test_a_start_that_fails_takes_its_request_back(self, api_v3_client, service, mailbox): + service["systemctl"].side_effect = lambda args: { + "returncode": 1, "stdout": "", "stderr": "denied"} + response = api_v3_client.post(START_URL, json={"plugin_id": "weather"}) + assert response.status_code == 500 + assert MAILBOX not in mailbox.entries + + def test_a_newer_request_is_left_alone_on_the_400(self, api_v3_client, service, mailbox): + newer = {"request_id": "someone-else", "action": "start", "plugin_id": "clock"} + + def stopped_and_another_post_lands(*args): + mailbox.entries[MAILBOX] = newer + return {"active": False} + + with patch(f"{DISPLAY}._get_display_service_status", + side_effect=stopped_and_another_post_lands): + response = api_v3_client.post(START_URL, json={ + "plugin_id": "weather", "start_service": False}) + assert response.status_code == 400 + assert mailbox.entries[MAILBOX] is newer + + def test_a_newer_request_is_left_alone_on_the_500(self, api_v3_client, service, mailbox): + newer = {"request_id": "someone-else", "action": "start", "plugin_id": "clock"} + + def start_fails_after_another_post(args): + mailbox.entries[MAILBOX] = newer + return {"returncode": 1, "stdout": "", "stderr": "denied"} + + service["systemctl"].side_effect = start_fails_after_another_post + response = api_v3_client.post(START_URL, json={"plugin_id": "weather"}) + assert response.status_code == 500 + assert mailbox.entries[MAILBOX] is newer + + class TestStop: def test_stop_posts_a_stop_request_and_leaves_the_service_running( self, api_v3_client, service): diff --git a/test/test_api_v3_operation_status_pending.py b/test/test_api_v3_operation_status_pending.py new file mode 100644 index 00000000..d107b566 --- /dev/null +++ b/test/test_api_v3_operation_status_pending.py @@ -0,0 +1,83 @@ +"""GET /api/v3/plugins/operation/ answers for an operation still waiting. + +PluginOperationQueue keeps an operation's callback in its parameters, under +``_callback``, until the worker takes it to run. PluginOperation.to_dict() +returned the parameters as they were, so for a pending operation the route +handed jsonify a function and answered 500 "A system error occurred". That +is every poll of an install queued behind another plugin's: the second of +two installs read as broken until the first one finished. +""" + +import json +import sys +import threading +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + +from src.plugin_system.operation_queue import PluginOperationQueue # noqa: E402 +from src.plugin_system.operation_types import ( # noqa: E402 + OperationType, PluginOperation, +) + + +def _callback(op): + return {"success": True, "message": "done"} + + +class TestToDict: + def test_private_parameters_are_left_out(self): + op = PluginOperation(OperationType.INSTALL, "demo", + parameters={"_callback": _callback, "branch": "main"}) + assert op.to_dict()["parameters"] == {"branch": "main"} + json.dumps(op.to_dict()) # serializable + + def test_the_operation_keeps_its_callback_for_the_worker(self): + op = PluginOperation(OperationType.INSTALL, "demo", + parameters={"_callback": _callback}) + op.to_dict() + assert op.parameters["_callback"] is _callback + + def test_the_other_fields_are_unchanged(self): + op = PluginOperation(OperationType.UNINSTALL, "demo", operation_id="op-1") + assert op.to_dict() == { + "operation_id": "op-1", "operation_type": "uninstall", "plugin_id": "demo", + "parameters": {}, "status": "pending", "progress": 0.0, "message": "", + "error": None, "result": None, + "created_at": op.created_at.isoformat(), "started_at": None, + "completed_at": None, + } + + +class TestTheRoute: + @pytest.fixture + def busy_queue(self, api_v3_module): + """A real queue whose worker is held by another plugin's operation.""" + queue = PluginOperationQueue(max_history=10) + api_v3_module.api_v3.operation_queue = queue + started, release = threading.Event(), threading.Event() + + def blocker(op): + started.set() + release.wait(10) + return {"success": True, "message": "done"} + + queue.enqueue_operation(OperationType.INSTALL, "busy", operation_callback=blocker) + assert started.wait(5) + yield queue + release.set() + queue.shutdown() + + def test_a_pending_operation_reports_pending(self, api_v3_client, busy_queue): + op_id = busy_queue.enqueue_operation( + OperationType.INSTALL, "demo", operation_callback=_callback) + response = api_v3_client.get(f"/api/v3/plugins/operation/{op_id}") + assert response.status_code == 200, response.get_json() + data = response.get_json()["data"] + assert data["status"] == "pending" + assert data["plugin_id"] == "demo" + assert "_callback" not in data["parameters"] diff --git a/test/test_api_v3_plugin_action_params.py b/test/test_api_v3_plugin_action_params.py new file mode 100644 index 00000000..a890290a --- /dev/null +++ b/test/test_api_v3_plugin_action_params.py @@ -0,0 +1,93 @@ +"""POST /api/v3/plugins/action hands ``params`` to the plugin's script intact. + +The route runs the script through a generated wrapper, and the params went +into that wrapper as Python source: ``params = {json.dumps(params)}``. JSON is +not Python. ``true``, ``false`` and ``null`` are undefined names there, so any +params holding a boolean or a null died with a NameError before the script +ran. The plugin file manager's category toggle sends ``{"category_name": ..., +"enabled": true}``, so of-the-day's category toggle failed every time with +"Action failed". + +The script's side of the contract is unchanged and pinned here too: the +params arrive on stdin as one JSON document, LEDMATRIX_ROOT is set, and what +the script prints to stdout is what the route parses. +""" + +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + +ACTION_URL = "/api/v3/plugins/action" + +# The action script: report what it was handed, as JSON on stdout. +ECHO_SCRIPT = ( + "import json, os, sys\n" + "raw = sys.stdin.read()\n" + "print(json.dumps({'status': 'success', 'got': json.loads(raw),\n" + " 'root': os.environ.get('LEDMATRIX_ROOT')}))\n" +) + + +@pytest.fixture +def echo_plugin(tmp_path, api_v3_module, monkeypatch): + plugin_dir = tmp_path / "demo" + plugin_dir.mkdir() + (plugin_dir / "manifest.json").write_text(json.dumps({ + "id": "demo", + "web_ui_actions": [{"id": "toggle", "type": "script", "script": "echo.py"}], + }), encoding="utf-8") + (plugin_dir / "echo.py").write_text(ECHO_SCRIPT, encoding="utf-8") + api_v3_module.api_v3.plugin_catalog.get_plugin_directory.return_value = str(plugin_dir) + + # The route runs `python3`; use this interpreter, so the test does not + # depend on what that name resolves to here. + real_run = subprocess.run + + def run(cmd, *args, **kwargs): + if isinstance(cmd, list) and cmd and cmd[0] == "python3": + cmd = [sys.executable] + cmd[1:] + return real_run(cmd, *args, **kwargs) + + monkeypatch.setattr(subprocess, "run", run) + return plugin_dir + + +@pytest.mark.parametrize("params", [ + {"category_name": "jokes", "enabled": True}, # the file manager's toggle + {"category_name": "jokes", "enabled": False}, + {"filename": None}, + {"nested": {"list": [1, None, True, 2.5], "empty": {}}}, + {"text": "café ✓ \U0001F600"}, + {"text": "he said \"hi\" and 'bye' \\ ''' \"\"\" \n\t end"}, +], ids=["true", "false", "null", "nested", "unicode", "quotes"]) +def test_the_script_receives_the_params_it_was_sent(api_v3_client, echo_plugin, params): + response = api_v3_client.post(ACTION_URL, json={ + "plugin_id": "demo", "action_id": "toggle", "params": params}) + body = response.get_json() + assert response.status_code == 200, body + assert body["got"] == params + + +def test_a_param_cannot_run_code_in_the_wrapper(api_v3_client, echo_plugin, tmp_path): + marker = tmp_path / "PWNED" + hostile = "\"}\nopen(%r, 'w').write('ran')\n#" % str(marker) + params = {"name": hostile, "flag": True} + response = api_v3_client.post(ACTION_URL, json={ + "plugin_id": "demo", "action_id": "toggle", "params": params}) + assert response.status_code == 200, response.get_json() + assert response.get_json()["got"] == params + assert not marker.exists(), "a param value ran as code" + + +def test_the_script_still_gets_ledmatrix_root(api_v3_client, echo_plugin, api_v3_module): + response = api_v3_client.post(ACTION_URL, json={ + "plugin_id": "demo", "action_id": "toggle", "params": {"enabled": True}}) + assert response.status_code == 200, response.get_json() + assert response.get_json()["root"] == str(api_v3_module.PROJECT_ROOT) diff --git a/test/test_api_v3_plugin_operation_conflict.py b/test/test_api_v3_plugin_operation_conflict.py new file mode 100644 index 00000000..eacd622c --- /dev/null +++ b/test/test_api_v3_plugin_operation_conflict.py @@ -0,0 +1,89 @@ +"""A second install or uninstall while one is in progress is a 409, not a 500. + +PluginOperationQueue refuses a second operation for a plugin that already +has one waiting or running (test_operation_queue_pending_and_trim.py), and +says so by raising ValueError. /plugins/install let that escape to the +blueprint's catch-all, so a double-clicked Install answered 500 "An error +occurred; see logs for details" while the first install carried on. +/plugins/uninstall caught it in its own catch-all: a 500 "Failed to +uninstall plugin", and an "uninstall failed" entry in the operation +history for an uninstall that never started. +""" + +import sys +import threading +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + +from src.plugin_system.operation_queue import PluginOperationQueue # noqa: E402 + +INSTALL = "/api/v3/plugins/install" +UNINSTALL = "/api/v3/plugins/uninstall" + + +@pytest.fixture +def installing(api_v3_module, tmp_path): + """A real queue with an install of "clock" running and held there.""" + queue = PluginOperationQueue(max_history=10) + api_v3_module.api_v3.operation_queue = queue + started, release = threading.Event(), threading.Event() + + def slow_install(plugin_id, branch=None): + started.set() + release.wait(10) + return True + + store = api_v3_module.api_v3.plugin_store_manager + store.install_plugin.side_effect = slow_install + store.get_registry_info.return_value = None + store.plugins_dir = str(tmp_path) + api_v3_module.api_v3.plugin_catalog.get_plugin_directory.return_value = None + yield {"queue": queue, "started": started, "store": store} + release.set() + queue.shutdown() + + +def _start_first_install(client, installing): + response = client.post(INSTALL, json={"plugin_id": "clock"}) + assert response.status_code == 200, response.get_json() + assert installing["started"].wait(5) + + +def _failed_history(api_v3_module): + return [c for c in api_v3_module.api_v3.operation_history.record_operation.call_args_list + if c.kwargs.get("status") == "failed"] + + +def test_a_second_install_click_is_a_conflict(api_v3_client, api_v3_module, installing): + _start_first_install(api_v3_client, installing) + response = api_v3_client.post(INSTALL, json={"plugin_id": "clock"}) + assert response.status_code == 409, response.get_json() + body = response.get_json() + assert body["status"] == "error" + assert body["error_code"] == "PLUGIN_OPERATION_CONFLICT" + assert "clock" in body["message"] + assert installing["store"].install_plugin.call_count == 1 + assert _failed_history(api_v3_module) == [] + + +def test_an_uninstall_during_the_install_is_a_conflict(api_v3_client, api_v3_module, + installing): + _start_first_install(api_v3_client, installing) + response = api_v3_client.post(UNINSTALL, json={"plugin_id": "clock"}) + assert response.status_code == 409, response.get_json() + assert response.get_json()["error_code"] == "PLUGIN_OPERATION_CONFLICT" + assert _failed_history(api_v3_module) == [], ( + "an uninstall that never started was recorded as failed") + api_v3_module.api_v3.plugin_store_manager.uninstall_plugin.assert_not_called() + + +def test_another_plugin_is_still_queued(api_v3_client, installing): + _start_first_install(api_v3_client, installing) + response = api_v3_client.post(INSTALL, json={"plugin_id": "weather"}) + assert response.status_code == 200, response.get_json() + assert response.get_json()["data"]["operation_id"] diff --git a/test/test_api_v3_plugin_static_files.py b/test/test_api_v3_plugin_static_files.py new file mode 100644 index 00000000..c2bfcc45 --- /dev/null +++ b/test/test_api_v3_plugin_static_files.py @@ -0,0 +1,73 @@ +"""GET /api/v3/plugins//static/ serves binary files too. + +The route opened every file as UTF-8 text, so an image -- what the API +reference says it is for, plugin previews and icons -- failed to decode and +answered 500 "UnicodeDecodeError". Files are now sent as bytes. The text +types the route always set are unchanged, and the path checks are pinned in +test_path_traversal_guards.py::TestServePluginStatic. +""" + +import json +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + +PNG = (b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01" + b"\x08\x06\x00\x00\x00\x1f\x15\xc4\x89") + + +@pytest.fixture +def plugin_dir(tmp_path, api_v3_module): + d = tmp_path / "demo" + (d / "web_ui").mkdir(parents=True) + (d / "manifest.json").write_text(json.dumps({"id": "demo"}), encoding="utf-8") + api_v3_module.api_v3.plugin_catalog.get_plugin_directory.side_effect = ( + lambda pid: str(d) if pid == "demo" else None) + return d + + +def _get(client, path): + return client.get(f"/api/v3/plugins/demo/static/{path}") + + +def test_an_image_is_served_as_its_bytes(api_v3_client, plugin_dir): + (plugin_dir / "web_ui" / "icon.png").write_bytes(PNG) + response = _get(api_v3_client, "web_ui/icon.png") + assert response.status_code == 200, response.get_json(silent=True) + assert response.mimetype == "image/png" + assert response.data == PNG + + +def test_an_unknown_binary_file_is_served_too(api_v3_client, plugin_dir): + blob = bytes(range(256)) + (plugin_dir / "data.bin").write_bytes(blob) + response = _get(api_v3_client, "data.bin") + assert response.status_code == 200, response.get_json(silent=True) + assert response.data == blob + + +@pytest.mark.parametrize("name,mimetype", [ + ("page.html", "text/html"), + ("app.js", "application/javascript"), + ("style.css", "text/css"), + ("data.json", "application/json"), + ("notes.txt", "text/plain"), + ("README.md", "text/plain"), + ("helper.py", "text/plain"), +]) +def test_text_files_keep_their_types(api_v3_client, plugin_dir, name, mimetype): + content = "caf\u00e9 \u2713

hi

\n" + (plugin_dir / name).write_bytes(content.encode("utf-8")) + response = _get(api_v3_client, name) + assert response.status_code == 200 + assert response.mimetype == mimetype + assert response.data == content.encode("utf-8") + + +def test_a_missing_file_is_still_a_404(api_v3_client, plugin_dir): + assert _get(api_v3_client, "nope.png").status_code == 404 diff --git a/web_interface/blueprints/api_v3/display.py b/web_interface/blueprints/api_v3/display.py index cbc83946..163fbc21 100644 --- a/web_interface/blueprints/api_v3/display.py +++ b/web_interface/blueprints/api_v3/display.py @@ -69,6 +69,26 @@ def _deliver_on_demand(payload): return 'mailbox', reason +def _withdraw_on_demand(request_id): + """Take a start request the route has refused back out of the mailbox. + + The display reads the mailbox for an hour without looking at a + request's age, so one left there after an error answer ran whenever the + display next started. Only this request is removed: the mailbox is + re-read and cleared only while it still holds this request_id, as the + display's _consume_on_demand_request does, so a newer request posted in + the meantime stays for the display to take. + """ + cache = _cache_manager() + try: + current = cache.get('display_on_demand_request', max_age=3600, memory_ttl=0) + if isinstance(current, dict) and current.get('request_id') == request_id: + cache.delete('display_on_demand_request') + except Exception: # the route is answering an error already + logger.warning("Could not withdraw on-demand request %s from the mailbox", + request_id, exc_info=True) + + @api_v3.route('/display/current', methods=['GET']) def get_display_current(): """The latest display preview, as the /stream/display SSE stream sends it. @@ -259,9 +279,26 @@ def start_on_demand_display(): } transport, socket_error = _deliver_on_demand(request_payload) + # A socket acknowledgement is the display itself answering: it is + # running and has the request queued, whatever systemd says (a display + # run by hand or in the emulator has no active unit). So nothing is + # checked or started for it -- that answered "not running" for a request + # that had already taken effect. The service is still reported the way + # _ensure_display_service_running reports a running one. + if transport == 'socket': + service_result = (dict(_get_display_service_status(), started=False) + if start_service else None) + return _on_demand_started(request_id, resolved_plugin, resolved_mode, + duration, pinned, service_result, transport, + socket_error) + service_status = _get_display_service_status() if not service_status.get('active') and not start_service: + # The request is in the mailbox, and the display reads it whenever + # it next starts: taken back out, or a request answered with this + # error ran later anyway. + _withdraw_on_demand(request_id) return jsonify({ 'status': 'error', 'message': 'Display service is not running. Please start the display service or enable "Start Service" option.', @@ -285,16 +322,25 @@ def start_on_demand_display(): service_result = _ensure_display_service_running() # Check if service actually started if service_result and not service_result.get('active'): + _withdraw_on_demand(request_id) return jsonify({ 'status': 'error', 'message': 'Failed to start display service. Please check service logs or start it manually.', 'service_result': service_result }), 500 + return _on_demand_started(request_id, resolved_plugin, resolved_mode, + duration, pinned, service_result, transport, + socket_error) + + +def _on_demand_started(request_id, plugin_id, mode, duration, pinned, + service_result, transport, socket_error): + """The success answer of /display/on-demand/start.""" response_data = { 'request_id': request_id, - 'plugin_id': resolved_plugin, - 'mode': resolved_mode, + 'plugin_id': plugin_id, + 'mode': mode, 'duration': duration, 'pinned': pinned, 'service': service_result, diff --git a/web_interface/blueprints/api_v3/plugin_assets.py b/web_interface/blueprints/api_v3/plugin_assets.py index 750f76fc..9fea89fc 100644 --- a/web_interface/blueprints/api_v3/plugin_assets.py +++ b/web_interface/blueprints/api_v3/plugin_assets.py @@ -3,8 +3,12 @@ Routes decorate the shared `api_v3` Blueprint from the package `__init__`, so their endpoint names do not depend on which module they live in. """ +import mimetypes + +from flask import send_file + from web_interface.blueprints.api_v3 import ( - PROJECT_ROOT, Response, _plugin_directory, api_v3, datetime, hashlib, + PROJECT_ROOT, _plugin_directory, api_v3, datetime, hashlib, json, jsonify, logger, os, request, uuid, ) from src.common.path_safety import ( @@ -231,8 +235,8 @@ def serve_plugin_static(plugin_id, file_path): if not requested_file.exists() or not requested_file.is_file(): return jsonify({'status': 'error', 'message': 'File not found'}), 404 - # Determine content type - content_type = 'text/plain' + # Determine content type. Text keeps the types this route always set; + # anything else (an icon, a preview image) gets its own. name = requested_file.name if name.endswith('.html'): content_type = 'text/html' @@ -242,12 +246,14 @@ def serve_plugin_static(plugin_id, file_path): content_type = 'text/css' elif name.endswith('.json'): content_type = 'application/json' + else: + guessed = mimetypes.guess_type(name)[0] + content_type = ('text/plain' if not guessed or guessed.startswith('text/') + else guessed) - # Read and return file - with open(requested_file, 'r', encoding='utf-8') as f: - content = f.read() - - return Response(content, mimetype=content_type) + # Sent as bytes. Opening it as UTF-8 text failed to decode any binary + # file, so an image answered 500 UnicodeDecodeError. + return send_file(requested_file, mimetype=content_type) @api_v3.route('/plugins/assets/delete', methods=['POST']) diff --git a/web_interface/blueprints/api_v3/plugin_store.py b/web_interface/blueprints/api_v3/plugin_store.py index e776918d..d0f31feb 100644 --- a/web_interface/blueprints/api_v3/plugin_store.py +++ b/web_interface/blueprints/api_v3/plugin_store.py @@ -81,6 +81,27 @@ def _listed_plugin_dir(base: Path, name: str) -> Optional[Path]: return None +def _enqueue_or_conflict(operation_type, plugin_id, callback): + """``(operation_id, None)``, or ``(None, a 409 response)``. + + The queue raises ValueError when the plugin already has an operation + waiting or running -- a double-clicked Install, an uninstall during an + install. That is the caller's timing, not a server fault: it reached + the client as a 500, and the uninstall route recorded a failed + uninstall that had never started. + """ + try: + return api_v3.operation_queue.enqueue_operation( + operation_type, plugin_id, operation_callback=callback), None + except ValueError: + return None, error_response( + ErrorCode.PLUGIN_OPERATION_CONFLICT, + f'Plugin {plugin_id} already has an install, update or uninstall ' + 'in progress; wait for it to finish, then try again', + status_code=409 + ) + + @api_v3.route('/plugins/update', methods=['POST']) def update_plugin(): """Update plugin""" @@ -402,11 +423,10 @@ def uninstall_plugin(): preserve_config=preserve_config)} # Enqueue operation - operation_id = api_v3.operation_queue.enqueue_operation( - OperationType.UNINSTALL, - plugin_id, - operation_callback=uninstall_callback - ) + operation_id, conflict = _enqueue_or_conflict( + OperationType.UNINSTALL, plugin_id, uninstall_callback) + if conflict: + return conflict return success_response( data={'operation_id': operation_id}, @@ -538,11 +558,10 @@ def install_plugin(): raise Exception(error_msg) # Enqueue operation - operation_id = api_v3.operation_queue.enqueue_operation( - OperationType.INSTALL, - plugin_id, - operation_callback=install_callback - ) + operation_id, conflict = _enqueue_or_conflict( + OperationType.INSTALL, plugin_id, install_callback) + if conflict: + return conflict branch_msg = f" (branch: {branch})" if branch else "" return success_response( diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index ec319e3e..15c3df9d 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -439,6 +439,10 @@ sys.exit(proc.returncode) import tempfile import json as json_lib + # The params reach the wrapper on its stdin, never in + # its source: written there as `params = `, a + # true, false or null was an undefined name and the + # wrapper died with a NameError before the script ran. params_json = json_lib.dumps(action_params) with tempfile.NamedTemporaryFile(mode='w', suffix='.py', delete=False) as wrapper: wrapper.write(f'''import sys @@ -449,6 +453,9 @@ import json # Set LEDMATRIX_ROOT os.environ['LEDMATRIX_ROOT'] = r"{PROJECT_ROOT}" +# The params, as JSON on this wrapper's own stdin +params = json.loads(sys.stdin.read()) + # Run the script and provide params as JSON via stdin proc = subprocess.Popen( [sys.executable, r"{script_file}"], @@ -460,7 +467,6 @@ proc = subprocess.Popen( ) # Send params as JSON to stdin -params = {params_json} stdout, _ = proc.communicate(input=json.dumps(params), timeout=120) print(stdout) sys.exit(proc.returncode) @@ -470,6 +476,7 @@ sys.exit(proc.returncode) try: result = subprocess.run( ['python3', wrapper_path], + input=params_json, capture_output=True, text=True, timeout=120,