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/a6059c4d2d43e43aa021069ce97d6424a76c4f41#diff-56b596ea4e46031303c060d360a16eddL226 [2] https://github.com/Koha-Community/Koha/blob/1b13c453e20e47c5e25bd946b50dd3838e29c3ce/C4/Members.pm#L448
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.
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/a6059c4d2d43e43aa021069ce97d6424a76c4f41/C4/Auth_with_ldap.pm#L231 [4] https://github.com/Koha-Community/Koha/blob/a6059c4d2d43e43aa021069ce97d6424a76c4f41/C4/Auth_with_ldap.pm#L227
Created attachment 86696 [details] [review] 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)
Created attachment 86697 [details] [review] 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)
Sorry about that Oliver, it's a silly mistake. I did not test that patch but it seems that it will restore the behavior.
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
Created attachment 86856 [details] [review] Bug 22461: Filter ldap mapping before inserting patron's info
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/1b13c453e20e47c5e25bd946b50dd3838e29c3ce/C4/Members.pm#L396 [6] https://github.com/Koha-Community/Koha/blob/1b13c453e20e47c5e25bd946b50dd3838e29c3ce/C4/Members.pm#L449
Created attachment 87721 [details] [review] 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>
Created attachment 87722 [details] [review] Bug 22461: Filter ldap mapping before inserting patron's info Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87723 [details] [review] 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>
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
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
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!
Created attachment 88192 [details] [review] 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>
Created attachment 88193 [details] [review] 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>
Created attachment 88194 [details] [review] 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>
Awesome work all! Pushed to master for 19.05
Agreed!
(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?
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.
Good catch, I think you're right, and exists would be more appropriate.
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 18.11.x for 18.11.05 (including followup not yet in master)
(In reply to Martin Renvoize from comment #23) > Created attachment 88921 [details] [review] [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
missing dependency not backporting to 18.05.x series