Bug 28385 - LDAP server configuration broken since migration from XML::Simple
Summary: LDAP server configuration broken since migration from XML::Simple
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Testopia
URL:
Keywords:
Depends on: 28278
Blocks: 28587
  Show dependency treegraph
 
Reported: 2021-05-19 10:24 UTC by Dobrica Pavlinusic
Modified: 2021-06-19 11:46 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 28385: ldap server configuration broken since migration from XML::Simple (1.40 KB, patch)
2021-05-19 10:29 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 28385: ldap server configuration broken since migration from XML::Simple (1.43 KB, patch)
2021-05-19 10:40 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 28385: ldap server configuration broken since migration from XML::Simple (1.50 KB, patch)
2021-05-20 15:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28385: ldap server configuration broken since migration from XML::Simple (1.99 KB, patch)
2021-05-20 22:14 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28385: Add tests (2.31 KB, patch)
2021-05-21 06:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28385: Add tests (2.37 KB, patch)
2021-05-21 14:57 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28385: Use ldapserver only if exists (1.65 KB, patch)
2021-05-25 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Dobrica Pavlinusic 2021-05-19 10:24:15 UTC

    
Comment 1 Dobrica Pavlinusic 2021-05-19 10:29:25 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.
Comment 2 Dobrica Pavlinusic 2021-05-19 10:36:50 UTC
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.
Comment 3 Dobrica Pavlinusic 2021-05-19 10:40:05 UTC
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.
Comment 4 Victor Grousset/tuxayo 2021-05-19 14:47:19 UTC
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
Comment 5 David Cook 2021-05-19 23:37:13 UTC
(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.
Comment 6 Martin Renvoize 2021-05-20 08:17:37 UTC
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 :)
Comment 7 Dobrica Pavlinusic 2021-05-20 11:12:41 UTC
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? ;-)
Comment 8 Martin Renvoize 2021-05-20 15:03:31 UTC
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>
Comment 9 Martin Renvoize 2021-05-20 15:05:00 UTC
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.
Comment 10 Victor Grousset/tuxayo 2021-05-20 22:14:33 UTC
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>
Comment 11 Victor Grousset/tuxayo 2021-05-20 22:15:10 UTC
It works :)

Tested with a public LDAP server. See the added test plan for details.
Comment 12 Victor Grousset/tuxayo 2021-05-20 22:28:46 UTC
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.
Comment 13 David Cook 2021-05-21 01:27:00 UTC
(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...
Comment 14 Jonathan Druart 2021-05-21 06:48:45 UTC
Created attachment 121265 [details] [review]
Bug 28385: Add tests

This does not test bug 28385 directly but 28278 instead.
Comment 15 Victor Grousset/tuxayo 2021-05-21 14:57:02 UTC
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>
Comment 16 Jonathan Druart 2021-05-25 07:45:11 UTC
Considering Victor's SO a QA stamp.
Comment 17 Jonathan Druart 2021-05-25 07:46:26 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 18 Jonathan Druart 2021-05-25 08:35:30 UTC
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.
Comment 19 Jonathan Druart 2021-05-25 08:37:59 UTC
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.
Comment 20 Jonathan Druart 2021-05-25 08:38:54 UTC
Follow-up pushed to master.
Comment 21 Fridolin Somers 2021-05-25 10:24:08 UTC
Depends on Bug 28278 not in 20.11.x