Bug 24067 - Refactor REST API allow-owner authorization logic
Summary: Refactor REST API allow-owner authorization logic
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-19 14:10 UTC by Lari Taskula
Modified: 2020-04-07 15:09 UTC (History)
4 users (show)

See Also:
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 (2.25 KB, patch)
2019-11-19 14:36 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24067: Use check_resource_ownership in authorization (2.12 KB, patch)
2019-11-19 14:36 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24067: Remove check_object_ownership and old authorization methods (3.83 KB, patch)
2019-11-19 14:36 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24067: Use owner_authorization in POST /public/patrons/{patron_id}/password (1.02 KB, patch)
2019-11-19 14:37 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24067: Use owner_authorization in current public endpoints (1.77 KB, patch)
2019-11-19 14:50 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24067: Use owner_authorization in current public endpoints (1.78 KB, patch)
2019-11-19 15:18 UTC, Lari Taskula
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!