fix(web): say when a system action failed for want of passwordless sudo (#560)

* 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>

test/test_system_action_sudo_diagnosis.py

@@ -0,0 +1,131 @@
+"""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', '')

web_interface/blueprints/api_v3.py

@@ -1979,6 +1979,33 @@ def check_for_update():
         return jsonify(_update_check_failed(
             "Could not check for updates; see logs for details."))
 
+#: sudo's own wording when it needs a password it cannot ask for. The web
+#: interface runs unprivileged, so its systemctl/reboot/journalctl calls only
+#: work once scripts/install/configure_web_sudo.sh has granted NOPASSWD --
+#: which first_time_install.sh does not do. That makes this the common case on
+#: a fresh device, and "Action failed; see logs for details" named none of it,
+#: while the log viewer was broken for the very same reason.
+_SUDO_NEEDS_PASSWORD = (
+    'a password is required',
+    'no tty present',
+    'a terminal is required',
+)
+
+_SUDO_HINT = (
+    'Passwordless sudo is not configured for the web interface user, so this '
+    'action cannot run. Run scripts/install/configure_web_sudo.sh as that user, '
+    'then retry.'
+)
+
+
+def _sudo_hint_for(text):
+    """An actionable hint when `text` is sudo refusing to prompt, else None."""
+    lowered = (text or '').lower()
+    if any(marker in lowered for marker in _SUDO_NEEDS_PASSWORD):
+        return _SUDO_HINT
+    return None
+
+
 @api_v3.route('/system/action', methods=['POST'])
 def execute_system_action():
     """Execute system actions (start/stop/reboot/etc)"""
@@ -2014,7 +2041,14 @@ def execute_system_action():
                     logger.error("start_display (%s) stderr: %s", mode, result.stderr.strip())
                 resp = {
                     'status': 'success' if result.returncode == 0 else 'error',
-                    'message': 'Display started' if result.returncode == 0 else 'Failed to start display',
+                    # This branch returns before the shared nonzero-result
+                    # response below, so it needs the hint of its own or an
+                    # on-demand start reports "Failed to start display" and
+                    # says nothing about the sudo that actually refused it.
+                    'message': (
+                        'Display started' if result.returncode == 0
+                        else _sudo_hint_for(result.stderr) or 'Failed to start display'
+                    ),
                 }
                 if result.returncode != 0:
                     resp['returncode'] = result.returncode
@@ -2346,6 +2380,9 @@ def execute_system_action():
         if result.returncode != 0:
             resp['returncode'] = result.returncode
             resp['stderr'] = result.stderr.strip()
+            hint = _sudo_hint_for(result.stderr)
+            if hint:
+                resp['message'] = hint
         return jsonify(resp)
 
     except subprocess.TimeoutExpired as e:
@@ -2353,7 +2390,13 @@ def execute_system_action():
         return jsonify({'status': 'error', 'message': 'Command timed out', 'returncode': -1, 'stderr': 'timeout'})
     except Exception as e:
         logger.error("execute_system_action failed: %s", e, exc_info=True)
-        return jsonify({'status': 'error', 'message': 'Action failed; see logs for details'}), 500
+        detail = describe_exception(e)
+        resp = {
+            'status': 'error',
+            'message': _sudo_hint_for(detail) or 'Action failed; see logs for details',
+            'details': detail,
+        }
+        return jsonify(resp), 500
 
 @api_v3.route('/system/git-info', methods=['GET'])
 def get_git_info():