Bug 36575 - Wrong patron can be returned for API validation route
Summary: Wrong patron can be returned for API validation route
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords:
: 36928 (view as bug list)
Depends on: 34893 30962
Blocks: 36161 36590 36771
  Show dependency treegraph
 
Reported: 2024-04-11 09:38 UTC by Nick Clemens (kidclamp)
Modified: 2024-06-20 16:11 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.01,23.11.06,23.05.12,22.11.18,22.05.22


Attachments
Bug 36575: Return correct patron when there is a shared userid / cardnumber (9.83 KB, patch)
2024-04-11 09:44 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36575: Return correct patron when there is a shared userid / cardnumber (9.89 KB, patch)
2024-04-11 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36575: (QA follow-up) (7.45 KB, patch)
2024-04-11 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36575: Return correct patron when there is a shared userid / cardnumber (9.94 KB, patch)
2024-04-18 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36575: (QA follow-up) (7.50 KB, patch)
2024-04-18 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36575: Return correct patron when there is a shared userid / cardnumber (9.94 KB, patch)
2024-04-24 15:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36575: (QA follow-up) (7.50 KB, patch)
2024-04-24 15:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36575: (bug 34893 follow-up) Return patron from LDAP (1.49 KB, patch)
2024-04-24 15:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36575: (bug 34893 follow-up) Return patron when autocreating in Shibboleth (815 bytes, patch)
2024-04-24 15:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36575: Adjust checkpw_internal to return patron (4.46 KB, patch)
2024-04-24 15:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36575: (QA follow-up) Shibboleth POD and checkpw_internal call (1.45 KB, patch)
2024-04-30 14:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36575: Return correct patron when there is a shared userid / cardnumber (10.03 KB, patch)
2024-05-10 06:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36575: (QA follow-up) (7.59 KB, patch)
2024-05-10 06:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36575: (bug 34893 follow-up) Return patron from LDAP (1.59 KB, patch)
2024-05-10 06:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36575: (bug 34893 follow-up) Return patron when autocreating in Shibboleth (910 bytes, patch)
2024-05-10 06:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36575: Adjust checkpw_internal to return patron (4.55 KB, patch)
2024-05-10 06:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36575: (QA follow-up) Shibboleth POD and checkpw_internal call (1.51 KB, patch)
2024-05-10 06:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36575: Unit tests for checkpw_ldap (2.05 KB, patch)
2024-05-30 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36575: Unit tests for checkpw_shib (7.21 KB, patch)
2024-05-30 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36575: Return correct patron when there is a shared userid / cardnumber (10.05 KB, patch)
2024-05-30 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36575: (QA follow-up) (7.61 KB, patch)
2024-05-30 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36575: (bug 34893 follow-up) Return patron from LDAP (1.60 KB, patch)
2024-05-30 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36575: (bug 34893 follow-up) Return patron when autocreating in Shibboleth (927 bytes, patch)
2024-05-30 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36575: Adjust checkpw_internal to return patron (4.57 KB, patch)
2024-05-30 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36575: (QA follow-up) Shibboleth POD and checkpw_internal call (1.53 KB, patch)
2024-05-30 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-04-11 09:38:47 UTC
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
Comment 1 Nick Clemens (kidclamp) 2024-04-11 09:44:53 UTC
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
Comment 2 Nick Clemens (kidclamp) 2024-04-11 09:47:09 UTC
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
Comment 3 Martin Renvoize 2024-04-11 10:37:29 UTC
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>
Comment 4 Martin Renvoize 2024-04-11 10:37:32 UTC
Created attachment 164671 [details] [review]
Bug 36575: (QA follow-up)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2024-04-11 10:39:03 UTC
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.
Comment 6 David Cook 2024-04-12 03:46:44 UTC
(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...
Comment 7 Marcel de Rooy 2024-04-12 06:53:21 UTC
    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 ) ) {
Comment 8 Marcel de Rooy 2024-04-12 06:54:08 UTC
The whole idea of guessing if it is a userid or a cardnumber etc is silly btw ;)
Comment 9 David Cook 2024-04-12 06:58:16 UTC
(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 🙃.

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...
Comment 10 David Cook 2024-04-12 07:02:52 UTC
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...
Comment 11 Tomás Cohen Arazi 2024-04-12 07:39:37 UTC
(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!
Comment 12 Nick Clemens (kidclamp) 2024-04-12 09:22:47 UTC
(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
Comment 13 Kyle M Hall 2024-04-18 15:09:29 UTC
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>
Comment 14 Kyle M Hall 2024-04-18 15:09:36 UTC
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>
Comment 15 Marcel de Rooy 2024-04-19 08:27:48 UTC
Looking here
Comment 16 Marcel de Rooy 2024-04-19 09:27:01 UTC
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.
Comment 17 Nick Clemens (kidclamp) 2024-04-24 15:13:42 UTC
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>
Comment 18 Nick Clemens (kidclamp) 2024-04-24 15:13:45 UTC
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>
Comment 19 Nick Clemens (kidclamp) 2024-04-24 15:13:48 UTC
Created attachment 165469 [details] [review]
Bug 36575: (bug 34893 follow-up) Return patron from LDAP
Comment 20 Nick Clemens (kidclamp) 2024-04-24 15:13:50 UTC
Created attachment 165470 [details] [review]
Bug 36575: (bug 34893 follow-up) Return patron when autocreating in Shibboleth
Comment 21 Nick Clemens (kidclamp) 2024-04-24 15:13:53 UTC
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
Comment 22 Martin Renvoize 2024-04-26 06:05:44 UTC
Bug 36161 touches the same code ;). We should look at both in quick succession in my opinion.
Comment 23 Marcel de Rooy 2024-04-30 13:43:58 UTC
Revisiting this one
Comment 24 Marcel de Rooy 2024-04-30 14:47:48 UTC
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.
Comment 25 Marcel de Rooy 2024-04-30 14:48:17 UTC
Created attachment 165908 [details] [review]
Bug 36575: (QA follow-up) Shibboleth POD and checkpw_internal call
Comment 26 Marcel de Rooy 2024-04-30 14:54:39 UTC
(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.
Comment 27 Nick Clemens (kidclamp) 2024-05-03 11:12:21 UTC
(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?
Comment 28 Kyle M Hall 2024-05-03 14:30:03 UTC
> 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.
Comment 29 Marcel de Rooy 2024-05-10 06:28:36 UTC
Coming here again
Comment 30 Marcel de Rooy 2024-05-10 06:50:59 UTC
(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.
Comment 31 Marcel de Rooy 2024-05-10 06:52:07 UTC
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>
Comment 32 Marcel de Rooy 2024-05-10 06:52:10 UTC
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>
Comment 33 Marcel de Rooy 2024-05-10 06:52:13 UTC
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>
Comment 34 Marcel de Rooy 2024-05-10 06:52:16 UTC
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>
Comment 35 Marcel de Rooy 2024-05-10 06:52:19 UTC
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>
Comment 36 Marcel de Rooy 2024-05-10 06:52:22 UTC
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>
Comment 37 wainuiwitikapark 2024-05-22 02:37:44 UTC
22.05 needs the dependency Bug 30962
Comment 38 wainuiwitikapark 2024-05-30 06:04:40 UTC
(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.
Comment 39 Tomás Cohen Arazi 2024-05-30 14:37:30 UTC
*** Bug 36928 has been marked as a duplicate of this bug. ***
Comment 40 Tomás Cohen Arazi 2024-05-30 14:45:26 UTC
Where are the tests?
Comment 41 Nick Clemens (kidclamp) 2024-05-30 14:51:26 UTC
(In reply to Tomás Cohen Arazi from comment #40)
> Where are the tests?

Tests are added in the first patch
Comment 42 Tomás Cohen Arazi 2024-05-30 16:22:59 UTC
(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.
Comment 43 Tomás Cohen Arazi 2024-05-30 16:54:34 UTC
Created attachment 167276 [details] [review]
Bug 36575: Unit tests for checkpw_ldap

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 44 Tomás Cohen Arazi 2024-05-30 16:54:38 UTC
Created attachment 167277 [details] [review]
Bug 36575: Unit tests for checkpw_shib

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Tomás Cohen Arazi 2024-05-30 16:54:41 UTC
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>
Comment 46 Tomás Cohen Arazi 2024-05-30 16:54:45 UTC
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>
Comment 47 Tomás Cohen Arazi 2024-05-30 16:54:48 UTC
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>
Comment 48 Tomás Cohen Arazi 2024-05-30 16:54:51 UTC
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>
Comment 49 Tomás Cohen Arazi 2024-05-30 16:54:55 UTC
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>
Comment 50 Tomás Cohen Arazi 2024-05-30 16:54:58 UTC
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>
Comment 51 Katrin Fischer 2024-06-20 16:11:19 UTC
Pushed for 24.11!
Well done everyone, thank you!