Bug 15823 - Can still access patron discharge slip without having the syspref on - Permissions breach
Summary: Can still access patron discharge slip without having the syspref on - Permis...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on: 8007
Blocks: 16670
  Show dependency treegraph
 
Reported: 2016-02-16 01:12 UTC by Aleisha Amohia
Modified: 2017-12-07 22:16 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15823: Can still access patron discharge slip without having the syspref on (1.52 KB, patch)
2016-02-16 01:23 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15823: Can still access patron discharge slip without having the syspref on (1.76 KB, patch)
2016-02-16 04:59 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15823: Can still access patron discharge slip without having the syspref on (3.43 KB, patch)
2016-02-23 04:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 15823: Can still access patron discharge slip without having the syspref on (3.60 KB, patch)
2016-03-15 16:11 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15823: Can still access patron discharge slip without having the syspref on (5.18 KB, patch)
2016-04-25 22:59 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 15823: Can still access patron discharge slip without having the syspref on (5.21 KB, patch)
2016-04-26 04:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15823: Can still access patron discharge slip without having the syspref on (4.99 KB, patch)
2016-04-27 00:28 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 15823: Can still access patron discharge slip without having the syspref on (5.15 KB, patch)
2016-05-02 23:41 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 15823: Can still access patron discharge slip without having the syspref on (5.17 KB, patch)
2016-05-03 12:30 UTC, Marc Véron
Details | Diff | Splinter Review
[FOLLOW-UP] Bug 15823: Redirect opac-discharge.pl to 404 page (2.27 KB, patch)
2016-05-05 06:24 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 15823: Can still access patron discharge slip without having the syspref on (5.22 KB, patch)
2016-05-05 22:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15823: Redirect opac-discharge.pl to 404 page (2.32 KB, patch)
2016-05-05 22:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15823: Force scalar context (1.03 KB, patch)
2016-05-24 19:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2016-02-16 01:12:05 UTC
To reproduce:

Ensure that useDischarge syspref is off.
Go to cgi-bin/koha/members/discharge.pl?borrowernumber=X
Confirm that you are still able to access this page and generate discharge slip.
This is a permissions breach
Comment 1 Aleisha Amohia 2016-02-16 01:23:32 UTC Comment hidden (obsolete)
Comment 2 Aleisha Amohia 2016-02-16 02:17:34 UTC
Sponsored-by: Catalyst IT
Comment 3 Mark Tompsett 2016-02-16 04:59:39 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-02-16 09:44:28 UTC
Hi Aleisha,
It's still possible to generate a discharge even if the pref is off.
For instance, you can get /cgi-bin/koha/members/discharge.pl?borrowernumber=42&discharge=1
And the pdf will be generated.
Usually we check the additional permissions and prefs in the pl script. And either redirect to 404 or, more friendly, display an error message.
Comment 5 Aleisha Amohia 2016-02-23 04:08:31 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2016-03-15 16:11:38 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-03-16 09:22:43 UTC
Aleisha, the problem also exists at the opac (opac-discharge.pl).
Could you provide a fix (redirect to 404), in order to fix both interfaces in the same patch set?
Comment 8 Aleisha Amohia 2016-04-25 22:59:49 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2016-04-26 04:41:18 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-04-26 16:18:02 UTC
Comment on attachment 50707 [details] [review]
Bug 15823: Can still access patron discharge slip without having the syspref on

Review of attachment 50707 [details] [review]:
-----------------------------------------------------------------

10/ Yes I see the message, but the discharge has been requested anyway.
You have to redirect to a 404 in case of opac-discharge.pl is called with the op parameter set.

