Bug 27957

Summary: Attributes removed in LDAP
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: AuthenticationAssignee: Bugs List <koha-bugs>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: dpavlin, jonathan.druart+koha
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 20443    
Bug Blocks: 27956    
Attachments: :x
Bug 27957: Fix LDAP and patron's attributes
Bug 27957: Fix LDAP and patron's attributes

Description Tomás Cohen Arazi 2021-03-15 15:07:53 UTC
Working on bug 27857 (actually, on the follow-up bug 27956), I found that the Koha::Patron->extended_attributes setter is used to write the new attributes. 
This happens inside a Koha::Patron::Attribute::Types loop which is used to determine 'valid' attributes (good).

The problem is 'extended_attributes' actually calls:

$self->extended_attributes->filter_by_branch_limitations->delete;

which deletes the existing attribute. So calling it for setting a single attribute makes it... delete all the attributes each time!

There's also some funky logic in checkpw_ldap. The $patron object seems to be retrieved in several places and even inside a loop.
Comment 1 Jonathan Druart 2021-03-17 11:11:07 UTC
Created attachment 118364 [details] [review]
:x
Comment 2 Jonathan Druart 2021-03-17 11:11:18 UTC
Something like that? What do we do if an exception is raised?
Comment 3 Jonathan Druart 2021-03-17 11:11:32 UTC
Created attachment 118365 [details] [review]
Bug 27957: Fix LDAP and patron's attributes
Comment 4 Tomás Cohen Arazi 2021-03-17 11:22:57 UTC
(In reply to Jonathan Druart from comment #2)
> Something like that? What do we do if an exception is raised?

I would rather loop through them using $patron->add_extended_attribute inside a try/catch block and warning as we already do.
Comment 5 Jonathan Druart 2021-03-17 11:36:09 UTC
Then we will keep adding them, and won't deal with the constraints.
Comment 6 Jonathan Druart 2021-03-17 11:37:27 UTC
Maybe I misunderstood you.

Don't you think the code is more readable as it?
Comment 7 Tomás Cohen Arazi 2021-03-17 12:20:30 UTC
(In reply to Jonathan Druart from comment #6)
> Maybe I misunderstood you.
> 
> Don't you think the code is more readable as it?

Not at all. But the old behavior seemed to be to just warn on failed attributes. Using a single call like that will prevent attributes to be set.
Comment 8 Jonathan Druart 2021-03-17 13:52:23 UTC
Created attachment 118384 [details] [review]
Bug 27957: Fix LDAP and patron's attributes