Fix PermissionError reading config_secrets.json in web interface (#416)

ledmatrix.service (main display) runs as root while ledmatrix-web.service
runs as the non-root install user (install_web_service.sh). Both
config_manager.py and config_manager_atomic.py only chmod'd
config_secrets.json to 0o640 without ever fixing its group, so a file
written by the root service ended up group-owned by root and unreadable
by the web user, crashing the settings page with a raw PermissionError.

Add ensure_shared_group_ownership() to chgrp secrets/config files (best
effort, root-only) to the project directory's owning group whenever they
are created or saved, and self-heal existing files on load. Also make
get_raw_file_content() tolerate an unreadable secrets file the same way
load_config() already does, degrading to empty secrets instead of a 500.

Co-authored-by: Claude <noreply@anthropic.com>

Author: ChuckBuilds

src/common/permission_utils.py

@@ -146,6 +146,60 @@ def ensure_file_permissions(path: Path, mode: int = 0o644) -> None:
         raise
 
 
+_shared_group_gid_cache: Optional[int] = None
+
+
+def get_shared_group_gid() -> Optional[int]:
+    """
+    Return the gid that should own config/secrets files shared between the
+    root-run ``ledmatrix.service`` (main display) and the non-root user that
+    ``ledmatrix-web.service`` runs as (see install_web_service.sh, which sets
+    ``User=$SUDO_USER``).
+
+    Resolved once from the project root directory's current group (normally
+    the login user's group from the initial ``git clone``), since that user
+    is stable across reinstalls unlike any single file's ownership.
+
+    Returns:
+        The gid, or None if it cannot be determined.
+    """
+    global _shared_group_gid_cache
+    if _shared_group_gid_cache is not None:
+        return _shared_group_gid_cache
+    try:
+        project_root = Path(__file__).resolve().parent.parent.parent
+        _shared_group_gid_cache = project_root.stat().st_gid
+        return _shared_group_gid_cache
+    except OSError:
+        return None
+
+
+def ensure_shared_group_ownership(path: Path) -> None:
+    """
+    Best-effort chgrp of ``path`` to the shared group (see
+    :func:`get_shared_group_gid`) when running as root.
+
+    Only root can change a file's group to one the calling process isn't a
+    member of, which is exactly the case that causes the web interface
+    (running as a non-root user) to get ``PermissionError`` reading files
+    the root-run display service just wrote with a 0o640/2775 mode: the mode
+    is group-readable, but without this the group is root's, not the web
+    user's. Silently does nothing if not running as root or on any error —
+    this is a hardening step, not a required one.
+    """
+    if os.geteuid() != 0:
+        return
+    gid = get_shared_group_gid()
+    if gid is None:
+        return
+    try:
+        if path.exists() and path.stat().st_gid != gid:
+            os.chown(path, -1, gid)
+            logger.debug(f"Set shared group ownership (gid {gid}) on {path}")
+    except OSError as e:
+        logger.debug(f"Could not set shared group ownership on {path}: {e}")
+
+
 def get_config_file_mode(file_path: Path) -> int:
     """
     Return appropriate permission mode for config files.

src/config_manager.py

@@ -38,6 +38,7 @@
 from src.common.permission_utils import (
     ensure_directory_permissions,
     ensure_file_permissions,
+    ensure_shared_group_ownership,
     get_config_file_mode,
     get_config_dir_mode
 )
@@ -234,6 +235,11 @@ def load_config(self) -> Dict[str, Any]:
 
             # Load and merge secrets if they exist (be permissive on errors)
             if os.path.exists(self.secrets_path):
+                # Self-heal stale group ownership (e.g. the root-run display
+                # service wrote this file before the web user was granted
+                # group access) before every load attempt; no-op unless
+                # running as root and the group is already wrong.
+                ensure_shared_group_ownership(Path(self.secrets_path))
                 try:
                     with open(self.secrets_path, 'r') as f:
                         secrets = json.load(f)
@@ -363,6 +369,7 @@ def _create_config_from_template(self) -> None:
         # Set proper file permissions after creation
         config_path_obj = Path(self.config_path)
         ensure_file_permissions(config_path_obj, get_config_file_mode(config_path_obj))
+        ensure_shared_group_ownership(config_path_obj)
 
         self.logger.info(f"Created config.json from template at {os.path.abspath(self.config_path)}")
 
@@ -475,14 +482,30 @@ def get_raw_file_content(self, file_type: str) -> Dict[str, Any]:
             self.logger.error(error_msg)
             raise ConfigError(error_msg, config_path=path_to_load)
 
+        if file_type == "secrets":
+            # Best-effort self-heal: no-op unless running as root and the
+            # group is stale (see load_config for why this can happen).
+            ensure_shared_group_ownership(Path(path_to_load))
+
         try:
             with open(path_to_load, 'r') as f:
                 return json.load(f)
         except json.JSONDecodeError as e:
             error_msg = f"Error parsing {file_type} configuration file: {path_to_load}"
             self.logger.error(error_msg, exc_info=True)
             raise ConfigError(error_msg, config_path=path_to_load) from e
-        except (IOError, OSError, PermissionError) as e:
+        except PermissionError as e:
+            if file_type == "secrets":
+                # Match load_config()'s tolerance: a secrets file the web
+                # process can't read (e.g. written 0640 by the root-run
+                # display service before the group was fixed up) shouldn't
+                # 500 the settings page — degrade to "no secrets" instead.
+                self.logger.warning(f"Secrets file not readable ({path_to_load}): {e}. Returning empty secrets.")
+                return {}
+            error_msg = f"Error loading {file_type} configuration file {path_to_load}: {str(e)}"
+            self.logger.error(error_msg, exc_info=True)
+            raise ConfigError(error_msg, config_path=path_to_load) from e
+        except (IOError, OSError) as e:
             error_msg = f"Error loading {file_type} configuration file {path_to_load}: {str(e)}"
             self.logger.error(error_msg, exc_info=True)
             raise ConfigError(error_msg, config_path=path_to_load) from e
@@ -539,6 +562,7 @@ def save_raw_file_content(self, file_type: str, data: Dict[str, Any]) -> None:
                 # Ensure final file has correct permissions
                 try:
                     ensure_file_permissions(path_obj, file_mode)
+                    ensure_shared_group_ownership(path_obj)
                 except OSError as perm_error:
                     # If we can't set permissions but file was written, log warning but don't fail
                     self.logger.warning(

src/config_manager_atomic.py

@@ -17,6 +17,7 @@
 
 from src.exceptions import ConfigError
 from src.logging_config import get_logger
+from src.common.permission_utils import ensure_shared_group_ownership
 
 
 class SaveResultStatus(Enum):
@@ -410,6 +411,13 @@ def _atomic_move(self, source: Path, destination: Path) -> None:
             # This is important because temp files may have different permissions
             # and we need root service to be able to read config.json
             os.chmod(destination, target_mode)
+
+            # Also fix group ownership when this save is running as root
+            # (the display service): 0o640 alone only helps the non-root web
+            # user read a root-written secrets file if its group already
+            # matches the web user's group, which isn't guaranteed. See
+            # permission_utils.ensure_shared_group_ownership for why.
+            ensure_shared_group_ownership(destination)
 
         except Exception as e:
             raise ConfigError(f"Error during atomic move: {e}") from e