fix(auth): run pelican under a PTY so fdp login can re-consent

fdp/auth.py

@@ -24,6 +24,7 @@
 import base64
 import json
 import os
+import select
 import shutil
 import subprocess
 import sys
@@ -120,37 +121,141 @@ class CachedToken:
     exp: "int | None"
 
 
+def _token_from_json(text: str) -> "str | None":
+    """If *text* is a JSON dict carrying access_token/token, or a bare JSON
+    string, return that token; otherwise None."""
+    try:
+        data = json.loads(text)
+    except (ValueError, TypeError):
+        return None
+    if isinstance(data, dict):
+        for key in ("access_token", "token"):
+            v = data.get(key)
+            if isinstance(v, str) and v:
+                return v
+    elif isinstance(data, str) and data:
+        return data
+    return None
+
+
 def _extract_token(stdout: str) -> "str | None":
-    """Pull the raw JWT out of pelican's output. Handles a bare JWT printed
-    on its own line (the observed real behavior), and also a --json dict or
-    bare JSON string for robustness across pelican versions."""
+    """Pull the raw JWT out of pelican's output.
+
+    Handles pure --json output (the whole stream is one JSON dict), and also
+    the interleaved stream produced when pelican runs under a PTY: WARNING
+    lines and the device-approval URL precede the token, which is printed
+    last. We therefore scan bottom-up, accepting either a per-line JSON dict
+    or a bare JWT (three dot-separated, space-free segments)."""
     text = stdout.strip()
     if not text:
         return None
-    try:
-        data = json.loads(text)
-        if isinstance(data, dict):
-            for key in ("access_token", "token"):
-                v = data.get(key)
-                if isinstance(v, str) and v:
-                    return v
-        elif isinstance(data, str) and data:
-            return data
-    except (ValueError, TypeError):
-        pass
+    tok = _token_from_json(text)
+    if tok:
+        return tok
     for line in reversed(text.splitlines()):
         line = line.strip()
-        if line.count(".") == 2 and " " not in line:
+        if not line:
+            continue
+        tok = _token_from_json(line)
+        if tok:
+            return tok
+        if (line.count(".") == 2 and " " not in line
+                and "{" not in line and '"' not in line):
             return line
     return None
 
 
+@dataclass
+class _PtyResult:
+    """Exit status and combined terminal output of a PTY-hosted subprocess."""
+    returncode: int
+    output: str
+
+
+def _run_in_pty(cmd) -> "_PtyResult":
+    """Run *cmd* with stdin/stdout/stderr attached to a pseudo-terminal.
+
+    pelican gates *interactive* token acquisition on its **stdout** being a
+    TTY: once the cached refresh token expires it must re-consent via the
+    device-code/browser flow, and it refuses to start that flow when stdout
+    is a pipe. Capturing stdout with an ordinary pipe (the obvious approach)
+    therefore makes re-consent impossible. So we give pelican a real terminal
+    on all three fds. The child's combined output is streamed to our stderr
+    (the user sees the approval URL and any prompt live) while also being
+    captured, so the JWT can be parsed once the flow completes. When our own
+    stdin is a TTY it is forwarded to the child so the user can answer a
+    prompt. Silent renewal (valid refresh token) simply prints the token and
+    returns the same way -- no terminal interaction needed.
+    """
+    import pty  # POSIX-only; the whole FDP stack is Linux.
+
+    master, slave = pty.openpty()
+    try:
+        proc = subprocess.Popen(
+            cmd, stdin=slave, stdout=slave, stderr=slave, close_fds=True)
+    except OSError:
+        os.close(master)
+        os.close(slave)
+        raise
+    os.close(slave)
+
+    try:
+        err_fd = sys.stderr.fileno()
+    except (OSError, ValueError, AttributeError):
+        err_fd = None
+    try:
+        stdin_fd = sys.stdin.fileno()
+        watch_stdin = sys.stdin.isatty()
+    except (OSError, ValueError, AttributeError):
+        stdin_fd, watch_stdin = None, False
+
+    chunks = []
+    try:
+        while True:
+            watch = [master] + ([stdin_fd] if watch_stdin else [])
+            try:
+                ready, _, _ = select.select(watch, [], [], 0.2)
+            except (OSError, ValueError):
+                break
+            if master in ready:
+                try:
+                    data = os.read(master, 4096)
+                except OSError:  # slave closed -> EIO on Linux
+                    data = b""
+                if not data:
+                    break
+                chunks.append(data)
+                if err_fd is not None:
+                    try:
+                        os.write(err_fd, data)
+                    except OSError:
+                        pass
+            if watch_stdin and stdin_fd in ready:
+                try:
+                    fwd = os.read(stdin_fd, 4096)
+                except OSError:
+                    fwd = b""
+                if fwd:
+                    try:
+                        os.write(master, fwd)
+                    except OSError:
+                        pass
+    finally:
+        os.close(master)
+    proc.wait()
+    return _PtyResult(
+        returncode=proc.returncode,
+        output=b"".join(chunks).decode("utf-8", errors="replace"))
+
+
 def _pelican_get_token(pelican_root: str, *, write: bool = False) -> str:
     """Run the pelican OAuth flow and return the raw JWT.
 
-    pelican requires a TTY: stdin and stderr are inherited so the user sees
-    the device-code/browser prompt and can answer any password prompt; only
-    stdout (which carries the bare token) is captured.
+    pelican is run under a pseudo-terminal (see _run_in_pty) so its
+    stdout-is-a-TTY gate is satisfied and a fresh interactive consent can
+    proceed when the cached refresh token has expired -- the case where
+    piping stdout would make `fdp login` fail with "must be run in a
+    terminal".
     """
     pelican = shutil.which("pelican")
     if pelican is None:
