Description
Julian Maurice
2012-10-31 10:26:34 UTC
Created attachment 13129 [details] Bug 8993: LDAP authentication improvements 1) allow multiple branch authentication 2) allow to use a Perl module to transform ldap entry into a Koha borrower These improvements needs koha-conf.xml to be modified. However, the old authentication mechanism is still there, and if you do not change your configuration file, it must continue to work exactly the same way. There's a POD file describing new features C4/LDAPAuthMethodTutorial.pod Created attachment 13130 [details] Bug 8993: Sample LDAP transformation module This is a sample transformation module for the new LDAP authentication system. It should not be pushed as it is customer-specific, but it should demonstrate how to implement your own. Contents of POD file: LDAP auth_method configuration This document is a step by step explaination for the new LDAP configuration method. The old one (see C4::Auth_with_ldap) still exists and you should give it a try if your needs of mappings and transformations are low. What do I have to ask to the directory administrator How to reach the service What we need is build the url of the ldap service we want to reach. So ask him for the URL. If he don't know, ask for the scheme (or protocol), the hostname and the port of the directory. Only hostname and scheme are mandatory. The scheme must be ldap or ldaps (ldaps is for crypted ldap, ldap over SSL). The url construction is: scheme://hostname:port scheme://hostname examples host=directory.example.com port=389 scheme=ldap gives you: ldap://directory.example.com:389 Also: Active Directory (the Microsoft implementation) is an LDAP alike directory but Koha have to know it's active directory to use it, so please ask. What is the binding method ? Are anonymous allowed to make some searches on the server? If not, what is the credentials for the koha account ? Credentials are a pair (DN, password) uid=koha,ou=people,dc=example,dc=com MYS3CRET in Active Directory, it can be a login@example.com with a password koha@example.com MYS3CRET Now configure koha This is a basic exemple of things you will add in the "/config" part of "koha-conf.xml". First of all, you need to tell koha that ldap authentication is now relevant <useldapserver>1</useldapserver> If you use anonymous method, you have to give credentials of koha account <ldapserver url="ldap://directory.example.com" manager="uid=koha,ou=people,dc=example,dc=com" password="MYS3CRET" authmethod="search_dn" > Also, you can set the values for replication and update (documented in legacy pod): <ldapserver url="ldap://directory.example.com" manager="uid=koha,ou=people,dc=example,dc=com" password="MYS3CRET" authmethod="search_dn" replicate="1" update="1" > You must now tell koha the ldap branches where to find users <useldapserver>1</useldapserver> <ldapserver url="ldap://directory.example.com" manager="uid=koha,ou=people,dc=example,dc=com" password="MYS3CRET" authmethod="search_dn" replicate="1" update="1" > <branch dn="ou=people,dc=site1,dc=example,dc=com" /> <branch dn="ou=people,dc=site2,dc=example,dc=com" /> <branch dn="ou=people,dc=site4,dc=example,dc=com" /> The best part of this new config is that you don't have to rely on simple mappings anymore (but you still can): you can use a perl module to write much sofisticated transformations directly in perl: <transformation module="C4::LDAPTransform::CustomTransform" /> This module only have to define a subroutine named get_borrower Created attachment 13131 [details] Bug 8993: LDAP authentication improvements 1) allow multiple branch authentication 2) allow to use a Perl module to transform ldap entry into a Koha borrower These improvements needs koha-conf.xml to be modified. However, the old authentication mechanism is still there, and if you do not change your configuration file, it must continue to work exactly the same way. There's a POD file describing new features C4/LDAPAuthMethodTutorial.pod Created attachment 13132 [details] Bug 8993: Sample LDAP transformation module Created attachment 14219 [details] [review] Bug 8993: Follow-up - LDAP authentication improvements sub SetBorrowerAttributes which is used in set_xattr is not exported. So i get the following error when login using ldap: "Software error: Undefined subroutine &C4::Auth_with_ldap::SetBorrowerAttributes called at /home/koha/src/C4/Auth_with_ldap.pm line 166, <DATA> line 522." So here is a follow-up patch. The content of attachment 13130 [details] has been deleted by Chris Cormack <chris@bigballofwax.co.nz> who provided the following reason: Asked for by Julian The token used to delete this attachment was generated at 2012-12-24 21:47:36 NZDT. The content of attachment 13132 [details] has been deleted by Chris Cormack <chris@bigballofwax.co.nz> who provided the following reason: Asked for by Julian The token used to delete this attachment was generated at 2012-12-24 21:48:42 NZDT. Created attachment 14460 [details] [review] Bug 8993: LDAP authentication improvements 1) allow multiple branch authentication 2) allow to use a Perl module to transform ldap entry into a Koha borrower These improvements needs koha-conf.xml to be modified. However, the old authentication mechanism is still there, and if you do not change your configuration file, it must continue to work exactly the same way. There's a POD file describing new features C4/LDAPAuthMethodTutorial.pod Created attachment 14461 [details] [review] Bug 8993: Follow-up - LDAP authentication improvements Patches rebased on master. Removed 2 scripts from the first patch that were not intended to be included. The content of attachment 13129 [details] has been deleted by Chris Cormack <chris@bigballofwax.co.nz> without providing any reason. The token used to delete this attachment was generated at 2013-01-11 06:38:38 NZDT. The content of attachment 13131 [details] has been deleted by Chris Cormack <chris@bigballofwax.co.nz> without providing any reason. The token used to delete this attachment was generated at 2013-01-11 06:39:27 NZDT. The patch is applied in our production environment which has several LDAP branches on a single LDAP server. It is fully working. Patch tested on an instance with a "regular" single branch LDAP : everything working as expected, no regression! Sorry fir butting in here but I am confused. Is this intended to be pushed or just for a customer? Elliott Hi Elliott, this patch is not customer-specific and is intended to be integrated into master. This change will conflict with Bug 7973 so it should probably be re-based if 7973 gets into master first. I think we should also provide example transformation module, because it's non-obvious how to write transformation module from looking at code alone (at least for me). Good example of transformation module might be one which converts repeatable values in ldap into multiple fields for easier import into koha since right now koha just concatenates all values with spaces which is sub-optimal if you want just one value from repeatable field. I would love to write one, but I don't really know where to start. I also don't like that existing LDAP configuration stops working with this patch applied (think about package upgrade) and only indication that something changed is message in error log: "deprecated ldap configuration, see documentation". It might at least point to C4/LDAPAuthMethodTutorial.pod as source of that documentation, but ideal solution would be to support old configuration if possible. Created attachment 16659 [details] [review] Bug 8993 - my attempt to get this module working QA note: please fail this patch. With all this changes, I'm still not able to login using following configuration: <ldapserver uri="ldaps://ldap.ffzg.hr" authmethod="search_dn" replicate="1" update="0" userid_from="uid" > <branch dn="dc=ffzg,dc=hr" /> <transformation module="C4::LDAPTransform::RepeatableValues" /> </ldapserver> This should be one of more simple configuration, and it doesn't work with this patch. While I would love to have LDAP transformations in perl code, I think this patch is too big rewrite with a lot of edge cases which aren't convered. Created attachment 16796 [details] [review] Bug 8993 [Follow-up] update and replicate options used at wrong place update was used instead of replicate and vice versa Also the package name of LDAPTransformation module was wrong. With this patch and Dobrica's patch applied, I'm able to get into the login process: ldap binding is successful and if user is not existing in koha database, it is created. Note: to really be able to login I had to modify C4::LDAPTransform::RepeatableValues to fill values for categorycode and branchcode since user creation fails if they are empty. We should include all required fields (categorycode and branchcode) in example plugin so people don't have to look at this bug to find that out :-) OOH, now that I have had enough sleep, is there really a good reason to introduce completely different configuration for LDAP server? Adding <branch>, <transform> and few other tags into existing configuration would probably result in simpler code and easier upgrade path. Created attachment 18804 [details] [review] Bug 8993: LDAP authentication improvements Rebased on master Created attachment 18805 [details] [review] Bug 8993: Follow-up - LDAP authentication improvements Rebased on master Created attachment 18806 [details] [review] Bug 8993 - my attempt to get this module working Rebased on master Created attachment 18807 [details] [review] Bug 8993 [Follow-up] update and replicate options used at wrong place Rebased on master Created attachment 18808 [details] [review] Bug 8993: Add branchcode and categorycode mappings for sample module (In reply to comment #23) > OOH, now that I have had enough sleep, is there really a good reason to > introduce completely different configuration for LDAP server? Adding > <branch>, <transform> and few other tags into existing configuration would > probably result in simpler code and easier upgrade path. If you are refering to the use of xml attributes instead of xml tags, I think you can use the one you prefer, as: <ldapserver update="1" /> and <ldapserver> <update>1</update> </ldapserver> will both result in a hash: { update => "1" } Applying: Bug 8993: LDAP authentication improvements Using index info to reconstruct a base tree... M C4/Auth_with_ldap.pm Falling back to patching base and 3-way merge... Auto-merging C4/Auth_with_ldap.pm CONFLICT (content): Merge conflict in C4/Auth_with_ldap.pm Patch failed at 0001 Bug 8993: LDAP authentication improvements The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Created attachment 21345 [details] [review] Bug 8993: LDAP authentication improvements 1) allow multiple branch authentication 2) allow to use a Perl module to transform ldap entry into a Koha borrower These improvements needs koha-conf.xml to be modified. However, the old authentication mechanism is still there, and if you do not change your configuration file, it must continue to work exactly the same way. There's a POD file describing new features C4/LDAPAuthMethodTutorial.pod Created attachment 21346 [details] [review] Bug 8993: Follow-up - LDAP authentication improvements Created attachment 21347 [details] [review] Bug 8993 - my attempt to get this module working QA note: please fail this patch. With all this changes, I'm still not able to login using following configuration: <ldapserver uri="ldaps://ldap.ffzg.hr" authmethod="search_dn" replicate="1" update="0" userid_from="uid" > <branch dn="dc=ffzg,dc=hr" /> <transformation module="C4::LDAPTransform::RepeatableValues" /> </ldapserver> This should be one of more simple configuration, and it doesn't work with this patch. While I would love to have LDAP transformations in perl code, I think this patch is too big rewrite with a lot of edge cases which aren't convered. Created attachment 21348 [details] [review] Bug 8993 [Follow-up] update and replicate options used at wrong place update was used instead of replicate and vice versa Also the package name of LDAPTransformation module was wrong. Created attachment 21349 [details] [review] Bug 8993: Add branchcode and categorycode mappings for sample module All patches rebased on master Patch applied cleanly, go forth and signoff Applying: Bug 8993: LDAP authentication improvements Using index info to reconstruct a base tree... M C4/Auth_with_ldap.pm Falling back to patching base and 3-way merge... Auto-merging C4/Auth_with_ldap.pm CONFLICT (content): Merge conflict in C4/Auth_with_ldap.pm Patch failed at 0001 Bug 8993: LDAP authentication improvements The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Any chance of rebasing this again Julian.. I'm hoping to test it. Created attachment 22580 [details] [review] Bug 8993: LDAP authentication improvements Rebased on master Created attachment 22581 [details] [review] Bug 8993: Follow-up - LDAP authentication improvements Rebased on master Created attachment 22582 [details] [review] Bug 8993 - my attempt to get this module working Rebased on master Created attachment 22583 [details] [review] Bug 8993 [Follow-up] update and replicate options used at wrong place Rebased on master Created attachment 22584 [details] [review] Bug 8993: Add branchcode and categorycode mappings for sample module Rebased on master I rebased all patches but not tested them. Hope it will works :) Great fast response Julian.. unfortunately I still can't get it to apply :( Falling back to patching base and 3-way merge... Auto-merging C4/Auth_with_ldap.pm CONFLICT (content): Merge conflict in C4/Auth_with_ldap.pm Failed to merge in the changes. Patch failed at 0001 Bug 8993: LDAP authentication improvements Looking at the merge markers, it looks like the main routine 'checkpw_ldap' is the issue, but it's such an extensive re-write that I don't want to screw it up by trying to merge it myself. Hmm I forgot to fetch the git remote before rebasing... Rebased patches are coming. Created attachment 22588 [details] [review] Bug 8993: LDAP authentication improvements Rebased on master Created attachment 22589 [details] [review] Bug 8993: Follow-up - LDAP authentication improvements Rebased on master Created attachment 22590 [details] [review] Bug 8993 - my attempt to get this module working Rebased on master Created attachment 22591 [details] [review] Bug 8993 [Follow-up] update and replicate options used at wrong place Rebased on master Created attachment 22592 [details] [review] Bug 8993: Add branchcode and categorycode mappings for sample module Rebased on master Thanks Julian, Applies beautifully... now for some testing :) Some of this may also be obsolete. :( Created attachment 38270 [details] [review] Bug 8993: LDAP authentication improvements 1) allow multiple branch authentication 2) allow to use a Perl module to transform ldap entry into a Koha borrower These improvements needs koha-conf.xml to be modified. However, the old authentication mechanism is still there, and if you do not change your configuration file, it must continue to work exactly the same way. There's a POD file describing new features C4/LDAPAuthMethodTutorial.pod Created attachment 38271 [details] [review] [DO NOT PUSH] Bug 8993: Sample LDAP transformation module Patches rebased and cleaned. All indentation-only stuff was removed from patch to ease reading it (and make it easier to apply in the future). Created attachment 38273 [details] [review] Bug 8993: LDAP authentication improvements 1) allow multiple branch authentication 2) allow to use a Perl module to transform ldap entry into a Koha borrower These improvements needs koha-conf.xml to be modified. However, the old authentication mechanism is still there, and if you do not change your configuration file, it must continue to work exactly the same way. There's a POD file describing new features C4/LDAPAuthMethodTutorial.pod Created attachment 38274 [details] [review] [DO NOT PUSH] Bug 8993: Sample LDAP transformation module Apparently it is not possible to specify in the LDAP XML configuration something such as: <base>OU=subunit,OU=unit,OU=supraunit,DC=dc1,DC=dc0;OU=othersubunit,OU=unit,OU=supraunit,DC=dc1,DC=dc0</base> Only one search context is allowed at a given time, i.e., <base>OU=subunit,OU=unit,OU=supraunit,DC=dc1,DC=dc0</base> As far as I understand (sorry, very new to Koha and perl) this patch will solve the problem? (I have one solution working already, with very minor changes to Auth_with_ldap.pm, based on this idea: my @base = split(';',$ldap->{base}) or die ldapserver_error('base'); but would like to see a solution upstream) (In reply to Rui from comment #60) > Apparently it is not possible to specify in the LDAP XML configuration > something such as: > > <base>OU=subunit,OU=unit,OU=supraunit,DC=dc1,DC=dc0;OU=othersubunit,OU=unit, > OU=supraunit,DC=dc1,DC=dc0</base> > It is possible with this patch, try something like this: <ldapserver uri="ldap://directory.example.com" manager="uid=koha,ou=people,dc=example,dc=com" password="MYS3CRET" authmethod="search_dn" replicate="1" update="1"> <branch dn="ou=people,dc=site1,dc=example,dc=com" /> <branch dn="ou=people,dc=site2,dc=example,dc=com" /> <branch dn="ou=people,dc=site4,dc=example,dc=com" /> </ldapserver> This example comes from C4/LDAPAuthMethodTutorial.pod Will this patch make it possible to configure LDAPS? I'm an LDAP noob, but working with a customer I have managed to get LDAPS working, by changing line 109 of C4/Auth_with_ldap.pm +109 from this: my $db = Net::LDAP->new(\@hosts); to this: my $db = Net::LDAP->new(\@hosts, version => 3, scheme => 'ldaps', port => 123, capath => '/usr/ssl/certs'); As far as I can tell, this patch does not touch that line, but I might be missing something... If this patch does not make LDAPS configurable, and if noone objects loudly, I'll do a patch on another bug that makes it possible to set version, scheme, port and capath in koha-conf.xml A few notes: - update_local: you remove "my $password" while it's used below - no doc on authmethod - not sure about standard regarding xml attributes vs sub elements. - Should explain what happens with old elements (auth_by_bind, timeout, etc...) - debug_msg "ldap $_:", $msg->$_ for qw/ error code /; fails if $msg is undef - function cnx returns an hash in case of error, then checks for undef to exit. It failed, does not exit, then crash further trying to use the $cnx. I really need this improvement, but lot of work needs to be done before QA gets on it. It works like I need it to. So this feature gets my full support. Now, to get it through QA, the points above (and many others) will need to be addressed. If you need someone to sign it off, I can work with you while I have the damned LDAP knowledge in my mind AND and environment allowing me to test it. Wait too long, and that's dead. I like the flexibility it offers. I could do all the complex mapping I wanted in my module. But I had to explore and put a lot of "warn" in the code to get there. Better doc would be helpful. Btw, annoying that <mapping> <userid is="name"></userid> </mapping> is still needed in koha-conf.xml. The last mapping, but without it I couldn't get through. The last 2 comments from Blou read like FQA - setting status. Julian, can you please comment?
> If you need someone to sign it off, I can work with you while I have the
> damned LDAP knowledge in my mind AND and environment allowing me to test it.
> Wait too long, and that's dead.
I assume that time window has closed now :(
The code here is 5+ years old now. Should we just close the bug? we still used the code within the last year as-is. Not saying the code should be in the master as-is, but the feature is useful to solve certain complicated LDAP environments. It's not like the school's library can dictate its IT department how to manage their systems, so customized mapping and multi access-points are a plus... I'm not willing to continue on this bug, so I don't mind closing it. I'm removing myself from the assignee field. If still useful, it should probably be rewritten with plugins in mind. Supporting multiple LDAP just came up here recently in a discussion about features that would be useful - maybe we should leave this open - even if the code is not ready for integration, it's still an enhancement request. We could reset it to 'NEW' maybe? (In reply to Katrin Fischer from comment #70) > Supporting multiple LDAP just came up here recently in a discussion about > features that would be useful - maybe we should leave this open - even if > the code is not ready for integration, it's still an enhancement request. We > could reset it to 'NEW' maybe? Hopefully bug 31378 could pave a path to making it easier to add multiple LDAP providers. I don't think I have any Koha clients using LDAP but it's not particularly complicated. With LDAP you get back a directory record, with OIDC you get a list of claims, with SAML you get a list of attributes. They're all amenable to the same type of mapping mechanism. |