ftpserverlib icon indicating copy to clipboard operation
ftpserverlib copied to clipboard

Implementing all 4 typical "messages" as in proftpd and other commercial FTP servers

Open syncplify opened this issue 1 year ago • 6 comments

The FTP protocol allows for (and many open source as well as commercial FTP servers support) custom messages to be sent to the client in 4 specific moments of the session:

  1. upon client connection
  2. upon successful user auth
  3. upon failed user auth
  4. upon receipt of the QUIT command (just prior to closing the connection)

We noticed that ftpserverlib only supports point number 1, therefore we would like to contribute some code to implement the other 3.

Our notes:

  • Implementing point number 4 was quite straightforward by means of an "extended" interface (like you did with several other things)
  • You may not like our implementation of point number 2 and 3 here above, because it breaks backwards compatibility by changing the AuthUser method prototype; this was the easiest way to implement it, but if you want to do it in a more backward-compatible way, by all means, feel free to totally reject our implementation

Regardless of the implementation details, we see there's value to this functionality, so we hope you'll consider including it into your excellent library.

Thank you, kind regards.

syncplify avatar Oct 14 '23 22:10 syncplify

Hello,

thank you for this contribution. I took a quick look at your patch. Do you need to set different messages based on the user? If the messages are global to the server maybe you can just add some new settings and thus keep backward compatibility. Not sure if I missing something

drakkan avatar Oct 21 '23 14:10 drakkan

Having those messages returned based on the user can be very useful indeed. Some enterprise customers may use variables or even need to return a totally different message based upon who's logging in, hence the need to build those messages in real-time, in code, based upon the user. Of course I do understand the reluctance for breaking backward compatibility, I promise I don't take this lightly. If there is a way to do it without breaking backwards compatibility I am all for it. I am simply reinforcing that the functionality - in and by itself - would be extremely useful to have in a user-dependent fashion.

syncplify avatar Oct 22 '23 21:10 syncplify

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (b16eb9c) 86.49% compared to head (947eff0) 86.06%.

Files Patch % Lines
handle_auth.go 54.54% 3 Missing and 2 partials :warning:
string_utils.go 55.55% 2 Missing and 2 partials :warning:
handle_files.go 33.33% 1 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #422      +/-   ##
==========================================
- Coverage   86.49%   86.06%   -0.44%     
==========================================
  Files          11       12       +1     
  Lines        1607     1629      +22     
==========================================
+ Hits         1390     1402      +12     
- Misses        151      156       +5     
- Partials       66       71       +5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 25 '23 13:11 codecov[bot]

Guys, I've seen the latest codecov reports, but I am not sure they are correct (unless I am missing something obvious, which is entirely possible). In fact, I have added both of my changes to the library's test suite, so I am not sure why codecov now says that those lines are "not covered by tests" (when indeed they are). Would it be possible for you to take a quick look and let me know what I'm missing (if anything)? Thanks!

syncplify avatar Nov 27 '23 16:11 syncplify

I have also fixed the generic HASH function... it was using strings.SplitN which fails if the path contains spaces, and substituted it with a custom advSplitN func that correctly splits strings in case there are quoted substrings that contain spaces (as the FTP protocol RFC requires).

syncplify avatar Jan 17 '24 14:01 syncplify

I have also fixed the generic HASH function... it was using strings.SplitN which fails if the path contains spaces, and substituted it with a custom advSplitN func that correctly splits strings in case there are quoted substrings that contain spaces (as the FTP protocol RFC requires).

Thanks for this fix. I think it should be a separate PR and writing a test cases should be quite simple.

As for the original issue, I have added to my TODO to write the required test cases, but I'm very busy at the moment, it might take some time

drakkan avatar Jan 20 '24 09:01 drakkan

This was done in #435.

fclairamb avatar Mar 02 '24 16:03 fclairamb