Bug 14625 - LDAP: mapped ExtendedPatronAttributes cause error when updated on authentication
Summary: LDAP: mapped ExtendedPatronAttributes cause error when updated on authentication
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P3 major (vote)
Assignee: Oliver Bock
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18587
  Show dependency treegraph
 
Reported: 2015-07-31 09:03 UTC by Oliver Bock
Modified: 2017-07-04 15:01 UTC (History)
9 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:


Attachments
Proposed patch (1.45 KB, patch)
2015-11-30 13:00 UTC, Oliver Bock
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14625 - LDAP: skip extended patron attributes in 'borrowers' attribute update (1.51 KB, patch)
2017-05-12 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14625 - LDAP: skip extended patron attributes in 'borrowers' attribute update (1.64 KB, patch)
2017-05-18 18:16 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 Oliver Bock 2015-07-31 09:03:27 UTC
When extended patron attributes are mapped for replication and update via LDAP the attributes (here: "LOCATION") are properly replicated now (as of bug #8919) but on update the following error gets thrown:

> opac-user.pl: DBD::mysql::st execute failed: Unknown column 'LOCATION' in 'field list' at /usr/share/koha/lib/C4/Auth_with_ldap.pm line 354, <DATA> line 558.

The reason is that update_local() in Auth_with_ldap.pm doesn't care about those extended attributes at all. It's harmless because checkpw_ldap() seems to cover the update, not only the initial replication. But it's still confusing since a given admin is fooled by a false alarm.

Thanks.
Comment 1 Oliver Bock 2015-11-30 10:09:18 UTC
Thinking about this: this is NOT a harmless error since the whole update fails in this case.
Comment 2 Oliver Bock 2015-11-30 13:00:19 UTC
Created attachment 45222 [details] [review]
Proposed patch

Attached a patch.
Comment 3 Oliver Bock 2015-11-30 13:04:30 UTC
Raising severity to "normal" (for us it's actually "major") because of the above reason.
Comment 4 Mason James 2017-01-30 07:32:11 UTC
hmm, setting status to 'needs signoff'
Comment 5 Mason James 2017-01-30 07:57:28 UTC
(In reply to Mason James from comment #4)
> hmm, setting status to 'needs signoff'

hi Oliver, can you provide some testing steps for this patchset?
Comment 6 Oliver Bock 2017-01-30 11:22:22 UTC
Not really, unfortunately. I'm not a Koha dev so I don't know anything about the unit test framework you use. Also, my patch was just meant as a proposal: it works for us but it should certainly be vetted (fixed/improved?) by the original author of that code section.

Thanks,
Oliver
Comment 7 Katrin Fischer 2017-01-30 11:37:31 UTC
Hi Oliver,
testing steps can also be a "step by step" on how to reproduce the error. 
1) go to the login page
2) enter...
... and so on. This makes it easier for people to understand the problem and test your code. Given the number of devs and Koha's age, we might not be able to find the original developer :)
Comment 8 Oliver Bock 2017-01-30 12:13:19 UTC
Sorry, I thought the description was clear enough. Ok, let's see:

1) Create an extended patron attribute (that exists in LDAP)
2) Map that attribute in your LDAP auth config
3) Log in (first replication, should give no error in OPAC log)
4) Log out
5) Update attribute in LDAP
6) Log in
7) Error (see first comment) should be visible in OPAC log
Comment 9 Katrin Fischer 2017-01-30 13:57:27 UTC
Thx!
Comment 10 Martin Renvoize 2017-05-12 07:22:37 UTC
Code looks sane and does the trick.. but we'll need an update to the unit tests to cover the change before it'll get past QA.

Oliver, are you happy to have a go at adding a test to Auth_with_ldap.t?
Comment 11 Martin Renvoize 2017-05-12 07:24:20 UTC
Created attachment 63410 [details] [review]
[SIGNED OFF] Bug 14625 - LDAP: skip extended patron attributes in 'borrowers' attribute update

* Any extended patron attributes will cause the update to fail as those attributes don't exist in the 'borrowers' table
* The update of the extended patron attributes is already dealt with in checkpw_ldap()
* Ergo: just skip those attributes here

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2017-05-12 07:32:02 UTC
I've signed off the proposed patch, but marked the overall bug as failed qa untill we have test coverage.
Comment 13 Martin Renvoize 2017-05-12 07:59:40 UTC
On second thoughts, as update_local isn't currently tested at all I think perhaps it widens the scope a bit too much to expect you to write the tests.

I'll open another bug to write the tests for the whole of update_local and hopefully I'll get around to writing them this century!
Comment 14 Oliver Bock 2017-05-12 09:34:21 UTC
(In reply to Martin Renvoize from comment #13)
> On second thoughts, as update_local isn't currently tested at all I think
> perhaps it widens the scope a bit too much to expect you to write the tests.

Yep, I agree :-) Glad to see progress!

Thanks
Comment 15 Jonathan Druart 2017-05-18 18:16:47 UTC
Created attachment 63537 [details] [review]
Bug 14625 - LDAP: skip extended patron attributes in 'borrowers' attribute update

* Any extended patron attributes will cause the update to fail as those attributes don't exist in the 'borrowers' table
* The update of the extended patron attributes is already dealt with in checkpw_ldap()
* Ergo: just skip those attributes here

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I did not test this patch but code looks good
Comment 16 Kyle M Hall 2017-05-19 13:52:40 UTC
Pushed to master for 17.05, thanks Oliver!
Comment 17 Oliver Bock 2017-05-19 14:00:49 UTC
Great news! Hope this gets backported to 16.x as well.

Thanks guys!
Comment 18 Katrin Fischer 2017-05-21 20:56:15 UTC
This patch has been pushed to 16.11.x and will be in 16.11.08.
Comment 19 Julian Maurice 2017-05-22 12:42:44 UTC
Pushed to 3.22.x for 3.22.21
Comment 20 Mason James 2017-05-24 03:10:22 UTC
Pushed to 16.05.x, for 16.05.13 release
Comment 21 Oliver Bock 2017-07-04 08:50:59 UTC
IMHO, this can be closed.

Thanks again!