From 8cb3f295bdc6b56c6bdc5d7490ea7cee04dc24ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johnny=20Marie=CC=81thoz?= Date: Thu, 3 Sep 2026 08:06:29 +0200 Subject: [PATCH 1/2] fix(snl): repair the SFTP repository and drop the sftpretty wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * The switch from `pysftp` to `sftpretty` only swapped the import, so `SNLRepository` kept calling methods that no longer exist or that silently changed behaviour: `urn snl-list-files` failed with an `AttributeError` on `walktree()`, `cwd()` called the `cd()` context manager and did nothing, and `mkdir()` created the directories on the SNL server with mode 700 instead of 777. * The repository only needs five SFTP operations, so it now talks to `paramiko` directly, the engine both wrappers are built on: one dependency less and no wrapper API to track. The SSH agent and the local keys are ignored, to keep the password authentication expected by the SNL server. * The host key of the server is still verified, and can now be given by `SONAR_APP_FTP_SNL_HOST_KEY` as a `known_hosts` line. A container has no `known_hosts` file to rely on, and the key is public data: it belongs to the deployment configuration, next to the host name. * `make_dir()` leaves an existing directory untouched, so an upload can be replayed on an URN directory that was already created. * The remote tree is walked by a `list_files()` generator built on `listdir_attr()`. * `urn snl-list-files` reports an empty server instead of printing nothing, and both `urn snl-*` commands close the connection once they are done. * Restores the unit tests of the repository, dropped when the class was migrated from `ftplib` to `pysftp`. Co-Authored-By: Johnny Mariéthoz Co-Authored-By: Claude Opus 5 (1M context) --- pyproject.toml | 2 +- sonar/config_sonar.py | 3 + sonar/modules/documents/cli/urn.py | 40 ++++++--- sonar/snl/ftp/__init__.py | 50 +++++++++-- tests/ui/documents/test_urn_cli.py | 29 ++++-- tests/unit/snl/__init__.py | 4 + tests/unit/snl/ftp/__init__.py | 4 + tests/unit/snl/ftp/test_snl_repository.py | 102 ++++++++++++++++++++++ uv.lock | 16 +--- 9 files changed, 209 insertions(+), 41 deletions(-) create mode 100644 tests/unit/snl/__init__.py create mode 100644 tests/unit/snl/ftp/__init__.py create mode 100644 tests/unit/snl/ftp/test_snl_repository.py diff --git a/pyproject.toml b/pyproject.toml index e7132b126..dda9be0da 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -73,7 +73,7 @@ dependencies = [ "netaddr>=0.9.0", "wand>=0.6.13", "webdavclient3>=3.14.6", - "sftpretty>=1.0.0", + "paramiko>=5.0.0", "python-slugify", "orcid>=1.0.3", "python3-saml>=1.16.0", diff --git a/sonar/config_sonar.py b/sonar/config_sonar.py index 2fbd47540..99c082edc 100644 --- a/sonar/config_sonar.py +++ b/sonar/config_sonar.py @@ -544,6 +544,9 @@ SONAR_APP_FTP_SNL_USER = "changeMe" SONAR_APP_FTP_SNL_PASSWORD = "changeMe" SONAR_APP_FTP_SNL_PATH = "changeMe" +# Public host key of the SNL server, as a `known_hosts` line. When it is not +# set, the host key is looked up in the `known_hosts` file of the user. +SONAR_APP_FTP_SNL_HOST_KEY = "" SONAR_APP_SNL_EMAIL_TEMPLATE = "sonar/modules/documents/templates/documents/emailSNL.txt" """FTP connection to SNL server.""" diff --git a/sonar/modules/documents/cli/urn.py b/sonar/modules/documents/cli/urn.py index 8572ee782..bc60bc36b 100644 --- a/sonar/modules/documents/cli/urn.py +++ b/sonar/modules/documents/cli/urn.py @@ -82,6 +82,20 @@ def register(): click.secho(f"{idx} URN registered.", fg="green") +def create_snl_repository(): + """Create a SNL repository from the application configuration. + + :returns: SNLRepository object. + """ + return SNLRepository( + host=current_app.config.get("SONAR_APP_FTP_SNL_HOST"), + user=current_app.config.get("SONAR_APP_FTP_SNL_USER"), + password=current_app.config.get("SONAR_APP_FTP_SNL_PASSWORD"), + directory=current_app.config.get("SONAR_APP_FTP_SNL_PATH"), + host_key=current_app.config.get("SONAR_APP_FTP_SNL_HOST_KEY"), + ) + + @urn.command("snl-upload-file") @click.argument("urn_code") @with_appcontext @@ -112,12 +126,7 @@ def snl_upload_file(urn_code): click.secho("Error: the document does not contains any files.") return - snl_repository = SNLRepository( - host=current_app.config.get("SONAR_APP_FTP_SNL_HOST"), - user=current_app.config.get("SONAR_APP_FTP_SNL_USER"), - password=current_app.config.get("SONAR_APP_FTP_SNL_PASSWORD"), - directory=current_app.config.get("SONAR_APP_FTP_SNL_PATH"), - ) + snl_repository = create_snl_repository() snl_repository.connect() dnb_base_urn = current_app.config.get("SONAR_APP_FTP_SNL_PATH") @@ -134,6 +143,8 @@ def snl_upload_file(urn_code): except Exception as exception: click.secho(str(exception), fg="red") + snl_repository.close() + # print email template template_email_snl = current_app.config.get("SONAR_APP_SNL_EMAIL_TEMPLATE") @@ -149,14 +160,17 @@ def snl_upload_file(urn_code): @with_appcontext def snl_list_files(): """List files uploaded on SNL server.""" - snl_repository = SNLRepository( - host=current_app.config.get("SONAR_APP_FTP_SNL_HOST"), - user=current_app.config.get("SONAR_APP_FTP_SNL_USER"), - password=current_app.config.get("SONAR_APP_FTP_SNL_PASSWORD"), - directory=current_app.config.get("SONAR_APP_FTP_SNL_PATH"), - ) + snl_repository = create_snl_repository() snl_repository.connect() - snl_repository.client.walktree(".", lambda x: click.secho(x), lambda x: click.secho(x), lambda x: click.secho(x)) + files = list(snl_repository.list_files()) + snl_repository.close() + + if not files: + click.secho("No file found on SNL server.", fg="yellow") + return + + for path in files: + click.secho(path) @urn.command() diff --git a/sonar/snl/ftp/__init__.py b/sonar/snl/ftp/__init__.py index b661f72db..bec7e8d3d 100644 --- a/sonar/snl/ftp/__init__.py +++ b/sonar/snl/ftp/__init__.py @@ -3,41 +3,61 @@ """SNL FTP repository.""" -from sftpretty import Connection +import os +from stat import S_ISDIR + +from paramiko import SSHClient +from paramiko.hostkeys import HostKeyEntry class SNLRepository: """SNL FTP repository.""" - def __init__(self, host, user, password, directory): + def __init__(self, host, user, password, directory, host_key=None): """Init class. :param host: FTP host. :param user: FTP user. :param password: FTP password. :param directory: Directory where files are stored. + :param host_key: Host key of the FTP server, as a `known_hosts` line. """ self.host = host self.user = user self.password = password self.directory = directory + self.host_key = host_key def connect(self): - """Connect to FTP server and change directory.""" - self.client = Connection( + """Connect to FTP server and change directory. + + The host key must be known, otherwise the connection is rejected. + """ + self.ssh = SSHClient() + self.ssh.load_system_host_keys() + if entry := HostKeyEntry.from_line((self.host_key or "").strip()): + for name in entry.hostnames: + self.ssh.get_host_keys().add(name, entry.key.get_name(), entry.key) + self.ssh.connect( self.host, username=self.user, password=self.password, - default_path=self.directory, + allow_agent=False, + look_for_keys=False, ) + self.client = self.ssh.open_sftp() + self.client.chdir(self.directory) def make_dir(self, pathname): - """Make new directory via FTP connection.""" - self.client.mkdir(pathname) + """Make new directory via FTP connection, if it does not exist yet.""" + try: + self.client.stat(pathname) + except FileNotFoundError: + self.client.mkdir(pathname, mode=0o777) def cwd(self, pathname): """Move to directory via FTP connection.""" - self.client.cd(pathname) + self.client.chdir(pathname) def upload_file(self, file_path, file_name): """Upload file to SNL server via FTP connection. @@ -46,6 +66,20 @@ def upload_file(self, file_path, file_name): """ self.client.put(file_path, file_name) + def list_files(self, pathname="."): + """Recursively list files stored in a directory via FTP connection. + + :param pathname: remote directory to walk through. + :returns: generator of remote file paths. + """ + for attribute in self.client.listdir_attr(pathname): + path = os.path.join(pathname, attribute.filename) + if S_ISDIR(attribute.st_mode): + yield from self.list_files(path) + else: + yield path + def close(self): """Close FTP connection.""" self.client.close() + self.ssh.close() diff --git a/tests/ui/documents/test_urn_cli.py b/tests/ui/documents/test_urn_cli.py index 9c52e943c..19c72c4d5 100644 --- a/tests/ui/documents/test_urn_cli.py +++ b/tests/ui/documents/test_urn_cli.py @@ -9,16 +9,17 @@ from click.testing import CliRunner from invenio_pidstore.providers.base import BaseProvider -from sonar.modules.documents.cli.urn import snl_upload_file +from sonar.modules.documents.cli.urn import snl_list_files, snl_upload_file from sonar.snl.ftp import SNLRepository -@mock.patch("sonar.snl.ftp.Connection", autospec=True) -def test_snl_upload_file(mock_ftp_constructor, app, script_info, minimal_thesis_document_with_urn): +@mock.patch("sonar.snl.ftp.SSHClient", autospec=True) +def test_snl_upload_file(mock_ssh_constructor, app, script_info, minimal_thesis_document_with_urn): """Test upload file.""" app.config["SONAR_APP_FTP_SNL_PATH"] = "/rero" - mock_ftp = mock_ftp_constructor.return_value + mock_ftp = mock_ssh_constructor.return_value.open_sftp.return_value + mock_ftp.stat.side_effect = FileNotFoundError repository = SNLRepository("snl_host", "user", "password", "snl_folder") repository.connect() @@ -53,4 +54,22 @@ def test_snl_upload_file(mock_ftp_constructor, app, script_info, minimal_thesis_ obj=script_info, ) assert "Template of email to send to SNL:" in result.output - mock_ftp.mkdir.assert_called_with("/rero/rero-006-17") + mock_ftp.mkdir.assert_called_with("/rero/rero-006-17", mode=0o777) + + +@mock.patch("sonar.snl.ftp.SNLRepository.list_files") +@mock.patch("sonar.snl.ftp.SSHClient", autospec=True) +def test_snl_list_files(mock_ssh_constructor, mock_list_files, app, script_info): + """Test listing of the files stored on the SNL server.""" + mock_list_files.return_value = ["./rero-006-17/test.pdf", "./readme.txt"] + + runner = CliRunner() + result = runner.invoke(snl_list_files, obj=script_info) + + assert result.output == "./rero-006-17/test.pdf\n./readme.txt\n" + + # empty server + mock_list_files.return_value = [] + result = runner.invoke(snl_list_files, obj=script_info) + + assert result.output == "No file found on SNL server.\n" diff --git a/tests/unit/snl/__init__.py b/tests/unit/snl/__init__.py new file mode 100644 index 000000000..9efbba26b --- /dev/null +++ b/tests/unit/snl/__init__.py @@ -0,0 +1,4 @@ +# SPDX-FileCopyrightText: Fondation RERO+ +# SPDX-License-Identifier: AGPL-3.0-or-later + +"""Tests unit snl.""" diff --git a/tests/unit/snl/ftp/__init__.py b/tests/unit/snl/ftp/__init__.py new file mode 100644 index 000000000..581aed4af --- /dev/null +++ b/tests/unit/snl/ftp/__init__.py @@ -0,0 +1,4 @@ +# SPDX-FileCopyrightText: Fondation RERO+ +# SPDX-License-Identifier: AGPL-3.0-or-later + +"""Tests unit snl ftp.""" diff --git a/tests/unit/snl/ftp/test_snl_repository.py b/tests/unit/snl/ftp/test_snl_repository.py new file mode 100644 index 000000000..a143ebf54 --- /dev/null +++ b/tests/unit/snl/ftp/test_snl_repository.py @@ -0,0 +1,102 @@ +# SPDX-FileCopyrightText: Fondation RERO+ +# SPDX-License-Identifier: AGPL-3.0-or-later + +"""Test SNL FTP repository.""" + +from stat import S_IFDIR, S_IFREG +from unittest import mock + +import pytest +from paramiko import SFTPAttributes + +from sonar.snl.ftp import SNLRepository + + +def sftp_attribute(filename, mode): + """Build a remote directory entry. + + :param filename: name of the entry. + :param mode: stat mode of the entry. + :returns: SFTPAttributes object. + """ + attribute = SFTPAttributes() + attribute.filename = filename + attribute.st_mode = mode + return attribute + + +@pytest.fixture +def repository(): + """Return a repository connected to a mocked SFTP server.""" + with mock.patch("sonar.snl.ftp.SSHClient", autospec=True): + repository = SNLRepository("snl_host", "user", "password", "/snl_folder") + repository.connect() + yield repository + + +SNL_HOST_KEY = "snl_host ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPZqFuISvMFSw/cZXBVt/AnjFXbb0N+xbwx0ZMzn3x6h" + + +@mock.patch("sonar.snl.ftp.SSHClient", autospec=True) +def test_connect(mock_ssh_client): + """Test connection to the SNL server, host key taken from known_hosts.""" + repository = SNLRepository("snl_host", "user", "password", "/snl_folder") + repository.connect() + + repository.ssh.load_system_host_keys.assert_called_with() + repository.ssh.get_host_keys.return_value.add.assert_not_called() + repository.ssh.connect.assert_called_with( + "snl_host", username="user", password="password", allow_agent=False, look_for_keys=False + ) + repository.client.chdir.assert_called_with("/snl_folder") + + +@mock.patch("paramiko.SSHClient.open_sftp") +@mock.patch("paramiko.SSHClient.connect") +def test_connect_with_configured_host_key(mock_connect, mock_open_sftp): + """Test connection to the SNL server, host key given by the configuration.""" + repository = SNLRepository("snl_host", "user", "password", "/snl_folder", host_key=f"\n{SNL_HOST_KEY}\n") + repository.connect() + + host_keys = repository.ssh.get_host_keys().lookup("snl_host") + assert list(host_keys) == ["ssh-ed25519"] + + +def test_make_dir(repository): + """Test directory creation, an existing directory is left untouched.""" + repository.make_dir("/snl_folder/rero-006-17") + repository.client.mkdir.assert_not_called() + + repository.client.stat.side_effect = FileNotFoundError + repository.make_dir("/snl_folder/rero-006-17") + repository.client.mkdir.assert_called_with("/snl_folder/rero-006-17", mode=0o777) + + +def test_cwd(repository): + """Test directory change.""" + repository.cwd("/snl_folder/rero-006-17") + repository.client.chdir.assert_called_with("/snl_folder/rero-006-17") + + +def test_upload_file(repository): + """Test file upload.""" + repository.upload_file("/tmp/test.pdf", "/snl_folder/rero-006-17/test.pdf") + repository.client.put.assert_called_with("/tmp/test.pdf", "/snl_folder/rero-006-17/test.pdf") + + +def test_list_files(repository): + """Test recursive listing of the stored files.""" + tree = { + ".": [sftp_attribute("rero-006-17", S_IFDIR), sftp_attribute("readme.txt", S_IFREG)], + "./rero-006-17": [sftp_attribute("test.pdf", S_IFREG)], + } + repository.client.listdir_attr.side_effect = lambda pathname: tree[pathname] + + assert list(repository.list_files()) == ["./rero-006-17/test.pdf", "./readme.txt"] + + +def test_close(repository): + """Test connection closing.""" + repository.close() + repository.client.close.assert_called_with() + repository.ssh.close.assert_called_with() diff --git a/uv.lock b/uv.lock index deff6b2b1..08c61a985 100644 --- a/uv.lock +++ b/uv.lock @@ -3636,18 +3636,6 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/95/9c/c510029fc6ef33a6275cd2c5d3cecd6613dfd6aa401d57c54f1c18852ccf/setuptools-84.0.0-py3-none-any.whl", hash = "sha256:51a52592b3b99e102b609654876bd65f19f999935166d1352678931132b0c670", size = 818216, upload-time = "2026-08-08T18:27:56.719Z" }, ] -[[package]] -name = "sftpretty" -version = "1.2.2" -source = { registry = "https://pypi.org/simple" } -dependencies = [ - { name = "paramiko" }, -] -sdist = { url = "https://files.pythonhosted.org/packages/6e/0f/8ba89d9f8e15855ae11dd4df6e8558e4e2b86457808dee2dcfc49d2c3752/sftpretty-1.2.2.tar.gz", hash = "sha256:d10aaf5effd5d00a4a60ce3da4d817781b2f5cbbe6145da8a09f6733a9cfabbc", size = 32451, upload-time = "2026-05-11T05:18:55.831Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/a0/da/571c9004d788d1d2cd25e3e13aff1a998b3aa69c719e286c25c789e334ca/sftpretty-1.2.2-py3-none-any.whl", hash = "sha256:87687446293ecc095b0a252b18dcc01cc5be1f15e646a84b213d7fa8a4703dc1", size = 21649, upload-time = "2026-05-11T05:18:54.604Z" }, -] - [[package]] name = "shellingham" version = "1.5.4" @@ -3781,6 +3769,7 @@ dependencies = [ { name = "jsonresolver" }, { name = "netaddr" }, { name = "orcid" }, + { name = "paramiko" }, { name = "poethepoet" }, { name = "pycountry" }, { name = "python-dotenv" }, @@ -3788,7 +3777,6 @@ dependencies = [ { name = "python3-saml" }, { name = "rero-invenio-base" }, { name = "rero-invenio-files" }, - { name = "sftpretty" }, { name = "urllib3" }, { name = "wand" }, { name = "webdavclient3" }, @@ -3852,6 +3840,7 @@ requires-dist = [ { name = "jsonresolver" }, { name = "netaddr", specifier = ">=0.9.0" }, { name = "orcid", specifier = ">=1.0.3" }, + { name = "paramiko", specifier = ">=5.0.0" }, { name = "poethepoet" }, { name = "pycountry", specifier = ">=23.12.11" }, { name = "python-dotenv", specifier = ">=0.13.0" }, @@ -3859,7 +3848,6 @@ requires-dist = [ { name = "python3-saml", specifier = ">=1.16.0" }, { name = "rero-invenio-base", specifier = ">=0.3.1" }, { name = "rero-invenio-files", specifier = ">=1.2.0" }, - { name = "sftpretty", specifier = ">=1.0.0" }, { name = "urllib3", specifier = "<2.0.0" }, { name = "wand", specifier = ">=0.6.13" }, { name = "webdavclient3", specifier = ">=3.14.6" }, From 502ab84546b0fbc9741b43963188adb817be38c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johnny=20Marie=CC=81thoz?= Date: Thu, 3 Sep 2026 10:00:42 +0200 Subject: [PATCH 2/2] fix(snl): bound the connection to the SNL server in time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SSHClient.connect` left the TCP connection phase unbounded, so an unreachable server made the command hang on the socket. The banner and the authentication phases are already bounded by the defaults of `Transport`, 15 and 30 seconds. Co-Authored-By: Johnny Mariéthoz Co-Authored-By: Claude Opus 5 (1M context) --- sonar/snl/ftp/__init__.py | 1 + tests/unit/snl/ftp/test_snl_repository.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sonar/snl/ftp/__init__.py b/sonar/snl/ftp/__init__.py index bec7e8d3d..4d60cf304 100644 --- a/sonar/snl/ftp/__init__.py +++ b/sonar/snl/ftp/__init__.py @@ -44,6 +44,7 @@ def connect(self): password=self.password, allow_agent=False, look_for_keys=False, + timeout=30, ) self.client = self.ssh.open_sftp() self.client.chdir(self.directory) diff --git a/tests/unit/snl/ftp/test_snl_repository.py b/tests/unit/snl/ftp/test_snl_repository.py index a143ebf54..cdd05ccdf 100644 --- a/tests/unit/snl/ftp/test_snl_repository.py +++ b/tests/unit/snl/ftp/test_snl_repository.py @@ -46,7 +46,7 @@ def test_connect(mock_ssh_client): repository.ssh.load_system_host_keys.assert_called_with() repository.ssh.get_host_keys.return_value.add.assert_not_called() repository.ssh.connect.assert_called_with( - "snl_host", username="user", password="password", allow_agent=False, look_for_keys=False + "snl_host", username="user", password="password", allow_agent=False, look_for_keys=False, timeout=30 ) repository.client.chdir.assert_called_with("/snl_folder")