Created attachment 3508 [details] [review]
The patch says "Users could log in intranet using their cardnumber, with superlibrarian rights."
I don't find this to be true for just any patron. Under what circumstances is this the case?
Owen: this is the case only if the library has a CAS server for authentication. ( The patch should have specified it )
also updating criticity, as it's really security issue !
Paul do you have a cas server set up we can use to test this against?
sorry but no :(
(we used to have one locally, we have a private acces to a customer one, but nothing I can give you :(((( )
I have test this locally, and it doesn't introduce any side effects. I will assume it fixes the problem with CAS and sign off, so it escalates up for QA
Created attachment 4091 [details] [review]
Signed off patch
To confirm, the problem is that ANY user could log in to a CAS server with their cardnumber and get superlibrarian privileges? I need to know exactly what I'm testing against in order to determine if it works or not.
Also, any instructions on how to set up a local CAS server would be welcome as well. (I'm not trying to gum up the works on getting this into Koha, I just want to do the full diligence on a blocker-level bug)
I also confirm the bug and the fix.
Unless cardnumber = userid in your system, completely unprivileged patrons can log into the staff client (with superlibrarian privileges) with their cardnumber and password. This DOES NOT depend on CAS or LDAP.
After applying the patch:
1. unprivileged users can no longer log into the staff client with cardnumber/password (problem resolved)
2. privileged users can still log into the staff client, either with username or cardnumber, and their correct privileges are retained
3. both privileged and unprivileged users can log into the OPAC as normal
Testing regimen does not include LDAP or CAS testing, since that level of authentication is done before falling back to cardnumber authentication.
Marking this patch as Passed QA.
Pushed to master.
I think this should be cherry picked to 3.2.x as well as 3.4.x and people encouraged strongly to upgrade their versions to 3.2.8 and 3.4.1 .
3.2.8 is due out in a couple of days, perhaps we should do a security release for 3.4.1 too?
This fix has been committed to master, 3.4.x, and 3.2.x. Please test and close.
There is a problem with checkpw's return value being misused since this patch was put in place. Cardnumber is returned, but treated as userid, particulary in the case of LDAP.
Created attachment 5895 [details] [review]
Propose Followup Patch
Makes checkpw return cardnumber AND userid, in all cases, so the code can use whichever is appropriate in the context.
Follow up pushed, nasty bug, thanks for the catch Ian
Created attachment 5953 [details] [review]
Another proposed followup patch
Fixes variable scoping issue on koha/sco/sco-main.pl that was revealed with the updates to checkpw's return values.
Created attachment 6022 [details] [review]
[SIGN-OFF]Bug 5995 Follow up: variable scope in koha/sco/sco-main.pl
The patronid value (cardnumber) set by checkpw in the case of SelfCheckoutByLogin
was improperly scoped with 'my' inside a conditional. The changes followup to 5995
made this more apparent, causing logins to fail.
Also added "parts copyright" statement to the script, since ByWater Solutions did make some
significant contributions to the operations of the page
Signed-off-by: Paul Poulain <firstname.lastname@example.org>
QA comment :
small patch, nothing to say from QA point of view.
I've checked everything works after the patch as before.
Patch submitted by ian, signed-off by me, marking "passed QA" otherwise we will be stuck ;-)
Chris, please check carefully