Opened 6 years ago

Closed 5 years ago

#4752 enhancement closed fixed (fixed)

Support cred plugins in the "twistd ftp" command line

Reported by: Jean-Paul Calderone Owned by: Ying Li
Priority: normal Milestone:
Component: ftp Keywords:
Cc: Branch: branches/ftp-AuthOptionMixin-4752-2
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli

Description

The FTP server would be 31% more awesome if it supported pluggable authentication backends, instead of only supporting a password file. It should use twisted.cred.strcred.AuthOptionMixin instead of its own --password-file thing.

It would be nice if --userAnonymous could also be eliminated, with the determination about whether a user is anonymous delegated to the credentials checker. However, it appears as though the server wants to return "331.2" instead of "331.1" when an anonymous login is being attempted, which requires the server to know if it is going to be an anonymous login before the "password" is sent (hence before the login is attempted).

Change History (17)

comment:1 Changed 6 years ago by Jean-Paul Calderone

Perhaps we can get rid of --userAnonymous.

After FTP receives the username, it can log in to the portal with UsernamePassword(username, ""). If this succeeds, the server can proceed to prompt for the "password" (ie, email address) and log it or whatever. If it fails, the server can prompt for the password and re-try login with UsernamePassword(username, password).

This would let an IUsernamePassword credentials checker decide which username corresponds to the anonymous user and return ANONYMOUS when it sees a login attempt for that username. If the realm also allows anonymous users, this would succeed with a usable avatar.

comment:2 Changed 6 years ago by <automation>

Owner: itamarst deleted

comment:3 Changed 6 years ago by Ying Li

Author: cyli
Branch: branches/ftp-AuthOptionMixin-4752

(In [32844]) Branching to 'ftp-AuthOptionMixin-4752'

comment:4 Changed 6 years ago by Ying Li

(In [32853]) Add cred plugin support to twistd ftp and deprecate --password-file option

refs #4752

comment:5 Changed 6 years ago by Ying Li

Keywords: review easy added

build results

Moved the --userAnonymous stuff to #5313

comment:6 Changed 6 years ago by Ying Li

Keywords: review easy removed

comment:7 Changed 6 years ago by Ying Li

(In [32976]) Forgot to add checkers to portal refs #4752

comment:8 Changed 6 years ago by Ying Li

Keywords: review added

comment:9 Changed 6 years ago by Glyph

Keywords: review removed
Owner: set to Ying Li

Thanks. This is an area of work we really need to devote some attention to, so thanks for getting the ball rolling here :-).

  1. .misc files should generally be empty; I think that this deserves a 4752.feature and a 4752.removal at the same time, since this is adding a feature and removing something at the same time. (I seem to remember that there's a precedent for this, but I can't remember where I saw it. Maybe exarkun can recall.)
  2. For some reason test_ftp_options.py and the .misc file don't have a newline at the end, which makes the diffs look a little weird.
  3. There's some trailing whitespace in test_ftp_items.py.
  4. You need to resolve some merge conflicts, so please merge forward and re-run the tests.
  5. There are a bunch of pyflakes errors, but they're all imported-but-unused:
    twisted/tap/ftp.py:11: 'error' imported but unused
    twisted/tap/ftp.py:11: 'credentials' imported but unused
    twisted/tap/ftp.py:16: 'os' imported but unused
    twisted/test/test_ftp_options.py:10: 'UsageError' imported but unused
    twisted/test/test_ftp_options.py:11: 'makeService' imported but unused
    twisted/test/test_ftp_options.py:12: 'deprecate' imported but unused
    
  6. opt_ methods are slightly special, since their docstrings are displayed as part of --help. So you shouldn't put @since markers (or any other epytext, really) in them. (Whenever you edit a tap plugin, you should probably have a look at their help interactively, e.g. twistd ftp --help.)
  7. testPasswordfileDeprecation should be test_passwordfileDeprecation; test_ is a method prefix.
  8. We try to avoid the word "correct" in test docstrings, since it doesn't actually say what is being verified. Instead, describe the property involved. Also, we don't describe what the test is testing, but just the property of the system is. So, for example, instead of saying "Test that the --passwordfile option will emit a correct warning", instead, "The --passwordfile option emits a warning stating that said option is deprecated."
  9. The docstring for FTPOptionsTestCase looks copy/pasted: it says it's testing twistd mail but I think it's pretty obvious it's testing `twistd ftp. Also, the C{}` epytext markup ("code") would be more appropriate than I{} ("italic") for annotating a shell command.
  10. This branch does involve new functionality for which there are no new tests: there really needs to be at least one test that invokes the new --auth option and makes sure it actually does something; after all, the old tests pass just fine if the new credCheckers field is totally ignored and not passed to the Portal at all.

