Bug 36066 - REST API: We should only allow deleting cancelled order lines
Summary: REST API: We should only allow deleting cancelled order lines
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-12 09:24 UTC by Marcel de Rooy
Modified: 2024-03-19 21:22 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.10


Attachments
Bug 36066: Only allow cancelled order deletion in API (3.32 KB, patch)
2024-02-12 10:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36066: Only allow cancelled order deletion in API (3.33 KB, patch)
2024-02-19 15:03 UTC, David Nind
Details | Diff | Splinter Review
Bug 36066: Add tests (2.10 KB, patch)
2024-03-04 22:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36066: Only allow cancelled order deletion in API (1.52 KB, patch)
2024-03-04 22:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36066: Add tests (2.53 KB, patch)
2024-03-05 12:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36066: Only allow cancelled order deletion in API (1.61 KB, patch)
2024-03-05 12:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36066: (follow-up) Move 403 to 409 when status ne cancelled (2.59 KB, patch)
2024-03-05 14:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36066: Add tests (2.55 KB, patch)
2024-03-05 21:40 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36066: Only allow cancelled order deletion in API (1.63 KB, patch)
2024-03-05 21:40 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36066: (follow-up) Move 403 to 409 when status ne cancelled (2.61 KB, patch)
2024-03-05 21:40 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2024-02-12 09:24:30 UTC
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.
Comment 1 Marcel de Rooy 2024-02-12 10:08:19 UTC
Created attachment 162017 [details] [review]
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
Comment 2 Katrin Fischer 2024-02-12 12:21:53 UTC
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?
Comment 3 Marcel de Rooy 2024-02-19 09:20:20 UTC
(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?
Comment 4 Marcel de Rooy 2024-02-19 10:48:55 UTC
(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.
Comment 5 David Nind 2024-02-19 15:03:11 UTC
Created attachment 162276 [details] [review]
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>
Comment 6 Marcel de Rooy 2024-02-20 12:04:36 UTC
(In reply to David Nind from comment #5)
> Signed-off-by: David Nind <david@davidnind.com>

Thanks!
Comment 7 Victor Grousset/tuxayo 2024-03-04 22:59:13 UTC
Created attachment 162735 [details] [review]
Bug 36066: Add tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Victor Grousset/tuxayo 2024-03-04 22:59:16 UTC
Created attachment 162736 [details] [review]
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>
Comment 9 Victor Grousset/tuxayo 2024-03-05 03:37:19 UTC
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.
Comment 10 Victor Grousset/tuxayo 2024-03-05 04:10:27 UTC
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 :(
Comment 11 Tomás Cohen Arazi 2024-03-05 11:08:34 UTC
(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
Comment 12 Marcel de Rooy 2024-03-05 12:13:58 UTC
Created attachment 162738 [details] [review]
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.
Comment 13 Marcel de Rooy 2024-03-05 12:14:01 UTC
Created attachment 162739 [details] [review]
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>
Comment 14 Marcel de Rooy 2024-03-05 12:14:45 UTC
(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.
Comment 15 Marcel de Rooy 2024-03-05 12:16:46 UTC
(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.
Comment 16 Tomás Cohen Arazi 2024-03-05 12:29:00 UTC
(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.
Comment 17 Marcel de Rooy 2024-03-05 14:56:11 UTC
Created attachment 162754 [details] [review]
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>
Comment 18 Marcel de Rooy 2024-03-05 14:56:57 UTC
(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.
Comment 19 Victor Grousset/tuxayo 2024-03-05 21:40:18 UTC
(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
Comment 20 Victor Grousset/tuxayo 2024-03-05 21:40:23 UTC
Created attachment 162790 [details] [review]
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>
Comment 21 Victor Grousset/tuxayo 2024-03-05 21:40:26 UTC
Created attachment 162791 [details] [review]
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>
Comment 22 Victor Grousset/tuxayo 2024-03-05 21:40:29 UTC
Created attachment 162792 [details] [review]
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>
Comment 23 Victor Grousset/tuxayo 2024-03-05 21:40:54 UTC
Works, makes sense, QA script happy, code looks good, passing QA :)
Comment 24 Katrin Fischer 2024-03-08 17:55:01 UTC
Thanks for also fixing POD :)
Comment 25 Katrin Fischer 2024-03-08 17:59:53 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 26 Fridolin Somers 2024-03-11 14:36:22 UTC
Pushed to 23.11.x for 23.11.04
Comment 27 Lucas Gass 2024-03-19 21:22:13 UTC
Backported to 23.05.x for upcoming 23.05.10