mechanism for specifying a cred checker for twistd plugins to use #2570
Comments
Isn't that what http://twistedmatrix.com/trac/wiki/Specification/AdministrativeComponentSelection is for? |
Replying to itamarst:
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 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. |
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 (#10475, #11359, #2571). We should probably define some priorities between all of this. |
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. |
I don't believe #10475 or #11359 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. |
Review me please. |
General comments:
that's almost done everywhere but not quite.
test_tap.py:
cred_passwd.py:
cred_unix.py:
cred_anonymous.py:
cred_memory.py:
strcred.py:
test_strcred.py:
checkers.py:
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! |
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? |
The things I would test for auth_list:
The exact output string doesn't really matter. |
The latter test could be done with assertRaises(SystemExit, auth_list, ...). |
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! |
Great, some other comments:
Thanks! |
I like it! |
Also, maybe add a plugin for PAM? |
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. |
Replying to mesozoic:
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 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. |
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. |
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. |
I've committed a change to the tests and the code that makes options['credInterfaces'] 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. |
Cool. A few aesthetics comments remaining:
That's it! |
Re: whitespace; the guidelines we've been using at Divmod are:
This can resolve ambiguities as to "how many blank lines are enough" :). |
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. |
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? |
Replying to itamarst:
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. |
Some more comments:
If you think #4 is out of scope, open a ticket for it. |
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. |
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. |
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. |
I believe I've resolved the issues you listed above.
Let me know if you see any other reasons this couldn't be merged. |
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:
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 I get The message could probably be improved (plugin only providing an unsupported interface? I'm not sure about it). Thanks! |
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. |
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 That means that everytime you're calling 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. |
One could also argue that this is a design flaw in the plugin system. |
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 :). |
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? |
(In [22016]) Branching to 'checker-2570-2' |
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. |
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.) |
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 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 |
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? |
Glyph wants the existing interfaces to stay in |
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. |
Oops, I didn't actually look at the code, I just read the discussion. :) Assume I said 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 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. |
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. |
I have no problem with doing this. Should I put the interfaces back in the original place? |
Replying to therve:
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. |
This is done! |
There's a little bit of trailing whitespace in It would also be nice if I leave it to you whether to create the new 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. |
(In [22257]) Merge checker-2570-2 Authors: mesozoic, therve Add the possibility to twistd plugins to use cred checkers, via |
|
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.
Searchable metadata
The text was updated successfully, but these errors were encountered: