Similar to bug 14323 - when two users share the same value between their userid and cardnumber, i.e. user1->cardnumber == user2->userid API patron validation will return the same patron when given the userid/cardnumebr value and the password from either patron
Created attachment 164668 [details] [review] Bug 36575: Return correct patron when there is a shared userid / cardnumber This patch moves some patron fetching code in C4/Auth to use to patron returned from the validation methods and only try to fetch the patron (to check if locked, update attempts, etc) if we didn't authenticate To test: 1 - Set a user to have userid = BANANA password = Password1 2 - Set a user to have cardnumber = BANANA password = Password2 3 - Hit the patron authentication API: http://localhost:8080/api/v1/auth/password/validation with data: { "identifier": "BANANA", "password":"Password1" } and: { "identifier": "BANANA", "password":"Password2" } 4 - Note you receive the same response for both 5 - Apply patch, restart all 6 - Repeat the API and confirm you get the correct patron for the password submitted
We should go further likely, and prevent the users from signing in if there is confusion, however, fixing the return and using the correct patron is also a good idea :-) submitting this small change to prevent privacy breach
Created attachment 164670 [details] [review] Bug 36575: Return correct patron when there is a shared userid / cardnumber This patch moves some patron fetching code in C4/Auth to use to patron returned from the validation methods and only try to fetch the patron (to check if locked, update attempts, etc) if we didn't authenticate To test: 1 - Set a user to have userid = BANANA password = Password1 2 - Set a user to have cardnumber = BANANA password = Password2 3 - Hit the patron authentication API: http://localhost:8080/api/v1/auth/password/validation with data: { "identifier": "BANANA", "password":"Password1" } and: { "identifier": "BANANA", "password":"Password2" } 4 - Note you receive the same response for both 5 - Apply patch, restart all 6 - Repeat the API and confirm you get the correct patron for the password submitted Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 164671 [details] [review] Bug 36575: (QA follow-up) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All working as described and I'm unable to spot any regressions. Tests passing and new tests make sense. QA follow-up added to correct a duplicate 'my' and tidy appropriately.
(In reply to Nick Clemens from comment #2) > We should go further likely, and prevent the users from signing in if there > is confusion, however, fixing the return and using the correct patron is > also a good idea :-) submitting this small change to prevent privacy breach We should definitely go further at a systemic level, but I think this is certainly a positive move forward (even if the code is difficult to read). If the tests are passing, then it's probably good to go, but I don't feel confident marking it as QA without going through the changes to C4/Auth.pm more. Hopefully someone else can get to this before I have time... away a lot the next couple weeks...
if ($check_internal_as_fallback) { # INTERNAL AUTH @return = checkpw_internal( $userid, $password, $no_set_userenv ); $passwd_ok = 1 if $return[0] > 0; # 1 or 2 $patron = Koha::Patrons->find( { cardnumber => $return[1] } ) if $passwd_ok; push @return, $patron if $patron; } if ( defined $userid && !$patron ) { $patron = Koha::Patrons->find( { userid => $userid } ); $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron; push @return, $patron if $check_internal_as_fallback; } Why not add the patron in checkpw_internal now? That would be consistent with all other checkpw_ routines? Impact seems not that great: C4/Auth.pm: checkauth check_api_auth get_session check_cookie_auth checkpw checkpw_internal checkpw_hash C4/Auth.pm: @return = checkpw_internal( $userid, $password, $no_set_userenv ); C4/Auth.pm:sub checkpw_internal { C4/Auth_with_ldap.pm:use C4::Auth qw( checkpw_internal ); C4/Auth_with_ldap.pm: my ($ok, $cardnum) = checkpw_internal($userid, $password); Koha/REST/V1/Auth.pm:use C4::Auth qw( check_cookie_auth checkpw_internal get_session haspermission ); Koha/REST/V1/Auth.pm: unless ( checkpw_internal($user_id, $password ) ) { Koha/REST/V1/Patrons/Password.pm:use C4::Auth qw(checkpw_internal); Koha/REST/V1/Patrons/Password.pm: unless ( checkpw_internal($user->userid, $old_password ) ) {
The whole idea of guessing if it is a userid or a cardnumber etc is silly btw ;)
(In reply to Marcel de Rooy from comment #8) > The whole idea of guessing if it is a userid or a cardnumber etc is silly > btw ;) Agreed. It drives me a little crazy. Especially since both are nullable [U+1F643]. I wish sometimes that userid was a required field, and cardnumber was the optional one. (But then cardnumber is used throughout the codebase as if it were a userid...) 2 minutes until 5pm...
Note at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14323#c7 I have an idea about how we could fix the data integrity...
(In reply to Marcel de Rooy from comment #8) > The whole idea of guessing if it is a userid or a cardnumber etc is silly > btw ;) Couldn't agree more!
(In reply to Marcel de Rooy from comment #7) > > Why not add the patron in checkpw_internal now? That would be consistent > with all other checkpw_ routines? > I had planned to do some more work in this area, and do more cleanup there. trying to keep this one small and clean for backport - bug 36590
Created attachment 165133 [details] [review] Bug 36575: Return correct patron when there is a shared userid / cardnumber This patch moves some patron fetching code in C4/Auth to use to patron returned from the validation methods and only try to fetch the patron (to check if locked, update attempts, etc) if we didn't authenticate To test: 1 - Set a user to have userid = BANANA password = Password1 2 - Set a user to have cardnumber = BANANA password = Password2 3 - Hit the patron authentication API: http://localhost:8080/api/v1/auth/password/validation with data: { "identifier": "BANANA", "password":"Password1" } and: { "identifier": "BANANA", "password":"Password2" } 4 - Note you receive the same response for both 5 - Apply patch, restart all 6 - Repeat the API and confirm you get the correct patron for the password submitted Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 165134 [details] [review] Bug 36575: (QA follow-up) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Looking here
How wonderful to dive in Auth again ;) Hope you can follow my concerns below: [1] What about this case: From ldap: my ($borrowernumber,$cardnumber,$local_userid,$savedpw) = exists_local($userid); my $patron; if (( $borrowernumber and $config{update} ) or (!$borrowernumber and $config{replicate}) ) { %borrower = ldap_entry_2_hash($userldapentry,$cardnumber); #warn "checkpw_ldap received \%borrower w/ " . keys(%borrower), " keys: ", join(' ', keys %borrower), "\n"; } if ($borrowernumber) { if ($config{update}) { # A1, B1 my $c2 = &update_local($local_userid,$password,$borrowernumber,\%borrower) || ''; ($cardnumber eq $c2) or warn "update_local returned cardnumber '$c2' instead of '$cardnumber'"; } else { # C1, D1 # maybe update just the password? return(1, $cardnumber, $local_userid); => So borrowernumber was found using userid or cardnumber in exists_local, but the C1 D1 else branch does not return the PATRON ! [2] What about shibboleth here: return ( 1, $borrower->get_column('cardnumber'), $borrower->get_column('userid'), $borrower ); This is fine but what does autocreate return: return ( 1, $patron->cardnumber, $patron->userid ); => NO PATRON ! [3] And now we have this code after checking pw in ldap, cas and shibboleth: if ($check_internal_as_fallback) { # INTERNAL AUTH @return = checkpw_internal( $userid, $password, $no_set_userenv ); $passwd_ok = 1 if $return[0] > 0; # 1 or 2 $patron = Koha::Patrons->find( { cardnumber => $return[1] } ) if $passwd_ok; push @return, $patron if $patron; } => We could check if return[1] is defined here? And why not fallback here to userid too? Note also that the patron may not even have a cardnumber. if ( defined $userid && !$patron ) { $patron = Koha::Patrons->find( { userid => $userid } ); $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron; push @return, $patron if $check_internal_as_fallback; } => Here we come in all cases. And this gets tricky. Reading the code it seems to be meant for internal_fallback but it has side-effects. What could happen? ladp may set internal_fallback, but shibboleth doesnt. There may be no patron (see above). So patron could be set but it is not returned! Instead of the current acrobatics here in the general checkpw, I still recommend to fix checkpw_internal as well as the returns of the ldap and shibboleth case as described earlier. This will reduce code complexity.
Created attachment 165467 [details] [review] Bug 36575: Return correct patron when there is a shared userid / cardnumber This patch moves some patron fetching code in C4/Auth to use to patron returned from the validation methods and only try to fetch the patron (to check if locked, update attempts, etc) if we didn't authenticate To test: 1 - Set a user to have userid = BANANA password = Password1 2 - Set a user to have cardnumber = BANANA password = Password2 3 - Hit the patron authentication API: http://localhost:8080/api/v1/auth/password/validation with data: { "identifier": "BANANA", "password":"Password1" } and: { "identifier": "BANANA", "password":"Password2" } 4 - Note you receive the same response for both 5 - Apply patch, restart all 6 - Repeat the API and confirm you get the correct patron for the password submitted Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 165468 [details] [review] Bug 36575: (QA follow-up) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 165469 [details] [review] Bug 36575: (bug 34893 follow-up) Return patron from LDAP
Created attachment 165470 [details] [review] Bug 36575: (bug 34893 follow-up) Return patron when autocreating in Shibboleth
Created attachment 165471 [details] [review] Bug 36575: Adjust checkpw_internal to return patron This patch refactors checkpw_internal to remove the SQL code, use patron ojbects, and return the patron that correctly matches the userid/caerdnumber when auth is successful
Bug 36161 touches the same code ;). We should look at both in quick succession in my opinion.
Revisiting this one
The follow-ups are surely improvements! But Auth always gives headaches so still having a few remarks/questions ;) Added follow-up for [1] Fixed POD for _autocreate [2] Fill passwd_ok after calling checkpw_internal for success and failure. Still think that we should get rid of these lines now: if ( defined $userid && !$patron ) { $patron = Koha::Patrons->find( { userid => $userid } ); $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron; push @return, $patron if $check_internal_as_fallback; # We pass back the patron if authentication fails } * cas tries both variations in checkpw * shibboleth works differently (using a matchpoint) * ldap checks both in exists_local * the earlier internal fallback checked both as well But note the remark below about sco-main.pl Why should we return a patron if the password authentication failed ? And then the internal callers of checkpw still ignore the work we did and do it again. [ We could move that work to another report. ] sub checkauth { L1043 ( $return, $cardnumber, $retuserid ) = checkpw( $q_userid, undef, $query ); Ignoring patron. And a few lines further for cas (L1055). See also L1124. my $patron; ( $return, $cardnumber, $retuserid, $patron, $cas_ticket ) = checkpw( $userid, $password, $query, $type ); The local $patron is ignored too. Later on we are going to fetch the patron again with userid and cardnumber. Aaarg. sub check_api_auth { my $patron; ( $return, $cardnumber, $newuserid, $patron, $cas_ticket ) = checkpw( $userid, $password, $query ); Gets the patron from checkpw. And still ignores it. Trying to find the patron later on with again userid and cardnumber ;) Some external callers: C4/ILSDI/Services.pm: my ($status, $cardnumber, $userid, $patron) = C4::Auth::checkpw( $username, $password ); Looks good C4/SIP/ILS/Patron.pm: ($ret) = checkpw( $self->{userid}, $pwd, undef, undef, 1 ); # userid, query, type, no_set_userenv Looks good Koha/REST/V1/Auth/Password.pm: my ( $status, $THE_cardnumber, $THE_userid, $patron ) = C4::Auth::checkpw( $identifier, $password ); Looks good opac/sco/sco-main.pl: ( undef, $patronid ) = checkpw( $patronlogin, $patronpw ); => This call looks strange. This might be the call that expects patron on its wrong position though ! See above. Can we fix this still? opac/sco/sco-main.pl L116 $patron = Koha::Patrons->find( { cardnumber => $patronid } ); Awful reading that. Not for here.
Created attachment 165908 [details] [review] Bug 36575: (QA follow-up) Shibboleth POD and checkpw_internal call
(In reply to Marcel de Rooy from comment #24) > opac/sco/sco-main.pl: ( undef, $patronid ) = checkpw( $patronlogin, > $patronpw ); > => This call looks strange. This might be the call that expects patron on > its wrong position though ! See above. Can we fix this still? > > opac/sco/sco-main.pl L116 $patron = Koha::Patrons->find( { cardnumber => > $patronid } ); > Awful reading that. Not for here. Reading it again. patronid is actually cardnumber. So it should be fine? But it is strange to ignore the first return.
(In reply to Marcel de Rooy from comment #24) > The follow-ups are surely improvements! But Auth always gives headaches so > still having a few remarks/questions ;) > > Added follow-up for > [1] Fixed POD for _autocreate > [2] Fill passwd_ok after calling checkpw_internal for success and failure. > > > Still think that we should get rid of these lines now: > if ( defined $userid && !$patron ) { > $patron = Koha::Patrons->find( { userid => $userid } ); > $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless > $patron; > push @return, $patron if $check_internal_as_fallback; # We pass > back the patron if authentication fails > } > * cas tries both variations in checkpw > * shibboleth works differently (using a matchpoint) > * ldap checks both in exists_local > * the earlier internal fallback checked both as well > But note the remark below about sco-main.pl > Why should we return a patron if the password authentication failed ? I agree - this is something to address on a future bug. For now, we need to get the patron to check/increase login attempts. In this case, we still need to guess the patron as the authentication didn't work so didn't select one - bug 14323 removes the ability to log in when you have username/cardnumber match so will remedy the 'guessing' > > And then the internal callers of checkpw still ignore the work we did and do > it again. [ We could move that work to another report. ] Yes, please, that should be a new bug - the patron return was added by bug 34893 and these are follow-ups from that. Filed bug 36771 > opac/sco/sco-main.pl: ( undef, $patronid ) = checkpw( $patronlogin, > $patronpw ); > => This call looks strange. This might be the call that expects patron on > its wrong position though ! See above. Can we fix this still? > > opac/sco/sco-main.pl L116 $patron = Koha::Patrons->find( { cardnumber => > $patronid } ); > Awful reading that. Not for here. It's extra work, yes, but I would rather not touch more here - I think it makes sense on 36771 as well?
> It's extra work, yes, but I would rather not touch more here - I think it > makes sense on 36771 as well? I would agree here. This is an issue that needs an immediate fix whereas other improvements can come later. As such we need a minimum viable change here to fix the bug.
Coming here again
(In reply to Nick Clemens (kidclamp) from comment #27) > > Why should we return a patron if the password authentication failed ? > > I agree - this is something to address on a future bug. > For now, we need to get the patron to check/increase login attempts. > In this case, we still need to guess the patron as the authentication didn't > work so didn't select one - bug 14323 removes the ability to log in when you > have username/cardnumber match so will remedy the 'guessing' We need to fill patron to update login_attempts. But imo there is no need to return a patron here in the place of the expected cardnumber. Thats seems to be a bug supported by invalid unit tests. But you are right. Lets fix on follow-ups for now. So remove this line somewhere in the tree: push @return, $patron if $check_internal_as_fallback; # We pass back the patron if authentication fails This is just wrong.
Created attachment 166493 [details] [review] Bug 36575: Return correct patron when there is a shared userid / cardnumber This patch moves some patron fetching code in C4/Auth to use to patron returned from the validation methods and only try to fetch the patron (to check if locked, update attempts, etc) if we didn't authenticate To test: 1 - Set a user to have userid = BANANA password = Password1 2 - Set a user to have cardnumber = BANANA password = Password2 3 - Hit the patron authentication API: http://localhost:8080/api/v1/auth/password/validation with data: { "identifier": "BANANA", "password":"Password1" } and: { "identifier": "BANANA", "password":"Password2" } 4 - Note you receive the same response for both 5 - Apply patch, restart all 6 - Repeat the API and confirm you get the correct patron for the password submitted Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166494 [details] [review] Bug 36575: (QA follow-up) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166495 [details] [review] Bug 36575: (bug 34893 follow-up) Return patron from LDAP Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166496 [details] [review] Bug 36575: (bug 34893 follow-up) Return patron when autocreating in Shibboleth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166497 [details] [review] Bug 36575: Adjust checkpw_internal to return patron This patch refactors checkpw_internal to remove the SQL code, use patron ojbects, and return the patron that correctly matches the userid/caerdnumber when auth is successful Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166498 [details] [review] Bug 36575: (QA follow-up) Shibboleth POD and checkpw_internal call Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
22.05 needs the dependency Bug 30962
(In reply to wainuiwitikapark from comment #37) > 22.05 needs the dependency Bug 30962 David Cook says on 30962: Hmm... are you sure that you need this dependency? My suggestion for bug 36575 would be to just not backport the changes to t/db_dependent/api/v1/password_validation.t Nothing else in bug 36575 should touch any code here.
*** Bug 36928 has been marked as a duplicate of this bug. ***
Where are the tests?
(In reply to Tomás Cohen Arazi from comment #40) > Where are the tests? Tests are added in the first patch
(In reply to Nick Clemens (kidclamp) from comment #41) > (In reply to Tomás Cohen Arazi from comment #40) > > Where are the tests? > > Tests are added in the first patch Yeah, I saw. But my main concern was that the return value is changed for `checkpw_ldap` and `checkpw_shib` and it was not tested. I wrote them and am submitting.
Created attachment 167276 [details] [review] Bug 36575: Unit tests for checkpw_ldap Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 167277 [details] [review] Bug 36575: Unit tests for checkpw_shib Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 167278 [details] [review] Bug 36575: Return correct patron when there is a shared userid / cardnumber This patch moves some patron fetching code in C4/Auth to use to patron returned from the validation methods and only try to fetch the patron (to check if locked, update attempts, etc) if we didn't authenticate To test: 1 - Set a user to have userid = BANANA password = Password1 2 - Set a user to have cardnumber = BANANA password = Password2 3 - Hit the patron authentication API: http://localhost:8080/api/v1/auth/password/validation with data: { "identifier": "BANANA", "password":"Password1" } and: { "identifier": "BANANA", "password":"Password2" } 4 - Note you receive the same response for both 5 - Apply patch, restart all 6 - Repeat the API and confirm you get the correct patron for the password submitted Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 167279 [details] [review] Bug 36575: (QA follow-up) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 167280 [details] [review] Bug 36575: (bug 34893 follow-up) Return patron from LDAP Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 167281 [details] [review] Bug 36575: (bug 34893 follow-up) Return patron when autocreating in Shibboleth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 167282 [details] [review] Bug 36575: Adjust checkpw_internal to return patron This patch refactors checkpw_internal to remove the SQL code, use patron ojbects, and return the patron that correctly matches the userid/caerdnumber when auth is successful Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 167283 [details] [review] Bug 36575: (QA follow-up) Shibboleth POD and checkpw_internal call Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed for 24.11! Well done everyone, thank you!
No changes required to the manual.