Bug 22461 - Regression in #20287: LDAP user replication broken with mapped extended patron attributes
Summary: Regression in #20287: LDAP user replication broken with mapped extended patro...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P1 - high critical (vote)
Target Milestone: ---
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 20287
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-06 11:09 UTC by Oliver Behnke
Modified: 2019-05-22 14:30 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05


Attachments
Bug 22461: Fix LDAP user replication (1.44 KB, patch)
2019-03-15 23:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22461: Fix LDAP user replication (1.44 KB, patch)
2019-03-15 23:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22461: Filter ldap mapping before inserting patron's info (1.21 KB, patch)
2019-03-21 17:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22461: Fix LDAP user replication (1.50 KB, patch)
2019-04-10 13:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22461: Filter ldap mapping before inserting patron's info (1.27 KB, patch)
2019-04-10 13:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22461: Clarify and correct the hash reduction (1.28 KB, patch)
2019-04-10 13:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22461: Fix LDAP user replication (1.55 KB, patch)
2019-04-17 13:10 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 22461: Filter ldap mapping before inserting patron's info (1.32 KB, patch)
2019-04-17 13:11 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 22461: Clarify and correct the hash reduction (1.32 KB, patch)
2019-04-17 13:11 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 22461: (follow-up) Use `exists` not `defined` (1.09 KB, patch)
2019-04-26 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Behnke 2019-03-06 11:09:09 UTC
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
Comment 1 Oliver Behnke 2019-03-06 13:04:22 UTC
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.
Comment 2 Oliver Behnke 2019-03-06 14:34:09 UTC
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
Comment 3 Jonathan Druart 2019-03-15 23:19:35 UTC
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)
Comment 4 Jonathan Druart 2019-03-15 23:20:26 UTC
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)
Comment 5 Jonathan Druart 2019-03-15 23:21:49 UTC
Sorry about that Oliver, it's a silly mistake.

I did not test that patch but it seems that it will restore the behavior.
Comment 6 Oliver Behnke 2019-03-19 11:02:34 UTC
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
Comment 7 Jonathan Druart 2019-03-21 17:22:18 UTC
Created attachment 86856 [details] [review]
Bug 22461: Filter ldap mapping before inserting patron's info
Comment 8 Oliver Behnke 2019-03-22 10:05:59 UTC
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
Comment 9 Martin Renvoize 2019-04-10 13:42:22 UTC
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>
Comment 10 Martin Renvoize 2019-04-10 13:42:25 UTC
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>
Comment 11 Martin Renvoize 2019-04-10 13:42:27 UTC
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>
Comment 12 Martin Renvoize 2019-04-10 13:44:08 UTC
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
Comment 13 Oliver Behnke 2019-04-12 08:26:40 UTC
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
Comment 14 Oliver Behnke 2019-04-15 10:03:30 UTC
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!
Comment 15 Alex Arnaud 2019-04-17 13:10:54 UTC
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>
Comment 16 Alex Arnaud 2019-04-17 13:11:05 UTC
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>
Comment 17 Alex Arnaud 2019-04-17 13:11:13 UTC
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>
Comment 18 Nick Clemens 2019-04-18 12:07:52 UTC
Awesome work all!

Pushed to master for 19.05
Comment 19 Oliver Behnke 2019-04-18 12:28:26 UTC
Agreed!
Comment 20 Jonathan Druart 2019-04-24 20:55:41 UTC
(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?
Comment 21 Jonathan Druart 2019-04-24 21:01:07 UTC
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.
Comment 22 Martin Renvoize 2019-04-26 13:33:48 UTC
Good catch, I think you're right, and exists would be more appropriate.
Comment 23 Martin Renvoize 2019-04-26 13:37:05 UTC
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>
Comment 24 Martin Renvoize 2019-04-26 13:41:10 UTC
Pushed to 18.11.x for 18.11.05 (including followup not yet in master)
Comment 25 Nick Clemens 2019-04-26 17:54:25 UTC
(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
Comment 26 Lucas Gass 2019-05-10 21:10:58 UTC
missing dependency not backporting to 18.05.x series