Adding docker support for samba shares.

controller/lib/docker_services.py

@@ -27,7 +27,7 @@ except ImportError:
 CONTAINER_PREFIX = os.environ.get("COMPOSE_PROJECT_NAME", "wrolpi")
 
 # Containers that should not have a "View" button (non-HTTP services)
-NON_VIEWABLE_CONTAINERS = {"db", "postgres", "postgresql", "redis", "memcached"}
+NON_VIEWABLE_CONTAINERS = {"db", "postgres", "postgresql", "redis", "memcached", "samba"}
 
 # Containers that should use HTTPS
 HTTPS_CONTAINERS = {"web"}

controller/lib/samba.py

@@ -2,6 +2,10 @@
 Samba share management for WROLPi Controller.
 
 Manages smb.conf generation, smbd/nmbd service control, and share CRUD.
+Works in both native (systemd) and Docker modes.
+
+The controller manages samba's lifecycle: samba is started when shares
+are added and stopped when the last share is removed.
 """
 
 import enum
@@ -21,8 +25,6 @@ from controller.lib.config import (
 
 logger = logging.getLogger(__name__)
 
-SMB_CONF_PATH = Path("/etc/samba/smb.conf")
-
 # Share name must be alphanumeric with hyphens, underscores, or spaces.
 SHARE_NAME_RE = re.compile(r'^[\w\s-]+$')
 
@@ -34,10 +36,26 @@ class SambaStatus(enum.Enum):
     unknown = enum.auto()
 
 
+def _get_smb_conf_path() -> Path:
+    """Get the smb.conf path for the current mode."""
+    if is_docker_mode():
+        return get_media_directory() / "config" / "smb.conf"
+    return Path("/etc/samba/smb.conf")
+
+
 def get_samba_status() -> SambaStatus:
     """Get Samba service status."""
     if is_docker_mode():
-        return SambaStatus.unavailable
+        from controller.lib.docker_services import can_manage_containers, get_container_status
+        if not can_manage_containers():
+            return SambaStatus.unavailable
+        status = get_container_status("samba")
+        container_status = status.get("status")
+        if container_status == "running":
+            return SambaStatus.running
+        elif container_status in ("stopped", "not_found"):
+            return SambaStatus.stopped
+        return SambaStatus.unknown
 
     try:
         result = subprocess.run(
@@ -117,18 +135,57 @@ def _generate_smb_conf() -> str:
     return "\n".join(lines)
 
 
-def _write_smb_conf() -> dict:
-    """Write smb.conf and reload Samba config."""
-    try:
-        content = _generate_smb_conf()
-        SMB_CONF_PATH.write_text(content)
+def _start_samba():
+    """Start samba services."""
+    if is_docker_mode():
+        from controller.lib.docker_services import can_manage_containers, start_container
+        if can_manage_containers():
+            start_container("samba")
+    else:
+        for service in ("smbd", "nmbd"):
+            try:
+                subprocess.run(["systemctl", "start", service],
+                               capture_output=True, text=True, timeout=10)
+            except (subprocess.SubprocessError, FileNotFoundError):
+                pass
 
-        # Reload config without full restart if Samba is running.
+
+def _stop_samba():
+    """Stop samba services."""
+    if is_docker_mode():
+        from controller.lib.docker_services import can_manage_containers, stop_container
+        if can_manage_containers():
+            stop_container("samba")
+    else:
+        for service in ("smbd", "nmbd"):
+            try:
+                subprocess.run(["systemctl", "stop", service],
+                               capture_output=True, text=True, timeout=10)
+            except (subprocess.SubprocessError, FileNotFoundError):
+                pass
+
+
+def _reload_samba():
+    """Reload Samba config. Native uses smbcontrol, Docker restarts the container."""
+    if is_docker_mode():
+        from controller.lib.docker_services import can_manage_containers, restart_container
+        if can_manage_containers():
+            restart_container("samba")
+    else:
         if get_samba_status() == SambaStatus.running:
             subprocess.run(
                 ["smbcontrol", "all", "reload-config"],
                 capture_output=True, text=True, timeout=10,
             )
+
+
+def _write_smb_conf() -> dict:
+    """Write smb.conf and reload Samba config if running."""
+    try:
+        content = _generate_smb_conf()
+        conf_path = _get_smb_conf_path()
+        conf_path.parent.mkdir(parents=True, exist_ok=True)
+        conf_path.write_text(content)
         return {"success": True}
     except PermissionError:
         return {"success": False, "error": "Permission denied writing smb.conf"}
@@ -152,9 +209,6 @@ def _validate_share_path(path: str) -> str | None:
 
 def add_share(name: str, path: str, read_only: bool = True, comment: str = "") -> dict:
     """Add a Samba share."""
-    if is_docker_mode():
-        return {"success": False, "error": "Not available in Docker mode"}
-
     if not SHARE_NAME_RE.match(name):
         return {"success": False, "error": "Share name must be alphanumeric (hyphens/underscores/spaces allowed)"}
 
@@ -175,17 +229,19 @@ def add_share(name: str, path: str, read_only: bool = True, comment: str = "") -
     update_config("samba.shares", shares)
     save_config()
 
-    # Write smb.conf so it's ready when user starts smbd.
     _write_smb_conf()
 
+    # Start samba if it isn't running.
+    if get_samba_status() != SambaStatus.running:
+        _start_samba()
+    else:
+        _reload_samba()
+
     return {"success": True, "share": new_share}
 
 
 def remove_share(name: str) -> dict:
     """Remove a Samba share by name."""
-    if is_docker_mode():
-        return {"success": False, "error": "Not available in Docker mode"}
-
     shares = get_config_value("samba.shares", [])
     new_shares = [s for s in shares if s["name"].lower() != name.lower()]
 
@@ -196,22 +252,33 @@ def remove_share(name: str) -> dict:
     save_config()
     _write_smb_conf()
 
+    if not new_shares:
+        # Last share removed, stop samba.
+        _stop_samba()
+    else:
+        _reload_samba()
+
     return {"success": True}
 
 
 def apply_samba_from_config():
     """Apply Samba config on startup. Called from lifespan.
 
