Koha::File::Transport::FTP::_list_files parsed the LIST / "ls -l" output
with a non-greedy date capture that only consumed the first of the three
date tokens (the month), leaving the day and time glued to the front of
the filename. Every filename returned for an FTP transport was therefore
prefixed with the listing date, e.g.
-rw-r--r-- 1 user group 1234 Jan 01 12:00 QUOTES_413514.CEQ
=> filename "01 12:00 QUOTES_413514.CEQ"
Callers (Koha::Edifact::Transport, marc_ordering_process.pl) then asked
the server for a file that does not exist, so no files were downloaded,
renamed or deleted over FTP. SFTP and Local transports were unaffected.
This regressed when the EDI transport was moved onto the file_transports
system (bug 38489). The previous Koha::Edifact::Transport::ftp_download
listed files with Net::FTP->ls (NLST, bare names); the refactor switched
to Net::FTP->dir (LIST) so it could return the richer hashref shape used
by the SFTP and Local transports, but the parse was faulty.
The date field in "ls -l" output is a fixed run of three whitespace
separated tokens (Mon DD HH:MM or Mon DD YYYY). Consume it explicitly so
the filename is captured cleanly, preserving spaces and still skipping
directories. The LIST-based approach and the filename/longname/size/perms
return shape are kept so the structure stays consistent across transport
classes (relied on by consumers, including out-of-tree ones).
Test plan:
1) prove t/db_dependent/Koha/File/Transport/FTP.t
2) git checkout Koha/File/Transport/FTP.pm and re-run: the list_files()
subtest fails on the filename/size/perms assertions
3) prove t/db_dependent/Koha/Edifact/Transport.t
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch fixes a critical bug where plain_text_password() was called
in list context during hash construction. When a transport has no
password, the method's bare 'return' statement returns an empty list ()
in list context rather than undef, causing all subsequent key-value
pairs to shift positions. This resulted in malformed connection
parameters being passed to Net::SFTP::Foreign and Net::FTP.
Example of the bug:
my %hash = (
user => 'john',
password => $self->plain_text_password, # Returns () when no password
timeout => 10
);
# Results in: (user => 'john', timeout => 10) instead of
# (user => 'john', password => undef, timeout => 10)
Changes to code:
- Wrap plain_text_password calls with scalar() to force scalar context
in both FTP.pm and SFTP.pm
- Make SFTP key and password authentication mutually exclusive using
ternary operator
- Prefer key-based auth over password when both are available (SFTP)
Test plan:
1. Run t/db_dependent/Koha/File/Transport/FTP.t - should pass
2. Run t/db_dependent/Koha/File/Transport/SFTP.t - should pass
3. Verify tests cover:
- Scalar context behavior with no password
- FTP authentication with proper parameter passing
- SFTP password-only authentication
- SFTP key-only authentication
- SFTP key preference when both key and password exist
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Fix several API inconsistencies in the FTP transport class:
1. Message format: Use standardized `message => $operation` format
instead of hardcoded strings like "Upload succeeded"
2. Operation tracking: Add $operation variable to all methods for
consistent error reporting and logging
3. Error handling: Update _abort_operation() to accept operation
parameter and provide more detailed error payloads
4. Success payloads: Replace empty `detail => ''` payloads with
meaningful operation-specific information (host, port, file paths,
directory info, file counts)
All transport classes now have consistent APIs for:
- Standardized message formats
- Consistent operation tracking
- Uniform error handling
- Informative success payloads
Sponsored-by: ByWater Solutions
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Fix API inconsistency where different transport types returned different
formats from list_files():
- SFTP: returned array of hashrefs with detailed file information
- FTP: returned simple array of filenames
- Local: returned simple array of filenames
Now all transports return an array reference of hashrefs with consistent
structure containing filename, longname, and additional metadata.
This eliminates the need for workarounds like the ref($file) eq 'HASH'
check in EDI code and provides a more robust, consistent API.
Sponsored-by: ByWater Solutions
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds proper connection cleanup functionality to the file
transport system, addressing a gap identified during EDI migration.
Changes:
- Add disconnect() method to base Transport class interface
- Implement SFTP disconnect using connection->disconnect()
- Implement FTP disconnect using connection->quit()
- Add DESTROY methods to ensure connections are cleaned up on object destruction
- Local transport includes no-op disconnect for API consistency
This ensures that FTP/SFTP connections are properly closed, preventing
connection leaks and potential resource exhaustion issues.
Sponsored-by: ByWater Solutions
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch enhances the file transport system with file renaming
capabilities and adds a Local transport class for development/testing.
Changes:
- Add rename_file() method to base Transport class and SFTP/FTP subclasses
- Create Koha::File::Transport::Local for local directory operations
- Add 'local' transport type to polymorphic mapping
- Update transport instantiation to handle Local transport
- Add Local transport tests to verify polymorphic behavior
- Enhanced connect() method for Local transport validates directories
The rename_file functionality is needed for EDI file processing workflow
where files are renamed after download to mark them as processed.
Sponsored-by: ByWater Solutions
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch:
* Removes unused logger object instantiation
* Adds missing Test::NoWarnings to the API tests
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch renames the entire API structure from SFTP-specific to generic
file transport terminology to reflect the polymorphic nature of the system.
API Changes:
- Endpoints: /config/sftp_servers -> /config/file_transports
- Parameters: sftp_server_id -> file_transport_id
- Definitions: sftp_server -> file_transport
- Controller: Config::SFTP::Servers -> Config::File::Transports
File Changes:
- api/v1/swagger/definitions/sftp_server.yaml -> file_transport.yaml
- api/v1/swagger/paths/config_sftp_servers.yaml -> config_file_transports.yaml
- Koha/REST/V1/Config/SFTP/Servers.pm -> Koha/REST/V1/Config/File/Transports.pm
- t/db_dependent/api/v1/sftp_servers.t -> file_transports.t
Updated References:
- swagger.yaml: paths, definitions, parameters, tags
- Template JavaScript: API URLs and variable names
- Controller: package name, variable names, error messages
- Tests: endpoints, parameters, descriptions
- API mapping: to_api_mapping field name correction
To test:
1. Apply this patch
2. Run: yarn api:bundle (to regenerate swagger bundle)
3. Run: prove t/db_dependent/api/v1/file_transports.t
4. All API tests should pass
5. Verify admin interface uses new API endpoints
6. Test CRUD operations work correctly
The API now consistently uses 'file_transports' terminology matching
the polymorphic file transport system that supports FTP, SFTP, and
future transport types.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Resolved circular dependency chain:
Transport.pm → TestTransport.pm → Transports.pm → SFTP.pm → Transport.pm
Changed 'use Koha::BackgroundJob::TestTransport' to runtime 'require'
in the _post_store_trigger method to break the dependency cycle.
This fixes the C3 method resolution order failures in
Koha::File::Transport::FTP and Koha::File::Transport::SFTP.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
We now use background jobs triggered by Transport->store to update the
connection status.
Status is now a json blob with more details, so we update the api
definition to reflect that.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
We add triggers to store in the base class and then use them in the SFTP
transport class to write out the key file to the filesystem for later
use in connect.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds polymorphic transport classes to Koha for FTP and SFTP
file transport configurations.
We introduce a base class for interacting with the base configuration
storage and subclasses to impliment the protocol specific transport
handling.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Martin Renvoize
Lucas Gass
Tomás Cohen Arazi
Martin Renvoize