Bugzilla – Attachment 129025 Details for
Bug 29543
Self-checkout allows returning everybody's loans
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 29543: Enforce authentication for self-checkout
Bug-29543-Enforce-authentication-for-self-checkout.patch (text/plain), 7.59 KB, created by
Jonathan Druart
on 2022-01-05 14:13:30 UTC
(
hide
)
Description:
Bug 29543: Enforce authentication for self-checkout
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2022-01-05 14:13:30 UTC
Size:
7.59 KB
patch
obsolete
>From f03233a9e2c0139d39395346eaaa16ea557c8da8 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Wed, 5 Jan 2022 12:47:10 +0100 >Subject: [PATCH] Bug 29543: 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. >--- > opac/sco/sco-main.pl | 63 +++++++++++++++++++++++++++----------------- > 1 file changed, 39 insertions(+), 24 deletions(-) > >diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl >index 4dbd97aabea..5ab0d42c76d 100755 >--- a/opac/sco/sco-main.pl >+++ b/opac/sco/sco-main.pl >@@ -21,13 +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. >+# The patron id/cardnumber is retrieved from the JWT > > use Modern::Perl; > >@@ -94,9 +93,8 @@ if (defined C4::Context->preference('SCOAllowCheckin')) { > } > > 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") || '', >@@ -104,16 +102,30 @@ 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; >+} >+ > $barcode = barcodedecode( $barcode ) if $barcode; > > 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 $patron; >@@ -125,15 +137,12 @@ if ( $patronid ) { > my $branch = $issuer->{branchcode}; > my $confirm_required = 0; > my $return_only = 0; >-if ($op eq "logout") { >- $template->param( loggedout => 1 ); >- $query->param( patronid => undef, patronlogin => undef, patronpw => undef ); >-} >-elsif ( $op eq "returnbook" && $allowselfcheckreturns ) { >+ >+if ( $patron && $op eq "returnbook" && $allowselfcheckreturns ) { > my $success = 0; > my $human_required = 0; >+ my $item = Koha::Items->find( { barcode => $barcode } ); > if ( C4::Context->preference("CircConfirmItemParts") ) { >- my $item = Koha::Items->find( { barcode => $barcode } ); > if ( defined($item) > && $item->materials ) > { >@@ -146,6 +155,8 @@ elsif ( $op eq "returnbook" && $allowselfcheckreturns ) { > $template->param( returned => $success ); > } > elsif ( $patron && ( $op eq 'checkout' ) ) { >+ >+ my $item = Koha::Items->find( { barcode => $barcode } ); > my $impossible = {}; > my $needconfirm = {}; > ( $impossible, $needconfirm ) = CanBookBeIssued( >@@ -166,7 +177,6 @@ elsif ( $patron && ( $op eq 'checkout' ) ) { > } > } > >- #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 >@@ -181,7 +191,6 @@ elsif ( $patron && ( $op eq 'checkout' ) ) { > 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( >@@ -198,7 +207,6 @@ elsif ( $patron && ( $op eq 'checkout' ) ) { > hide_main => 1, > ); > } elsif ( $confirm_required && !$confirmed ) { >- #warn "failed confirmation"; > $template->param( > impossible => 1, > "circ_error_$issue_error" => 1, >@@ -248,7 +256,6 @@ elsif ( $patron && ( $op eq 'checkout' ) ) { > } > } else { > $confirm_required = 1; >- #warn "issue confirmation"; > $template->param( > confirm => "Issuing title: " . $item->biblio->title, > barcode => $barcode, >@@ -259,16 +266,16 @@ elsif ( $patron && ( $op eq 'checkout' ) ) { > } # $op > > if ( $patron && ( $op eq 'renew' ) ) { >+ my $item = Koha::Items->find({ barcode => $barcode }); > my ($status,$renewerror) = CanBookBeRenewed( $patron->borrowernumber, $item->itemnumber ); > if ($status) { >- #warn "renewing"; > AddRenewal( $patron->borrowernumber, $item->itemnumber, undef, undef, undef, undef, 1 ); > push @newissueslist, $barcode; > $template->param( renewed => 1 ); > } > } > >-if ($patron) { >+if ( $patron) { > my $borrowername = sprintf "%s %s", ($patron->firstname || ''), ($patron->surname || ''); > my $pending_checkouts = $patron->pending_checkouts; > my @checkouts; >@@ -307,7 +314,6 @@ if ($patron) { > ISSUES => \@checkouts, > HOLDS => $holds, > newissues => join(',',@newissueslist), >- patronid => $patronid, > patronlogin => $patronlogin, > patronpw => $patronpw, > waiting_holds_count => $waiting_holds_count, >@@ -344,9 +350,18 @@ if ($patron) { > } > } 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 }; >-- >2.25.1
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 29543
:
129022
|
129023
|
129024
|
129025
|
129026
|
129031
|
129032
|
129033
|
129034
|
129035
|
129036
|
129037
|
129039
|
129133
|
129210
|
129211
|
129212
|
129213
|
129214
|
129215
|
129216
|
129217
|
129679