Bug 27957 - Attributes removed in LDAP
Summary: Attributes removed in LDAP
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 20443
Blocks: 27956
  Show dependency treegraph
 
Reported: 2021-03-15 15:07 UTC by Tomás Cohen Arazi
Modified: 2023-07-05 07:58 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
:x (2.19 KB, patch)
2021-03-17 11:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27957: Fix LDAP and patron's attributes (2.19 KB, patch)
2021-03-17 11:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27957: Fix LDAP and patron's attributes (2.47 KB, patch)
2021-03-17 13:52 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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
Comment 9 Katrin Fischer 2023-07-04 20:43:49 UTC
Shoudl this have been marked as NSO?
Comment 10 Jonathan Druart 2023-07-05 07:58:21 UTC
(In reply to Katrin Fischer from comment #9)
> Shoudl this have been marked as NSO?

No, it's missing tests.