Bug 26023 - Incorrect permissions handling for cashup actions on the library level registers summary page
Summary: Incorrect permissions handling for cashup actions on the library level regist...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 26017
Blocks: 13985
  Show dependency treegraph
 
Reported: 2020-07-20 08:50 UTC by Martin Renvoize
Modified: 2020-09-10 03:55 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04


Attachments
Bug 26023: Properly secure the cashup action for libraries (5.63 KB, patch)
2020-07-20 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26023: Properly secure the cashup action for libraries (5.83 KB, patch)
2020-07-20 09:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26023: Properly secure the cashup action for libraries (5.83 KB, patch)
2020-08-04 14:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26023: Properly secure the cashup and refund actions (5.45 KB, patch)
2020-08-04 14:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26023: Properly secure the cashup action for libraries (5.90 KB, patch)
2020-08-13 10:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26023: Properly secure the cashup and refund actions (5.49 KB, patch)
2020-08-13 10:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26023: Properly secure the cashup action for libraries (5.95 KB, patch)
2020-08-20 23:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26023: Properly secure the cashup and refund actions (5.55 KB, patch)
2020-08-20 23:10 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-07-20 08:50:32 UTC
The cashup action is available via the registers page, which summarises a libraries takings for all registers and is available to users with the 'refund' OR 'cashup' permission.  Users without the 'cashup' permission should not be able to take the cashup action.
Comment 1 Martin Renvoize 2020-07-20 08:51:22 UTC
Created attachment 107081 [details] [review]
Bug 26023: Properly secure the cashup action for libraries

The libraries summary page for cash management is available for users
wit the 'anonymous_refund' permission to allow them to navigate to
alternate cash registers and search for the prior transaction to refund.

