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
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).
Any update on this?
(In reply to Oliver Bock from comment #2) > Any update on this? hi Oliver, can you provide some testing steps for this patchset?
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
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
Any update on this? Could you reproduce the issue? Thanks
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>
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>
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
Seems like you accepted both of my patches. Nice! Thanks guys!
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>
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>
Created attachment 98575 [details] [review] Bug 16719: (QA follow-up) Remove tabs Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
https://wiki.koha-community.org/wiki/Ldap_testing
Awesome to see the old and bad moving - thx guys!
Nice work everyone! Pushed to master for 20.05
Pushed to 19.11.x branch for 19.11.04
backported to 19.05.x for 19.05.09