Bug 35761 - Add an administration editor for FTP and SFTP servers
Summary: Add an administration editor for FTP and SFTP servers
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Jake Deery
QA Contact: Testopia
URL:
Keywords:
: 35622 38038 (view as bug list)
Depends on:
Blocks: 33308 37248 38115 38489 38629 39190 25246 35622 38038
  Show dependency treegraph
 
Reported: 2024-01-10 16:46 UTC by Kyle M Hall (khall)
Modified: 2025-03-14 09:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 35761: Introduce central FTP/SFTP management (102.79 KB, patch)
2024-11-22 15:19 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (103.99 KB, patch)
2024-11-22 16:50 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (112.78 KB, patch)
2024-11-29 16:18 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (112.45 KB, patch)
2024-12-02 12:44 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: (follow-up) Increase length of password (2.10 KB, patch)
2024-12-03 09:28 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Integrate EDI into new FTP/SFTP management UI (32.22 KB, patch)
2024-12-04 10:45 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (124.05 KB, patch)
2024-12-17 12:13 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (124.11 KB, patch)
2024-12-23 19:10 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (124.43 KB, patch)
2025-01-13 10:37 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (124.98 KB, patch)
2025-01-13 12:11 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (121.18 KB, patch)
2025-02-06 14:26 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: DB schemas and atomicupdate (4.08 KB, patch)
2025-02-06 14:26 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (121.39 KB, patch)
2025-02-11 15:50 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: DB schemas and atomicupdate (4.08 KB, patch)
2025-02-11 15:51 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 35761: Database updates (5.46 KB, patch)
2025-02-24 13:13 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35761: DBIC Schema (4.07 KB, patch)
2025-02-24 13:13 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (117.31 KB, patch)
2025-02-24 13:13 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35761: Database updates (5.46 KB, patch)
2025-03-13 14:35 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35761: DBIC Schema (4.07 KB, patch)
2025-03-13 14:35 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35761: Introduce central FTP/SFTP management (117.31 KB, patch)
2025-03-13 14:35 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall (khall) 2024-01-10 16:46:29 UTC
For EDI, our partners will often create a vendor and EDI vendor record for each account they have with a vendor. This ends with a ton of duplicate FTP data. It would be great if we defined FTP/SFTP servers in their own section of the admin area and could then select a single defined FTP/SFTP for multiple EDI vendor records. That way if the password needs changed, or a vendor upgrades from FTP to SFTP, only a single record needs to be edited.

In the future, this could be use to allow tools such as runreport.pl, export_records.pl, export_borrowers.pl and others to directly upload their file outputs to a remote server rather than require a second step afterward.
Comment 1 Kyle M Hall (khall) 2024-01-31 16:53:38 UTC
We can accept the host keys when we add a new SFTP server via
 ssh-keyscan -h <ip or name of host> >> known_hosts 
when a new SFTP server is added

Then, if the key changes we can detect the error ( via Net::SFTP::Foreign ) to flag e given SFTP server has no longer having a matching key.

We could then add the option to "reset" the public key via "ssh-keygen -R <ip or name of host>"
Comment 2 Jake Deery 2024-10-08 10:33:05 UTC
Really nice suggestion, I'm going to look at progressing this.

Jake.
Comment 3 Jake Deery 2024-11-20 09:23:57 UTC
I should be in a position to push an attachment this week :-)

Jake.
Comment 4 Jake Deery 2024-11-22 15:19:13 UTC Comment hidden (obsolete)
Comment 5 Jake Deery 2024-11-22 15:20:43 UTC
Whoops, will add a test plan!!
Comment 6 Jake Deery 2024-11-22 16:50:52 UTC Comment hidden (obsolete)
Comment 7 Jake Deery 2024-11-29 16:18:05 UTC Comment hidden (obsolete)
Comment 8 Jake Deery 2024-12-02 12:44:34 UTC
Created attachment 175052 [details] [review]
Bug 35761: Introduce central FTP/SFTP management

This patch adds a central FTP/SFTP management UI, with an associated
database object and class, plus the beginnings of an API. We can use
this code as a building block for adding FTP/SFTP connections to
other parts of the system, as well as to improve the usability of the
EDI transports. It is also now possible to test these connections
from within the Koha UI.

To test:-
You will need access to an FTP or SFTP server
Contact me if you need a disposable FTP/SFTP server

a)  Go to Koha Administration
b)  Notice no FTP/SFTP referenced on this page
c)  APPLY PATCH
d)  Create new user, let's say sftpuser
e)  Set permissions for new user to include:
    1)  catalogue
    2)  parameters -> manage_sftp_servers
f)  Log in with this new user, go back to Koha Administration
    1)  Notice the FTP/SFTP servers option
