Bug 29523

Summary: Add a way to prevent embedding objects that should not be allowed
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Martin Renvoize <martin.renvoize>
Status: Pushed to master --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: agustinmoyano, david, dcook, fridolin.somers, jonathan.druart, joonas.kylmala, katrin.fischer, kyle, m.de.rooy, martin.renvoize, nick, nugged, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29503
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30019
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00
Bug Depends on:    
Bug Blocks: 29275, 33568    
Attachments: Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
ALT: An idea of doing it at singular level?
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Remove the FIXME
Bug 29523: Remove the FIXME
Bug 29523: Remove no longer required methods
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Remove the FIXME
Bug 29523: Remove no longer required methods
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Remove the FIXME
Bug 29523: Remove no longer required methods
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Remove the FIXME
Bug 29523: Remove no longer required methods
Bug 29523: Add redaction for inaccessible objects
Bug 29523: WIP - Add support for 'unredact_list'
Bug 29523: Add support for 'unredact_list'
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Remove the FIXME
Bug 29523: Remove no longer required methods
Bug 29523: Add redaction for inaccessible objects
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Remove the FIXME
Bug 29523: Remove no longer required methods
Bug 29523: Add redaction for inaccessible objects
Bug 29523: (follow-up) Comprehensive tests for redaction
Bug 29523: (follow-up) Comprehensive tests for redaction
Bug 29523: (follow-up) Comprehensive tests for redaction
Bug 29523: (follow-up) Comprehensive tests for redaction
Bug 29523: Add Koha::Objects->search_limited stub method
Bug 29523: Add Koha::Object->accessible
Bug 29523: Regression tests
Bug 29523: Make Koha::Object->to_api respect accessibility
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases
Bug 29523: (QA follow-up) Fix failing test in club holds
Bug 29523: Pass the logged user around and use for validating
Bug 29523: Cache the restricted branches list
Bug 29523: Remove the FIXME
Bug 29523: Remove no longer required methods
Bug 29523: Add redaction for inaccessible objects
Bug 29523: (follow-up) Comprehensive tests for redaction
Bug 29523: (follow-up) Adding documentation to swagger.yaml
Bug 29523: Fix tests

Description Tomás Cohen Arazi 2021-11-18 21:13:37 UTC
A user can be allowed to see only patrons from its own library, for example. So fetching (through the API) all the current checkouts for a biblio and embedding the patron each checkout in the response, could violate this rule (this is the case on bug 29275).

We need some mechanism to prevent this at a lower level so controller developers don't need to code for that, and also to avoid unintended leaks.

Possible implementations will need to standardize things like the one used in bug 29506, which relies on the existence of ->search_limited to filter out forbidden results.
Comment 1 Tomás Cohen Arazi 2021-11-19 16:26:04 UTC
Created attachment 127886 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-11-19 16:26:09 UTC
Created attachment 127887 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-11-19 16:26:15 UTC
Created attachment 127888 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2021-11-19 16:26:20 UTC
Created attachment 127889 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2021-11-19 19:49:30 UTC
We should consider this one for 21.11.
Comment 6 David Nind 2021-11-25 06:49:39 UTC
Created attachment 127991 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2021-11-25 06:49:50 UTC
Created attachment 127992 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2021-11-25 06:50:09 UTC
Created attachment 127993 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2021-11-25 06:50:27 UTC
Created attachment 127994 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2021-11-25 06:58:20 UTC
Testing notes:

1. Step 6: had to give Henry circulate and catalogue permissions.

2. Step 8: URL should be ../vi/biblios/...
Comment 11 Martin Renvoize 2021-11-26 14:48:41 UTC
Created attachment 128042 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2021-11-26 14:48:45 UTC
Created attachment 128043 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-11-26 14:48:49 UTC
Created attachment 128044 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-11-26 14:48:53 UTC
Created attachment 128045 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2021-11-26 14:48:57 UTC
Created attachment 128046 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-11-26 14:49:01 UTC
Created attachment 128047 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.
Comment 17 Martin Renvoize 2021-11-26 14:53:59 UTC
OK.. this works as-is, mostly, but I think we can continue to refine it.

However, currently, there is one test failing and I couldn't grok how to fix it: `t/db_dependent/api/v1/patrons.t`

In a future place, I'd be tempted to try and not rely on 'search_related' from the plural class from the singular. I'm wondering if we could instead just have a 'can_be_seen' or 'accessible' method at the object/singular level.  The challenge will be how do we prevent db calls etc in that function when it's being called in the iterative loop to build a to_api resultset... we need a way of caching the 'why I cannot be seen' functionality at the set level.. hence I think I can see where Tomas was going trying to use search_related.

Great work so far..
Comment 18 Martin Renvoize 2021-11-26 15:41:46 UTC
Created attachment 128048 [details] [review]
ALT: An idea of doing it at singular level?

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible_in_context' method added to Koha::Patron.

The method name is a bit of a mouthfull.. it could be 'is_limited' as
it's really the equivilent of 'search_limited' in the plural class.. but
I wasn't sure that was actually clearer... if we like it we could strip
back out some of the work around search_related.

As an asside, I also update Koha::Patron->can_see_patrons_from to use
Koha::Patron->libraries_where_can_see_patrons internally.. DRY!
Comment 19 Martin Renvoize 2021-11-26 16:34:20 UTC
I think we also need a regression test for the to_api method.. I might be just overlooking it?

I have a question.. if you embed a plural on a route.. like 'patrons'.. should the result be an array with 'null' gaps.. or an array that just contains valid patrons without gaps?

This relates closely to my query regards should we return undefined vs should we return something that states the user is restricted from the consumer's view.

