Bug 18349 - Possibility to checkout/renew bypassing the circ rules using SCO
Summary: Possibility to checkout/renew bypassing the circ rules using SCO
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 18251
  Show dependency treegraph
 
Reported: 2017-03-29 21:36 UTC by Jonathan Druart
Modified: 2020-08-26 07:01 UTC (History)
10 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:


Attachments
Bug 18349: SCO - Do not trust the confirmed flag (1.66 KB, patch)
2017-03-29 21:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18349: Remove unused $issuenoconfirm var (1.59 KB, patch)
2017-03-29 21:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18349: $borrower is a hashref (882 bytes, patch)
2017-03-31 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18349: If a confirmation is required, consider as blocker (2.47 KB, patch)
2017-03-31 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18349: Remove useless info in alert message (4.19 KB, patch)
2017-03-31 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18349: SCO - Do not trust the confirmed flag (1.72 KB, patch)
2017-04-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18349: $borrower is a hashref (939 bytes, patch)
2017-04-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18349: If a confirmation is required, consider as blocker (2.53 KB, patch)
2017-04-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18349: Remove useless info in alert message (4.25 KB, patch)
2017-04-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18349: [QA Follow-up] Add four missing error messages (2.30 KB, patch)
2017-04-07 08:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-03-29 21:36:30 UTC

    
Comment 1 Jonathan Druart 2017-03-29 21:43:03 UTC
Created attachment 61719 [details] [review]
Bug 18349: SCO - Do not trust the confirmed flag

The "confirmed" flag is used to know if that user confirmed a situation
that needs a confirmation.
But if the issue/renew is impossible the CanBookBeIssued and the
'impossible flags' should be checked.

Otherwise a patron can checkout and renew bypassing the circulation
rules (Understand 'no limit' here...)

Test plan:
Want to renew?
Checkin $barcode, then
/cgi-bin/koha/sco/sco-main.pl?patronid=$cardnumber&barcode=$barcode&confirmed=1&op=checkout

Want to bypass the checkin?
Same url...
Comment 2 Jonathan Druart 2017-03-29 21:43:09 UTC
Created attachment 61720 [details] [review]
Bug 18349: Remove unused $issuenoconfirm var
Comment 3 Jonathan Druart 2017-03-29 21:45:02 UTC
Want some fun guys?
Comment 4 Nick Clemens 2017-03-30 21:04:11 UTC
Hmm..With this patch
1 - I checkout item #1 to borrower A
2 - Sign in to SCO as borrower B
3 - Construct link as specified using barcode of item #1 and cardnumber of borrower B
4 - Item checks out to borrower B with no problem
5 - Renewals never work and no failure notice is passed
Comment 5 Jonathan Druart 2017-03-31 15:15:13 UTC
Created attachment 61755 [details] [review]
Bug 18349: $borrower is a hashref