-    Writes smb.conf whenever shares are configured so the config is ready
-    when the user starts smbd from the services table.
+    Writes smb.conf and manages samba lifecycle based on configured shares.
     """
-    if is_docker_mode():
+    shares = get_config_value("samba.shares", [])
+
+    # Always write a valid smb.conf so samba doesn't crash if started manually.
+    result = _write_smb_conf()
+    if not result["success"]:
+        logger.warning("Failed to write Samba config: %s", result.get("error"))
         return
 
-    shares = get_config_value("samba.shares", [])
     if shares:
-        result = _write_smb_conf()
-        if result["success"]:
-            logger.info("Applied Samba config with %d shares", len(shares))
-        else:
-            logger.warning("Failed to apply Samba config: %s", result.get("error"))
+        logger.info("Applied Samba config with %d shares, starting samba", len(shares))
+        if get_samba_status() != SambaStatus.running:
+            _start_samba()
+    else:
+        logger.info("No Samba shares configured, stopping samba")
+        if get_samba_status() == SambaStatus.running:
+            _stop_samba()

controller/test/test_samba.py

@@ -8,6 +8,10 @@ from unittest import mock
 from controller.lib.samba import (
     SambaStatus,
     _generate_smb_conf,
+    _get_smb_conf_path,
+    _reload_samba,
+    _start_samba,
+    _stop_samba,
     add_share,
     apply_samba_from_config,
     get_samba_status,
@@ -19,10 +23,6 @@ from controller.lib.samba import (
 class TestGetSambaStatus:
     """Tests for get_samba_status function."""
 
-    def test_returns_unavailable_in_docker_mode(self):
-        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
-            assert get_samba_status() == SambaStatus.unavailable
-
     def test_returns_running_when_active(self):
         mock_result = mock.Mock()
         mock_result.stdout = "active\n"
@@ -47,22 +47,56 @@ class TestGetSambaStatus:
             with mock.patch("subprocess.run", side_effect=subprocess.TimeoutExpired("cmd", 5)):
                 assert get_samba_status() == SambaStatus.unknown
 
+    def test_docker_mode_returns_running(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
+            with mock.patch("controller.lib.docker_services.can_manage_containers", return_value=True):
+                with mock.patch("controller.lib.docker_services.get_container_status",
+                                return_value={"status": "running"}):
+                    assert get_samba_status() == SambaStatus.running
+
+    def test_docker_mode_returns_stopped(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
+            with mock.patch("controller.lib.docker_services.can_manage_containers", return_value=True):
+                with mock.patch("controller.lib.docker_services.get_container_status",
+                                return_value={"status": "stopped"}):
+                    assert get_samba_status() == SambaStatus.stopped
+
+    def test_docker_mode_unavailable_without_docker(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
+            with mock.patch("controller.lib.docker_services.can_manage_containers", return_value=False):
+                assert get_samba_status() == SambaStatus.unavailable
+
 
 class TestGetSambaStatusDict:
     """Tests for get_samba_status_dict function."""
 
     def test_returns_expected_fields(self):
         with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
-            result = get_samba_status_dict()
-            assert "enabled" in result
-            assert "available" in result
-            assert "shares" in result
+            with mock.patch("controller.lib.docker_services.can_manage_containers", return_value=False):
+                result = get_samba_status_dict()
+                assert "enabled" in result
+                assert "available" in result
+                assert "shares" in result
 
-    def test_unavailable_in_docker(self):
+    def test_unavailable_without_docker(self):
         with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
-            result = get_samba_status_dict()
-            assert result["available"] is False
-            assert result["reason"] is not None
+            with mock.patch("controller.lib.docker_services.can_manage_containers", return_value=False):
+                result = get_samba_status_dict()
+                assert result["available"] is False
+                assert result["reason"] is not None
+
+
+class TestGetSmbConfPath:
+    """Tests for _get_smb_conf_path function."""
+
+    def test_native_mode(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
+            assert str(_get_smb_conf_path()) == "/etc/samba/smb.conf"
+
+    def test_docker_mode(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
+            path = _get_smb_conf_path()
+            assert "config/smb.conf" in str(path)
 
 
 class TestGenerateSmbConf:
@@ -96,20 +130,66 @@ class TestGenerateSmbConf:
     def test_empty_shares_produces_global_only(self, reset_runtime_config):
         conf = _generate_smb_conf()
         assert "[global]" in conf
-        # No share sections.
         lines = conf.split("\n")
         sections = [l for l in lines if l.startswith("[") and l != "[global]"]
         assert len(sections) == 0
 
 
+class TestReloadSamba:
+    """Tests for _reload_samba function."""
+
+    def test_native_mode_calls_smbcontrol(self):
+        mock_result = mock.Mock()
+        mock_result.stdout = "active\n"
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
+            with mock.patch("subprocess.run", return_value=mock_result) as mock_run:
+                _reload_samba()
+                calls = [str(c) for c in mock_run.call_args_list]
+                assert any("smbcontrol" in c for c in calls)
+
+    def test_docker_mode_restarts_container(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
+            with mock.patch("controller.lib.docker_services.can_manage_containers", return_value=True):
+                with mock.patch("controller.lib.docker_services.restart_container") as mock_restart:
+                    _reload_samba()
+                    mock_restart.assert_called_once_with("samba")
+
+
+class TestStartStopSamba:
+    """Tests for _start_samba and _stop_samba functions."""
+
+    def test_start_native(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
+            with mock.patch("subprocess.run") as mock_run:
+                _start_samba()
+                calls = [str(c) for c in mock_run.call_args_list]
+                assert any("start" in c and "smbd" in c for c in calls)
+
+    def test_stop_native(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
+            with mock.patch("subprocess.run") as mock_run:
+                _stop_samba()
+                calls = [str(c) for c in mock_run.call_args_list]
+                assert any("stop" in c and "smbd" in c for c in calls)
+
+    def test_start_docker(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
+            with mock.patch("controller.lib.docker_services.can_manage_containers", return_value=True):
+                with mock.patch("controller.lib.docker_services.start_container") as mock_start:
+                    _start_samba()
+                    mock_start.assert_called_once_with("samba")
+
+    def test_stop_docker(self):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
+            with mock.patch("controller.lib.docker_services.can_manage_containers", return_value=True):
+                with mock.patch("controller.lib.docker_services.stop_container") as mock_stop:
+                    _stop_samba()
+                    mock_stop.assert_called_once_with("samba")
+
+
 class TestAddShare:
     """Tests for add_share function."""
 
-    def test_fails_in_docker_mode(self):
-        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
-            result = add_share("test", "/media/wrolpi")
-            assert result["success"] is False
-
     def test_rejects_invalid_name(self, reset_runtime_config):
         with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
             result = add_share("test/bad", "/media/wrolpi")
@@ -143,15 +223,27 @@ class TestAddShare:
                         assert result["success"] is False
                         assert "already exists" in result["error"]
 
-    def test_adds_share_successfully(self, reset_runtime_config, test_directory):
+    def test_adds_share_and_starts_samba(self, reset_runtime_config, test_directory):
         with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
             with mock.patch("controller.lib.samba.get_media_directory", return_value=test_directory):
                 with mock.patch("controller.lib.samba.save_config"):
                     with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
-                        result = add_share("MyShare", str(test_directory), read_only=False, comment="My files")
-                        assert result["success"] is True
-                        assert result["share"]["name"] == "MyShare"
-                        assert result["share"]["read_only"] is False
+                        with mock.patch("controller.lib.samba.get_samba_status", return_value=SambaStatus.stopped):
+                            with mock.patch("controller.lib.samba._start_samba") as mock_start:
+                                result = add_share("MyShare", str(test_directory))
+                                assert result["success"] is True
+                                mock_start.assert_called_once()
+
+    def test_adds_share_reloads_if_already_running(self, reset_runtime_config, test_directory):
+        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
+            with mock.patch("controller.lib.samba.get_media_directory", return_value=test_directory):
+                with mock.patch("controller.lib.samba.save_config"):
+                    with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
+                        with mock.patch("controller.lib.samba.get_samba_status", return_value=SambaStatus.running):
+                            with mock.patch("controller.lib.samba._reload_samba") as mock_reload:
+                                result = add_share("MyShare", str(test_directory))
+                                assert result["success"] is True
+                                mock_reload.assert_called_once()
 
 
 class TestRemoveShare:
@@ -161,44 +253,70 @@ class TestRemoveShare:
         from controller.lib.config import update_config
         update_config("samba.shares", [
             {"name": "test", "path": "/media/wrolpi", "read_only": True, "comment": ""},
+            {"name": "other", "path": "/media/wrolpi", "read_only": True, "comment": ""},
         ])
-        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
-            with mock.patch("controller.lib.samba.save_config"):
-                with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
+        with mock.patch("controller.lib.samba.save_config"):
+            with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
+                with mock.patch("controller.lib.samba._reload_samba"):
                     result = remove_share("test")
                     assert result["success"] is True
 
                     from controller.lib.config import get_config_value
-                    assert len(get_config_value("samba.shares", [])) == 0
+                    assert len(get_config_value("samba.shares", [])) == 1
+
+    def test_stops_samba_when_last_share_removed(self, reset_runtime_config):
+        from controller.lib.config import update_config
+        update_config("samba.shares", [
+            {"name": "last", "path": "/media/wrolpi", "read_only": True, "comment": ""},
+        ])
+        with mock.patch("controller.lib.samba.save_config"):
+            with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
+                with mock.patch("controller.lib.samba._stop_samba") as mock_stop:
+                    result = remove_share("last")
+                    assert result["success"] is True
+                    mock_stop.assert_called_once()
 
     def test_returns_error_for_missing_share(self, reset_runtime_config):
-        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
-            result = remove_share("nonexistent")
-            assert result["success"] is False
-            assert "not found" in result["error"]
+        result = remove_share("nonexistent")
+        assert result["success"] is False
+        assert "not found" in result["error"]
 
 
 class TestApplySambaFromConfig:
     """Tests for apply_samba_from_config function."""
 
-    def test_skips_in_docker_mode(self):
-        with mock.patch("controller.lib.samba.is_docker_mode", return_value=True):
-            with mock.patch("controller.lib.samba._write_smb_conf") as mock_write:
-                apply_samba_from_config()
-                mock_write.assert_not_called()
-
-    def test_writes_config_when_shares_exist(self, reset_runtime_config):
+    def test_starts_samba_when_shares_exist(self, reset_runtime_config):
         from controller.lib.config import update_config
         update_config("samba.shares", [
             {"name": "test", "path": "/media/wrolpi", "read_only": True, "comment": ""},
         ])
-        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
-            with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}) as mock_write:
-                apply_samba_from_config()
-                mock_write.assert_called_once()
+        with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
+            with mock.patch("controller.lib.samba.get_samba_status", return_value=SambaStatus.stopped):
+                with mock.patch("controller.lib.samba._start_samba") as mock_start:
+                    apply_samba_from_config()
+                    mock_start.assert_called_once()
 
-    def test_skips_when_no_shares(self, reset_runtime_config):
-        with mock.patch("controller.lib.samba.is_docker_mode", return_value=False):
-            with mock.patch("controller.lib.samba._write_smb_conf") as mock_write:
-                apply_samba_from_config()
-                mock_write.assert_not_called()
+    def test_stops_samba_when_no_shares(self, reset_runtime_config):
+        with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
+            with mock.patch("controller.lib.samba.get_samba_status", return_value=SambaStatus.running):
+                with mock.patch("controller.lib.samba._stop_samba") as mock_stop:
+                    apply_samba_from_config()
+                    mock_stop.assert_called_once()
+
+    def test_does_not_start_if_already_running(self, reset_runtime_config):
+        from controller.lib.config import update_config
+        update_config("samba.shares", [
+            {"name": "test", "path": "/media/wrolpi", "read_only": True, "comment": ""},
+        ])
+        with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
+            with mock.patch("controller.lib.samba.get_samba_status", return_value=SambaStatus.running):
+                with mock.patch("controller.lib.samba._start_samba") as mock_start:
+                    apply_samba_from_config()
+                    mock_start.assert_not_called()
+
+    def test_does_not_stop_if_already_stopped(self, reset_runtime_config):
+        with mock.patch("controller.lib.samba._write_smb_conf", return_value={"success": True}):
+            with mock.patch("controller.lib.samba.get_samba_status", return_value=SambaStatus.stopped):
+                with mock.patch("controller.lib.samba._stop_samba") as mock_stop:
+                    apply_samba_from_config()
+                    mock_stop.assert_not_called()

controller/test/test_samba_api.py

@@ -17,11 +17,6 @@ class TestSambaStatusEndpoint:
         assert "available" in data
         assert "shares" in data
 
-    def test_shows_unavailable_in_docker(self, test_client_docker_mode):
-        response = test_client_docker_mode.get("/api/samba/status")
-        data = response.json()
-        assert data["available"] is False
-
 
 class TestSambaShareAddEndpoint:
     """Tests for POST /api/samba/shares endpoint."""
@@ -33,13 +28,6 @@ class TestSambaShareAddEndpoint:
         })
         assert response.status_code == 400
 
-    def test_fails_in_docker(self, test_client_docker_mode):
-        response = test_client_docker_mode.post("/api/samba/shares", json={
-            "name": "test",
-            "path": "/media/wrolpi",
-        })
-        assert response.status_code == 400
-
 
 class TestSambaShareRemoveEndpoint:
     """Tests for DELETE /api/samba/shares/{name} endpoint."""
@@ -47,9 +35,3 @@ class TestSambaShareRemoveEndpoint:
     def test_returns_404_for_missing_share(self, test_client):
         response = test_client.delete("/api/samba/shares/nonexistent")
         assert response.status_code == 404
-
-    def test_fails_in_docker(self, test_client_docker_mode):
-        response = test_client_docker_mode.delete("/api/samba/shares/test")
-        assert response.status_code == 404
-
-

docker-compose.yml

@@ -29,8 +29,8 @@ services:
       # Host system info (read-only)
       - /proc:/host/proc:ro
       - /sys:/host/sys:ro
-      # Config directory
-      - ${MEDIA_DIRECTORY:-./test}/config:/media/wrolpi/config
+      # Media directory (needed for Samba share path validation)
+      - ${MEDIA_DIRECTORY:-./test}:/media/wrolpi
     environment:
       - DOCKERIZED=true
       - MEDIA_DIRECTORY=/media/wrolpi
@@ -159,6 +159,19 @@ services:
       test: [ 'CMD-SHELL', 'curl http://127.0.0.1:80' ]
       interval: 1m
 
+  samba: # Samba file sharing - started/stopped by the controller based on configured shares.
+    build:
+      context: .
+      dockerfile: docker/samba/Dockerfile
+    restart: "no"
+    depends_on:
+      controller:
+        condition: service_healthy
+    ports:
+      - "445:445"
+    volumes:
+      - ${MEDIA_DIRECTORY:-./test}:/media/wrolpi
+
   help: # The WROLPi Help documentation.
     build:
       context: .

docker/samba/Dockerfile

@@ -0,0 +1,6 @@
+FROM debian:12-slim
+RUN apt-get update && \
+    apt-get install -y --no-install-recommends samba samba-common && \
+    rm -rf /var/lib/apt/lists/*
+EXPOSE 445
+CMD ["smbd", "--foreground", "--no-process-group", "--debug-stdout", "-s", "/media/wrolpi/config/smb.conf"]

wrolpi/version.txt

@@ -1 +1 @@
-0.20.0-beta
+0.20.1-beta