::: members/discharge.pl
@@ +55,5 @@
> +
> +unless ( C4::Context->preference('useDischarge') ) {
> +   print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber&nopermission=1");
> +   exit;
> +} else {

The else is not needed. You are exiting the script in case of the pref is on.
Comment 11 Jonathan Druart 2016-04-26 16:18:50 UTC
(In reply to Jonathan Druart from comment #10)
> The else is not needed. You are exiting the script in case of the pref is on.

In case of the pref is off of course...
Comment 12 Katrin Fischer 2016-04-26 22:09:53 UTC
See comment#10 and comment#11!
Comment 13 Aleisha Amohia 2016-04-27 00:28:01 UTC Comment hidden (obsolete)
Comment 14 Marc Véron 2016-04-29 13:33:17 UTC
Sorry, does not apply on current master.

Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging circ/circulation.pl
CONFLICT (content): Merge conflict in circ/circulation.pl
Failed to merge in the changes.
Comment 15 Aleisha Amohia 2016-05-02 23:41:37 UTC Comment hidden (obsolete)
Comment 16 Marc Véron 2016-05-03 12:30:00 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-05-04 16:45:59 UTC
(In reply to Jonathan Druart from comment #10)
> 10/ Yes I see the message, but the discharge has been requested anyway.
> You have to redirect to a 404 in case of opac-discharge.pl is called with
> the op parameter set.

And what about the redirect at the OPAC?
Switch to signoff when you will submit a follow-up.
Comment 18 Marc Véron 2016-05-05 06:24:54 UTC Comment hidden (obsolete)
Comment 19 Marc Véron 2016-05-05 06:26:00 UTC
Reset to Signed Off (see comment 17)
Comment 20 Katrin Fischer 2016-05-05 22:26:51 UTC
Created attachment 51268 [details] [review]
[PASSED QA] Bug 15823: Can still access patron discharge slip without having the syspref on

EDIT: Fix for OPAC side
EDIT: Comment 10
EDIT: Merge conflicts

To test:
1) Ensure syspref useDischarge is disabled
2) Go to /cgi-bin/koha/members/discharge.pl?borrowernumber=X&discharge=1
3) Validate that you are still able to generate a discharge slip for this patron
4) Apply patch and refresh page
5) Confirm that you are redirected to the circulation.pl page for the user and that an error message is there.
OPAC SIDE
6) Go to the OPAC
7) Go to /cgi-bin/koha/opac-discharge.pl
8) Confirm you get a message saying discharges are disabled
9) Go to /cgi-bin/koha/opac-discharge.pl?op=request
10) Confirm you see same message

Sponsored-by: Catalyst IT

Followed test plan, works as expected (both staff client and OPAC).
Re-tested, works OK.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2016-05-05 22:27:37 UTC
Created attachment 51269 [details] [review]
[PASSED QA] Bug 15823: Redirect opac-discharge.pl to 404 page

See comment #17: Redirect to 404 in opac-discharge.pl and remove
message in template because with the redirect it will never be
reached.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Brendan Gallagher 2016-05-06 04:21:39 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!
Comment 23 Julian Maurice 2016-05-16 07:23:57 UTC
Patches pushed to 3.22.x, will be in 3.22.7
Comment 24 Jonathan Druart 2016-05-24 19:46:54 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2016-05-24 19:47:48 UTC
@RM & Rmaint, please push the follow-up.
Comment 26 Jacek Ablewicz 2016-06-06 09:11:56 UTC
(In reply to Jonathan Druart from comment #25)
> @RM & Rmaint, please push the follow-up.

Looks like 3.22.7 and 16.05.0 got released in the meantime, without the last follow-up. There are 2 (now 3) branches affected by this issue; for 3.22.x branch, there is a patch in Bug 16646. Because Bug 15823 and Bug 9393 got pushed into master & 3.22.7 in different order, symptoms and severity of this issue are different in both branches.

Not sure what would be better: to have two separate bug reports, for 3.22 and master (to be backported for 16.05), or a single report for master only (to be backported for 3.22 and 16.05)?
Comment 27 Jonathan Druart 2016-06-06 12:22:38 UTC
Comment on attachment 51750 [details] [review]
Bug 15823: Force scalar context

Moved to bug 16670.