diff --git a/CHANGELOG.md b/CHANGELOG.md index bfa708ef..757d7bc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ [breaking] +- Cache files follow the process umask instead of being forced to `0o777`; `permissions` options were removed. For shared caches, call `exca.utils.setup_shared_folder(folder)` once on the cache root; without default ACL support, also set `umask 002`. [#324] - `CacheDict`: deletions require a `write()` context (like writes). [#326] - `DumpContext.shared_file`: content suffixes must start with `.`. [#326] - `steps`: `Parallel` cannot be a `Chain` step; call it directly. [#328, #329] diff --git a/docs/dev/planned-work.md b/docs/dev/planned-work.md index 898de4e9..0a4168e9 100644 --- a/docs/dev/planned-work.md +++ b/docs/dev/planned-work.md @@ -48,20 +48,6 @@ Non-breaking behavior changes and internal cleanup can proceed without waiting. - **Migration:** switch to `@DumpContext.register` with new-style handlers - **When:** after confirming no external subclasses are in use -### Simplify permission handling -- Shared filesystems (NFS) need explicit chmod on created folders and files - so other users/jobs can read/write cached results. -- Old attempt on branch `set-permissions` (aborted — mixed into a large - refactor): added `PermissionSetter` utility in `utils.py`, a - `permissions: int | None = 0o777` field on `BaseInfra`/`Backend`/`CacheDict`, - and chmod calls after each mkdir/file-write. -- Next attempt should: - - Extract the permission logic cleanly (standalone PR, no other refactors) - - Also handle submitit log/job folders (currently created by submitit - itself, which doesn't set permissions — may need upstream changes in - submitit or post-creation fixup) - - Consider a umask-based approach as an alternative to post-hoc chmod - ## Internal cleanup (non-breaking, can do anytime) ### Remove `_track_legacy_files` recursion diff --git a/docs/infra/explanation.md b/docs/infra/explanation.md index 81e4158c..a0ff9cd0 100644 --- a/docs/infra/explanation.md +++ b/docs/infra/explanation.md @@ -128,6 +128,8 @@ The class is initialized with these parameters: For details on the serialization system and how to write custom handlers, see [Serialization](serialization.md). +Files follow the process umask. For a cache shared between users, call `exca.utils.setup_shared_folder(folder)` once on its root before launching jobs. If default ACLs are unavailable, the function warns and writers must run with `umask 002`. + **Example** ```python fixture:tmp_path import numpy as np diff --git a/docs/internal/proposals/inflight-registry.md b/docs/internal/proposals/inflight-registry.md index 7045676c..ebd315d5 100644 --- a/docs/internal/proposals/inflight-registry.md +++ b/docs/internal/proposals/inflight-registry.md @@ -144,7 +144,7 @@ Located in `exca/cachedict/inflight.py`. class InflightRegistry: """Advisory SQLite registry of in-flight cache items.""" - def __init__(self, folder: Path, permissions: int | None = 0o777) -> None: + def __init__(self, folder: Path) -> None: # DB at /inflight.db ... @@ -287,8 +287,7 @@ where coordination matters — which is what `docs/internal/debug/concurrent-wri identified as the core problem. The DB file is visible (no leading dot) for easy manual deletion if needed. File -permissions default to `0o777` (matching CacheDict's shared-access model) and are -applied after DB creation. +permissions follow the process umask. ## Same-PID Ownership diff --git a/docs/internal/steps/spec.md b/docs/internal/steps/spec.md index 1e7db02f..7c89d944 100644 --- a/docs/internal/steps/spec.md +++ b/docs/internal/steps/spec.md @@ -247,7 +247,7 @@ required for MapInfra parity or current step semantics. ### Safety Measures (from TaskInfra/MapInfra) - Config consistency checking (`identity.write_configs`) -- Permissions on CacheDict (`permissions=0o777`) +- Shared cache access follows the process umask - Force/retry one-shot tracking per Backend lifetime - Job lifecycle status — `LookupHandle.status` returns `"success"` / `"error"` / `"running"` / `None` diff --git a/exca/base.py b/exca/base.py index 6b7253f3..700629ff 100644 --- a/exca/base.py +++ b/exca/base.py @@ -128,10 +128,6 @@ def model_with_infra_validator_before(obj: tp.Any) -> tp.Any: class BaseInfra(pydantic.BaseModel): folder: Path | str | None = None - # general permission for folders and files - # use os.chmod / path.chmod compatible numbers, or None to deactivate - # eg: 0o777 for all rights to all users - permissions: int | None = 0o777 # {folder} will be replaced by the class folder # {user} by user id and %j by job id logs: Path | str = "{folder}/logs/{user}/%j" @@ -197,6 +193,11 @@ def _exclude_from_cls_uid(self) -> list[str]: def model_post_init(self, log__: tp.Any) -> None: # Pydantic's private-attr hook would otherwise shadow SubmititMixin's hook. super().model_post_init(log__) + if ".." in Path(self.version).parts: + raise ValueError( + f"version={self.version!r} must not contain '..': it is a path " + "component of the cache folder and would escape the cache root" + ) def __repr_args__(self) -> tp.Iterator[tuple[str | None, tp.Any]]: """Compact repr: only show fields that differ from their default value.""" @@ -250,15 +251,6 @@ def _check_configs(self, write: bool = True) -> None: ) dump.check_and_write(xpfolder, write=write) state.checked_configs = True - # Set permissions on written files - if write: - for name in ("uid", "full-uid", "config"): - fp = xpfolder / f"{name}.yaml" - if fp.exists(): - try: - self._set_permissions(fp) - except (OSError, FileNotFoundError): - pass def _factory(self) -> str: state = _fast_state(self) @@ -346,7 +338,7 @@ def uid_folder(self, create: bool = False) -> Path | None: folder = Path(self.folder) / self.uid() if not create: return folder - utils.mkdir_with_permissions(folder, self.permissions, root=self.folder) + folder.mkdir(parents=True, exist_ok=True) return folder def iter_cached(self) -> tp.Iterable[pydantic.BaseModel]: @@ -362,14 +354,6 @@ def iter_cached(self) -> tp.Iterable[pydantic.BaseModel]: cfg = ConfDict.from_yaml(fp) yield cls(**cfg) - def _set_permissions(self, path: str | Path) -> None: - if self.permissions is not None: - try: - Path(path).chmod(self.permissions) - except Exception as e: - msg = f"Failed to set permission to {self.permissions} on '{path}'\n({e})" - logger.warning(msg) - def clone_obj(self, *args: dict[str, tp.Any], **kwargs: tp.Any) -> tp.Any: """Create a new decorated object by applying a diff config to the underlying object""" if args: diff --git a/exca/cachedict/core.py b/exca/cachedict/core.py index 4544a1b9..bf56d72d 100644 --- a/exca/cachedict/core.py +++ b/exca/cachedict/core.py @@ -90,10 +90,6 @@ class CacheDict(tp.Generic[X]): If `None`, the type will be deduced automatically (Json for JSON-serializable values, or a type-specific handler for numpy arrays, tensors, etc.). Loading is handled using the cache_type specified in info files. - permissions: optional int - permissions for generated files - use os.chmod / path.chmod compatible numbers, or None to deactivate - eg: 0o777 for all rights to all users Usage ----- @@ -124,10 +120,8 @@ def __init__( folder: Path | str | None, keep_in_ram: bool = False, cache_type: None | str = None, - permissions: int | None = 0o777, ) -> None: self.folder = None if folder is None else Path(folder) - self.permissions = permissions self.cache_type = cache_type self._keep_in_ram = keep_in_ram if self.folder is None and not keep_in_ram: @@ -142,7 +136,7 @@ def __init__( # DumpContext for this folder (load/delete; writes use per-thread _write_ctx) self._dumper: DumpContext | None = None if self.folder is not None: - self._dumper = DumpContext(self.folder, permissions=self.permissions) + self._dumper = DumpContext(self.folder) self._local = threading.local() # per-thread write context, see _write_ctx def __repr__(self) -> str: @@ -155,7 +149,7 @@ def __repr__(self) -> str: def __reduce__(self) -> tp.Any: return ( self.__class__, - (self.folder, self._keep_in_ram, self.cache_type, self.permissions), + (self.folder, self._keep_in_ram, self.cache_type), ) def clear(self) -> None: @@ -310,7 +304,7 @@ def write(self) -> tp.Iterator["CacheDict[X]"]: if self._write_ctx is not None: raise RuntimeError("Cannot re-open an already open writer") if self.folder is not None: - self._write_ctx = DumpContext(self.folder, permissions=self.permissions) + self._write_ctx = DumpContext(self.folder) self._local.deleted_in_scope = False try: if self._write_ctx is not None: diff --git a/exca/cachedict/dumpcontext.py b/exca/cachedict/dumpcontext.py index 38e4305c..85bd80e1 100644 --- a/exca/cachedict/dumpcontext.py +++ b/exca/cachedict/dumpcontext.py @@ -114,13 +114,10 @@ class DumpContext: DATA_DIR = "data" INFO_SUFFIX = "-info.jsonl" - def __init__( - self, folder: str | Path, *, key: str = "", permissions: int | None = None - ) -> None: + def __init__(self, folder: str | Path, *, key: str = "") -> None: self.folder = Path(folder) self.key = key self.level: int = -1 - self.permissions = permissions self.options = DumpOptions() # write state self._thread_id = threading.get_native_id() @@ -198,21 +195,9 @@ def __enter__(self) -> tp.Self: self._stack = contextlib.ExitStack() self._stack.__enter__() self.folder.mkdir(parents=True, exist_ok=True) - self._created_files.append(self.folder) # re-chmod for shared caches return self def __exit__(self, *exc: tp.Any) -> None: - if self.permissions is not None: - for fp in self._created_files: - paths = [fp, *(fp.rglob("*") if fp.is_dir() else [])] - for path in paths: - try: - path.chmod(self.permissions) - except FileNotFoundError: - pass # deleted mid-walk — nothing to fix - except Exception: - msg = "Failed to set permissions on %s" - logger.warning(msg, path, exc_info=True) if self._stack is None: raise RuntimeError("DumpContext.__exit__ called without __enter__") try: @@ -221,13 +206,6 @@ def __exit__(self, *exc: tp.Any) -> None: self._files.clear() self._created_files.clear() - def _ensure_parent(self, path: Path) -> None: - """Create parent directories and track them for permission setting.""" - parent = path.parent - if parent != self.folder and not parent.exists(): - parent.mkdir(parents=True, exist_ok=True) - self._created_files.append(parent) - def shared_file(self, suffix: str) -> tuple[tp.IO[bytes], str]: """Open a shared file for appending. Returns (handle, relative_name). Content files go under DATA_DIR/; info files (-info.jsonl) @@ -244,7 +222,7 @@ def shared_file(self, suffix: str) -> tuple[tp.IO[bytes], str]: name = basename if is_info else f"{self.DATA_DIR}/{basename}" if name not in self._files: path = self.folder / name - self._ensure_parent(path) + path.parent.mkdir(parents=True, exist_ok=True) f = path.open("ab") self._stack.enter_context(f) self._files[name] = f @@ -260,7 +238,7 @@ def key_path(self, suffix: str = "") -> str: basename = string_uid(self.key) + suffix name = f"{self.DATA_DIR}/{basename}" path = self.folder / name - self._ensure_parent(path) + path.parent.mkdir(parents=True, exist_ok=True) if path in self._created_files: # Same dump context tried to create this path twice: user error raise RuntimeError( @@ -396,21 +374,10 @@ def _dump_cls(self, cls: tp.Any, value: tp.Any) -> tuple[dict[str, tp.Any], str] "ctx.key must be set before dumping with a legacy DumperLoader" ) info = self._loaders[cls].dump(self.key, value) - self._track_legacy_files(info) else: info = cls.__dump_info__(self, value) return info, cls.__name__ - def _track_legacy_files(self, info: tp.Any) -> None: - """Record files from legacy DumperLoader info dicts for permission setting. - New-style handlers track files at creation (keyed_filepath / shared_file).""" - if isinstance(info, dict): - if "filename" in info: - self._created_files.append(self.folder / info["filename"]) - for val in info.values(): - if isinstance(val, dict): - self._track_legacy_files(val) - def _resolve_type(self, info: dict[str, tp.Any]) -> tuple[tp.Any, dict[str, tp.Any]]: """Extract #type and #key from an info dict, return (cls, remaining_info). Applies ``options.replace`` before handler lookup.""" diff --git a/exca/cachedict/registry.py b/exca/cachedict/registry.py index 0b000f8c..17a338d7 100644 --- a/exca/cachedict/registry.py +++ b/exca/cachedict/registry.py @@ -73,9 +73,8 @@ class AdvisoryRegistry: _SCHEMA: tp.ClassVar[str] # passed to executescript(), multi-statement OK _LABEL: tp.ClassVar[str] # short prefix in log messages - def __init__(self, folder: Path | str, permissions: int | None = 0o777) -> None: + def __init__(self, folder: Path | str) -> None: self.db_path = Path(folder) / self._DB_NAME - self.permissions = permissions self._conn: sqlite3.Connection | None = None def _connect(self, *, create: bool = False) -> sqlite3.Connection | None: @@ -111,15 +110,6 @@ def _connect(self, *, create: bool = False) -> sqlite3.Connection | None: # WAL needs cross-host shared memory (broken on NFS) -> DELETE journal conn.execute("PRAGMA journal_mode=DELETE") conn.executescript(self._SCHEMA) - if self.permissions is not None: - try: - self.db_path.chmod(self.permissions) - except Exception: - logger.warning( - "Failed to set permissions on %s", - self.db_path, - exc_info=True, - ) self._conn = conn return conn diff --git a/exca/cachedict/test_cachedict.py b/exca/cachedict/test_cachedict.py index 15c8324e..8bef71d7 100644 --- a/exca/cachedict/test_cachedict.py +++ b/exca/cachedict/test_cachedict.py @@ -143,12 +143,6 @@ def test_specialized_dump( assert files, "Some memmaps should stay open" del cache gc.collect() - # check permissions - octal_permissions = oct(tmp_path.stat().st_mode)[-3:] - assert octal_permissions == "777", f"Wrong permissions for {tmp_path}" - for fp in tmp_path.rglob("*"): - octal_permissions = oct(fp.stat().st_mode)[-3:] - assert octal_permissions == "777", f"Wrong permissions for {fp}" # after del, all files should be closed files = proc.open_files() assert not files, "No file should remain open after del cache" diff --git a/exca/cachedict/test_dumpcontext.py b/exca/cachedict/test_dumpcontext.py index 1fe63b34..5d6bc337 100644 --- a/exca/cachedict/test_dumpcontext.py +++ b/exca/cachedict/test_dumpcontext.py @@ -122,16 +122,14 @@ def test_shared_file_lifecycle(tmp_path: Path) -> None: assert (tmp_path / name1).read_bytes() == b"hello" -def test_context_permissions(tmp_path: Path) -> None: +def test_context_creates_folder_lazily(tmp_path: Path) -> None: folder = tmp_path / "fresh" - ctx = DumpContext(folder, permissions=0o755) + ctx = DumpContext(folder) assert not folder.exists(), "construction must not materialise the folder" with ctx: f, name = ctx.shared_file(".data") f.write(b"test") - assert folder.is_dir() - assert oct(folder.stat().st_mode)[-3:] == "755" - assert oct((folder / name).stat().st_mode)[-3:] == "755" + assert (folder / name).read_bytes() == b"test" # ============================================================================= diff --git a/exca/cachedict/test_registry.py b/exca/cachedict/test_registry.py index a1dc2fa4..f391c860 100644 --- a/exca/cachedict/test_registry.py +++ b/exca/cachedict/test_registry.py @@ -194,11 +194,3 @@ def test_graceful_degradation( reg2._plant(["recovered"]) assert reg2.get(["recovered"]) == {"recovered"} reg2.close() - - -def test_permissions_applied(tmp_path: Path) -> None: - reg = errors.ErrorRegistry(tmp_path, permissions=0o600) - reg._plant(["a"]) - mode = stat.S_IMODE((tmp_path / "errors.db").stat().st_mode) - assert mode == 0o600 - reg.close() diff --git a/exca/map.py b/exca/map.py index 896d8e17..967ef37b 100644 --- a/exca/map.py +++ b/exca/map.py @@ -180,7 +180,7 @@ def _inflight_registry(self) -> inflight.InflightRegistry | None: cache_folder = self.uid_folder() if cache_folder is None: return None - return inflight.InflightRegistry(cache_folder, permissions=self.permissions) + return inflight.InflightRegistry(cache_folder) # pylint: disable=unused-argument def apply( diff --git a/exca/steps/backends.py b/exca/steps/backends.py index f018e490..a4d06fb1 100644 --- a/exca/steps/backends.py +++ b/exca/steps/backends.py @@ -551,7 +551,6 @@ def _cache_dict( folder=cache_folder, cache_type=cache_type, keep_in_ram=self.keep_in_ram, - permissions=0o777, ) self._cds[cache_folder] = cd return cd diff --git a/exca/steps/base.py b/exca/steps/base.py index 10f2c2eb..a28f1f7b 100644 --- a/exca/steps/base.py +++ b/exca/steps/base.py @@ -18,7 +18,6 @@ import pydantic import exca -from exca import utils as xkutils from . import backends, identity, items, utils @@ -295,7 +294,7 @@ def _make_paths(self, aligned: tp.Sequence[Step]) -> backends.StepPaths: identity.step_uid(aligned), cache_type=self._infer_cache_type(), ) - xkutils.mkdir_with_permissions(paths.step_folder, 0o777, root=paths.base_folder) + paths.step_folder.mkdir(parents=True, exist_ok=True) return paths def _exca_uid_dict_override(self) -> dict[str, tp.Any] | None: diff --git a/exca/steps/test_backends.py b/exca/steps/test_backends.py index 29019929..a4e72e00 100644 --- a/exca/steps/test_backends.py +++ b/exca/steps/test_backends.py @@ -8,7 +8,6 @@ import contextlib import logging -import stat import sys import time import typing as tp @@ -68,16 +67,6 @@ def test_backend_execution(tmp_path: Path, backend: str) -> None: assert (job is not None) == (backend == "LocalProcess") -def test_step_permissions(tmp_path: Path) -> None: - infra: tp.Any = {"backend": "Cached", "folder": tmp_path} - chain = Chain(steps=[conftest.Mult(coeff=2), conftest.Add(value=1)], infra=infra) - intermediate = chain.lookup(1).paths.step_folder.parent - intermediate.mkdir(parents=True) - intermediate.chmod(0o700) - chain.run(1) - assert stat.S_IMODE(intermediate.stat().st_mode) == 0o777 - - def test_slurm_backend_param_forwarding( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/exca/task.py b/exca/task.py index a489a061..f368c2bf 100644 --- a/exca/task.py +++ b/exca/task.py @@ -240,7 +240,6 @@ def job_array( else: executor.update_parameters(slurm_array_parallelism=max_workers) executor.folder.mkdir(exist_ok=True, parents=True) - self._set_permissions(executor.folder) name = self.uid().split("/", maxsplit=1)[0] # select jobs to run statuses: dict[Status, list[TaskInfra]] = collections.defaultdict(list) @@ -326,7 +325,6 @@ def _set_job( with utils.temporary_save_path(job_path) as tmp: with tmp.open("wb") as f: pickle.dump(job, f) - self._set_permissions(job_path) # dump config self._check_configs(write=True) return job diff --git a/exca/test_task.py b/exca/test_task.py index fd7c098b..3d413d88 100644 --- a/exca/test_task.py +++ b/exca/test_task.py @@ -9,7 +9,6 @@ import logging import pickle import shutil -import stat import sys import typing as tp import uuid @@ -436,24 +435,10 @@ class Whenever(Whatever): # type: ignore _ = whenever.process() -def test_uid_folder_permissions(tmp_path: Path) -> None: - infra: tp.Any = {"folder": tmp_path} - whatever = Whatever(param1=13, infra1=infra) - uid_folder = whatever.infra1.uid_folder() - assert uid_folder is not None - intermediate = uid_folder.parent - intermediate.mkdir(parents=True) - intermediate.chmod(0o700) - whatever.infra1.uid_folder(create=True) - assert stat.S_IMODE(intermediate.stat().st_mode) == 0o777 - - -def test_uid_folder_rejects_escape(tmp_path: Path) -> None: +def test_version_rejects_escape(tmp_path: Path) -> None: infra: tp.Any = {"folder": tmp_path / "cache", "version": "x/../../escape"} - whatever = Whatever(param1=13, infra1=infra) with pytest.raises(ValueError, match="must not contain"): - whatever.infra1.uid_folder(create=True) - assert not (tmp_path / "escape").exists() + Whatever(param1=13, infra1=infra) class D2(pydantic.BaseModel): diff --git a/exca/test_utils.py b/exca/test_utils.py index 41b6bc62..98d82a4d 100644 --- a/exca/test_utils.py +++ b/exca/test_utils.py @@ -8,6 +8,7 @@ import concurrent.futures import datetime import os +import stat import threading import typing as tp from pathlib import Path @@ -672,3 +673,39 @@ def boom(**_kwargs: tp.Any) -> tp.Any: ex = utils.make_pool_executor("processpool", max_workers=2) assert isinstance(ex, concurrent.futures.ThreadPoolExecutor) ex.shutdown() + + +def test_setup_shared_folder(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + root = tmp_path / "shared" + root.mkdir() + fp = root / "file" + fp.touch() + fp.chmod(0o600) + monkeypatch.setattr(utils, "_current_umask", lambda: 0o077) + monkeypatch.setattr(utils.shutil, "which", lambda _: None) + with pytest.warns(UserWarning, match="umask 002"): + utils.setup_shared_folder(root) + assert stat.S_IMODE(fp.stat().st_mode) == 0o660 + + +@pytest.mark.parametrize( + "mode,mask,expected", + [ + (0o700, 0o022, 0o755), + (0o600, 0o022, 0o644), + (0o600, 0o002, 0o664), + (0o400, 0o022, 0o444), + (0o600, 0o077, 0o600), + ], +) +def test_widen_to_umask(tmp_path: Path, mode: int, mask: int, expected: int) -> None: + fp = tmp_path / "sub" / "a_file" + fp.parent.mkdir() + fp.touch() + fp.chmod(mode) + previous = os.umask(mask) + try: + utils.widen_to_umask(tmp_path) + finally: + os.umask(previous) + assert stat.S_IMODE(fp.stat().st_mode) == expected diff --git a/exca/test_workdir.py b/exca/test_workdir.py index 49eeb9d3..da2c553d 100644 --- a/exca/test_workdir.py +++ b/exca/test_workdir.py @@ -6,6 +6,7 @@ import logging import os +import stat import subprocess import sys from pathlib import Path @@ -73,6 +74,23 @@ def test_workdir_absolute(tmp_path: Path) -> None: assert Path("folder/a_file.py").exists() +def test_workdir_widens_private_source(tmp_path: Path) -> None: + folder = tmp_path / "folder" + folder.mkdir() + fp = folder / "a_file.py" + fp.touch() + fp.chmod(0o600) + folder.chmod(0o700) + mask = os.umask(0o022) + try: + wdir = workdir.WorkDir(folder=tmp_path / "new", copied=[folder]) + with wdir.activate(): + copied = Path("folder").absolute() + finally: + os.umask(mask) + assert stat.S_IMODE(copied.stat().st_mode) == 0o755, "copy must stay traversable" + + def test_double_workdir(tmp_path: Path) -> None: folder = tmp_path / "folder" folder.mkdir() diff --git a/exca/utils.py b/exca/utils.py index 32fc8ca5..19302205 100644 --- a/exca/utils.py +++ b/exca/utils.py @@ -9,11 +9,15 @@ import copy import difflib import hashlib +import itertools import logging import math import os import shutil +import stat +import subprocess import sys +import tempfile import time import typing as tp import uuid @@ -38,6 +42,15 @@ X = tp.TypeVar("X") +def _current_umask() -> int: + """Read the process umask without changing it.""" + with tempfile.TemporaryDirectory() as tmp: + probe = Path(tmp) / "probe" + probe.mkdir() + mode = stat.S_IMODE(probe.stat().st_mode) + return 0o777 & ~mode + + def best_effort_utime(folder: Path) -> None: """Advance *folder*'s mtime, tolerating EPERM on foreign-owned directories.""" # dir mtime unchanged on file-append → must stamp explicitly @@ -52,31 +65,73 @@ def best_effort_utime(folder: Path) -> None: pass -def mkdir_with_permissions( - folder: Path | str, - permissions: int | None, - *, - root: Path | str, -) -> None: - """Create a folder and chmod its directory chain within a root.""" - root_path = Path(root) - folder_path = Path(folder) - parts = folder_path.relative_to(root_path).parts - if ".." in parts: - raise ValueError(f"Folder path must not contain '..': {folder}") - folder_path.mkdir(parents=True, exist_ok=True) - if permissions is None: +def setup_shared_folder(folder: Path | str, group: str | int | None = None) -> None: + """Make a folder tree group-writable, including future files. + + Symlinks and paths the caller cannot modify are skipped. + + Parameters + ---------- + folder: Path | str + Root of the shared tree. + group: str | int | None + Group name or ID to assign, or ``None`` to keep the current group. + """ + root = Path(folder).resolve() + shared_mask = _current_umask() & 0o007 + directories: list[Path] = [] + for path in itertools.chain((root,), root.rglob("*")): + if path.is_symlink(): + continue + try: + if group is not None: + shutil.chown(path, group=group) + is_directory = path.is_dir() + mode = _widened_mode(path, shared_mask) + if is_directory: + mode |= stat.S_ISGID + path.chmod(mode) + if is_directory: + directories.append(path) + except (PermissionError, FileNotFoundError): + pass + if not directories: return - path = root_path - try: - path.chmod(permissions) - for part in parts: - path /= part - path.chmod(permissions) - except Exception as e: - logger.warning( - "Failed to set permission to %s on '%s'\n(%s)", permissions, path, e - ) + setfacl = shutil.which("setfacl") + if setfacl is not None: + try: + for start in range(0, len(directories), 32): + batch = directories[start : start + 32] + paths = [str(path) for path in batch] + command = [setfacl, "-m", "d:g::rwx,d:m::rwx", *paths] + subprocess.run(command, check=True, capture_output=True) + return + except (OSError, subprocess.CalledProcessError): + pass + warnings.warn( + f"Future files under {root} may not be writable by teammates; " + "run 'umask 002' in the shell before launching jobs that write there", + stacklevel=2, + ) + + +def widen_to_umask(folder: Path | str) -> None: + """Mirror owner access to group and other where allowed by the umask.""" + mask = _current_umask() + root = Path(folder) + for path in itertools.chain((root,), root.rglob("*")): + path.chmod(_widened_mode(path, mask)) + + +def _widened_mode(path: Path, mask: int) -> int: + """*path*'s mode with the owner's access bits mirrored to group and other.""" + mode = stat.S_IMODE(path.stat().st_mode) + owner = (mode >> 6) & 0o7 + group_mask = (mask >> 3) & 0o7 + other_mask = mask & 0o7 + group = owner & ~group_mask + other = owner & ~other_mask + return mode | (group << 3) | other def to_chunks( diff --git a/exca/workdir.py b/exca/workdir.py index 09534f9f..973e479a 100644 --- a/exca/workdir.py +++ b/exca/workdir.py @@ -18,6 +18,8 @@ import pydantic import yaml as _yaml +from . import utils + logger = logging.getLogger(__name__) @@ -147,6 +149,7 @@ def activate(self) -> tp.Iterator[None]: if not out.exists(): if path.is_dir(): shutil.copytree(path, out, ignore=ignore) + utils.widen_to_umask(out) # copytree replicates source modes else: out.parent.mkdir(exist_ok=True, parents=True) shutil.copyfile(path, out, follow_symlinks=True)