Opened 9 years ago

Last modified 5 years ago

#2875 enhancement new

IFTPShell access implementations are not complete

Reported by: therve Owned by: Adi Roiban
Priority: normal Milestone:
Component: ftp Keywords:
Cc: Thijs Triemstra, Adi Roiban Branch:

Description (last modified by therve)

This method is called in response of the CWD command in the ftp server. However the 2 implementations don't check enough things:

  • the one in ftp just do a listdir()
  • the one in vfs in trunk does nothing (and isn't totally fixed by #1264)

The expected behavior should be documented, tests added, and both implementations fixed.

Attachments (2)

2875-ftp-access-1.diff (1.8 KB) - added by Adi Roiban 5 years ago.
2875-ftp-access-2.diff (4.2 KB) - added by Adi Roiban 5 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 9 years ago by therve

Description: modified (diff)

comment:2 Changed 6 years ago by <automation>

Owner: therve deleted

comment:3 in reply to:  description Changed 6 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

Replying to therve:

  • the one in vfs in trunk does nothing (and isn't totally fixed by #1264)

fwiw, #4934 is gone now.

comment:4 Changed 5 years ago by Adi Roiban

Cc: Adi Roiban added
Keywords: review added


I would like to have this ticket closed.

Below is the current code which checks that the folder/path exists and then checks that it can get the folder list.

    def access(self, path):
        p = self._path(path)
        if not p.exists():
            # Again, win32 doesn't report a sane error after, so let's fail
            # early if we can
        # For now, just see if we can os.listdir() it
        except (IOError, OSError), e:
            return errnoToFailure(e.errno, path)
            return defer.succeed(None)

From my point of view, this is a valid implementation which should work across multiple OS.

How would you like this implementation fixed?

Below is the code for the tests:

    def test_access(self):
        Try to access a resource.
        d ='ned',))
        return d

    def test_accessNotFound(self):
        access should fail on a resource that doesn't exist.
        d ='foo',))
        return self.assertFailure(d, ftp.FileNotFoundError)

There is a test for successful condition and a test for a missing path.

There is no test for checking for permission errors. I added a integration test which uses 'chmod' for removing list permissions from a folder. The test is skipped on non Unix systems since there is no chmod command on Windows and I am not aware or any alternatives provided by FilePath.

I have attached the diff.

Please let me know what needs to be done to have this ticket closed. Thanks!

Changed 5 years ago by Adi Roiban

Attachment: 2875-ftp-access-1.diff added

comment:5 Changed 5 years ago by therve

Keywords: review removed
Owner: set to Adi Roiban

Listing is not enough, at least on posix systems. For example, if you have a directory owned by root, and the permissions are 644, you can do a listdir, but you can't change your path to that directory (as it's missing the 'x' bit).

Changed 5 years ago by Adi Roiban

Attachment: 2875-ftp-access-2.diff added

comment:6 Changed 5 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted


I have attached a new diff which used 'os.chdir' for Unix systems. I can also go for checking using os.access, but I went for EAFP technique as recommended by Python os.access documentation.


On Windows, os.access will not work since it can not map execution permission. os.chdir also does not work since I think that internally it checks for execution permissions (maybe by using os.access).

For example, if a set the permissions for a folder only to 'Read', and remove all other permissions (including List folder content), I can still browse folder's files in Windows Explorer, I can access child folders. os.listdir will work, but os.chdir will raise a WindowsError:5 permissions error.

This is why I went for using os.chdir on Unix and os.listdir on non-Unix (Windows).

The permission tests are skipped on non-Unix since os.chmod does not work on Windows.

Please take a look at latest changes and let me know what needs to be changed.


comment:7 Changed 5 years ago by Ying Li

Owner: set to Ying Li

comment:8 Changed 5 years ago by Ying Li

Keywords: review removed
Owner: changed from Ying Li to Adi Roiban

Thank you for working on this adiroiban!

  1. I think it'd be better to add the cleanup as a callback to the Deferred that is returned by self.assertFailure - that way if the cleanup function raises any errors itself, the assertion itself will not fail (and the traceback may be more helpful). Also, I think it is unnecessary to remove the file, since each test has its own temporary directory in _trial_temp. Leaving the file may also be useful for debugging purposes, even if the permissions get reset.
  1. On windows I think you can set permissions if pywin32 is installed:, if you wanted to test with that.
  1. If there are any tests that you want to skip, a more detailed skip string would be great (for instance specifying briefly why the test is not supported).
  1. Could you please include a news file with your patch?
Note: See TracTickets for help on using tickets.