Bug 17615 - LDAP Auth: regression causes attribute updates to silently fail and corrupt existing data
Summary: LDAP Auth: regression causes attribute updates to silently fail and corrupt e...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Keywords:
Depends on: 15889
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-11 15:24 UTC by Oliver Behnke
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

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


Attachments
Bug 17615 - Fix updating borrower attributes in checkpw_ldap (3.05 KB, patch)
2016-11-16 10:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
sign-off patch (3.11 KB, patch)
2016-11-17 08:51 UTC, Oliver Behnke
Details | Diff | Splinter Review
Bug 17615 - Fix updating borrower attributes in checkpw_ldap (3.11 KB, patch)
2016-11-17 10:43 UTC, Oliver Behnke
Details | Diff | Splinter Review
[PASSED QA] Bug 17615 - Fix updating borrower attributes in checkpw_ldap (3.18 KB, patch)
2016-11-18 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17615 - Fix updating borrower attributes in checkpw_ldap (3.18 KB, patch)
2017-01-19 10:52 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 17615 - Fix updating borrower attributes in checkpw_ldap (3.56 KB, patch)
2017-01-19 14:48 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 17615 - Fix updating borrower attributes in checkpw_ldap (3.18 KB, patch)
2017-01-19 15:22 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 17615 - Fix unit tests (972 bytes, patch)
2017-01-19 15:22 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 17615 - Fix unit tests (1.02 KB, patch)
2017-01-19 15:26 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 Behnke 2016-11-11 15:24:24 UTC
There's a regression in c74678a: when calling C4::Members::Attributes::UpdateBorrowerAttribute() the compiled attribute hash uses the member name "value" instead of "attribute".

This causes silent data corruption as all updated borrower attributes are overwritten with NULL values.

Best,
Oliver
Comment 1 Jonathan Druart 2016-11-14 12:11:15 UTC
This one sounds very sensible, Alex please fix ASAP.
Comment 2 Jonathan Druart 2016-11-14 12:12:24 UTC
It affects master as well
Comment 3 Oliver Behnke 2016-11-14 12:36:46 UTC
Jonathan, as you just added #15889, please also take a look at #14625 and #16719 which have been sitting around for while now. There are a number of similar LDAP-Auth issues right now.

Thanks guys,
Oliver
Comment 4 Alex Arnaud 2016-11-16 10:38:23 UTC
Created attachment 57532 [details] [review]
Bug 17615 - Fix updating borrower attributes in checkpw_ldap
Comment 5 Alex Arnaud 2016-11-16 10:42:56 UTC
Test plan:

- Update your configuration file to use LDAP authentication and enable update
  (<update>1</update>) option,
- create a mapping on an existing extended attrbitutes in koha-conf file,
- login with a user who has not this extended attribute set. The attribute of related LDAP entry should be set.
- check that the user attribute has been created.
Comment 6 Oliver Behnke 2016-11-17 08:51:12 UTC
Created attachment 57587 [details] [review]
sign-off patch

