fix: make everything written into the mirror group-readable
The mirror is written by one account and read by another -- an SMB share, or whatever else serves it -- but nothing here produced a group-readable file. Encodes go through `tempfile.mkstemp`, which creates 0600 regardless of the umask and keeps that mode through the rename into place, so every encoded track landed unreadable. Copies of existing MP3s inherit the mode of a source file in a library this tool does not own, which may be no better. Add the group-read bit explicitly: to the temporary file before it is renamed, so a mirror file is never visible without it, and to a copy once it has landed. Directories are handled by clearing the group bits from the process umask rather than chmod'ing each one, since a file the group cannot reach is no more useful than one it cannot read. Only the group bits are touched; the world bits and ownership stay with the umask as before. Mirror files written before this are repaired on the next pass. Their mtimes are correct, so no other part of the pass would revisit them, and topping up the mode costs a stat rather than a re-encode.
This commit is contained in:
@@ -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."""
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user