perf(pull): apply CANFAR permissions without subprocesses

dtcli/src/functions.py

@@ -4,7 +4,7 @@
 import os
 import re
 import shutil
-import subprocess
+import stat
 import time
 from collections import Counter
 from collections.abc import Sequence
@@ -461,6 +461,63 @@ def find_missing_dataset_files(
     return {"missing": missing_files, "existing": existing_files}
 
 
+def _apply_file_group_permissions(
+    filename: str, directory_fd: int, group_id: Optional[int]
+) -> None:
+    """Update a regular file relative to its directory without following links."""
+    try:
+        metadata = os.stat(filename, dir_fd=directory_fd, follow_symlinks=False)
+        if not stat.S_ISREG(metadata.st_mode):
+            return
+        if group_id is not None:
+            try:
+                os.chown(
+                    filename, -1, group_id, dir_fd=directory_fd, follow_symlinks=False
+                )
+            except OSError as error:
+                logger.warning("Could not update CANFAR ownership: %s", error)
+        metadata = os.stat(filename, dir_fd=directory_fd, follow_symlinks=False)
+        if stat.S_ISREG(metadata.st_mode):
+            os.chmod(
+                filename,
+                stat.S_IMODE(metadata.st_mode) | stat.S_IWGRP,
+                dir_fd=directory_fd,
+                follow_symlinks=False,
+            )
+    except (OSError, ValueError, NotImplementedError) as error:
+        # If a symlink replaces the file, do not follow it. Some platforms
+        # reject no-follow chmod rather than applying it.
+        logger.warning("Could not update CANFAR permissions: %s", error)
+
+
+def _apply_chime_frb_rw_permissions(folder: str, group_id: Optional[int]) -> None:
+    """Set group ownership and write permission without following symlinks."""
+
+    def report_error(error: OSError) -> None:
+        logger.warning("Could not update CANFAR permissions: %s", error)
+
+    try:
+        # fwalk keeps directory access relative to open descriptors and skips
+        # symlinked directories, including a symlink passed as the root folder.
+        for _, _, filenames, directory_fd in os.fwalk(
+            folder, follow_symlinks=False, onerror=report_error
+        ):
+            if group_id is not None:
+                try:
+                    os.fchown(directory_fd, -1, group_id)
+                except OSError as error:
+                    report_error(error)
+            try:
+                mode = stat.S_IMODE(os.fstat(directory_fd).st_mode)
+                os.fchmod(directory_fd, mode | stat.S_IWGRP)
+            except OSError as error:
+                report_error(error)
+            for filename in filenames:
+                _apply_file_group_permissions(filename, directory_fd, group_id)
+    except OSError as error:
+        report_error(error)
+
+
 def get_files(
     files: List[str],
     site: str,
@@ -499,10 +556,16 @@ def get_files(
         # make directory structure if it does not exist.
         folders = {os.path.dirname(path) for path in destinations}
         if site == "canfar":
+            import grp
+
+            try:
+                group_id = grp.getgrnam("chime-frb-rw").gr_gid
+            except (KeyError, OSError) as error:
+                logger.warning("Could not resolve CANFAR group: %s", error)
+                group_id = None
             for folder in folders:
                 os.makedirs(folder, exist_ok=True)
-                subprocess.run(["chgrp", "-R", "chime-frb-rw", folder])
-                subprocess.run(["chmod", "-R", "g+w", folder])
+                _apply_chime_frb_rw_permissions(folder, group_id)
         else:
             for folder in folders:
                 os.makedirs(folder, exist_ok=True)

tests/test_canfar_permissions.py

@@ -0,0 +1,206 @@
+"""Native CANFAR permissions stay within the requested directory tree."""
+
+import grp
+import logging
+import os
+import stat
+from types import SimpleNamespace
+
+import pytest
+
+from dtcli.src import functions
+
+
+def _mode(path):
+    return stat.S_IMODE(path.stat().st_mode)
+
+
+def test_permissions_preserve_modes_and_update_nested_files(tmp_path):
+    folder = tmp_path / "dataset"
+    nested = folder / "nested"
+    nested.mkdir(parents=True)
+    ordinary = nested / "data.h5"
+    executable = folder / "script"
+    ordinary.write_text("data")
+    executable.write_text("script")
+    folder.chmod(0o750)
+    nested.chmod(0o700)
+    ordinary.chmod(0o640)
+    executable.chmod(0o751)
+
+    functions._apply_chime_frb_rw_permissions(str(folder), os.getgid())
+
+    assert _mode(folder) == 0o770
+    assert _mode(nested) == 0o720
+    assert _mode(ordinary) == 0o660
+    assert _mode(executable) == 0o771
+    assert all(
+        path.stat().st_gid == os.getgid()
+        for path in (folder, nested, ordinary, executable)
+    )
+
+
+def test_permissions_do_not_follow_file_directory_or_root_symlinks(tmp_path):
+    folder = tmp_path / "dataset"
+    folder.mkdir()
+    outside = tmp_path / "outside"
+    outside.mkdir()
+    external_file = outside / "private"
+    external_file.write_text("private")
+    outside.chmod(0o700)
+    external_file.chmod(0o600)
+    (folder / "file-link").symlink_to(external_file)
+    (folder / "directory-link").symlink_to(outside, target_is_directory=True)
+    (folder / "cycle").symlink_to(folder, target_is_directory=True)
+    root_link = tmp_path / "root-link"
+    root_link.symlink_to(outside, target_is_directory=True)
+
+    functions._apply_chime_frb_rw_permissions(str(folder), os.getgid())
+    functions._apply_chime_frb_rw_permissions(str(root_link), os.getgid())
+
+    assert _mode(outside) == 0o700
+    assert _mode(external_file) == 0o600
+
+
+def test_permissions_reject_file_replaced_by_symlink(tmp_path, monkeypatch):
+    folder = tmp_path / "dataset"
+    folder.mkdir()
+    victim = folder / "victim"
+    victim.write_text("data")
+    outside = tmp_path / "private"
+    outside.write_text("private")
+    outside.chmod(0o600)
+    original_chmod = os.chmod
+
+    def replace_before_chmod(path, mode, *args, **kwargs):
+        if path == "victim":
+            victim.unlink()
+            victim.symlink_to(outside)
+        return original_chmod(path, mode, *args, **kwargs)
+
+    monkeypatch.setattr(functions.os, "chmod", replace_before_chmod)
+    functions._apply_chime_frb_rw_permissions(str(folder), os.getgid())
+
+    assert victim.is_symlink()
+    assert _mode(outside) == 0o600
+
+
+def test_chown_failure_does_not_skip_chmod(tmp_path, monkeypatch, caplog):
+    folder = tmp_path / "dataset"
+    folder.mkdir(mode=0o700)
+    data = folder / "data"
+    data.write_text("data")
+    data.chmod(0o600)
+
+    def deny_chown(*args, **kwargs):
+        raise PermissionError("group change denied")
+
+    monkeypatch.setattr(functions.os, "fchown", deny_chown)
+    monkeypatch.setattr(functions.os, "chown", deny_chown)
+    with caplog.at_level(logging.WARNING):
+        functions._apply_chime_frb_rw_permissions(str(folder), os.getgid())
+
+    assert _mode(folder) == 0o720
+    assert _mode(data) == 0o620
+    assert "group change denied" in caplog.text
+
+
+def test_chmod_failure_does_not_stop_other_files(tmp_path, monkeypatch, caplog):
+    folder = tmp_path / "dataset"
+    folder.mkdir()
+    denied = folder / "denied"
+    allowed = folder / "allowed"
+    for path in (denied, allowed):
+        path.write_text("data")
+        path.chmod(0o600)
+    original_chmod = os.chmod
+
+    def selective_chmod(path, mode, **kwargs):
+        if path == "denied":
+            raise PermissionError("mode change denied")
+        original_chmod(path, mode, **kwargs)
+
+    monkeypatch.setattr(functions.os, "chmod", selective_chmod)
+    with caplog.at_level(logging.WARNING):
+        functions._apply_chime_frb_rw_permissions(str(folder), os.getgid())
+
+    assert _mode(denied) == 0o600
+    assert _mode(allowed) == 0o620
+    assert "mode change denied" in caplog.text
+
+
+def test_get_files_resolves_group_once_and_preserves_transfer_result(
+    tmp_path, monkeypatch
+):
+    lookups = []
+    transfers = []
+    failed = [{"source": "failed"}]
+
+    def resolve_group(name):
+        lookups.append(name)
+        return SimpleNamespace(gr_gid=os.getgid())
+
+    def transfer(**kwargs):
+        transfers.append(kwargs)
+        return failed
+
+    monkeypatch.setattr(grp, "getgrnam", resolve_group)
+    monkeypatch.setattr(functions, "procure", lambda: {"root_mounts": {}})
+    monkeypatch.setattr(functions.cadcclient, "pget", transfer)
+
+    result = functions.get_files(
+        ["data/a/one.h5", "data/b/two.h5"], "canfar", str(tmp_path), 2, 0
+    )
+
+    assert lookups == ["chime-frb-rw"]
+    assert result is failed
+    assert len(transfers) == 1
+    assert transfers[0]["destination"] == [
+        str(tmp_path / "data/a/one.h5"),
+        str(tmp_path / "data/b/two.h5"),
+    ]
+    assert all(_mode(tmp_path / p) & stat.S_IWGRP for p in ["data/a", "data/b"])
+
+
+@pytest.mark.parametrize("error", [KeyError("missing group"), OSError("lookup failed")])
+def test_missing_group_keeps_group_write_and_downloads(
+    tmp_path, monkeypatch, caplog, error
+):
+    folder = tmp_path / "data"
+    folder.mkdir(mode=0o700)
+    transferred = []
+
+    def missing_group(name):
+        raise error
+
+    monkeypatch.setattr(grp, "getgrnam", missing_group)
+    monkeypatch.setattr(functions, "procure", lambda: {"root_mounts": {}})
+    monkeypatch.setattr(
+        functions.cadcclient, "pget", lambda **kwargs: transferred.append(kwargs) or []
+    )
+    with caplog.at_level(logging.WARNING):
+        result = functions.get_files(["data/file.h5"], "canfar", str(tmp_path), 1, 0)
+
+    assert result == []
+    assert len(transferred) == 1
+    assert _mode(folder) == 0o720
+    assert "Could not resolve CANFAR group" in caplog.text
+
+
+def test_permissions_do_not_require_read_access_to_owned_files(tmp_path):
+    folder = tmp_path / "dataset"
+    folder.mkdir()
+    data = folder / "unreadable"
+    data.write_text("data")
+    data.chmod(0)
+    try:
+        functions._apply_chime_frb_rw_permissions(str(folder), os.getgid())
+        assert _mode(data) == 0o020
+    finally:
+        data.chmod(0o600)
+
+
+def test_missing_directory_is_best_effort(tmp_path, caplog):
+    with caplog.at_level(logging.WARNING):
+        functions._apply_chime_frb_rw_permissions(str(tmp_path / "missing"), os.getgid())
+    assert "Could not update CANFAR permissions" in caplog.text