Attaching the signed-off patch manually since "git bz" never worked for me (returns that I'm not logged in on "attach", despite "apply" works).

FYI, I verified the actual patch but not the unit test.

Cheers
Comment 7 Jonathan Druart 2016-11-17 09:09:30 UTC
(In reply to Oliver Bock from comment #6)
> Attaching the signed-off patch manually since "git bz" never worked for me
> (returns that I'm not logged in on "attach", despite "apply" works).

You must use the fishsoup branch (up-to-date) and correctly set your credential. See the wiki page for a step-by-step configuration.
https://wiki.koha-community.org/wiki/Git_bz_configuration
Comment 8 Oliver Behnke 2016-11-17 10:43:37 UTC
Created attachment 57593 [details] [review]
Bug 17615 - Fix updating borrower attributes in checkpw_ldap

Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de>
Comment 9 Oliver Behnke 2016-11-17 10:44:44 UTC
(In reply to Jonathan Druart from comment #7)
> You must use the fishsoup branch (up-to-date)

Ah, I missed that one a few months ago. Thanks!
Comment 10 Martin Renvoize 2016-11-18 08:53:41 UTC
Created attachment 57619 [details] [review]
[PASSED QA] Bug 17615 - Fix updating borrower attributes in checkpw_ldap

Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2016-11-18 08:54:15 UTC
Cheers for the quick fix, works as expected
Comment 12 Kyle M Hall 2016-11-18 13:42:39 UTC
Please rebase asap!
Comment 13 Oliver Behnke 2017-01-18 15:21:39 UTC
Hi guys, any update on this?
Comment 14 Katrin Fischer 2017-01-18 17:42:29 UTC
The patch currently doesn't apply, we are waiting for a rebase to the current code base.
Comment 15 Oliver Behnke 2017-01-19 08:17:11 UTC
That's what I meant by "this" :-)
Comment 16 Jonathan Druart 2017-01-19 08:29:38 UTC
(In reply to Oliver Bock from comment #15)
> That's what I meant by "this" :-)

This bug report is assignee to you, so we are waiting for you.
Comment 17 Oliver Behnke 2017-01-19 08:35:57 UTC
Sorry, but I'm not the patch author. The author should rebase his patch, right?

Cheers,
Oliver
Comment 18 Chris Cormack 2017-01-19 08:40:53 UTC
(In reply to Oliver Bock from comment #17)
> Sorry, but I'm not the patch author. The author should rebase his patch,
> right?
> 
> Cheers,
> Oliver

Yep thats right
Comment 19 Oliver Behnke 2017-01-19 08:43:54 UTC
Ok, so just to confirm: you're not waiting for me but for Martin. Should the bug thus be assigned to him?
Comment 20 Alex Arnaud 2017-01-19 10:52:22 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2017-01-19 10:57:18 UTC
Was it a complex rebase? Maybe we can keep the sign-offs on this one.
Comment 22 Oliver Behnke 2017-01-19 11:00:24 UTC
Yes, please. The actual change is a single word :-)
Comment 23 Jonathan Druart 2017-01-19 13:55:32 UTC
Alex, 1 test fail for me:

t/db_dependent/Auth_with_ldap.t .. 1/4 
        #   Failed test 'Mapped attribute is BAR'
        #   at t/db_dependent/Auth_with_ldap.t line 199.
        #          got: undef
        #     expected: 'BAR'
        # Looks like you failed 1 test of 9.

    #   Failed test 'auth_by_bind = 1 tests'
    #   at t/db_dependent/Auth_with_ldap.t line 243.
    # Looks like you failed 1 test of 4.

#   Failed test 'checkpw_ldap tests'
#   at t/db_dependent/Auth_with_ldap.t line 309.
# Looks like you failed 1 test of 4.

Does it pass for you?
Comment 24 Alex Arnaud 2017-01-19 14:24:49 UTC Comment hidden (obsolete)
Comment 25 Alex Arnaud 2017-01-19 14:30:28 UTC Comment hidden (obsolete)
Comment 26 Alex Arnaud 2017-01-19 14:48:17 UTC Comment hidden (obsolete)
Comment 27 Alex Arnaud 2017-01-19 15:22:42 UTC
Created attachment 59264 [details] [review]
Bug 17615 - Fix updating borrower attributes in checkpw_ldap

Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Alex Arnaud 2017-01-19 15:22:49 UTC
Created attachment 59265 [details] [review]
Bug 17615 - Fix unit tests
Comment 29 Jonathan Druart 2017-01-19 15:26:02 UTC
Created attachment 59266 [details] [review]
Bug 17615 - Fix unit tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2017-01-19 15:27:14 UTC
I confirm that the tests now pass.
If the patch was ok before the rebase, it must still be.
Passing QA, trusting author and previous signoffs
Comment 31 Kyle M Hall 2017-01-20 13:37:38 UTC
Pushed to master for 17.05, thanks Alex!
Comment 32 Katrin Fischer 2017-01-22 19:33:03 UTC
This patch has been pushed to 16.11.x and will be in 16.11.02.
Comment 33 Katrin Fischer 2017-01-22 19:33:29 UTC
I couldn't test this, trusting the tests, please don't disappoint!
Comment 34 Mason James 2017-01-27 03:16:13 UTC
Pushed to 16.05.x, for 16.05.08 release
Comment 35 Oliver Behnke 2017-07-04 08:51:17 UTC
IMHO, this can be closed.

Thanks again!
Comment 36 Katrin Fischer 2017-07-04 14:56:05 UTC
After releases we move older bugs from "RESOLVED FIXED" to "CLOSED FIXED" in a big bulk edit, but if you tested, it's ok to just do it.