search

pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.52k stars 380 forks source link

Add support for working directory in Server #528

Closed mafredri closed 2 years ago

mafredri commented 2 years ago

This commit allows the working directory for the (old) Server implementation to be changed without doing a os.Chdir first.

The feature can be enabled with sftp.WithServerWorkingDirectory(dir) passed as an option to sftp.NewServer.

It is useful when the sftp is used as part of a larger service that does more than just serve sftp and using os.Chdir is not an option.

The fallback behavior (when the option is not specified) is that the path remains unmodified (as before).

Motivation: I know the new sftp.RequestServer already supports this, however, since it currently only has an in-memory filesystem and is potentially going to see a larger refactor in the future, I think it's a good idea to include it for this older implementation as well.

drakkan commented 2 years ago

Hi,

thanks for your PR. At first glance it looks ok, but I have never user the server implementation myself. If no other contributors have time to review this PR you will have to wait a while for a proper review. Thanks for your patience.

Please note that no larger refactor is planned for the request server, we just don't provide a sample implementation that uses the local filesystem but you can easily write your own implementation and it is much more customizable than the old server implementation. I'm not sure we'll continue to maintain the old server implementation in the future, maybe we'll rewrite it using the request server interfaces (and probably outside the root package)

mafredri commented 2 years ago

Did I misinterpret what was being discussed in https://github.com/pkg/sftp/issues/407? To me it sounds like a refactor/rework of the API, and I think it sounds wonderful TBH. But it's also a reason why I wasn't looking to work with the RequestServer at this time.

The fact that RequestServer is missing a FS implementation is also a reason I opted out, it means Server is what people reach for when they need SFTP, so it'd be more battle tested. (My reasoning may of course be wrong.)

As drakkan says, you should be able to just implement a raw OS RequestServer and I just don’t see the value in adding features to a deprecated implementation.

I appreciate the review, but I'm left wondering if I should clean up this PR or discard it? It sounds like you're not interested in merging it but I could be wrong.

I tried to make sure that no code paths are altered unless this new feature is enabled so that it'd be easier to merge in, but I can understand if you don't want it at all.

Edit: Made some updates anyway, happy to work on this further to get it merged, so let me know what you think/want.

puellanivis commented 2 years ago

😐 It seems like github had been spamming out all of my draft responses. Learn from me, don’t do code review responses after a long hard day of toiling in the salt mines of legacy code…

mafredri commented 2 years ago

@puellanivis thanks for the thorough PR review! I've amended most of the feedback now. The only thing I have yet to revert is https://github.com/pkg/sftp/pull/528#discussion_r997951182 because doing so may introduce a bug when used with a workDir.

I went ahead with refactoring toLocalPath to be a method on Server. This also moved the code out of request_* files which seems appropriate, considering it's not used by the RequestServer.

prochac commented 2 years ago

Cheers guys, It's being awesome for me to come just a week after this feature has been landed. I was seeking for it, it's not in release yet, and found this. Many thanks.