Description
Janik
2022-01-22 11:50:51 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 I am sorry, but it looks like this hasn't been signed off yet. Moving back into the SO queue. Is it possible to test this without a full LDAP setup? 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. **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. 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> **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. Thanks for your patch. A module change requires a unit test. Please add. Look at t/db_dependent/Auth_with_ldap.t 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. (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 ? 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. Ok, I wrote some tests. Wrapping them up together later or early morning tomorrow. Created attachment 161078 [details] [review] Bug 29930: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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 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> (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. :) 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> 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> Still a confusing comment: # Pass LDAP entry object and local cardnumber (userid). 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> 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> 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 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 :) Thanks Alexander for very fast response and your work of course here. Pushed for 24.05! Well done everyone, thank you! We should fix this disturbing comment : # Pass LDAP entry object and local cardnumber (userid). ... sub ldap_entry_2_hash { Pushed to 23.11.x for 23.11.03 Backported to 23.05.x for upcoming 23.05.09 |