FTP server lacks tests for RNTO and RNFR.

[adiroiban]

calling RNTO while being authenticated, without calling RNFR will generate an unhandled exceptions since self._fromName is only defined in RNFR.

Here is a patch that identifies the problem:

Please let me know if you agree with with fix and I could provide a full patch together with the required tests.

TwistedCore-10.2.0/twisted/protocols/ftp.py

@@ -752 +752 @@
                 return BAD_CMD_SEQ, "PASS required after USER"
 
         elif self.state == self.AUTHED:
+            if cmd == 'RNTO':
+                return BAD_CMD_SEQ, "RNFR required before RNTO"
             method = getattr(self, "ftp_" + cmd, None)
             if method is not None:
                 return method(*params)

[adiroiban]

It looks like this was already implemented in latest Twisted version.

I was not able to find the test for server so I start adding a test for this case.

While working at this, I also found that there is no test for a proper RNFR and RNTO for the server. There are many tests for the client.

I have attached a patch containing the 2 tests.

[thijs]

Thanks. Before this can land the testcase names:

1. should start with test_
2. need a docstring

[adiroiban]

Hi,

I have attached a new diff.

Sorry for missing the docstring and the test_ prefix.

I have also renamed testSYST to test_SYST and added a docstring since I used it as an example for starting the new test.

For test_RNFRandRNTO I have also added an 'integration' check for file renaming on disk.

Please let me know if it needs other changes. Cheers

[spiv]

As the original goal of this ticket has been satisfied I've updated the summary to reflect the goal of the current patches.

            userHome=self.directory, 

It's a bit of a misfeature that the anonymous user implies read-only access, as it leads to ugliness like adding cruft about logging-in non-anonymous users to write tests for RNFR/RNTO. The capabilities of the client should be orthogonal to how the login was done. Oh well, something to worry about in another ticket.

 	371	    def test_RNFRandRNTO(self): 
 	372	        """ 
 	373	        Sending the RNFR command followed by RNTO should perform a 
 	374	        successful rename operation. 
 	375	        """ 

Say "will perform" rather than "should perform". Be assertive in test docstrings :)

Also, the claim isn't strictly true: it also requires that the source and destination filenames are valid (or even more pedantically, that IFTPShell implementation accepts the rename(src, dest) call). So I'd say "Sending the RNFR command followed by RNTO, with valid filenames, will perform a successful rename operation".

            ["250 Requested File Action Completed OK"], 

This test highlights that the Title Cased Phrase here is pretty weird and inconsistent with the other messages. Not the fault of this patch, but if you're inclined to please submit a patch to tidy this up.

As my only comment requesting action is a tweak to a docstring, I'll go ahead and submit this (assuming the tests actually pass).

[spiv]

(In [34797]) Branch to ftp-rnfr-tests-4930.

[spiv]

(In [34798]) Commit adiroiban's 4930-rnto-3.diff.

Refs #4930.

[spiv]

I forgot to add: we could do with even more tests here, e.g. exercising what happens when the server encounters an error when tries to perform the rename.

[spiv]

(In [34801]) Merge ftp-rnfr-tests-4930: Add more FTP server tests.

Author: adiroiban Reviewer: spiv Fixes: #4930

Adds some test coverage for the FTP server implementation of the RNFR and RNTO verbs.

[adiroiban]

Hi spiv and many thanks for the review.

Your comments are very helpful.

Hope it the future the patches will be perfect :)

Cheers!