Let's add a public route to get items. This requires a way to filter out attributes that are not meant for public access.
I will need advise on the fields to hide for public access :-D
Created attachment 114922 [details] [review] Bug 27358: 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 item looks like: GET /items The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->to_api ); Implementing an unprivileged (public) route would look like: GET /public/items/:item_id The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->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
Created attachment 114935 [details] [review] Bug 27358: 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 item looks like: GET /items The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->to_api ); Implementing an unprivileged (public) route would look like: GET /public/items/:item_id The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->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
Created attachment 114936 [details] [review] Bug 27358: 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
Created attachment 114937 [details] [review] Bug 27358: Teach objects.search about public requests
Created attachment 114938 [details] [review] Bug 27358: Add GET /public/biblios/:biblio_id/items This patch introduces a route to fetch items belonging to a biblio. It is expected to return the 'public' representation of the Koha::Item objects. It is also enforcing the visibility rules, by using Koha::Items->filter_by_visible_in_opac. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Test pass and they cover all the cases! 3. Try your favourite REST tool against the new route. 4. Sign off :-D
Created attachment 114939 [details] [review] Bug 27358: Unit tests for public items retrieval
Hi Tomas, if I understood correctly you wanted me to check on what fields should not be exposed on the public route? One is easy: itemnotes_nonpublic We also have libraries requesting to hide the acq related information (price fields, booksellerid, etc.) and other fields describing internal processes too. Maybe it would be easier to work backwards on what fields make the most sense to expose? Database fields (not the names for the API) itemnumber biblionumber homebranch holdingbranch location collectioncode itemcallnumber copynumber enumchron barcode dateaccessioned (for finding new things) itemnotes onloan uri itype Not sure how useful these would be as they require a solution for resolving the internal codes into something more useful: notforloan damaged itemlost withdrawn restricted It might make more sense to replace these by a more general availability route? I think we had done some work to specify things like "can be renewed", "can be checked out", "can be placed on hold" etc. A more difficult one: sometimes strictly internal, sometimes visible in the OPAC: materials How will we be handling more_subfields_xml? As it can contain about anything, probably best to hide by default. Could it make sense to fall back on the "visible in OPAC" setting in the frameworks here?
Hi Katrina, I like both ideas: - A core list of fields that should be returned - Filter the rest out based on the frameworks About more_subfields_xml, they are not present in the current implementation. But we could do something about that too. with 'x-koha-embed' and a method that translates the XML into a hash, and filtering them using the frameworks.
This looks pretty clever and lays a nice foundation for other routes of a similar nature. Testing :)
Created attachment 116187 [details] [review] Bug 27358: 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 item looks like: GET /items The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->to_api ); Implementing an unprivileged (public) route would look like: GET /public/items/:item_id The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->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 116188 [details] [review] Bug 27358: 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 116189 [details] [review] Bug 27358: Teach objects.search about public requests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 116190 [details] [review] Bug 27358: Add GET /public/biblios/:biblio_id/items This patch introduces a route to fetch items belonging to a biblio. It is expected to return the 'public' representation of the Koha::Item objects. It is also enforcing the visibility rules, by using Koha::Items->filter_by_visible_in_opac. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.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 116191 [details] [review] Bug 27358: Unit tests for public items retrieval Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This works well and I like it.. signing off.. However, I have one security/qa comment... I think it would be best to reverse the logic here.. Instead of the `BlockList` approach, we have with `api_privileged_attrs` I think we should instead implement an `AllowList` to 'fail safe' if someone forgets to add a field to the list: `api_public_atts` perhaps?
I am a little worried about the short list here: +sub api_privileged_attrs { + return [ + 'checked_out_date', + 'checkouts_count', + 'holds_count', + 'internal_notes', + 'extended_subfields', + ]; +} + Can you help me? Just wondering if it also uses the framework visibility, then I'd be happy already :)
Hmmm, I hadn't considered the framework visibility at all... I suppose that could work with the infrastructure here.. I still think we should convert the generic handling to be an AllowList approach, but the list method could easily be looked up from the frameworks... I think?
Created attachment 123843 [details] [review] Bug 27358: 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 item looks like: GET /items The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->to_api ); Implementing an unprivileged (public) route would look like: GET /public/items/:item_id The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->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 123844 [details] [review] Bug 27358: 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 123845 [details] [review] Bug 27358: Teach objects.search about public requests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123846 [details] [review] Bug 27358: Add GET /public/biblios/:biblio_id/items This patch introduces a route to fetch items belonging to a biblio. It is expected to return the 'public' representation of the Koha::Item objects. It is also enforcing the visibility rules, by using Koha::Items->filter_by_visible_in_opac. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.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 123847 [details] [review] Bug 27358: Unit tests for public items retrieval Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Katrin Fischer from comment #17) > I am a little worried about the short list here: > > +sub api_privileged_attrs { > + return [ > + 'checked_out_date', > + 'checkouts_count', > + 'holds_count', > + 'internal_notes', > + 'extended_subfields', > + ]; > +} > + > > Can you help me? Just wondering if it also uses the framework visibility, > then I'd be happy already :) If we leave more_subfields_xml/frameworks out of the item representation (we have plans for that), would y'all help me refine this deny-list for the items? I have just rebased this work and it still works nicely. If I don't get feedback in a few days, I will move the 'public' layer work to another (simpler) table, so other devs see the benefit from this and can work on top of it. My feeling is we can have a list of 'hidden in opac' attributes from the 'items' table, and then we can sort visibility in the views. I might be wrong, though. Looking for feedback. Please PM me if you feel like there's a good use case that could be simpler than this (I'm thinking accountlines).
(In reply to Tomás Cohen Arazi from comment #24) > (In reply to Katrin Fischer from comment #17) > > I am a little worried about the short list here: > > > > +sub api_privileged_attrs { > > + return [ > > + 'checked_out_date', > > + 'checkouts_count', > > + 'holds_count', > > + 'internal_notes', > > + 'extended_subfields', > > + ]; > > +} > > + > > > > Can you help me? Just wondering if it also uses the framework visibility, > > then I'd be happy already :) > > If we leave more_subfields_xml/frameworks out of the item representation (we > have plans for that), would y'all help me refine this deny-list for the > items? I still think we should switch from 'deny-list' to 'allow-list'.. security by default ;) > > I have just rebased this work and it still works nicely. If I don't get > feedback in a few days, I will move the 'public' layer work to another > (simpler) table, so other devs see the benefit from this and can work on top > of it. Hmm, I don't think it would be a bad idea to move the core idea to another, simpler, endpoint/table so other work can be based upon it. > > My feeling is we can have a list of 'hidden in opac' attributes from the > 'items' table, and then we can sort visibility in the views. I might be > wrong, though. Looking for feedback. Hmm, not sure I understand this one.. do you mean expose fields in the API and only use the 'hidden in opac' options for the final display.. I can see a use case for that.. but I can also see people complaining that some hidden fields are still publically available if you know how to use the API. > > Please PM me if you feel like there's a good use case that could be simpler > than this (I'm thinking accountlines). Accountlines could work.. though I still have a way to go regarding the api's there. Questions like '/credits vs /debits vs /lines' and how embeds should work for offsets and things.
Created attachment 124454 [details] [review] Bug 27358: 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 item looks like: GET /items The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->to_api ); Implementing an unprivileged (public) route would look like: GET /public/items/:item_id The controller will look like: my $item = Koha::Items->find( $c->validation->param('item_id') ); return $c->render( status => 200, openapi => $item->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 124455 [details] [review] Bug 27358: 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 124456 [details] [review] Bug 27358: Teach objects.search about public requests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124457 [details] [review] Bug 27358: Add GET /public/biblios/:biblio_id/items This patch introduces a route to fetch items belonging to a biblio. It is expected to return the 'public' representation of the Koha::Item objects. It is also enforcing the visibility rules, by using Koha::Items->filter_by_visible_in_opac. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.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 124458 [details] [review] Bug 27358: Unit tests for public items retrieval Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124459 [details] [review] Bug 27358: (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>
Cloned the foundational code to bug 28948 so we can keep this public allow list stuff moving. Lets come back to this once that's pushed and just add the API route and additional filter by framework visibility stuff in this one.
Created attachment 126156 [details] [review] Bug 27358: Add GET /public/biblios/:biblio_id/items This patch introduces a route to fetch items belonging to a biblio. It is expected to return the 'public' representation of the Koha::Item objects. It is also enforcing the visibility rules, by using Koha::Items->filter_by_visible_in_opac. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.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 126157 [details] [review] Bug 27358: Unit tests for public items retrieval Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126158 [details] [review] Bug 27358: Unit tests for public items retrieval Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126159 [details] [review] Bug 27358: Add GET /public/biblios/:biblio_id/items This patch introduces a route to fetch items belonging to a biblio. It is expected to return the 'public' representation of the Koha::Item objects. It is also enforcing the visibility rules, by using Koha::Items->filter_by_visible_in_opac. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.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 126164 [details] [review] Bug 27358: Unit tests for public items retrieval Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126165 [details] [review] Bug 27358: Add GET /public/biblios/:biblio_id/items This patch introduces a route to fetch items belonging to a biblio. It is expected to return the 'public' representation of the Koha::Item objects. It is also enforcing the visibility rules, by using Koha::Items->filter_by_visible_in_opac. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.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>
Bug updated to reflect dependence on bug 28948 where we took the base code from here for a public allow list implementation and refined it using a simpler case, libraries. Allowlist taken from Katrins list in comment 8 as I agree with that list. We leave out more_subfields_xml so it can be handled separately, likely as an embed. All looking good here now for the next QA round.. Signing off.
Created attachment 126971 [details] [review] Bug 27358: Unit tests for public items retrieval Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126972 [details] [review] Bug 27358: Add GET /public/biblios/:biblio_id/items This patch introduces a route to fetch items belonging to a biblio. It is expected to return the 'public' representation of the Koha::Item objects. It is also enforcing the visibility rules, by using Koha::Items->filter_by_visible_in_opac. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.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: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 21.11, thanks to everybody involved!
Great job! Would it be possible to backport to 20.05 as well?