Bug 28907

Summary: Potential unauthorized access in public REST routes
Product: Koha Reporter: Lari Taskula <lari.taskula>
Component: REST APIAssignee: Lari Taskula <lari.taskula>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: 1joynelson, alexbuckley, dcook, fridolin.somers, jesse, jonathan.druart, lucas, m.de.rooy, martin.renvoize, nick, paul.derscheid, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.02,24.05.07,23.11.12,22.11.24
Circulation function:
Attachments: Bug 28907: Add REST exceptions for public routes auth
Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes
Bug 28097: REST - Remove allow-owner from public password route
Bug 28907: REST - Remove allow-owner from public checkouts route
Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts
Bug 28907: REST - Remove allow-owner from public patron hold cancellation
Bug 28907: REST - Remove allow-owner from public checkout availability route
Bug 28907: REST - Remove allow-owner from public article requests cancel route
Bug 28907: REST - Drop support for allow-owner functionality
Bug 28907: Add REST exceptions for public routes auth
Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes
Bug 28097: REST - Remove allow-owner from public password route
Bug 28907: REST - Remove allow-owner from public checkouts route
Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts
Bug 28907: REST - Remove allow-owner from public patron hold cancellation
Bug 28907: REST - Remove allow-owner from public checkout availability route
Bug 28907: REST - Remove allow-owner from public article requests cancel route
Bug 28907: REST - Drop support for allow-owner functionality
Bug 28907: Add REST exceptions for public routes auth
Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes
Bug 28097: REST - Remove allow-owner from public password route
Bug 28907: REST - Remove allow-owner from public checkouts route
Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts
Bug 28907: REST - Remove allow-owner from public patron hold cancellation
Bug 28907: REST - Remove allow-owner from public checkout availability route
Bug 28907: REST - Remove allow-owner from public article requests cancel route
Bug 28907: REST - Drop support for allow-owner functionality
Bug 28907: Add REST exceptions for public routes auth
Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes
Bug 28097: REST - Remove allow-owner from public password route
Bug 28907: REST - Remove allow-owner from public checkouts route
Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts
Bug 28907: REST - Remove allow-owner from public patron hold cancellation
Bug 28907: REST - Remove allow-owner from public checkout availability route
Bug 28907: REST - Remove allow-owner from public article requests cancel route
Bug 28907: REST - Drop support for allow-owner functionality
[24.05.x] Bug 28907: Add REST exceptions for public routes auth
[24.05.x] Bug 28907: Add Koha::REST::Plugin::Auth::PublicRoutes
[24.05.x] Bug 28097: REST - Remove allow-owner from public password route
[24.05.x] Bug 28907: REST - Remove allow-owner from public checkouts route
[24.05.x] Bug 28907: REST - Remove allow-owner from public guarantors can see charges and checkouts
[24.05.x] Bug 28907: REST - Remove allow-owner from public patron hold cancellation
[24.05.x] Bug 28907: REST - Remove allow-owner from public checkout availability route
[24.05.x] Bug 28907: REST - Remove allow-owner from public article requests cancel route
[24.05.x] Bug 28907: REST - Drop support for allow-owner functionality
Bug 28907: [With corrected bug id] REST - Remove allow-owner from public password route

Description Lari Taskula 2021-08-26 19:04:37 UTC
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
Comment 1 Katrin Fischer 2023-07-01 13:25:22 UTC
Has anyone ever reviewed this closely?
Comment 2 Jonathan Druart 2023-07-04 08:40:59 UTC
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.
Comment 3 Lari Taskula 2024-09-18 14:07:34 UTC
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.
Comment 4 Lari Taskula 2024-09-18 14:07:36 UTC
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
Comment 5 Lari Taskula 2024-09-18 14:07:39 UTC
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.
Comment 6 Lari Taskula 2024-09-18 14:07:42 UTC
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.
Comment 7 Lari Taskula 2024-09-18 14:07:44 UTC
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.
Comment 8 Lari Taskula 2024-09-18 14:07:47 UTC
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
Comment 9 Lari Taskula 2024-09-18 14:07:49 UTC
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.
Comment 10 Lari Taskula 2024-09-18 14:07:52 UTC
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.
Comment 11 Lari Taskula 2024-09-18 14:07:54 UTC
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);
      ...
