|
Description
Dobrica Pavlinusic
2021-05-19 10:24:15 UTC
Created attachment 121147 [details] [review] Bug 28385: ldap server configuration broken since migration from XML::Simple Bug 28278 removed usage of XML::Simple which had side-affect that C4::Context->config("ldapserver") returns one more level of hash based on id of element, so ldap configuration was no longer available generating error in log: No ldapserver "hostname" defined in KOHA_CONF: /etc/koha/sites/srvgit/koha-conf.xml which is very confusing since hostname is there, but we are loging at wrong place for it. We don't have any other place in koha-conf.xml where we use same tag and id so I think that this is best place to put fix into as opposed to emulation of XML::Simple behavior. Created attachment 121148 [details] [review] Bug 28385: ldap server configuration broken since migration from XML::Simple Bug 28278 removed usage of XML::Simple which had side-affect that C4::Context->config("ldapserver") returns one more level of hash based on id of element, so ldap configuration was no longer available generating error in log: No ldapserver "hostname" defined in KOHA_CONF: /etc/koha/sites/srvgit/koha-conf.xml which is very confusing since hostname is there, but we are looking at wrong place for it. What would be the test plan? I would like to try to test this even though I don't know much about LDAP. No need for it to be an "explain like I'm five" test plan though ^^ It can looks like this: 1. Step to prepare some stuff 2. Another step 3. Do something else that exposes the current issue 4. Apply the patch 5. Redo some of the above steps 6. See that the issue is gone (In reply to Dobrica Pavlinusic from comment #2) > We don't have any other place in koha-conf.xml where we use same tag and id > so I think that this is best place to put fix into as opposed to emulation > of XML::Simple behavior. Agreed. This works as expected.. however I wonder if we ought to support both <ldapserver> and <ldapserver id="ldapserver"> syntax and perhaps start recommending the less verbose <ldapserver> option? Also, nice to see you still around Dobrica :) I was wondering about dropping id and there is also ticket about multiple ldap servers which would have different ids, but we need to support id="ldapserver" for upgraded instances, so I decided to start here.
I also considered adding something like:
foreach my $ldap_id ( keys %$ldap ) {
if ( exists($ldap->{$ldap_id}->{hostname}) ) {
$ldap = $ldap->{$ldap_id};
last;
}
}
but it seemed like too much code. It would support <ldapserver> without any id and different id from ldapserver, but since we suggest id="ldapserver" is included in documentation and koha-conf.xml template I decided against it.
Was I wrong about this? ;-)
Created attachment 121244 [details] [review] Bug 28385: ldap server configuration broken since migration from XML::Simple Bug 28278 removed usage of XML::Simple which had side-affect that C4::Context->config("ldapserver") returns one more level of hash based on id of element, so ldap configuration was no longer available generating error in log: No ldapserver "hostname" defined in KOHA_CONF: /etc/koha/sites/srvgit/koha-conf.xml which is very confusing since hostname is there, but we are looking at wrong place for it. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> OK, I think I agree, this is the most pragmatic solution for now.. great to know you've thought it all through though Dobrica. It works as expected, so signing off. Created attachment 121263 [details] [review] Bug 28385: ldap server configuration broken since migration from XML::Simple Bug 28278 removed usage of XML::Simple which had side-affect that C4::Context->config("ldapserver") returns one more level of hash based on id of element, so ldap configuration was no longer available generating error in log: No ldapserver "hostname" defined in KOHA_CONF: /etc/koha/sites/srvgit/koha-conf.xml which is very confusing since hostname is there, but we are looking at wrong place for it. == Test plan == 1. Setup an LDAP server with users or use a public one See https://wiki.koha-community.org/wiki/Ldap_testing 2. Configure Koha to use it, see example config above 3. Go to OPAC 4. Koha doesn't compile due to LDAP config not parsed correctly 5. Apply the patch 6. Koha compiles and page loads 7. Log in successfully with one of the LDAP users 8. Bonus: Try to log-in with a wrong username & a good password and vice-versa Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> It works :) Tested with a public LDAP server. See the added test plan for details. QA script reports missing POD (in-code Perl documentation) The relevant code is not even in a subroutine https://git.koha-community.org/Koha-community/Koha/src/branch/master/C4/Auth_with_ldap.pm#L54 so I don't know what should be done QA wise, if there is something to do. (In reply to Martin Renvoize from comment #6) > This works as expected.. however I wonder if we ought to support both > <ldapserver> and <ldapserver id="ldapserver"> syntax and perhaps start > recommending the less verbose <ldapserver> option? My hot take would be to look at migrating from koha-conf.xml to koha-conf.yml heh. Down with koha-conf.xml... Created attachment 121265 [details] [review] Bug 28385: Add tests This does not test bug 28385 directly but 28278 instead. Created attachment 121279 [details] [review] Bug 28385: Add tests This does not test bug 28385 directly but 28278 instead. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Considering Victor's SO a QA stamp. Pushed to master for 21.05, thanks to everybody involved! Created attachment 121361 [details] [review] Bug 28385: Use ldapserver only if exists Fix test t/db_dependent/Auth_with_ldap.t .. 1/4 # Failed test 'use C4::Auth_with_ldap;' # at t/db_dependent/Auth_with_ldap.t line 123. # Tried to use 'C4::Auth_with_ldap'. # Error: No ldapserver "id="ldapserver"" defined in KOHA_CONF: /etc/koha/sites/kohadev/koha-conf.xml at /kohadevbox/koha/C4/Auth_with_ldap.pm line 55. # Compilation failed in require at t/db_dependent/Auth_with_ldap.t line 123. # BEGIN failed--compilation aborted at t/db_dependent/Auth_with_ldap.t line 123. This last patch fixes failing tests in Auth_with_ldap.t and actually restores the previous behaviour (don't explode if no ldapserver entry defined). I am not saying it's the correct behaviour, but how it behave before this change. Follow-up pushed to master. Depends on Bug 28278 not in 20.11.x |