diff --git a/README.md b/README.md index f74904a..aaae683 100644 --- a/README.md +++ b/README.md @@ -51,9 +51,30 @@ The mtime is read _before_ encoding rather than after. A file still being written when the pass reaches it would otherwise be stamped with its final mtime while holding truncated audio, and never be revisited. -Encodes are written to a temporary file and renamed into place, so an -interrupted run cannot leave a truncated MP3 that the next run mistakes for -finished work. A lock file in the mirror root stops two passes overlapping. +Both encodes and copies are written to a temporary file and renamed into place, +so an interrupted run cannot leave a truncated MP3 that the next run mistakes +for finished work. Copies need it as much as encodes do: the mtime comes across +with the bytes, so a half-written copy would look current for ever. A lock file +in the mirror root stops two passes overlapping. + +### Permissions + +Everything written into the mirror is made group-readable, and its directories +group-traversable, so the mirror can be read back by whatever serves it. Neither +writer does that unaided: the temporary file an encode renames into place is +created `0600` regardless of the umask, and a straight copy of an existing MP3 +inherits the mode of a source file in a library this tool does not own. + +Directories are handled by clearing the owner and group read/execute bits from +the process umask, once, at startup. Owner as well as group, because a umask +carrying `0400` produces directories of mode `0300` — writable and enterable, +unreadable to the very run that created them. The `other` bits are left where +the umask puts them: whether the mirror is world-readable is a genuine policy +question, and so is its ownership. + +Mirror files written before this existed are topped up on the next pass. Their +mtimes are correct, so nothing else would revisit them — and they are not +re-encoded, only chmod'ed. ## Usage diff --git a/music_mirror.py b/music_mirror.py index 4756609..7b4c263 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -77,6 +77,20 @@ MIRROR_SUFFIX = ".mp3" # Filesystems disagree about mtime precision; SMB in particular rounds. MTIME_TOLERANCE_SECONDS = 2 +# The mirror exists to be read back by something else -- an SMB share, another +# account on the box -- so everything written into it has to be group-readable. +# Neither writer manages that unaided: tempfile.mkstemp forces 0600 whatever the +# umask, and shutil.copy2 carries the source file's mode across from a library +# that may be tighter still. Directories need the execute bit too, or the group +# cannot enter them to reach the readable files inside. +GROUP_READ = 0o040 + +# Cleared from the umask so directories this run creates can be listed and +# entered. Owner as well as group: a umask carrying 0400 -- which is unusual but +# not ours to assume away -- otherwise produces a mirror tree that not even the +# process that built it can read back. +DIRECTORY_ACCESS = 0o550 + @dataclass class Result: @@ -124,6 +138,13 @@ def is_current(source, mirror): return abs(source.stat().st_mtime - mirror.stat().st_mtime) <= MTIME_TOLERANCE_SECONDS +def make_group_readable(path): + """Add the group-read bit to a mirror file, leaving the rest of the mode alone.""" + mode = path.stat().st_mode + if not mode & GROUP_READ: + path.chmod(mode | GROUP_READ) + + @functools.lru_cache(maxsize=4096) def find_cover(directory): """Return an external cover image for a directory, if one is present. @@ -234,6 +255,9 @@ def encode(source, mirror, quality_args, dry_run): lines = completed.stderr.strip().splitlines() return Result("failed", source, lines[-1] if lines else "ffmpeg failed") os.utime(temporary, (stat.st_atime, stat.st_mtime)) + # Before the rename, so the file is never visible in the mirror without + # the bit. + make_group_readable(temporary) os.replace(temporary, mirror) except Exception as error: # noqa: BLE001 - reported per file, run continues return Result("failed", source, str(error)) @@ -245,16 +269,31 @@ def encode(source, mirror, quality_args, dry_run): def copy(source, mirror, dry_run): - """Copy an already-MP3 source into the mirror.""" + """Copy an already-MP3 source into the mirror, atomically.""" if dry_run: logger.info("would copy %s", source) return Result("copied", mirror) mirror.parent.mkdir(parents=True, exist_ok=True) + + # Through a temporary file and a rename, for the same reason encodes go + # that way, and a sharper one: copy2 reproduces the source's mtime as well + # as its bytes, so a copy cut short by a full disk or a killed container + # would leave a truncated MP3 that every later pass reads as current. + handle, temporary = tempfile.mkstemp(dir=mirror.parent, suffix=".mp3.part") + os.close(handle) + temporary = Path(temporary) + try: - shutil.copy2(source, mirror) + shutil.copy2(source, temporary) + # copy2 brings the source's mode with it, and the source library is not + # ours to have permissions opinions about. + make_group_readable(temporary) + os.replace(temporary, mirror) except OSError as error: return Result("failed", source, str(error)) + finally: + temporary.unlink(missing_ok=True) logger.info("copied %s", source) return Result("copied", mirror) @@ -263,6 +302,14 @@ def copy(source, mirror, dry_run): def process(source, mirror, quality_args, dry_run): """Bring one source file's mirror entry up to date.""" if is_current(source, mirror): + # A mirror written before this bit was set has a correct mtime, so + # nothing else in the pass would ever revisit it. Top it up here + # instead: one stat per file, and no chmod at all once it is right. + if not dry_run: + try: + make_group_readable(mirror) + except OSError as error: + return Result("failed", mirror, str(error)) return Result("skipped", mirror) if source.suffix.lower() in COPY_EXTENSIONS: return copy(source, mirror, dry_run) @@ -463,6 +510,15 @@ def main(argv=None): logging.basicConfig(format="%(asctime)s %(levelname)s %(message)s", level=logging.INFO) args = build_parser().parse_args(argv) + # Directories are created with 0o777 masked by the umask, so clear the bits + # that matter from it once here rather than chmod'ing every directory the + # walk creates. The `other` bits are left alone, since whether the mirror is + # world-readable is a real policy question; owner and group access is not. + # Files cannot be handled this way -- mkstemp and copy2 both set a mode + # outright, ignoring the umask -- so they get an explicit chmod instead. + inherited = os.umask(0o077) + os.umask(inherited & ~DIRECTORY_ACCESS) + if not args.source or not args.mirror: logger.error("both --source and --mirror are required") return 2 diff --git a/tests/conftest.py b/tests/conftest.py index 7854130..753d029 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -19,6 +19,29 @@ def require_ffmpeg(): pytest.skip(f"{tool} is not on PATH", allow_module_level=True) +@pytest.fixture +def tight_umask(): + """Run a test under a umask that would otherwise make the mirror private.""" + previous = os.umask(0o077) + yield + os.umask(previous) + + +@pytest.fixture +def owner_hostile_umask(): + """Return a callable applying a umask that masks off the owner's read bit. + + Unusual, but it is what produces a mirror tree of mode 0300 -- writable and + enterable, unreadable to the very process that built it. Applied on demand + rather than for the whole test, because the source library is built by + something else entirely and the same umask would make the test's own + fixtures unreadable before the run under test even started. + """ + previous = os.umask(0o022) + yield lambda: os.umask(0o477) + os.umask(previous) + + @pytest.fixture def make_flac(): """Return a factory writing a short tagged FLAC file.""" diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index 825dfae..b609985 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -1,7 +1,9 @@ import os import shutil +import stat import subprocess import time +from pathlib import Path import pytest @@ -153,6 +155,126 @@ def test_existing_mp3_is_copied_not_re_encoded(tmp_path, make_flac): assert (mirror / "b.mp3").read_bytes() == (source / "b.mp3").read_bytes() +def test_interrupted_copy_leaves_nothing_behind(tmp_path, make_flac, monkeypatch): + """copy2 reproduces the source mtime, so a truncated copy left in the mirror + would be read as current by every later pass.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + flac = make_flac(source / "a.flac") + subprocess.run( + ["ffmpeg", "-loglevel", "error", "-y", "-i", str(flac), str(source / "b.mp3")], + check=True, + capture_output=True, + ) + flac.unlink() + + def truncated(src, destination, **kwargs): + Path(destination).write_bytes(Path(src).read_bytes()[:64]) + raise OSError("no space left on device") + + monkeypatch.setattr(music_mirror.shutil, "copy2", truncated) + + assert run(source, mirror) == 1 + assert not (mirror / "b.mp3").exists() + assert list(mirror.rglob("*.part")) == [] + + +def test_encoded_file_is_group_readable(tmp_path, make_flac, tight_umask): + """mkstemp creates 0600 whatever the umask, so the bit has to be added.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "a.flac") + + run(source, mirror) + + assert (mirror / "a.mp3").stat().st_mode & stat.S_IRGRP + + +def test_copied_file_is_group_readable(tmp_path, make_flac, tight_umask): + """copy2 carries the source's mode across, and the source may be private.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + flac = make_flac(source / "a.flac") + subprocess.run( + ["ffmpeg", "-loglevel", "error", "-y", "-i", str(flac), str(source / "b.mp3")], + check=True, + capture_output=True, + ) + flac.unlink() + (source / "b.mp3").chmod(0o600) + + run(source, mirror) + + assert (mirror / "b.mp3").stat().st_mode & stat.S_IRGRP + + +def test_mirror_directories_are_group_traversable(tmp_path, make_flac, tight_umask): + """A readable file is unreachable if the group cannot enter its directory.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "Artist" / "Album" / "a.flac") + + run(source, mirror) + + for directory in (mirror, mirror / "Artist", mirror / "Artist" / "Album"): + mode = directory.stat().st_mode + assert mode & stat.S_IRGRP, directory + assert mode & stat.S_IXGRP, directory + + +def test_mirror_directories_survive_an_owner_hostile_umask( + tmp_path, make_flac, owner_hostile_umask +): + """A umask carrying 0400 otherwise builds a tree the run cannot read back.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "Artist" / "Album" / "a.flac") + + # Applied only now: the library already exists, and the umask under test is + # the one the container starts this run with. + owner_hostile_umask() + run(source, mirror) + + for directory in (mirror, mirror / "Artist", mirror / "Artist" / "Album"): + mode = directory.stat().st_mode + assert mode & stat.S_IRUSR, directory + assert mode & stat.S_IXUSR, directory + assert mode & stat.S_IRGRP, directory + assert mode & stat.S_IXGRP, directory + + +def test_private_mirror_file_is_repaired_without_re_encoding(tmp_path, make_flac): + """A mirror written by an older version has a correct mtime, so nothing + else in the pass would revisit it.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "a.flac") + + run(source, mirror) + output = mirror / "a.mp3" + output.chmod(output.stat().st_mode & ~stat.S_IRGRP) + before = output.stat().st_mtime_ns + + run(source, mirror) + + assert output.stat().st_mode & stat.S_IRGRP + assert output.stat().st_mtime_ns == before + + +def test_dry_run_does_not_change_permissions(tmp_path, make_flac): + source = tmp_path / "src" + mirror = tmp_path / "dst" + make_flac(source / "a.flac") + + run(source, mirror) + output = mirror / "a.mp3" + output.chmod(output.stat().st_mode & ~stat.S_IRGRP) + + run(source, mirror, "--dry-run") + + assert not output.stat().st_mode & stat.S_IRGRP + + def test_format_upgrade_replaces_rather_than_duplicating(tmp_path, make_flac): """Lidarr replacing an MP3 with a FLAC must not leave two mirror files.""" source = tmp_path / "src"