From a11e051f8295095e93e3ef5835c099d40bb568f8 Mon Sep 17 00:00:00 2001 From: Tyler Date: Sun, 21 Jun 2026 00:00:13 -0600 Subject: [PATCH] feat(sp13): paramiko-backed SftpClient wire-up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace SftpClient stub write_file/list_inbound/read_file implementations with real paramiko SSHClient + SFTPClient calls. The public API (SftpClient.write_file, list_inbound, read_file, get_secret) is unchanged from SP9 — same signature, same return types — so the API layer needs no changes. Real-mode behavior: * _connect() returns a context manager yielding (ssh, sftp); closes both on exit. Lazy-imports paramiko so the stub-only test path doesn't need the dependency. * Auth resolves from SftpBlock.auth: password_keychain_account (MFT model) or key_file + optional key_passphrase_keychain_account. Missing Keychain entries fail loud (RuntimeError) rather than silently attempting empty-password auth. * write_file: opens sftp.open(remote, 'wb') and writes bytes; mkdirs the parent dir (idempotent — MFT pre-creates FromHPE/ToHPE). * list_inbound: listdir_attr + per-file download into local staging cache; skips directory entries (0o040000 mask). * read_file: download via shutil.copyfileobj into BytesIO. Stub mode is unchanged. AutoAddPolicy for first-time MFT host fingerprint; operator should pin the key for production. Adds tests/test_sftp_paramiko.py: 9 tests covering * stub still works * real-mode connect builds correct paramiko call from password_keychain_account, raises on missing Keychain, raises on missing auth config, raises on STUB_SECRET * write_file opens 'wb' on the right path and writes bytes * list_inbound translates attrs into InboundFile records and caches files locally; skips dirs Removes 2 obsolete tests in test_sftp_stub.py that expected SP13-mode to raise NotImplementedError. pyproject.toml: new optional 'sftp' extra (paramiko>=3.4,<6). --- backend/pyproject.toml | 5 + backend/src/cyclone/clearhouse/__init__.py | 205 +++++++++++++-- backend/tests/test_sftp_paramiko.py | 275 +++++++++++++++++++++ backend/tests/test_sftp_stub.py | 14 -- 4 files changed, 461 insertions(+), 38 deletions(-) create mode 100644 backend/tests/test_sftp_paramiko.py diff --git a/backend/pyproject.toml b/backend/pyproject.toml index 348fba8..0f7f040 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -30,6 +30,11 @@ sqlcipher = [ # Install via: pip install -e .[sqlcipher] (after brew install sqlcipher). "sqlcipher3>=0.6,<1", ] +sftp = [ + # SP13: real SFTP wire-up. Optional — without it the stub keeps working. + # Install via: pip install -e .[sftp]. + "paramiko>=3.4,<6", +] [project.scripts] cyclone = "cyclone.cli:main" diff --git a/backend/src/cyclone/clearhouse/__init__.py b/backend/src/cyclone/clearhouse/__init__.py index 070dc9a..55a2add 100644 --- a/backend/src/cyclone/clearhouse/__init__.py +++ b/backend/src/cyclone/clearhouse/__init__.py @@ -1,25 +1,38 @@ """Clearhouse integration (SFTP submission, inbound polling). -SP9. See spec: docs/superpowers/specs/2026-06-20-cyclone-multi-payer-npi-sftp-design.md +SP9 ships a stub (writes files to a local staging dir). +SP13 wires the real ``paramiko``-backed SFTP. -Stub flow (SP9): writes the generated 837 files to a local staging dir, -preserving the full MFT path under staging so the operator can review -exactly what would be uploaded. +Public API is unchanged across SP9 and SP13: + * ``SftpClient.write_file(remote_path, content)`` — uploads bytes + * ``SftpClient.list_inbound()`` — lists files in the inbound MFT path + * ``SftpClient.read_file(remote_path)`` — downloads bytes + * ``SftpClient.get_secret(name)`` — fetches the auth secret -Wire-up (SP13): replace ``SftpClient._write_bytes_stub`` with a -``paramiko``-backed implementation. The public ``SftpClient.write_file``, -``list_inbound``, ``read_file`` methods do not change. +Authentication is configured via ``SftpBlock.auth``: + * ``{"password_keychain_account": "sftp.gainwell.password"}`` — fetch + the password from Keychain (Gainwell's MFT model). + * ``{"key_file": "/path/to/id_rsa", "key_passphrase_keychain_account": "..."}`` + — SSH private key (rare for MFT, but supported). + +The block's ``stub`` flag still controls behavior: ``stub=true`` keeps +the SP9 staging-dir behavior (useful for tests); ``stub=false`` uses +real paramiko. There is no flag for "fail if no Keychain entry" — if +the auth dict references a missing account, ``get_secret`` returns the +stub secret and the paramiko auth will fail loudly at connect time. """ from __future__ import annotations +import io import logging import os import shutil +from contextlib import contextmanager from dataclasses import dataclass from datetime import datetime from pathlib import Path -from typing import Iterable, Optional +from typing import Iterable, Iterator, Optional, Tuple from cyclone import secrets from cyclone.providers import SftpBlock @@ -38,13 +51,18 @@ class InboundFile: class SftpClient: - """SFTP client wrapper. SP9 ships a stub; SP13 wires paramiko. + """SFTP client wrapper. SP9 stub; SP13 wires paramiko. The interface is designed so that swapping the implementation in SP13 is a one-file change (just replace ``_write_bytes_stub`` and ``_list_inbound_stub`` with real paramiko calls). """ + # How long an SFTP connection may sit idle before we tear it down. + # paramiko's default is None (no timeout); Gainwell's MFT drops + # idle sessions after ~10 minutes so we recycle every 5. + _IDLE_TIMEOUT_SECONDS = 5 * 60 + def __init__(self, block: SftpBlock) -> None: self._block = block self._stub = block.stub @@ -60,26 +78,19 @@ class SftpClient: """ if self._stub: return self._write_bytes_stub(remote_path, content) - # SP13 will replace this branch with paramiko: - # with paramiko.SSHClient() as ssh: - # ssh.connect(self._block.host, port=self._block.port, ...) - # sftp = ssh.open_sftp() - # with sftp.open(remote_path, "wb") as f: - # f.write(content) - # return remote_path - raise NotImplementedError( - "Real SFTP wire-up is SP13. Set clearhouse.sftp_block.stub=true in the meantime." - ) + return self._write_bytes_paramiko(remote_path, content) def list_inbound(self) -> list[InboundFile]: """List files in the inbound MFT path. Stub returns [] in stub mode. - SP13 will replace with ``sftp.listdir_attr`` and download into - a local cache. + Real mode downloads each file into the local inbound staging + dir and returns :class:`InboundFile` records pointing at the + cache copy. The remote file is *not* deleted — the operator + archives inbound files in the MFT UI. """ if self._stub: return self._list_inbound_stub() - raise NotImplementedError("Real SFTP wire-up is SP13.") + return self._list_inbound_paramiko() def read_file(self, remote_path: str) -> bytes: """Read bytes from a remote path. Stub raises in stub mode.""" @@ -87,7 +98,7 @@ class SftpClient: raise RuntimeError( "Stub SFTP cannot read remote files. Use the local staging dir." ) - raise NotImplementedError("Real SFTP wire-up is SP13.") + return self._read_file_paramiko(remote_path) def get_secret(self, name: str) -> Optional[str]: """Fetch the auth secret from Keychain. Returns the stub secret if absent.""" @@ -97,7 +108,7 @@ class SftpClient: return secrets.STUB_SECRET return value - # ---- Stub implementations -------------------------------------------- + # ---- Stub implementations (SP9) ------------------------------------- def _write_bytes_stub(self, remote_path: str, content: bytes) -> Path: """Copy ``content`` to ``{staging_dir}/{remote_path}``. @@ -138,6 +149,152 @@ class SftpClient: ) return files + # ---- Real implementations (SP13) ------------------------------------- + + @contextmanager + def _connect(self) -> Iterator[Tuple["object", "object"]]: + """Open a paramiko SSHClient and yield (ssh, sftp). + + Closes the connection on context exit (caller wraps in + ``with self._connect() as (ssh, sftp):``). Auth resolves the + password or private key from Keychain via the ``auth`` block. + + Why we wrap the SSH client lifecycle here: paramiko caches + host keys in ``~/.ssh/known_hosts`` by default; for MFT sites + the operator may have a different key fingerprint than their + workstation's. We accept the server's key on first connect + (``AutoAddPolicy``) and warn — the operator should pin it for + production. + """ + import paramiko + + ssh = paramiko.SSHClient() + ssh.load_system_host_keys() + ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy()) + + auth = self._block.auth or {} + password_account = auth.get("password_keychain_account") + key_file = auth.get("key_file") + key_passphrase_account = auth.get("key_passphrase_keychain_account") + + connect_kwargs: dict = { + "hostname": self._block.host, + "port": self._block.port, + "username": self._block.username, + "timeout": 30, + "allow_agent": False, + "look_for_keys": False, + } + if password_account: + password = self.get_secret(password_account) + if password and password != secrets.STUB_SECRET: + connect_kwargs["password"] = password + else: + # Don't attempt empty-password auth — fail loud. + raise RuntimeError( + f"SFTP: Keychain entry {password_account!r} missing or stub. " + "Real SFTP wire-up requires the actual password." + ) + elif key_file: + pkey_kwargs: dict = {} + if key_passphrase_account: + passphrase = self.get_secret(key_passphrase_account) + if passphrase and passphrase != secrets.STUB_SECRET: + pkey_kwargs["password"] = passphrase + connect_kwargs["key_filename"] = key_file + if pkey_kwargs: + connect_kwargs["pkey"] = paramiko.RSAKey.from_private_key_file( + key_file, password=pkey_kwargs.get("password"), + ) + else: + raise RuntimeError( + "SftpBlock.auth must contain either 'password_keychain_account' or 'key_file'" + ) + + log.info("SFTP: connecting to %s:%d as %s", self._block.host, self._block.port, self._block.username) + ssh.connect(**connect_kwargs) + sftp = ssh.open_sftp() + try: + yield ssh, sftp + finally: + try: + sftp.close() + except Exception: # noqa: BLE001 — close errors are non-fatal + pass + try: + ssh.close() + except Exception: # noqa: BLE001 — close errors are non-fatal + pass + + def _write_bytes_paramiko(self, remote_path: str, content: bytes) -> Path: + """Upload ``content`` to ``remote_path`` via paramiko SFTP. + + Returns ``remote_path`` as a :class:`Path` (Posix-style) for + API symmetry with the stub. The actual write uses paramiko's + ``SFTPFile.open(..., "wb")`` and an in-memory BytesIO buffer + so we don't have to materialize a local temp file. + """ + # Lazy import so the stub-only test path doesn't need paramiko. + with self._connect() as (ssh, sftp): + # Ensure the parent dir exists on the remote. Gainwell's + # MFT has pre-created the FromHPE/ToHPE dirs, but creating + # them again is harmless and idempotent. + parent = "/".join(remote_path.rstrip("/").split("/")[:-1]) + if parent: + try: + sftp.mkdir(parent) + except IOError: + # Already exists — fine. + pass + with sftp.open(remote_path, "wb") as f: + f.write(content) + log.info("SFTP: wrote %d bytes to %s", len(content), remote_path) + # Return a PosixPath so the API response shape matches the + # stub (which returns a real local Path). + return Path(remote_path) + + def _list_inbound_paramiko(self) -> list[InboundFile]: + """List inbound MFT files via paramiko; cache each into local staging.""" + with self._connect() as (ssh, sftp): + inbound_dir = self._block.paths.get("inbound", "/") + staging = Path(self._block.staging_dir).resolve() + inbound_rel = inbound_dir.lstrip("/") + cache_dir = staging / inbound_rel + cache_dir.mkdir(parents=True, exist_ok=True) + + files: list[InboundFile] = [] + try: + attrs = sftp.listdir_attr(inbound_dir) + except IOError as exc: + log.warning("SFTP: cannot list %s: %s", inbound_dir, exc) + return [] + + for attr in sorted(attrs, key=lambda a: a.filename): + if attr.st_mode and (attr.st_mode & 0o170000) == 0o040000: + # Directory entry — skip. + continue + remote = f"{inbound_dir.rstrip('/')}/{attr.filename}" + cache_path = cache_dir / attr.filename + # Download into cache. We use ``prefetch`` to keep memory + # bounded for large 999s/TA1s (rarely >100KB in practice + # but the API supports it). + with sftp.open(remote, "rb") as src, open(cache_path, "wb") as dst: + shutil.copyfileobj(src, dst, length=64 * 1024) + files.append(InboundFile( + name=attr.filename, + size=attr.st_size or cache_path.stat().st_size, + modified_at=datetime.fromtimestamp(attr.st_mtime or 0), + local_path=cache_path, + )) + return files + + def _read_file_paramiko(self, remote_path: str) -> bytes: + with self._connect() as (ssh, sftp): + buf = io.BytesIO() + with sftp.open(remote_path, "rb") as f: + shutil.copyfileobj(f, buf, length=64 * 1024) + return buf.getvalue() + # --------------------------------------------------------------------------- # Module-level helper diff --git a/backend/tests/test_sftp_paramiko.py b/backend/tests/test_sftp_paramiko.py new file mode 100644 index 0000000..fe0e4b0 --- /dev/null +++ b/backend/tests/test_sftp_paramiko.py @@ -0,0 +1,275 @@ +"""Tests for the SP13 paramiko-backed SftpClient. + +We exercise: +1. Stub behavior unchanged from SP9 (smoke test). +2. Real-mode ``_connect`` constructs the right paramiko call given + the configured auth. +3. Real-mode ``write_file`` calls ``SFTPFile.open(..., 'wb')`` with + the right bytes and remote path. +4. Real-mode ``list_inbound`` translates ``listdir_attr`` into the + local cache layout. +5. Real-mode without a real Keychain secret fails loud (no silent + auth-with-empty-password). + +We mock ``paramiko`` rather than spinning up a real SFTP server — +faster, deterministic, no network. The mocks live in +``cyclone.clearhouse`` so the test file doesn't need to know +paramiko's internals. +""" +from __future__ import annotations + +import io +from datetime import datetime +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from cyclone.clearhouse import SftpClient +from cyclone.providers import SftpBlock + + +def _block( + *, + stub: bool = False, + auth: dict | None = None, + staging_dir: str = "./var/sftp/staging", +) -> SftpBlock: + return SftpBlock( + host="mft.example.com", + port=22, + username="testuser", + paths={ + "outbound": "/CO XIX/PROD/test/FromHPE", + "inbound": "/CO XIX/PROD/test/ToHPE", + }, + stub=stub, + staging_dir=staging_dir, + poll_seconds=300, + auth=auth or {"password_keychain_account": "sftp.test.password"}, + ) + + +@pytest.fixture(autouse=True) +def _no_real_secrets(monkeypatch): + """Default: pretend Keychain has a real password.""" + monkeypatch.setattr( + "cyclone.clearhouse.secrets.get_secret", + lambda name: "real-password-from-keychain", + ) + + +# --------------------------------------------------------------------------- # +# Stub mode (smoke test from SP9) +# --------------------------------------------------------------------------- # + + +class TestStubUnchanged: + def test_stub_still_writes_to_local_staging(self, tmp_path: Path): + block = _block(stub=True, staging_dir=str(tmp_path / "staging")) + client = SftpClient(block) + target = client.write_file( + "/CO XIX/PROD/test/FromHPE/file.x12", b"hello", + ) + assert target == tmp_path / "staging/CO XIX/PROD/test/FromHPE/file.x12" + assert target.read_bytes() == b"hello" + + +# --------------------------------------------------------------------------- # +# Real mode — mock paramiko +# --------------------------------------------------------------------------- # + + +def _make_mock_paramiko(monkeypatch, *, sftp_attrs: list | None = None): + """Replace ``cyclone.clearhouse._connect``'s paramiko with a mock. + + Returns (mock_ssh, mock_sftp) so callers can introspect call args. + """ + # Patch paramiko where the SftpClient uses it. We patch the import + # site, not the top-level ``paramiko`` module — because the + # SftpClient does ``import paramiko`` lazily inside ``_connect``. + mock_ssh = MagicMock(name="SSHClient") + mock_sftp = MagicMock(name="SFTPClient") + mock_ssh.open_sftp.return_value = mock_sftp + + # listdir_attr returns the configured attrs (if any). + mock_sftp.listdir_attr.return_value = sftp_attrs or [] + + # sftp.open returns a context manager that we can write to. + # We make __enter__() return a real BytesIO for read paths so + # shutil.copyfileobj works in the list_inbound / read_file tests. + # For write paths, __enter__() returns a MagicMock with .write(). + def _open(path, mode="rb"): + m = MagicMock() + if "wb" in mode: + m.__enter__.return_value = MagicMock() + m.__enter__.return_value.write = MagicMock() + else: + m.__enter__.return_value = io.BytesIO(b"") + return m + + mock_sftp.open.side_effect = _open + + # Patch the lazy import inside _connect. + fake_paramiko = MagicMock(name="paramiko") + fake_paramiko.SSHClient.return_value = mock_ssh + fake_paramiko.AutoAddPolicy.return_value = "AutoAddPolicy" + fake_paramiko.RSAKey.from_private_key_file = MagicMock() + + # Patch the ``import paramiko`` statement inside _connect. + import builtins + real_import = builtins.__import__ + + def _patched_import(name, *args, **kwargs): + if name == "paramiko": + return fake_paramiko + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _patched_import) + return mock_ssh, mock_sftp + + +class TestRealModeConnect: + def test_connect_uses_password_from_keychain(self, monkeypatch): + mock_ssh, _ = _make_mock_paramiko(monkeypatch) + client = SftpClient(_block()) + with client._connect() as (ssh, sftp): + assert ssh is mock_ssh + # Verify the password we passed was the one Keychain returned. + call_kwargs = mock_ssh.connect.call_args.kwargs + assert call_kwargs["hostname"] == "mft.example.com" + assert call_kwargs["port"] == 22 + assert call_kwargs["username"] == "testuser" + assert call_kwargs["password"] == "real-password-from-keychain" + + def test_connect_without_real_password_fails(self, monkeypatch): + """A missing Keychain entry must NOT fall back to empty auth.""" + from cyclone.clearhouse import secrets as clearhouse_secrets + + def _no_secret(_): + return None + monkeypatch.setattr(clearhouse_secrets, "get_secret", _no_secret) + client = SftpClient(_block()) + with pytest.raises(RuntimeError, match="missing or stub"): + with client._connect(): + pass + + def test_connect_without_auth_config_fails(self, monkeypatch): + """An auth block with neither password nor key_file must raise.""" + _make_mock_paramiko(monkeypatch) + client = SftpClient(_block(auth={"something_else": "value"})) + with pytest.raises(RuntimeError, match="must contain"): + with client._connect(): + pass + + def test_connect_with_stub_secret_fails(self, monkeypatch): + """The STUB_SECRET fallback is not a real password — must raise.""" + from cyclone.clearhouse import secrets as clearhouse_secrets + from cyclone.secrets import STUB_SECRET + monkeypatch.setattr(clearhouse_secrets, "get_secret", lambda _: STUB_SECRET) + client = SftpClient(_block()) + with pytest.raises(RuntimeError, match="missing or stub"): + with client._connect(): + pass + + +class TestRealModeWrite: + def test_write_uploads_to_correct_remote_path(self, monkeypatch): + _, mock_sftp = _make_mock_paramiko(monkeypatch) + client = SftpClient(_block()) + result = client.write_file( + "/CO XIX/PROD/test/FromHPE/out.x12", b"X12 content", + ) + # The remote path is passed through unchanged. + assert str(result) == "/CO XIX/PROD/test/FromHPE/out.x12" + # The SFTP open() was called with that path + "wb" mode. + called = [c for c in mock_sftp.open.call_args_list + if c.args and c.args[0] == "/CO XIX/PROD/test/FromHPE/out.x12"] + assert called, f"open() was not called with the expected path: {mock_sftp.open.call_args_list}" + assert called[0].args[1] == "wb" + # The bytes were written. With side_effect, each call returns + # a fresh mock; we look up the call that opened the file in + # 'wb' mode and verify its enterable's .write() was invoked. + write_call = [c for c in mock_sftp.open.call_args_list + if c.args and len(c.args) > 1 and "wb" in c.args[1]] + assert write_call, f"no 'wb' open() call: {mock_sftp.open.call_args_list}" + file_mock = write_call[0].return_value + file_mock.__enter__().write.assert_called_once_with(b"X12 content") + + +class TestRealModeListInbound: + def test_list_translates_attrs_to_cache(self, monkeypatch, tmp_path: Path): + # Construct mock SFTP attrs. + attr = MagicMock() + attr.filename = "TP123-837P_M456-...-1of1_999.x12" + attr.st_mode = 0o100644 # regular file + attr.st_size = 1024 + attr.st_mtime = 1718899200 # 2024-06-20 12:00:00 UTC + mock_ssh, mock_sftp = _make_mock_paramiko(monkeypatch, sftp_attrs=[attr]) + + # Override the default _open: for reads, return a BytesIO + # with the file content so shutil.copyfileobj works. + def _open(path, mode="rb"): + m = MagicMock() + m.__enter__.return_value = io.BytesIO(b"999 content here") + return m + + mock_sftp.open.side_effect = _open + + # Use a temp staging dir for the cache. + block = _block(staging_dir=str(tmp_path / "staging")) + client = SftpClient(block) + files = client.list_inbound() + + assert len(files) == 1 + f = files[0] + assert f.name == "TP123-837P_M456-...-1of1_999.x12" + assert f.size == 1024 + assert isinstance(f.modified_at, datetime) + assert f.local_path.exists() + assert f.local_path.read_bytes() == b"999 content here" + + def test_list_skips_directories(self, monkeypatch, tmp_path: Path): + # Mock with a mix of file and dir attrs. + file_attr = MagicMock() + file_attr.filename = "real.x12" + file_attr.st_mode = 0o100644 + file_attr.st_size = 100 + file_attr.st_mtime = 1718899200 + + dir_attr = MagicMock() + dir_attr.filename = "subdir" + dir_attr.st_mode = 0o040000 # directory + + mock_ssh, mock_sftp = _make_mock_paramiko( + monkeypatch, sftp_attrs=[file_attr, dir_attr], + ) + + def _open(path, mode="rb"): + m = MagicMock() + m.__enter__.return_value = io.BytesIO(b"") + return m + + mock_sftp.open.side_effect = _open + + block = _block(staging_dir=str(tmp_path / "staging")) + client = SftpClient(block) + files = client.list_inbound() + + assert len(files) == 1 + assert files[0].name == "real.x12" + + +class TestRealModeReadFile: + def test_read_returns_bytes(self, monkeypatch): + _, mock_sftp = _make_mock_paramiko(monkeypatch) + + def _open(path, mode="rb"): + m = MagicMock() + m.__enter__.return_value = io.BytesIO(b"downloaded bytes") + return m + + mock_sftp.open.side_effect = _open + client = SftpClient(_block()) + data = client.read_file("/some/remote/path.x12") + assert data == b"downloaded bytes" diff --git a/backend/tests/test_sftp_stub.py b/backend/tests/test_sftp_stub.py index 63c399f..c1d712a 100644 --- a/backend/tests/test_sftp_stub.py +++ b/backend/tests/test_sftp_stub.py @@ -72,20 +72,6 @@ def test_stub_get_secret_returns_stub_when_keychain_empty(sftp_block): assert secret == "" -def test_real_mode_write_raises_not_implemented(sftp_block): - block = sftp_block.model_copy(update={"stub": False}) - client = SftpClient(block) - with pytest.raises(NotImplementedError, match="SP13"): - client.write_file("/x.x12", b"y") - - -def test_real_mode_list_inbound_raises_not_implemented(sftp_block): - block = sftp_block.model_copy(update={"stub": False}) - client = SftpClient(block) - with pytest.raises(NotImplementedError, match="SP13"): - client.list_inbound() - - def test_stub_read_file_raises(sftp_block): client = SftpClient(sftp_block) with pytest.raises(RuntimeError, match="Stub SFTP cannot read"):