Bug 24067

Summary: Refactor REST API allow-owner authorization logic
Product: Koha Reporter: Lari Taskula <lari.taskula>
Component: REST APIAssignee: Bugs List <koha-bugs>
Status: Patch doesn't apply --- QA Contact:
Severity: enhancement    
Priority: P5 - low CC: black23, david, katrin.fischer, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 24067: Add new subroutine check_resource_ownership
Bug 24067: Use check_resource_ownership in authorization
Bug 24067: Remove check_object_ownership and old authorization methods
Bug 24067: Use owner_authorization in POST /public/patrons/{patron_id}/password
Bug 24067: Use owner_authorization in current public endpoints
Bug 24067: Use owner_authorization in current public endpoints

Description Lari Taskula 2019-11-19 14:10:43 UTC
It is possible to authorize patron an access to their own resources by defining "allow-owner" under x-koha-authorization block in endpoint's OpenAPI specification.

Currently the logic for this type of authorization is centralized under Koha::REST::V1::Auth::check_object_ownership with some messy, hard to understand and possibly at some point in future even insecure logic.

A better and more flexible solution is to have each REST API controller handle the "allow-owner" authorization on their own.
Comment 1 Lari Taskula 2019-11-19 14:36:37 UTC
Created attachment 95553 [details] [review]
Bug 24067: Add new subroutine check_resource_ownership

This patch adds a new subroutine called check_resource_ownership.

The method is called when API consumer has no permission but the request may
still be authorized if the user is accessing only their own resources. In
this case, it will find a method called "owner_authorization" from the controller
specified by the endpoint.

Testable in following patches.

Sponsored-by: Koha-Suomi Oy
Comment 2 Lari Taskula 2019-11-19 14:36:48 UTC
Created attachment 95554 [details] [review]
Bug 24067: Use check_resource_ownership in authorization

To test:
1. prove t/db_dependent/api/v1/patrons_password.t
2. Observe failure (yes, really a failure - tests will pass in later test)

Looks like you failed 7 tests of 15.
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 235.
 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

Sponsored-by: Koha-Suomi Oy
Comment 3 Lari Taskula 2019-11-19 14:36:59 UTC
Created attachment 95555 [details] [review]
Bug 24067: Remove check_object_ownership and old authorization methods

Sponsored-by: Koha-Suomi Oy
Comment 4 Lari Taskula 2019-11-19 14:37:23 UTC
Created attachment 95556 [details] [review]
Bug 24067: Use owner_authorization in POST /public/patrons/{patron_id}/password

As an example, use this functionality in Koha::REST::V1::Patrons::Password

To test:
1. prove t/db_dependent/api/v1/patrons_password.t
2. Observe success

Sponsored-by: Koha-Suomi Oy
Comment 5 Lari Taskula 2019-11-19 14:50:06 UTC
Created attachment 95561 [details] [review]
Bug 24067: Use owner_authorization in current public endpoints

To test:
1. prove t/db_dependent/api/v1/patrons_password.t
2. Observe success
3. prove t/db_dependent/api/v1/patrons.t
4. Observe success

Sponsored-by: Koha-Suomi Oy
Comment 6 Lari Taskula 2019-11-19 15:18:59 UTC
Created attachment 95571 [details] [review]
Bug 24067: Use owner_authorization in current public endpoints

To test:
1. prove t/db_dependent/api/v1/patrons_password.t
2. Observe success
3. prove t/db_dependent/api/v1/patrons.t
4. Observe success

Sponsored-by: Koha-Suomi Oy
Comment 7 Michal Denar 2020-03-08 21:53:16 UTC
Hi Lari,
can you rebase this patch?

Thank you.
Comment 8 David Nind 2020-04-04 21:57:07 UTC
Patch no longer applies 8-(:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24067: Add new subroutine check_resource_ownership
Applying: Bug 24067: Use check_resource_ownership in authorization
Applying: Bug 24067: Remove check_object_ownership and old authorization methods
Applying: Bug 24067: Use owner_authorization in current public endpoints
Using index info to reconstruct a base tree...
M	Koha/REST/V1/Patrons.pm
M	Koha/REST/V1/Patrons/Password.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/REST/V1/Patrons/Password.pm
Auto-merging Koha/REST/V1/Patrons.pm
CONFLICT (content): Merge conflict in Koha/REST/V1/Patrons.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 24067: Use owner_authorization in current public endpoints
Comment 9 Lari Taskula 2020-04-07 15:09:33 UTC
(In reply to Michal Denar from comment #7)
> Hi Lari,
> can you rebase this patch?
> 
> Thank you.

Sorry I don't currently have the resources to work with this Bug. But if you can and want to help, please!