Bug 29523 - Add a way to prevent embedding objects that should not be allowed
Summary: Add a way to prevent embedding objects that should not be allowed
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 29275
  Show dependency treegraph
 
Reported: 2021-11-18 21:13 UTC by Tomás Cohen Arazi
Modified: 2022-10-02 23:30 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 29523: Add Koha::Objects->search_limited stub method (1.15 KB, patch)
2021-11-19 16:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Add Koha::Object->accessible (3.39 KB, patch)
2021-11-19 16:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Regression tests (1.98 KB, patch)
2021-11-19 16:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Make Koha::Object->to_api respect accessibility (2.42 KB, patch)
2021-11-19 16:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Add Koha::Objects->search_limited stub method (1.20 KB, patch)
2021-11-25 06:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 29523: Add Koha::Object->accessible (3.44 KB, patch)
2021-11-25 06:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 29523: Regression tests (2.02 KB, patch)
2021-11-25 06:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 29523: Make Koha::Object->to_api respect accessibility (2.46 KB, patch)
2021-11-25 06:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 29523: Add Koha::Objects->search_limited stub method (1.26 KB, patch)
2021-11-26 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Add Koha::Object->accessible (3.50 KB, patch)
2021-11-26 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Regression tests (2.09 KB, patch)
2021-11-26 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Make Koha::Object->to_api respect accessibility (2.53 KB, patch)
2021-11-26 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases (3.54 KB, patch)
2021-11-26 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: (QA follow-up) Fix failing test in club holds (782 bytes, patch)
2021-11-26 14:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
ALT: An idea of doing it at singular level? (3.84 KB, patch)
2021-11-26 15:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Add Koha::Objects->search_limited stub method (1.32 KB, patch)
2021-12-17 21:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Add Koha::Object->accessible (3.56 KB, patch)
2021-12-17 21:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Regression tests (2.14 KB, patch)
2021-12-17 21:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Make Koha::Object->to_api respect accessibility (2.58 KB, patch)
2021-12-17 21:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases (3.59 KB, patch)
2021-12-17 21:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: (QA follow-up) Fix failing test in club holds (838 bytes, patch)
2021-12-17 21:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Pass the logged user around and use for validating (3.79 KB, patch)
2021-12-17 21:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29523: Cache the restricted branches list (1.97 KB, patch)
2022-03-03 14:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Pass the logged user around and use for validating (5.08 KB, patch)
2022-03-03 15:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Cache the restricted branches list (1.97 KB, patch)
2022-03-03 15:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Add Koha::Objects->search_limited stub method (1.35 KB, patch)
2022-06-28 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Add Koha::Object->accessible (3.56 KB, patch)
2022-06-28 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Regression tests (2.14 KB, patch)
2022-06-28 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Make Koha::Object->to_api respect accessibility (2.58 KB, patch)
2022-06-28 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: (QA follow-up) Catch remaining SUPER::to_api cases (3.59 KB, patch)
2022-06-28 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: (QA follow-up) Fix failing test in club holds (838 bytes, patch)
2022-06-28 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Pass the logged user around and use for validating (5.78 KB, patch)
2022-06-28 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Cache the restricted branches list (2.32 KB, patch)
2022-06-28 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Remove the FIXME (8.11 KB, patch)
2022-06-28 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Remove the FIXME (8.94 KB, patch)
2022-06-29 09:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29523: Remove no longer required methods (1.61 KB, patch)
2022-06-29 13:19 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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