Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle StartDir differently in case of VirtFS #354

Merged
merged 7 commits into from
Jun 23, 2024

Conversation

lmagyar
Copy link
Contributor

@lmagyar lmagyar commented Jun 4, 2024

UPDATE: complete rewrite of this comment

Without this PR, pftpd VirtFS behaves errorneously:

  • if eg. /storage/emulated/0 is selected as startDir
    • SSHFS fails, can't see anything under /fs
    • WinSCP behaves strangely, it "jumps" from /fs to /fs/storage/emulated/0, that's OK, we can .. out, and when on /fs again, it shows the startDir's content, but can't access the folders in it anymore, so gets broken
  • only when / is the startDir, works everything

With this PR:

  • the startDir's content is seen under /fs, eg. we have /fs/Android if startDir is /storage/emulated/0 or we have /fs/storage if startDir is /
  • we've got real home folders for virtFS SFTP and FTP, pointing to eg. /fs/storage/emulated/0
  • both SSHFS and WinSCP works fine (Note: SSHFS must select / as start folder)
  • this is analogous to the /saf and /rosaf folders, where we can see the content of the selected SD card folder

Just fun fact: I made a version (https://github.com/lmagyar/prim-ftpd/tree/dev-dual-root-and-home-for-sshfs), where depending on SSHFS's start dir, / or nothing=user's home, it was able to see the / folder under /fs (when / is SSHFS's start dir), or the selected startDir, eg. /storage/emulated/0 under /fs (when `` is the SSHFS start dir), it was soo cool, but WinSCP got completely broken and couldn't figure out the reason, even my fake symlink experiments on /fs etc. folders were broken, so gave up. :(

@lmagyar
Copy link
Contributor Author

lmagyar commented Jun 6, 2024

Now it's ready to review.

@lmagyar lmagyar marked this pull request as ready for review June 6, 2024 17:06
@wolpi
Copy link
Owner

wolpi commented Jun 9, 2024

but WinSCP got completely broken and couldn't figure out the reason

Over the years I had similar issues with some clients 😄

With this PR:

Well, I also see that current code and behavior is not perfect. With current code it would be possible to access files on root folder which is not possible with new code. But would be ok for me to change this.
Wit new code IDE complains about Paths.get() would require a higher API level than the App demands. On my test systems/devices that does not cause an issue but it might for people with old phones.
Could you find a solution without Paths.get() ?

@wolpi
Copy link
Owner

wolpi commented Jun 9, 2024

Oh, and this change breaks my test script.
tests/tests.py
As it expects the old behavior.
Thus some more work is required to switch to new behavior.

@lmagyar lmagyar marked this pull request as draft June 9, 2024 18:01
@lmagyar
Copy link
Contributor Author

lmagyar commented Jun 9, 2024

So, I think I've found finally an even better solution:

  • removed all realPath manipulation in VirtualFileSystemView.getFile()
  • used the "official" absolute() (SFTP) and getHomeDirectory() (FTP) to create home folders
  • works with WinSCP, works with SSHFS
  • no tests have to be modified

@lmagyar lmagyar marked this pull request as ready for review June 9, 2024 23:49
@lmagyar lmagyar changed the title Handle StartDir differently (as chroot) in case of VirtFS Handle StartDir differently in case of VirtFS Jun 22, 2024
@wolpi
Copy link
Owner

wolpi commented Jun 23, 2024

That seems to be a good and elegant approach 👍

@wolpi wolpi merged commit 14cf6f0 into wolpi:master Jun 23, 2024
1 check passed
@wolpi wolpi added this to the 7.2 milestone Jun 23, 2024
@lmagyar lmagyar deleted the pr-startdir-for-virtfs branch June 23, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants