Bug 22724

Summary: Staff without writeoff permissions have access to 'Write off selected' button on Pay Fines tab
Product: Koha Reporter: Barton Chittenden <barton>
Component: Fines and feesAssignee: Hayley Pelham <hayleypelham>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: major    
Priority: P5 - low CC: barton, chris, hayleypelham, margaret, martin.renvoize, nick, wizzyrea
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06
Attachments: Bug 22724: Allow only users with correct permissions to 'Write off selected'
Bug 22724: Allow only users with correct permissions to 'Write off selected'
Bug 22724: (follow-up) Block writeoffs when user has wrong permissions
Bug 22724: (follow-up) Block writeoffs when user has wrong permissions
Bug 22724: Allow only users with correct permissions to 'Write off selected'
Bug 22724: (follow-up) Block writeoffs when user has wrong permissions
Bug 22724: (QA follow-up) Fix permission when using the individual writeoff button
Bug 22724: Allow only users with correct permissions to 'Write off selected'
Bug 22724: Check permissions in the script before displaying template
Bug 22724: Allow only users with correct permissions to 'Write off selected'
Bug 22724: Check permissions in the script before displaying template
Bug 22724: Allow only users with correct permissions to 'Write off selected'

Description Barton Chittenden 2019-04-17 15:13:36 UTC
To re-create:

1) Make sure that a staff user has '(writeoff) Write off fines and fees' permission disabled.
2) Bring up a patron with an unpaid fee, go to Fines -> Pay fines
3) Note that 'Write off selected' button is displayed.
4) Select a fee, click 'Write off selected'. Note that staff member has access to 'Write off an amount toward selected fines'.

This is a regression; prior to 18.11, a staff user without '(writeoff) Write off fines and fees' permissions would not have seen the 'Write off selected' button in step 3.
Comment 1 Hayley Pelham 2019-05-08 04:03:42 UTC
Created attachment 89457 [details] [review]
Bug 22724: Allow only users with correct permissions to 'Write off selected'

Due to the addition of a 'Write off selected' button in 18.11, users without
writeoff permissions could mistakenly view and action the 'Write off
selected' option for a patron in Accounting > Make a payment (Fines > Pay fines).
The attached patch corrects this error.

Test plan:
1) Make sure that a staff user has '(writeoff) Write off fines and fees'
permission disabled
2) Bring up a patron with an unpaid fee, go to Accounting -> Make a
payment
3) Note that 'Write off selected' button is displayed.
4) Select a fee, click 'Write off selected'. Note that staff member has
access to 'Write off an amount toward selected fines'
5) Apply the patch
6) Repeat steps 1-2, and note that the button is no longer displayed
7) Bonus points, as a staff user with writeoff permissions enabled check
that all four buttons (Pay amount, Pay selected, Write off all, Write
off selected) are displayed

Sponsored-by: Catalyst IT
Comment 2 Liz Rea 2019-05-08 15:15:22 UTC
Created attachment 89475 [details] [review]
Bug 22724: Allow only users with correct permissions to 'Write off selected'

Due to the addition of a 'Write off selected' button in 18.11, users without
writeoff permissions could mistakenly view and action the 'Write off
selected' option for a patron in Accounting > Make a payment (Fines > Pay fines).
The attached patch corrects this error.

Test plan:
1) Make sure that a staff user has '(writeoff) Write off fines and fees'
permission disabled
2) Bring up a patron with an unpaid fee, go to Accounting -> Make a
payment
3) Note that 'Write off selected' button is displayed.
4) Select a fee, click 'Write off selected'. Note that staff member has
access to 'Write off an amount toward selected fines'
5) Apply the patch
6) Repeat steps 1-2, and note that the button is no longer displayed
7) Bonus points, as a staff user with writeoff permissions enabled check
that all four buttons (Pay amount, Pay selected, Write off all, Write
off selected) are displayed

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 3 Liz Rea 2019-05-08 15:31:33 UTC
Hi Hailey,

Thanks for this patch, it does fix part of the problem, which is the button appearing when it shouldn't, but we can still do the actual write off by going to a URL constructed as follows:

http://localhost:8081/cgi-bin/koha/members/paycollect.pl?borrowernumber=21&type=writeoff&amt=4&selected=1&notes=

Your patch for the button is good, and can stay signed off. A follow up to fix this bad behaviour as well would be good as part of fixing this bug.

Thanks, and good luck,

Liz
Comment 4 Hayley Pelham 2019-05-08 22:23:41 UTC
Created attachment 89482 [details] [review]
Bug 22724: (follow-up) Block writeoffs when user has wrong permissions

This patch is a followup which displays an alert and removes the submit
button when a user who does not have writeoff permissions manages to
make it to the 'Make a payment' tab with a writeoff URL.

