Code that handles authorization to a client's own objects in REST API public routes at Koha::REST::V1::Auth::check_object_ownership() [1] is dangerous, potentially revealing someone else's data to an unauthorized user. This is the so called "allow-owner" bit in OpenAPI path x-koha-authorization definition. As far as I know it is unexploitable in currently supported releases, but it has been possible to exploit in Koha versions from 16.11 to 18.05 for getting a list of all holds without permissions. The exploit was possible with a following curl request... curl -X GET http://koha/api/v1/holds -d '{"borrowernumber":123}' -H 'Cookie: CGISESSID=28f8ff1caf89f2bf263f6c3f59b4f625' ...where 123 is the borrowernumber of an authenticated but unprivileged user, and CGISESSID is the user's active session id. The idea behind this code is to check if an user performing a request to an endpoint is requesting their own data. It looks at path, query and JSON-body parameters to see if authorization is possible. Path and query parameter would grant access at /patrons/123 and /patrons?borrowernumber=123 if the user performing the request is borrowernumber 123. The reason it looks at JSON body is for cases where there are no path nor query parameters, but something in the body could still authorize the user. A typical use case for this would be a POST request where an user passes a borrowernumber in JSON body with some other data, like placing a hold. But the code completely misses a case where a GET request with a JSON body content is performed to an endpoint with no path nor required query parameter (as in the curl exploit example above), therefore fooling it to thinking you are accessing your own resources when you are actually requesting everything else as well. It might also be possible to exploit it in other ways, but I haven't explored this enough to be sure. It was very horribly exploitable in the Finnish Koha (17.05 with hundreds of custom commits) where an unauthorized user could have a complete read-access to tables like patrons, checkouts, holds etc. They now have a temporary patch in place to stop any damage until a permanent upstream solution is implemented. They are unsure if any data was actually leaked, but some routes caused an internal server error and at least none of those were found in their logs. This could currently affect some Koha plugins with public REST API routes. The most common place for it to occur is a list operation (GET /whatever) where allow-owner: true. If the route has a path parameter as follows /whatever/{id} or a *required* query parameter /whatever?id= then you are be safe for the body-exploit, because those parameters are checked before the body. This authorization logic must be refactored or replaced completely. Related Bug 24067 [1] https://github.com/Koha-Community/Koha/blob/9d6d641d1f8b77271800f43bc027b651f9aea52b/Koha/REST/V1/Auth.pm#L378-L406
Has anyone ever reviewed this closely?
This is still valid I think, but only theoretical so far. We must fix it before it becomes real however. We should not rely on the body parameters.
Created attachment 171666 [details] [review] Bug 28907: Add REST exceptions for public routes auth To test: 1. perl -c Koha/REST/Plugin/Exceptions.pm 2. perl -c Koha/Exceptions/REST.pm More tests coming in following patches.
Created attachment 171667 [details] [review] Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes To test: 1. prove t/db_dependent/Koha/REST/Plugin/Auth/PublicRoutes.t
Created attachment 171668 [details] [review] Bug 28097: REST - Remove allow-owner from public password route To test: 1. prove t/db_dependent/api/v1/patrons_password.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_password.t Observe success in both cases.
Created attachment 171669 [details] [review] Bug 28907: REST - Remove allow-owner from public checkouts route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases.
Created attachment 171670 [details] [review] Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts To test: 1. prove t/db_dependent/api/v1/patrons.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons.t Observe success in both cases.
Created attachment 171671 [details] [review] Bug 28907: REST - Remove allow-owner from public patron hold cancellation To test: 1. prove t/db_dependent/api/v1/patrons_holds.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_holds.t
Created attachment 171672 [details] [review] Bug 28907: REST - Remove allow-owner from public checkout availability route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases.
Created attachment 171673 [details] [review] Bug 28907: REST - Remove allow-owner from public article requests cancel route To test: 1. prove t/db_dependent/api/v1/article_requests.t 2. Apply patch 3. prove t/db_dependent/api/v1/article_requests.t Observe success in both cases.
Created attachment 171674 [details] [review] Bug 28907: REST - Drop support for allow-owner functionality ...and allow-guarantor functionality. Replaced by $c->auth->public($patron_id) and/or $c->auth->public_guarantor($patron_id), where $patron_id is the patron's id that owns the requested resource. Old method, was applicable to both privileged and public routes: api/v1/swagger/paths/route.yaml x-koha-authorization: allow-owner: true allow-guarantor: true New method, use public routes with no x-koha-authorization: GET /public/route/{patron_id} Koha/REST/V1/Controller#public_action: sub public_action { my $c = shift->openapi->valid_input or return; my $patron_id = $c->param( 'patron_id' ); try { # Throws an exception that will render a response of 401 if not # authenticated and 403 if trying to access another user's resources $c->auth->public($patron_id); #or $c->auth->public_guarantor($patron_id) ... # other code ... } catch { $c->unhandled_exception($_); } } Another example of retrieving $patron_id when patron_id is not a request parameter: GET /public/another/object/{another_object_id} my $patron_id = Another::Object->find($another_object_id)->borrowernumber; try { # 403 if $another_object_id does not belong to API user $c->auth->public($patron_id); ...
Still valid and the vulnerability is currently exploitable, at least in following cases: A. Public hold cancellation, DELETE /api/v1/public/patrons/{patron_id}/holds/{hold_id} To reproduce: 1. Have system preference RESTPublicAPI enabled 2. You as an exploiter, must know both a victim hold's borrowernumber and reserve_id in order to construct valid request 3. curl -X DELETE http://koha/api/v1/public/patrons/1/holds/100 -d '{"borrowernumber":2}' -H 'Cookie: CGISESSID=22c22222d2d222bfd22f22f2a2cf2e22' -v ...where 1 is the victim's borrowernumber and 100 is the victim's reserve_id, and 2 is the exploiter's borrowernumber and 22c22222d2d222bfd22f22f2a2cf2e22 is the exploiter's session id. The exploiter can be any normal logged-in OPAC user with no permissions. B. Public article request cancellation, DELETE /api/v1/public/patrons/1/article_requests/100 To reproduce, use same method as step A but replace the endpoint. I have proposed a solution in my patches. Attempting to reproduce after patches are applied gives the following response: HTTP/1.1 403 Forbidden {"error":"Unprivileged user cannot access another user's resources"}
Testing notes: attack works on either staff and OPAC with a regular patron account for attack. holds: curl -X DELETE http://localhost:8080/api/v1/public/patrons/$VICTIM_BORROWERNUMBER/holds/$VICTIM_RESERVE_ID -d '{"borrowernumber":$ATTACKER_BORROWERNUMBER}' -H 'Cookie: CGISESSID=$ATTACKER_CGISESSID' -v article request request: curl -X DELETE http://localhost:8080/api/v1/public/patrons/$VICTIM_BORROWERNUMBER/article_requests/$VICTIM_RESERVE_ID -d '{"borrowernumber":$ATTACKER_BORROWERNUMBER}' -H 'Cookie: CGISESSID=$ATTACKER_CGISESSID' -v For article request ***It needs to be ran twice to work, the 1st time it fails*** (this is weird) «"error":"Authorization failure. Missing required permission(s).","required_permissions":null}» You can spam it and it will alternate between 403 and 204. Somehow there is a state somewhere. And from was I see in htop, the workers aren't alternating so that doesn't look like that. -------- > {"error":"Unprivileged user cannot access another user's resources"} I get instead {"error":"Authorization failure. Missing required permission(s).","required_permissions":null}% But the attack still fails thanks to the patches, it's just not the expected response. Is that an issue?
Created attachment 172434 [details] [review] Bug 28907: Add REST exceptions for public routes auth To test: 1. perl -c Koha/REST/Plugin/Exceptions.pm 2. perl -c Koha/Exceptions/REST.pm More tests coming in following patches. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 172435 [details] [review] Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes To test: 1. prove t/db_dependent/Koha/REST/Plugin/Auth/PublicRoutes.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 172436 [details] [review] Bug 28097: REST - Remove allow-owner from public password route To test: 1. prove t/db_dependent/api/v1/patrons_password.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_password.t Observe success in both cases. https://bugs.koha-community.org/show_bug.cgi?id=28907 Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 172437 [details] [review] Bug 28907: REST - Remove allow-owner from public checkouts route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 172438 [details] [review] Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts To test: 1. prove t/db_dependent/api/v1/patrons.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 172439 [details] [review] Bug 28907: REST - Remove allow-owner from public patron hold cancellation To test: 1. prove t/db_dependent/api/v1/patrons_holds.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_holds.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 172440 [details] [review] Bug 28907: REST - Remove allow-owner from public checkout availability route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 172441 [details] [review] Bug 28907: REST - Remove allow-owner from public article requests cancel route To test: 1. prove t/db_dependent/api/v1/article_requests.t 2. Apply patch 3. prove t/db_dependent/api/v1/article_requests.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 172442 [details] [review] Bug 28907: REST - Drop support for allow-owner functionality ...and allow-guarantor functionality. Replaced by $c->auth->public($patron_id) and/or $c->auth->public_guarantor($patron_id), where $patron_id is the patron's id that owns the requested resource. Old method, was applicable to both privileged and public routes: api/v1/swagger/paths/route.yaml x-koha-authorization: allow-owner: true allow-guarantor: true New method, use public routes with no x-koha-authorization: GET /public/route/{patron_id} Koha/REST/V1/Controller#public_action: sub public_action { my $c = shift->openapi->valid_input or return; my $patron_id = $c->param( 'patron_id' ); try { # Throws an exception that will render a response of 401 if not # authenticated and 403 if trying to access another user's resources $c->auth->public($patron_id); #or $c->auth->public_guarantor($patron_id) ... # other code ... } catch { $c->unhandled_exception($_); } } Another example of retrieving $patron_id when patron_id is not a request parameter: GET /public/another/object/{another_object_id} my $patron_id = Another::Object->find($another_object_id)->borrowernumber; try { # 403 if $another_object_id does not belong to API user $c->auth->public($patron_id); ... Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
It works! :) restart_all in KTD isn't enough to get the tests passing. reset_all is necessary. Likely that's the cause of the last point of comment 13. Since one of tests fails for that precise reason (different error message). Mystery solved, pretty much. - start KTD on main - tests pass - apply patches - restart_all - tests fail: t/db_dependent/api/v1/patrons_password.t ............. 1/2 # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_password.t line 151. # Structures begin differing at: # $got->{required_permissions} = undef # $expected->{required_permissions} = Does not exist # Failed test 'exact match for JSON Pointer "/error"' # at t/db_dependent/api/v1/patrons_password.t line 179. # got: 'Authorization failure. Missing required permission(s).' # expected: 'Changing other patron's password is forbidden' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_password.t line 191. # Structures begin differing at: # $got->{required_permissions} = undef # $expected->{required_permissions} = Does not exist # Failed test '400 Bad Request' # at t/db_dependent/api/v1/patrons_password.t line 204. # got: '403' # expected: '400' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_password.t line 204. # Structures begin differing at: # $got->{required_permissions} = undef # $expected->{required_permissions} = Does not exist # Failed test '400 Bad Request' # at t/db_dependent/api/v1/patrons_password.t line 218. # got: '403' # expected: '400' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_password.t line 218. # Structures begin differing at: # $got->{error} = 'Authorization failure. Missing required permission(s).' # $expected->{error} = 'Invalid password' # Failed test '200 OK' # at t/db_dependent/api/v1/patrons_password.t line 232. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_password.t line 232. # Structures begin differing at: # $got = HASH(0x5810c4d2efd0) # $expected = '' # Looks like you failed 9 tests of 21. t/db_dependent/api/v1/patrons_password.t ............. 2/2 # Failed test 'set_public() (unprivileged user tests)' # at t/db_dependent/api/v1/patrons_password.t line 243. # Looks like you failed 1 test of 2. t/db_dependent/api/v1/patrons_password.t ............. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/2 subtests t/db_dependent/api/v1/checkouts.t .................... 97/108 # Failed test '200 OK' # at t/db_dependent/api/v1/checkouts.t line 371. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer "/blockers"' # at t/db_dependent/api/v1/checkouts.t line 371. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac635e788) # Failed test 'exact match for JSON Pointer "/confirms"' # at t/db_dependent/api/v1/checkouts.t line 371. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac66bf8a8) # Failed test 'exact match for JSON Pointer "/warnings"' # at t/db_dependent/api/v1/checkouts.t line 371. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac6c36538) # Failed test 'has value for JSON Pointer "/confirmation_token"' # at t/db_dependent/api/v1/checkouts.t line 371. # Failed test '200 OK' # at t/db_dependent/api/v1/checkouts.t line 378. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer "/blockers"' # at t/db_dependent/api/v1/checkouts.t line 378. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac686c270) # Failed test 'exact match for JSON Pointer "/confirms"' # at t/db_dependent/api/v1/checkouts.t line 378. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac687b8c0) # Failed test 'exact match for JSON Pointer "/warnings"' # at t/db_dependent/api/v1/checkouts.t line 378. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac67e5da0) # Failed test 'has value for JSON Pointer "/confirmation_token"' # at t/db_dependent/api/v1/checkouts.t line 378. # Failed test '200 OK' # at t/db_dependent/api/v1/checkouts.t line 428. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer "/blockers"' # at t/db_dependent/api/v1/checkouts.t line 428. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac6c1a3d8) # Failed test 'exact match for JSON Pointer "/confirms"' # at t/db_dependent/api/v1/checkouts.t line 428. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac6c7e770) # Failed test 'exact match for JSON Pointer "/warnings"' # at t/db_dependent/api/v1/checkouts.t line 428. # Structures begin differing at: # $got = undef # $expected = HASH(0x629ac6c24100) # Failed test 'has value for JSON Pointer "/confirmation_token"' # at t/db_dependent/api/v1/checkouts.t line 428. # Looks like you failed 15 tests of 22. # Failed test 'public availability' # at t/db_dependent/api/v1/checkouts.t line 435. # Looks like you failed 1 test of 29. t/db_dependent/api/v1/checkouts.t .................... 107/108 # Failed test 'get_availability' # at t/db_dependent/api/v1/checkouts.t line 438. # Failed test '405 Method Not Allowed' # at t/db_dependent/api/v1/checkouts.t line 525. # got: '403' # expected: '405' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/checkouts.t line 525. # Structures begin differing at: # $got->{error} = 'Authorization failure. Missing required permission(s).' # $expected->{error} = 'Feature disabled' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/checkouts.t line 536. # Structures begin differing at: # $got->{error} = 'Authorization failure. Missing required permission(s).' # $expected->{error} = 'Unprivileged user cannot access another user's resources' # Failed test '201 Created' # at t/db_dependent/api/v1/checkouts.t line 543. # got: '403' # expected: '201' # Looks like you failed 4 tests of 14. # Failed test 'public add' # at t/db_dependent/api/v1/checkouts.t line 545. # Looks like you failed 1 test of 10. t/db_dependent/api/v1/checkouts.t .................... 108/108 # Failed test 'add checkout' # at t/db_dependent/api/v1/checkouts.t line 548. # Looks like you failed 2 tests of 108. t/db_dependent/api/v1/checkouts.t .................... Dubious, test returned 2 (wstat 512, 0x200) Failed 2/108 subtests t/db_dependent/api/v1/patrons.t ...................... 5/7 # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons.t line 1163. # Structures begin differing at: # $got->{required_permissions} = undef # $expected->{required_permissions} = Does not exist # Failed test 'exact match for JSON Pointer "/error"' # at t/db_dependent/api/v1/patrons.t line 1167. # got: 'Authorization failure. Missing required permission(s).' # expected: 'The current configuration doesn't allow the requested action.' # Failed test '200 OK' # at t/db_dependent/api/v1/patrons.t line 1173. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons.t line 1173. # Structures begin differing at: # $got->{required_permissions} = undef # $expected->{required_permissions} = Does not exist # Failed test 'privacy_guarantor_fines has been set correctly' # at t/db_dependent/api/v1/patrons.t line 1177. # Failed test '200 OK' # at t/db_dependent/api/v1/patrons.t line 1179. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons.t line 1179. # Structures begin differing at: # $got->{error} = 'Authorization failure. Missing required permission(s).' # $expected->{error} = Does not exist # Looks like you failed 7 tests of 17. t/db_dependent/api/v1/patrons.t ...................... 6/7 # Failed test 'guarantors_can_see_charges() tests' # at t/db_dependent/api/v1/patrons.t line 1186. # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons.t line 1207. # Structures begin differing at: # $got->{error} = 'Authorization failure. Missing required permission(s).' # $expected->{error} = 'Unprivileged user cannot access another user's resources' # Failed test 'exact match for JSON Pointer "/error"' # at t/db_dependent/api/v1/patrons.t line 1213. # got: 'Authorization failure. Missing required permission(s).' # expected: 'The current configuration doesn't allow the requested action.' # Failed test '200 OK' # at t/db_dependent/api/v1/patrons.t line 1219. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons.t line 1219. # Structures begin differing at: # $got->{required_permissions} = undef # $expected->{required_permissions} = Does not exist # Failed test 'privacy_guarantor_checkouts has been set correctly' # at t/db_dependent/api/v1/patrons.t line 1223. # Failed test '200 OK' # at t/db_dependent/api/v1/patrons.t line 1225. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons.t line 1225. # Structures begin differing at: # $got->{error} = 'Authorization failure. Missing required permission(s).' # $expected->{error} = Does not exist # Looks like you failed 7 tests of 17. # Failed test 'guarantors_can_see_checkouts() tests' # at t/db_dependent/api/v1/patrons.t line 1232. # Looks like you failed 2 tests of 7. t/db_dependent/api/v1/patrons.t ...................... Dubious, test returned 2 (wstat 512, 0x200) Failed 2/7 subtests t/db_dependent/api/v1/patrons_holds.t ................ 1/2 # Failed test '404 Not Found' # at t/db_dependent/api/v1/patrons_holds.t line 119. # got: '403' # expected: '404' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_holds.t line 124. # Structures begin differing at: # $got->{required_permissions} = undef # $expected->{required_permissions} = Does not exist # Failed test 'Invalid patron_id and hold_id combination yields 404' # at t/db_dependent/api/v1/patrons_holds.t line 130. # got: '403' # expected: '404' # Failed test 'SWAGGER3.2.4' # at t/db_dependent/api/v1/patrons_holds.t line 144. # got: '403' # expected: '204' # Failed test 'SWAGGER3.3.4' # at t/db_dependent/api/v1/patrons_holds.t line 144. # got: '{"error":"Authorization failure. Missing required permission(s).","required_permissions":null}' # expected: '' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_holds.t line 166. # Structures begin differing at: # $got->{required_permissions} = undef # $expected->{required_permissions} = Does not exist # Failed test '202 Accepted' # at t/db_dependent/api/v1/patrons_holds.t line 171. # got: '403' # expected: '202' # Failed test 'Cancellation request recorded' # at t/db_dependent/api/v1/patrons_holds.t line 175. # got: '0' # expected: '1' # Looks like you failed 8 tests of 18. t/db_dependent/api/v1/patrons_holds.t ................ 2/2 # Failed test 'delete_public() tests' # at t/db_dependent/api/v1/patrons_holds.t line 178. # Looks like you failed 1 test of 2. t/db_dependent/api/v1/patrons_holds.t ................ Dubious, test returned 1 (wstat 256, 0x100) Failed 1/2 subtests t/db_dependent/api/v1/article_requests.t ............. 1/2 # Failed test '404 Not Found' # at t/db_dependent/api/v1/article_requests.t line 110. # got: '403' # expected: '404' # Failed test 'exact match for JSON Pointer "/error_code"' # at t/db_dependent/api/v1/article_requests.t line 110. # got: undef # expected: 'not_found' # Failed test 'exact match for JSON Pointer "/error"' # at t/db_dependent/api/v1/article_requests.t line 124. # got: 'Authorization failure. Missing required permission(s).' # expected: 'Unprivileged user cannot access another user's resources' # Failed test '404 Not Found' # at t/db_dependent/api/v1/article_requests.t line 135. # got: '403' # expected: '404' # Failed test 'exact match for JSON Pointer "/error_code"' # at t/db_dependent/api/v1/article_requests.t line 135. # got: undef # expected: 'not_found' # Failed test 'SWAGGER3.2.4' # at t/db_dependent/api/v1/article_requests.t line 149. # got: '403' # expected: '204' # Failed test 'SWAGGER3.2.4' # at t/db_dependent/api/v1/article_requests.t line 149. # got: '{"error":"Authorization failure. Missing required permission(s).","required_permissions":null}' # expected: '' # Failed test 'Reason stored correctly' # at t/db_dependent/api/v1/article_requests.t line 159. # got: 'b3kvX_duTO98c_Hx9v1epWgHQYq8mpGgKEZ1jqW81R1Su8kR4JWgIwGkJ02QsCwrHP9CUVwelBm' # expected: 'A reason' # Failed test 'Notes stored correctly' # at t/db_dependent/api/v1/article_requests.t line 160. # got: 'LVI34x6' # expected: 'Some notes' # Looks like you failed 9 tests of 17. t/db_dependent/api/v1/article_requests.t ............. 2/2 # Failed test 'patron_cancel() tests' # at t/db_dependent/api/v1/article_requests.t line 163. # Looks like you failed 1 test of 2. t/db_dependent/api/v1/article_requests.t ............. Dubious, test returned 1 (wstat 256, 0x100)
Created attachment 174662 [details] [review] Bug 28907: Add REST exceptions for public routes auth To test: 1. perl -c Koha/REST/Plugin/Exceptions.pm 2. perl -c Koha/Exceptions/REST.pm More tests coming in following patches. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174663 [details] [review] Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes To test: 1. prove t/db_dependent/Koha/REST/Plugin/Auth/PublicRoutes.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174664 [details] [review] Bug 28097: REST - Remove allow-owner from public password route To test: 1. prove t/db_dependent/api/v1/patrons_password.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_password.t Observe success in both cases. https://bugs.koha-community.org/show_bug.cgi?id=28907 Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174665 [details] [review] Bug 28907: REST - Remove allow-owner from public checkouts route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174666 [details] [review] Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts To test: 1. prove t/db_dependent/api/v1/patrons.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174667 [details] [review] Bug 28907: REST - Remove allow-owner from public patron hold cancellation To test: 1. prove t/db_dependent/api/v1/patrons_holds.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_holds.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174668 [details] [review] Bug 28907: REST - Remove allow-owner from public checkout availability route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174669 [details] [review] Bug 28907: REST - Remove allow-owner from public article requests cancel route To test: 1. prove t/db_dependent/api/v1/article_requests.t 2. Apply patch 3. prove t/db_dependent/api/v1/article_requests.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174670 [details] [review] Bug 28907: REST - Drop support for allow-owner functionality ...and allow-guarantor functionality. Replaced by $c->auth->public($patron_id) and/or $c->auth->public_guarantor($patron_id), where $patron_id is the patron's id that owns the requested resource. Old method, was applicable to both privileged and public routes: api/v1/swagger/paths/route.yaml x-koha-authorization: allow-owner: true allow-guarantor: true New method, use public routes with no x-koha-authorization: GET /public/route/{patron_id} Koha/REST/V1/Controller#public_action: sub public_action { my $c = shift->openapi->valid_input or return; my $patron_id = $c->param( 'patron_id' ); try { # Throws an exception that will render a response of 401 if not # authenticated and 403 if trying to access another user's resources $c->auth->public($patron_id); #or $c->auth->public_guarantor($patron_id) ... # other code ... } catch { $c->unhandled_exception($_); } } Another example of retrieving $patron_id when patron_id is not a request parameter: GET /public/another/object/{another_object_id} my $patron_id = Another::Object->find($another_object_id)->borrowernumber; try { # 403 if $another_object_id does not belong to API user $c->auth->public($patron_id); ... Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
CONFLICT (content): Merge conflict in Koha/REST/V1/Checkouts.pm Patches rebased. Tests are passing for me. I have not done any code review.
WARN Koha/REST/Plugin/Exceptions.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 2, now: 4) WARN Koha/REST/V1/Checkouts.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 43, now: 44) WARN t/db_dependent/api/v1/checkouts.t WARN tidiness The file is less tidy than before (bad/messy lines before: 133, now: 138) WARN t/db_dependent/api/v1/patrons.t WARN tidiness The file is less tidy than before (bad/messy lines before: 267, now: 269)
Having lots of failures in the tests: /usr/share/koha/t/db_dependent/api/v1/checkouts.t (Wstat: 12288 (exited 48) Tests: 108 Failed: 48) Failed tests: 11-17, 20-26, 29-31, 34-35, 41-47, 50, 55-57 60, 65-67, 70-73, 76-77, 79-80, 82-83, 98-99 107-108 Non-zero exit status: 48 Files=1, Tests=108, 16 wallclock secs ( 0.04 usr 0.02 sys + 14.13 cusr 0.86 csys = 15.05 CPU) Result: FAIL /usr/share/koha/t/db_dependent/api/v1/article_requests.t (Wstat: 256 (exited 1) Tests: 2 Failed: 1) Failed test: 2 Non-zero exit status: 1 Files=1, Tests=2, 21 wallclock secs ( 0.03 usr 0.00 sys + 18.08 cusr 2.33 csys = 20.44 CPU) Result: FAIL /usr/share/koha/t/db_dependent/api/v1/patrons_holds.t (Wstat: 256 (exited 1) Tests: 2 Failed: 1) Failed test: 2 Non-zero exit status: 1 Files=1, Tests=2, 13 wallclock secs ( 0.04 usr 0.00 sys + 12.20 cusr 0.93 csys = 13.17 CPU) Result: FAIL /usr/share/koha/t/db_dependent/api/v1/patrons.t (Wstat: 512 (exited 2) Tests: 7 Failed: 2) Failed tests: 6-7 Non-zero exit status: 2 Files=1, Tests=7, 24 wallclock secs ( 0.05 usr 0.01 sys + 20.90 cusr 2.05 csys = 23.01 CPU) Result: FAIL /usr/share/koha/t/db_dependent/api/v1/patrons_password.t (Wstat: 256 (exited 1) Tests: 2 Failed: 1) Failed test: 2 Non-zero exit status: 1 Files=1, Tests=2, 11 wallclock secs ( 0.03 usr 0.01 sys + 10.47 cusr 0.49 csys = 11.00 CPU) Result: FAIL
> Having lots of failures in the tests: Maybe it's the same issue I had when testing: reset_all is needed after applying. Or yarn api:bundle restart_all https://wiki.koha-community.org/wiki/Rest_Api_HowTo#Testing_API_patches But I'm not sure, your output seems to show more failures than I have when doing only restart_all. Do you have zero failures without the patches? With the patches and after running just yarn api:bundle, all 5 tests files still pass on my side.
Running a yarn rebuild resolves the failures indeed.
Created attachment 176708 [details] [review] Bug 28907: Add REST exceptions for public routes auth To test: 1. perl -c Koha/REST/Plugin/Exceptions.pm 2. perl -c Koha/Exceptions/REST.pm More tests coming in following patches. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 176709 [details] [review] Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes To test: 1. prove t/db_dependent/Koha/REST/Plugin/Auth/PublicRoutes.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 176710 [details] [review] Bug 28097: REST - Remove allow-owner from public password route To test: 1. prove t/db_dependent/api/v1/patrons_password.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_password.t Observe success in both cases. https://bugs.koha-community.org/show_bug.cgi?id=28907 Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 176711 [details] [review] Bug 28907: REST - Remove allow-owner from public checkouts route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 176712 [details] [review] Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts To test: 1. prove t/db_dependent/api/v1/patrons.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 176713 [details] [review] Bug 28907: REST - Remove allow-owner from public patron hold cancellation To test: 1. prove t/db_dependent/api/v1/patrons_holds.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_holds.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 176714 [details] [review] Bug 28907: REST - Remove allow-owner from public checkout availability route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 176715 [details] [review] Bug 28907: REST - Remove allow-owner from public article requests cancel route To test: 1. prove t/db_dependent/api/v1/article_requests.t 2. Apply patch 3. prove t/db_dependent/api/v1/article_requests.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 176716 [details] [review] Bug 28907: REST - Drop support for allow-owner functionality ...and allow-guarantor functionality. Replaced by $c->auth->public($patron_id) and/or $c->auth->public_guarantor($patron_id), where $patron_id is the patron's id that owns the requested resource. Old method, was applicable to both privileged and public routes: api/v1/swagger/paths/route.yaml x-koha-authorization: allow-owner: true allow-guarantor: true New method, use public routes with no x-koha-authorization: GET /public/route/{patron_id} Koha/REST/V1/Controller#public_action: sub public_action { my $c = shift->openapi->valid_input or return; my $patron_id = $c->param( 'patron_id' ); try { # Throws an exception that will render a response of 401 if not # authenticated and 403 if trying to access another user's resources $c->auth->public($patron_id); #or $c->auth->public_guarantor($patron_id) ... # other code ... } catch { $c->unhandled_exception($_); } } Another example of retrieving $patron_id when patron_id is not a request parameter: GET /public/another/object/{another_object_id} my $patron_id = Another::Object->find($another_object_id)->borrowernumber; try { # 403 if $another_object_id does not belong to API user $c->auth->public($patron_id); ... Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Hi, We are seeing the unit test is failing for the third patch on the 24.05.x-security branch. See below output: kohadev-koha@kohadevbox:koha(24.05bug_28907)$ prove t/db_dependent/api/v1/patrons_password.t t/db_dependent/api/v1/patrons_password.t .. 1/2 # Failed test 'exact match for JSON Pointer "/error"' # at t/db_dependent/api/v1/patrons_password.t line 179. # got: 'Authorization failure. Missing required permission(s).' # expected: 'Changing other patron's password is forbidden' # Looks like you failed 1 test of 21. t/db_dependent/api/v1/patrons_password.t .. 2/2 # Failed test 'set_public() (unprivileged user tests)' # at t/db_dependent/api/v1/patrons_password.t line 243. # Looks like you failed 1 test of 2. t/db_dependent/api/v1/patrons_password.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/2 subtests Test Summary Report ------------------- t/db_dependent/api/v1/patrons_password.t (Wstat: 256 (exited 1) Tests: 2 Failed: 1) Failed test: 2 Non-zero exit status: 1 Files=1, Tests=2, 4 wallclock secs ( 0.02 usr 0.00 sys + 3.12 cusr 0.25 csys = 3.39 CPU) Result: FAIL kohadev-koha@kohadevbox:koha(24.05bug_28907)$
(In reply to Alex Buckley from comment #47) > We are seeing the unit test is failing for the third patch on the > 24.05.x-security branch. Could it be the same issue as comment 13? Did you try: reset_all is needed after applying. Or yarn api:bundle restart_all (from comment 36)
(In reply to Lari Taskula from comment #48) > (In reply to Alex Buckley from comment #47) > > We are seeing the unit test is failing for the third patch on the > > 24.05.x-security branch. > Could it be the same issue as comment 13? > > Did you try: > > reset_all is needed after applying. > Or > yarn api:bundle > restart_all > > (from comment 36) Thanks Lari the reset_all worked for getting the third patch test to pass.
Hi all, We have tried to apply the fourth commit https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176711 and it did not apply cleanly, see below conflict: $ sed -n '/<<<<</,/>>>>>/p' Koha/REST/V1/Checkouts.pm <<<<<<< HEAD my $item = Koha::Items->find($item_id); ======= unless ( $item_id or $barcode ) { return $c->render( status => 400, openapi => { error => 'Missing or wrong parameters', error_code => 'MISSING_OR_WRONG_PARAMETERS', } ); } if ( $c->stash('is_public') ) { $c->auth->public($patron_id); return $c->render( status => 405, openapi => { error => 'Feature disabled', error_code => 'FEATURE_DISABLED' } ) if !C4::Context->preference('OpacTrustedCheckout'); } my $item; if ($item_id) { $item = Koha::Items->find($item_id); } else { $item = Koha::Items->find( { barcode => $barcode } ); } >>>>>>> Bug 28907: REST - Remove allow-owner from public checkouts route kohadev-koha@kohadevbox:koha(24.05.x-security-28907|AM 1/1)$ Could someone please rebase for 24.05 and attach the patchsets to the bug report? Thanks, Alex
Created attachment 177679 [details] [review] [24.05.x] Bug 28907: Add REST exceptions for public routes auth To test: 1. perl -c Koha/REST/Plugin/Exceptions.pm 2. perl -c Koha/Exceptions/REST.pm More tests coming in following patches. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 177680 [details] [review] [24.05.x] Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes To test: 1. prove t/db_dependent/Koha/REST/Plugin/Auth/PublicRoutes.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 177681 [details] [review] [24.05.x] Bug 28097: REST - Remove allow-owner from public password route To test: 1. prove t/db_dependent/api/v1/patrons_password.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_password.t Observe success in both cases. https://bugs.koha-community.org/show_bug.cgi?id=28907 Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 177682 [details] [review] [24.05.x] Bug 28907: REST - Remove allow-owner from public checkouts route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 177683 [details] [review] [24.05.x] Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts To test: 1. prove t/db_dependent/api/v1/patrons.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 177684 [details] [review] [24.05.x] Bug 28907: REST - Remove allow-owner from public patron hold cancellation To test: 1. prove t/db_dependent/api/v1/patrons_holds.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_holds.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 177685 [details] [review] [24.05.x] Bug 28907: REST - Remove allow-owner from public checkout availability route To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Apply patch 3. prove t/db_dependent/api/v1/checkouts.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 177686 [details] [review] [24.05.x] Bug 28907: REST - Remove allow-owner from public article requests cancel route To test: 1. prove t/db_dependent/api/v1/article_requests.t 2. Apply patch 3. prove t/db_dependent/api/v1/article_requests.t Observe success in both cases. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 177687 [details] [review] [24.05.x] Bug 28907: REST - Drop support for allow-owner functionality ...and allow-guarantor functionality. Replaced by $c->auth->public($patron_id) and/or $c->auth->public_guarantor($patron_id), where $patron_id is the patron's id that owns the requested resource. Old method, was applicable to both privileged and public routes: api/v1/swagger/paths/route.yaml x-koha-authorization: allow-owner: true allow-guarantor: true New method, use public routes with no x-koha-authorization: GET /public/route/{patron_id} Koha/REST/V1/Controller#public_action: sub public_action { my $c = shift->openapi->valid_input or return; my $patron_id = $c->param( 'patron_id' ); try { # Throws an exception that will render a response of 401 if not # authenticated and 403 if trying to access another user's resources $c->auth->public($patron_id); #or $c->auth->public_guarantor($patron_id) ... # other code ... } catch { $c->unhandled_exception($_); } } Another example of retrieving $patron_id when patron_id is not a request parameter: GET /public/another/object/{another_object_id} my $patron_id = Another::Object->find($another_object_id)->borrowernumber; try { # 403 if $another_object_id does not belong to API user $c->auth->public($patron_id); ... Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Alex Buckley from comment #50) > Could someone please rebase for 24.05 and attach the patchsets to the bug > report? Done, see patches above
(In reply to Lari Taskula from comment #60) > (In reply to Alex Buckley from comment #50) > > Could someone please rebase for 24.05 and attach the patchsets to the bug > > report? > > Done, see patches above Thank you!
Applied the 24.05.x patches on 24.05.x-security Note: There was no test plan in the final patch ( https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177687 ). Please let us know if there anything we should run for that?
(In reply to Alex Buckley from comment #62) > Applied the 24.05.x patches on 24.05.x-security > > Note: There was no test plan in the final patch ( > https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177687 ). Please > let us know if there anything we should run for that? Tests are pretty much covered by other patches but you're right, in theory this patch could still break the API so here's a test plan for the last patch "Bug 28907: REST - Drop support for allow-owner functionality". To test: 1. prove t/db_dependent/api/v1/auth_authenticate_api_request.t 2. In the command line, navigate to Koha root directory 3. Run the following command line command: grep --exclude-dir=.git -Prn 'allow[-_]owner'; grep --exclude-dir=.git -Prn 'allow[-_]guarantor'; 4. Observe no results
Aïe there is a typo in bug id : Bug 28097: REST - Remove allow-owner from public password route Be careful, this will generate invalid content in release notes.
Applied to 23.11.x-security branch
Applied to 24.11.x-security
Does not apply to 22.11.x. Merge Conflicts!
(In reply to Jesse Maseto from comment #67) > Does not apply to 22.11.x. Merge Conflicts! Taking care of this rebase now. Gimme a few, JesseM!
Applied to 22.11.x-security
Created attachment 179000 [details] [review] Bug 28907: [With corrected bug id] REST - Remove allow-owner from public password route To test: 1. prove t/db_dependent/api/v1/patrons_password.t 2. Apply patch 3. prove t/db_dependent/api/v1/patrons_password.t Observe success in both cases. https://bugs.koha-community.org/show_bug.cgi?id=28907 Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed for 25.05! Well done everyone, thank you!