Bug 36190

Summary: op param for stateful requests must start with 'cud-'
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens (kidclamp) <nick>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: david, dcook, fridolin.somers, m.de.rooy, nick
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36305
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00
Bug Depends on: 34478    
Bug Blocks: 36192    
Attachments: Bug 36190: Adjust the test
Bug 36190: To not assume op is invalid if it is a TT variable
Bug 36190: To not assume op is invalid if it is a TT variable
Bug 36190: (QA follow-up) Tidy
Bug 36190: Fix problems found by test
Bug 36190: Adjust the test
Bug 36190: To not assume op is invalid if it is a TT variable
Bug 36190: (QA follow-up) Tidy
Bug 36190: Fix problems found by test
Bug 36190: Adjust the test
Bug 36190: To not assume op is invalid if it is a TT variable
Bug 36190: (QA follow-up) Tidy
Bug 36190: Fix problems found by test

Description Jonathan Druart 2024-02-28 13:05:53 UTC
This is enforced in Koha::Middleware::CSRF but the tests is not catching incorrect occurrences.
Comment 1 Jonathan Druart 2024-02-28 13:06:50 UTC
Created attachment 162530 [details] [review]
Bug 36190: Adjust the test
Comment 2 Jonathan Druart 2024-02-28 13:11:16 UTC
Created attachment 162531 [details] [review]
Bug 36190: To not assume op is invalid if it is a TT variable
Comment 3 Nick Clemens (kidclamp) 2024-03-13 12:40:17 UTC
Created attachment 163110 [details] [review]
Bug 36190: To not assume op is invalid if it is a TT variable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens (kidclamp) 2024-03-13 12:40:19 UTC
Created attachment 163111 [details] [review]
Bug 36190: (QA follow-up) Tidy
Comment 5 Nick Clemens (kidclamp) 2024-03-13 12:40:21 UTC
Created attachment 163112 [details] [review]
Bug 36190: Fix problems found by test

opac-shelves           - forms were stateless - changed to GET
sco-main               - forms stateless - changed to GET
                         ** untested because sco + auth is broken
smart-rules.tt         - JS form template - added placeholder 'cud-' op
ill-requests           - added cud- tp ops
                         ** tested comments, but not checkout, simple changes
boraccount             - removed repeated op - updated script
patroncards/edit-batch - add placeholder 'cud-' op
patroncards/manage     - add placheholder 'cud-' op
elasticsearch/mappings - separate forms - this could use a style follow-up, but makes more sense separate I think
reports/dictionary      - stateless - changed to GET
guided_reports_start    - stateless - changed to GET
suggestion/suggestion   - add placeholder 'cud-' op
inventory               - filed bug 36305, needs more handling
Comment 6 David Nind 2024-03-13 22:26:25 UTC
I'm getting a sha1/fake ancestor error when trying to apply the patch:

