Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#4930 defect closed fixed (fixed)

FTP server lacks tests for RNTO and RNFR.

Reported by: adiroiban Owned by: spiv
Priority: low Milestone:
Component: ftp Keywords:
Cc: itamarst, adi@…, thijs Branch: branches/ftp-rnfr-tests-4930
(diff, github, buildbot, log)
Author: spiv Launchpad Bug:

Description (last modified by thijs)

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

    === modified file 'TwistedCore-10.2.0/twisted/protocols/ftp.py'
     
    752752                return BAD_CMD_SEQ, "PASS required after USER" 
    753753 
    754754        elif self.state == self.AUTHED: 
     755            if cmd == 'RNTO': 
     756                return BAD_CMD_SEQ, "RNFR required before RNTO" 
    755757            method = getattr(self, "ftp_" + cmd, None) 
    756758            if method is not None: 
    757759                return method(*params) 

Attachments (3)

4930-rnto-1.diff (2.8 KB) - added by adiroiban 2 years ago.
4930-rnto-2.diff (3.7 KB) - added by adiroiban 2 years ago.
4930-rnto-3.diff (3.7 KB) - added by adiroiban 2 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc itamarst added

comment:2 Changed 3 years ago by thijs

  • Description modified (diff)

Changed 2 years ago by adiroiban

comment:3 Changed 2 years ago by adiroiban

  • Keywords review added

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.

comment:4 Changed 2 years ago by adiroiban

  • Cc adi@… added

comment:5 Changed 2 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to adiroiban

Thanks. Before this can land the testcase names:

  1. should start with test_
  2. need a docstring

Changed 2 years ago by adiroiban

comment:6 Changed 2 years ago by adiroiban

  • Keywords review added

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

Changed 2 years ago by adiroiban

comment:7 Changed 2 years ago by adiroiban

  • Owner adiroiban deleted

comment:8 Changed 2 years ago by spiv

  • Owner set to spiv

comment:9 Changed 2 years ago by spiv

  • Status changed from new to assigned

comment:10 Changed 2 years ago by spiv

  • Keywords review removed
  • Summary changed from RNTO without prior calling RNFR should send a bad command sequence response to FTP server lacks tests for RNTO and RNFR.

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

comment:11 Changed 2 years ago by spiv

  • Author set to spiv
  • Branch set to branches/ftp-rnfr-tests-4930

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

comment:12 Changed 2 years ago by spiv

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

Refs #4930.

comment:13 Changed 2 years ago by 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.

comment:14 Changed 2 years ago by spiv

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:15 Changed 2 years ago by adiroiban

Hi spiv and many thanks for the review.

Your comments are very helpful.

Hope it the future the patches will be perfect :)

Cheers!

Note: See TracTickets for help on using tickets.