Patch for 'ldapauth' module with fixes/improvements

[rossigee]

Attached a patch that fixes some behavioural problems with the 'ldapauth' module.

Firstly, our LDAP server contains only scant information on each user. Currently, the only data we trust in it is the login, passwords and group memberships. Therefore, we have disabled the behaviour whereby on a successful login, the LDAP details are used to update the 'go_users' record. This was imperative, as we have set up all the 'go_users' correctly, and rely on them to be correct for our custom modules to work as expected. The old behaviour may still be desirable in the majority of cases, so I would recommend making this condition configurable if this patch is to be considered for upstream.

So, given that it no longer updates the 'go_users' table, the ldapauth class has to log the user in during the 'before_login' hook (with $GO_SECURITY->logged_in($user_id), if they have successfully authenticated against the LDAP server. This is really all we were hoping and expecting the ldapauth module to do. To complete this, it was necessary to update 'auth' to only authenticate against the 'go_users' table if none of the 'before_login' hooks had agreed to log the user in already.

And, as 'ldapauth' and 'imapauth' should be siblings (i.e. no direct dependency), I figured that it would be better for it ldapauth not to inherit imapauth, but rather to instantiate it in order to make use of it's 'get_domain_config' function *only if* the 'imapauth' module exists and is enabled.

Hope this helps. Please feel free to include upstream, as-is, in part or modified, as you see fit.

Regards,

--
Ross

[rossigee]

Another patch, this one fixes a minor regression against the latest release - fixing the 'ldap_connect' call in the case that the host specified is an LDAP URL, and no port number is specified (because it is implied in the URL).

--
Ross

[Intermesh]

Thanks, I've implemented it.

[rossigee]

Great! Thanks.

I've got a follow-up patch that takes care of the loose-ends from the first patch. In particular, we needed to connect to the LDAP connection using an LDAP URL (specifying host, port and tls options seperately too cumbersome, URL's being the de-facto method these days), and bind using a DN that is not part of the 'people' OU.

This time, I've prepared and tested it against a fresh GO trunk checkout to make integration easier. In summary...


Main Changes
------------

* Ability to connect to SSL-based LDAP servers (add URL-based config parameters)
* Backwards-compatible for instances still configured using old LDAP config parameters.
* Ability to bind to the LDAP server using any DN (replace user/pass config with 'binddn' and 'bindpw').
* Backwards-compatible with old config params.
* Refactor main functionality of 'before_login' hook into smaller functions, for simplicity/readability. Add comments.
* Simplify 'add', 'modify' and 'delete' methods on LDAP class. Duplicated code unnecessary, as caller should really have connected first anyway.
* Add a simple 'get_connection()' method, allowing calling modules (e.g. LDAP address book or directory management modules) to get an LDAP handle without duplicating code between them.
* Create 'ldap_create_groupoffice_account' configuration parameter, so GO accounts are only created from successful LDAP authentications if explicitly configured to be.
* Create 'ldap_disable_password_update' configuration parameter, to allow the user to disable the (often undesirable) behaviour of LDAP passwords getting copied into GroupOffice's database. IMHO, it should probably really be disabled by default and configurable explicitly (?).
* Create 'ldap_create_mailboxes_for_email_domain' to explicitly enable the attempt to create mailboxes in conjunction with the imapauth module.

Also attached, updated source for the following wiki page to cover new configuration parameters.

http://www.group-office.com/wiki/IMAP_o ... entication

Please accept these patches with our best regards.

--
Ross
Agent Design Ltd

[Intermesh]

I don't want to implement this in 3.5 because I'm afraid it might break some customers configuration. I'm going to create a 3.6 branch and beta release tomorrow. Let's apply it there and do some extensive testing.

[rossigee]

Updated patch which fixes a bug where 'ldap://localhost/' is wrongly used instead of the configured 'ldap_url'.

[Intermesh]

Can you create a patch for the current trunk version?