Bugzilla – Attachment 164670 Details for
Bug 36575
Wrong patron can be returned for API validation route
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 36575: Return correct patron when there is a shared userid / cardnumber
Bug-36575-Return-correct-patron-when-there-is-a-sh.patch (text/plain), 9.89 KB, created by
Martin Renvoize (ashimema)
on 2024-04-11 10:37:29 UTC
(
hide
)
Description:
Bug 36575: Return correct patron when there is a shared userid / cardnumber
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2024-04-11 10:37:29 UTC
Size:
9.89 KB
patch
obsolete
>From 475c9beeea2bb9937cb2e01dd19c3cefc987d404 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Thu, 11 Apr 2024 09:39:03 +0000 >Subject: [PATCH] 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> >--- > C4/Auth.pm | 35 +++++++----- > t/db_dependent/Auth.t | 29 +++++++++- > t/db_dependent/api/v1/password_validation.t | 61 ++++++++++++++++++++- > 3 files changed, 107 insertions(+), 18 deletions(-) > >diff --git a/C4/Auth.pm b/C4/Auth.pm >index 86ddd3baf09..594ef21f8a9 100644 >--- a/C4/Auth.pm >+++ b/C4/Auth.pm >@@ -1965,23 +1965,17 @@ sub checkpw { > my $shib_login = $shib ? get_login_shib() : undef; > > my @return; >- my $patron; >- if ( defined $userid ) { >- $patron = Koha::Patrons->find( { userid => $userid } ); >- $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron; >- } > my $check_internal_as_fallback = 0; > my $passwd_ok = 0; >+ my $patron; >+ > > # Note: checkpw_* routines returns: > # 1 if auth is ok > # 0 if auth is nok > # -1 if user bind failed (LDAP only) > >- if ( $patron and ( $patron->account_locked ) ) { >- >- # Nothing to check, account is locked >- } elsif ( $ldap && defined($password) ) { >+ if ( $ldap && defined($password) ) { > my ( $retval, $retcard, $retuserid ); > ( $retval, $retcard, $retuserid, $patron ) = checkpw_ldap(@_); # EXTERNAL AUTH > if ( $retval == 1 ) { >@@ -1995,7 +1989,8 @@ sub checkpw { > # In case of a CAS authentication, we use the ticket instead of the password > my $ticket = $query->param('ticket'); > $query->delete('ticket'); # remove ticket to come back to original URL >- my ( $retval, $retcard, $retuserid, $cas_ticket, $patron ) = >+ my ( $retval, $retcard, $retuserid, $cas_ticket ); >+ ( $retval, $retcard, $retuserid, $cas_ticket, $patron ) = > checkpw_cas( $ticket, $query, $type ); # EXTERNAL AUTH > if ($retval) { > @return = ( $retval, $retcard, $retuserid, $patron, $cas_ticket ); >@@ -2016,7 +2011,8 @@ sub checkpw { > > # Then, we check if it matches a valid koha user > if ($shib_login) { >- my ( $retval, $retcard, $retuserid, $patron ) = >+ my ( $retval, $retcard, $retuserid ); >+ ( $retval, $retcard, $retuserid, $patron ) = > C4::Auth_with_shibboleth::checkpw_shib($shib_login); # EXTERNAL AUTH > if ($retval) { > @return = ( $retval, $retcard, $retuserid, $patron ); >@@ -2027,20 +2023,29 @@ sub checkpw { > $check_internal_as_fallback = 1; > } > >+ if ( $check_internal_as_fallback ){ > # INTERNAL AUTH >- if ($check_internal_as_fallback) { > @return = checkpw_internal( $userid, $password, $no_set_userenv ); >- push( @return, $patron ); > $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; > } > > if ($patron) { >- if ($passwd_ok) { >+ if( $patron->account_locked ){ >+ @return = (); >+ } elsif ($passwd_ok) { > $patron->update( { login_attempts => 0 } ); > if ( $patron->password_expired ) { > @return = ( -2, $patron ); > } >- } elsif ( !$patron->account_locked ) { >+ } else { > $patron->update( { login_attempts => $patron->login_attempts + 1 } ); > } > } >diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t >index c66b2a469af..a8bfa6ea65b 100755 >--- a/t/db_dependent/Auth.t >+++ b/t/db_dependent/Auth.t >@@ -7,7 +7,7 @@ use CGI qw ( -utf8 ); > use Test::MockObject; > use Test::MockModule; > use List::MoreUtils qw/all any none/; >-use Test::More tests => 21; >+use Test::More tests => 22; > use Test::Warn; > use t::lib::Mocks; > use t::lib::TestBuilder; >@@ -458,7 +458,7 @@ subtest 'checkpw lockout tests' => sub { > ( $checkpw, undef, undef ) = checkpw( $patron->userid, $password, undef, undef, 1 ); > is( $checkpw, undef, 'checkpw returns undef with right password when account locked' ); > ( $checkpw, undef, undef ) = checkpw( $patron->cardnumber, $password, undef, undef, 1 ); >- is( $checkpw, undef, 'checkpw returns undefwith right password when logging in via cardnumber if account locked' ); >+ is( $checkpw, undef, 'checkpw returns undef with right password when logging in via cardnumber if account locked' ); > > }; > >@@ -1442,6 +1442,31 @@ subtest 'AutoSelfCheckAllowed' => sub { > is( $loggedinuser, $sco_patron->id ); > }; > >+subtest 'checkpw for users with shared cardnumber / userid ' => sub { >+ >+ plan tests => 8; >+ >+ t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); >+ my $library = $builder->build_object( { class => 'Koha::Libraries' } ); >+ my $patron_1 = $builder->build_object( { class => 'Koha::Patrons' } ); >+ $patron_1->set_password( { password => "OnePassword" } ); >+ my $patron_2 = $builder->build_object( { class => 'Koha::Patrons', value => { userid => $patron_1->cardnumber } } ); >+ $patron_2->set_password( { password => "PasswordTwo" } ); >+ >+ my ( $checkpw, $cardnumber, $userid, $patron ) = checkpw( $patron_1->cardnumber, "OnePassword", undef, undef, 1 ); >+ ok( $checkpw, 'checkpw returns true for right password when logging in via cardnumber' ); >+ is( $cardnumber, $patron_1->cardnumber, 'checkpw returns correct cardnumber' ); >+ is( $userid, $patron_1->userid, 'checkpw returns correct userid' ); >+ is( $patron->id, $patron_1->id, 'checkpw returns correct patron' ); >+ >+ ( $checkpw, $cardnumber, $userid, $patron ) = checkpw( $patron_2->userid, "PasswordTwo", undef, undef, 1 ); >+ ok( $checkpw, 'checkpw returns true for right password when logging in via userid' ); >+ is( $cardnumber, $patron_2->cardnumber, 'checkpw returns correct cardnumber' ); >+ is( $userid, $patron_2->userid, 'checkpw returns correct userid' ); >+ is( $patron->id, $patron_2->id, 'checkpw returns correct patron' ); >+ >+}; >+ > sub set_weak_password { > my ($patron) = @_; > my $password = 'password'; >diff --git a/t/db_dependent/api/v1/password_validation.t b/t/db_dependent/api/v1/password_validation.t >index 10527bc93d0..1aef7187008 100755 >--- a/t/db_dependent/api/v1/password_validation.t >+++ b/t/db_dependent/api/v1/password_validation.t >@@ -18,7 +18,7 @@ > > use Modern::Perl; > >-use Test::More tests => 5; >+use Test::More tests => 6; > use Test::Mojo; > > use t::lib::TestBuilder; >@@ -239,4 +239,63 @@ subtest 'password validation - expired password' => sub { > $schema->storage->txn_rollback; > }; > >+subtest 'password validation - users with shared cardnumber / userid' => sub { >+ >+ plan tests => 12; >+ >+ $schema->storage->txn_begin; >+ >+ my $patron_1 = $builder->build_object( >+ { >+ class => 'Koha::Patrons', >+ value => { } >+ } >+ ); >+ my $patron_password_1 = 'thePassword123'; >+ $patron_1->set_password( { password => $patron_password_1, skip_validation => 1 } ); >+ >+ my $patron_2 = $builder->build_object( >+ { >+ class => 'Koha::Patrons', >+ value => { userid => $patron_1->cardnumber } >+ } >+ ); >+ my $patron_password_2 = 'thePassword345'; >+ $patron_2->set_password( { password => $patron_password_2, skip_validation => 1 } ); >+ >+ my $json = { >+ identifier => $patron_1->cardnumber, >+ password => $patron_password_1, >+ }; >+ >+ $t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201) >+ ->json_is({ cardnumber => $patron_1->cardnumber, patron_id => $patron_1->borrowernumber, userid => $patron_1->userid} ); >+ >+ $json = { >+ identifier => $patron_2->userid, >+ password => $patron_password_2, >+ }; >+ >+ $t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201) >+ ->json_is({ cardnumber => $patron_2->cardnumber, patron_id => $patron_2->borrowernumber, userid => $patron_2->userid} ); >+ >+ my $json = { >+ userid => $patron_1->cardnumber, >+ password => $patron_password_1, >+ }; >+ >+ $t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201) >+ ->json_is({ cardnumber => $patron_1->cardnumber, patron_id => $patron_1->borrowernumber, userid => $patron_1->userid} ); >+ >+ $json = { >+ userid => $patron_2->userid, >+ password => $patron_password_2, >+ }; >+ >+ $t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201) >+ ->json_is({ cardnumber => $patron_2->cardnumber, patron_id => $patron_2->borrowernumber, userid => $patron_2->userid} ); >+ >+ $schema->storage->txn_rollback; >+}; >+ > $schema->storage->txn_rollback; >-- >2.44.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 36575
:
164668
|
164670
|
164671
|
165133
|
165134
|
165467
|
165468
|
165469
|
165470
|
165471
|
165908
|
166493
|
166494
|
166495
|
166496
|
166497
|
166498
|
167276
|
167277
|
167278
|
167279
|
167280
|
167281
|
167282
|
167283