From b83a9cf017f72b6dc135f38308dde6e8718dec16 Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Tue, 13 Oct 2026 16:19:41 -0600 Subject: [PATCH] Onboarding a drive keeps a library on the internal disk (#828) The onboarding wizard deleted /media/wrolpi/config before mounting a drive, and the shadowed data check ignored config/. A library kept on the internal disk keeps wrolpi.db and its configs there, so onboarding a drive later deleted them without warning. Only a config/ holding nothing but the ssl/ CA is now treated as transient. Co-authored-by: Claude Opus 5.5 --- controller/lib/disks.py | 23 +++++++++--- controller/lib/onboarding.py | 15 ++++++-- controller/test/test_disks.py | 17 +++++++-- controller/test/test_onboarding.py | 59 +++++++++++++++++++++++------- 4 files changed, 89 insertions(+), 25 deletions(-) diff --git a/controller/lib/disks.py b/controller/lib/disks.py index badb031d..83ac0338 100644 --- a/controller/lib/disks.py +++ b/controller/lib/disks.py @@ -271,15 +271,25 @@ def is_mount_busy(mount_point: str) -> bool: return False +def is_transient_config(config_dir: Path) -> bool: + """True when `config_dir` holds only the `ssl/` CA that `generate_certificates.sh` writes before the primary + drive is mounted. Anything else (wrolpi.db, configs) belongs to a library kept on that disk.""" + try: + return all(e.name == "ssl" for e in config_dir.iterdir()) + except FileNotFoundError: + return True + except OSError: + return False + + def check_shadowed_data(target: str) -> Optional[dict]: """Look for user data at `target` that would be hidden by a mount over it. - Returns a dict with `size_bytes` and `entries` when non-config content is - present, or None when the target is empty (or contains only `config/`). + Returns a dict with `size_bytes` and `entries` when user data is present, + or None when the target is empty (or contains only a transient `config/`). - `config/` is whitelisted because it is created transiently by - `generate_certificates.sh` before the primary drive is mounted; that is - expected, not user data. + A `config/` holding only `ssl/` is ignored because `generate_certificates.sh` + creates it before the primary drive is mounted; see `is_transient_config`. Returns None for non-existent targets. """ @@ -288,7 +298,8 @@ def check_shadowed_data(target: str) -> Optional[dict]: return None try: - entries = [e.name for e in target_path.iterdir() if e.name != "config"] + entries = [e.name for e in target_path.iterdir() + if not (e.name == "config" and is_transient_config(e))] except OSError: return None if not entries: diff --git a/controller/lib/onboarding.py b/controller/lib/onboarding.py index aceafc9c..be82bf7c 100644 --- a/controller/lib/onboarding.py +++ b/controller/lib/onboarding.py @@ -21,7 +21,13 @@ from controller.lib.config import ( save_config, update_config, ) -from controller.lib.disks import check_shadowed_data, get_block_devices, mount_drive, unmount_drive +from controller.lib.disks import ( + check_shadowed_data, + get_block_devices, + is_transient_config, + mount_drive, + unmount_drive, +) from controller.lib.fstab import add_fstab_entry from controller.lib.scripts import start_script @@ -226,9 +232,12 @@ def commit_onboarding( # Step 1b: Clean up any transient config directory created on the root # filesystem (e.g. by generate_certificates.sh running before the drive - # was mounted). This prevents stale root CAs from lingering. + # was mounted). This prevents stale root CAs from lingering. A config + # directory with anything else in it is a library kept on the internal + # disk; the shadowed data check below protects it. transient_config = Path(media_dir) / "config" - if transient_config.exists() and not Path(media_dir).is_mount(): + if transient_config.exists() and not Path(media_dir).is_mount() \ + and is_transient_config(transient_config): import shutil shutil.rmtree(transient_config) logger.info("Removed transient config directory at %s", transient_config) diff --git a/controller/test/test_disks.py b/controller/test/test_disks.py index a8c19b8e..2b24ae5e 100644 --- a/controller/test/test_disks.py +++ b/controller/test/test_disks.py @@ -437,13 +437,24 @@ class TestCheckShadowedData: assert check_shadowed_data(str(target)) is None def test_returns_none_when_only_config_present(self, tmp_path): - """`config/` is whitelisted — it's expected pre-mount state.""" + """A `config/` holding only the CA from generate_certificates.sh is expected pre-mount state.""" target = tmp_path / "wrolpi" target.mkdir() - (target / "config").mkdir() - (target / "config" / "ssl.crt").write_text("cert") + (target / "config" / "ssl").mkdir(parents=True) + (target / "config" / "ssl" / "ca.crt").write_text("cert") assert check_shadowed_data(str(target)) is None + def test_detects_config_with_user_data(self, tmp_path): + """A library kept on the internal disk has its database and configs in `config/`.""" + target = tmp_path / "wrolpi" + target.mkdir() + (target / "config" / "ssl").mkdir(parents=True) + (target / "config" / "ssl" / "ca.crt").write_text("cert") + (target / "config" / "wrolpi.db").write_bytes(b"x" * 10) + result = check_shadowed_data(str(target)) + assert result["entries"] == ["config"] + assert result["size_bytes"] == 14 + def test_detects_directory_entry(self, tmp_path): target = tmp_path / "wrolpi" target.mkdir() diff --git a/controller/test/test_onboarding.py b/controller/test/test_onboarding.py index e5c82fd0..745932a0 100644 --- a/controller/test/test_onboarding.py +++ b/controller/test/test_onboarding.py @@ -259,17 +259,20 @@ class TestCommitOnboarding: """Full onboarding with config file on drive.""" media_dir = tmp_path / "wrolpi" media_dir.mkdir() - config_dir = media_dir / "config" - config_dir.mkdir() - config_data = {"drives": {"mounts": [ {"device": "/dev/sdb1", "mount_point": "/media/backup", "fstype": "ext4"}, ]}} - (config_dir / "controller.yaml").write_text(yaml.dump(config_data)) + + def mount_side_effect(device, mount_point, **kwargs): + if device == "/dev/sda1": + # The drive's config appears once it is mounted. + (media_dir / "config").mkdir() + (media_dir / "config" / "controller.yaml").write_text(yaml.dump(config_data)) + return {"success": True} with mock.patch("controller.lib.onboarding.get_media_directory", return_value=media_dir), \ mock.patch("controller.lib.onboarding._cleanup_temp_mount"), \ - mock.patch("controller.lib.onboarding.mount_drive", return_value={"success": True}), \ + mock.patch("controller.lib.onboarding.mount_drive", side_effect=mount_side_effect), \ mock.patch("controller.lib.onboarding.add_fstab_entry", return_value={"success": True}), \ mock.patch("controller.lib.onboarding.reload_config_from_drive", return_value=True), \ mock.patch("controller.lib.onboarding.save_config"), \ @@ -277,7 +280,7 @@ class TestCommitOnboarding: result = commit_onboarding("/dev/sda1", "ext4") assert result["success"] is True - assert str(media_dir) in result["mounts"] + assert result["mounts"] == [str(media_dir), "/media/backup"] assert result["repair_started"] is True def test_fresh_drive_proceeds(self, mock_docker_mode, tmp_path): @@ -402,13 +405,9 @@ class TestCommitOnboarding: """Secondary mount failure should not abort onboarding.""" media_dir = tmp_path / "wrolpi" media_dir.mkdir() - config_dir = media_dir / "config" - config_dir.mkdir() - config_data = {"drives": {"mounts": [ {"device": "/dev/sdb1", "mount_point": "/media/backup", "fstype": "ext4"}, ]}} - (config_dir / "controller.yaml").write_text(yaml.dump(config_data)) call_count = 0 @@ -416,7 +415,10 @@ class TestCommitOnboarding: nonlocal call_count call_count += 1 if call_count == 1: - return {"success": True} # Primary mount succeeds + # Primary mount succeeds, and the drive's config appears. + (media_dir / "config").mkdir() + (media_dir / "config" / "controller.yaml").write_text(yaml.dump(config_data)) + return {"success": True} return {"success": False, "error": "Not found"} # Secondary fails with mock.patch("controller.lib.onboarding.get_media_directory", return_value=media_dir), \ @@ -429,6 +431,7 @@ class TestCommitOnboarding: result = commit_onboarding("/dev/sda1", "ext4") assert result["success"] is True + assert call_count == 2 # Only primary mount should be in the list assert len(result["mounts"]) == 1 @@ -476,8 +479,8 @@ class TestCommitOnboardingShadowedData: """A target containing only `config/` is the expected pre-mount state.""" media_dir = tmp_path / "wrolpi" media_dir.mkdir() - (media_dir / "config").mkdir() - (media_dir / "config" / "ssl.crt").write_text("cert") + (media_dir / "config" / "ssl").mkdir(parents=True) + (media_dir / "config" / "ssl" / "ca.crt").write_text("cert") with contextlib.ExitStack() as stack: for p in self._patched_mount_calls(media_dir): @@ -486,6 +489,36 @@ class TestCommitOnboardingShadowedData: assert result["success"] is True + def test_internal_library_config_blocks_and_is_kept(self, mock_docker_mode, tmp_path): + """A library kept on the internal disk is never deleted by onboarding a drive.""" + media_dir = tmp_path / "wrolpi" + (media_dir / "config" / "ssl").mkdir(parents=True) + (media_dir / "config" / "wrolpi.db").write_bytes(b"x" * 10) + + with contextlib.ExitStack() as stack: + for p in self._patched_mount_calls(media_dir): + stack.enter_context(p) + result = commit_onboarding("/dev/sda1", "ext4") + + assert result["success"] is False + assert result["needs_force"] == "shadowed" + assert result["shadowed_data"]["entries"] == ["config"] + assert (media_dir / "config" / "wrolpi.db").is_file() + + def test_internal_library_config_kept_when_forced(self, mock_docker_mode, tmp_path): + """Forcing the mount hides the internal library, but does not delete it.""" + media_dir = tmp_path / "wrolpi" + (media_dir / "config" / "ssl").mkdir(parents=True) + (media_dir / "config" / "wrolpi.db").write_bytes(b"x" * 10) + + with contextlib.ExitStack() as stack: + for p in self._patched_mount_calls(media_dir): + stack.enter_context(p) + result = commit_onboarding("/dev/sda1", "ext4", force_shadowed=True) + + assert result["success"] is True + assert (media_dir / "config" / "wrolpi.db").is_file() + def test_shadowed_data_blocks_without_force(self, mock_docker_mode, tmp_path): """A target with non-config data should soft-block.""" media_dir = tmp_path / "wrolpi"