g)  Undo step e2) for the sftpuser via the koha admin user
    1)  Refresh the Koha Administration page as the sftpuser
    2)  Notice how FTP/SFTP servers has disappeared
h)  Back to koha admin, go to Koha Administration -> FTP/SFTP Servers -> New
i)  Complete form depending on the FTP or SFTP server you have access to, Submit
    1)  Notice you are prompted to confirm before saving
j)  Select Edit, observe your details are the same, and Submit again
k)  Repeat step J one additional time
l)  Select Test, observe all tests passing
m)  Select Delete, confirm deletion, and observe server disappear
n)  Optionally, run prove on new tests
    1)  t/Koha/Auth/Permissions.t
    2)  t/db_dependent/Koha/SFTP/Server.t
	3)  t/db_dependent/api/v1/sftp_servers.t
o)  SIGN-OFF
Comment 9 Jake Deery 2024-12-02 12:45:10 UTC
Ready for SO :-)

Jake.
Comment 10 Jake Deery 2024-12-03 09:28:15 UTC
Created attachment 175067 [details] [review]
Bug 35761: (follow-up) Increase length of password

I was finding that the password was getting truncated way too soon. I've
lengthened this field to accomodate longer (encrypted) passwords.
Comment 11 Jake Deery 2024-12-04 10:45:22 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize (ashimema) 2024-12-04 17:09:17 UTC
*** Bug 35622 has been marked as a duplicate of this bug. ***
Comment 13 Jake Deery 2024-12-05 09:16:43 UTC
*** Bug 38038 has been marked as a duplicate of this bug. ***
Comment 14 Andrew Fuerste-Henry 2024-12-05 17:46:07 UTC
When I've entered invalid SFTP credentials and save, I'm no longer able to load the table of SFTP servers. I just get a 404 error and ended up deleting that server via SQL. Users need to be able to delete broken entries themselves from the UI.
Comment 15 David Nind 2024-12-05 21:57:06 UTC
I had a go at testing, and have a couple of questions - these are probably more related to the SFTP server I tested with, and how it is configured:

1. I can connect to the SFTP server (see testing notes), but I can't write to or delete a file in the root directory:

  Testing we can write a test file... Failed
  Error message: Couldn't open remote file '.koha_test_file': Permission denied

  Testing we can delete test file... Failed
  Error message: Couldn't delete remote file: No such file

2. In this case, the root folder for the SFTP user is not writeable. However, the upload directory is (/upload). I think this is by design in this case with the atmoz/sftp Docker image.

3. However, if a SFTP server only allows uploads to a specific directory (such as uploads), should this be configurable?

4. When I refresh the FTP/SFTP servers listing page, there is a browser console error - this is probably related to 1 and 2 above:

  GET http://127.0.0.1:8081/api/v1/sftp_server//test_connection 404 (Not Found)

5. Should reloading or accessing the page actually try to connect, or should this be user initiated with the test action?


Testing notes (using KTD):

1. Add these commands to docker-compose-light.yml (before the networks section):

    sftp:
        image: atmoz/sftp
        command: koha:koha:::upload
        networks:
            - kohanet

2. Do a ktd pull and ktd up.

3. Identify the IP address of the SFTP server (kohadev-sftp-1):

   docker inspect -f '{{printf "====================\n"}}{{.Name}} {{range $net,$v := .NetworkSettings.Networks}}{{printf "\n"}}{{printf "\n"}}{{printf "%s\n" $net}}{{.IPAddress}}{{printf "\n"}}{{end}}' $(docker ps -q)

4. Check that you can connect to the server locally (not from the ktd shell, use koha as the password and the IP address for kohadev-sftp-1), for example: sftp koha@172.18.0.2 

5. Check that you can connect to the server from within your ktd shell: sftp koha@sftp
Comment 16 Jake Deery 2024-12-09 15:35:53 UTC
Thanks for the testing, both. Digesting and will reply with some notes shortly!

Jake.
Comment 17 Jake Deery 2024-12-13 16:28:24 UTC
Thanks for waiting folks, I've addressed most of the points here. Moving the directories used to the SFTP server config seems sensible, so I've done that and added a few more tests.

Just on the mentioned:

> 5. Should reloading or accessing the page actually try to connect, or should this be user initiated with the test action?

How do we envisage this working? A big red “push me” button on that page? A modal when clicking “test” on the main config table?

