Bugzilla – Attachment 172442 Details for
Bug 28907
Potential unauthorized access in public REST routes
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28907: REST - Drop support for allow-owner functionality
Bug-28907-REST---Drop-support-for-allow-owner-func.patch (text/plain), 6.81 KB, created by
Victor Grousset/tuxayo
on 2024-10-07 01:06:51 UTC
(
hide
)
Description:
Bug 28907: REST - Drop support for allow-owner functionality
Filename:
MIME Type:
Creator:
Victor Grousset/tuxayo
Created:
2024-10-07 01:06:51 UTC
Size:
6.81 KB
patch
obsolete
>From cb66742bfa5fcca5a6fa9ab78972baf394bc38b7 Mon Sep 17 00:00:00 2001 >From: Lari Taskula <lari.taskula@hypernova.fi> >Date: Wed, 18 Sep 2024 13:35:06 +0000 >Subject: [PATCH] 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> >--- > Koha/REST/V1/Auth.pm | 156 +------------------------------------------ > 1 file changed, 1 insertion(+), 155 deletions(-) > >diff --git a/Koha/REST/V1/Auth.pm b/Koha/REST/V1/Auth.pm >index c10e81cf8a..b63d4e0140 100644 >--- a/Koha/REST/V1/Auth.pm >+++ b/Koha/REST/V1/Auth.pm >@@ -319,9 +319,7 @@ sub authenticate_api_request { > > my $permissions = $authorization->{'permissions'}; > # Check if the user is authorized >- if ( ( defined($permissions) and haspermission($user->userid, $permissions) ) >- or allow_owner($c, $authorization, $user) >- or allow_guarantor($c, $authorization, $user) ) { >+ if ( ( defined($permissions) and haspermission($user->userid, $permissions) ) ) { > > validate_query_parameters( $c, $spec ); > >@@ -360,158 +358,6 @@ sub validate_query_parameters { > ) if @errors; > } > >-=head3 allow_owner >- >-Allows access to object for its owner. >- >-There are endpoints that should allow access for the object owner even if they >-do not have the required permission, e.g. access an own reserve. This can be >-achieved by defining the operation as follows: >- >-"/holds/{reserve_id}": { >- "get": { >- ..., >- "x-koha-authorization": { >- "allow-owner": true, >- "permissions": { >- "borrowers": "1" >- } >- } >- } >-} >- >-=cut >- >-sub allow_owner { >- my ($c, $authorization, $user) = @_; >- >- return unless $authorization->{'allow-owner'}; >- >- return check_object_ownership($c, $user) if $user and $c; >-} >- >-=head3 allow_guarantor >- >-Same as "allow_owner", but checks if the object is owned by one of C<$user>'s >-guarantees. >- >-=cut >- >-sub allow_guarantor { >- my ($c, $authorization, $user) = @_; >- >- if (!$c || !$user || !$authorization || !$authorization->{'allow-guarantor'}){ >- return; >- } >- >- my $guarantees = $user->guarantee_relationships->guarantees->as_list; >- foreach my $guarantee (@{$guarantees}) { >- return 1 if check_object_ownership($c, $guarantee); >- } >-} >- >-=head3 check_object_ownership >- >-Determines ownership of an object from request parameters. >- >-As introducing an endpoint that allows access for object's owner; if the >-parameter that will be used to determine ownership is not already inside >-$parameters, add a new subroutine that checks the ownership and extend >-$parameters to contain a key with parameter_name and a value of a subref to >-the subroutine that you created. >- >-=cut >- >-sub check_object_ownership { >- my ($c, $user) = @_; >- >- return if not $c or not $user; >- >- my $parameters = { >- accountlines_id => \&_object_ownership_by_accountlines_id, >- borrowernumber => \&_object_ownership_by_patron_id, >- patron_id => \&_object_ownership_by_patron_id, >- checkout_id => \&_object_ownership_by_checkout_id, >- reserve_id => \&_object_ownership_by_reserve_id, >- }; >- >- foreach my $param ( keys %{ $parameters } ) { >- my $check_ownership = $parameters->{$param}; >- if ($c->stash($param)) { >- return &$check_ownership($c, $user, $c->stash($param)); >- } >- elsif ($c->param($param)) { >- return &$check_ownership($c, $user, $c->param($param)); >- } >- elsif ($c->match->stack->[-1]->{$param}) { >- return &$check_ownership($c, $user, $c->match->stack->[-1]->{$param}); >- } >- elsif ($c->req->json && $c->req->json->{$param}) { >- return 1 if &$check_ownership($c, $user, $c->req->json->{$param}); >- } >- } >-} >- >-=head3 _object_ownership_by_accountlines_id >- >-Finds a Koha::Account::Line-object by C<$accountlines_id> and checks if it >-belongs to C<$user>. >- >-=cut >- >-sub _object_ownership_by_accountlines_id { >- my ($c, $user, $accountlines_id) = @_; >- >- my $accountline = Koha::Account::Lines->find($accountlines_id); >- return $accountline && $user->borrowernumber == $accountline->borrowernumber; >-} >- >-=head3 _object_ownership_by_borrowernumber >- >-Compares C<$borrowernumber> to currently logged in C<$user>. >- >-=cut >- >-sub _object_ownership_by_patron_id { >- my ($c, $user, $patron_id) = @_; >- >- return $user->borrowernumber == $patron_id; >-} >- >-=head3 _object_ownership_by_checkout_id >- >-First, attempts to find a Koha::Checkout-object by C<$issue_id>. If we find one, >-compare its borrowernumber to currently logged in C<$user>. However, if an issue >-is not found, attempt to find a Koha::Old::Checkout-object instead and compare its >-borrowernumber to currently logged in C<$user>. >- >-=cut >- >-sub _object_ownership_by_checkout_id { >- my ($c, $user, $issue_id) = @_; >- >- my $issue = Koha::Checkouts->find($issue_id); >- $issue = Koha::Old::Checkouts->find($issue_id) unless $issue; >- return $issue && $issue->borrowernumber >- && $user->borrowernumber == $issue->borrowernumber; >-} >- >-=head3 _object_ownership_by_reserve_id >- >-Finds a Koha::Hold-object by C<$reserve_id> and checks if it >-belongs to C<$user>. >- >-TODO: Also compare against old_reserves >- >-=cut >- >-sub _object_ownership_by_reserve_id { >- my ($c, $user, $reserve_id) = @_; >- >- my $reserve = Koha::Holds->find($reserve_id); >- return $reserve && $user->borrowernumber == $reserve->borrowernumber; >-} >- > =head3 _basic_auth > > Internal method that performs Basic authentication. >-- >2.46.2
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 28907
:
171666
|
171667
|
171668
|
171669
|
171670
|
171671
|
171672
|
171673
|
171674
|
172434
|
172435
|
172436
|
172437
|
172438
|
172439
|
172440
|
172441
|
172442
|
174662
|
174663
|
174664
|
174665
|
174666
|
174667
|
174668
|
174669
|
174670
|
176708
|
176709
|
176710
|
176711
|
176712
|
176713
|
176714
|
176715
|
176716
|
177679
|
177680
|
177681
|
177682
|
177683
|
177684
|
177685
|
177686
|
177687
|
179000