FTP.ftp_STOR() should pass FTPCmdError errors back to the client

[bigjools]

ftp_STOR() has an errorBack, ebSent(), which ignores the error. It would be better if it could pass that error back to the client (as opposed to the default "internal server error")

[bigjools]

This change is to support a Launchpad change that I am making which will send errors back to 'dput' when people upload packages without a valid GPG signature on their .changes file. It is the source of many questions when their uploads go missing!

[exarkun]

Thanks! This sounds like a great feature to add. The approach of handling the particular exception type specially seems right, too.

1. Can you also add a unit test that verifies the code and message are sent back in this case?
2. What would you link about not logging the error in this case?
3. Can you write a news fragment describing this new feature?

Thanks again!

[bigjools]

1. I've not contributed to Twisted before. Can you point me at the right place to add a test for this please? (I want to make sure I don't do it wrong)

2. Not logging the error - possibly, but I think it's more useful to log it for audit reasons. At least, I'd like it logged in my ftp server.

3. Okay. Jeez, and I thought our Launchpad landing process was tough :)

[exarkun]

Can you point me at the right place to add a test for this please?

The tests for FTP are in twisted/test/test_ftp.py (you can see this module named in the test-case-name definition at the top of ftp.py - many, but not all, Twisted files use this to point to their tests). It looks like BasicFTPServerTestCase in that file might be a good test case to add the new test to.

it's more useful to log it for audit reasons

Okay, that's fine. I think we can probably provide a better logging experience for audit purposes than reporting a full traceback, but that's clearly a separate feature, so don't worry about it.

and I thought our Launchpad landing process was tough

:)

[bigjools]

Latest patch attached, with thanks to therve and jml for helping me fix my test.

[exarkun]

(In [30840]) Branching to 'ftp-stor-error-reporting-4909'

[exarkun]

Thanks very much for adding the unit test. This makes it clear that my original review missed the fact that the error comes out formatted rather strangely. :) Trying to understand why the message comes out as:

550 Cannot rmd, ('blah',) is not a directory

instead of:

550 Cannot rmd, blah is not a directory

led me first to the seemingly bizarre signature of FTPCmdError.__init__, which takes *args and just saves them away. So IsNotADirectoryError("foo") gives an errorMessage attribute of ("foo",). Not a very nice "message". But then I realized this is why there's the FTPCmdError.response method, which will spit out a properly formatted string.

However, calling this in ebSent is no good, because it returns the complete response line, and ebSent needs to return a tuple giving a response code and arguments to interpolate into the message for that code.

Fortunately, it seems all is not lost. For the very next errback in the chain after ebSent is the nested function processFailed in FTP.lineReceived. And that errback checks for FTPCmdError failures and writes out the result of calling their FTPCmdError.response methods. Just what's wanted here. So I think this tweak to the patch is in order:

twisted/protocols/ftp.py

@@ -1112 +1112 @@
         def ebSent(err):
             log.msg("Unexpected error receiving file from client:")
             log.err(err)
+            if err.check(FTPCmdError):
+                return err
             return (CNX_CLOSED_TXFR_ABORTED,)
 
         def cbConsumer(cons):

That is, just pass on the failure if it is wrapping an FTPCmdError exception and let the next errback deal with it.

As I was figuring all this out, I noticed there is another nested ebSent method in the implementation of RETR which has more or less the exact same issue as this one! That probably merits a bit of fixing as well (separate ticket), and maybe the code can be refactored to remove the duplication as well.

With that overly long exposition out of the way, I think we can get on to the action. I'll check your latest into a branch, make my tweak to improve the message rendering, make sure it works everywhere, and then land it. Thanks!

[exarkun]

(In [30841]) Apply latest patch

refs #4909

[exarkun]

(In [30842]) Get rid of the extra tupleyness of the error message reported back to the client.

refs #4909

[bigjools]

Great! Thanks for fixing up and accepting. When's the next Twisted release expected?

[exarkun]

(In [30844]) Merge ftp-stor-error-reporting-4909

Author: bigjools Reviewer: exarkun Fixes: #4909

If an FTPCmdError is encountered while writing a file when handling a STOR command in twisted.protocols.ftp.FTP, allow the details of that exception to be sent back to the client, rather than flattening them into a generic CNX_CLOSED_TXFR_ABORTED.