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
 

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

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."""

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"