This bug will add the public facing libraries endpoint.
I'm going to grab the foundations from bug 27358 to push here so we can get those in but based on a simpler final goal with the libraries table.
Created attachment 124479 [details] [review] Bug 28948: Add a generic way to handle API privileged access attributes deny-list This patch introduces a way for Koha::Object(s)->to_api to filter out attributes that require privileged access. It is done in a way that the 'public' parameter is recursively passed to nested objects in recursive to_api() calls. This way, Koha::Object-based classes can determine how they will render depending on this parameter. For example, for implementing a route for fetching an library looks like: GET /libraries The controller will look like: my $library = Koha::Libraries->find( $c->validation->param('library_id') ); return $c->render( status => 200, openapi => $library->to_api ); Implementing an unprivileged (public) route would look like: GET /public/libraries/:library_id The controller will look like: my $library = Koha::Libraries->find( $c->validation->param('library_id') ); return $c->render( status => 200, openapi => $library->to_api({ public => 1 }) ); To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object*.t => SUCCESS: Tests pass (i.e. current behaviour is kept, new behaviour passes the tests) 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124480 [details] [review] Bug 28948: Make is_public stashed on public routes This patch makes the API authentication code stash the 'is_public' value when public routes are hit. This will be particularly useful to have $c->objects->search generically pass this info down to the ->to_api method. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass! When a public route is reached, the controller has the 'is_public' value stashed 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124481 [details] [review] Bug 28948: Teach objects.search about public requests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124482 [details] [review] Bug 28948: Add GET /public/libraries routes This patch introduces a route to fetch a list of libraries or a single library as expected on the /public namespace. It is expected to return the 'public' representation of the Koha::Library objects. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/libraries.t => SUCCESS: Test pass and they cover all the cases! 3. Try your favourite REST tool against the new route. 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124483 [details] [review] Bug 28948: (QA follow-up) Convert to allow-list This patch converts the code to use an allow-list as aposed to a deny-list. This is more 'fail safe' than requireing maintanence of a deny-list. We also switch to using db fields names for the list as aposed to api mapped names. This way, the list can be re-used for non-api related sanitising if required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
OK.. grabbed the patches from bug 27358 as agreed and reworked them for the libraries endpoints.. just minor alterations to tests to use the alternate route really.
root@kohadevbox:koha(bug28948-qa)$ prove t/db_dependent/Koha/Objects.t t/db_dependent/Koha/Objects.t .. 18/24 # Failed test at t/db_dependent/Koha/Objects.t line 1214. # Failed test at t/db_dependent/Koha/Objects.t line 1226. # Looks like you failed 2 tests of 13. t/db_dependent/Koha/Objects.t .. 20/24 # Failed test 'filter_by_last_update' # at t/db_dependent/Koha/Objects.t line 1234. # Looks like you failed 1 test of 24. t/db_dependent/Koha/Objects.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/24 subtests Test Summary Report ------------------- t/db_dependent/Koha/Objects.t (Wstat: 256 Tests: 24 Failed: 1) Failed test: 20 Non-zero exit status: 1 Files=1, Tests=24, 17 wallclock secs ( 0.05 usr 0.01 sys + 12.17 cusr 2.62 csys = 14.85 CPU) Result: FAIL
(In reply to Kyle M Hall from comment #8) > root@kohadevbox:koha(bug28948-qa)$ prove t/db_dependent/Koha/Objects.t > t/db_dependent/Koha/Objects.t .. 18/24 > # Failed test at t/db_dependent/Koha/Objects.t line 1214. > > # Failed test at t/db_dependent/Koha/Objects.t line 1226. > # Looks like you failed 2 tests of 13. > t/db_dependent/Koha/Objects.t .. 20/24 > # Failed test 'filter_by_last_update' > # at t/db_dependent/Koha/Objects.t line 1234. > # Looks like you failed 1 test of 24. > t/db_dependent/Koha/Objects.t .. Dubious, test returned 1 (wstat 256, 0x100) > Failed 1/24 subtests That's a failure in master.
Created attachment 124749 [details] [review] Bug 28948: (follow-up) Use ::Allowlist structure This patch updates the patchset to use the ::Allowlist structure introduced with bug 28935. I think we need more work here to provide for 'read' and 'write' allowlists.. we also need a way to alter the allowlist from the Koha object when required as with the load time at to_api here we currently have no way to pass overrides to the allowlist.
+ my $allowlist = Koha::Library::Allowlist->new({ interface => 'opac' })->load(); We should allow chaining then. IIRC load does not return self now.
(In reply to David Cook from comment #12) > I think that we've overloaded the allow list concept. I originally came up > with the allow list concept to function as a user input validation > allow/valid list rather than an attribute-based access control list. I am submitting shortly a generic AllowList and a Koha::Object::ColumnSet proposal on bug 28999.
Please, keep in mind that the API already has input/output validation, specified in the OpenAPI spec. That's why this dev was originally simple and only included a list of attributes for the 'public' context. Also, this allowed for easy implementation of calculated lists: frameworks telling what item structure to hide? Sysprefs? Also, keep in mind that public and staff are not the only representations we might want for resources: we could have a 'brief' representation to be used in dropdowns (id+name). I mention it, because that's where the simplicity came from. For what is worth, I think we should do use base wq(Koha::Object Koha::Object::AllowList) And have AllowList just read the library's defined lists... I mean not implementing a new AllowList class for each thing
(In reply to Tomás Cohen Arazi from comment #14) > And have AllowList just read the library's defined lists... I mean not > implementing a new AllowList class for each thing Bug 28999 centralizes attributes lists in the schema file for the table. In the ColumnSet concept.
(In reply to Marcel de Rooy from comment #15) > (In reply to Tomás Cohen Arazi from comment #14) > > And have AllowList just read the library's defined lists... I mean not > > implementing a new AllowList class for each thing > > Bug 28999 centralizes attributes lists in the schema file for the table. In > the ColumnSet concept. +1 looks interesting!
+ # FIXME: All modules should have a corresponding allowlist eventually + my $allowclass = ref($self) . '::Allowlist'; + if ( can_load( modules => { $allowclass => undef } ) ) { + my $allowlist = $allowclass->new( + { interface => $params->{public} ? 'opac' : 'staff' } ); + $allowlist->load; + my $blocked = $allowlist->apply( { input => $json_object } ); + Koha::Logger->get->debug( + ref($self) + . "::to_api allowlist blocked fields: " + . ( + join ', ', + map { $_ . '=' . $blocked->{$_} } sort keys %$blocked + ) + ) if keys %$blocked; So this would become something like (using api_staff and api_public; whats in a name): $json_object = $self->filter({ column_set => $params->{public} ? 'api_public' : 'api' }); It is just one line if you skip logging. Which is probably fine here. If we need it, the idea is now to add a dump => $dump parameter. Note that the concept is still a concept.
Created attachment 124911 [details] [review] Bug 28948: Changes
(In reply to Marcel de Rooy from comment #18) > Created attachment 124911 [details] [review] [review] > Bug 28948: Changes Not completely finished here. Other test needs adjustment still. Read something about overriding..
(In reply to Martin Renvoize from comment #10) > This patch updates the patchset to use the ::Allowlist structure > introduced with bug 28935. I think we need more work here to provide > for 'read' and 'write' allowlists.. we also need a way to alter the > allowlist from the Koha object when required as with the load time at > to_api here we currently have no way to pass overrides to the allowlist. The ColumnSet allows you to create lists for various contexts. You can make that as complex as you want. You could just create a complete attribute list for each use case initially, but surely get_column_set could return the merge of two sets or the intersection etc. $column_sets = { stuff }; $column_sets->{another1} = [ uniq( @$set1, @$set2) ]; $column_sets->{another2} = [ intersect( $set1, $set2) ]; where $set1 and $set2 refer to entries in $column_sets As to overriding, I would tend to leave that to the controller. After running the specified filter, you can always add overrides like: $data = { $obj->filter, @overrides }; The number of needed overrides may be an indication for the need of improving the defined sets.
Created attachment 124914 [details] [review] Bug 28948: Changes Touched Objects.t now too.
I elaborated on the changes of Martin, but note the following: - # Remove forbidden attributes if required - if ( $params->{public} - and $self->can('api_privileged_attrs') ) - { - foreach my $privileged_attr ( @{ $self->api_privileged_attrs } ) { - delete $json_object->{$privileged_attr}; - } - } Initially we only filtered the public side removing the api privileged keys. What we now do, is filter both public and staff ! Which is a serious difference obviously.
Did it not just move: if ( can_load( modules => { $allowclass => undef } ) ) { { my $allowlist = $allowclass->new( { interface => $params->{public} ? 'opac' : 'staff' } ); $allowlist->load;
(In reply to Martin Renvoize from comment #23) > Did it not just move: > > if ( can_load( modules => { $allowclass => undef } ) ) { > { > my $allowlist = $allowclass->new( > { interface => $params->{public} ? 'opac' : 'staff' } ); > $allowlist->load; It became more or less the ->can('get_column_set') test. But this might not be what you mean.
(In reply to Marcel de Rooy from comment #22) > Initially we only filtered the public side removing the api privileged keys. > What we now do, is filter both public and staff ! > Which is a serious difference obviously. We apply an allow list on staff too now. Tomas did not. So we are removing a few staff fields, like: | branchphone | longtext | YES | | NULL | | | issuing | tinyint(4) | YES | | NULL | | | opac_info | mediumtext | YES | | NULL | | | geolocation | varchar(255) | YES | | NULL | | | pickup_location | tinyint(1) | NO | | 1 | |
What he did, is something like: # Remove forbidden attributes if required if( $params->{public} && $self->_result->can('get_column_set') ) { #FIXME Temporary measure $json_object = $self->filter({ input => $json_object, column_set => 'public_read', log_level => 'debug' }); }
Created attachment 125901 [details] [review] Bug 28948: Add a generic way to handle API privileged access attributes deny-list This patch introduces a way for Koha::Object(s)->to_api to filter out attributes that require privileged access. It is done in a way that the 'public' parameter is recursively passed to nested objects in recursive to_api() calls. This way, Koha::Object-based classes can determine how they will render depending on this parameter. For example, for implementing a route for fetching an library looks like: GET /libraries The controller will look like: my $library = Koha::Libraries->find( $c->validation->param('library_id') ); return $c->render( status => 200, openapi => $library->to_api ); Implementing an unprivileged (public) route would look like: GET /public/libraries/:library_id The controller will look like: my $library = Koha::Libraries->find( $c->validation->param('library_id') ); return $c->render( status => 200, openapi => $library->to_api({ public => 1 }) ); To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object*.t => SUCCESS: Tests pass (i.e. current behaviour is kept, new behaviour passes the tests) 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125902 [details] [review] Bug 28948: Make is_public stashed on public routes This patch makes the API authentication code stash the 'is_public' value when public routes are hit. This will be particularly useful to have $c->objects->search generically pass this info down to the ->to_api method. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass! When a public route is reached, the controller has the 'is_public' value stashed 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125903 [details] [review] Bug 28948: Teach objects.search about public requests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125904 [details] [review] Bug 28948: Add GET /public/libraries routes This patch introduces a route to fetch a list of libraries or a single library as expected on the /public namespace. It is expected to return the 'public' representation of the Koha::Library objects. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/libraries.t => SUCCESS: Test pass and they cover all the cases! 3. Try your favourite REST tool against the new route. 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125905 [details] [review] Bug 28948: (QA follow-up) Convert to allow-list This patch converts the code to use an allow-list as aposed to a deny-list. This is more 'fail safe' than requireing maintanence of a deny-list. We also switch to using db fields names for the list as aposed to api mapped names. This way, the list can be re-used for non-api related sanitising if required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125906 [details] [review] Bug 28948: Remove FIXME This patch reproduces what we did for `to_api_mapping`: make it always present on Koha::Object classes. This has the side-effect of... making things more secure! Before this patch, if undefined, all attributes were returned. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125907 [details] [review] Bug 28948: Remove query params, 'q' param covers everything needed
Created attachment 125908 [details] [review] Bug 28948: Remove query params, 'q' param covers everything needed
Created attachment 125911 [details] [review] Bug 28948: Remove query params, 'q' param covers everything needed
Created attachment 125912 [details] [review] Bug 28948: Don't require catalogue permission for public route, don't allow smtp server embed
The route /public/libraries/{library_id} needs to be implemented.
Created attachment 126111 [details] [review] Bug 28948: Add a generic way to handle API privileged access attributes deny-list This patch introduces a way for Koha::Object(s)->to_api to filter out attributes that require privileged access. It is done in a way that the 'public' parameter is recursively passed to nested objects in recursive to_api() calls. This way, Koha::Object-based classes can determine how they will render depending on this parameter. For example, for implementing a route for fetching an library looks like: GET /libraries The controller will look like: my $library = Koha::Libraries->find( $c->validation->param('library_id') ); return $c->render( status => 200, openapi => $library->to_api ); Implementing an unprivileged (public) route would look like: GET /public/libraries/:library_id The controller will look like: my $library = Koha::Libraries->find( $c->validation->param('library_id') ); return $c->render( status => 200, openapi => $library->to_api({ public => 1 }) ); To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object*.t => SUCCESS: Tests pass (i.e. current behaviour is kept, new behaviour passes the tests) 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126112 [details] [review] Bug 28948: Make is_public stashed on public routes This patch makes the API authentication code stash the 'is_public' value when public routes are hit. This will be particularly useful to have $c->objects->search generically pass this info down to the ->to_api method. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass! When a public route is reached, the controller has the 'is_public' value stashed 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126113 [details] [review] Bug 28948: Teach objects.search about public requests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126114 [details] [review] Bug 28948: Add GET /public/libraries routes This patch introduces a route to fetch a list of libraries or a single library as expected on the /public namespace. It is expected to return the 'public' representation of the Koha::Library objects. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/libraries.t => SUCCESS: Test pass and they cover all the cases! 3. Try your favourite REST tool against the new route. 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126115 [details] [review] Bug 28948: (QA follow-up) Convert to allow-list This patch converts the code to use an allow-list as aposed to a deny-list. This is more 'fail safe' than requireing maintanence of a deny-list. We also switch to using db fields names for the list as aposed to api mapped names. This way, the list can be re-used for non-api related sanitising if required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126116 [details] [review] Bug 28948: Remove FIXME This patch reproduces what we did for `to_api_mapping`: make it always present on Koha::Object classes. This has the side-effect of... making things more secure! Before this patch, if undefined, all attributes were returned. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126117 [details] [review] Bug 28948: Remove query params, 'q' param covers everything needed Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126118 [details] [review] Bug 28948: Don't require catalogue permission for public route, don't allow smtp server embed Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Issues fixed and squashed into original patches to save tiny typo fix commits Back to signed off as I ran through all the tests again and am happy it all appears to be working.
Created attachment 126119 [details] [review] Bug 28948: Add a generic way to handle API privileged access attributes deny-list This patch introduces a way for Koha::Object(s)->to_api to filter out attributes that require privileged access. It is done in a way that the 'public' parameter is recursively passed to nested objects in recursive to_api() calls. This way, Koha::Object-based classes can determine how they will render depending on this parameter. For example, for implementing a route for fetching an library looks like: GET /libraries The controller will look like: my $library = Koha::Libraries->find( $c->validation->param('library_id') ); return $c->render( status => 200, openapi => $library->to_api ); Implementing an unprivileged (public) route would look like: GET /public/libraries/:library_id The controller will look like: my $library = Koha::Libraries->find( $c->validation->param('library_id') ); return $c->render( status => 200, openapi => $library->to_api({ public => 1 }) ); To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object*.t => SUCCESS: Tests pass (i.e. current behaviour is kept, new behaviour passes the tests) 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 126120 [details] [review] Bug 28948: Make is_public stashed on public routes This patch makes the API authentication code stash the 'is_public' value when public routes are hit. This will be particularly useful to have $c->objects->search generically pass this info down to the ->to_api method. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass! When a public route is reached, the controller has the 'is_public' value stashed 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 126121 [details] [review] Bug 28948: Teach objects.search about public requests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 126122 [details] [review] Bug 28948: Add GET /public/libraries routes This patch introduces a route to fetch a list of libraries or a single library as expected on the /public namespace. It is expected to return the 'public' representation of the Koha::Library objects. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/libraries.t => SUCCESS: Test pass and they cover all the cases! 3. Try your favourite REST tool against the new route. 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 126123 [details] [review] Bug 28948: (QA follow-up) Convert to allow-list This patch converts the code to use an allow-list as aposed to a deny-list. This is more 'fail safe' than requireing maintanence of a deny-list. We also switch to using db fields names for the list as aposed to api mapped names. This way, the list can be re-used for non-api related sanitising if required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 126124 [details] [review] Bug 28948: Remove FIXME This patch reproduces what we did for `to_api_mapping`: make it always present on Koha::Object classes. This has the side-effect of... making things more secure! Before this patch, if undefined, all attributes were returned. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 126125 [details] [review] Bug 28948: Remove query params, 'q' param covers everything needed Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 126126 [details] [review] Bug 28948: Don't require catalogue permission for public route, don't allow smtp server embed Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 127054 [details] [review] Bug 28948: Fix random failure This patch makes the query for randomly generated libraries deterministic, thus getting rid of the random tests failures. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 127055 [details] [review] Bug 28948: Fix tests t/db_dependent/Koha/REST/Plugin/Objects.t .............. 12/13 # Failed test 'Public representation of $library_1 is retrieved' # at t/db_dependent/Koha/REST/Plugin/Objects.t line 546. # Structures begin differing at: # $got->{country} = 'cCaL3l' # $expected->{country} = 'jcxuV1zl' # Failed test 'Public representation of $library_2 is retrieved' # at t/db_dependent/Koha/REST/Plugin/Objects.t line 546. # Structures begin differing at: # $got->{state} = 'QVAutHLBs' # $expected->{state} = 'hZotjENErQ' # Looks like you failed 2 tests of 3.
Tests are failing for me t/db_dependent/api/v1/auth_authenticate_api_request.t .. 1/6 # Failed test '200 OK' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 131. # got: '401' # expected: '200' # Failed test 'The 'koha.user' object is defined in the stash' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 134. # Failed test 'userenv set correctly' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 137. # got: undef # expected: '382' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 154. # Structures begin differing at: # $got->{error} = 'Session has been expired.' # $expected->{error} = 'Authentication failure.' # Looks like you failed 1 test of 3. # Failed test 'logged-out tests' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 157. # Looks like you planned 8 tests but ran 6. # Looks like you failed 4 tests of 6 run. # Failed test 'cookie-based tests' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 160. Rebased version of the patches at https://gitlab.com/joubu/Koha/-/tree/bug_28948
(In reply to Jonathan Druart from comment #57) > Tests are failing for me Not related, see bug 29353.
Pushed to master for 21.11, thanks to everybody involved!