From b257f25b1b030cf08f35fa8ecaf313bdc514a931 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 5 Jan 2022 11:24:12 +0100 Subject: [PATCH] Bug 29543: [19.11] Enforce authentication for self-checkout The self-checkout feature is assuming a patron is logged in if patronid is passed. It also assumes that "We're in a controlled environment; we trust the user", which is terribly wrong! This patch is suggesting to generate a JSON Web Token (JWT) to store in a cookie and only allow action (renew, check in/out) is the token is valid. The token is only generated once the user has been authenticated And is removed when the user finish the session/logout. Test plan: You must know exactly how the self-checkout feature works to test this patch. The 4 following sysprefs must be tested: SelfCheckoutByLogin, AutoSelfCheckAllowed, AutoSelfCheckID, AutoSelfCheckPass Confirm that you can renew, checkin for the items you own, and checkout new items. Confirm that you are not allowed to access other account's info. Signed-off-by: Nick Clemens Bug 29543: Remove borrower variable It's not needed, we have $patron Signed-off-by: Nick Clemens Bug 29543: Remove inputfocus variable It's not used in template Signed-off-by: Nick Clemens Bug 29543: Add JWT token handling Mojo::JWT is installed already, it's not a new dependency. We need a way to send the patron a token when it's correctly logged in, and not assumed it's logged in only if patronid is passed Signed-off-by: Nick Clemens Bug 29543: Prevent user to checkin or renew items they don't own Checkin or renew must be restricted to the items they own. Test plan: Create an item with barcode bc_1 Check it in to user A Login to SCO with user B Get the token using the browser dev tool, from the cookie Hit (replace $JWT) /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=renew&barcode=bc_1 /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=returnbook&barcode=bc_1 You should see an error message Signed-off-by: Nick Clemens Bug 29543: (follow-up) Add a warning to SelfCheckoutByLogin This updates the language to warn users of risk if using cardnumber for login and auto-self-check is enabled Signed-off-by: Nick Clemens Bug 29543: Add Mojo::JWT dependency Bug 29543: Set autocomplete off for SCO login fields Cardnumber already had it set, adding for username and password Signed-off-by: Wainui Witika-Park --- C4/Installer/PerlDependencies.pm | 5 + Koha/Token.pm | 88 +++++++++++++- .../admin/preferences/circulation.pref | 1 + .../bootstrap/en/modules/sco/sco-main.tt | 9 +- opac/sco/sco-main.pl | 107 ++++++++++-------- t/Token.t | 18 ++- 6 files changed, 179 insertions(+), 49 deletions(-) diff --git a/C4/Installer/PerlDependencies.pm b/C4/Installer/PerlDependencies.pm index 19dd06c417..0599acb7e5 100644 --- a/C4/Installer/PerlDependencies.pm +++ b/C4/Installer/PerlDependencies.pm @@ -767,6 +767,11 @@ our $PERL_DEPS = { 'required' => '1', 'min_ver' => '0.05', }, + 'Mojo::JWT' => { + 'usage' => 'Core', + 'required' => '1', + 'min_ver' => '0.08', + }, 'Mojolicious' => { 'usage' => 'REST API', 'required' => '1', diff --git a/Koha/Token.pm b/Koha/Token.pm index dafa1b24b6..d3bca1cd18 100644 --- a/Koha/Token.pm +++ b/Koha/Token.pm @@ -1,6 +1,6 @@ package Koha::Token; -# Created as wrapper for CSRF tokens, but designed for more general use +# Created as wrapper for CSRF and JWT tokens, but designed for more general use # Copyright 2016 Rijksmuseum # @@ -52,6 +52,7 @@ use Modern::Perl; use Bytes::Random::Secure (); use String::Random (); use WWW::CSRF (); +use Mojo::JWT; use Digest::MD5 qw(md5_base64); use Encode qw( encode ); use Koha::Exceptions::Token; @@ -78,6 +79,9 @@ sub new { my $csrf_token = $tokenizer->generate({ type => 'CSRF', id => $id, secret => $secret, }); + my $jwt = $tokenizer->generate({ + type => 'JWT, id => $id, secret => $secret, + }); Generate several types of tokens. Now includes CSRF. For non-CSRF tokens an optional pattern parameter overrides length. @@ -101,6 +105,8 @@ sub generate { my ( $self, $params ) = @_; if( $params->{type} && $params->{type} eq 'CSRF' ) { $self->{lasttoken} = _gen_csrf( $params ); + } elsif( $params->{type} && $params->{type} eq 'JWT' ) { + $self->{lasttoken} = _gen_jwt( $params ); } else { $self->{lasttoken} = _gen_rand( $params ); } @@ -122,6 +128,21 @@ sub generate_csrf { return $self->generate({ %$params, type => 'CSRF' }); } +=head2 generate_jwt + + Like: generate({ type => 'JWT', ... }) + Note that JWT is designed to encode a structure but here we are actually only allowing a value + that will be store in the key 'id'. + +=cut + +sub generate_jwt { + my ( $self, $params ) = @_; + return if !$params->{id}; + $params = _add_default_jwt_params( $params ); + return $self->generate({ %$params, type => 'JWT' }); +} + =head2 check my $result = $tokenizer->check({ @@ -138,6 +159,9 @@ sub check { if( $params->{type} && $params->{type} eq 'CSRF' ) { return _chk_csrf( $params ); } + elsif( $params->{type} && $params->{type} eq 'JWT' ) { + return _chk_jwt( $params ); + } return; } @@ -156,6 +180,33 @@ sub check_csrf { return $self->check({ %$params, type => 'CSRF' }); } +=head2 check_jwt + + Like: check({ type => 'JWT', id => $id, token => $token }) + + Will return true if the token contains the passed id + +=cut + +sub check_jwt { + my ( $self, $params ) = @_; + $params = _add_default_jwt_params( $params ); + return $self->check({ %$params, type => 'JWT' }); +} + +=head2 decode_jwt + + $tokenizer->decode_jwt({ type => 'JWT', token => $token }) + + Will return the value of the id stored in the token. + +=cut +sub decode_jwt { + my ( $self, $params ) = @_; + $params = _add_default_jwt_params( $params ); + return _decode_jwt( $params ); +} + # --- Internal routines --- sub _add_default_csrf_params { @@ -220,6 +271,41 @@ sub _gen_rand { return $token; } +sub _add_default_jwt_params { + my ( $params ) = @_; + my $pw = C4::Context->config('pass'); + $params->{secret} //= md5_base64( Encode::encode( 'UTF-8', $pw ) ), + return $params; +} + +sub _gen_jwt { + my ( $params ) = @_; + return if !$params->{id} || !$params->{secret}; + + return Mojo::JWT->new( + claims => { id => $params->{id} }, + secret => $params->{secret} + )->encode; +} + +sub _chk_jwt { + my ( $params ) = @_; + return if !$params->{id} || !$params->{secret} || !$params->{token}; + + my $claims = Mojo::JWT->new(secret => $params->{secret})->decode($params->{token}); + + return 1 if exists $claims->{id} && $claims->{id} == $params->{id}; +} + +sub _decode_jwt { + my ( $params ) = @_; + return if !$params->{token} || !$params->{secret}; + + my $claims = Mojo::JWT->new(secret => $params->{secret})->decode($params->{token}); + + return $claims->{id}; +} + =head1 AUTHOR Marcel de Rooy, Rijksmuseum Amsterdam, The Netherlands diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 2c17f08a7f..bcb647443c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -986,6 +986,7 @@ Circulation: choices: yes: Username and Password no: Cardnumber + - ".
NOTE: If using 'cardnumber' and AutoSelfCheckAllowed you should set SelfCheckAllowByIPRanges to prevent brute force attacks to gain patron information outside the library." - - "Time out the current patron's web-based self checkout system login after" - pref: SelfCheckTimeout diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt index 8a8d70bdfe..65912102f7 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt @@ -190,6 +190,11 @@

Item renewed

+ [% ELSIF ( renewed == 0) %] + +
+

Item not renewed

+
[% ELSIF ( returned == 0 ) %]
@@ -372,9 +377,9 @@ [% IF ( Koha.Preference('SelfCheckoutByLogin') ) %] Log in to your account - + - +
diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl index a901d254b1..ceaa19c474 100755 --- a/opac/sco/sco-main.pl +++ b/opac/sco/sco-main.pl @@ -21,15 +21,12 @@ # We're going to authenticate a self-check user. we'll add a flag to borrowers 'selfcheck' # -# We're in a controlled environment; we trust the user. -# So the selfcheck station will accept a patronid and issue items to that borrower. -# FIXME: NOT really a controlled environment... We're on the internet! +# We're not in a controlled environment; we never trust the user. # # The checkout permission comes form the CGI cookie/session of a staff user. # The patron is not really logging in here in the same way as they do on the # rest of the OPAC. So don't confuse loggedinuser with the patron user. -# -# FIXME: inputfocus not really used in TMPL +# The patron id/cardnumber is retrieved from the JWT use Modern::Perl; @@ -99,9 +96,8 @@ if (defined C4::Context->preference('AllowSelfCheckReturns')) { } my $issuerid = $loggedinuser; -my ($op, $patronid, $patronlogin, $patronpw, $barcode, $confirmed, $newissues) = ( +my ($op, $patronlogin, $patronpw, $barcode, $confirmed, $newissues) = ( $query->param("op") || '', - $query->param("patronid") || '', $query->param("patronlogin")|| '', $query->param("patronpw") || '', $query->param("barcode") || '', @@ -109,36 +105,56 @@ my ($op, $patronid, $patronlogin, $patronpw, $barcode, $confirmed, $newissues) = $query->param("newissues") || '', ); +my $jwt = $query->cookie('JWT'); +if ($op eq "logout") { + $template->param( loggedout => 1 ); + $query->param( patronlogin => undef, patronpw => undef ); + undef $jwt; +} + my @newissueslist = split /,/, $newissues; my $issuenoconfirm = 1; #don't need to confirm on issue. my $issuer = Koha::Patrons->find( $issuerid )->unblessed; -my $item = Koha::Items->find({ barcode => $barcode }); -if (C4::Context->preference('SelfCheckoutByLogin') && !$patronid) { - my $dbh = C4::Context->dbh; - my $resval; - ($resval, $patronid) = checkpw($dbh, $patronlogin, $patronpw); + +my $patronid = $jwt ? Koha::Token->new->decode_jwt({ token => $jwt }) : undef; +unless ( $patronid ) { + if ( C4::Context->preference('SelfCheckoutByLogin') ) { + my $dbh = C4::Context->dbh; + ( undef, $patronid ) = checkpw( $dbh, $patronlogin, $patronpw ); + } + else { # People should not do that unless they know what they are doing! + # SelfCheckAllowByIPRanges MUST be configured + $patronid = $query->param('patronid'); + } + $jwt = Koha::Token->new->generate_jwt({ id => $patronid }) if $patronid; } -my ( $borrower, $patron ); +my $patron; if ( $patronid ) { $patron = Koha::Patrons->find( { cardnumber => $patronid } ); - $borrower = $patron->unblessed if $patron; } my $branch = $issuer->{branchcode}; my $confirm_required = 0; my $return_only = 0; -#warn "issuer cardnumber: " . $issuer->{cardnumber}; -#warn "patron cardnumber: " . $borrower->{cardnumber}; if ($op eq "logout") { $template->param( loggedout => 1 ); $query->param( patronid => undef, patronlogin => undef, patronpw => undef ); } -elsif ( $op eq "returnbook" && $allowselfcheckreturns ) { - my ($doreturn) = AddReturn( $barcode, $branch ); + +if ( $op eq "returnbook" && $allowselfcheckreturns ) { + + # Patron cannot checkin an item they don't own + my $item = Koha::Items->find( { barcode => $barcode } ); + my $doreturn = $patron->checkouts->find( { itemnumber => $item->itemnumber} ) ? 1 : 0; + + if ( $doreturn ) { + ($doreturn) = AddReturn( $barcode, $branch ); + } $template->param( returned => $doreturn ); } elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { + my $item = Koha::Items->find( { barcode => $barcode } ); my $impossible = {}; my $needconfirm = {}; ( $impossible, $needconfirm ) = CanBookBeIssued( @@ -159,7 +175,6 @@ elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { } } - #warn "confirm_required: " . $confirm_required ; if (scalar keys %$impossible) { my $issue_error = (keys %$impossible)[0]; # FIXME This is wrong, we assume only one error and keys are not ordered @@ -174,7 +189,6 @@ elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { if ($issue_error eq 'DEBT') { $template->param(DEBT => $impossible->{DEBT}); } - #warn "issue_error: " . $issue_error ; if ( $issue_error eq "NO_MORE_RENEWALS" ) { $return_only = 1; $template->param( @@ -184,10 +198,13 @@ elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { } } elsif ( $needconfirm->{RENEW_ISSUE} || $op eq 'renew' ) { if ($confirmed) { - #warn "renewing"; - AddRenewal( $borrower->{borrowernumber}, $item->itemnumber ); - push @newissueslist, $barcode; - $template->param( renewed => 1 ); + if ( $patron->checkouts->find( { itemnumber => $item->itemnumber } ) ) { + AddRenewal( $patron->borrowernumber, $item->itemnumber ); + push @newissueslist, $barcode; + $template->param( renewed => 1 ); + } else { + $template->param( renewed => 0 ); + } } else { #warn "renew confirmation"; $template->param( @@ -199,7 +216,6 @@ elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { ); } } elsif ( $confirm_required && !$confirmed ) { - #warn "failed confirmation"; $template->param( impossible => 1, "circ_error_$issue_error" => 1, @@ -218,7 +234,7 @@ elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { $hold_existed = Koha::Holds->search( { -and => { - borrowernumber => $borrower->{borrowernumber}, + borrowernumber => $patron->borrowernumber, -or => { biblionumber => $item->biblionumber, itemnumber => $item->itemnumber @@ -228,7 +244,7 @@ elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { )->count; } - AddIssue( $borrower, $barcode ); + AddIssue( $patron->unblessed, $barcode ); $template->param( issued => 1 ); push @newissueslist, $barcode; @@ -239,7 +255,7 @@ elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { # Note that this should not be needed but since we do not have proper exception handling here we do it this way patron_has_hold_fee => Koha::Account::Lines->search( { - borrowernumber => $borrower->{borrowernumber}, + borrowernumber => $patron->borrowernumber, debit_type_code => 'RESERVE', description => $item->biblio->title, date => $dtf->format_date(dt_from_string) @@ -249,27 +265,23 @@ elsif ( $patron && ( $op eq 'checkout' || $op eq 'renew' ) ) { } } else { $confirm_required = 1; - #warn "issue confirmation"; $template->param( confirm => "Issuing title: " . $item->biblio->title, barcode => $barcode, hide_main => 1, - inputfocus => 'confirm', ); } } } # $op -if ($borrower) { -# warn "issuer's branchcode: " . $issuer->{branchcode}; -# warn "user's branchcode: " . $borrower->{branchcode}; - my $borrowername = sprintf "%s %s", ($borrower->{firstname} || ''), ($borrower->{surname} || ''); +if ($patron) { + my $borrowername = sprintf "%s %s", ($patron->firstname || ''), ($patron->surname || ''); my $pending_checkouts = $patron->pending_checkouts; my @checkouts; while ( my $c = $pending_checkouts->next ) { my $checkout = $c->unblessed_all_relateds; my ($can_be_renewed, $renew_error) = CanBookBeRenewed( - $borrower->{borrowernumber}, + $patron->borrowernumber, $checkout->{itemnumber}, ); $checkout->{can_be_renewed} = $can_be_renewed; # In the future this will be $checkout->can_be_renewed @@ -301,12 +313,11 @@ if ($borrower) { ISSUES => \@checkouts, HOLDS => $holds, newissues => join(',',@newissueslist), - patronid => $patronid, patronlogin => $patronlogin, patronpw => $patronpw, waiting_holds_count => $waiting_holds_count, noitemlinks => 1 , - borrowernumber => $borrower->{'borrowernumber'}, + borrowernumber => $patron->borrowernumber, SuspendHoldsOpac => C4::Context->preference('SuspendHoldsOpac'), AutoResumeSuspendedHolds => C4::Context->preference('AutoResumeSuspendedHolds'), howpriority => $show_priority, @@ -316,34 +327,40 @@ if ($borrower) { my $patron_messages = Koha::Patron::Messages->search( { - borrowernumber => $borrower->{'borrowernumber'}, + borrowernumber => $patron->borrowernumber, message_type => 'B', } ); $template->param( patron_messages => $patron_messages, - opacnote => $borrower->{opacnote}, + opacnote => $patron->opacnote, ); - my $inputfocus = ($return_only == 1) ? 'returnbook' : - ($confirm_required == 1) ? 'confirm' : 'barcode' ; $template->param( - inputfocus => $inputfocus, nofines => 1, ); if (C4::Context->preference('ShowPatronImageInWebBasedSelfCheck')) { - my $patron_image = Koha::Patron::Images->find($borrower->{borrowernumber}); + my $patron_image = $patron->image; $template->param( display_patron_image => 1, - csrf_token => Koha::Token->new->generate_csrf( { session_id => scalar $query->cookie('CGISESSID') . $borrower->{cardnumber}, id => $borrower->{userid}} ), + csrf_token => Koha::Token->new->generate_csrf( { session_id => scalar $query->cookie('CGISESSID') . $patron->cardnumber, id => $patron->userid } ), ) if $patron_image; } } else { $template->param( - patronid => $patronid, nouser => $patronid, ); } +$cookie = $query->cookie( + -name => 'JWT', + -value => $jwt // '', + -expires => $jwt ? '+1d' : '', + -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), +); + +$template->param(patronid => $patronid); + output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 }; diff --git a/t/Token.t b/t/Token.t index 13548e3d17..e9c63a279c 100644 --- a/t/Token.t +++ b/t/Token.t @@ -20,7 +20,7 @@ # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. use Modern::Perl; -use Test::More tests => 11; +use Test::More tests => 12; use Test::Exception; use Time::HiRes qw|usleep|; use C4::Context; @@ -101,3 +101,19 @@ subtest 'Pattern parameter' => sub { ok( $id !~ /[^A-Z]/, 'Only uppercase letters' ); throws_ok( sub { $tokenizer->generate({ pattern => 'abc{d,e}', }) }, 'Koha::Exceptions::Token::BadPattern', 'Exception should be thrown when wrong pattern is used'); }; + +subtest 'JWT' => sub { + plan tests => 3; + + my $id = 42; + my $jwt = $tokenizer->generate_jwt({ id => $id }); + + my $is_valid = $tokenizer->check_jwt({ id => $id, token => $jwt }); + is( $is_valid, 1, 'valid token should return 1' ); + + $is_valid = $tokenizer->check_jwt({ id => 24, token => $jwt }); + isnt( $is_valid, 1, 'invalid token should not return 1' ); + + my $retrieved_id = $tokenizer->decode_jwt({ token => $jwt }); + is( $retrieved_id, $id, 'id stored in jwt should be correct' ); +}; -- 2.39.5