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

[syncplify]

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.

[drakkan]

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

[syncplify]

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.

[codecov]

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 ⚠️
string_utils.go	55.55%	2 Missing and 2 partials ⚠️
handle_files.go	33.33%	1 Missing and 1 partial ⚠️

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[syncplify]

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!

[drakkan]

Thank you for working on these additions and sorry for the delay.

The changes looks good! Please see my minor comments in the review.
I would like to see a test case checking that the received messages match the expected ones.

If you want to cover both the old and new code in test case you may evaluate to extend TestServerDriver and add the new methods there, so you can use the old driver to cover the old code paths and the new driver to test the new methods.
I think this requires a fair amount of effort, maybe you can wait for the project owner to tell you if it's needed for merging these changes

[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).

[drakkan]

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]

missing return

[fclairamb]

This was done in #435.