gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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 <nick@bywatersolutions.com>

Bug 29543: Remove borrower variable

It's not needed, we have $patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Remove inputfocus variable

It's not used in template

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

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 <nick@bywatersolutions.com>

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 <nick@bywatersolutions.com>

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 <nick@bywatersolutions.com>

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 <wainuiwitikapark@catalyst.net.nz>
This commit is contained in:
Jonathan Druart 2022-01-05 11:24:12 +01:00 committed by Wainui Witika-Park
parent ecfdb05e9e
commit b257f25b1b
6 changed files with 179 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
C4/Installer/PerlDependencies.pm
View File

@ -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',

88
Koha/Token.pm
View File

@ -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

1
koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
View File

@ -986,6 +986,7 @@ Circulation:
choices:
yes: Username and Password
no: Cardnumber
- ".</br>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

9
koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt
View File

@ -190,6 +190,11 @@
<div class="alert alert-info">
<p>Item renewed</p>
</div>
[% ELSIF ( renewed == 0) %]
<span class="sco-alert-warning renew"></span>
<div class="alert alert-info">
<p>Item not renewed</p>
</div>
[% ELSIF ( returned == 0 ) %]
<span class="sco-alert-warning return"></span>
<div class="alert alert-info">
@ -372,9 +377,9 @@
[% IF ( Koha.Preference('SelfCheckoutByLogin') ) %]
<legend>Log in to your account</legend>
<label for="patronlogin">Login:</label>
<input type="text" id="patronlogin" class="focus" size="20" name="patronlogin" />
<input type="text" id="patronlogin" class="focus" size="20" name="patronlogin" autocomplete="off"/>
<label for="patronpw">Password:</label>
<input type="password" id="patronpw" size="20" name="patronpw" />
<input type="password" id="patronpw" size="20" name="patronpw" autocomplete="off"/>
<fieldset class="action">
<button type="submit" class="btn">Log in</button>
</fieldset>

107
opac/sco/sco-main.pl
View File

@ -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 };

18
t/Token.t
View File

@ -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' );
};