Opened 7 years ago

Last modified 2 years ago

#2875 enhancement new

IFTPShell access implementations are not complete

Reported by: therve Owned by: adiroiban
Priority: normal Milestone:
Component: ftp Keywords:
Cc: thijs, adi@… Branch:
Author: Launchpad Bug:

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 adiroiban 3 years ago.
2875-ftp-access-2.diff (4.2 KB) - added by adiroiban 2 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by therve

  • Description modified (diff)

comment:2 Changed 4 years ago by <automation>

  • Owner therve deleted

comment:3 in reply to: ↑ description Changed 4 years ago by thijs

  • Cc thijs 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 3 years ago by adiroiban

  • Cc adi@… added
  • Keywords review added

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!

Changed 3 years ago by adiroiban

comment:5 Changed 3 years ago by therve

  • Keywords review removed
  • Owner set to adiroiban

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 2 years ago by adiroiban

comment:6 Changed 2 years ago by adiroiban

  • Keywords review added
  • Owner adiroiban deleted

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,

comment:7 Changed 2 years ago by cyli

  • Owner set to cyli

comment:8 Changed 2 years ago by cyli

  • Keywords review removed
  • Owner changed from cyli to adiroiban

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
Note: See TracTickets for help on using tickets.