Bug 21046

Summary: ILSDI - AuthenticatePatron returns a wrong borrowernumber if cardnumber is empty
Product: Koha Reporter: Alex Arnaud <alex.arnaud>
Component: Web servicesAssignee: 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
Using ILS-DI AuthenticatePatron (cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=userid&password=pass) with a user having an empty cardnumber returns a wrong borrowernumber if there is at least one other empty cardnumber.

Put the severity to critical because this has very bad consequences on Koha and front applications.

Patch is coming.
Comment 1 Alex Arnaud 2018-07-06 13:41:31 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
Comment 2 Katrin Fischer 2018-07-08 14:08:08 UTC
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?
Comment 3 Katrin Fischer 2018-07-08 14:08:45 UTC
We assume cardnumbers need to be unique, NULL should not be treated as a valid value.
Comment 4 Alex Arnaud 2018-07-09 07:43:22 UTC
(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).
Comment 5 Katrin Fischer 2018-07-09 09:40:55 UTC
Ah, I think I've misread the test plan then! userid makes sense and can't be empty. Sorry!
Comment 6 Kyle M Hall (khall) 2018-07-09 11:05:37 UTC
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.
Comment 7 Alex Arnaud 2018-07-09 13:15:49 UTC
(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.
Comment 8 Kyle M Hall (khall) 2018-07-09 14:25:40 UTC
(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 ;)
Comment 9 Katrin Fischer 2018-07-09 20:01:54 UTC
Kyle, you or me for sign-off now? ;)
Comment 10 Kyle M Hall (khall) 2018-07-10 11:09:15 UTC
Created attachment 76806 [details] [review]
Bug 21046: Return the correct borrowernumber when there is  empty cardnumber(s)
Comment 11 Kyle M Hall (khall) 2018-07-10 11:10:17 UTC
(In reply to Katrin Fischer from comment #9)
> Kyle, you or me for sign-off now? ;)

You can qa ;)
Comment 12 Chris Cormack 2018-07-11 01:45:26 UTC
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>
Comment 13 Nick Clemens (kidclamp) 2018-07-13 11:26:58 UTC
Awesome work all!

Pushed to master for 18.11
Comment 14 Martin Renvoize (ashimema) 2018-07-15 18:39:07 UTC
Pushed to 18.05.x for 18.05.02
Comment 15 Fridolin Somers 2018-07-25 09:47:08 UTC
Pushed to 17.11.x for 17.11.08
Comment 16 Fridolin Somers 2018-09-24 13:30:02 UTC
Does not apply on 17.05.x
Comment 17 Jonathan Druart 2018-09-29 14:45:09 UTC
(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.