diff --git a/music_mirror.py b/music_mirror.py index 97a57dd..1fd7ede 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -76,6 +76,15 @@ 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 +GROUP_ENTER = 0o050 + @dataclass class Result: @@ -123,6 +132,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) + + def find_cover(directory): """Return an external cover image for a directory, if one is present.""" for name in COVER_NAMES: @@ -223,6 +239,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)) @@ -242,6 +261,9 @@ def copy(source, mirror, dry_run): mirror.parent.mkdir(parents=True, exist_ok=True) try: shutil.copy2(source, mirror) + # copy2 brings the source's mode with it, and the source library is not + # ours to have permissions opinions about. + make_group_readable(mirror) except OSError as error: return Result("failed", source, str(error)) @@ -252,6 +274,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) @@ -432,6 +462,13 @@ 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 group + # bits from it once here rather than chmod'ing every directory the walk + # creates. Files cannot be handled this way -- mkstemp and copy2 both set a + # mode outright -- so they get an explicit chmod instead. + inherited = os.umask(0o077) + os.umask(inherited & ~GROUP_ENTER) + 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..abcac25 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -19,6 +19,14 @@ 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 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..0964c97 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -1,5 +1,6 @@ import os import shutil +import stat import subprocess import time @@ -153,6 +154,81 @@ 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_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_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"