Thanks again; aside from item 10, none of this is really functional, so it's almost ready to land; I suspect the next review will be very brief.

comment:10 Changed 5 years ago by Ying Li

Branch: branches/ftp-AuthOptionMixin-4752branches/ftp-AuthOptionMixin-4752-2

(In [33006]) Branching to 'ftp-AuthOptionMixin-4752-2'

comment:11 Changed 5 years ago by Ying Li

(In [33007]) Pyflakes and trailing whitespace and other changes refs #4752

comment:12 Changed 5 years ago by Ying Li

(In [33008]) Add auth mixin tests refs #4752

comment:13 Changed 5 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted

comment:14 Changed 5 years ago by Glyph

Owner: set to Glyph
Status: newassigned

Reviewing.

comment:15 Changed 5 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to Ying Li
Status: assignednew

Looks pretty good. A few things you may want to tweak before merging:

  1. The 'longdesc' attribute talks about creating a .tap file, which we haven't done for many years. That should probably just be restored to be ''. Looks like this may have been copied from twisted.mail.tap's example?
  2. It looks like you didn't quite respond to review point 8. "Test that" and "correct" are in the same genre of things not to put in test docstrings. Just describe the property being verified in the test docstrings, not the method of its verification. Also, be as specific as possible about the properties of correct/incorrect, valid/invalid inputs and outputs. For example, instead of:
            Tests that the --auth command generates a checker does not authorize
            invalid logins
    
    what about:
            The C{--auth} command-line option add a checker to the list of checkers,
            which returns a L{Deferred} that fails with L{UnauthorizedLogin} when
            presented with credentials that are unknown to that checker.
    
    This isn't the best possible docstring, but it says exactly what the expected behavior is, rather than having the user guess what "authorize" and "valid credentials" means.
  3. test_passwordfileDeprecation refers to the --passwordfile option in its docstring, but the actual option is --password-file.
  4. I notice that you're using the file: strcred plugin and writing a temporary file in these tests, but you could use memory: and skip that step.
  5. It's a good idea to avoid returning Deferreds from test methods unless you need to process real events in your tests. In the case of test_ftp_options, you know exactly what is supposed to happen: the Deferred returned by the requestAvatarId method for the file: plugin should always return something synchronously. If the Deferred returned by requestAvatarId doesn't have a result immediately, you want the test to fail immediately and report that problem, not hang forever and require the user (or the buildbot) to time out the test or kill it with ^C. So, for example, instead of this:
    return checker.requestAvatarId(correct).addCallback(
        lambda username: self.assertEqual(username, correct.username)
    )
    
    do something like this:
    usernames = []
    checker.requestAvatarId(correct).addCallback(usernames.append)
    self.assertEqual(usernames, [correct.username])
    

Address these points to your satisfaction and then land. Thanks!

comment:16 Changed 5 years ago by Ying Li

(In [33009]) Change docstrings to match review comments, and clear out longdesc refs #4752

comment:17 Changed 5 years ago by Ying Li

Resolution: fixed
Status: newclosed

(In [33010]) Merge ftp-AuthOptionMixin-4752-2 : add cred plugin to ftp tap

Author: cyli Reviewer: glyph Fixes: #4752

Add support for cred plugins in twistd ftp command line

Note: See TracTickets for help on using tickets.