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

[Feature] Distinct SFTP_BASE_URL and [Bugfix] sftp.exists() works incorrectly when using root_path #1363

Closed
Toruitas opened this issue Mar 15, 2024 · 1 comment · Fixed by #1372

Comments

@Toruitas
Copy link
Contributor

[Feature]
I'm using Django Storages for both S3 and SFTP, so the fact they both use MEDIA_URL was problematic.

I've added a new setting SFTP_BASE_URL which will be preferred. If it doesn't exist, MEDIA_URL will be used.

# sftpstorage.py
def get_default_settings(self):
        return {
            ...
            "base_url": setting("SFTP_BASE_URL") or setting("MEDIA_URL"),
        }

And to set it in

# django settings.py
SFTP_BASE_URL = os.getenv('SFTP_BASE_URL')  # use this first
MEDIA_URL= os.getenv('MEDIA_URL')  # use this second

[Bug]

Additionally, in implementing the SFTP storage with a root path, the SFTPStorage.exists() would always prepend the root path to what it was checking.

If the root path was uploads, it would check for the existence of uploads/uploads rather than just uploads when going through the recursive _mkdir() calls during a _save().

This has been fixed by removing the use of self._remote_path(name) within SFTPStorage.exists(), and just returning True if name is falsy.

While in my manual testing in a Django project this works fine both with and without a root path, I have not managed to get tox to run. If someone would help me getting tox up and running, I can add a test for it.

Toruitas added a commit to Toruitas/django-storages that referenced this issue Mar 15, 2024
@Toruitas
Copy link
Contributor Author

@jschneier Additional details about the bugfix. I've just added a couple prints at the start of sftpstorage.py's exists method. This shows the process as it starts with _save() and the recursive _mkdir() calls.

I hadn't tried the pre-fix code without the root_path earlier, but I see there's a recursion error there, too.

As I see it, there are 2 possible places to address the issue. Either in exists() or in _mkdir().


def exists(self, name):
        print(f"exists: Checking if '{name}' exists")
        print(f"exists: Statting {self._remote_path(name)}")
        try:
            self.sftp.stat(self._remote_path(name))
            return True
        except FileNotFoundError:
            return False

Scenario 1:
root_path: "" Using the default here
file_path: file.txt Not placing in a directory.
sftp server initial directory: /

Outputs:

exists: Checking if 'file.txt' exists
exists: Statting file.txt
exists: Checking if '' exists
exists: Statting
exists: Checking if '' exists
exists: Statting
exists: Checking if '' exists
exists: Statting
Eventual recursion error. 

Scenario 2:
root_path: "" Using the default here
file_path: path/to/file.txt Should be put in upload/path/to/file.txt.
sftp server initial directory: /

Outputs:

exists: Checking if 'path/to' exists
exists: Statting path/to
exists: Checking if 'path' exists
exists: Statting patn
exists: Checking if '' exists
exists: Statting
exists: Checking if '' exists
exists: Statting
exists: Checking if '' exists
exists: Statting
Eventual recursion error. 

Scenario 3:
root_path: "upload/" This directory already exists.
file_path: file.txt Should be put in upload/file.txt
sftp server start path: /

Outputs:

exists: Checking if 'upload' exists
exists: Statting upload/upload (this is the bug)
exists: Checking if '' exists
exists: Statting upload/
OSError: Failure. (My user doesn't have permissions to create anything in root, but the directory "upload/" already exists anyway so doesn't need creating, so further recursive mkdir fails in this buggy attempt)

Scenario 4:
root_path: "upload/" This directory already exists.
file_path: path/to/file.txt Should be put in upload/path/to/file.txt. These other dirs don't exist.
sftp server start path: /

Outputs:

exists: Checking if 'path/to/file.txt' exists
exists: Statting upload/path/to/file.txt
_save for path/to/file.txt
exists: Checking if 'upload/path/to' exists
exists: Statting upload/upload/path/to (this is the bug)
exists: Checking if 'upload/path' exists
exists: Statting upload/upload/path
exists: Checking if 'upload' exists
exists: Statting upload/upload
exists: Checking if '' exists
exists: Statting upload/
OSError: Failure (My user doesn't have permissions to create anything in root, but the directory "upload/" already exists anyway so doesn't need creating, so further recursive mkdir fails in this buggy attempt)

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 a pull request may close this issue.

1 participant