Bug 16719 - LDAP: Using empty strings as 'failsafe' attribute mapping defeats database constraints
Summary: LDAP: Using empty strings as 'failsafe' attribute mapping defeats database co...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-13 13:45 UTC by Oliver Behnke
Modified: 2020-03-09 18:07 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
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 (700 bytes, patch)
2016-06-13 13:45 UTC, Oliver Behnke
Details | Diff | Splinter Review
Patch: support password mapping with empty default (431 bytes, patch)
2016-07-06 10:07 UTC, Oliver Behnke
Details | Diff | Splinter Review
Bug 16719: Pass through undef rather than empty string in LDAP mapping (1.20 KB, patch)
2020-02-07 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 16719: (follow-up) Update check on password mapping (870 bytes, patch)
2020-02-07 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 16719: Pass through undef rather than empty string in LDAP mapping (1.26 KB, patch)
2020-02-07 16:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16719: (follow-up) Update check on password mapping (927 bytes, patch)
2020-02-07 16:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16719: (QA follow-up) Remove tabs (1.02 KB, patch)
2020-02-07 16:57 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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