From 1e2e59ec2e976491a17acfb21fefaaf8969f02e3 Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 29 Mar 2026 11:00:09 -0600 Subject: [PATCH] Adding docker support for samba shares. --- controller/lib/docker_services.py | 2 +- controller/lib/samba.py | 117 +++++++++++++---- controller/test/test_samba.py | 212 +++++++++++++++++++++++------- controller/test/test_samba_api.py | 18 --- docker-compose.yml | 17 ++- docker/samba/Dockerfile | 6 + wrolpi/version.txt | 2 +- 7 files changed, 280 insertions(+), 94 deletions(-) create mode 100644 docker/samba/Dockerfile diff --git a/controller/lib/docker_services.py b/controller/lib/docker_services.py index ce4736db..83afb516 100644 --- a/controller/lib/docker_services.py +++ b/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"} diff --git a/controller/lib/samba.py b/controller/lib/samba.py index 45a201b1..f56c7903 100644 --- a/controller/lib/samba.py +++ b/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() diff --git a/controller/test/test_samba.py b/controller/test/test_samba.py index 8bd796af..9cab1ccf 100644 --- a/controller/test/test_samba.py +++ b/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() diff --git a/controller/test/test_samba_api.py b/controller/test/test_samba_api.py index 27023c4c..d488d9d1 100644 --- a/controller/test/test_samba_api.py +++ b/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 - - diff --git a/docker-compose.yml b/docker-compose.yml index 67b423af..aa4661c1 100644 --- a/docker-compose.yml +++ b/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: . diff --git a/docker/samba/Dockerfile b/docker/samba/Dockerfile new file mode 100644 index 00000000..082365b6 --- /dev/null +++ b/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"] diff --git a/wrolpi/version.txt b/wrolpi/version.txt index b136380a..a576fa18 100644 --- a/wrolpi/version.txt +++ b/wrolpi/version.txt @@ -1 +1 @@ -0.20.0-beta +0.20.1-beta