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: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 29275
  Show dependency treegraph
 
Reported: 2021-11-18 21:13 UTC by Tomás Cohen Arazi
Modified: 2021-11-26 16:35 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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

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!>?