Bug 16282 - Usernames should be case sensitive
Summary: Usernames should be case sensitive
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Chris Cormack
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-18 03:05 UTC by Chris Cormack
Modified: 2018-05-02 01:11 UTC (History)
7 users (show)

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


Attachments
Bug 16282 : Setting up the database for case sensitive usernames (2.86 KB, patch)
2016-04-19 21:09 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16282 : Make check_pw_internal case sensitve for user names (2.16 KB, patch)
2016-04-19 21:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16282 : Making GetMember be case sensitive for userid (4.02 KB, patch)
2016-04-19 22:26 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2016-04-18 03:05:57 UTC
Currently we don't care about case sensitivity for usernames, this is mostly ok, except when doing SSO ... with many systems who do care about case sensitivity, we can get username collisions.
Comment 1 Chris Cormack 2016-04-19 20:54:40 UTC
This only changes for the internal methods of Koha

So not Shibboleth, LDAP etc, they all function the same as they were.
Just anything that uses Koha's internal login method is affected.

For upgrades, behaviour will stay the same, the syspref will go in off. But for new installs they will have case sensitive usernames.
Comment 2 Chris Cormack 2016-04-19 21:09:42 UTC
Created attachment 50442 [details] [review]
Bug 16282 : Setting up the database for case sensitive usernames

Just adding a syspref

For upgrade, current behaviour stays the same
For new installs, usernames are now case sensitive
Comment 3 Chris Cormack 2016-04-19 21:10:17 UTC
Created attachment 50443 [details] [review]
Bug 16282 : Make check_pw_internal case sensitve for user names

To test:
1/ Make a user with a username like Fish
2/ Login to OPAC and staff side, test you can login with username fish, FISH, FIsh etc
3/ Apply patch, set UsernamesCaseSensitive to No
4/ Login again to OPAC and staff side, test you can still use fish, FISH
5/ Set syspref to Yes
6/ Try logging in with fish, note you cant
7/ Login with Fish successfully
Comment 4 Chris Cormack 2016-04-19 22:26:29 UTC
Created attachment 50447 [details] [review]
Bug 16282 : Making GetMember be case sensitive for userid

The main place this changes is SIP

GetMember with a username is only used in 3 places

C4/SIP/ILS/Patron.pm:    $kp = GetMember(cardnumber=>$patron_id) || GetMember(userid=>$patron_id);
misc/load_testing/benchmark_staff.pl:    unless defined C4::Members::GetMember(userid => $user);
tools/import_borrowers.pl:            $member = GetMember( 'userid' => $borrower{'userid'} );

To Test SIP

1/ Apply patches, set syspref to No (it should be No already but just in case)
2/ Restart the SIP server
3/ Login to SIP with a userid and password combo
4/ Notice no change, try with different case for userid
5/ Switch on the syspref
6/ Restart the SIP server
7/ Notice the userid/username case sensitive now

The benchmarking script appears faulty anyway, but if you can get it to run, it should
now care about case sensitive usernames if you have the syspref on.

To test borrower import
1/ Apply patch, check syspref is No
2/ Import borrowers with the userid being the matchpoint notice case doesn't matter
3/ Switch syspref to Yes
4/ Import borrowers again, now they will only match if the userid(username) matches exactly (Case sensitive)
Comment 5 Chris Cormack 2016-04-19 22:33:41 UTC
Just to note ILSDI AuthenticPatron, svc/authenticate will all respect this syspref also

I assume the REST api will too, but it's all woo woo magic so hard to actually tell how it is authenticating.
Comment 6 Nick Clemens 2016-04-21 00:52:12 UTC
I tried testing this patch, it works well for regular sign in to koha.

Import borrowers I couldn't test (but I think 15840 has an issue)

SIP - can this be tested using the sip_cli_emulator? If so I couldn't figure a method
Comment 7 Chris Cormack 2016-04-21 02:02:53 UTC
> SIP - can this be tested using the sip_cli_emulator? If so I couldn't figure
> a method

Perhaps, I tested this with an EzProxy set up that was connected to Koha via SIP. I'll see if I can figure out how to get the emulator to do it. I think it's just using cardnumber though. But probably can extend it
Comment 8 Mark Tompsett 2016-04-30 19:23:01 UTC
Comment on attachment 50443 [details] [review]
Bug 16282 : Make check_pw_internal case sensitve for user names

Review of attachment 50443 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +1793,5 @@
> +    my $sth;
> +    if ( C4::Context->preference('UsernamesCaseSensitive') ){
> +        # This query does a case sensitive query, but it first does a insensitive one so that the index can still be used
> +        # This will mean we don't get a nasty performance hit
> +        $sth = $dbh->prepare("SELECT * FROM (SELECT password,cardnumber,borrowernumber,userid,firstname,surname,borrowers.branchcode,branches.branchname,flags FROM borrowers JOIN branches ON borrowers.branchcode=branches.branchcode WHERE userid= ?) AS firstpass WHERE BINARY userid = ?");

WHERE BINARY? This is a MySQL-ism, is it not?
Comment 9 Owen Leonard 2017-01-30 17:45:01 UTC
(In reply to M. Tompsett from comment #8)

> WHERE BINARY? This is a MySQL-ism, is it not?

Is it? Anyone able to point to documentation?
Comment 10 Mark Tompsett 2017-01-31 06:13:44 UTC
(In reply to Owen Leonard from comment #9)
> (In reply to M. Tompsett from comment #8)
> 
> > WHERE BINARY? This is a MySQL-ism, is it not?
> 
> Is it? Anyone able to point to documentation?

https://dev.mysql.com/doc/refman/5.7/en/cast-functions.html
I can't find such a casting name for postgres  sqlite or other SQLs. Though, I could be wrong. Which is why I was asking for confirmation.
Comment 11 Marc Véron 2017-04-12 14:28:51 UTC
(In reply to M. Tompsett from comment #10)
> (In reply to Owen Leonard from comment #9)
> > (In reply to M. Tompsett from comment #8)
> > 
> > > WHERE BINARY? This is a MySQL-ism, is it not?
> > 
> > Is it? Anyone able to point to documentation?
> 
> https://dev.mysql.com/doc/refman/5.7/en/cast-functions.html
> I can't find such a casting name for postgres  sqlite or other SQLs. Though,
> I could be wrong. Which is why I was asking for confirmation.

WHERE BINARY does not exist e.g. for Oracle:
http://stackoverflow.com/questions/14819724/where-binary-in-oracle
Comment 12 Owen Leonard 2017-04-12 14:50:30 UTC
> WHERE BINARY does not exist e.g. for Oracle:
> http://stackoverflow.com/questions/14819724/where-binary-in-oracle

So this should be Failed QA? Please correct the status if not.
Comment 13 Jonathan Druart 2017-04-12 14:53:10 UTC
Note that I am going to remove GetMember on bug 17829.