mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 14:25:08 +00:00
* fix(web): say when a system action failed for want of passwordless sudo
POSTing reboot_system to a Pi returns, in full:
{"message": "Action failed; see logs for details", "status": "error"}
The cause is that the web interface runs unprivileged, and its
systemctl/reboot/journalctl calls only work once
scripts/install/configure_web_sudo.sh has granted NOPASSWD. first_time_install.sh
never invokes that script and no user-facing doc mentions it, so on a fresh
device every privileged action fails -- start_display, stop_display, the
autostart toggles, reboot, and the log viewer.
That last one closes the loop: "see logs for details" is unreachable advice
when journalctl is refused for the same reason. This is exactly the failure
src/web_interface/error_handler.py's describe_exception() was written to break,
and /system/action's exception handler was still discarding the cause instead
of using the helper the module already imports.
Two changes, no behaviour change when things work:
- The exception path now returns 'details': describe_exception(e), matching how
the other handlers in this blueprint already report.
- A failure whose stderr or exception text is sudo refusing to prompt ("a
password is required", "no tty present", "a terminal is required") reports
what to do about it, naming configure_web_sudo.sh. Unrelated failures keep
the generic message and their stderr, so a missing unit is not blamed on
sudo.
Granting the sudo rights is left alone deliberately: auto-running a script that
hands out NOPASSWD is a security decision for the maintainer, not something to
slip into an installer. Making the refusal legible is the part that is
unambiguously an improvement.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web): apply the sudo hint on the on-demand start_display path too
start_display with a mode builds its own response and returns before the shared
nonzero-result path, so a recognized sudo refusal there reported only "Failed to
start display" and said nothing about the passwordless sudo that refused it --
the exact gap the rest of this PR closes everywhere else.
Raised by CodeRabbit on #560 and verified against the code before fixing: the
branch at api_v3.py:2058 does return early past the shared handler.
Three regression cases: the on-demand branch reports the sudo cause, keeps its
"Display started" message on success, and does not blame an unrelated failure on
sudo.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
132 lines
5.5 KiB
Python
132 lines
5.5 KiB
Python
"""A privileged system action that cannot run must say why.
|
|
|
|
The web interface runs unprivileged. Its systemctl/reboot/journalctl calls only
|
|
work once scripts/install/configure_web_sudo.sh has granted NOPASSWD, and
|
|
first_time_install.sh never invokes that script -- so on a fresh device every
|
|
one of those actions fails. What the user got back was:
|
|
|
|
{"message": "Action failed; see logs for details", "status": "error"}
|
|
|
|
...and the log viewer was broken for exactly the same reason, so "see logs" led
|
|
nowhere. This is the closed loop that src/web_interface/error_handler.py's
|
|
describe_exception() was written to break; /system/action's exception handler
|
|
was simply still discarding the cause.
|
|
|
|
Observed live: POSTing reboot_system to a Pi returned that bare sentence, and
|
|
pinning down "no passwordless sudo" took reading the installer instead of
|
|
reading the response.
|
|
"""
|
|
|
|
import subprocess
|
|
import sys
|
|
from pathlib import Path
|
|
from unittest.mock import patch
|
|
|
|
import pytest
|
|
from flask import Flask
|
|
|
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
|
|
|
from web_interface.blueprints.api_v3 import api_v3 # noqa: E402
|
|
|
|
|
|
@pytest.fixture
|
|
def client():
|
|
app = Flask(__name__)
|
|
app.config['TESTING'] = True
|
|
app.register_blueprint(api_v3, url_prefix='/api/v3')
|
|
return app.test_client()
|
|
|
|
|
|
def _act(client, action='stop_display'):
|
|
return client.post('/api/v3/system/action', json={'action': action})
|
|
|
|
|
|
def _result(args, rc=0, stderr=''):
|
|
return subprocess.CompletedProcess(args, rc, stdout='', stderr=stderr)
|
|
|
|
|
|
class TestSudoRefusalIsNamed:
|
|
@pytest.mark.parametrize("stderr", [
|
|
"sudo: a password is required",
|
|
"sudo: no tty present and no askpass program specified",
|
|
"sudo: a terminal is required to read the password",
|
|
])
|
|
def test_every_sudo_refusal_wording_is_recognised(self, client, stderr):
|
|
with patch('subprocess.run', side_effect=lambda a, **k: _result(a, 1, stderr)):
|
|
body = _act(client).get_json()
|
|
|
|
assert body['status'] == 'error'
|
|
# The point: the response tells the user what to do about it.
|
|
assert 'configure_web_sudo.sh' in body['message']
|
|
|
|
def test_the_raw_stderr_is_still_returned(self, client):
|
|
with patch('subprocess.run',
|
|
side_effect=lambda a, **k: _result(a, 1, "sudo: a password is required")):
|
|
body = _act(client).get_json()
|
|
assert 'a password is required' in body['stderr']
|
|
|
|
|
|
class TestUnrelatedFailuresAreNotMisattributed:
|
|
def test_a_real_systemctl_error_keeps_the_generic_message(self, client):
|
|
with patch('subprocess.run',
|
|
side_effect=lambda a, **k: _result(a, 5, "Unit ledmatrix.service not found.")):
|
|
body = _act(client).get_json()
|
|
|
|
assert 'configure_web_sudo.sh' not in body['message'], \
|
|
"a missing unit was blamed on sudo"
|
|
assert 'Unit ledmatrix.service not found.' in body['stderr']
|
|
assert body['returncode'] == 5
|
|
|
|
|
|
class TestTheExceptionPathNamesTheCause:
|
|
def test_the_exception_type_and_message_are_returned(self, client):
|
|
with patch('subprocess.run', side_effect=FileNotFoundError(2, "sudo")):
|
|
body = _act(client).get_json()
|
|
|
|
# The regression: 'details' was absent and the cause was discarded.
|
|
assert 'FileNotFoundError' in body['details']
|
|
|
|
def test_a_sudo_shaped_exception_also_gets_the_hint(self, client):
|
|
with patch('subprocess.run',
|
|
side_effect=PermissionError("sudo: a password is required")):
|
|
body = _act(client).get_json()
|
|
assert 'configure_web_sudo.sh' in body['message']
|
|
|
|
|
|
class TestTheEarlyReturnPathsAlsoGetTheHint:
|
|
"""start_display with a mode builds its own response and returns before the
|
|
shared nonzero-result path, so it needed the hint separately. Found by
|
|
CodeRabbit on #560; verified against the code before fixing."""
|
|
|
|
def test_on_demand_start_reports_the_sudo_cause(self, client):
|
|
with patch('subprocess.run',
|
|
side_effect=lambda a, **k: _result(a, 1, "sudo: a password is required")):
|
|
body = client.post('/api/v3/system/action',
|
|
json={'action': 'start_display', 'mode': 'nfl_live'}).get_json()
|
|
|
|
assert 'configure_web_sudo.sh' in body['message'], "the on-demand branch still reported only 'Failed to start display'"
|
|
assert body['status'] == 'error'
|
|
|
|
def test_on_demand_start_keeps_its_success_message(self, client):
|
|
with patch('subprocess.run', side_effect=lambda a, **k: _result(a, 0)):
|
|
body = client.post('/api/v3/system/action',
|
|
json={'action': 'start_display', 'mode': 'nfl_live'}).get_json()
|
|
assert body['status'] == 'success'
|
|
assert body['message'] == 'Display started'
|
|
|
|
def test_an_unrelated_on_demand_failure_is_not_blamed_on_sudo(self, client):
|
|
with patch('subprocess.run',
|
|
side_effect=lambda a, **k: _result(a, 5, "Unit not found.")):
|
|
body = client.post('/api/v3/system/action',
|
|
json={'action': 'start_display', 'mode': 'nfl_live'}).get_json()
|
|
assert body['message'] == 'Failed to start display'
|
|
|
|
|
|
class TestSuccessIsUnchanged:
|
|
def test_a_working_action_still_reports_success(self, client):
|
|
with patch('subprocess.run', side_effect=lambda a, **k: _result(a, 0)):
|
|
body = _act(client).get_json()
|
|
assert body['status'] == 'success'
|
|
assert 'configure_web_sudo.sh' not in body.get('message', '')
|