[Bug 22461] New: Regression in #20287: LDAP user replication broken with mapped extended patron attributes
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Bug ID: 22461 Summary: Regression in #20287: LDAP user replication broken with mapped extended patron attributes Change sponsored?: --- Product: Koha Version: 18.11 Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: oliver.behnke@aei.mpg.de QA Contact: testopia@bugs.koha-community.org Target Milestone: --- Reproduce: 1) Use LDAP authentication with user replication 2) Add a field mapping for an extended patron attribute (e.g. MYATTRIB) 3) Try to log in for the first time (new Koha user) 4) Error: "opac-user.pl: DBIx::Class::Row::store_column(): No such column 'MYATTRIB' on Koha::Schema::Result::Borrower at /usr/share/koha/lib/Koha/Object.pm line 75" Presumed cause: Commit [1] changed the way new patrons are created during LDAP replication (first login). The previous implementation (C4::Members::AddMember) took into account extended patron attributes by filtering them out before instantiating the new patron DB object [2]. The current implementation lacks this vital step. I chose a severity of critical because after upgrading from 18.05 to 18.11 no new users are able to log in anymore. Cheers [1] https://github.com/Koha-Community/Koha/commit/a6059c4d2d43e43aa021069ce97d64... [2] https://github.com/Koha-Community/Koha/blob/1b13c453e20e47c5e25bd946b50dd383... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Oliver Behnke <oliver.behnke@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |20287 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20287 [Bug 20287] Move AddMember and ModMember to Koha::Patron -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #1 from Oliver Behnke <oliver.behnke@aei.mpg.de> --- Update: while adding a filter for the extended attributes at [1] ensures account replication, the newly created account will lack the mapped extended attribute content until a subsequent update (e.g. on the second login). So that's another aspect that needs to be fixed. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #2 from Oliver Behnke <oliver.behnke@aei.mpg.de> --- Regarding the missing extended attribute replication: I think the reason is that the original code [1] set $borrowernumber to the newly created value, and the current version's Patron:new()->store() doesn't do that, so $borrowernumber is always 0. This would then cause [3] to never execute, hence the missing update of the mapped extended attributes. FYI, this also causes the setting of the messaging preferences to fail with a foreign key constraint violation at [4]. [3] https://github.com/Koha-Community/Koha/blob/a6059c4d2d43e43aa021069ce97d6424... [4] https://github.com/Koha-Community/Koha/blob/a6059c4d2d43e43aa021069ce97d6424... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Oliver Behnke <oliver.behnke@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Architecture, internals, |Authentication |and plumbing | CC| |dpavlin@rot13.org -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461
--- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> ---
Created attachment 86696
-->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86696&action=edit
Bug 22461: Fix LDAP user replication
>From bug 20287:
- $borrowernumber = C4::Members::AddMember(%borrower) or die "AddMember
failed";
+ Koha::Patron->new( \%borrower )->store;
C4::Members::Messaging::SetMessagingPreferencesFromDefaults( {
borrowernumber => $borrowernumber, categorycode => $borrower{'categorycode'} }
);
$borrowernumber is not updated with the value of the newly created patron
This patch restores the previous behavior (as well as the die)
--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461
Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #86696|0 |1
is obsolete| |
--- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> ---
Created attachment 86697
-->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86697&action=edit
Bug 22461: Fix LDAP user replication
>From bug 20287:
- $borrowernumber = C4::Members::AddMember(%borrower) or die "AddMember
failed";
+ Koha::Patron->new( \%borrower )->store;
C4::Members::Messaging::SetMessagingPreferencesFromDefaults( {
borrowernumber => $borrowernumber, categorycode => $borrower{'categorycode'} }
);
$borrowernumber is not updated with the value of the newly created patron
This patch restores the previous behavior (as well as the die)
--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|18.11 |master Assignee|koha-bugs@lists.koha-commun |jonathan.druart@bugs.koha-c |ity.org |ommunity.org CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Sorry about that Oliver, it's a silly mistake. I did not test that patch but it seems that it will restore the behavior. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Oliver Behnke <oliver.behnke@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #6 from Oliver Behnke <oliver.behnke@aei.mpg.de> --- Hi Jonathan, thanks for the patch which should fix what I described in comments 1 and 2. What it doesn't resolve, though, is what's stated in the original bug description (see "presumed cause"), so the reported error persists. Oliver -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 86856 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86856&action=edit Bug 22461: Filter ldap mapping before inserting patron's info -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Oliver Behnke <oliver.behnke@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #8 from Oliver Behnke <oliver.behnke@aei.mpg.de> --- Almost: "Global symbol "%data" requires explicit package name (did you forget to declare "my %data"?) at /usr/share/koha/lib/C4/Auth_with_ldap.pm line 228" The original indeed declares that at [5] and adding that line right above "my @columns" does work for me. But I'm no perl expert, so I have to leave the fix to you. That aside, it seems the patch isn't yet complete as I get the following error when replicating a new user from LDAP: "{UNKNOWN}: DBIC result _type isn't of the _type Category at /usr/share/perl5/DBIx/Class/Storage/BlockRunner.pm line 130. at /usr/share/koha/lib/Koha/Patron.pm line 288" Cheers FWIW, I propose to remove line 449 at [6] (or replace it with a more suitable comment) as we now know that, yes, that check is indeed vital :-) [5] https://github.com/Koha-Community/Koha/blob/1b13c453e20e47c5e25bd946b50dd383... [6] https://github.com/Koha-Community/Koha/blob/1b13c453e20e47c5e25bd946b50dd383... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461
Martin Renvoize <martin.renvoize@ptfs-europe.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #86697|0 |1
is obsolete| |
--- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> ---
Created attachment 87721
-->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87721&action=edit
Bug 22461: Fix LDAP user replication
>From bug 20287:
- $borrowernumber = C4::Members::AddMember(%borrower) or die "AddMember
failed";
+ Koha::Patron->new( \%borrower )->store;
C4::Members::Messaging::SetMessagingPreferencesFromDefaults( {
borrowernumber => $borrowernumber, categorycode => $borrower{'categorycode'} }
);
$borrowernumber is not updated with the value of the newly created patron
This patch restores the previous behavior (as well as the die)
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=22461 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86856|0 |1 is obsolete| | --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 87722 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87722&action=edit Bug 22461: Filter ldap mapping before inserting patron's info 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=22461 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 87723 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87723&action=edit Bug 22461: Clarify and correct the hash reduction There were a couple of bugs in the previous patch which meant it wasn't working as intended. This patch corrects those bugs and simplifies the code a little along the way. 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=22461 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|In Discussion |Needs Signoff --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Hi Oliver, Jonathan isn't around much at the moment so Katrin asked me to check in here. I believe Jonathans last followup was somewhat flawed (looks like a copy paste issue). I've added a followup on top which should clarify the intention of his code and fixes the two bugs I found in it. Could you test the full set for me and report back/signoff if they're working for you? Many thanks -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|jonathan.druart@bugs.koha-c |martin.renvoize@ptfs-europe |ommunity.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P1 - high -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #13 from Oliver Behnke <oliver.behnke@aei.mpg.de> --- Thanks Martin, I'll try and test your patches ASAP. Might take until next week though. Just to sure, the order in which to apply your three patches is 87721, 87722, 87723? Thanks -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Oliver Behnke <oliver.behnke@aei.mpg.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #14 from Oliver Behnke <oliver.behnke@aei.mpg.de> --- Great, I tested the three latest patches and confirm the fix! I tried the new sandbox service (http://sandboxes.ptfs-europe.co.uk) to have them signed off formally, so we should be good to go ahead. Thanks everyone! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461
Alex Arnaud <alex.arnaud@biblibre.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #87721|0 |1
is obsolete| |
--- Comment #15 from Alex Arnaud <alex.arnaud@biblibre.com> ---
Created attachment 88192
-->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88192&action=edit
Bug 22461: Fix LDAP user replication
>From bug 20287:
- $borrowernumber = C4::Members::AddMember(%borrower) or die "AddMember
failed";
+ Koha::Patron->new( \%borrower )->store;
C4::Members::Messaging::SetMessagingPreferencesFromDefaults( {
borrowernumber => $borrowernumber, categorycode => $borrower{'categorycode'} }
);
$borrowernumber is not updated with the value of the newly created patron
This patch restores the previous behavior (as well as the die)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
--
You are receiving this mail because:
You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Alex Arnaud <alex.arnaud@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #87722|0 |1 is obsolete| | --- Comment #16 from Alex Arnaud <alex.arnaud@biblibre.com> --- Created attachment 88193 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88193&action=edit Bug 22461: Filter ldap mapping before inserting patron's info Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Alex Arnaud <alex.arnaud@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #87723|0 |1 is obsolete| | --- Comment #17 from Alex Arnaud <alex.arnaud@biblibre.com> --- Created attachment 88194 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88194&action=edit Bug 22461: Clarify and correct the hash reduction There were a couple of bugs in the previous patch which meant it wasn't working as intended. This patch corrects those bugs and simplifies the code a little along the way. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Alex Arnaud <alex.arnaud@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |alex.arnaud@biblibre.com Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Passed QA |Pushed to Master --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 19.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #19 from Oliver Behnke <oliver.behnke@aei.mpg.de> --- Agreed! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Martin Renvoize from comment #12)
Jonathan isn't around much at the moment so Katrin asked me to check in here. I believe Jonathans last followup was somewhat flawed (looks like a copy paste issue).
Martin, I tried to reuse the previous code to prevent other regressions. But it seems I left a 'grep'. The code was: - my $new_member = { map { join(' ',@columns) =~ /$_/ ? ( $_ => $data{$_} ) : () } keys(%data) } ; So, I am now wondering: should not you test 'exists' instead of 'defined' in your code? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Try that: use Koha::Patrons; my @columns = Koha::Patrons->columns; my $xxx = { cardnumber => 1, foo => 2, borrowernumber => 3, bar => 4, opacnote => undef }; my $h1 = { map { join(' ',@columns) =~ /$_/ ? ( $_ => $xxx->{$_} ) : () } keys(%$xxx) } ; # original version my $h2 = { map { defined( $xxx->{$_} ) ? ( $_ => $xxx->{$_} ) : () } @columns }; # new version use Data::Printer colored => 1; warn p $h1; use Data::Printer colored => 1; warn p $h2; Note sure if it is relevant however. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Good catch, I think you're right, and exists would be more appropriate. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 88921 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88921&action=edit Bug 22461: (follow-up) Use `exists` not `defined` Prior to this patch there is a regression in the LDAP replication functionality such that clearing a field become impossible. This patch restores that functionality. 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=22461 --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Pushed to 18.11.x for 18.11.05 (including followup not yet in master) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |19.05.00, 18.11.05 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 --- Comment #25 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Martin Renvoize from comment #23)
Created attachment 88921 [details] [review] Bug 22461: (follow-up) Use `exists` not `defined`
Prior to this patch there is a regression in the LDAP replication functionality such that clearing a field become impossible. This patch restores that functionality.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to master for 19.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #26 from Lucas Gass <lucas@bywatersolutions.com> --- missing dependency not backporting to 18.05.x series -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22461 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org