IFTPShell access implementations are not complete

[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.

[thijs]

Replying to therve:

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

fwiw, #4934 is gone now.

[adiroiban]

Hi,

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
            return defer.fail(FileNotFoundError(path))
        # For now, just see if we can os.listdir() it
        try:
            p.listdir()
        except (IOError, OSError), e:
            return errnoToFailure(e.errno, path)
        except:
            return defer.fail()
        else:
            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.
        """
        self.createDirectory('ned')
        d = self.shell.access(('ned',))
        return d


    def test_accessNotFound(self):
        """
        access should fail on a resource that doesn't exist.
        """
        d = self.shell.access(('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!

[therve]

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).

[adiroiban]

Hi,

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  http://docs.python.org/glossary.html#term-eafp 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.

Cheers,

[cyli]

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:  http://techarttiki.blogspot.com/2008/08/read-only-windows-files-with-python.html, 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? http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles