ftpserverlib
ftpserverlib copied to clipboard
Implementing all 4 typical "messages" as in proftpd and other commercial FTP servers
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:
- upon client connection
- upon successful user auth
- upon failed user auth
- 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.
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
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 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.
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!
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).
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
This was done in #435.