Jake.
Comment 18 David Nind 2024-12-13 21:17:30 UTC
(In reply to Jake Deery from comment #17)
> Thanks for waiting folks, I've addressed most of the points here. Moving the
> directories used to the SFTP server config seems sensible, so I've done that
> and added a few more tests.
> 
> Just on the mentioned:
> 
> > 5. Should reloading or accessing the page actually try to connect, or should this be user initiated with the test action?
> 
> How do we envisage this working? A big red “push me” button on that page? A
> modal when clicking “test” on the main config table?
> 
> Jake.

From memory, there was a 'Test' button in the table's action column.
Comment 19 Jake Deery 2024-12-17 12:13:51 UTC Comment hidden (obsolete)
Comment 20 Jake Deery 2024-12-17 12:16:10 UTC
I've reworked a few aspects of this work to now:
1)  Include an upload & download directory in the FTP/SFTP Server config page
2)  Include a status which gives an at-a-glance of whether the server is working or not, based on the results of the latest test
3)  Include a prescriptive run tests button which prevents the tests from running without prior user confirmatory action
4)  Added a warning on the test page for SFTP users, informing them of the risks of using unknown SFTP servers
Comment 21 Martin Renvoize (ashimema) 2024-12-18 08:09:25 UTC
Comment on attachment 175608 [details] [review]
Bug 35761: Introduce central FTP/SFTP management

Review of attachment 175608 [details] [review]:
-----------------------------------------------------------------

