Fix permission denied errors

isic_cli/cli/image.py

@@ -19,7 +19,7 @@
 from rich.console import Console
 from rich.progress import Progress
 
-from isic_cli.cli.types import CommaSeparatedCollectionIds, SearchString
+from isic_cli.cli.types import CommaSeparatedCollectionIds, SearchString, WritableDirectoryPath
 from isic_cli.cli.utils import _extract_metadata, get_attributions, suggest_guest_login
 from isic_cli.io.http import (
     download_image,
@@ -114,7 +114,7 @@ def image(ctx):
 )
 @click.argument(
     "outdir",
-    type=click.Path(file_okay=False, dir_okay=True, path_type=Path),
+    type=WritableDirectoryPath(file_okay=False, dir_okay=True, path_type=Path),
 )
 @click.pass_obj
 @suggest_guest_login

isic_cli/cli/types.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import contextlib
 import re
 import sys
 
@@ -135,17 +136,77 @@ def __init__(self, *args, **kwargs):
     def convert(self, value, param, ctx):
         value = super().convert(value, param, ctx)
 
-        # writeable checks on click.Path only apply to already existing paths, see
-        # https://github.com/pallets/click/issues/2495.
-        # check if the final path is writable before going to the effort of downloading the data.
+        # click.Path(writable=True) only validates existing paths, so it can't catch
+        # the case where the file must be created. See
+        # https://github.com/pallets/click/issues/2495. Attempt to open the file so we
+        # exercise the real OS check, then remove it so this stays a pure probe —
+        # a subsequent argument failing to parse must not leave a stray file behind.
+        # The callback recreates it for real.
+        #
+        # Actually performing the operation is more reliable than heuristics like
+        # os.access, which only checks POSIX mode bits and misses ACLs, read-only
+        # mounts, immutable flags, etc. — it can say "writable" for a path the
+        # kernel will still reject. If we want to know whether the write will work,
+        # the most truthful test is to try it.
         if value is not None and str(value) != "-":
             try:
+                # .exists() can itself raise (e.g. a filename that's too long), so
+                # it has to live inside the try alongside the write probe.
+                existed_before = value.exists()
                 value.parent.mkdir(parents=True, exist_ok=True)
                 with value.open("w", newline="", encoding="utf8"):
                     pass
-            except (PermissionError, OSError):
-                # a user can end up here from lacking permissions, a read only filesystem,
-                # filenames that are too long or have invalid chars, etc.
-                self.fail(f"Permission denied - cannot write to '{value}'.", param, ctx)
+            except OSError as e:
+                msg = f"Permission denied - cannot write to '{value}': {e.strerror}."
+                self.fail(msg, param, ctx)
+
+            # remove what we created — a later argument could still fail to parse
+            if not existed_before:
+                with contextlib.suppress(OSError):
+                    value.unlink()
+
+        return value
+
+
+class WritableDirectoryPath(click.Path):
+    name = "writable_directory_path"
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+        if self.file_okay:
+            raise ValueError("file_okay must be False")
+        elif not self.dir_okay:
+            raise ValueError("dir_okay must be True")
+
+    def convert(self, value, param, ctx):
+        value = super().convert(value, param, ctx)
+
+        # click.Path(writable=True) only validates existing paths, so it can't catch
+        # the case where the directory must be created. See
+        # https://github.com/pallets/click/issues/2495. Attempt the mkdir so we
+        # exercise the real OS check, then remove it so this stays a pure probe —
+        # a subsequent argument failing to parse must not leave a stray directory
+        # behind. The callback recreates it for real.
+        #
+        # Actually performing the operation is more reliable than heuristics like
+        # os.access, which only checks POSIX mode bits and misses ACLs, read-only
+        # mounts, immutable flags, etc. — it can say "writable" for a path the
+        # kernel will still reject. If we want to know whether mkdir will work,
+        # the most truthful test is to try it.
+        if value is not None:
+            try:
+                # .exists() can itself raise (e.g. a filename that's too long), so
+                # it has to live inside the try alongside the write probe.
+                existed_before = value.exists()
+                value.mkdir(parents=True, exist_ok=True)
+            except OSError as e:
+                msg = f"Permission denied - cannot write to '{value}': {e.strerror}."
+                self.fail(msg, param, ctx)
+
+            # remove what we created — a later argument could still fail to parse
+            if not existed_before:
+                with contextlib.suppress(OSError):
+                    value.rmdir()
 
         return value

tests/test_cli_image.py

@@ -3,6 +3,7 @@
 import logging
 import os
 from pathlib import Path
+import sys
 
 import pytest
 from requests import HTTPError
@@ -125,6 +126,23 @@ def test_image_download_legacy_diagnosis_unsupported(cli_run, outdir):
     assert "no longer supported" in result.output
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="chmod doesn't restrict directory writes")
+@pytest.mark.usefixtures("_isolated_filesystem")
+def test_image_download_unwritable_outdir(cli_run):
+    readonly = Path("readonly")
+    readonly.mkdir()
+    readonly.chmod(0o555)
+    try:
+        result = cli_run(["image", "download", str(readonly / "child")])
+    finally:
+        readonly.chmod(0o755)
+
+    assert result.exit_code == 2
+    assert "Permission denied" in result.output
+    # probe must be side-effect-free: a failed validation leaves no stray dir
+    assert not (readonly / "child").exists()
+
+
 @pytest.mark.usefixtures("_isolated_filesystem", "_mock_images")
 def test_image_download_shows_size_info(cli_run, outdir):
     result = cli_run(["image", "download", outdir])

tests/test_cli_metadata.py

@@ -104,6 +104,23 @@ def test_metadata_download_permission_denied(cli_run, output_file):
     assert re.search(r"Permission denied", result.output), result.output
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="chmod doesn't restrict directory writes")
+@pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem")
+def test_metadata_download_unwritable_output(cli_run):
+    readonly = Path("readonly")
+    readonly.mkdir()
+    readonly.chmod(0o555)
+    try:
+        result = cli_run(["metadata", "download", "-o", str(readonly / "child.csv")])
+    finally:
+        readonly.chmod(0o755)
+
+    assert result.exit_code == 2
+    assert re.search(r"Permission denied", result.output), result.output
+    # probe must be side-effect-free: a failed validation leaves no stray file
+    assert not (readonly / "child.csv").exists()
+
+
 @pytest.mark.usefixtures("_mock_image_metadata")
 @pytest.mark.parametrize(
     "cli_runner",