diff --git a/README.md b/README.md index dc23bf1..aaae683 100644 --- a/README.md +++ b/README.md @@ -63,9 +63,14 @@ 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. Only the -group bits are touched; whether the mirror is world-readable stays with the -umask, as does the ownership. +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 diff --git a/music_mirror.py b/music_mirror.py index cf8a6b6..7b4c263 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -84,7 +84,12 @@ MTIME_TOLERANCE_SECONDS = 2 # 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 + +# 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 @@ -505,12 +510,14 @@ 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. + # 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 & ~GROUP_ENTER) + os.umask(inherited & ~DIRECTORY_ACCESS) if not args.source or not args.mirror: logger.error("both --source and --mirror are required") diff --git a/tests/conftest.py b/tests/conftest.py index abcac25..753d029 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -27,6 +27,21 @@ def tight_umask(): 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 238302e..b609985 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -222,6 +222,27 @@ def test_mirror_directories_are_group_traversable(tmp_path, make_flac, tight_uma 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."""