mechanism for specifying a cred checker for twistd plugins to use

[glyph]

Several plugins, such as words, ftp, mail, news, and conch, mainly exist to expose a protocol implementation to the world. These protocols all have the concept of a user and a session, which is implemented with twisted cred.

The twistd plugins in question exist to expose these protocols as useful tools to users without involving Python programming. However, in order to be truly useful, the plugins must be able to connect to an established user-database. Some ad-hoc work (conch's use of pwd, words' use of --passwd) has already been done in this direction, however, the authentication glue must be replicated (therefore replicated inconsistently) between twistd plugins. Currently they provide less than a pale shadow of the generality and utility of cred's implementation, even those simple checkers present in Twisted itself.

Checker pluggability should be easy to achieve with simple command-line libraries for each plugin to use, and a simple plugin system for checkers to register themselves.

[itamarst]

Isn't that what http://twistedmatrix.com/trac/wiki/Specification/AdministrativeComponentSelection is for?

[glyph]

Replying to itamarst:

Isn't that what http://twistedmatrix.com/trac/wiki/Specification/AdministrativeComponentSelection is for?

At the very least, although that feature could be used to implement this ticket (I don't think it should be), it is at a different level of abstraction. This ticket describes a particular desirable feature, not a way to implement that feature. I'll describe how I believe mesozoic will be implementing that feature in this comment, though :).

The specification for administrative component selection mainly deals with a generic API usable by many subsystems. This ticket is specific to twistd plugins, and further, specific to cred checkers. It will use the same command-line user interface as the rest of twistd. So far, the only nod to UI on the administrative component selection page is "The UI for selecting these things might initially be a text editor (to edit a config file) or a shell (to set environment variables)."

Alex and I discussed a nice simple UI for this which shouldn't touch very much code at all. A --checker option will be provided by some library functionality (so that you can do opt_checker = checkerPluginOption() rather than re-implementing it each time). There's no global registry for arbitrary interface providers; your IServiceMaker options object will get a "checkers" attribute which will be a list of ICredentialsChecker objects. We might be able to expand this idiom to provide other cross-plugin idiomatic options later (e.g.: listening port, realm), but, those are separate issues, and as such, should have separate tickets. There will be a new type of plugin for checker factories, but again, those factories will specifically be for creating checkers from simple string input, not a generic series of structured fields or anything fancy like that.

This came from JP suggesting on IRC, and me agreeing, that this need not be mixed together with other multi-plugin integration issues. He doesn't like the idea of mixing in extra ad-hoc, poorly specified crud to twistd to support more than one plugin at a time on the command line, and I don't like the idea of requiring the rather ambitious and complex functionality on the page you reference for functionality as simple as an idiomatic way to specify a passwd-format file, in-memory database, or other simple implementation of a cred checker.

[therve]

I've been pushed to look at this so I'll give some meta-comments :).

Overall I'm really enthusiast, it brings great flexibility. I'm a bit worried about the size of the branch, but it's mainly new things.

It would be great to have tests for cred plugins. #2598 would be handy for that.

I would prefer that authOptionsHelper be a mixin (a la ReactorSelectionMixin).

Finally, some documentation is needed. I don't know if it's a plug into another documentation (cred, tap?) or a new one.

I'm not sure how this fits within all the twistd's refactoring done currently (#739, #638, #2571). We should probably define some priorities between all of this.

[mesozoic]

I like making it a mixin instead of a helper function; I'll do that now.

I've already got some basic tests for the cred plugins, but they're more for making sure strcred works than testing that the plugins actually do what they're supposed to. Some checkers are easy to test without extensive monkey-patching, but others aren't (like the UNIXChecker).

I'll definitely add documentation before I mark this for review, but I'll also take a look at those last three tickets and see if anything I've done is related.

[mesozoic]

I don't believe #739 or #638 really impact these changes (though they're along the same vein). #2571 looks like it could have an impact, and might even simplify what I'm doing, but I usually get scared away whenever glyph and exarkun start arguing about layers of abstraction.

[mesozoic]

Review me please.

[therve]

General comments:

