[Bug 36066] New: REST API: We should not allow deleting open order lines
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Bug ID: 36066 Summary: REST API: We should not allow deleting open order lines Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: m.de.rooy@rijksmuseum.nl CC: tomascohen@gmail.com See also bug 10869 and bug 18360. Until now we focused on not deleting order lines in the interface entirely. So it is quite strange that we allow it without any checks on the REST API. In harmony with 18360 we should check if the status is cancelled. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18360 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|REST API: We should not |REST API: We should only |allow deleting open order |allow deleting cancelled |lines |order lines -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 162017 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162017&action=edit Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I think the change makes sense, but I worry a bit if this could break plugins using delete - there doesn't seem to be a way to cancel an order via the API yet? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Katrin Fischer from comment #2)
I think the change makes sense, but I worry a bit if this could break plugins using delete - there doesn't seem to be a way to cancel an order via the API yet?
No reason to worry. We should not allow plugins to delete in the first place (since Koha did not allow that yet). But I am reading the code and guess that it is just added in line with regular CRUD patterns. It is possible to cancel an order via API using ->update (PUT) and sending a modified orderstatus. Copying Kyle and Martin here: Do you have such plugins in your plugin repos? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #3)
(In reply to Katrin Fischer from comment #2)
No reason to worry. We should not allow plugins to delete in the first place (since Koha did not allow that yet). But I am reading the code and guess that it is just added in line with regular CRUD patterns. It is possible to cancel an order via API using ->update (PUT) and sending a modified orderstatus.
But you should also update the cancellation date! See also bug 10869 comment109. You can delete a whole basket in the interface, but the interface takes care of cancelling etc. Since the API method ->delete does not do that (yet), the proposed check is still fine in its current form imo. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=10869 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162017|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 162276 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162276&action=edit Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Nind from comment #5)
Signed-off-by: David Nind <david@davidnind.com>
Thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #7 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162735 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162735&action=edit Bug 36066: Add tests Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162276|0 |1 is obsolete| | --- Comment #8 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162736 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162736&action=edit Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |victor@tuxayo.net Status|Signed Off |Failed QA CC| |victor@tuxayo.net --- Comment #9 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Hi :) After moving the tests to their own commit, I was able to run them without the implementation. And it turns out the tests pass :o
$t->delete_ok( "//$unauth_userid:$password@/api/v1/acquisitions/orders/" . $order->ordernumber )->status_is(403);
It's likely because of $unauth_userid is used instead of $auth_userid. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #10 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Another thing that made it slip unnoticed is the return code being the same for not having the right permission and trying to delete something not in the right state for deletion. This is totally out of this ticket: Isn't there any code to differential these two things? I'm just asking to know if I should open a ticket or if there is really nothing to do about this and just move on. https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_client_errors - maybe moving permission failure response to 401? nope, 401 is super specific so out of our case: «The response must include a WWW-Authenticate header field containing a challenge applicable to the requested resource» - «403 [...] user not having the necessary permissions for a resource or needing an account of some sort, or attempting a prohibited action (e.g. creating a duplicate record where only one is allowed).» Ok it seems that's how 403 was made, grouping lack of permission and prohibited action by business rules :( -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Victor Grousset/tuxayo from comment #10)
Another thing that made it slip unnoticed is the return code being the same for not having the right permission and trying to delete something not in the right state for deletion.
Good catch!
This is totally out of this ticket: Isn't there any code to differential these two things? I'm just asking to know if I should open a ticket or if there is really nothing to do about this and just move on.
We use 409 (Conflict) for such cases: "Indicates that the request could not be processed because of conflict in the current state of the resource, such as an edit conflict between multiple simultaneous updates." Try ``` cd api/v1/swagger/paths git grep 409 ``` Best regards -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162735|0 |1 is obsolete| | --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 162738 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162738&action=edit Bug 36066: Add tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] As Victor discovered, the test with status new in subtest 'delete' needed the authorised user now. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Without the follow-up patch this should FAIL. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162736|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 162739 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162739&action=edit Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Victor Grousset/tuxayo from comment #9)
It's likely because of $unauth_userid is used instead of $auth_userid.
Thanks for catching that. Fixed now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Victor Grousset/tuxayo from comment #10)
Another thing that made it slip unnoticed is the return code being the same for not having the right permission and trying to delete something not in the right state for deletion.
Yeah we also use 409 in the REST API. Mostly when trying to add a duplicate. In this case I have my doubts if we should go there now. I am just reading it like You are not authorized to delete a non-cancelled order line. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Tomás Cohen Arazi from comment #11)
We use 409 (Conflict) for such cases:
"Indicates that the request could not be processed because of conflict in the current state of the resource, such as an edit conflict between multiple simultaneous updates."
(In reply to Marcel de Rooy from comment #15)
Yeah we also use 409 in the REST API. Mostly when trying to add a duplicate. In this case I have my doubts if we should go there now. I am just reading it like You are not authorized to delete a non-cancelled order line.
This is clearly a case for the 409, as the resource status is conflicting with the requested action. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 162754 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162754&action=edit Bug 36066: (follow-up) Move 403 to 409 when status ne cancelled Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Tomás Cohen Arazi from comment #16)
This is clearly a case for the 409, as the resource status is conflicting with the requested action.
It wasnt so clear to me. But fixed now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #19 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Tomás Cohen Arazi from comment #16)
This is clearly a case for the 409, as the resource status is conflicting with the requested action.
The description of 409 was way to abstract for me to make the connection with our case but ok good, 409 then :D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162738|0 |1 is obsolete| | --- Comment #20 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162790 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162790&action=edit Bug 36066: Add tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] As Victor discovered, the test with status new in subtest 'delete' needed the authorised user now. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Without the follow-up patch this should FAIL. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162739|0 |1 is obsolete| | --- Comment #21 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162791 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162791&action=edit Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162754|0 |1 is obsolete| | --- Comment #22 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162792 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162792&action=edit Bug 36066: (follow-up) Move 403 to 409 when status ne cancelled Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #23 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Works, makes sense, QA script happy, code looks good, passing QA :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #24 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Thanks for also fixing POD :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 --- Comment #25 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|24.05.00 |24.05.00,23.11.04 released in| | CC| |fridolin.somers@biblibre.co | |m --- Comment #26 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.11.x for 23.11.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00,23.11.04 |24.05.00,23.11.04,23.05.10 released in| | CC| |lucas@bywatersolutions.com Status|Pushed to stable |Pushed to oldstable --- Comment #27 from Lucas Gass <lucas@bywatersolutions.com> --- Backported to 23.05.x for upcoming 23.05.10 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|martin.renvoize@ptfs-europe | |.com | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Resolution|--- |FIXED Status|Pushed to oldstable |RESOLVED --- Comment #28 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11 unless requested -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org