So, not that I'm replying to my own post, but (other than the input validation issue) what do you guys think? What changes do I need to make before someone commits this?
There are a couple of things I'd like to see changed before I commit this:
1) The current code retrieves the 5 configuration files for each transaction, even though they are unlikely to change after loading; it would be better if these were obtained during register() and stashed in the plugin object itself (which, if you are concerned about performance and running the forkserver implementation, is a persistent object).
2) In addition, each of the 5 configuration values is a single value, so I would prefer that there either be a single external configuration file containing all values or better yet, just make them config line options (as name/value pairs); there's a couple of ways to do the latter very compactly. My current favorite is:
%{ $self->{"_args"} } = @args;works very nice (you get a big fat warning if you have unbalanced key/value pairs). Then you can check for default values one line at a time:
$self->{"_args"}->{"ldhost"} ||= "localhost";
$self->{"_args"}->{"ldPORT"} ||= 389;
# etc3) Don't DENY unless you are absolutely sure this is going to be the only auth module loaded. In particular, return DECLINED if any of the mandatory configuration values are unset, so someone could potentially add this to a running config and then set it up without disturbing existing users. It is almost always best to DECLINED and let some other plugin take over than to DENY and block further action. It is not inconceivable that someone wanting to run this LDAP plugin already has other auth methods enabled and cannot convert all users to LDAP at the same time.
On the theory of return values from plugins, I like to look at it this way:
1) return DENY or OK if the plugin is a _positive test_, i.e. this host *is* on a blacklist, this e-mail address *is* banned, or this host is in the relayclients list;
2) return DECLINED if the plugin is a _negative test_, i.e. this host address *isn't* on any blacklist, or this host *isn't* in relayclients , etc.
The idea is that a _postive_ test is for a specific condition which, if met, absolutely determines the next action. A _negative_ test simply states that some condition hasn't *yet* been met, but the next plugin might succeed.
HTH
John
