Bug 33176

Summary: Improve enforcing of RequirePaymentType
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: PatronsAssignee: Lucas Gass (lukeg) <lucas>
Status: RESOLVED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: normal    
Priority: P5 - low CC: david, gmcharlt, jonathan.druart, kyle.m.hall
Version: Main   
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:
23.11.00,23.05.02,22.11.08
Circulation function:
Bug Depends on:    
Bug Blocks: 34620    
Attachments: Bug 33176: Handle RequirePaymentType
Bug 33176: Enforce RequirePaymentType with API
Bug 33176: Handle RequirePaymentType
Bug 33176: Enforce RequirePaymentType with API
Bug 33176: Enforce bad values
Bug 33176: Handle RequirePaymentType
Bug 33176: Enforce RequirePaymentType with API
Bug 33176: Enforce bad values
Bug 33176: Handle RequirePaymentType
Bug 33176: Enforce RequirePaymentType with API
Bug 33176: Enforce bad values

Description Lucas Gass (lukeg) 2023-03-08 21:30:24 UTC
We have notices, with RequirePaymentType on, that sometimes payments still go through without a payment type. 

RequirePaymentType adds the required attribute to the template but this can be circumnavigated by using your browser's dev tools. We should add a check in the script to make sure there is a payment type and if not, prevent the payment.
Comment 1 Katrin Fischer 2023-03-09 18:51:47 UTC
Maybe also in the module? Thinking of the API.
Comment 2 Lucas Gass (lukeg) 2023-03-09 19:46:47 UTC
(In reply to Katrin Fischer from comment #1)
> Maybe also in the module? Thinking of the API.

Yes, good point.
Comment 3 Lucas Gass (lukeg) 2023-04-13 17:34:14 UTC
Created attachment 149617 [details] [review]
Bug 33176: Handle RequirePaymentType

Test plan:

1. Turn on RequirePaymentType
2. Create a manual invocie on a patron account
3. Go to pay it, 'Payment type:' is marked as required.
4. In the inscept the select input ( #payment_type ) with your browser's dev tools. Removed the required attribute.
5. You are able to make the payment without a payment type.
6. Apply patch and restart_all
7. Try 4-5 again. This time you should get a 500 error and the payment should not go through.
8. Turn RequirePaymentType off. Try a payment with a payment type, you shoud be successful.
9. Make sure tests will pass:

prove -v t/db_dependent/Koha/Account.t
Comment 4 Lucas Gass (lukeg) 2023-04-13 17:53:28 UTC
Created attachment 149619 [details] [review]
Bug 33176: Enforce RequirePaymentType with API
Comment 5 David Nind 2023-04-26 18:14:39 UTC
Hi Lucas.

I get an error trace after applying the patch and repeating the steps. The payment is recorded. The start of the error trace:

Exception 'Koha::Exceptions::Account::PaymentTypeRequired' thrown 'Account transaction requires a payment type'
 at /usr/share/perl5/Exception/Class/Base.pm line 88

    in Exception::Class::Base::throw at /usr/share/perl5/Exception/Class/Base.pm line 88

       85: 
       86:     $proto->rethrow if ref $proto;
       87: 
       88:     die $proto->new(@_);
       89: }
       90: 
       91: sub rethrow {

    Show function arguments
    in Koha::Account::pay at /kohadevbox/koha/Koha/Account.pm line 105
....

David

Testing notes (using KTD):

1. Editing the page HTML using Firefox:
   - Right click on the payment type label
   - Select 'Inspect'
   - In the web development tools window that opens, select <select name="payment_type" id="payment_type" class="required" required="required">, right click, and then select 'Edit HTML'
   - Remove: class="required" required="required"
   - Click on the Submit button in the page
Comment 6 Lucas Gass (lukeg) 2023-04-26 18:18:47 UTC
(In reply to David Nind from comment #5)
> Hi Lucas.
> 
> I get an error trace after applying the patch and repeating the steps. The
> payment is recorded. The start of the error trace:
> 
> Exception 'Koha::Exceptions::Account::PaymentTypeRequired' thrown 'Account
> transaction requires a payment type'
>  at /usr/share/perl5/Exception/Class/Base.pm line 88
> 
>     in Exception::Class::Base::throw at
> /usr/share/perl5/Exception/Class/Base.pm line 88
> 
>        85: 
>        86:     $proto->rethrow if ref $proto;
>        87: 
>        88:     die $proto->new(@_);
>        89: }
>        90: 
>        91: sub rethrow {
> 
>     Show function arguments
>     in Koha::Account::pay at /kohadevbox/koha/Koha/Account.pm line 105
> ....
> 
> David
> 
> Testing notes (using KTD):
> 
> 1. Editing the page HTML using Firefox:
>    - Right click on the payment type label
>    - Select 'Inspect'
>    - In the web development tools window that opens, select <select
> name="payment_type" id="payment_type" class="required" required="required">,
> right click, and then select 'Edit HTML'
>    - Remove: class="required" required="required"
>    - Click on the Submit button in the page

David, if this is happening after you remove class="required" required="required" it is the desired behavior.
Comment 7 David Nind 2023-04-26 19:01:47 UTC
Created attachment 150254 [details] [review]
Bug 33176: Handle RequirePaymentType

Test plan:

1. Turn on RequirePaymentType
2. Create a manual invocie on a patron account
3. Go to pay it, 'Payment type:' is marked as required.
4. In the inscept the select input ( #payment_type ) with your browser's dev tools. Removed the required attribute.
5. You are able to make the payment without a payment type.
6. Apply patch and restart_all
7. Try 4-5 again. This time you should get a 500 error and the payment should not go through.
8. Turn RequirePaymentType off. Try a payment with a payment type, you shoud be successful.
9. Make sure tests will pass:

prove -v t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2023-04-26 19:01:49 UTC
Created attachment 150255 [details] [review]
Bug 33176: Enforce RequirePaymentType with API

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2023-04-26 19:18:35 UTC
Updated testing notes (using KTD):

1. For step 4, editing the page HTML using Firefox:
   - Right click on the payment type label
   - Select 'Inspect'
   - In the web development tools window that opens, select <select name="payment_type" id="payment_type" class="required" required="required">, right click, and then select 'Edit HTML'
   - Remove: class="required" required="required"
   - Click on the Submit button in the page
   - Note: If you just remove 'required="required"', a standard form error message is displayed saying this is required

2. For step 7, when editing the HTML just remove: required="required"
   - If you remove 'class="required" required="required"', you will get an error trace and the payment will be recorded (as expected - see comment #6)
Comment 10 Jonathan Druart 2023-05-03 09:44:25 UTC
If you want to prevent manual edition of the DOM to get empty value, then you should also prevent bad value. It's possible to get payment_type=FOO.
Comment 11 Lucas Gass (lukeg) 2023-05-03 21:22:33 UTC
(In reply to Jonathan Druart from comment #10)
> If you want to prevent manual edition of the DOM to get empty value, then
> you should also prevent bad value. It's possible to get payment_type=FOO.

Good point.
Comment 12 Lucas Gass (lukeg) 2023-06-07 18:43:55 UTC
Created attachment 152131 [details] [review]
Bug 33176: Enforce bad values

1. Turn on RequirePaymentType
2. Create a manual invocie on a patron account
3. Go to pay it, 'Payment type:' is marked as required.
4. In the inscept the select input ( #payment_type ) with your browser's dev tools. Removed the required attribute.
5. You are able to make the payment without a payment type.
6. Apply patch and restart_all
7. Try 4-5 again. This time you should get a 500 error and the payment should not go through.
8. Try a paymnet again this time manipulate the DOM and change the value of 'CASH' to something else like 'SOMETHINGELSE'.
9. Try to submit the payment and again you will get a 500 error. The payment should not go through.
10. Turn RequirePaymentType off. Try a payment with a payment type, you shoud be successful.
11. Make sure tests will pass
Comment 13 Sam Lau 2023-06-08 14:48:58 UTC
Created attachment 152203 [details] [review]
Bug 33176: Handle RequirePaymentType

Test plan:

1. Turn on RequirePaymentType
2. Create a manual invocie on a patron account
3. Go to pay it, 'Payment type:' is marked as required.
4. In the inscept the select input ( #payment_type ) with your browser's dev tools. Removed the required attribute.
5. You are able to make the payment without a payment type.
6. Apply patch and restart_all
7. Try 4-5 again. This time you should get a 500 error and the payment should not go through.
8. Turn RequirePaymentType off. Try a payment with a payment type, you shoud be successful.
9. Make sure tests will pass:

prove -v t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 14 Sam Lau 2023-06-08 14:49:01 UTC
Created attachment 152204 [details] [review]
Bug 33176: Enforce RequirePaymentType with API

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 15 Sam Lau 2023-06-08 14:49:03 UTC
Created attachment 152205 [details] [review]
Bug 33176: Enforce bad values

1. Turn on RequirePaymentType
2. Create a manual invocie on a patron account
3. Go to pay it, 'Payment type:' is marked as required.
4. In the inscept the select input ( #payment_type ) with your browser's dev tools. Removed the required attribute.
5. You are able to make the payment without a payment type.
6. Apply patch and restart_all
7. Try 4-5 again. This time you should get a 500 error and the payment should not go through.
8. Try a paymnet again this time manipulate the DOM and change the value of 'CASH' to something else like 'SOMETHINGELSE'.
9. Try to submit the payment and again you will get a 500 error. The payment should not go through.
10. Turn RequirePaymentType off. Try a payment with a payment type, you shoud be successful.
11. Make sure tests will pass

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 16 Katrin Fischer 2023-06-10 19:22:02 UTC
Created attachment 152271 [details] [review]
Bug 33176: Handle RequirePaymentType

Test plan:

1. Turn on RequirePaymentType
2. Create a manual invocie on a patron account
3. Go to pay it, 'Payment type:' is marked as required.
4. In the inscept the select input ( #payment_type ) with your browser's dev tools. Removed the required attribute.
5. You are able to make the payment without a payment type.
6. Apply patch and restart_all
7. Try 4-5 again. This time you should get a 500 error and the payment should not go through.
8. Turn RequirePaymentType off. Try a payment with a payment type, you shoud be successful.
9. Make sure tests will pass:

prove -v t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2023-06-10 19:22:05 UTC
Created attachment 152272 [details] [review]
Bug 33176: Enforce RequirePaymentType with API

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2023-06-10 19:22:09 UTC
Created attachment 152273 [details] [review]
Bug 33176: Enforce bad values

1. Turn on RequirePaymentType
2. Create a manual invocie on a patron account
3. Go to pay it, 'Payment type:' is marked as required.
4. In the inscept the select input ( #payment_type ) with your browser's dev tools. Removed the required attribute.
5. You are able to make the payment without a payment type.
6. Apply patch and restart_all
7. Try 4-5 again. This time you should get a 500 error and the payment should not go through.
8. Try a paymnet again this time manipulate the DOM and change the value of 'CASH' to something else like 'SOMETHINGELSE'.
9. Try to submit the payment and again you will get a 500 error. The payment should not go through.
10. Turn RequirePaymentType off. Try a payment with a payment type, you shoud be successful.
11. Make sure tests will pass

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Tomás Cohen Arazi (tcohen) 2023-06-12 14:51:44 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 20 Martin Renvoize (ashimema) 2023-07-17 07:22:24 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 21 Pedro Amorim 2023-07-18 10:50:08 UTC
Nice work everyone!

Pushed to 22.11.x for next release