[Bug 16719] New: LDAP: Using empty strings as 'failsafe' attribute mapping defeats database constraints
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Bug ID: 16719 Summary: LDAP: Using empty strings as 'failsafe' attribute mapping defeats database constraints Change sponsored?: --- Product: Koha Version: 3.22 Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Authentication Assignee: gmcharlt@gmail.com Reporter: oliver.bock@aei.mpg.de QA Contact: testopia@bugs.koha-community.org CC: dpavlin@rot13.org Created attachment 52316 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52316&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #1 from Oliver Bock <oliver.bock@aei.mpg.de> --- Created attachment 53124 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53124&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #2 from Oliver Bock <oliver.bock@aei.mpg.de> --- Any update on this? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=17615 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com Status|NEW |In Discussion --- Comment #3 from Mason James <mtj@kohaaloha.com> --- (In reply to Oliver Bock from comment #2)
Any update on this?
hi Oliver, can you provide some testing steps for this patchset? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #4 from Oliver Bock <oliver.bock@aei.mpg.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #5 from Oliver Bock <oliver.bock@aei.mpg.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #6 from Oliver Bock <oliver.bock@aei.mpg.de> --- Any update on this? Could you reproduce the issue? Thanks -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 98560 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98560&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 98561 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98561&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Attachment #52316|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53124|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off --- Comment #9 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|3.22 |master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #10 from Oliver Behnke <oliver.behnke@aei.mpg.de> --- Seems like you accepted both of my patches. Nice! Thanks guys! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #98560|0 |1 is obsolete| | Attachment #98561|0 |1 is obsolete| | --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 98573 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98573&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #12 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 98574 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98574&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 98575 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98575&action=edit Bug 16719: (QA follow-up) Remove tabs Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- https://wiki.koha-community.org/wiki/Ldap_testing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #15 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Awesome to see the old and bad moving - thx guys! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com Status|Pushed to master |Pushed to stable Version(s)|20.05.00 |20.05.00, 19.11.04 released in| | --- Comment #17 from Joy Nelson <joy@bywatersolutions.com> --- Pushed to 19.11.x branch for 19.11.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16719 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|20.05.00, 19.11.04 |20.05.00, 19.11.04, released in| |19.05.09 Status|Pushed to stable |Pushed to oldstable CC| |lucas@bywatersolutions.com --- Comment #18 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 19.05.x for 19.05.09 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org