Bug 17775

Summary: Add new user with LDAP not works under Plack
Product: Koha Reporter: Mohammed Nashbat <mnashbat>
Component: AuthenticationAssignee: Liz Rea <liz>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: blocker    
Priority: P5 - low CC: alex.arnaud, costalc, dpavlin, fridolin.somers, jonathan.druart, julian.maurice, katrin.fischer, kyle, liz, martin.renvoize, mtj
Version: unspecified   
Hardware: All   
OS: Linux   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7172    
Attachments: Bug 17775 - Add new user with LDAP not works under Plack
y# Attachment to Bug 17775 - Add new user with LDAP not works under Plack
Bug 17775 - Add new user with LDAP not works under Plack

Description Mohammed Nashbat 2016-12-14 13:22:43 UTC
Koha & ldap authentication fail authenticating new users when
enabling plack, it works fine only if plack is desabled. Ldap authentication works fine with old registered users.
Comment 1 Zeno Tajoli 2016-12-14 13:52:59 UTC
More on this bug: the problem is add new user from LDAP, using the automatic population of Koha borrower table by LDAP.
Comment 2 Katrin Fischer 2016-12-22 13:16:17 UTC
This is a rather bad bug, can we do something about it?
Comment 3 Jonathan Druart 2017-01-13 10:45:38 UTC
Is anybody interested in fixing this bad bug?
Comment 4 Alex Arnaud 2017-01-19 09:08:29 UTC
Hello Jonathan,

We plan to work on this bug with Fridolin during our tech camp (30/01 -> 03/02).
I'm comfortable with LDAP things and Fridolin with plack.
Comment 5 Liz Rea 2017-01-20 02:48:50 UTC
A slightly more concerning part of this, is after you fix the more obvious problem of C4::AddMember being undefined (even though it's technically there), there is a problem with the sessions between the OPAC and the intranet - i.e. the next action a logged in staff user will make is "permission denied" requiring them to log in again. 

I'm still testing to see if this is specifically to do with "replicate" or if it happens with LDAP logins generally.

Liz
Comment 6 Liz Rea 2017-01-20 02:56:09 UTC
and when that ldap user logs out, the staff user, no matter who it is, is logged out as well.
Comment 7 Dobrica Pavlinusic 2017-01-30 11:44:29 UTC
Created attachment 59644 [details] [review]
Bug 17775 - Add new user with LDAP not works under Plack

This patch fixes internal server error:

Undefined subroutine &C4::Auth_with_ldap::AddMember called at /srv/koha_ffzg/C4/Auth_with_ldap.pm line 213.

It occurs only under plack, and it's strange since C4::Members
does EXPORT AddMember and we are importing it into Auth_with_ldap.pm
(and it does work under CGI).
Comment 8 Dobrica Pavlinusic 2017-01-30 11:45:34 UTC
Alex, sorry to steal this one from you, but we hit this one when we deploy master in production, so here if fix for it.
Comment 9 Liz Rea 2017-01-30 20:31:20 UTC
Hi, 
I thought this was the problem too - but there is another problem. Sessions created for the LDAP created user hijack any sessions from the same IP, including staff ones. I don't think of it as a separate bug - I think symptoms of the same problem.

Cheers,
Liz
Comment 10 Liz Rea 2017-01-31 02:31:34 UTC
Actually never mind, this sessions thing is a problem generally with plack, with or without LDAP.

I only wish I had submitted my identical patch 2 weeks ago >.<

Thanks for fixing this, Dobrica!

Liz
Comment 11 Liz Rea 2017-01-31 02:33:30 UTC
Created attachment 59662 [details] [review]
y# Attachment to Bug 17775 - Add new user with LDAP not works under Plack

Bug 17775 - Add new user with LDAP not works under Plack

This patch fixes internal server error:

Undefined subroutine &C4::Auth_with_ldap::AddMember called at /srv/koha_ffzg/C4/Auth_with_ldap.pm line 213.

It occurs only under plack, and it's strange since C4::Members
does EXPORT AddMember and we are importing it into Auth_with_ldap.pm
(and it does work under CGI).

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Comment 12 Jonathan Druart 2017-01-31 08:51:14 UTC
Having a quick look at C4::Auth_with_ldap I am pretty sure there are other bugs under plack. Variable scoping in this module is very bad, have a look at $ldap, $prefhost, $base,...
Comment 13 Dobrica Pavlinusic 2017-01-31 14:05:50 UTC
(In reply to Liz Rea from comment #9)
> Hi, 
> I thought this was the problem too - but there is another problem. Sessions
> created for the LDAP created user hijack any sessions from the same IP,
> including staff ones. I don't think of it as a separate bug - I think
> symptoms of the same problem.
> 
> Cheers,
> Liz

Liz, can you expand on this a little bit more? I know that login into OPAC or Intranet will be transferred between then (so if you login into intranet you will also be logged into opac). Is that what you are having problem with?

This behavior is because we are registering cookies on hostname (whole hostname) so if OPAC and intranet are on the same hostname (but different path or ports) they will share session. If they are on different hostnames, they will not share sessions.

Does this make sense?
Comment 14 Dobrica Pavlinusic 2017-01-31 14:07:16 UTC
(In reply to Jonathan Druart from comment #12)
> Having a quick look at C4::Auth_with_ldap I am pretty sure there are other
> bugs under plack. Variable scoping in this module is very bad, have a look
> at $ldap, $prefhost, $base,...

Yes, it's very bad code but we haven't hit any bugs in it (yet) and we'll need to rewrite it for DBIx anyway.
Comment 15 Liz Rea 2017-01-31 20:42:34 UTC
Just forget I said anything at all.

Patch fixes the issue, and it's quite a bad one.
Comment 16 Jonathan Druart 2017-02-01 08:17:41 UTC
Created attachment 59722 [details] [review]
Bug 17775 - Add new user with LDAP not works under Plack

This patch fixes internal server error:

Undefined subroutine &C4::Auth_with_ldap::AddMember called at /srv/koha_ffzg/C4/Auth_with_ldap.pm line 213.

It occurs only under plack, and it's strange since C4::Members
does EXPORT AddMember and we are importing it into Auth_with_ldap.pm
(and it does work under CGI).

Signed-off-by: Liz Rea <liz@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
I did not test but trust author and signoffer. The change cannot hurt.
Comment 17 Kyle M Hall 2017-02-07 17:45:21 UTC
Pushed to master for 17.05, thanks Dobrica!
Comment 18 Katrin Fischer 2017-02-07 21:08:03 UTC
This patch has been pushed to 16.11.x and will be in 16.11.04.
Comment 19 Julian Maurice 2017-02-09 09:29:37 UTC
Pushed to 3.22.x for 3.22.17
Comment 20 Mason James 2017-02-22 23:07:39 UTC
Pushed to 16.05.x, for 16.05.10 release