Bug 18349 - Possibility to checkout/renew bypassing the circ rules using SCO
Summary: Possibility to checkout/renew bypassing the circ rules using SCO
Status: Pushed to Stable
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:
Whiteboard:
Keywords:
Depends on:
Blocks: 18251
  Show dependency treegraph
 
Reported: 2017-03-29 21:36 UTC by Jonathan Druart
Modified: 2017-08-08 04:28 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


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