However, currently the cashup option appears, and is not blocked at the
server, for all user who may access the page. It should be blocked for
those users without the 'cashup' permission.
Comment 2 Martin Renvoize 2020-07-20 08:56:11 UTC
Test Plan
1/ Setup some cash registers
2/ Login as a user with just the 'refund' permission
3/ Note that you can still access the 'Cashup registers' page from either 'Tools' or the left menu that appears on Point of Sale pages.
4/ Note that you do not see the 'Cashup' actions available
5/ Login as a user with the 'cashup' permission
6/ You should still be able to access the above page
7/ You should not see the cashup actions
Bonus points
8/ Without the 'cashup' permission attempt to 'POST' a cashup action (copy a the URL for a cashup action that appears when you were logged in as a user with correction permissions, and paste it into the address bar once you are logged in as a user without the permission
9/ You should be displayed with the registers page with an error message appearing to state that the cashup action was not allowed to take place due to permissions deficiencies.
Comment 3 Martin Renvoize 2020-07-20 09:00:29 UTC
Created attachment 107082 [details] [review]
Bug 26023: Properly secure the cashup action for libraries

The libraries summary page for cash management is available for users
wit the 'anonymous_refund' permission to allow them to navigate to
alternate cash registers and search for the prior transaction to refund.

However, currently the cashup option appears, and is not blocked at the
server, for all user who may access the page. It should be blocked for
those users without the 'cashup' permission.
Comment 4 Martin Renvoize 2020-08-04 14:09:26 UTC
Created attachment 107767 [details] [review]
Bug 26023: Properly secure the cashup action for libraries

The libraries summary page for cash management is available for users
wit the 'anonymous_refund' permission to allow them to navigate to
alternate cash registers and search for the prior transaction to refund.

However, currently the cashup option appears, and is not blocked at the
server, for all user who may access the page. It should be blocked for
those users without the 'cashup' permission.
Comment 5 Martin Renvoize 2020-08-04 14:09:29 UTC
Created attachment 107768 [details] [review]
Bug 26023: Properly secure the cashup and refund actions

The cash register summary page for cash management is available for users
with the 'anonymous_refund' or 'cashup' permission and the actions available
are appropriately displayed.

However, the actions are not yet correctly tested for at the server and
so a user may force submit to accomplish the action.
Comment 6 Nick Clemens 2020-08-13 10:02:54 UTC
Created attachment 108171 [details] [review]
Bug 26023: Properly secure the cashup action for libraries

The libraries summary page for cash management is available for users
wit the 'anonymous_refund' permission to allow them to navigate to
alternate cash registers and search for the prior transaction to refund.

However, currently the cashup option appears, and is not blocked at the
server, for all user who may access the page. It should be blocked for
those users without the 'cashup' permission.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens 2020-08-13 10:02:57 UTC
Created attachment 108172 [details] [review]
Bug 26023: Properly secure the cashup and refund actions

The cash register summary page for cash management is available for users
with the 'anonymous_refund' or 'cashup' permission and the actions available
are appropriately displayed.

However, the actions are not yet correctly tested for at the server and
so a user may force submit to accomplish the action.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Katrin Fischer 2020-08-14 11:33:05 UTC
(In reply to Martin Renvoize from comment #2)
> Test Plan
> 1/ Setup some cash registers
> 2/ Login as a user with just the 'refund' permission
> 3/ Note that you can still access the 'Cashup registers' page from either
> 'Tools' or the left menu that appears on Point of Sale pages.
> 4/ Note that you do not see the 'Cashup' actions available
> 5/ Login as a user with the 'cashup' permission
> 6/ You should still be able to access the above page
> 7/ You should not see the cashup actions
> Bonus points
> 8/ Without the 'cashup' permission attempt to 'POST' a cashup action (copy a
> the URL for a cashup action that appears when you were logged in as a user
> with correction permissions, and paste it into the address bar once you are
> logged in as a user without the permission
> 9/ You should be displayed with the registers page with an error message
> appearing to state that the cashup action was not allowed to take place due
> to permissions deficiencies.

I have a bit of trouble following the test plan here:

1-4)
My user has catalog and refund permissions.
With the patch applied, this prevents me from accessing:
http://localhost:8081/cgi-bin/koha/pos/registers.pl

5-7)
If the user has cashup permission, should they not be able to see the cashup actions? (typo)

So I cannot check for the actions not showing.
Comment 9 Martin Renvoize 2020-08-20 16:10:12 UTC
(In reply to Katrin Fischer from comment #8)
> (In reply to Martin Renvoize from comment #2)
> > Test Plan
> > 1/ Setup some cash registers
> > 2/ Login as a user with just the 'refund' permission

Oops.. this should have been 'anonymous_refund'.. i.e. the subpermission in cash_management rather than the subpermission of accounts.. my apologies.

> 
> I have a bit of trouble following the test plan here:
> 
> 1-4)
> My user has catalog and refund permissions.
> With the patch applied, this prevents me from accessing:
> http://localhost:8081/cgi-bin/koha/pos/registers.pl

See above: However I do wonder if at some point the availability of this page may need/want to fall outside of the cash_management permissions or have it's own permission associated with it (one for another bug however, once I understand the possible use case)
Comment 10 Katrin Fischer 2020-08-20 23:09:36 UTC
I think we might still want to some refinements, for example having "cash register" on admin and on tools, but not being the same thing is a little confusing. But that's out of scope here and this improves things.
Comment 11 Katrin Fischer 2020-08-20 23:10:12 UTC
Created attachment 108772 [details] [review]
Bug 26023: Properly secure the cashup action for libraries

The libraries summary page for cash management is available for users
wit the 'anonymous_refund' permission to allow them to navigate to
alternate cash registers and search for the prior transaction to refund.

However, currently the cashup option appears, and is not blocked at the
server, for all user who may access the page. It should be blocked for
those users without the 'cashup' permission.

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2020-08-20 23:10:17 UTC
Created attachment 108773 [details] [review]
Bug 26023: Properly secure the cashup and refund actions

The cash register summary page for cash management is available for users
with the 'anonymous_refund' or 'cashup' permission and the actions available
are appropriately displayed.

However, the actions are not yet correctly tested for at the server and
so a user may force submit to accomplish the action.

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Jonathan Druart 2020-08-24 08:22:23 UTC
Martin, I think we should use "blocking errors" way for that kind of error.

At the top of the script we can have 

output_and_exit( $input, $cookie, $template, 'insufficient_permission' )
  if $op eq 'cashup' && not $logged_in_user->has_permission( { cash_management => 'cashup' } )

Not enough to block the push however.
Comment 14 Jonathan Druart 2020-08-24 10:33:27 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 15 Aleisha Amohia 2020-09-10 03:55:55 UTC
backported to 20.05.x for 20.05.04 (comment for Lucas)

missing dependencies, not backported to 19.11.x