Bug 29930 - 'cardnumber' overwritten with userid when not mapped (LDAP auth)
Summary: 'cardnumber' overwritten with userid when not mapped (LDAP auth)
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal with 1 vote (vote)
Assignee: Janik
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 17215
  Show dependency treegraph
 
Reported: 2022-01-22 11:50 UTC by Janik
Modified: 2024-02-08 18:54 UTC (History)
10 users (show)

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


Attachments
Bug 29930: Auth with LDAP: Update tag leads to unwanted updates (1.38 KB, patch)
2022-02-20 17:15 UTC, Janik
Details | Diff | Splinter Review
Bug 29930: Auth with LDAP: Update tag leads to unwanted updates (1.44 KB, patch)
2023-08-10 15:24 UTC, Alexander Wagner
Details | Diff | Splinter Review
Bug 29930: Regression tests (3.63 KB, patch)
2024-01-16 23:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29930: Cardnumber incorrectly set to userid on LDAP auth if cardnumber not mapped (1.76 KB, patch)
2024-01-16 23:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29930: Cardnumber incorrectly set to userid on LDAP auth if cardnumber not mapped (1.81 KB, patch)
2024-01-17 10:04 UTC, Alexander Wagner
Details | Diff | Splinter Review
Bug 29930: Regression tests (3.69 KB, patch)
2024-01-17 10:44 UTC, Alexander Wagner
Details | Diff | Splinter Review
Bug 29930: Cardnumber incorrectly set to userid on LDAP auth if cardnumber not mapped (1.81 KB, patch)
2024-01-17 10:44 UTC, Alexander Wagner
Details | Diff | Splinter Review
Bug 29930: Regression tests (3.78 KB, patch)
2024-01-19 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29930: Cardnumber incorrectly set to userid on LDAP auth if cardnumber not mapped (1.91 KB, patch)
2024-01-19 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janik 2022-01-22 11:50:51 UTC
I have observed that as soon as the update tag is set to true in koha-config.xml, the cardnumber is also updated on every login, even if it is not in the mappings.

After a login the cardnumber is equal to the userId, but according to log entries it is empty in between. I could see this in the log entry that is generated in line 214 of the Auth_with_ldap.pm. Maybe line 262 of the same file has something to do with it?
Comment 1 Janik 2022-02-20 17:15:19 UTC
Created attachment 130926 [details] [review]
Bug 29930: Auth with LDAP: Update tag leads to unwanted updates

In the sub the userId was given, but the cardnumber was expected, therefore at every login the cardnumber changed to the userId.

To test:
1. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
2. Check the patron record's cardnumber.
3. The cardnumber should equal to the userId
4. Apply the patch.
5. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
6. Check the patron record's cardnumber.
7. The cardnumber should be the same as before
Comment 2 Katrin Fischer 2022-08-21 11:30:19 UTC
I am sorry, but it looks like this hasn't been signed off yet. Moving back into the SO queue.
Comment 3 Magnus Enger 2023-03-27 09:54:30 UTC
Is it possible to test this without a full LDAP setup?
Comment 4 Alexander Wagner 2023-07-20 14:00:02 UTC
As the suggested path is only one line I applied it to a current installation of Koha 23.05 by hand.

I can confirm that the patch does leave the card number alone, so the patch works as requested.

Tested using:

- Koha 23.05
- Ubuntu 22.04.2 LTS

against our production LDAP.
Comment 5 Alexander Wagner 2023-07-20 14:09:18 UTC
**Note** `card number` is a mandatory field. Setting

```xml

<replicate>1</replicate> 

```

will happily create a user but it will _not_ assign a card number (leave it blank) with this patch applied. I have no idea if this causes headache further down the road.
Comment 6 Alexander Wagner 2023-08-10 15:24:20 UTC
Created attachment 154347 [details] [review]
Bug 29930: Auth with LDAP: Update tag leads to unwanted updates

In the sub the userId was given, but the cardnumber was expected, therefore at every login the cardnumber changed to the userId.

