diff --git a/app/src/components/admin/ControllerPage.js b/app/src/components/admin/ControllerPage.js index b68711b0..dc7d60b2 100644 --- a/app/src/components/admin/ControllerPage.js +++ b/app/src/components/admin/ControllerPage.js @@ -633,7 +633,7 @@ function SmartDetailsModal({drive, open, onClose}) { // Protected mount points that should not be unmounted -const PROTECTED_MOUNTS = ['/', '/boot', '/boot/firmware']; +const PROTECTED_MOUNTS = ['/', '/boot', '/boot/firmware', '/media/wrolpi']; // Helper to format disk size const formatSize = (size) => { diff --git a/controller/api/disks.py b/controller/api/disks.py index b8c28162..90292c4e 100644 --- a/controller/api/disks.py +++ b/controller/api/disks.py @@ -2,14 +2,21 @@ Disk management API endpoints for WROLPi Controller. The Controller defines what should be mounted (writes fstab.yaml on the -WROLPi drive) but never calls mount(8) or umount(8) itself. After every -write to fstab.yaml the Controller triggers wrolpi-mounts.service via +WROLPi drive) but never calls mount(8) itself. After every write to +fstab.yaml the Controller triggers wrolpi-mounts.service via ``systemctl restart``; the service reconciles live state to match. The Controller verifies the outcome by reading /proc/mounts through the same MountExecutor abstraction the service uses. + +One exception: the reconciler deliberately never unmounts paths it has +not claimed, so foreign mounts (udisks2 desktop automounts, manual +mounts) under /media/ would otherwise be impossible to remove from the +UI. When a reconcile leaves the requested mount point live, /unmount +falls back to a direct umount(8) for non-reserved /media/ paths. """ import logging +import os import subprocess from typing import Optional @@ -33,6 +40,7 @@ from controller.lib.mount_executor import ( MountExecutor, SubprocessMountExecutor, ) +from controller.lib.reconciler import RESERVED_MOUNT_POINTS from controller.lib.smart import ( get_all_smart_status, is_smart_available, @@ -222,7 +230,12 @@ async def disk_mount(request: MountRequest): @router.post("/unmount") async def disk_unmount(request: UnmountRequest): - """Remove a mount from fstab.yaml and reconcile.""" + """Remove a mount from fstab.yaml and reconcile. + + Falls back to a direct umount when the mount survives the reconcile — + a foreign mount (udisks2 automount, manual mount) the reconciler never + claimed, or a managed mount the reconciler could not release. + """ _check_native_mode() try: @@ -235,6 +248,22 @@ async def disk_unmount(request: UnmountRequest): except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) + # The direct-umount fallback below runs as root, so be strict about + # what we will pass to umount(8): a normalized path strictly under + # /media/ that is not the primary drive. + normalized = os.path.normpath(request.mount_point) + if normalized in RESERVED_MOUNT_POINTS: + raise HTTPException( + status_code=400, + detail=f"Cannot unmount {request.mount_point}: it is the primary " + f"WROLPi drive.", + ) + if not normalized.startswith("/media/"): + raise HTTPException( + status_code=400, + detail=f"Refusing to unmount {normalized}: not under /media/.", + ) + data = load_fstab() removed = data.remove_by_mount_point(request.mount_point) save_fstab(data) @@ -244,12 +273,15 @@ async def disk_unmount(request: UnmountRequest): raise HTTPException(status_code=500, detail=f"Failed to reconcile mounts: {err}") - if request.mount_point in _executor.current_mount_points(): - raise HTTPException( - status_code=500, - detail=f"Reconciler ran but {request.mount_point} is still mounted. " - f"Check journalctl -u wrolpi-mounts.service.", - ) + if normalized in _executor.current_mount_points(): + # The reconciler did not release it — either a foreign mount it + # never claimed, or the umount failed. Try directly. + result = _executor.unmount(normalized) + if not result.ok: + raise HTTPException( + status_code=500, + detail=f"{normalized} is still mounted: {result.error}", + ) return { "success": True, diff --git a/controller/templates/index.html b/controller/templates/index.html index 53073664..aa7a5807 100644 --- a/controller/templates/index.html +++ b/controller/templates/index.html @@ -1604,7 +1604,7 @@ `; - const protectedMounts = ['/', '/boot', '/boot/firmware']; + const protectedMounts = ['/', '/boot', '/boot/firmware', '/media/wrolpi']; const isProtected = protectedMounts.includes(disk.mountpoint); // Check if this disk has a persistent entry (by mount point or UUID) diff --git a/controller/test/test_disks_api.py b/controller/test/test_disks_api.py index 1ba09ccb..ebb53bdf 100644 --- a/controller/test/test_disks_api.py +++ b/controller/test/test_disks_api.py @@ -219,7 +219,8 @@ class TestUnmountEndpoint: assert fstab_yaml.load(disks_env["fstab_path"]).mounts == [] def test_unmount_returns_500_when_still_mounted(self, test_client, disks_env): - # Reconciler refuses the unmount — surface the failure. + # Reconciler refuses the unmount (e.g. busy) — the direct fallback + # fails the same way; surface the failure. disks_env["fake"]._live.add("/media/wrolpi/usb") disks_env["fake"].fail_for_unmount = frozenset({"/media/wrolpi/usb"}) (disks_env["fstab_path"].parent / "managed").write_text( @@ -232,6 +233,54 @@ class TestUnmountEndpoint: assert response.status_code == 500 assert "still mounted" in response.json()["detail"] + def test_unmount_foreign_mount_falls_back_to_direct_umount( + self, test_client, disks_env, + ): + # A udisks2 desktop automount: live, but never in fstab.yaml and + # never claimed by the reconciler. The reconciler will not touch + # it, so the endpoint must unmount it directly. + foreign = "/media/wrolpi/31a8ecf1-c00b-4691-b26f-b0966073659f" + disks_env["fake"]._live.add(foreign) + + response = test_client.post( + "/api/disks/unmount", + json={"mount_point": foreign}, + ) + assert response.status_code == 200 + body = response.json() + assert body["success"] is True + assert body["removed_from_fstab"] is False + assert disks_env["fake"].current_mount_points() == set() + + @pytest.mark.parametrize("mount_point", [ + "/media", # not strictly under /media/ + "/media_evil", # prefix-collides with /media + "/media/wrolpi/../wrolpi", # normalizes to the reserved primary + ]) + def test_unmount_rejects_paths_outside_media( + self, test_client, disks_env, mount_point, + ): + response = test_client.post( + "/api/disks/unmount", + json={"mount_point": mount_point}, + ) + assert response.status_code == 400 + assert disks_env["fake"].unmount_calls == [] + + def test_unmount_reserved_mount_point_rejected(self, test_client, disks_env): + # The primary WROLPi drive must never be unmountable from the API. + disks_env["fake"]._live.add("/media/wrolpi") + + response = test_client.post( + "/api/disks/unmount", + json={"mount_point": "/media/wrolpi"}, + ) + assert response.status_code == 400 + assert "primary" in response.json()["detail"].lower() + # No umount was attempted, directly or via the reconciler. + assert disks_env["fake"].unmount_calls == [] + assert disks_env["fake"].current_mount_points() == {"/media/wrolpi"} + # --- /api/disks/fstab read endpoint ---------------------------------------