Test plan:
1) Login as a staff user with writeoff permissions disabled, ensure you
have sample users, then click
the URL: http://localhost:8081/cgi-bin/koha/members/paycollect.pl?borrowernumber=21&type=writeoff&amt=4&selected=1&notes=
=> Note that you are able to write off the charge with no warning
2) Apply the patch
3) Repeat step 1
=> Note that an alert displays and the button to confirm the writeoff
has been removed
4) Repeat step 1, this time logging in as a staff user with writeoff
permissions enabled
=> Note that the button displays and you can perform the writeoff

Sponsored-by: Catalyst IT
Comment 5 Liz Rea 2019-05-08 22:34:25 UTC
Created attachment 89485 [details] [review]
Bug 22724: (follow-up) Block writeoffs when user has wrong permissions

This patch is a followup which displays an alert and removes the submit
button when a user who does not have writeoff permissions manages to
make it to the 'Make a payment' tab with a writeoff URL.

Test plan:
1) Login as a staff user with writeoff permissions disabled, ensure you
have sample users, then click
the URL: http://localhost:8081/cgi-bin/koha/members/paycollect.pl?borrowernumber=21&type=writeoff&amt=4&selected=1&notes=
=> Note that you are able to write off the charge with no warning
2) Apply the patch
3) Repeat step 1
=> Note that an alert displays and the button to confirm the writeoff
has been removed
4) Repeat step 1, this time logging in as a staff user with writeoff
permissions enabled
=> Note that the button displays and you can perform the writeoff

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 6 Liz Rea 2019-05-08 22:35:27 UTC
This works, I'm going to leave it to people up the chain to say whether they like this implementation, but thank you very much for the quick turnaround, it will be great to get this fixed!

Cheers,
Liz
Comment 7 Hayley Pelham 2019-05-08 22:37:36 UTC
(In reply to Liz Rea from comment #6)
> This works, I'm going to leave it to people up the chain to say whether they
> like this implementation, but thank you very much for the quick turnaround,
> it will be great to get this fixed!
> 
> Cheers,
> Liz

Thanks for signing off Liz! Definitely, I figured I would just get it out so that I can make any changes needed quicker.

Cheers,
Hayley
Comment 8 Katrin Fischer 2019-05-11 13:12:55 UTC
Created attachment 89602 [details] [review]
Bug 22724: Allow only users with correct permissions to 'Write off selected'

Due to the addition of a 'Write off selected' button in 18.11, users without
writeoff permissions could mistakenly view and action the 'Write off
selected' option for a patron in Accounting > Make a payment (Fines > Pay fines).
The attached patch corrects this error.

Test plan:
1) Make sure that a staff user has '(writeoff) Write off fines and fees'
permission disabled
2) Bring up a patron with an unpaid fee, go to Accounting -> Make a
payment
3) Note that 'Write off selected' button is displayed.
4) Select a fee, click 'Write off selected'. Note that staff member has
access to 'Write off an amount toward selected fines'
5) Apply the patch
6) Repeat steps 1-2, and note that the button is no longer displayed
7) Bonus points, as a staff user with writeoff permissions enabled check
that all four buttons (Pay amount, Pay selected, Write off all, Write
off selected) are displayed

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2019-05-11 13:12:59 UTC
Created attachment 89603 [details] [review]
Bug 22724: (follow-up) Block writeoffs when user has wrong permissions

This patch is a followup which displays an alert and removes the submit
button when a user who does not have writeoff permissions manages to
make it to the 'Make a payment' tab with a writeoff URL.

Test plan:
1) Login as a staff user with writeoff permissions disabled, ensure you
have sample users, then click
the URL: http://localhost:8081/cgi-bin/koha/members/paycollect.pl?borrowernumber=21&type=writeoff&amt=4&selected=1&notes=
=> Note that you are able to write off the charge with no warning
2) Apply the patch
3) Repeat step 1
=> Note that an alert displays and the button to confirm the writeoff
has been removed
4) Repeat step 1, this time logging in as a staff user with writeoff
permissions enabled
=> Note that the button displays and you can perform the writeoff

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2019-05-11 13:13:02 UTC
Created attachment 89604 [details] [review]
Bug 22724: (QA follow-up) Fix permission when using the individual writeoff button

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Nick Clemens 2019-05-14 19:28:45 UTC
Hi Hayley,

We need a server side check here for the permissions. With these patches I can inspect the element, add the write-off button to submit, and write off the charge

This is probably true for payments as well. This will prevent 90% of the cases, but we should probably strictly enforce.