Applying: Bug 36190: To not assume op is invalid if it is a TT variable
error: sha1 information is lacking or useless (xt/find-missing-op-in-forms.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 36190: To not assume op is invalid if it is a TT variable
...

Testing notes - how I planned to test:
1. Run the tests before the patch is applied: prove xt/find-missing-op-in-forms.t
2. Apply the first patch and run the tests: tests should fail
3. Apply the rest of the patches and run the tests: tests should pass!
Comment 7 Nick Clemens (kidclamp) 2024-03-14 10:52:47 UTC
(In reply to David Nind from comment #6)
> I'm getting a sha1/fake ancestor error when trying to apply the patch:

Fixed, I obsoleted a patch without reattaching
> 
> Testing notes - how I planned to test:
> 1. Run the tests before the patch is applied: prove
> xt/find-missing-op-in-forms.t
> 2. Apply the first patch and run the tests: tests should fail
> 3. Apply the rest of the patches and run the tests: tests should pass!

Correct, except the test will have 1 remaining failure, should be handled on bug 36305
Comment 8 David Nind 2024-03-14 23:26:54 UTC
Created attachment 163149 [details] [review]
Bug 36190: Adjust the test

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2024-03-14 23:26:56 UTC
Created attachment 163150 [details] [review]
Bug 36190: To not assume op is invalid if it is a TT variable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2024-03-14 23:26:59 UTC
Created attachment 163151 [details] [review]
Bug 36190: (QA follow-up) Tidy

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2024-03-14 23:27:02 UTC
Created attachment 163152 [details] [review]
Bug 36190: Fix problems found by test

opac-shelves           - forms were stateless - changed to GET
sco-main               - forms stateless - changed to GET
                         ** untested because sco + auth is broken
smart-rules.tt         - JS form template - added placeholder 'cud-' op
ill-requests           - added cud- tp ops
                         ** tested comments, but not checkout, simple changes
boraccount             - removed repeated op - updated script
patroncards/edit-batch - add placeholder 'cud-' op
patroncards/manage     - add placheholder 'cud-' op
elasticsearch/mappings - separate forms - this could use a style follow-up, but makes more sense separate I think
reports/dictionary      - stateless - changed to GET
guided_reports_start    - stateless - changed to GET
suggestion/suggestion   - add placeholder 'cud-' op
inventory               - filed bug 36305, needs more handling

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2024-03-14 23:35:13 UTC
Thanks Nick!

Testing notes (using KTD):

1. Run the tests before the patch is applied - tests should pass: prove -v xt/find-missing-op-in-forms.t
2. Apply the first three patches (using interactive mode) and run the tests: tests should fail with 12 forms missing the required information
3. Apply the remaining patch and run the tests again: tests still fail,[1] but this time there is only 1 form missing the required information (addressed in bug 36305)

[1] After all patches are applied, only 1 form is missing the required information

prove -v xt/find-missing-op-in-forms.t
xt/find-missing-op-in-forms.t .. 
1..1
not ok 1 - The <form> in the following files are missing it's corresponding op parameter, or op does not start with 'cud-' (see bug 34478)

#   Failed test 'The <form> in the following files are missing it's corresponding op parameter, or op does not start with 'cud-' (see bug 34478)'
#   at xt/find-missing-op-in-forms.t line 43.
#          got: '1'
#     expected: '0'
# $VAR1 = 'koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt:71,248';
# Looks like you failed 1 test of 1.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

Test Summary Report
-------------------
xt/find-missing-op-in-forms.t (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=1,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.15 cusr  0.01 csys =  0.17 CPU)
Result: FAIL
Comment 13 Marcel de Rooy 2024-03-15 07:55:31 UTC
Created attachment 163162 [details] [review]
Bug 36190: Adjust the test

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2024-03-15 07:55:33 UTC
Created attachment 163163 [details] [review]
Bug 36190: To not assume op is invalid if it is a TT variable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2024-03-15 07:55:36 UTC
Created attachment 163164 [details] [review]
Bug 36190: (QA follow-up) Tidy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2024-03-15 07:55:38 UTC
Created attachment 163165 [details] [review]
Bug 36190: Fix problems found by test

opac-shelves           - forms were stateless - changed to GET
sco-main               - forms stateless - changed to GET
                         ** untested because sco + auth is broken
smart-rules.tt         - JS form template - added placeholder 'cud-' op
ill-requests           - added cud- tp ops
                         ** tested comments, but not checkout, simple changes
boraccount             - removed repeated op - updated script
patroncards/edit-batch - add placeholder 'cud-' op
patroncards/manage     - add placheholder 'cud-' op
elasticsearch/mappings - separate forms - this could use a style follow-up, but makes more sense separate I think
reports/dictionary      - stateless - changed to GET
guided_reports_start    - stateless - changed to GET
suggestion/suggestion   - add placeholder 'cud-' op
inventory               - filed bug 36305, needs more handling

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Katrin Fischer 2024-03-15 08:40:13 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 18 Fridolin Somers 2024-03-18 09:06:20 UTC
Depends on Bug 34478 not in 23.11.x