::: Koha/SFTP/Server.pm
@@ +182,5 @@
> +
> +    return $self->set(
> +        {
> +            status   => $value,
> +            password => $self->plain_text_password,

What's the purpose of updating the password and key here?  It looks to me like your setting status and also decrypting the password and key file at the same time?
Comment 22 Martin Renvoize (ashimema) 2024-12-18 08:10:24 UTC
When one updates the configuration details we should reset the status as without running a test we don't know whether the updated details are correct so the 'Passed' or 'Failed' states in the table are misleading ;)
Comment 23 Martin Renvoize (ashimema) 2024-12-18 09:14:36 UTC
I also prefer the behaviour where clicking 'Test' from the table took you to the new page AND ran the tests.. I find it a bit disjointed that I need to click 'Run tests' as a second step.  I also find you get 'stuck' on that test page now without a clear way back to the display page.. yes you can click the breadcrumb but I think as it would make sense to have one of the buttons beneath the "form" take you back too for user flow
Comment 24 Martin Renvoize (ashimema) 2024-12-18 09:15:03 UTC
For testing, this is a pretty cool service btw: https://sftpcloud.io/tools/free-sftp-server
Comment 25 Andrew Fuerste-Henry 2024-12-23 19:10:16 UTC Comment hidden (obsolete)
Comment 26 Andrew Fuerste-Henry 2024-12-23 19:13:28 UTC
(In reply to Martin Renvoize (ashimema) from comment #23)
> I also prefer the behaviour where clicking 'Test' from the table took you to
> the new page AND ran the tests.. I find it a bit disjointed that I need to
> click 'Run tests' as a second step.  I also find you get 'stuck' on that
> test page now without a clear way back to the display page.. yes you can
> click the breadcrumb but I think as it would make sense to have one of the
> buttons beneath the "form" take you back too for user flow

I wouldn't mind if it automatically testing upon saving so long as a failed test doesn't prevent the save. Agreed that there could be a clearer post-test button to get back to the server list.
But I signed off anyway; those could both be followup issues.
Comment 27 Jake Deery 2025-01-13 10:37:39 UTC Comment hidden (obsolete)
Comment 28 Jake Deery 2025-01-13 12:11:24 UTC Comment hidden (obsolete)
Comment 29 Jake Deery 2025-01-13 12:15:58 UTC
Made further improvements to this work:

*  Reverted behaviour, so tests now run when tests button clicked (shortening path to user goal)
*   Test status now resets when an edit is made
*   Separated out password and key_file storage functionality, to avoid a bloated store subroutine, and to negate the need to 'work around' password and key_file storing when it is not necessary
*   Removed key_file from API response
Comment 30 Lucas Gass (lukeg) 2025-01-14 16:53:44 UTC
Missing some POD: 

 FAIL	Koha/Schema/Result/SftpServer.pm
   FAIL	  pod coverage
		POD is missing for koha_object_class
		POD is missing for koha_objects_class

A test is failing, but I think this could just be due to my setup:

    #   Failed test 'SWAGGER3.3.4'
    #   at /kohadevbox/koha/t/db_dependent/api/v1/sftp_servers.t line 436.
    #          got: '{"1_ftp_conn":{"err":"cannot connect to ZuxQPXx5x71QvmLk9O9rtmTOKJGgtbPZn3CJD8g66: Temporary failure in name resolution","msg":null,"passed":false}}'
    #     expected: '{"1_ftp_conn":{"err":"cannot connect to ZuxQPXx5x71QvmLk9O9rtmTOKJGgtbPZn3CJD8g66: Name or service not known","msg":null,"passed":false}}'
     # Looks like you failed 1 test of 5.


Failing for POD.
Comment 31 Jake Deery 2025-01-15 11:38:08 UTC
Do we often write POD documents for dbix class files, because I can't see examples of this anywhere else? This was automatically generated by `./misc/devel/update_dbix_class_files.pl`.

Jake.
Comment 32 Jake Deery 2025-02-04 11:37:31 UTC
Hi,

Any chance at another look on this one, please? I can't see anything wrong per se when I'm running QA tools, and I don't believe those missing POD documents are essential.

Jake.
Comment 33 Lucas Gass (lukeg) 2025-02-04 15:36:51 UTC
(In reply to Jake Deery from comment #32)
> Hi,
> 
> Any chance at another look on this one, please? I can't see anything wrong
> per se when I'm running QA tools, and I don't believe those missing POD
> documents are essential.
> 
> Jake.

Jake, my fault. I can take a go at QA'ing this again. Can we have the database update/schema put into a separate commit?
Comment 34 Jake Deery 2025-02-06 14:26:37 UTC Comment hidden (obsolete)
Comment 35 Jake Deery 2025-02-06 14:26:46 UTC Comment hidden (obsolete)
Comment 36 Jake Deery 2025-02-06 14:27:41 UTC
No prob, easy to miss, I know! I've divided that commits up into two patches. Hopefully this helps. :-)

Jake.
Comment 37 Jake Deery 2025-02-11 15:50:54 UTC Comment hidden (obsolete)
Comment 38 Jake Deery 2025-02-11 15:51:04 UTC Comment hidden (obsolete)
Comment 39 Jake Deery 2025-02-11 15:51:18 UTC
Rebased atop main
Comment 40 Nick Clemens (kidclamp) 2025-02-14 14:25:54 UTC
All works as expected from test plan

QA failures:
   FAIL	  spelling
		 passiv ==> passive
Why did you name the field 'passiv' instead of 'passive'?

 FAIL	installer/data/mysql/atomicupdate/bug_35761-add_SFTP_tables_and_perm.pl
   FAIL	  file permissions
		File must have the exec flag

Koha/Schema/Result/SftpServer.pm - this should be in its own patch
Bug 35761: DO NOT PUSH - Schema changes
so that RM can generate the file on their own when they push
Comment 41 Martin Renvoize (ashimema) 2025-02-17 12:44:53 UTC
(In reply to Nick Clemens (kidclamp) from comment #40)
> All works as expected from test plan
> 
> QA failures:
>    FAIL	  spelling
> 		 passiv ==> passive
> Why did you name the field 'passiv' instead of 'passive'?
> 
>  FAIL	installer/data/mysql/atomicupdate/bug_35761-add_SFTP_tables_and_perm.pl
>    FAIL	  file permissions
> 		File must have the exec flag
> 
> Koha/Schema/Result/SftpServer.pm - this should be in its own patch
> Bug 35761: DO NOT PUSH - Schema changes
> so that RM can generate the file on their own when they push

I have a whole patchset above this now.. just considering whether it should go here or on it's own bug.   I do fix the passiv -> passive spelling in that set though.
Comment 42 Martin Renvoize (ashimema) 2025-02-24 13:13:49 UTC Comment hidden (obsolete)
Comment 43 Martin Renvoize (ashimema) 2025-02-24 13:13:52 UTC Comment hidden (obsolete)
Comment 44 Martin Renvoize (ashimema) 2025-02-24 13:13:56 UTC Comment hidden (obsolete)
Comment 45 Martin Renvoize (ashimema) 2025-02-24 13:19:25 UTC
I added the re-work on top as it's own bug.. it warranted a proper review as it does a few things not seen in Koha before.

I've asked Tomas to review it :).

Setting this one back to Signed Off so it can be reviewed, but take into consideration I believe this and the follow-up bug should be pushed together as the follow-up changes a fair bit of what's implemented here.
Comment 46 Martin Renvoize (ashimema) 2025-03-13 14:35:18 UTC Comment hidden (obsolete)
Comment 47 Martin Renvoize (ashimema) 2025-03-13 14:35:21 UTC Comment hidden (obsolete)
Comment 48 Martin Renvoize (ashimema) 2025-03-13 14:35:25 UTC Comment hidden (obsolete)
Comment 49 David Cook 2025-03-14 02:49:52 UTC
Getting a "Does not apply" error.

Think you've got a bit of an issue here with your patches. I think "Bug 35761: DB schemas and atomicupdate" needs to be obsoleted.

But I'll let you double-check things.
Comment 50 Martin Renvoize (ashimema) 2025-03-14 09:24:39 UTC
Yup, somehow a patch got unobsoleted