You can git grep for haspermission to see some examples
Comment 12 Katrin Fischer 2019-05-14 19:30:46 UTC
Hm, I tried to reach the URL directly and got a login 'not enough permissions' - can you explain a bit more?
Comment 13 Hayley Pelham 2019-05-14 22:38:10 UTC
(In reply to Nick Clemens from comment #11)
> Hi Hayley,
> 
> We need a server side check here for the permissions. With these patches I
> can inspect the element, add the write-off button to submit, and write off
> the charge
> 
> This is probably true for payments as well. This will prevent 90% of the
> cases, but we should probably strictly enforce.
> 
> You can git grep for haspermission to see some examples

The second patch that I added enforced removal of the submit button if the staff user managed to find a way to get to paycollect.tt to confirm the payment/writeoff (either through constructing a url or adding the button somehow). If the user doesn't have the permissions, the button will not be there. Is this button you talked about adding manually to the page?

If this isn't your concern, I will look into the server side check you mentioned

Thanks for looking at it!
Comment 14 Nick Clemens 2019-05-16 10:56:55 UTC
(In reply to Hayley Mapley from comment #13)
> The second patch that I added enforced removal of the submit button if the
> staff user managed to find a way to get to paycollect.tt to confirm the
> payment/writeoff (either through constructing a url or adding the button
> somehow). If the user doesn't have the permissions, the button will not be
> there. Is this button you talked about adding manually to the page?
> 
> If this isn't your concern, I will look into the server side check you
> mentioned
> 
> Thanks for looking at it!

Yup, that was the button I edited back in. I looked again today, we don't need haspermission, we just need to make sure the page requires the writeoff permission whenever it is a write off.  Attaching a patch for testing
Comment 15 Nick Clemens 2019-05-16 10:57:30 UTC
Created attachment 89826 [details] [review]
Bug 22724: Allow only users with correct permissions to 'Write off selected'

Due to the addition of a 'Write off selected' button in 18.11, users without
writeoff permissions could mistakenly view and action the 'Write off
selected' option for a patron in Accounting > Make a payment (Fines > Pay fines).
The attached patch corrects this error.

Test plan:
1) Make sure that a staff user has '(writeoff) Write off fines and fees'
permission disabled
2) Bring up a patron with an unpaid fee, go to Accounting -> Make a
payment
3) Note that 'Write off selected' button is displayed.
4) Select a fee, click 'Write off selected'. Note that staff member has
access to 'Write off an amount toward selected fines'
5) Apply the patch
6) Repeat steps 1-2, and note that the button is no longer displayed
7) Bonus points, as a staff user with writeoff permissions enabled check
that all four buttons (Pay amount, Pay selected, Write off all, Write
off selected) are displayed

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Nick Clemens 2019-05-16 10:57:33 UTC
Created attachment 89827 [details] [review]
Bug 22724: Check permissions in the script before displaying template
Comment 17 Nick Clemens 2019-05-16 10:59:24 UTC
Note if we check in the script, we never display the writeoff template to the user, so we don't need the alerts, or the check to display the writeoff button on paycollect

I renamed two variables for clarity as well

The script could use some work overall, the logical flow is not as clear as it could be.

Let me know if this works for you and makes sense Hayley
Comment 18 Liz Rea 2019-05-16 19:33:12 UTC
Created attachment 89878 [details] [review]
Bug 22724: Allow only users with correct permissions to 'Write off selected'

Due to the addition of a 'Write off selected' button in 18.11, users without
writeoff permissions could mistakenly view and action the 'Write off
selected' option for a patron in Accounting > Make a payment (Fines > Pay fines).
The attached patch corrects this error.

Test plan:
1) Make sure that a staff user has '(writeoff) Write off fines and fees'
permission disabled
2) Bring up a patron with an unpaid fee, go to Accounting -> Make a
payment
3) Note that 'Write off selected' button is displayed.
4) Select a fee, click 'Write off selected'. Note that staff member has
access to 'Write off an amount toward selected fines'
5) Apply the patch
6) Repeat steps 1-2, and note that the button is no longer displayed
7) Bonus points, as a staff user with writeoff permissions enabled check
that all four buttons (Pay amount, Pay selected, Write off all, Write
off selected) are displayed

Sponsored-by: Catalyst IT

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 19 Liz Rea 2019-05-16 19:33:20 UTC
Created attachment 89879 [details] [review]
Bug 22724: Check permissions in the script before displaying template

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 20 Kyle M Hall 2019-05-28 13:53:34 UTC
Created attachment 90153 [details] [review]
Bug 22724: Allow only users with correct permissions to 'Write off selected'

Due to the addition of a 'Write off selected' button in 18.11, users without
writeoff permissions could mistakenly view and action the 'Write off
selected' option for a patron in Accounting > Make a payment (Fines > Pay fines).
The attached patch corrects this error.

Test plan:
1) Make sure that a staff user has '(writeoff) Write off fines and fees'
permission disabled
2) Bring up a patron with an unpaid fee, go to Accounting -> Make a
payment
3) Note that 'Write off selected' button is displayed.
4) Select a fee, click 'Write off selected'. Note that staff member has
access to 'Write off an amount toward selected fines'
5) Apply the patch
6) Repeat steps 1-2, and note that the button is no longer displayed
7) Bonus points, as a staff user with writeoff permissions enabled check
that all four buttons (Pay amount, Pay selected, Write off all, Write
off selected) are displayed

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Nick Clemens 2019-05-30 10:34:36 UTC
Awesome work all!

Pushed to master for 19.05
Comment 22 Martin Renvoize 2019-05-30 20:23:16 UTC
Pushed to 18.11.x for 18.11.06