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.
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
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>
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¬es= 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
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¬es= => 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
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¬es= => 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>
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
(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
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>
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¬es= => 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>
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>
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
Hm, I tried to reach the URL directly and got a login 'not enough permissions' - can you explain a bit more?
(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!
(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
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>
Created attachment 89827 [details] [review] Bug 22724: Check permissions in the script before displaying template
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
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>
Created attachment 89879 [details] [review] Bug 22724: Check permissions in the script before displaying template Signed-off-by: Liz Rea <wizzyrea@gmail.com>
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>
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.06