Comment 12 Lari Taskula 2024-09-18 14:14:30 UTC
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"}
Comment 13 Victor Grousset/tuxayo 2024-10-05 18:02:24 UTC
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?
Comment 14 Victor Grousset/tuxayo 2024-10-07 01:06:30 UTC
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>
Comment 15 Victor Grousset/tuxayo 2024-10-07 01:06:32 UTC
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>
Comment 16 Victor Grousset/tuxayo 2024-10-07 01:06:35 UTC
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>
Comment 17 Victor Grousset/tuxayo 2024-10-07 01:06:38 UTC
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>
Comment 18 Victor Grousset/tuxayo 2024-10-07 01:06:40 UTC
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>
Comment 19 Victor Grousset/tuxayo 2024-10-07 01:06:43 UTC
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>
Comment 20 Victor Grousset/tuxayo 2024-10-07 01:06:46 UTC
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>
Comment 21 Victor Grousset/tuxayo 2024-10-07 01:06:49 UTC
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>
Comment 22 Victor Grousset/tuxayo 2024-10-07 01:06:51 UTC
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>
Comment 23 Victor Grousset/tuxayo 2024-10-07 01:08:06 UTC
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)
Comment 24 Jonathan Druart 2024-11-18 09:02:56 UTC
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>
Comment 25 Jonathan Druart 2024-11-18 09:02:59 UTC
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>
Comment 26 Jonathan Druart 2024-11-18 09:03:03 UTC
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>
Comment 27 Jonathan Druart 2024-11-18 09:03:06 UTC
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>
Comment 28 Jonathan Druart 2024-11-18 09:03:09 UTC
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>
Comment 29 Jonathan Druart 2024-11-18 09:03:13 UTC
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>
Comment 30 Jonathan Druart 2024-11-18 09:03:16 UTC
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>
Comment 31 Jonathan Druart 2024-11-18 09:03:19 UTC
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>
Comment 32 Jonathan Druart 2024-11-18 09:03:22 UTC
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>
Comment 33 Jonathan Druart 2024-11-18 09:03:38 UTC
CONFLICT (content): Merge conflict in Koha/REST/V1/Checkouts.pm

Patches rebased.

Tests are passing for me.

I have not done any code review.
Comment 34 Marcel de Rooy 2024-11-22 07:30:31 UTC
 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)
Comment 35 Marcel de Rooy 2024-11-22 07:33:40 UTC
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
Comment 36 Victor Grousset/tuxayo 2024-12-16 21:41:20 UTC
> 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.
Comment 37 Marcel de Rooy 2025-01-17 09:43:22 UTC
Running a yarn rebuild resolves the failures indeed.
Comment 38 Marcel de Rooy 2025-01-17 09:44:49 UTC
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>
Comment 39 Marcel de Rooy 2025-01-17 09:44:52 UTC
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>
Comment 40 Marcel de Rooy 2025-01-17 09:44:55 UTC
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>
Comment 41 Marcel de Rooy 2025-01-17 09:44:58 UTC
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>
Comment 42 Marcel de Rooy 2025-01-17 09:45:00 UTC
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>
Comment 43 Marcel de Rooy 2025-01-17 09:45:03 UTC
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>
Comment 44 Marcel de Rooy 2025-01-17 09:45:06 UTC
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>
Comment 45 Marcel de Rooy 2025-01-17 09:45:09 UTC
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>
Comment 46 Marcel de Rooy 2025-01-17 09:45:12 UTC
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>
Comment 47 Alex Buckley 2025-02-04 00:41:18 UTC
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)$
Comment 48 Lari Taskula 2025-02-06 12:37:30 UTC
(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)
Comment 49 Alex Buckley 2025-02-10 03:07:09 UTC
(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.
Comment 50 Alex Buckley 2025-02-10 03:08:40 UTC
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
Comment 51 Lari Taskula 2025-02-10 13:01:33 UTC
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>
Comment 52 Lari Taskula 2025-02-10 13:01:37 UTC
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>
Comment 53 Lari Taskula 2025-02-10 13:01:40 UTC
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>
Comment 54 Lari Taskula 2025-02-10 13:01:44 UTC
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>
Comment 55 Lari Taskula 2025-02-10 13:01:47 UTC
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>
Comment 56 Lari Taskula 2025-02-10 13:01:51 UTC
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>
Comment 57 Lari Taskula 2025-02-10 13:01:54 UTC
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>
Comment 58 Lari Taskula 2025-02-10 13:01:57 UTC
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>
Comment 59 Lari Taskula 2025-02-10 13:02:00 UTC
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>
Comment 60 Lari Taskula 2025-02-10 13:02:41 UTC
(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
Comment 61 Alex Buckley 2025-02-12 02:07:08 UTC
(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!
Comment 62 Alex Buckley 2025-02-12 02:08:02 UTC
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?
Comment 63 Lari Taskula 2025-02-14 06:58:10 UTC
(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
Comment 64 Fridolin Somers 2025-02-14 14:07:11 UTC
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.
Comment 65 Fridolin Somers 2025-02-14 14:35:05 UTC
Applied to 23.11.x-security branch
Comment 66 Paul Derscheid 2025-02-22 18:19:27 UTC
Applied to 24.11.x-security
Comment 67 Jesse Maseto 2025-02-24 15:38:01 UTC
Does not apply to 22.11.x. Merge Conflicts!
Comment 68 Tomás Cohen Arazi (tcohen) 2025-02-24 17:42:30 UTC
(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!
Comment 69 Jesse Maseto 2025-02-24 19:08:12 UTC
Applied to 22.11.x-security
Comment 70 Paul Derscheid 2025-03-05 17:31:34 UTC
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>
Comment 71 Katrin Fischer 2025-03-06 12:55:59 UTC
Pushed for 25.05!

Well done everyone, thank you!