The use of search_limited in .search and .find methods (and thus returning 404) makes me think for consistency we should not leave null gaps here.. but at the same time knowing something exists but is restricted from view is meaningful data.. so should .search and .find actually return 404 (or conversely 403/451 to show it exists but isn't allowed to be seen).

I can see uses cases for all of the above.. so perhaps it needs a header to allow switching modes!>?
Comment 20 Tomás Cohen Arazi 2021-12-06 11:25:10 UTC
(In reply to Martin Renvoize from comment #18)
> Created attachment 128048 [details] [review] [review]
> ALT: An idea of doing it at singular level?
> 
> In this patch I add 'user', containing the Koha::Patron object for the
> logged in user in the params hash we pass around in to_api. I then use
> that in a new 'is_accessible_in_context' method added to Koha::Patron.
> 
> The method name is a bit of a mouthfull.. it could be 'is_limited' as
> it's really the equivilent of 'search_limited' in the plural class.. but
> I wasn't sure that was actually clearer... if we like it we could strip
> back out some of the work around search_related.
> 
> As an asside, I also update Koha::Patron->can_see_patrons_from to use
> Koha::Patron->libraries_where_can_see_patrons internally.. DRY!

I like your approach. Also, things like $self->libraries_where_can_see_patrons in Koha::Patron could be cached and thus reused in further recursive calls. That would be great, to mitigate the overhead this will introduce!
Comment 21 Jonathan Druart 2021-12-13 12:17:21 UTC
(In reply to Martin Renvoize from comment #18)
> Created attachment 128048 [details] [review] [review]
> As an asside, I also update Koha::Patron->can_see_patrons_from to use
> Koha::Patron->libraries_where_can_see_patrons internally.. DRY!

I would remove this part from the patch. It's not related and I don't think it's correct, it could bring perf regressions for installs with large number of libraries.
Comment 22 Jonathan Druart 2021-12-13 12:28:37 UTC
(In reply to Martin Renvoize from comment #19)
> I have a question.. if you embed a plural on a route.. like 'patrons'..
> should the result be an array with 'null' gaps.. or an array that just
> contains valid patrons without gaps?

I would not leave gaps for searches.
In my understanding we could have a "no accessible" string displayed in place of patron's info (or whatever other entity/object) close to another info you can see.
For instance the checkout list for a given biblio record (you can see when/what but not who).
I don't really see a use case where we would display a list of patrons with "no accessible" in the result view with links to point to 403's.
Comment 23 Jonathan Druart 2021-12-13 12:28:58 UTC
Can you clarify the situation? What's missing to make it ready for a QA review? I like the idea but don't manage to see how far we are from something done.
Comment 24 Tomás Cohen Arazi 2021-12-13 12:38:19 UTC
(In reply to Jonathan Druart from comment #23)
> Can you clarify the situation? What's missing to make it ready for a QA
> review? I like the idea but don't manage to see how far we are from
> something done.

I think this is done. But Martin's follow-up was an attempt to think of further enhancements. Particulary, as he passes the $patron object around, we could rely on some implicit caching to calculate the libraries_... stuff once, no matter how many times it will be used.

As for the gaps situation, this patch only covers the 'singular' use case, so that's a different bug. I narrowed the implementation to what I needed for bug 29275, and am willing to work on the plural embeds once we agree on the implementation. I also think we shouldn't leave gaps.
Comment 25 Martin Renvoize 2021-12-13 13:54:59 UTC
I'm happy for this to progress without my final patch... but I would love to see it moved to another bug for the next round of performance enhancements and code cleaning.

I think the following statement still needs some action though: However, currently, there is one test failing and I couldn't grok how to fix it: `t/db_dependent/api/v1/patrons.t`
Comment 26 Tomás Cohen Arazi 2021-12-17 21:39:15 UTC
Created attachment 128688 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2021-12-17 21:39:20 UTC
Created attachment 128689 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2021-12-17 21:39:26 UTC
Created attachment 128690 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2021-12-17 21:39:30 UTC
Created attachment 128691 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi 2021-12-17 21:39:35 UTC
Created attachment 128692 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi 2021-12-17 21:39:40 UTC
Created attachment 128693 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Tomás Cohen Arazi 2021-12-17 21:39:45 UTC
Created attachment 128694 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible_in_context' method added to Koha::Patron.

The method name is a bit of a mouthfull.. it could be 'is_limited' as
it's really the equivilent of 'search_limited' in the plural class.. but
I wasn't sure that was actually clearer... if we like it we could strip
back out some of the work around search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Katrin Fischer 2021-12-19 14:15:40 UTC
What's missing here to move on? 

(In reply to Martin Renvoize from comment #25)
> I'm happy for this to progress without my final patch... but I would love to
> see it moved to another bug for the next round of performance enhancements
> and code cleaning.
> 
> I think the following statement still needs some action though: However,
> currently, there is one test failing and I couldn't grok how to fix it:
> `t/db_dependent/api/v1/patrons.t`

Has this been taken care of yet?
Comment 34 Tomás Cohen Arazi 2021-12-19 14:18:14 UTC
(In reply to Katrin Fischer from comment #33)
> What's missing here to move on?

This is ready as-is, but I need to discuss something with Martin tomorrow morning.

> (In reply to Martin Renvoize from comment #25)
> > I'm happy for this to progress without my final patch... but I would love to
> > see it moved to another bug for the next round of performance enhancements
> > and code cleaning.
> > 
> > I think the following statement still needs some action though: However,
> > currently, there is one test failing and I couldn't grok how to fix it:
> > `t/db_dependent/api/v1/patrons.t`
> 
> Has this been taken care of yet?

I took care of that
Comment 35 Katrin Fischer 2021-12-19 14:50:21 UTC
Thx Tomas, go back to your Sunday :)
Comment 36 Marcel de Rooy 2022-01-14 07:26:57 UTC
+    my $type = ref($self);
+
+    $type =~ s|Schema::Result::||;
+    return $type . "s";

Frowning. Educated guess?
Comment 37 Marcel de Rooy 2022-01-14 07:27:34 UTC
Out of scope, but heh:
libraries_where_can_see_patrons
This is a really awful name
Comment 38 Marcel de Rooy 2022-01-14 07:33:08 UTC
+sub accessible {
+    my ($self) = @_;
+
+    return $self->_get_objects_class->search_limited(
+        {
+            map { $_ => $self->$_ }
+              $self->_result->result_source->primary_columns
+        }
+      )->count > 0
+      ? 1
+      : 0;
+}

At first glance this looks like a great performance killer.
If we are in the singular object already, it should be very cheap to see if user A has access.
Okay, we might have to duplicate a bit of code in accessible and search_limited to make that work, but save performance. Which might be a more important gain.
Comment 39 Marcel de Rooy 2022-01-14 07:38:25 UTC
-    return unless $self->accessible;
+    return unless $self->is_accessible_in_context($params);

Not sure if this is an improvement. Access probably always needs a context, so it seems implicit.
Comment 40 Marcel de Rooy 2022-01-14 07:40:44 UTC
-sub accessible {
+sub is_accessible_in_context {
     my ($self) = @_;

-    return $self->_get_objects_class->search_limited(
-        {
-            map { $_ => $self->$_ }
-              $self->_result->result_source->primary_columns
-        }
-      )->count > 0
-      ? 1
-      : 0;
+    return 1;
 }

This happens in the last patch (from another author). This mystifies matters.
Comment 41 Marcel de Rooy 2022-01-14 07:45:09 UTC
+sub is_accessible_in_context {
+    my ( $self, $params ) = @_;
+
+    # FIXME? It felt tempting to return 0 instead
+    # but it would mean needing to explicitly add the 'user'
+    # param in all tests...
+    return 1
+      unless $params->{user};
+
+    my $consumer = $params->{user};
+    return $consumer->can_see_patron_infos($self);
+}

User, consumer, patron. Unclear.
The FIXME tells me that we should think a little bit more about what we really want to do here?
We should obviously try to avoid adding it like this.
Comment 42 Marcel de Rooy 2022-01-14 07:47:02 UTC
Looking at the complete patch set, this raises enough questions to consider it as not ready for production. Should we discuss this in a larger audience?
Comment 43 Martin Renvoize 2022-03-03 14:42:02 UTC
Created attachment 131326 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.
Comment 44 Martin Renvoize 2022-03-03 15:19:58 UTC
Created attachment 131331 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible' method added to Koha::Patron.

The new method is really the equivilent of 'search_limited' in the plural
class and could perhaps be renamed 'is_limited' or something clearer for
the singular form 'is_filtered' or 'fitler_for_api' or something?

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Martin Renvoize 2022-03-03 15:20:04 UTC
Created attachment 131332 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.
Comment 46 Martin Renvoize 2022-03-03 15:23:02 UTC
Back to NSO.. I'd like Tomas to take a look at the additional follow-ups.  Perhaps we should work through the final bits at hackfest.
Comment 47 Tomás Cohen Arazi 2022-03-09 12:37:50 UTC
(In reply to Martin Renvoize from comment #46)
> Back to NSO.. I'd like Tomas to take a look at the additional follow-ups. 
> Perhaps we should work through the final bits at hackfest.

I like it.
Comment 48 Martin Renvoize 2022-06-28 07:31:38 UTC
Gone cold again.. to think this one's getting increasingly important with the API getting more and more use.

I also think the search_limited stuff could perhaps be expanded further after this to handle things like the special 'allow_owner' access permission.

The background jobs list API Jonathan introduces in a recent bug submission hard codes such a filter in the controller.. it's clear and simple but won't carry across should we end up allowing embedding of jobs in other routes in the future.
Comment 49 Martin Renvoize 2022-06-28 16:03:49 UTC
Created attachment 136684 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 50 Martin Renvoize 2022-06-28 16:03:54 UTC
Created attachment 136685 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 51 Martin Renvoize 2022-06-28 16:03:59 UTC
Created attachment 136686 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 52 Martin Renvoize 2022-06-28 16:04:05 UTC
Created attachment 136687 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 53 Martin Renvoize 2022-06-28 16:04:11 UTC
Created attachment 136688 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 54 Martin Renvoize 2022-06-28 16:04:16 UTC
Created attachment 136689 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 55 Martin Renvoize 2022-06-28 16:04:20 UTC
Created attachment 136690 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible' method added to Koha::Patron.

The new method is really the equivilent of 'search_limited' in the plural
class and could perhaps be renamed 'is_limited' or something clearer for
the singular form 'is_filtered' or 'fitler_for_api' or something?

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 56 Martin Renvoize 2022-06-28 16:04:25 UTC
Created attachment 136691 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.
Comment 57 Martin Renvoize 2022-06-28 16:04:30 UTC Comment hidden (obsolete)
Comment 58 Martin Renvoize 2022-06-28 16:07:30 UTC
Rebased and ready to go. The final patch is the removal of a FIXME I introduced in a prior patch in the series.. It's certainly the more secure way of doing it, requiring the user is passed in.. but it does introduce some slight peculiarities in some methods where we call to_api already.
Comment 59 Martin Renvoize 2022-06-29 09:31:00 UTC
Created attachment 136706 [details] [review]
Bug 29523: Remove the FIXME

This patch works through the unit tests and existing code to allow
removal of the FIXME I introduced earlier in the patchset.

We now require the `user` parameter be passed to `is_accessible` which
in turn makes `user` a required parameter for `to_api` in the
`Koha::Patron` case.
Comment 60 Jonathan Druart 2022-06-29 09:58:29 UTC
QA comments:

1. Koha::Object::_get_objects_class added but no tests, and I don't see where it's used from.

2. +    return $self->{_restricted_branchcodes} if exists($self->{_restricted_branchcodes});

Didn't we decide already that this was a wrong way to cache things?
You cannot invalidate the "cache".
Not considering blocker but worth raising it once again.

3. If I read the history correctly it seems that we don't longer need Koha::Objects->search_limited
Comment 61 Martin Renvoize 2022-06-29 10:15:12 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> As for the gaps situation, this patch only covers the 'singular' use case,
> so that's a different bug. I narrowed the implementation to what I needed
> for bug 29275, and am willing to work on the plural embeds once we agree on
> the implementation. I also think we shouldn't leave gaps.

Actually, we do cover the plural somewhat here..

As we call `to_api` from within `_handle_to_api_child` and we're passing around the `user` now and using `is_accessible` we will end up null gaps in our plural embeds.

I actually think that's correct as it stands.. we're sending back a null for the singular case and we're including the null in the count variable of the embed.. so gaps are meaningful.

I however have no idea if our client-side code knows how to handle such situations as yet.
Comment 62 Martin Renvoize 2022-06-29 13:19:17 UTC
Created attachment 136714 [details] [review]
Bug 29523: Remove no longer required methods
Comment 63 Martin Renvoize 2022-06-29 13:29:01 UTC
Documenting for my own sanity.

This patchset works as is.. but there are various ways we could improve.

Right now when building an API response we...
1) Build up a query that includes a prefetch any relations we add with an embed (or add via the query)
2) We can't drop the prefetch because if we do we may affect the resulting related data we return if part of the query passed filters on the joined data.
3) We then recurse through the embeds and with this patchset we filter out based on the is_accessible method in each Object class.
4) This can result in 'nulls' in the array of related data if the relation returns multiple rows.. or a null for the related singular object for the singular case.

This may not be the most efficient way to do things.. we're fetching all the data all the time and then filtering in perl as we iterate through the resultsets calling to_api.

If we wanted to move this into a filtered query, because of the prefetch nature of it, we'd need to do it as a filtered relationship accessor added to the DBIx::Class schema files.. We should test whether adding a 'filter_by_accessible' method to the Koha::Objects class would work when chained onto an existing prefetched relation.  I'm not sure how it handles such a chain and whether our original query would be taken into account correctly.

If we do opt to add a filter_by_accessible to the Koha::Objects class, we will still need the is_accessible for the singular forms here anyway as the relationship will have already been prefetched by the original call.
Comment 64 Jonathan Druart 2022-08-17 09:47:33 UTC
Comment 60 is waiting for an answer.
Comment 65 Martin Renvoize 2022-08-18 13:55:46 UTC
(In reply to Jonathan Druart from comment #60)
> QA comments:
> 
> 1. Koha::Object::_get_objects_class added but no tests, and I don't see
> where it's used from.
> 
> 2. +    return $self->{_restricted_branchcodes} if
> exists($self->{_restricted_branchcodes});
> 
> Didn't we decide already that this was a wrong way to cache things?
> You cannot invalidate the "cache".
> Not considering blocker but worth raising it once again.
> 
> 3. If I read the history correctly it seems that we don't longer need
> Koha::Objects->search_limited

1. and 3. were dealt with in that last patch.. you were right.. no longer used.
Comment 66 Katrin Fischer 2022-10-02 23:30:34 UTC
I am afraid I am not the right QA person for this one, but also it does no longer apply :(

error: sha1 information is lacking or useless (Koha/Patron.pm).
error: could not build fake ancestor
Comment 67 Martin Renvoize 2023-03-27 08:59:46 UTC
Created attachment 148736 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Martin Renvoize 2023-03-27 08:59:56 UTC
Created attachment 148737 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Martin Renvoize 2023-03-27 09:00:04 UTC
Created attachment 148738 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 70 Martin Renvoize 2023-03-27 09:00:08 UTC
Created attachment 148739 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 71 Martin Renvoize 2023-03-27 09:00:13 UTC
Created attachment 148740 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 72 Martin Renvoize 2023-03-27 09:00:17 UTC
Created attachment 148741 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 73 Martin Renvoize 2023-03-27 09:00:21 UTC
Created attachment 148742 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible' method added to Koha::Patron.

The new method is really the equivilent of 'search_limited' in the plural
class and could perhaps be renamed 'is_limited' or something clearer for
the singular form 'is_filtered' or 'fitler_for_api' or something?

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 74 Martin Renvoize 2023-03-27 09:00:26 UTC
Created attachment 148743 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.
Comment 75 Martin Renvoize 2023-03-27 09:00:33 UTC
Created attachment 148744 [details] [review]
Bug 29523: Remove the FIXME

This patch works through the unit tests and existing code to allow
removal of the FIXME I introduced earlier in the patchset.

We now require the `user` parameter be passed to `is_accessible` which
in turn makes `user` a required parameter for `to_api` in the
`Koha::Patron` case.
Comment 76 Martin Renvoize 2023-03-27 09:00:38 UTC
Created attachment 148745 [details] [review]
Bug 29523: Remove no longer required methods
Comment 77 Martin Renvoize 2023-03-27 09:02:01 UTC
Rebased..  I'm having an issue spotting where we're missing a right curly in one of the tests after the rebase though.. anyone fancy taking a look during a QA run please feel free.
Comment 78 Marcel de Rooy 2023-04-28 09:32:10 UTC
Still applies. Looking here (a bit)
Searched for the syntax error by going back patches:

HEAD is now at 3aa1c17185 Bug 29523: Add Koha::Object->accessible
root@master:/usr/share/koha# perl -cw t/db_dependent/Koha/Object.t
t/db_dependent/Koha/Object.t syntax OK

The next patch is the faulty one.
Comment 79 Marcel de Rooy 2023-04-28 09:33:32 UTC
+    subtest 'accessible usage tests' => sub {
+
+        plan tests => 2;
+
+        $schema->storage->txn_begin;
+
+        my $library_1 = $builder->build_object( { class => 'Koha::Libraries' } );
+        my $library_2 = $builder->build_object( { class => 'Koha::Libraries' } );
+
+        my $patron = $builder->build_object(
+            {
+                class => 'Koha::Patrons',
+                value => {
+                    flags      => 2**2,            # only has catalogue permissions
+                    branchcode => $library_1->id
+                }
+            }
+        );
+

There is no end subtest here
Comment 80 Marcel de Rooy 2023-04-28 09:39:18 UTC
Code for this test has been moved during the rebase somehow.
Please fix.
Comment 81 Martin Renvoize 2023-05-10 12:56:46 UTC
Created attachment 150989 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 82 Martin Renvoize 2023-05-10 12:56:49 UTC
Created attachment 150990 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Martin Renvoize 2023-05-10 12:56:53 UTC
Created attachment 150991 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Martin Renvoize 2023-05-10 12:56:57 UTC
Created attachment 150992 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 85 Martin Renvoize 2023-05-10 12:57:00 UTC
Created attachment 150993 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 86 Martin Renvoize 2023-05-10 12:57:04 UTC
Created attachment 150994 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 87 Martin Renvoize 2023-05-10 12:57:07 UTC
Created attachment 150995 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible' method added to Koha::Patron.

The new method is really the equivilent of 'search_limited' in the plural
class and could perhaps be renamed 'is_limited' or something clearer for
the singular form 'is_filtered' or 'fitler_for_api' or something?

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 88 Martin Renvoize 2023-05-10 12:57:10 UTC
Created attachment 150996 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.
Comment 89 Martin Renvoize 2023-05-10 12:57:14 UTC
Created attachment 150997 [details] [review]
Bug 29523: Remove the FIXME

This patch works through the unit tests and existing code to allow
removal of the FIXME I introduced earlier in the patchset.

We now require the `user` parameter be passed to `is_accessible` which
in turn makes `user` a required parameter for `to_api` in the
`Koha::Patron` case.
Comment 90 Martin Renvoize 2023-05-10 12:57:17 UTC
Created attachment 150998 [details] [review]
Bug 29523: Remove no longer required methods
Comment 91 Martin Renvoize 2023-05-10 12:58:01 UTC
Aha.. thanks Marcel.. that helped me spot the issue!

All rebased again and the 'Regressions tests' patch fixed.

Tests all passing again now. :)
Comment 92 Marcel de Rooy 2023-05-10 13:00:50 UTC
(In reply to Martin Renvoize from comment #91)
> Aha.. thanks Marcel.. that helped me spot the issue!
> 
> All rebased again and the 'Regressions tests' patch fixed.
> 
> Tests all passing again now. :)

You chose the wrong status, I am afraid. It came from SO.
Comment 93 Jonathan Druart 2023-05-23 13:02:19 UTC
I have a use case on bug 33568 where I need at least the library of the patron, to display "a patron from LIBRARY".

Bug 29275 has the same problem I think:

+                            if ( row.patron != null ) {
+                                return $patron_to_html( row.patron, { display_cardnumber: true, url: true } );
+                            }
+                            else {
+                                return _("A patron from library %s").format(escape_str(row.library.name));
+                            }

We need patron.library.name here, not library.name
Comment 94 Jonathan Druart 2023-05-23 13:55:02 UTC
Another thing, on bug 29275 we have a column with patron's name:
  "data": "patron.firstname:patron.surname:patron.cardnumber"

I cannot test right now because the patch does not apply but in my understanding we are going to search on the patron's data, get the corresponding row, then hide the patron's info. We should remove the whole row I'd say, as we don't want to search on the patron's info.

Are not we trying to provide a global solution for a tricky problem?

What would be the other use cases?

Both bug 29275 and bug 33568 need to hide patron's personal info, and this does not even answer this use case.
Comment 95 Jonathan Druart 2023-05-23 13:56:25 UTC
(In reply to Jonathan Druart from comment #94)
> Another thing, on bug 29275 we have a column with patron's name:
>   "data": "patron.firstname:patron.surname:patron.cardnumber"
> 
> I cannot test right now because the patch does not apply but in my
> understanding we are going to search on the patron's data, get the
> corresponding row, then hide the patron's info. We should remove the whole
> row I'd say, as we don't want to search on the patron's info.

And, to be honest, I have no idea how we could deal with that, even with a simpler (more focus on patron) solution.
Comment 96 Jonathan Druart 2023-07-25 08:58:42 UTC
How can we move this forward?
Comment 97 David Cook 2023-07-27 02:35:00 UTC
(In reply to Jonathan Druart from comment #96)
> How can we move this forward?

I suppose more eyes on it. I've tried reading through everything a couple times, and it seems complicated.
Comment 98 David Cook 2023-07-27 02:38:48 UTC
I actually just thought of another potential issue...

Other than Koha's internal use of the API, I think most third-party API usage involves using 1 potentially high privileged user. 

I assume there will be times where that API user is making a call on behalf of an anonymous user or a low-privileged user, but since the API user is the "logged in" user, the anonymous/low-privileged user will get access to data that they shouldn't - unless the third-party API user does post-processing on their end (which is something we said we wanted to avoid on bug 29275).
Comment 99 David Cook 2023-07-27 02:51:48 UTC
(In reply to Jonathan Druart from comment #94)
> Are not we trying to provide a global solution for a tricky problem?

Jonathan, are you saying that you think the solution is trying to be "too" global and that we should instead just be focusing on protecting patron information?

A global solution would be great, but it's hard to do.
Comment 100 David Cook 2023-07-27 02:59:31 UTC
I have a different system where I needed to control access to resources based on requesting user. Fortunately, it is a smaller system than Koha with clearer boundaries around the key data, so I focused on the key data.

For "single" access, each resource has a method for testing authorization to itself. It takes a requesting user, a policy, a requesting action, and optionally resource attributes for more fine-grained control. Since it's just 1 resource, I could use a lot of logic without having problems with scale/efficiency.

For "plural" access, it got more complicated, because I needed to leverage SQL and not code logic. Fortunately, I really only needed to worry about GETs in this context, since I wasn't doing bulk modifications or bulk deletions. For the GETs, I was able to insert extra SQL into the WHERE clause where needed to make sure only resources the requesting user was authorized to see were retrieved from the database.

That's probably a lot harder in Koha, especially if we're trying to do it globally. 

But if we're just trying to restrict what patrons a requesting user can see based on if they're from the same library... that could be a lot easier. 

It wouldn't feel great having a resource-specific authorization scheme instead of a global one, but it would be a lot easier.
Comment 101 Tomás Cohen Arazi 2023-07-27 03:00:35 UTC
(In reply to Jonathan Druart from comment #93)
> I have a use case on bug 33568 where I need at least the library of the
> patron, to display "a patron from LIBRARY".

You should just embed the patron's library... Then if the patron of undefined, you use the patron library string.
Comment 102 David Cook 2023-07-27 03:39:04 UTC
(In reply to Tomás Cohen Arazi from comment #101)
> (In reply to Jonathan Druart from comment #93)
> > I have a use case on bug 33568 where I need at least the library of the
> > patron, to display "a patron from LIBRARY".
> 
> You should just embed the patron's library... Then if the patron of
> undefined, you use the patron library string.

How do you know the patron's library if they've been filtered out ahead of time?
Comment 103 Jonathan Druart 2023-07-27 07:53:19 UTC
(In reply to Tomás Cohen Arazi from comment #101)
> (In reply to Jonathan Druart from comment #93)
> > I have a use case on bug 33568 where I need at least the library of the
> > patron, to display "a patron from LIBRARY".
> 
> You should just embed the patron's library... Then if the patron of
> undefined, you use the patron library string.

I don't understand what you mean.

If patron is undefined I should access patron.library? That does not make sense.

Or do you mean it's what we need to do here?

(In reply to David Cook from comment #99)
> (In reply to Jonathan Druart from comment #94)
> > Are not we trying to provide a global solution for a tricky problem?
> 
> Jonathan, are you saying that you think the solution is trying to be "too"
> global and that we should instead just be focusing on protecting patron
> information?
> 
> A global solution would be great, but it's hard to do.

As said previously, I don't know how to implement (correctly) a more focus (on patron) solution. So it's hard to unlock the situation with this global solution.
The original (comment 0) need is the same as the need I have on bug 33568. Maybe we should provide something hacky but that will solve the problem...?
Comment 104 Martin Renvoize 2023-08-16 12:31:52 UTC
I think I've mentioned it before.. but I think the solution likely lies in redaction rather than outright removal of results.. I think we probably need a way to identify what fields should be redacted for the redacted case at each object level and then obviously a way to know when to call a redaction function to clear out the fields for the API response.
Comment 105 Jonathan Druart 2023-08-31 12:48:25 UTC
(In reply to Martin Renvoize from comment #104)
> I think I've mentioned it before.. but I think the solution likely lies in
> redaction rather than outright removal of results.. I think we probably need
> a way to identify what fields should be redacted for the redacted case at
> each object level and then obviously a way to know when to call a redaction
> function to clear out the fields for the API response.

I have written a quick follow-up on top of bug_33568 (that is based on top of this one):
https://gitlab.com/joubu/Koha/-/commit/75b8cd39135966d2ea1b8a48df9aec4a3254d0d5

Would that work?
Comment 106 Jonathan Druart 2023-08-31 12:51:37 UTC
(In reply to Jonathan Druart from comment #105)
> (In reply to Martin Renvoize from comment #104)
> > I think I've mentioned it before.. but I think the solution likely lies in
> > redaction rather than outright removal of results.. I think we probably need
> > a way to identify what fields should be redacted for the redacted case at
> > each object level and then obviously a way to know when to call a redaction
> > function to clear out the fields for the API response.
> 
> I have written a quick follow-up on top of bug_33568 (that is based on top
> of this one):
> https://gitlab.com/joubu/Koha/-/commit/
> 75b8cd39135966d2ea1b8a48df9aec4a3254d0d5
> 
> Would that work?

One thing that is not really nice is that, in Koha::Patron::to_api we are now having a json_patron and so not early returning, and we get 'restricted' in the response whereas the unredacted version should not contain it. But that would mean another call to is_accessible.
Comment 107 Martin Renvoize 2023-10-12 10:52:23 UTC
Created attachment 156939 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 108 Martin Renvoize 2023-10-12 10:52:27 UTC
Created attachment 156940 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 109 Martin Renvoize 2023-10-12 10:52:30 UTC
Created attachment 156941 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 110 Martin Renvoize 2023-10-12 10:52:34 UTC
Created attachment 156942 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 111 Martin Renvoize 2023-10-12 10:52:37 UTC
Created attachment 156943 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 112 Martin Renvoize 2023-10-12 10:52:40 UTC
Created attachment 156944 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 113 Martin Renvoize 2023-10-12 10:52:44 UTC
Created attachment 156945 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible' method added to Koha::Patron.

The new method is really the equivilent of 'search_limited' in the plural
class and could perhaps be renamed 'is_limited' or something clearer for
the singular form 'is_filtered' or 'fitler_for_api' or something?

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 114 Martin Renvoize 2023-10-12 10:52:47 UTC
Created attachment 156946 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.
Comment 115 Martin Renvoize 2023-10-12 10:52:51 UTC
Created attachment 156947 [details] [review]
Bug 29523: Remove the FIXME

This patch works through the unit tests and existing code to allow
removal of the FIXME I introduced earlier in the patchset.

We now require the `user` parameter be passed to `is_accessible` which
in turn makes `user` a required parameter for `to_api` in the
`Koha::Patron` case.
Comment 116 Martin Renvoize 2023-10-12 10:52:54 UTC
Created attachment 156948 [details] [review]
Bug 29523: Remove no longer required methods
Comment 117 Martin Renvoize 2023-10-12 10:52:58 UTC
Created attachment 156949 [details] [review]
Bug 29523: Add redaction for inaccessible objects

This patch switches from removing inaccessible items from the responses
to instead redacting all the core fields in innaccessible responses.

This allows for embed traversal and keeps counts etc correct but also
hides the data we want to hide.
Comment 118 Martin Renvoize 2023-10-12 10:57:42 UTC
(In reply to Jonathan Druart from comment #106)
> > I have written a quick follow-up on top of bug_33568 (that is based on top
> > of this one):
> > https://gitlab.com/joubu/Koha/-/commit/
> > 75b8cd39135966d2ea1b8a48df9aec4a3254d0d5
> > 
> > Would that work?
> 
> One thing that is not really nice is that, in Koha::Patron::to_api we are
> now having a json_patron and so not early returning, and we get 'restricted'
> in the response whereas the unredacted version should not contain it. But
> that would mean another call to is_accessible.

I've rebased the patchset and used your follow-up for inspiration but written my own version.  Instead of returning an empty hash I've bound the redaction inside our TO_JSON method and replace the column values with redaction replacements..

I'm not sure if this is better or worse.  Also.. as you've highlighted we'll need to deal with overloaded to_api methods somehow as they often add in data after an inititial to_api call and we can also switch remove the code that looks for undefined again if we go with either of these redaction techniques instead.
Comment 119 Jonathan Druart 2023-10-12 12:27:33 UTC
This patch does not answer the need I have on bug 33568. I need a redacted version of the object, not to hide all the fields.

The use case is:
Display who has checked out of the item to a librarian who does not have the permission to see the patron's info. In this case we want to display "A user from Centerville"

So the response should contain
  checkout.patron = { branchcode => "CPL" }
Comment 120 Martin Renvoize 2023-10-12 13:54:49 UTC
Created attachment 156965 [details] [review]
Bug 29523: WIP - Add support for 'unredact_list'

This work in progress patch adds support for an 'unredact_list' method
to be added at the Koha::* class level allowing for individual classes
to specify which fields they wish to expose to restricted users
regardless.

It drops the type handling as we move from TO_JSON to to_api.. I'm
considering whether we should really be moving both public and redaction
handling into TO_JSON as apposed to to_api... but it adds complication
to the strings mapping
Comment 121 Jonathan Druart 2023-10-12 14:43:34 UTC
Almost "good enough for now" for me. However I am seeing "Checked out to ##### ##### (#####) #####: due 10/17/2023" with the same patch I used previously.
I don't think having a pattern ##### is a good idea.
Comment 122 Jonathan Druart 2023-10-12 14:47:25 UTC
(In reply to Jonathan Druart from comment #121)
> Almost "good enough for now" for me. However I am seeing "Checked out to
> ##### ##### (#####) #####: due 10/17/2023" with the same patch I used
> previously.
> I don't think having a pattern ##### is a good idea.

And "#####" is not valid for dates, or integers, etc.
Comment 123 Jonathan Druart 2023-10-12 14:52:22 UTC
(In reply to Jonathan Druart from comment #122)
> (In reply to Jonathan Druart from comment #121)
> > Almost "good enough for now" for me. However I am seeing "Checked out to
> > ##### ##### (#####) #####: due 10/17/2023" with the same patch I used
> > previously.
> > I don't think having a pattern ##### is a good idea.
> 
> And "#####" is not valid for dates, or integers, etc.

New version that is working for me on https://gitlab.com/joubu/Koha/-/commit/b72d69e3e083167e3e3dcaf8c453cf15217b7193

Still not correct because of the above "" is not valid for booleans, integers, etc.

I am feeling a bit stuck.
Comment 124 Martin Renvoize 2023-10-13 09:55:06 UTC
Created attachment 157049 [details] [review]
Bug 29523: Add support for 'unredact_list'

This patch adds support for an 'unredact_list' method at the Koha::*
class level allowing for individual classes to specify which fields
they wish to expose to restricted users regardless of their restriction.

It is to be used in combination with the is_accessible method introduced
earlier in this patchset which is used to denote whether the current
user should be allowed to see the full record or only a subset of it as
defined in the unredacted_list.

We undefine any fields not listed in the unredact_list for the API
response. This has the effect of still returning the full object of
keys, but setting most fields to a JSON null.
Comment 125 Martin Renvoize 2023-10-13 10:43:41 UTC
Running tests and doing a little squashing here at the moment.. should be ready for a final QA run imminently.
Comment 126 Martin Renvoize 2023-10-13 11:57:09 UTC
Created attachment 157071 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 127 Martin Renvoize 2023-10-13 11:57:12 UTC
Created attachment 157072 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 128 Martin Renvoize 2023-10-13 11:57:16 UTC
Created attachment 157073 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 129 Martin Renvoize 2023-10-13 11:57:19 UTC
Created attachment 157074 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 130 Martin Renvoize 2023-10-13 11:57:23 UTC
Created attachment 157075 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 131 Martin Renvoize 2023-10-13 11:57:27 UTC
Created attachment 157076 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 132 Martin Renvoize 2023-10-13 11:57:31 UTC
Created attachment 157077 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible' method added to Koha::Patron.

The new method is really the equivilent of 'search_limited' in the plural
class and could perhaps be renamed 'is_limited' or something clearer for
the singular form 'is_filtered' or 'fitler_for_api' or something?

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 133 Martin Renvoize 2023-10-13 11:57:34 UTC
Created attachment 157078 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.
Comment 134 Martin Renvoize 2023-10-13 11:57:38 UTC
Created attachment 157079 [details] [review]
Bug 29523: Remove the FIXME

This patch works through the unit tests and existing code to allow
removal of the FIXME I introduced earlier in the patchset.

We now require the `user` parameter be passed to `is_accessible` which
in turn makes `user` a required parameter for `to_api` in the
`Koha::Patron` case.
Comment 135 Martin Renvoize 2023-10-13 11:57:41 UTC
Created attachment 157080 [details] [review]
Bug 29523: Remove no longer required methods
Comment 136 Martin Renvoize 2023-10-13 11:57:44 UTC
Created attachment 157081 [details] [review]
Bug 29523: Add redaction for inaccessible objects

This patch switches from removing inaccessible items from the responses
to instead redacting fields in innaccessible responses.

This allows for embed traversal and keeps counts etc correct but also
hides the data we want to hide.

We add support for an 'unredact_list' method at the Koha::* class level
allowing for individual classes to specify which fields they wish to
expose to restricted users regardless of their restriction.

It is to be used in combination with the is_accessible method introduced
earlier in this patchset which is used to denote whether the current
user should be allowed to see the full record or only a subset of it as
defined in the unredacted_list.

We undefine any fields not listed in the unredact_list for the API
response. This has the effect of still returning the full object of
keys, but setting most fields to a JSON null.
Comment 137 Martin Renvoize 2023-10-13 12:04:33 UTC
Rebased and squashed some bits.. tidied the PM's to get the QA scripts happy and run the test suit.  There's still some QA issues around tidyness of tests though.

I think this is a good resolution at this point.. we could squash some more, but I wasn't sure whether to leave the patches for visibility of thought process and effort that's gone into this by all the verious parties.
Comment 138 Jonathan Druart 2023-10-19 09:57:29 UTC
Created attachment 157406 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 139 Jonathan Druart 2023-10-19 09:57:32 UTC
Created attachment 157407 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 140 Jonathan Druart 2023-10-19 09:57:36 UTC
Created attachment 157408 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 141 Jonathan Druart 2023-10-19 09:57:40 UTC
Created attachment 157409 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 142 Jonathan Druart 2023-10-19 09:57:44 UTC
Created attachment 157410 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 143 Jonathan Druart 2023-10-19 09:57:48 UTC
Created attachment 157411 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 144 Jonathan Druart 2023-10-19 09:57:52 UTC
Created attachment 157412 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible' method added to Koha::Patron.

The new method is really the equivilent of 'search_limited' in the plural
class and could perhaps be renamed 'is_limited' or something clearer for
the singular form 'is_filtered' or 'fitler_for_api' or something?

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 145 Jonathan Druart 2023-10-19 09:57:56 UTC
Created attachment 157413 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 146 Jonathan Druart 2023-10-19 09:58:00 UTC
Created attachment 157414 [details] [review]
Bug 29523: Remove the FIXME

This patch works through the unit tests and existing code to allow
removal of the FIXME I introduced earlier in the patchset.

We now require the `user` parameter be passed to `is_accessible` which
in turn makes `user` a required parameter for `to_api` in the
`Koha::Patron` case.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 147 Jonathan Druart 2023-10-19 09:58:05 UTC
Created attachment 157415 [details] [review]
Bug 29523: Remove no longer required methods

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 148 Jonathan Druart 2023-10-19 09:58:09 UTC
Created attachment 157416 [details] [review]
Bug 29523: Add redaction for inaccessible objects

This patch switches from removing inaccessible items from the responses
to instead redacting fields in innaccessible responses.

This allows for embed traversal and keeps counts etc correct but also
hides the data we want to hide.

We add support for an 'unredact_list' method at the Koha::* class level
allowing for individual classes to specify which fields they wish to
expose to restricted users regardless of their restriction.

It is to be used in combination with the is_accessible method introduced
earlier in this patchset which is used to denote whether the current
user should be allowed to see the full record or only a subset of it as
defined in the unredacted_list.

We undefine any fields not listed in the unredact_list for the API
response. This has the effect of still returning the full object of
keys, but setting most fields to a JSON null.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 149 Jonathan Druart 2023-10-19 09:58:38 UTC
I have rebased bug 33568 on top of this last iteration and everything is working as expected. Thanks!

I am adding my signoff, but letting Marcel adding his finaly QA stamp (more eyes the better on this one).
Comment 150 Marcel de Rooy 2023-10-19 11:35:06 UTC
(In reply to Jonathan Druart from comment #149)
> I have rebased bug 33568 on top of this last iteration and everything is
> working as expected. Thanks!
> 
> I am adding my signoff, but letting Marcel adding his finaly QA stamp (more
> eyes the better on this one).

Willing to look at this tomorrow.
Comment 151 Marcel de Rooy 2023-10-20 06:30:12 UTC
QA: Looking here now
Comment 152 Marcel de Rooy 2023-10-20 08:23:30 UTC
Just a few dumb questions:

Name unredact_list Where does this name originate from ?


+        $patron_11_1 = Koha::Patrons->find( $patron_11_1->borrowernumber );
Added a few times. This seems not needed?  Why do you add it?


t/db_dependent/Koha/REST/Plugin/Objects.t 
-      $builder->build_object( { class => 'Koha::Patrons', value => { flags => 1 } } );
+      $builder->build_object( { class => 'Koha::Patrons', value => { flags => 0 } } );
=> If I set the user to no permissions, the test still PASSes. Can you point me to an example in the tests where we can actually see the difference, i.e. where flags makes a difference?

How do we make sure that we did not forget some calls of patron->to_api with user parameter ?
Comment 153 Marcel de Rooy 2023-10-20 08:24:33 UTC
(In reply to Marcel de Rooy from comment #152)
> t/db_dependent/Koha/REST/Plugin/Objects.t 
> -      $builder->build_object( { class => 'Koha::Patrons', value => { flags
> => 1 } } );
> +      $builder->build_object( { class => 'Koha::Patrons', value => { flags
> => 0 } } );

This is around here
 @@ -425,7 +425,7 @@ subtest 'objects.search helper with query parameter' => sub {
Comment 154 Martin Renvoize 2023-10-20 09:01:49 UTC
(In reply to Marcel de Rooy from comment #152)
> Just a few dumb questions:
> 
> Name unredact_list Where does this name originate from ?

I'm open to a name change.. initially I thought 'unredact' felt silly too when Jonathan first introduced it.. but I came to the conclusion it made sense.. we now encourage 'allow lists' rather than 'deny lists' to default to secure.  So it's literally a list of those fields we don't want to redact rather than having to list the one's we do.

> 
> 
> +        $patron_11_1 = Koha::Patrons->find( $patron_11_1->borrowernumber );
> Added a few times. This seems not needed?  Why do you add it?

Basically we need a fresh Koha::Patron object so that we flush the object level micro cache that was introduced in the patch (the micro cache was introduced to improve performance when this is called in a large loop, for example an embed call with many related objects being returned). I don't think we can do a simple discard_changes as that only flushes the dbic level result cache and not the localised object caching we employ here.

> 
> t/db_dependent/Koha/REST/Plugin/Objects.t 
> -      $builder->build_object( { class => 'Koha::Patrons', value => { flags
> => 1 } } );
> +      $builder->build_object( { class => 'Koha::Patrons', value => { flags
> => 0 } } );
> => If I set the user to no permissions, the test still PASSes. Can you point
> me to an example in the tests where we can actually see the difference, i.e.
> where flags makes a difference?

Good question.. I'll have to have a dig on that one.. it's long enough ago in my memory that I'm strugglig to remember the test logic in this case.

> How do we make sure that we did not forget some calls of patron->to_api with
> user parameter ?

It's a good question.. I ran the full api test suit and ripgrepped for to_api manually too.  Not sure there's an easy way to automate further.  What we really need is for anywhere patron can be embedded in the API schema, for there to be a corresponding api unit test.. I think when I last checked that was a pretty solid assumption.. but that may have changed since.
Comment 155 Marcel de Rooy 2023-10-20 09:41:51 UTC
(In reply to Martin Renvoize from comment #154)
> I'm open to a name change.. initially I thought 'unredact' felt silly too
> when Jonathan first introduced it.. but I came to the conclusion it made
> sense.. we now encourage 'allow lists' rather than 'deny lists' to default
> to secure.  So it's literally a list of those fields we don't want to redact
> rather than having to list the one's we do.

Wont ask for a new name here. Just looked it up. It is something like uncensored.
Comment 156 Marcel de Rooy 2023-10-20 09:53:35 UTC
(In reply to Martin Renvoize from comment #154)
> Basically we need a fresh Koha::Patron object so that we flush the object
> level micro cache that was introduced in the patch (the micro cache was
> introduced to improve performance when this is called in a large loop, for
> example an embed call with many related objects being returned). I don't
> think we can do a simple discard_changes as that only flushes the dbic level
> result cache and not the localised object caching we employ here.

Clear.

> It's a good question.. I ran the full api test suit and ripgrepped for
> to_api manually too.  Not sure there's an easy way to automate further. 
> What we really need is for anywhere patron can be embedded in the API
> schema, for there to be a corresponding api unit test.. I think when I last
> checked that was a pretty solid assumption.. but that may have changed since.

Looking at for instance (Koha/REST/Plugin/Objects.pm):

    $app->helper(
        'objects.find' => sub {
            my ( $c, $result_set, $id ) = @_;
            my $object = $c->objects->find_rs( $result_set, $id );
            return unless $object;
            return $c->objects->to_api($object);
        }
    );

So what if the object is a patron?
Comment 157 Marcel de Rooy 2023-10-20 09:57:51 UTC
Will come back here next week to resume. Still want to test some things.
Comment 158 Marcel de Rooy 2023-10-23 13:16:01 UTC
Okay, this looks very good to me in general.

But I am not yet convinced about the last patch with just these test lines:

-        is( ref($patron_1->to_api({ user => $patron })), 'HASH', 'Returns the object hash' );
-        is( $patron_2->to_api({ user => $patron }), undef, 'Not accessible, returns undef' );
+        is(
+            $patron_1->to_api( { user => $patron } )->{firstname}, $patron_1->firstname,
+            'Returns unredacted object hash'
+        );
+        is( $patron_2->to_api( { user => $patron } )->{firstname}, undef, 'Returns redacted object hash' );

This is all or nothing. But I would like to see the clear difference where a patron can see some columns because of unredact and when he is not.

Since patron->unredact_list now only contains branchcode, please show that?
You could mock unredact_list and show more?

Please still extend this unit test a little bit.

Do you have a real life example to test on the REST API. Which endpoint, which embed. Which permissions? Etc?
Comment 159 Jonathan Druart 2023-10-23 14:17:48 UTC
> Do you have a real life example to test on the REST API. Which endpoint,
> which embed. Which permissions? Etc?

Bug 33568
Comment 160 Martin Renvoize 2023-10-23 15:19:50 UTC Comment hidden (obsolete)
Comment 161 Martin Renvoize 2023-10-23 16:20:11 UTC Comment hidden (obsolete)
Comment 162 Martin Renvoize 2023-10-23 16:34:39 UTC
Created attachment 157683 [details] [review]
Bug 29523: (follow-up) Comprehensive tests for redaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 163 Martin Renvoize 2023-10-23 16:38:42 UTC
Regarding permissions..

So long as your user does not have 'view_borrower_infos_from_any_libraries' permission and they're not in a library group with other libraries and permission to view users within the group.. said user should receive a redacted copy of any user who resides in another library than their own when fetching them from the API via a search or an embed. (I believe we still return a 404 should they try to retrieve such a borrower directly however..?)

So.. in short.. create a user (patron A) in one library with the catalogue permission only.

Create some other patrons in other libraries.  Test the API using patron A for login and confirm that your other patrons are returned in a redacted form (with most fields set to 'null' in the json response).
Comment 164 Marcel de Rooy 2023-10-24 11:25:05 UTC
Resuming here
Comment 165 Marcel de Rooy 2023-10-24 11:52:18 UTC
Just hacking Koha::Library a bit to test things:

API crashes with 500:
[2023/10/24 11:46:55] [WARN] OpenAPI >>> GET api/v1/public/libraries [{"message":"Expected string - got null.","path":"\/body\/0\/name"},{"message":"Expected string - got null.","path":"\/body\/1\/name"},{"message":"Expected string - got null.","path":"\/body\/10\/name"},{"message":"Expected string - got null.","path":"\/body\/11\/name"},{"message":"Expected string - got null.","path":"\/body\/12\/name"},{"message":"Expected string - got null.","path":"\/body\/2\/name"},{"message":"Expected string - got null.","path":"\/body\/3\/name"},{"message":"Expected string - got null.","path":"\/body\/4\/name"},{"message":"Expected string - got null.","path":"\/body\/5\/name"},{"message":"Expected string - got null.","path":"\/body\/6\/name"},{"message":"Expected string - got null.","path":"\/body\/7\/name"},{"message":"Expected string - got null.","path":"\/body\/8\/name"},{"message":"Expected string - got null.","path":"\/body\/9\/name"}]

What did I do?

1) public_read_list (Remove country)
-        'branchcity',     'branchstate',    'branchcountry',
+        'branchcity',     'branchstate',

2) Add a not accessible condition:
+sub is_accessible { return 0; }

3) Add two fields on unredact_list
+sub unredact_list { return [ 'branchcode', 'branchcountry' ]; }

Please explain to me why the API crashes?
I would expect a record where I can see the two unredacted fields on anonymous acces via public.
Comment 166 Marcel de Rooy 2023-10-24 11:54:31 UTC
And note that when I change is_accessible to 1, the api responds :)
But I do not get my branchcountry. It is not on public list, but it is in the unredact_list. It this really intuitive?
Comment 167 Marcel de Rooy 2023-10-24 12:02:32 UTC
Thinking about it: The whole thing is that we create some sort of redundancy. If we do not really check if the swagger spec allows a null or a missing column, we can get very easily in trouble like the above.
Comment 168 Marcel de Rooy 2023-10-24 12:03:59 UTC
Waiting for feedback now.
Comment 169 Marcel de Rooy 2023-10-24 12:39:50 UTC
[14:14] <Joubu> marcelr, ashimema: maybe "unredacted" should only be use for "embed"
Comment 170 Martin Renvoize 2023-10-24 12:53:37 UTC
Hmmm.. I hadn't really tested public and accessible together.  The real use case for the accessible stuff here is staff side where a staff user can expect the same overall representation for all objects that exist but they amy not have the right to see all the data.  Whereas 'public' is about thrid party facing api's where they will never see some of the fields at all so the object representation is actually different.

'Public' removes fields outright.. and really should have it's own schema's.
'Accessible' redacts the values of fields, but leaves the keys present so the object itself is a consistent representation.

No.. for your errors in particular.

'Expected string - got null' - If this were a staff side request I'd say that highlights an issue in our schema.. with this patch, the majority of fields should actually be 'type or null' in the schema's.

For the public side, I'm less sure.
Comment 171 Martin Renvoize 2023-10-24 12:54:57 UTC
Yeah.. I wasn't sure if this should be embed only or not in the end.. as highlighted by my comment above

> I believe we still return a 404 should they try to retrieve such a borrower directly however..?
Comment 172 Martin Renvoize 2023-10-25 13:31:44 UTC
Created attachment 157799 [details] [review]
Bug 29523: (follow-up) Comprehensive tests for redaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 173 Marcel de Rooy 2023-10-25 14:52:18 UTC
Will continue tomorrow morning here
Comment 174 Marcel de Rooy 2023-10-26 07:07:08 UTC Comment hidden (obsolete)
Comment 175 Marcel de Rooy 2023-10-26 07:18:05 UTC Comment hidden (obsolete)
Comment 176 Marcel de Rooy 2023-10-26 07:31:28 UTC
Created attachment 157881 [details] [review]
Bug 29523: Add Koha::Objects->search_limited stub method

This method is just a passthru to the search method. It is defined here
to avoid the need to check if each class implements it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 177 Marcel de Rooy 2023-10-26 07:31:31 UTC
Created attachment 157882 [details] [review]
Bug 29523: Add Koha::Object->accessible

This patch introduces a method for checking if an object can be
retrieved by the current user. It depends on the plural class
implementation of the ->search_limited method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 178 Marcel de Rooy 2023-10-26 07:31:34 UTC
Created attachment 157883 [details] [review]
Bug 29523: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 179 Marcel de Rooy 2023-10-26 07:31:38 UTC
Created attachment 157884 [details] [review]
Bug 29523: Make Koha::Object->to_api respect accessibility

This patch makes the *to_api* method honour the accessibility check for
the object. This is relevant in the context of embedding single objects.

The Koha::Patron->to_api method is adjusted to reflect this behavior as
well (it does some manipulation after the ->to_api call and we need to
prevent it).

To test:
1. Apply up to the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: A patron, that shouldn't be accessed, is returned by ->to_api
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
6. Pick Henry Acevedo from the sample data, assign him 'catalogue'
   permissions and a know user/password combination
7. Enable basic authentication
8. Point your favourite tool (Postman?) to
   GET http://kohadev-intra.myDNSname.org:8081/api/v1/biblio/245/checkouts
   Set the following header: x-koha-embed: patron
   Pick whatever biblio you want, actually.
=> SUCCESS: No checkouts
9. Perform a couple checkouts on the chosen biblio. Make sure one
   checkout is for a patron on the same library as Henry, and the other
   on a different one.
10. Repeat 8
=> SUCCESS: You see two checkouts. One of them has an attribute 'patron'
   containing the patron from Henry's library. The other, has the
   attribute set to 'null'.
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 180 Marcel de Rooy 2023-10-26 07:31:41 UTC
Created attachment 157885 [details] [review]
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases

Koha::Object->to_api can now return undefined.. we should be catching
that cases in all post manipulation cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 181 Marcel de Rooy 2023-10-26 07:31:44 UTC
Created attachment 157886 [details] [review]
Bug 29523: (QA follow-up) Fix failing test in club holds

With this patch series, all singular objects need to 'use' their plural
counterparts.. otherwise the parent can't find search_related.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 182 Marcel de Rooy 2023-10-26 07:31:48 UTC
Created attachment 157887 [details] [review]
Bug 29523: Pass the logged user around and use for validating

In this patch I add 'user', containing the Koha::Patron object for the
logged in user in the params hash we pass around in to_api. I then use
that in a new 'is_accessible' method added to Koha::Patron.

The new method is really the equivilent of 'search_limited' in the plural
class and could perhaps be renamed 'is_limited' or something clearer for
the singular form 'is_filtered' or 'fitler_for_api' or something?

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 183 Marcel de Rooy 2023-10-26 07:31:51 UTC
Created attachment 157888 [details] [review]
Bug 29523: Cache the restricted branches list

This patch introduces a very localised cache of the restricted branches
list in the logged in patron object.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 184 Marcel de Rooy 2023-10-26 07:31:55 UTC
Created attachment 157889 [details] [review]
Bug 29523: Remove the FIXME

This patch works through the unit tests and existing code to allow
removal of the FIXME I introduced earlier in the patchset.

We now require the `user` parameter be passed to `is_accessible` which
in turn makes `user` a required parameter for `to_api` in the
`Koha::Patron` case.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 185 Marcel de Rooy 2023-10-26 07:31:58 UTC
Created attachment 157890 [details] [review]
Bug 29523: Remove no longer required methods

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 186 Marcel de Rooy 2023-10-26 07:32:01 UTC
Created attachment 157891 [details] [review]
Bug 29523: Add redaction for inaccessible objects

This patch switches from removing inaccessible items from the responses
to instead redacting fields in innaccessible responses.

This allows for embed traversal and keeps counts etc correct but also
hides the data we want to hide.

We add support for an 'unredact_list' method at the Koha::* class level
allowing for individual classes to specify which fields they wish to
expose to restricted users regardless of their restriction.

It is to be used in combination with the is_accessible method introduced
earlier in this patchset which is used to denote whether the current
user should be allowed to see the full record or only a subset of it as
defined in the unredacted_list.

We undefine any fields not listed in the unredact_list for the API
response. This has the effect of still returning the full object of
keys, but setting most fields to a JSON null.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 187 Marcel de Rooy 2023-10-26 07:32:05 UTC
Created attachment 157892 [details] [review]
Bug 29523: (follow-up) Comprehensive tests for redaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 188 Marcel de Rooy 2023-10-26 07:32:08 UTC
Created attachment 157893 [details] [review]
Bug 29523: (follow-up) Adding documentation to swagger.yaml

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 189 Marcel de Rooy 2023-10-26 07:32:46 UTC
Changing status after adding a QA comment. Hang on
Comment 190 Marcel de Rooy 2023-10-26 07:58:30 UTC
QA Comment:

Great work. We have a base to build further. We might still improve on details?

[1] The code around strings, embeds and related permissions is not simple. We could perhaps add some more comments. Maintenance might get harder along the way. For instance, we trust on the recursion in to_api via the child calls in the embeds loop. But who still remembers that a year later?
[2] We could still extend the unit tests by showing that the approach really works with embed. I tested biblios/checkouts with embed patron on the API versus patrons to see if it worked for a less privileged staff user.
[3] Based on the unredact_list we may be nulling some columns. Developers should be aware of mismatches with swagger specifications. See earlier examples.
[4] Still wondering if we should combine public_read_list with accessible + unredact_list for the public interface. Note that we do now, is safer since we might still null some fields (theoretically). But the result is harder to grasp.
[5 ] Some unit tests are not at the highest tidy level. Given the history and complexity, I gladly ignore that here.
 WARN   t/db_dependent/Koha/REST/Plugin/Objects.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 295, now: 303)
 WARN   t/db_dependent/api/v1/acquisitions_baskets.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 10, now: 14)
 WARN   t/db_dependent/api/v1/acquisitions_funds.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 35, now: 45)
 WARN   t/db_dependent/api/v1/patrons.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 258, now: 263)
Comment 191 Tomás Cohen Arazi 2023-10-26 18:12:51 UTC
(In reply to Marcel de Rooy from comment #190)
> QA Comment:
> 
> Great work. We have a base to build further. We might still improve on
> details?

To infinity, and beyond!
Comment 192 Tomás Cohen Arazi 2023-10-27 19:52:08 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 193 Tomás Cohen Arazi 2023-10-28 12:48:34 UTC
Created attachment 158013 [details] [review]
Bug 29523: Fix tests

Some tests compare things to the output of `->to_api` and are exploding
because the (now mandatory) `user` parameter is not passed in the call.

In the case of IdP.t I just got rid of the use of `to_api` as we are
just trying to acknowledge a new user has been created and the API
representation of it is irrelevant.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 194 Andrii Nugged 2023-11-05 11:17:45 UTC
(In reply to Tomás Cohen Arazi from comment #193)
> Some tests compare things to the output of `->to_api` and are exploding
> because the (now mandatory) `user` parameter is not passed in the call.

... just came to confirm that - the current master has broken mine APIs' for my VuFind (Finna) on preproduction tests.
Comment 195 Fridolin Somers 2023-11-08 07:35:19 UTC
Does not apply easy on 23.05.x
Please provided rebased patches or a branch if needed