Bug 16719

Summary: LDAP: Using empty strings as 'failsafe' attribute mapping defeats database constraints
Product: Koha Reporter: Oliver Behnke <oliver.behnke>
Component: AuthenticationAssignee: Galen Charlton <gmcharlt>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: 1joynelson, dpavlin, katrin.fischer, lucas, martin.renvoize, mtj, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17615
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04, 19.05.09
Attachments: Patch: proper NULL mapping
Patch: support password mapping with empty default
Bug 16719: Pass through undef rather than empty string in LDAP mapping
Bug 16719: (follow-up) Update check on password mapping
Bug 16719: Pass through undef rather than empty string in LDAP mapping
Bug 16719: (follow-up) Update check on password mapping
Bug 16719: (QA follow-up) Remove tabs

Description Oliver Behnke 2016-06-13 13:45:43 UTC
Created attachment 52316 [details] [review]
Patch: proper NULL mapping

LDAP attributes with empty values are currently mapped as empty strings, which is described as 'failsafe'. This defeats the existing database constraints (NULL and UNIQUE) without an apparent need. If some kind of empty default is required, it should be set in the mapping config itself, not the code.

Please note that this is not just a semantic issue since mapping empty LDAP values as an empty string does indeed cause a constraint violation (read: database error, hence our severity) when such an attribute bears a UNIQUE constraint. This is due to the fact that an empty string is an actual value validated by the constraint (NULL isn't).

I don't know the reasoning behind the empty-string mapping but should a NULL-mapping have been the original intention then please note that this behavior is database-dependent. The correct way to map NULL in a database-agnostic way is to use "undef" (see my patch).

Thanks
Comment 1 Oliver Behnke 2016-07-06 10:07:44 UTC
Created attachment 53124 [details] [review]
Patch: support password mapping with empty default

This patch fixes a related piece of code. The comment says "if password was mapped". That's not what the original code does as the condition was actually "if password was mapped and it's default is not empty".

This patch is required in the context of the other patch in this bug report which removes the broken failsafe (" " -> null/undef, true empty defaults).
Comment 2 Oliver Behnke 2016-11-11 14:49:21 UTC
Any update on this?
Comment 3 Mason James 2017-01-30 07:55:56 UTC
(In reply to Oliver Bock from comment #2)
> Any update on this?

hi Oliver, can you provide some testing steps for this patchset?
Comment 4 Oliver Behnke 2017-01-30 11:30:37 UTC
Not really, unfortunately. I'm not a Koha dev so I don't know anything about the unit test framework you use. Also, my patch was just meant as a proposal: it works for us but it should certainly be vetted (fixed/improved?) by the original author of that code section. My patches only reflect my understanding of code concerned which build while tracking down the issues we saw (this was a tricky one). They're based on my interpretation of the original author's intentions, the reasoning of which I just don't know. Thus my hope was that my patch starts a discussion.

Thanks,
Oliver
Comment 5 Oliver Behnke 2017-01-30 12:36:03 UTC
Steps to reproduce:

1) Map the attribute "cardnumber" for replication and update in your LDAP config
2) Create at least two users that don't yet have a value for that attribute in LDAP
3) Log in with both users one after the other
4) The second log in should result in a UNIQUE CONSTRAINT violation

While "cardnumber" is NULLable, inserting/updating two borrowers with an empty string for it result in a UNIQUE CONSTRAINT violation. This proves that a semantically correct action fails with the current implementation. This wouldn't happen if NULL would be used instead of an empty string.

HTH,
Oliver
Comment 6 Oliver Behnke 2017-07-04 08:41:51 UTC
Any update on this? Could you reproduce the issue?

Thanks
Comment 7 Martin Renvoize 2020-02-07 12:38:27 UTC
Created attachment 98560 [details] [review]
Bug 16719: Pass through undef rather than empty string in LDAP mapping

Nullable DB fields should be passed null in cases where ldap fields are
empty and not replaced with empty strings.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-02-07 12:38:30 UTC
Created attachment 98561 [details] [review]
Bug 16719: (follow-up) Update check on password mapping

Change to allow for unmapped default passwords

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-02-07 12:40:07 UTC
Code works as expected and seems to resolve the issue described. I could not spot any regressions and it appears a sane change to me.

Signing off
Comment 10 Oliver Behnke 2020-02-07 12:47:37 UTC
Seems like you accepted both of my patches. Nice! Thanks guys!
Comment 11 Nick Clemens 2020-02-07 16:56:58 UTC
Created attachment 98573 [details] [review]
Bug 16719: Pass through undef rather than empty string in LDAP mapping

Nullable DB fields should be passed null in cases where ldap fields are
empty and not replaced with empty strings.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2020-02-07 16:57:01 UTC
Created attachment 98574 [details] [review]
Bug 16719: (follow-up) Update check on password mapping

Change to allow for unmapped default passwords

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2020-02-07 16:57:03 UTC
Created attachment 98575 [details] [review]
Bug 16719: (QA follow-up) Remove tabs

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2020-02-07 17:00:20 UTC
https://wiki.koha-community.org/wiki/Ldap_testing
Comment 15 Katrin Fischer 2020-02-08 11:48:35 UTC
Awesome to see the old and bad moving - thx guys!
Comment 16 Martin Renvoize 2020-02-10 09:23:22 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 17 Joy Nelson 2020-03-05 00:38:23 UTC
Pushed to 19.11.x branch for 19.11.04
Comment 18 Lucas Gass 2020-03-09 18:07:15 UTC
backported to 19.05.x for 19.05.09