To test:
1. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
2. Check the patron record's cardnumber.
3. The cardnumber should equal to the userId
4. Apply the patch.
5. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
6. Check the patron record's cardnumber.
7. The cardnumber should be the same as before

Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>
Comment 7 Alexander Wagner 2023-08-11 08:06:21 UTC
**Note** `card number` is a mandatory field. Setting

```xml

<replicate>1</replicate> 

```

will now happily create a user without assign a card number with this patch applied. It seems simple to change it to the old behaviour and assign the ldap-id in case a new user is created.
Comment 8 Marcel de Rooy 2023-08-25 09:40:41 UTC
Thanks for your patch. A module change requires a unit test. Please add.
Look at t/db_dependent/Auth_with_ldap.t
Comment 9 Katrin Fischer 2023-08-25 09:44:59 UTC
This patch has been provided more than a year ago from someone outside the project - maybe we'll need to lend a hand hear to make this happen.
Comment 10 Marcel de Rooy 2023-08-28 09:05:36 UTC
(In reply to Katrin Fischer from comment #9)
> This patch has been provided more than a year ago from someone outside the
> project - maybe we'll need to lend a hand hear to make this happen.

Perhaps via dev mailing list ?
Comment 11 Tomás Cohen Arazi 2024-01-16 15:00:58 UTC
Hi all, I'm not *that* familiar with the LDAP code, but I reverse engineered how it works (on its interaction with Net::LDAP) for writing those mocked tests, so I could help.

At first sight, it doesn't look like a harmless tiny change. That 15 year old line seems to deal with how the `cardnumber` is set. So tests cannot be skipped.

Looking at the code, what we have is:

1. LDAP login
2. Login ok => Locally query the patron on the DB. At this point, the user-entered identifier is compared to `borrowers.userid` and then `borrowers.cardnumber`. At this point we have the Koha-defined `cardnumber` (nullable) and the `userid` (nullable on the DB, but auto-generated if NULL, by Koha::Patron->store).
3. In either case, %borrower is populated with the mapped data from the LDAP response, and using the user-introduced identifier as cardnumber...
=> SEEMS incorrect

I agree at this point we actually know the local userid and cardnumber and the current code is naive in terms of how data is cared about.

My understanding is that the patch is correct, meaning that if there's a mapping for the cardnumber, the loop in ldap_entry_2_hash() would overwrite it anyway. So we are just avoiding the cardnumber being set incorrectly when it is not mapped.

Will try to provide a test for this.
Comment 12 Tomás Cohen Arazi 2024-01-16 19:58:39 UTC
Ok, I wrote some tests. Wrapping them up together later or early morning tomorrow.
Comment 13 Tomás Cohen Arazi 2024-01-16 23:26:04 UTC
Created attachment 161078 [details] [review]
Bug 29930: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2024-01-16 23:26:08 UTC
Created attachment 161079 [details] [review]
Bug 29930: Cardnumber incorrectly set to userid on LDAP auth if cardnumber not mapped

In the sub the userId was given, but the cardnumber was expected, therefore at every login the cardnumber changed to the userId.

To test:
1. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
2. Check the patron record's cardnumber.
3. The cardnumber should equal to the userId
4. Apply the patch.
5. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
6. Check the patron record's cardnumber.
7. The cardnumber should be the same as before

Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Extra test plan:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Auth_with_ldap.t
=> FAIL: Tests fail! 'cardnumber' incorrectly overwritten
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests now pass!
5. Sign off :-D
Comment 15 Alexander Wagner 2024-01-17 10:04:32 UTC
Created attachment 161085 [details] [review]
Bug 29930: Cardnumber incorrectly set to userid on LDAP auth if cardnumber not mapped

In the sub the userId was given, but the cardnumber was expected, therefore at every login the cardnumber changed to the userId.

To test:
1. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
2. Check the patron record's cardnumber.
3. The cardnumber should equal to the userId
4. Apply the patch.
5. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
6. Check the patron record's cardnumber.
7. The cardnumber should be the same as before

Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Extra test plan:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Auth_with_ldap.t
=> FAIL: Tests fail! 'cardnumber' incorrectly overwritten
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests now pass!
5. Sign off :-D

Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>
Comment 16 Alexander Wagner 2024-01-17 10:07:19 UTC
(In reply to Tomás Cohen Arazi from comment #14)

> => SUCCESS: Tests now pass!
> 5. Sign off :-D

That was fast! Many thanks. I'll now dive into your changes and try to understand what I should have done. :)
Comment 17 Alexander Wagner 2024-01-17 10:44:31 UTC
Created attachment 161089 [details] [review]
Bug 29930: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>
Comment 18 Alexander Wagner 2024-01-17 10:44:33 UTC
Created attachment 161090 [details] [review]
Bug 29930: Cardnumber incorrectly set to userid on LDAP auth if cardnumber not mapped

In the sub the userId was given, but the cardnumber was expected, therefore at every login the cardnumber changed to the userId.

To test:
1. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
2. Check the patron record's cardnumber.
3. The cardnumber should equal to the userId
4. Apply the patch.
5. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
6. Check the patron record's cardnumber.
7. The cardnumber should be the same as before

Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Extra test plan:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Auth_with_ldap.t
=> FAIL: Tests fail! 'cardnumber' incorrectly overwritten
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests now pass!
5. Sign off :-D

Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>
Comment 19 Marcel de Rooy 2024-01-19 10:33:45 UTC
Still a confusing comment:
# Pass LDAP entry object and local cardnumber (userid).
Comment 20 Marcel de Rooy 2024-01-19 10:35:44 UTC
Created attachment 161200 [details] [review]
Bug 29930: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2024-01-19 10:35:46 UTC
Created attachment 161201 [details] [review]
Bug 29930: Cardnumber incorrectly set to userid on LDAP auth if cardnumber not mapped

In the sub the userId was given, but the cardnumber was expected, therefore at every login the cardnumber changed to the userId.

To test:
1. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
2. Check the patron record's cardnumber.
3. The cardnumber should equal to the userId
4. Apply the patch.
5. Login to the OPAC with an LDAP account that has a cardnumber which is different to the userId
6. Check the patron record's cardnumber.
7. The cardnumber should be the same as before

Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Extra test plan:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Auth_with_ldap.t
=> FAIL: Tests fail! 'cardnumber' incorrectly overwritten
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests now pass!
5. Sign off :-D

Signed-off-by: Alexander Wagner <alexander.wagner@desy.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2024-01-19 10:37:36 UTC
QA Comment:
Thx for adding tests. Change looks logical code-wise. Cant test this with an actual LDAP environment. Assuming that Alexander actually did that with a real LDAP. If that is not the case, please provide feedback, Alexander.

Passing QA
Comment 23 Alexander Wagner 2024-01-19 10:46:57 UTC
Indeed, I tested the original behaviour (which, with the help of Katrin led me to this bug) as well as the patched behaviour against our DESY LDAP on my test box and indeed with the patch everything is fine and I did not see any side effects. (Cf. my comments https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29930#c4 and https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29930#c5)

Besides I also read the test-code quite extensively to learn more on how the mockup routines work here (this was new to me) and replayed everything according to the test plan in ktd.

Many thanks to all contributing here :)
Comment 24 Marcel de Rooy 2024-01-19 10:48:34 UTC
Thanks Alexander for very fast response and your work of course here.
Comment 25 Katrin Fischer 2024-01-26 14:17:13 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 26 Fridolin Somers 2024-01-31 13:04:01 UTC
We should fix this disturbing comment :

# Pass LDAP entry object and local cardnumber (userid).
...
sub ldap_entry_2_hash {
Comment 27 Fridolin Somers 2024-01-31 13:04:27 UTC
Pushed to 23.11.x for 23.11.03
Comment 28 Lucas Gass 2024-02-08 18:54:54 UTC
Backported to 23.05.x for upcoming 23.05.09