| Summary: | ILSDI - AuthenticatePatron returns a wrong borrowernumber if cardnumber is empty | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Alex Arnaud <alex.arnaud> |
| Component: | Web services | Assignee: | Alex Arnaud <alex.arnaud> |
| Status: | CLOSED FIXED | QA Contact: | Katrin Fischer <katrin.fischer> |
| Severity: | critical | ||
| Priority: | P1 - high | CC: | fridolin.somers, jonathan.druart, kyle, martin.renvoize, nick |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
Bug 21046: Return the correct borrowernumber when there is empty cardnumber(s)
Bug 21046: Return the correct borrowernumber when there is empty cardnumber(s) Bug 21046: Return the correct borrowernumber when there is empty cardnumber(s) |
||
|
Description
Alex Arnaud
2018-07-06 13:30:59 UTC
Created attachment 76737 [details] [review] Bug 21046: Return the correct borrowernumber when there is empty cardnumber(s) Test plan: - Use a patron with an empty cardnumber to authenticate with ILSDI AuthenticatePatron (cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=userid&password=pass), - make sure you have other patron(s) with empty cardnumber and there borrowernumber is smaller than the one you authenticate with, - you should get a wrong borrowernumber, - apply this patch, - test again, you should get the right one I am not sure we should allow returning of a borrowernumber for an empty cardnumber. This could lead to all kinds of problems. I feel we should return an error for an empty cardnumber. What if 2 patrons pick the same password or are imported with the same default password? We assume cardnumbers need to be unique, NULL should not be treated as a valid value. (In reply to Katrin Fischer from comment #2) > I am not sure we should allow returning of a borrowernumber for an empty > cardnumber. This could lead to all kinds of problems. I feel we should > return an error for an empty cardnumber. It's quite common to have borrowers without cradnumber. Mostly when using third party import tools. So this leads to have dangerous behavior on portals that use Koha as back-end. Some patron get the loans, reserves and all information about an other one. > > What if 2 patrons pick the same password or are imported with the same > default password? Duplicate password doesn't matter here because we retrieve patrons with their userid (which is unique). Ah, I think I've misread the test plan then! userid makes sense and can't be empty. Sorry! Won't this be a breaking change for anyone currently using ILS-DI that doesn't have cardnumber and userid set to the same thing? Would it be better to try cardnumber and fall back to userid, and simply not allow either to be an empty field? That would make it behave like the rest of Koha's login functionality, while minimizing breakages. (In reply to Kyle M Hall from comment #6) > Won't this be a breaking change for anyone currently using ILS-DI that > doesn't have cardnumber and userid set to the same thing? Would it be better > to try cardnumber and fall back to userid, and simply not allow either to be > an empty field? That would make it behave like the rest of Koha's login > functionality, while minimizing breakages. Not sure to understand how it could be broken. For me this patch make sense even though i didn't find a bug. We pass a userid to the function, so we retrieve the patron with the userid. It is unique, and it has been authenticated by checkpw just before. (In reply to Alex Arnaud from comment #7) > (In reply to Kyle M Hall from comment #6) > > Won't this be a breaking change for anyone currently using ILS-DI that > > doesn't have cardnumber and userid set to the same thing? Would it be better > > to try cardnumber and fall back to userid, and simply not allow either to be > > an empty field? That would make it behave like the rest of Koha's login > > functionality, while minimizing breakages. > > Not sure to understand how it could be broken. For me this patch make sense > even though i didn't find a bug. We pass a userid to the function, so we > retrieve the patron with the userid. It is unique, and it has been > authenticated by checkpw just before. Never mind me, I am a dummy. I should have read the patch more closely. I blame mornings ;) Kyle, you or me for sign-off now? ;) Created attachment 76806 [details] [review] Bug 21046: Return the correct borrowernumber when there is empty cardnumber(s) (In reply to Katrin Fischer from comment #9) > Kyle, you or me for sign-off now? ;) You can qa ;) Created attachment 76854 [details] [review] Bug 21046: Return the correct borrowernumber when there is empty cardnumber(s) Test plan: - Use a patron with an empty cardnumber to authenticate with ILSDI AuthenticatePatron (cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=userid&password=pass), - make sure you have other patron(s) with empty cardnumber and there borrowernumber is smaller than the one you authenticate with, - you should get a wrong borrowernumber, - apply this patch, - test again, you should get the right one Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Awesome work all! Pushed to master for 18.11 Pushed to 18.05.x for 18.05.02 Pushed to 17.11.x for 17.11.08 Does not apply on 17.05.x (In reply to Fridolin SOMERS from comment #16) > Does not apply on 17.05.x Reading the code I'd say it's needed for 17.05.x as well. |