• Some pyflakes errors:

  twisted/words/tap.py:16: 'checkers' imported but unused
  twisted/cred/plugins/cred_passwd.py:12: 'Interface' imported but unused
  twisted/cred/plugins/cred_passwd.py:12: 'classProvides' imported but unused
  twisted/cred/plugins/cred_unix.py:16: 'Interface' imported but unused
  twisted/cred/plugins/cred_unix.py:16: 'classProvides' imported but unused
  twisted/cred/plugins/cred_anonymous.py:12: 'Interface' imported but unused
  twisted/cred/plugins/cred_anonymous.py:12: 'classProvides' imported but unused
  twisted/cred/plugins/cred_memory.py:12: 'Interface' imported but unused
  twisted/cred/plugins/cred_memory.py:12: 'classProvides' imported but unused
  twisted/cred/strcred.py:18: 'sys' imported but unused
  twisted/cred/strcred.py:22: 'implements' imported but unused
  twisted/cred/strcred.py:22: 'Interface' imported but unused
  

• all copyright notices of the files touched should be updated to 2007. New files should probably only contain the 2007 year.
• docstrings should be

  """
  doc
  """
  

  that's almost done everywhere but not quite.
• Most new classes should be made new-style except if there is a good reason (like the mixin).
• Please remove trailing whitespaces
• Exception should be created this way: raise Exception("foo") for consistency

test_tap.py:

• joe_wrong should be spelled joeWrong
• please add docstrings to all tests methods (test_hostname, test_auth), if possible to all methods (setUp, tearDown, _loginTest)
• instead of tempfile, use the method mktemp of unittest.TestCase

cred_passwd.py:

• missing docstrings of PasswdCheckerFactory and generateChecker
• passwdcheckerfactory should use mixed case.

cred_unix.py:

• missing docstrings
• unixcheckerfactory should use mixed case
• check_pwd and check_spwd should be spelled checkPwd and checkSpwd
• I don't get the 'return None' in check_pwd, and the subsequent 'if checked is None' in requestAvatarId. I guess you want to try spwd after if pwd failed, but that's not really clear. Maybe that's only need to be clarified with some comments.

cred_anonymous.py:

• docstrings, rename anonymouscheckerfactory

cred_memory.py:

• rename memorycheckerfactory
• missing docstrings. In particular I'm a bit worried that the warning of InMemoryUsernamePasswordDatabaseDontUse disappears.
• The ValueError raised here is OK, but it would great if it can be trapped somewhere else to raise a UsageError at command line.

strcred.py:

• there is big bug in opt_auth_list: it must raise SystemExit(0), or the server will be launched
• the main docstring is not 'proper epytext'. I think you have to indent the list of examples.
• docstring of AuthOptionMixin should be updated to mention opt_auth_list.
• I'm not sure that KeyError is the good exception in makeChecker. I'm not sure you even need to have a special case for empty string, do you?
• opt_auth_list could be a little more descriptive
• the check in opt_auth shouldn't swallow the problem when you provide 2 authentication methods with the same interfaces, because it can be something hard to debug.

test_strcred.py:

• the assertFailure returned deferreds should be returned by the tests, to prevent strange problems.
• several variables use 'foo_bar' case instead of 'fooBar'.
• opt_auth_list is not tested.
• cred_passwd is not tested.

checkers.py:

• I don't really like the 'I am foo' style of docstring.

Other uses of cred in tap files should migrate to this in the future, but it's in the scope of the ticket. We should create tickets for this after merge.

All of this is mainly aesthetic remarks, I hope there's no blocking problem, because it looks really good. Thanks!

[mesozoic]

I'm working through most of these tonight. I'm not sure how to test --auth-list, though, besides just capturing stdout and making sure something shows up. Since it's entirely plugin-based, the resulting text could vary from one system to another. Thoughts?

[therve]

The things I would test for auth_list:

