Bug 28948 - Add a /public counterpart for the libraries REST endpoints
Summary: Add a /public counterpart for the libraries REST endpoints
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 28965 27358 29593
  Show dependency treegraph
 
Reported: 2021-09-03 12:04 UTC by Martin Renvoize
Modified: 2021-11-29 18:28 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 28948: Add a generic way to handle API privileged access attributes deny-list (8.55 KB, patch)
2021-09-03 15:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Make is_public stashed on public routes (2.48 KB, patch)
2021-09-03 15:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Teach objects.search about public requests (2.83 KB, patch)
2021-09-03 15:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Add GET /public/libraries routes (9.18 KB, patch)
2021-09-03 15:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: (QA follow-up) Convert to allow-list (11.51 KB, patch)
2021-09-03 15:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: (follow-up) Use ::Allowlist structure (5.79 KB, patch)
2021-09-10 10:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Changes (6.59 KB, patch)
2021-09-16 07:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28948: Changes (10.70 KB, patch)
2021-09-16 08:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28948: Add a generic way to handle API privileged access attributes deny-list (8.55 KB, patch)
2021-10-07 18:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28948: Make is_public stashed on public routes (2.48 KB, patch)
2021-10-07 18:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28948: Teach objects.search about public requests (2.95 KB, patch)
2021-10-07 18:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28948: Add GET /public/libraries routes (9.24 KB, patch)
2021-10-07 18:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28948: (QA follow-up) Convert to allow-list (11.50 KB, patch)
2021-10-07 18:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28948: Remove FIXME (1.79 KB, patch)
2021-10-07 18:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28948: Remove query params, 'q' param covers everything needed (4.47 KB, patch)
2021-10-07 18:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Remove query params, 'q' param covers everything needed (8.26 KB, patch)
2021-10-07 18:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Remove query params, 'q' param covers everything needed (4.33 KB, patch)
2021-10-07 18:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Don't require catalogue permission for public route, don't allow smtp server embed (923 bytes, patch)
2021-10-07 18:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Add a generic way to handle API privileged access attributes deny-list (8.55 KB, patch)
2021-10-12 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Make is_public stashed on public routes (2.48 KB, patch)
2021-10-12 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Teach objects.search about public requests (2.95 KB, patch)
2021-10-12 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Add GET /public/libraries routes (9.14 KB, patch)
2021-10-12 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: (QA follow-up) Convert to allow-list (11.50 KB, patch)
2021-10-12 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Remove FIXME (1.85 KB, patch)
2021-10-12 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Remove query params, 'q' param covers everything needed (4.39 KB, patch)
2021-10-12 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Don't require catalogue permission for public route, don't allow smtp server embed (986 bytes, patch)
2021-10-12 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28948: Add a generic way to handle API privileged access attributes deny-list (8.61 KB, patch)
2021-10-12 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Make is_public stashed on public routes (2.54 KB, patch)
2021-10-12 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Teach objects.search about public requests (3.01 KB, patch)
2021-10-12 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Add GET /public/libraries routes (9.20 KB, patch)
2021-10-12 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: (QA follow-up) Convert to allow-list (11.57 KB, patch)
2021-10-12 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Remove FIXME (1.91 KB, patch)
2021-10-12 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Remove query params, 'q' param covers everything needed (4.45 KB, patch)
2021-10-12 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Don't require catalogue permission for public route, don't allow smtp server embed (1.02 KB, patch)
2021-10-12 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28948: Fix random failure (1.73 KB, patch)
2021-10-28 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28948: Fix tests (2.18 KB, patch)
2021-10-28 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-09-03 12:04:08 UTC
This bug will add the public facing libraries endpoint.
Comment 1 Martin Renvoize 2021-09-03 12:05:10 UTC
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.
Comment 2 Martin Renvoize 2021-09-03 15:08:43 UTC
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>
Comment 3 Martin Renvoize 2021-09-03 15:08:46 UTC
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>
Comment 4 Martin Renvoize 2021-09-03 15:08:49 UTC
Created attachment 124481 [details] [review]
Bug 28948: Teach objects.search about public requests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-09-03 15:08:53 UTC
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>
Comment 6 Martin Renvoize 2021-09-03 15:08:56 UTC
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>
Comment 7 Martin Renvoize 2021-09-03 15:12:41 UTC
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.
Comment 8 Kyle M Hall 2021-09-07 13:33:19 UTC
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
Comment 9 Tomás Cohen Arazi 2021-09-07 22:56:42 UTC
(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.
Comment 10 Martin Renvoize 2021-09-10 10:24:01 UTC
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.
Comment 11 Marcel de Rooy 2021-09-10 14:41:19 UTC
+        my $allowlist = Koha::Library::Allowlist->new({ interface => 'opac' })->load();

We should allow chaining then. IIRC load does not return self now.
Comment 13 Marcel de Rooy 2021-09-13 06:00:48 UTC
(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.
Comment 14 Tomás Cohen Arazi 2021-09-13 09:17:59 UTC
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
Comment 15 Marcel de Rooy 2021-09-13 09:37:50 UTC
(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.
Comment 16 Tomás Cohen Arazi 2021-09-14 11:46:38 UTC
(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!
Comment 17 Marcel de Rooy 2021-09-14 14:47:29 UTC
+    # 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.
Comment 18 Marcel de Rooy 2021-09-16 07:10:37 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2021-09-16 07:11:30 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2021-09-16 08:05:44 UTC
(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.
Comment 21 Marcel de Rooy 2021-09-16 08:47:07 UTC
Created attachment 124914 [details] [review]
Bug 28948: Changes

Touched Objects.t now too.
Comment 22 Marcel de Rooy 2021-09-16 08:58:26 UTC
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.
Comment 23 Martin Renvoize 2021-09-16 09:02:46 UTC
Did it not just move:

if ( can_load( modules => { $allowclass => undef } ) ) {
    {		
        my $allowlist = $allowclass->new(
            { interface => $params->{public} ? 'opac' : 'staff' } );
        $allowlist->load;
Comment 24 Marcel de Rooy 2021-09-16 09:12:03 UTC
(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.
Comment 25 Marcel de Rooy 2021-09-16 09:16:12 UTC
(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       |       |
Comment 26 Marcel de Rooy 2021-09-16 09:18:46 UTC
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' });
    }
Comment 27 Tomás Cohen Arazi 2021-10-07 18:04:52 UTC
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>
Comment 28 Tomás Cohen Arazi 2021-10-07 18:04:56 UTC
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>
Comment 29 Tomás Cohen Arazi 2021-10-07 18:05:00 UTC
Created attachment 125903 [details] [review]
Bug 28948: Teach objects.search about public requests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Tomás Cohen Arazi 2021-10-07 18:05:04 UTC
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>
Comment 31 Tomás Cohen Arazi 2021-10-07 18:05:08 UTC
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>
Comment 32 Tomás Cohen Arazi 2021-10-07 18:05:14 UTC
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>
Comment 33 Kyle M Hall 2021-10-07 18:39:51 UTC
Created attachment 125907 [details] [review]
Bug 28948: Remove query params, 'q' param covers everything needed
Comment 34 Kyle M Hall 2021-10-07 18:42:47 UTC
Created attachment 125908 [details] [review]
Bug 28948: Remove query params, 'q' param covers everything needed
Comment 35 Kyle M Hall 2021-10-07 18:44:01 UTC
Created attachment 125911 [details] [review]
Bug 28948: Remove query params, 'q' param covers everything needed
Comment 36 Kyle M Hall 2021-10-07 18:47:09 UTC
Created attachment 125912 [details] [review]
Bug 28948: Don't require catalogue permission for public route, don't allow smtp server embed
Comment 37 Kyle M Hall 2021-10-07 18:54:51 UTC
The route /public/libraries/{library_id} needs to be implemented.
Comment 38 Martin Renvoize 2021-10-12 16:10:21 UTC
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>
Comment 39 Martin Renvoize 2021-10-12 16:10:25 UTC
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>
Comment 40 Martin Renvoize 2021-10-12 16:10:29 UTC
Created attachment 126113 [details] [review]
Bug 28948: Teach objects.search about public requests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize 2021-10-12 16:10:33 UTC
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>
Comment 42 Martin Renvoize 2021-10-12 16:10:39 UTC
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>
Comment 43 Martin Renvoize 2021-10-12 16:10:43 UTC
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>
Comment 44 Martin Renvoize 2021-10-12 16:10:47 UTC
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>
Comment 45 Martin Renvoize 2021-10-12 16:10:52 UTC
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>
Comment 46 Martin Renvoize 2021-10-12 16:12:54 UTC
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.
Comment 47 Kyle M Hall 2021-10-12 18:45:19 UTC
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>
Comment 48 Kyle M Hall 2021-10-12 18:45:29 UTC
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>
Comment 49 Kyle M Hall 2021-10-12 18:45:33 UTC
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>
Comment 50 Kyle M Hall 2021-10-12 18:45:37 UTC
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>
Comment 51 Kyle M Hall 2021-10-12 18:45:41 UTC
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>
Comment 52 Kyle M Hall 2021-10-12 18:45:44 UTC
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>
Comment 53 Kyle M Hall 2021-10-12 18:45:48 UTC
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>
Comment 54 Kyle M Hall 2021-10-12 18:45:52 UTC
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>
Comment 55 Tomás Cohen Arazi 2021-10-28 12:33:51 UTC
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>
Comment 56 Jonathan Druart 2021-10-28 13:17:18 UTC
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.
Comment 57 Jonathan Druart 2021-10-28 13:20:34 UTC
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
Comment 58 Jonathan Druart 2021-10-28 14:16:27 UTC
(In reply to Jonathan Druart from comment #57)
> Tests are failing for me

Not related, see bug 29353.
Comment 59 Jonathan Druart 2021-10-28 15:44:59 UTC
Pushed to master for 21.11, thanks to everybody involved!