John Peacock wrote:
> Elliot F wrote:
>
>> 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).
Ah, yes, thank you.
> 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;
> # etc
Sounds great, and I'll do this for this release. Eventually, however, we may
want to have a central LDAP (or MYSQL, or whatever repository) config. I (for
one) would not like to configure the same information for each LDAP plugin I use
(LDAP recip verification, LDAP auth, LDAP user pref, etc.)
How about a single ldap_config file that contains the generic LDAP settings
(host, port, baseDN, bindDN, bindPW), but have the plugin specific settings as
ARGs (ldap_auth_bind_filter.) That sounds fairly reasonable to me.
Additionally, the filter would accept arguments as well, in case settings should
be overwritten (if different Directories are used for different items.) Sound
fair?
> 3) 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.
That makes sense, in case there is more than one plugin for a given hook. I
will make that change as well.
> HTH
>
> John