• it does output a string
• this string contains each auth plugins identifier
• it exits the program (that's the hard part :)): maybe monkeypatch sys.exit, or override SystemExit exception.

The exact output string doesn't really matter.

[itamarst]

The latter test could be done with assertRaises(SystemExit, auth_list, ...).

[mesozoic]

I've updated the branch with everything mentioned here, with the exception of the "I am foo" docstring comment. Those were there beforehand, and I'm not game for rewriting all of them right now. Either way it doesn't impact this functionality.

Ready for re-review!

[therve]

Great, some other comments:

• I don't expect you to modify all the 'I am foo' docstrings, just don't add new ones, so that's ok
• In the same way, you didn't have to change the copyrights for all the files, just the ones you had previously touched. I guess that doesn't really matter now.
• the new StrcredException is great. But the raise usage.UsageError(e.message) in opt_auth doesn't work (there is no message attribute). That also means that this case is not tested.
• The log in addChecker is something, but first it should not be printed in the tests, and then I think that you should raise an exception here. I don't expected something sane when it happens.
• There are still a few wrong docstring format (add a new line after the first """).

Thanks!

[itamarst]

1. Exceptions should have docstrings.

2. It would be nice if --auth-list explained the format of the various auth plugins; perhaps the interface should provide that information? You already have it in nice docstrings so it should be simple to move into an attribute provided by ICheckerFactory.

3. Perhaps --auth-list should be --help-auth? Since we also have --help-reactors for a similar task.

I like it!

[itamarst]

Also, maybe add a plugin for PAM?

[mesozoic]

I've committed code for most of the feedback above. I'm not so thrilled about delaying this for a PAM plugin; it could be added later, either standalone or as part of cred_unix.

I'm also not convinced that overlapping interfaces should always raise an exception. I think it's conceivable that some other plugin could provide a large number of interfaces, and the user only wants to use it for some of them. Or is that over-complicating things?

I'm also unable to get log.msg to actually write output at the time that parseOptions is called. That would solve the problem with spitting out messages during the tests. I'll put some more time into it later this week.

[glyph]

Replying to mesozoic:

I've committed code for most of the feedback above. I'm not so thrilled about delaying this for a PAM plugin; it could be added later, either standalone or as part of cred_unix.

Yes. I am concerned it was even brought up. Scope creep should not be part of the review process. Let's file a separate ticket.

I'm also not convinced that overlapping interfaces should always raise an exception. I think it's conceivable that some other plugin could provide a large number of interfaces, and the user only wants to use it for some of them. Or is that over-complicating things?

I don't understand this comment. Nobody said anything about "overlapping interfaces" up until that point, and I can't match up one of the other comments about raising exceptions.

[mesozoic]

The "overlap" is when you use addChecker (or just --auth) twice, and the two checkers you add happen to provide some of the same credential interfaces. Right now the behavior is first come, first serve; the suggested behavior was to raise an exception and exit.

[therve]

Thanks for the corrections. FWIW, I'm against the pam plugin for this branch.

Regarding the overlap, after looking at it more, I'm a bit lost. What's the purpose of the credInterfaces option at the first place? For example it doesn't seem nessary for the words tap plugin. As I imagine you have a usecase, why not use a list for each interfaces? I don't remember a restriction on this.

[mesozoic]

I've committed a change to the tests and the code that makes optionscredInterfaces? a dict of lists of checkers, rather than a dict of checkers. I can't think of any reason why that restriction was there in the first place, but maybe someone else can.

Please re-review.

[therve]

Cool. A few aesthetics comments remaining:

• twisted/cred/plugins/init.py needs a copyright line
• twisted/test/test_strcred.py copyright year should be 2007 (currently 2001-2004)
• there are few lines over 80 characters that could be easily removed
• strcred and test_strcred could benefit from a few blank lines between classes for clarity
• in twisted/cred/checkers.py, please remove 'self' from ICheckerFactory.createChecker arguments list. Also, I think that the method is called generateChecker, not createChecker.

That's it!

[glyph]

Re: whitespace; the  guidelines we've been using at Divmod are:

• 2 blank lines between methods
• 3 blank lines between classes

This can resolve ambiguities as to "how many blank lines are enough" :).

[mesozoic]

Do you mean two or three newlines? Because that seems like a lot of padding, and it's not what I see in many of the Divmod projects out there. I see one blank line between methods and two between classes or module-level functions.

[itamarst]

Divmod's whitespace guidelines are, AFAIK, not Twisted's guidelines (unless you've changed the coding standard?). Should this really be holding up a merge?

[mesozoic]

I've committed changes based on everyone's feedback. Ready for unbranching?

[glyph]

Replying to itamarst:

Divmod's whitespace guidelines are, AFAIK, not Twisted's guidelines (unless you've changed the coding standard?). Should this really be holding up a merge?

No. I simply mentioned them because therve already mentioned blank lines in his previous review. I am thinking of adding this to the coding standard at some point, but before requiring it I've been recommending it.

[itamarst]

Some more comments:

1. Help is better, but still insufficient. E.g. for "unix" it says I don't need args, which is good, but what *is* unix authentication? How is it different than passwd auth? Which do I use to authenticate against /etc/shadow?

2. Probably don't need the @cvars on ICheckerFactory, since you have Attributes.

3. Document adding cred plugins in the cred howto.

4. A more fundamental design suggestion: rather than showing all checkers, some of which will not actually work, perhaps --auth-help and the available plugins should be filtered by the interface or interfaces the developer actually wanted? Some way of telling the mixin "I want IHasedPasswordChecker" (implicitly including subclasses).

If you think #4 is out of scope, open a ticket for it.

[itamarst]

Another question -

any reason the plugins go in twisted.cred.plugins? Depending on how plugins are implemented that may not allow third-party people to make their own plugins. Even if not, using twisted.plugins as usual seems simpler.

[mesozoic]

I assumed that twisted.plugins was just for TAPs. I figured I'd follow the model of other Divmod apps that use twisted.plugin, which define their own namespace for a specific type of plugin.

How would twisted.cred.plugins prevent people from making their own?

I'm working on documentation for adding third-party cred plugins and some extended help features. I'll take a crack at item 4 in the list above, and if it turns out to be too broad we can open another ticket. I'll commit my changes for review soon, and hopefully we can roll this out.

[itamarst]

I would say, twisted.plugins are for any type of Twisted-specific plugins. Divmod apps have their own namespace since it's plugins for Divmod stuff.

I'm not sure it would prevent people, just a vague suspicion based on how it's implemented.

Anyway, thanks!

[glyph]

It wouldn't prevent people from writing their own. The convention thus far seems to be twisted.plugins for all plugins within Twisted, though, so I think we should stick with that.

[mesozoic]

As I look at it further, there's no "right way" (as I see it) to preemptively determine whether a checker factory will support a particular interface. In the example of the FilePasswordDB checker, until you instantiate the class (and choose whether to pass it a hash function), you don't know whether it supports IHashedUsernamePassword in addition to IUsernamePassword.

We could make it a requirement for a checker factory to define which interfaces it plans on supporting, but it seems like a redundant place to put information that isn't actually tied to real functionality.

Thoughts? Is this something worth holding up incorporating this into trunk?

[glyph]

Replying to mesozoic:

As I look at it further, there's no "right way" (as I see it) to preemptively determine whether a checker factory will support a particular interface. In the example of the FilePasswordDB checker, until you instantiate the class (and choose whether to pass it a hash function), you don't know whether it supports IHashedUsernamePassword in addition to IUsernamePassword.

Since you asked, I'll answer the implicit question.

The "right way" that I can see would be to have the declarations on the factories (as you suggest), and rather than having string-formatting tricks determine whether to pass a hash function, you could have different plugins which would result in FilePasswordDB instances with different construction parameters.

We could make it a requirement for a checker factory to define which interfaces it plans on supporting, but it seems like a redundant place to put information that isn't actually tied to real functionality.

Interface declarations are always superfluous metadata in some sense, but in this case it would be used to satisfy a particular use-case in the UI: to only present options which actually make sense.

Thoughts? Is this something worth holding up incorporating this into trunk?

Your call. Procedurally, I don't think so, I would be happy to have the functionality already in the branch and then improve it with these declarations later. On the other hand, if you feel like working on this more before merging, I'd also be happy to have something like what I just described, since I think that it's cleaner to separate things which provide different interfaces into different plugins, even if they happen to yield instances of the same class to do it.

[mesozoic]

So, this should have been marked for review a long time ago. I apologize that I let it slip.

I've implemented what Glyph and Itamar describe above. There is now a credentialInterfaces attribute on every class that implements ICheckerFactory; this is required. It tells the plugin what credential interfaces it's able to support. There is also a supportedInterfaces attribute that is inherited through AuthOptionsMixin. By default it is None, meaning that all interfaces are 'welcomed' by the application. If set to an iterable of credential interfaces, only plugins which claim to provide those will be allowed.

I've also taken a stab at the documentation, though I could use some help with figuring out whether it's clear to a new user.

[therve]

• I have the following pyflakes errors:

  twisted/words/test/test_tap.py:4: 'tempfile' imported but unused
  twisted/cred/util.py:22: 'Unauthorized' imported but unused
  twisted/cred/strcred.py:17: 'sys' imported but unused
  twisted/cred/strcred.py:20: 'log' imported but unused
  twisted/plugins/cred_memory.py:11: 'credentials' imported but unused
  twisted/plugins/cred_anonymous.py:11: 'credentials' imported but unused
  

  Some of these files are only modified to have cleanups (util, credentials, error, __init__, pamauth). It would be better to revert these changes.
• in strcred.py AuthOptionMixin.opt_auth, the except UnsupportedInterfaces is not tested
• same file AuthOptionMixin.opt_auth_auth_type, the except InvalidAuthType is not tested
• for all the tests modifying sys.stdout: I would prefer that AuthOptionMixin get a stream as class variable, default to sys.stdout, that you could parametrize in the tests. Then instead of doing print, you'd have to use stream.write. But be careful of the differences.

I already found the previous version good, but the new help system is really great, so I hope we'll manage to merge this soon. Thanks!

[therve]

Oh, I have a test failing:

[ERROR]: twisted.words.test.test_tap.WordsTap.test_auth

Traceback (most recent call last):
  File "twisted/trunk/twisted/words/test/test_tap.py", line 54, in test_auth
    opt.parseOptions(['--auth', 'passwd:'+self.file.name])
  File "twisted/trunk/twisted/python/usage.py", line 219, in parseOptions
    self._dispatch[optMangled](optMangled, arg)
  File "twisted/trunk/twisted/python/usage.py", line 373, in <lambda>
    fn = lambda name, value, m=method: m(value)
  File "/home/th/twisted_dev/twisted/trunk/twisted/cred/strcred.py", line 156, in opt_auth
    raise usage.UsageError(
twisted.python.usage.UsageError: Auth plugin not recognized: passwd

[mesozoic]

Thanks for the feedback. Have you had a chance to look at the documentation and make sure it's detailed enough?

I've tried to address most of what you put in here. Here are my notes on the rest:

* I think that the code cleanup, if valid, should happen wherever it can. I have, though, reverted any files that were purely changes to the copyright notice. * The UnsupportedInterfaces is part of test_invalidAuthError; it gets raised by --auth InvalidWhatever. I've added another one for --help-auth-type. * I agree the sys.stdout stuff is ugly, but I question adding a dependency from twisted.cred on something in the twisted.web2 package. * I apologize for the missed error. I didn't test twisted.words.test before my last commit; I just did twisted.test. (Is there a ticket open to move that code?)

[mesozoic]

I meant to remove that last bit, about twisted.words.test, after I saw that other packages followed the same pattern. Please disregard it.

Also, please accept my apologies that the comment is unreadable. Basecamp list formatting and Trac list formatting work differently and it's been the source of many problems :) Here it is correctly:

• I think that the code cleanup, if valid, should happen wherever it can. I have, though, reverted any files that were purely changes to the copyright notice.
• The UnsupportedInterfaces? is part of test_invalidAuthError; it gets raised by --auth InvalidWhatever?. I've added another one for --help-auth-type.
• I agree the sys.stdout stuff is ugly, but I question adding a dependency from twisted.cred on something in the twisted.web2 package.

[therve]

Replying to mesozoic:

* I agree the sys.stdout stuff is ugly, but I question adding a dependency from twisted.cred on something in the twisted.web2 package.

Oh I was not talking about web2 stream at all :). My comment was to do that :

class AuthOptionMixin:
    stream = sys.stdout

    def opt_help_auth(...):
        self.stream.write('Help!\n')

And in tests, define stream to be a StringIO.

[therve]

Replying to mesozoic:

* I think that the code cleanup, if valid, should happen wherever it can.

The problem is that it makes merging very difficult, and is not that useful. Self contained changes are better with branch development.

I have, though, reverted any files that were purely changes to the copyright notice.

I always set them modified :/.

* The UnsupportedInterfaces is part of test_invalidAuthError; it gets raised by --auth InvalidWhatever.

I think that's wrong: the exception raised here is InvalidAuthType, and is raised by !findCheckerFactory.

I've added another one for --help-auth-type.

Cool!

[therve]

Replying to therve:

I always set them modified :/.

Grr. I meant I always see them modified.

[mesozoic]

Okay, I've added streams. Are we good? :)

[therve]

One of my previous comment is still true: in strcred.py AuthOptionMixin.opt_auth, the except UnsupportedInterfaces is not tested

[mesozoic]

I must have misinterpreted your earlier comment, or just missed that one. I've added an explicit test for UnsupportedInterfaces being raised from within AuthOptionsMixin.addChecker.

We do already test the fact that opt_auth catches UnsupportedInterfaces and turns it into UsageError. This test is in TestCheckerOptions.test_invalidAuthError.

Ready for re-review.

[therve]

Just one small thing:

twisted/test/test_strcred.py:5: 'sys' imported but unused

Replying to mesozoic:

I must have misinterpreted your earlier comment, or just missed that one. I've added an explicit test for UnsupportedInterfaces being raised from within AuthOptionsMixin.addChecker. We do already test the fact that opt_auth catches UnsupportedInterfaces and turns it into UsageError. This test is in TestCheckerOptions.test_invalidAuthError.

No. please read above, I already made a comment about that: the exception raised here is InvalidAuthType, and is raised by findCheckerFactory.

[mesozoic]

I see what I missed; my mistake. I've checked in a new test for that circumstance.

[therve]

Great! That's OK for me. As itamar and glyph made comments before that prevented from merging, it would be cool to have a go from them.

[itamarst]

I don't have the typing time to do more than skim the code, but other than my belief that "print >>" is totally icky, it's looking good. Beyond that I'll leave it up to therve.

[therve]

Yep, itamar's right, "print >> foo" should be replaced by "foo.write()" calls, with appropriate "\n".

[mesozoic]

Okay. The ugly syntax is gone, the tests work -- ready to merge? (Finally?)

[therve]

OK, there are still several problems :).

First the test suite doesn't pass on windows. See  http://twistedmatrix.com/buildbot/win32-py2.5-select/builds/449/step-select/2, it just need a try/except around the crypt imports.

The second problem is trickier:  http://twistedmatrix.com/buildbot/debian-py2.4-reactors/builds/1886/step-poll/0, because it's linked to plugins import. The (easy) solution would be to remove the top level reactor import in pamauth. The other solution is to only import at runtime, like the _tapHelper class, but it's much more work.

Last problem is that memory and file plugins don't enforce the presence of an argument, whereas I think they should.

[therve]

For second point, exarkun told me that the intended behavior is to lessen the impact of importing all plugins, so we should try to create _tapHelper-like solution.

[mesozoic]

Replying to therve:

OK, there are still several problems :). First the test suite doesn't pass on windows. See  http://twistedmatrix.com/buildbot/win32-py2.5-select/builds/449/step-select/2, it just need a try/except around the crypt imports.

Okay, I've added a try/except block, and raise NotImplementedError if someone actually tries to use the UNIXChecker on an unsupported platform.

The second problem is trickier:  http://twistedmatrix.com/buildbot/debian-py2.4-reactors/builds/1886/step-poll/0, because it's linked to plugins import. The (easy) solution would be to remove the top level reactor import in pamauth. The other solution is to only import at runtime, like the _tapHelper class, but it's much more work.

I have to play the ignorance card -- I really don't understand, from the traceback, where the problem is happening. (I also can't reproduce it on my machine.) What's the problem with just moving the import into the pamAuthenticateThread function? I'm struggling to figure out how _tapHelper would solve this problem.

Last problem is that memory and file plugins don't enforce the presence of an argument, whereas I think they should.

Memory and file plugins, in the current version, DO require an argstring. For the file plugin, it's a filename. For the memory plugin, that argstring can be empty, which I think is valid: "this database has no users at start-time". If your application has remote management functionality and you're building a demo environment, that's all you need.

[therve]

Replying to mesozoic:

Okay, I've added a try/except block, and raise NotImplementedError if someone actually tries to use the UNIXChecker on an unsupported platform.

Alright.

I have to play the ignorance card -- I really don't understand, from the traceback, where the problem is happening. (I also can't reproduce it on my machine.) What's the problem with just moving the import into the pamAuthenticateThread function? I'm struggling to figure out how _tapHelper would solve this problem.

Sorry, I should have been more explanatory. To reproduce the problem, simply run this :

PYTHONPATH=. ./bin/trial -r poll twisted/web/test/test_webclient.py

It should fail with 'reactor already installed' error. Why? Because pamauth has a toplevel reactor import, that the cred plugins import crec.checkers, and that cred.checkers import pamauth. So when trial load plugins, it imports pamauth, whereas what we're trying to do has nothing to do with cred or pamauth.

My solution is to have something like twisted.scripts.mktap._tapHelper: a class wrapping your plugins, and using a twisted.python.reflect.namedAny to import the real plugin at execution time, not at import time. You could create a twisted.cred.interfaces module with only the interfaces here (ICheckerFactory), without dependencies. There are probably a few other problems though.

Last problem is that memory and file plugins don't enforce the presence of an argument, whereas I think they should.

Memory and file plugins, in the current version, DO require an argstring. For the file plugin, it's a filename. For the memory plugin, that argstring can be empty, which I think is valid: "this database has no users at start-time". If your application has remote management functionality and you're building a demo environment, that's all you need.

Well, it's just that I expect an error in this case:

PYTHONPATH=. ./bin/twistd -n words --auth file

The error here will only appear when the first user try to connect.

[therve]

The problem is still present under windows:  http://twistedmatrix.com/buildbot/win32-py2.5-select/builds/451/step-select/2, because the unix plugin is imported everytime. That would be corrected by the solution mentioned above.

[mesozoic]

I believe I've resolved the issues you listed above.

• I didn't add any additional layers of complexity to the plugin process; instead, I moved the reactor import into the function that needed it.
• Calling --auth file raises an appropriately-named error, and there is a test for it.
• I ran the tests on the win32-py2.5-select buildslave, and only one error came up. But it looks like a TCP problem; I can't really see how it's related to what I've changed. See  http://twistedmatrix.com/buildbot/win32-py2.5-select/builds/512/step-select/2

Let me know if you see any other reasons this couldn't be merged.

[therve]

I'm sorry, but the plugins problem has to be really solved, not just workaround. I wasn't aware of this, but this is a big requirement. What to do:

• move the real body of the plugins into a twisted.cred.plugins package
• move ICheckerFactory in a new twisted.cred.interfaces file. You can probably move ICredentialsChecker, as long as you import it in checkers for backward compatibility.
• refactor the current plugins to only import stuff from twisted.cred.interfaces (and probably plugin, zope and such), and use twisted.python.reflect.namedAny to dynamically load the real plugins.

I realize it's a bit late, so I'm willing to help you on this if you want.

Another little unrelated problem, if I do

./bin/twistd -n words --auth anonymous

I get

Auth plugin not supported: <InterfaceClass twisted.cred.credentials.IAnonymous>

The message could probably be improved (plugin only providing an unsupported interface? I'm not sure about it).

Thanks!

[mesozoic]

I appreciate the offer to help, but I think I can do the coding. I'm honestly more interested in understanding why this is a requirement -- because I still don't see it. I get that we're importing a lot if an application does "from twisted.plugins import *" but I don't see how that's a functional problem, so long as it doesn't introduce bugs like the ones we've fixed together. (And I don't want to downplay how much of a help you've been, so please know it's been a great learning experience for me.)

I originally had plugins in the twisted.cred.plugins folder, and moved them to twisted.plugins for consistency. If the standard practice is to put Twisted plugins into twisted.plugins and build wrappers around them, I don't feel comfortable duplicating the tapHelper code on my own -- instead, it seems to me that "wrapper" functionality should be an integral part of twisted.plugin, so that the next person to come along and use it doesn't have to reinvent the wheel.

As a last note, refactoring twisted.cred.checkers is, in my opinion, outside of the scope of this ticket. I would rather see all interfaces live in one place -- and be consistent -- than to begin refactoring just my additions, and leave it up to someone else to finish the work. Perhaps we can open a ticket for refactoring twisted.cred to follow more current coding guidelines? Are things like "have a separate interfaces module" even documented?

I will accept this ticket because I'm willing to do the work, but I'd like to provoke some discussion (mostly on my second point above) before I just start tearing up code left and right. I'm all for getting working code into trunk as part of this branch, and making sweeping stylistic changes in a separate ticket with larger scope.

[therve]

I'll try to explain how I end up asking you this.

When a plugin is added or modifier, a cache is built (droping.cache), which contains the plugins name, doc, and interfaces. In the case of the checkers plugins, the interface ICheckerFactory is in the checkers file.

That means that everytime you're calling plugin.getPlugins(MyInterface) (MyInterface having nothing to do with cred), you end up importing checkers.

Why is it a problem? Well, in my application using twistd, I use the epoll reactor (so implicitely calling getPlugins(IReactor)). Fine. On current trunk version, this has no side effect. In your branch, the application imports a lot of things it doesn't need, like pamauth, crypt, spwd or whatever, so it uses more memory, and I think it's simply not clean.

There are already some plugins that don't do that very well (for example, lore), and that should be fixed. In the mean time, we probably don't want other plugins that do that.

I hope I've been clear and you understand what is at stake. If not, don't hesitate to come back to me.

[itamarst]

One could also argue that this is a design flaw in the plugin system.

[therve]

This is what I thought in the first place, but the plugin system only import interfaces by default. This looks like a reasonable requirement to have these interfaces with few dependencies. At least, I don't see a better design for now :).

[mesozoic]

I've moved the new interface (ICheckerFactory) into twisted.cred.icred. I think it's worth opening a separate ticket to move all of cred's interfaces into icred, to be consistent, but for the time being this will avoid the dropin.cache import problem described above.

I apologize for the long period of time it's taken for me to get back to this. I'd like to blame it on having pneumonia for the last two weeks, but that doesn't really explain the total lack of activity in October.

So, are we ready for a merge and a new ticket?

[therve]

(In [22016]) Branching to 'checker-2570-2'

[therve]

(In [22017]) Merge forward.

Refs #2570

[therve]

(In [22019]) Unused imports

Refs #2570

[therve]

(In [22020]) Move interfaces to icred, cleanups.

Refs #2570

[therve]

I made the move of the other interfaces to icred, because the current state didn't solve the problem I mentioned before. I also did a bunch fo cleanups in the process.

[mesozoic]

What do we do about other applications that might be using those interfaces in the old locations? Will backwards-compatibility imports reintroduce the problem you've identified?

(I thought the problem was that the module containing the plugin interface was being imported when dropin.cache gets processed; I'm a bit lost why the rest of the interfaces need to move as part of this ticket.)

[glyph]

It occurs to me that changing these interface names will also create backward-compatibility problems with Axiom databases. The cred interfaces are centrally used (by axiom.userbase), and axiom remembers fully qualified interface names, treating them as stable.

Even if we leave the compatibility imports in place forever, this creates an issue where an Axiom database which was opened by a newer version of twisted will be unopenable; i.e. if you create a database with Axiom 0.5.20 and Twisted 7.0, then roll back to Twisted 2.5 without rolling back your Axiom version, you have a record of interfaces in icred which no longer exist.

I'm fully willing to admit that this is really Axiom's problem, not Twisted's, but I'm not sure how to resolve it. I agree with Alex that it's a bad idea to mix these two problems together, given that only new interfaces need to live in icred to fix the problem at hand. A note in icred explaining that other interfaces predate the ixxx convention and are present in portal should be sufficient.

[therve]

Sorry I wasn't clear. The first moves solved the half of the problem: loading dropin.cache just loaded things from icred. But building the cache still needed to import checkers and all. As most of the job was done, I thought it would be cleaner to do all the moves here.

glyph, I didn't fully understand your comment. Can you clarify what you want me to do?

[exarkun]

Glyph wants the existing interfaces to stay in twisted.cred.portal. It sounds like you're saying this is a problem, though?

[therve]

I didn't move the interface in portal (IRealm), because the plugins don't use portal. I removed the interface from checkers because this file import pamauth.

[exarkun]

Oops, I didn't actually look at the code, I just read the discussion. :) Assume I said twisted.cred.checkers instead - the issue is the same.

Axiom should really be  fixed. I'm tempted to say we should just do the interface rename in this branch and Axiom will have to catch up. However, I notice that there's a fairly trivial alternate solution to the problem of importing PAM too much. If the pamauth import in checkers.py is moved into PluggableAuthenticationModulesChecker.requestAvatarId, then neither pam nor reactor gets imported. It's still more overhead than moving the interfaces to a dedicated module, but it avoids loading an extension module and prevents reactor selection from breaking.

It would be nice to have some automated checking for the dependencies of plugin modules (and other stuff too). It's far too easy to accidentally do something like this.

[exarkun]

What do you think about making the PAM import change?

(Not sure who wants to have this assigned to them now, giving it to therve since he touched it last.)

[glyph]

FWIW I am intending to do something about this while I'm on vacation, but if I actually get the time to get started, I'll steal the ticket. therve, if you want to do something about it in the meantime, please feel free.

[therve]

I have no problem with doing this. Should I put the interfaces back in the original place?

[glyph]

Replying to therve:

I have no problem with doing this. Should I put the interfaces back in the original place?

That's the gist of my feedback, yes. Thanks. It's all yours. :)

We should still address the axiom issue, but it's just one more reason why moving interfaces is problematic... there are others.

I think I'm distant enough from this branch to give it a review if you want to do the last chunk of implementation.

[therve]

(In [22250]) Move the interface back, change the import of pamauth.

Refs #2570

[therve]

This is done!

[glyph]

There's a little bit of trailing whitespace in cred.xhtml and tap.xhtml.

It would also be nice if ICheckerFactory's docstring would explain that it is used for plugins. In fact, I guess I expressed myself poorly in my previous feedback. ICheckerFactory is new, so it should be in icred; or if you don't want to create another module perhaps in strcred. Nothing in checkers uses it, so it doesn't really belong there. It was the old interfaces, the ones that existing code could be referring to, that I wanted to have left where they were before.

I leave it to you whether to create the new icred or to put the interface in strcred, but if it's not in strcred, it should at least mention strcred, since it currently doesn't have any purpose beyond that.

I made a new plugin for the PAM aspect of this already; #2970.

Please clean up the whitespace and move / document the interface, and then merge, since this is all mostly cosmetic.

[therve]

(In [22256]) Process review comments.

Refs #2570

[therve]

(In [22257]) Merge checker-2570-2

Authors: mesozoic, therve Reviewers: glyph, exarkun Fixes #2570

Add the possibility to twistd plugins to use cred checkers, via twisted.cred.strcred.AuthOptionMixin. This allows twistd plugins to accept checker via command-line arguments, and also offer a way to create other pluggable checkers. The only plugin modified for now is the words one.