I guess this code was never called before...
Comment 6 Jonathan Druart 2017-03-31 15:16:33 UTC
(In reply to Nick Clemens from comment #4)

Sorry, I assumed that the existing code was working! Silly me...
Comment 7 Jonathan Druart 2017-03-31 16:26:39 UTC
Created attachment 61756 [details] [review]
Bug 18349: If a confirmation is required, consider as blocker

Only RENEW_ISSUE should not be considered as blocker, others should.
Note that this code is not robust and the whole script as well as the
return of CanBookBeIssued should be rewrittent completely.
We need a small patch to easy backport to stable releases
Comment 8 Jonathan Druart 2017-03-31 16:26:44 UTC
Created attachment 61757 [details] [review]
Bug 18349: Remove useless info in alert message

Empty tags should be removed, and message id
Note that not all error codes are covered here.
Comment 9 Nick Clemens 2017-04-05 16:43:17 UTC
Created attachment 61908 [details] [review]
Bug 18349: SCO - Do not trust the confirmed flag

The "confirmed" flag is used to know if that user confirmed a situation
that needs a confirmation.
But if the issue/renew is impossible the CanBookBeIssued and the
'impossible flags' should be checked.

Otherwise a patron can checkout and renew bypassing the circulation
rules (Understand 'no limit' here...)

Test plan:
Want to renew?
Checkin $barcode, then
/cgi-bin/koha/sco/sco-main.pl?patronid=$cardnumber&barcode=$barcode&confirmed=1&op=checkout

Want to bypass the checkin?
Same url...

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2017-04-05 16:43:22 UTC
Created attachment 61909 [details] [review]
Bug 18349: $borrower is a hashref

I guess this code was never called before...

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens 2017-04-05 16:43:27 UTC
Created attachment 61910 [details] [review]
Bug 18349: If a confirmation is required, consider as blocker

Only RENEW_ISSUE should not be considered as blocker, others should.
Note that this code is not robust and the whole script as well as the
return of CanBookBeIssued should be rewrittent completely.
We need a small patch to easy backport to stable releases

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2017-04-05 16:43:31 UTC
Created attachment 61911 [details] [review]
Bug 18349: Remove useless info in alert message

Empty tags should be removed, and message id
Note that not all error codes are covered here.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Marcel de Rooy 2017-04-07 07:46:42 UTC
QA: Looking here now
Comment 14 Marcel de Rooy 2017-04-07 08:46:45 UTC
Created attachment 61959 [details] [review]
Bug 18349: [QA Follow-up] Add four missing error messages

The error codes come from CanBookBeIssued.
The warnings speak for themselves. Note that the GNA message is similar
to the one used in opac-user.tt (gonenoaddress is translated to the patron
as contact information not up-to-date).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Kyle M Hall 2017-04-13 12:15:01 UTC
Pushed to master for 17.05, thanks Jonathan, Marcel!
Comment 16 Katrin Fischer 2017-04-16 11:32:37 UTC
These patches have been pushed to 16.11.x and will be in 16.11.07.
Comment 17 Julian Maurice 2017-04-21 10:57:08 UTC
Pushed to 3.22.x for 3.22.20
Comment 18 Mason James 2017-05-03 04:10:05 UTC
Pushed to 16.05.x, for 16.05.12 release
Comment 19 David Cook 2020-08-25 05:56:01 UTC
The downside of this code is that it traps the user in a loop.

They keep pressing "Renew item" in the "Please confirm the checkout" box, and it just loops back to the same screen prompting them to do the same thing.
Comment 20 David Cook 2020-08-25 05:57:06 UTC
This patch blocks the "OverduesBlockRenewing" system preference. That may or may not be intentional.

But in any case we should alert the user that they're forbidden from doing what they're trying to do... or not show them the option at all... it's not clear what Koha is trying to do here.
Comment 21 Jonathan Druart 2020-08-25 13:02:44 UTC
David, if you think there is a bug can you open a new bug report with a step-by-step plan to recreate the problem?
Comment 22 David Cook 2020-08-26 01:08:20 UTC
(In reply to Jonathan Druart from comment #21)
> David, if you think there is a bug can you open a new bug report with a
> step-by-step plan to recreate the problem?

Well, there is the issue of the looping bug, but then there's the issue of whether or not this patch made 100% sense in the first place, so good to alert the CC list to it. 

But... yeah I'll be looking into this more soon.
Comment 23 David Cook 2020-08-26 06:01:55 UTC
(In reply to Jonathan Druart from comment #7)
> Note that this code is not robust and the whole script as well as the
> return of CanBookBeIssued should be rewrittent completely.

Agreed. The sco-main.pl script is terrible. 

Trying to patch it is a challenge. 

See Bug 26301...
Comment 24 David Cook 2020-08-26 06:57:08 UTC
(In reply to David Cook from comment #23)
> (In reply to Jonathan Druart from comment #7)
> > Note that this code is not robust and the whole script as well as the
> > return of CanBookBeIssued should be rewrittent completely.
> 
> Agreed. The sco-main.pl script is terrible. 
> 
> Trying to patch it is a challenge. 
> 
> See Bug 26301...

Ok now I can't reproduce this issue in master. I thought I could, but maybe it's just limited to older versions of 19.11...
Comment 25 David Cook 2020-08-26 07:01:16 UTC
(In reply to David Cook from comment #24)
> Ok now I can't reproduce this issue in master. I thought I could, but maybe
> it's just limited to older versions of 19.11...

Ah no it is definitely a problem when OverduesBlockCirc is set to "Ask for confirmation". Patching on Bug 26301 after all...