@@ -159,13 +264,13 @@ def _pelican_get_token(pelican_root: str, *, write: bool = False) -> str:
     cmd = [pelican, "credentials", "token", "get", scope, pelican_root,
            "--json"]
     try:
-        proc = subprocess.run(cmd, stdout=subprocess.PIPE, text=True)
+        result = _run_in_pty(cmd)
     except OSError as exc:
         raise AuthError(f"failed to invoke pelican: {exc}")
-    if proc.returncode != 0:
+    if result.returncode != 0:
         raise AuthError(
-            f"pelican token request failed (exit {proc.returncode})")
-    token = _extract_token(proc.stdout)
+            f"pelican token request failed (exit {result.returncode})")
+    token = _extract_token(result.output)
     if not token:
         raise AuthError("could not parse a token from pelican output")
     return token

tests/test_auth.py

@@ -6,6 +6,7 @@
 import base64
 import json
 import os
+import sys
 import tempfile
 import time
 import unittest
@@ -155,10 +156,10 @@ def setUp(self):
 
     def test_login_writes_cache_0600(self):
         token = _make_jwt(3600)
-        fake = SimpleNamespace(returncode=0, stdout=json.dumps(
-            {"access_token": token}), stderr="")
+        fake = auth._PtyResult(returncode=0,
+                               output=json.dumps({"access_token": token}))
         with mock.patch.object(auth.shutil, "which", return_value="/bin/pelican"), \
-             mock.patch.object(auth.subprocess, "run", return_value=fake):
+             mock.patch.object(auth, "_run_in_pty", return_value=fake):
             result = auth.login(_bearer_handle())
         self.assertEqual(result.device, "d3d")
         self.assertEqual(result.scope, "read")
@@ -168,9 +169,9 @@ def test_login_writes_cache_0600(self):
 
     def test_login_write_scope_passes_write_arg(self):
         token = _make_jwt(3600)
-        fake = SimpleNamespace(returncode=0, stdout=token, stderr="")
+        fake = auth._PtyResult(returncode=0, output=token)
         with mock.patch.object(auth.shutil, "which", return_value="/bin/pelican"), \
-             mock.patch.object(auth.subprocess, "run", return_value=fake) as run:
+             mock.patch.object(auth, "_run_in_pty", return_value=fake) as run:
             auth.login(_bearer_handle(), write=True)
         argv = run.call_args[0][0]
         self.assertIn("write", argv)
@@ -183,9 +184,9 @@ def test_login_missing_pelican_raises_autherror(self):
                 auth.login(_bearer_handle())
 
     def test_login_pelican_nonzero_raises_autherror(self):
-        fake = SimpleNamespace(returncode=1, stdout="", stderr="boom")
+        fake = auth._PtyResult(returncode=1, output="")
         with mock.patch.object(auth.shutil, "which", return_value="/bin/pelican"), \
-             mock.patch.object(auth.subprocess, "run", return_value=fake):
+             mock.patch.object(auth, "_run_in_pty", return_value=fake):
             with self.assertRaises(auth.AuthError):
                 auth.login(_bearer_handle())
 
@@ -229,6 +230,53 @@ def test_access_token_beats_token(self):
             {"access_token": "a.b.c", "token": "x.y.z"}))
         self.assertEqual(out, "a.b.c")
 
+    def test_interleaved_pty_stream_bare_jwt(self):
+        # What a PTY run really looks like: warnings + device URL, then the
+        # bare JWT printed last. The URL has >2 dots and must not be mistaken
+        # for the token.
+        stream = (
+            "WARNING Failed to renew an expired token\r\n"
+            "To approve, navigate to the following URL:\r\n"
+            "https://origin.example.org:8000/api/v1.0/issuer/device?"
+            "user_code=ABC\r\n"
+            "eyJhbGc.eyJleHA.sig\r\n"
+        )
+        self.assertEqual(auth._extract_token(stream), "eyJhbGc.eyJleHA.sig")
+
+    def test_interleaved_pty_stream_json_line(self):
+        stream = (
+            "WARNING something\r\n"
+            '{"access_token":"a.b.c","token_type":"Bearer"}\r\n'
+        )
+        self.assertEqual(auth._extract_token(stream), "a.b.c")
+
+
+class TestRunInPty(unittest.TestCase):
+    """The regression guard for the login bug: pelican gates interactive
+    token acquisition on stdout being a TTY, so _run_in_pty must give the
+    child a real terminal on stdout (not a pipe)."""
+
+    def test_child_sees_tty_on_stdout_and_output_captured(self):
+        # Mimic pelican's gate: fail unless stdout is a TTY, else emit a JWT.
+        script = (
+            "import sys;"
+            "sys.exit('must be run in a terminal') "
+            "if not sys.stdout.isatty() else print('h.p.s')"
+        )
+        result = auth._run_in_pty([sys.executable, "-c", script])
+        self.assertEqual(result.returncode, 0)
+        self.assertIn("h.p.s", result.output)
+        self.assertEqual(auth._extract_token(result.output), "h.p.s")
+
+    def test_nonzero_returncode_propagated(self):
+        result = auth._run_in_pty(
+            [sys.executable, "-c", "import sys; sys.exit(3)"])
+        self.assertEqual(result.returncode, 3)
+
+    def test_missing_binary_raises_oserror(self):
+        with self.assertRaises(OSError):
+            auth._run_in_pty(["/nonexistent/pelican-xyz"])
+
 
 class TestEnsureToken(unittest.TestCase):
     def setUp(self):