Bug 29510 - objects.find should call search_limited if present
Summary: objects.find should call search_limited if present
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 29508
Blocks: 29509
  Show dependency treegraph
 
Reported: 2021-11-17 21:01 UTC by Tomás Cohen Arazi
Modified: 2024-03-04 10:38 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03,23.05.09,22.11.15,22.05.19


Attachments
Bug 29510: Regression tests (2.70 KB, patch)
2021-11-17 21:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29510: Make objects.find call search_limited if present (1.30 KB, patch)
2021-11-17 21:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29510: (follow-up) Adapt GET /patrons/:patron_id (1.13 KB, patch)
2021-11-17 21:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29510: Regression tests (2.76 KB, patch)
2021-11-18 07:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29510: Make objects.find call search_limited if present (1.36 KB, patch)
2021-11-18 07:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29510: (follow-up) Adapt GET /patrons/:patron_id (1.20 KB, patch)
2021-11-18 07:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29510: Regression tests (3.05 KB, patch)
2024-01-22 16:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29510: Make objects.find call search_limited if present (1.35 KB, patch)
2024-01-22 16:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29510: (follow-up) Adapt GET /patrons/:patron_id (1.17 KB, patch)
2024-01-22 16:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29510: Regression tests (3.14 KB, patch)
2024-01-26 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29510: Make objects.find call search_limited if present (1.45 KB, patch)
2024-01-26 07:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29510: (follow-up) Adapt GET /patrons/:patron_id (1.27 KB, patch)
2024-01-26 07:49 UTC, Marcel de Rooy
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-17 21:01:09 UTC

    
Comment 1 Tomás Cohen Arazi 2021-11-17 21:11:31 UTC
Created attachment 127759 [details] [review]
Bug 29510: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-11-17 21:11:37 UTC
Created attachment 127760 [details] [review]
Bug 29510: Make objects.find call search_limited if present

This patch makes objects.find implicitly update the passed
*$result_set* to use search_limited. This way no object leaks could
happen without noticing.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail because search_limited is not used
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Results are correctly filtered based on userenv!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-11-17 21:11:42 UTC
Created attachment 127761 [details] [review]
Bug 29510: (follow-up) Adapt GET /patrons/:patron_id

This patch makes GET /patrons/:patron_id rely on this new behavior from the
objects.find helper.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Martin Renvoize 2021-11-18 07:56:11 UTC
Created attachment 127772 [details] [review]
Bug 29510: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-11-18 07:56:15 UTC
Created attachment 127773 [details] [review]
Bug 29510: Make objects.find call search_limited if present

This patch makes objects.find implicitly update the passed
*$result_set* to use search_limited. This way no object leaks could
happen without noticing.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail because search_limited is not used
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Results are correctly filtered based on userenv!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-11-18 07:56:18 UTC
Created attachment 127774 [details] [review]
Bug 29510: (follow-up) Adapt GET /patrons/:patron_id

This patch makes GET /patrons/:patron_id rely on this new behavior from the
objects.find helper.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-11-18 07:56:37 UTC
Nice work.. signing off
Comment 8 Jonathan Druart 2021-11-18 08:32:39 UTC
Then we get a 404, is that expected?
Comment 9 Jonathan Druart 2021-11-18 08:38:21 UTC
We can access holds using /api/v1/patrons/5/holds, I don't think we should.

Note that /cgi-bin/koha/members/holdshistory.pl?borrowernumber=5 is accessible using Koha, but that sounds like a bug.
Comment 10 Jonathan Druart 2021-11-18 08:45:02 UTC
And, finally, what about /api/v1/checkouts with x-koha-embed = 'issuer'?
Comment 11 Jonathan Druart 2021-11-18 08:45:45 UTC
(sorry just noticed I should have put the comments on the other bug report)
Comment 12 Tomás Cohen Arazi 2021-11-18 10:16:19 UTC
(In reply to Jonathan Druart from comment #10)
> And, finally, what about /api/v1/checkouts with x-koha-embed = 'issuer'?

I'm going through them. Just was the end of my day.
Comment 13 Tomás Cohen Arazi 2021-11-18 10:17:32 UTC
(In reply to Jonathan Druart from comment #8)
> Then we get a 404, is that expected?

I talked with Martin and Kyle about it, the other option was 403, but 404 seems better.
Comment 14 Jonathan Druart 2021-11-18 10:48:26 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> (In reply to Jonathan Druart from comment #8)
> > Then we get a 404, is that expected?
> 
> I talked with Martin and Kyle about it, the other option was 403, but 404
> seems better.

Don't we raise 403 for other similar situations?
Comment 15 Tomás Cohen Arazi 2021-11-18 11:03:22 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Tomás Cohen Arazi from comment #13)
> > (In reply to Jonathan Druart from comment #8)
> > > Then we get a 404, is that expected?
> > 
> > I talked with Martin and Kyle about it, the other option was 403, but 404
> > seems better.
> 
> Don't we raise 403 for other similar situations?

Not really. It is usually when trying to change things without the right permissions (e.g. Koha/REST/V1/Checkouts.pm:131)

But we have constructs like this (mock, didn't search a real one):

my $biblio = Koha::Biblios->search(id);
unless ( $biblio ) {
    return 404
}

my $holds = $biblio->holds
...
Comment 16 Jonathan Druart 2021-12-02 13:32:55 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > (In reply to Tomás Cohen Arazi from comment #13)
> > > (In reply to Jonathan Druart from comment #8)
> > > > Then we get a 404, is that expected?
> > > 
> > > I talked with Martin and Kyle about it, the other option was 403, but 404
> > > seems better.
> > 
> > Don't we raise 403 for other similar situations?
> 
> Not really. It is usually when trying to change things without the right
> permissions (e.g. Koha/REST/V1/Checkouts.pm:131)
> 
> But we have constructs like this (mock, didn't search a real one):
> 
> my $biblio = Koha::Biblios->search(id);
> unless ( $biblio ) {
>     return 404
> }
> 
> my $holds = $biblio->holds
> ...

In this case it makes sense to return a 404, the biblio does not exist, isn't it?
Comment 17 Katrin Fischer 2022-03-14 20:54:41 UTC
1) I am sorry, these patches no longer apply:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 29510: Regression tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Koha/REST/Plugin/Objects.t
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Bug 29510: Make objects.find call search_limited if present
Using index info to reconstruct a base tree...
M	Koha/REST/Plugin/Objects.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/REST/Plugin/Objects.pm
No changes -- Patch already applied.
Applying: Bug 29510: (follow-up) Adapt GET /patrons/:patron_id
error: sha1 information is lacking or useless (Koha/REST/V1/Patrons.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 29510: (follow-up) Adapt GET /patrons/:patron_id
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-29510-follow-up-Adapt-GET-patronspatronid-lYzuyv.patch

2) Second: I am not sure what if 404 or 403 is better, but from what I understand:

403: Forbidden - would lend itself to something like a permission error or an 'illegal' operation.
404: Not found - would lend itself to situations where something is requested that doesn't exist.

I was trying to look at the tests to better understand what the situation is here, but there only seems to be success tests (200).
Comment 18 Martin Renvoize 2024-01-22 16:12:58 UTC
I think this is still rather important.. without it we're risking data leak.
Comment 19 Martin Renvoize 2024-01-22 16:34:54 UTC
Created attachment 161253 [details] [review]
Bug 29510: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2024-01-22 16:34:56 UTC
Created attachment 161254 [details] [review]
Bug 29510: Make objects.find call search_limited if present

This patch makes objects.find implicitly update the passed
*$result_set* to use search_limited. This way no object leaks could
happen without noticing.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail because search_limited is not used
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Results are correctly filtered based on userenv!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2024-01-22 16:34:59 UTC
Created attachment 161255 [details] [review]
Bug 29510: (follow-up) Adapt GET /patrons/:patron_id

This patch makes GET /patrons/:patron_id rely on this new behavior from the
objects.find helper.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Jonathan Druart 2024-01-22 21:11:51 UTC
All previous comments got an answer?
Comment 23 Jonathan Druart 2024-01-22 21:12:21 UTC
Missing QA.
Comment 24 Marcel de Rooy 2024-01-26 07:33:49 UTC
Looking here
Comment 25 Marcel de Rooy 2024-01-26 07:49:18 UTC
Created attachment 161477 [details] [review]
Bug 29510: Regression tests

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2024-01-26 07:49:20 UTC
Created attachment 161478 [details] [review]
Bug 29510: Make objects.find call search_limited if present

This patch makes objects.find implicitly update the passed
*$result_set* to use search_limited. This way no object leaks could
happen without noticing.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail because search_limited is not used
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Results are correctly filtered based on userenv!
5. Sign off :-D

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2024-01-26 07:49:23 UTC
Created attachment 161479 [details] [review]
Bug 29510: (follow-up) Adapt GET /patrons/:patron_id

This patch makes GET /patrons/:patron_id rely on this new behavior from the
objects.find helper.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Katrin Fischer 2024-01-27 08:52:27 UTC
What is the risk assessment here with the upcoming releases in mind?
Comment 29 Martin Renvoize 2024-01-27 10:19:24 UTC
It's minor in my opinion. We've lived this way for over two years, one more release shouldn't be an issue
Comment 30 Katrin Fischer 2024-01-29 11:31:15 UTC
(In reply to Martin Renvoize from comment #29)
> It's minor in my opinion. We've lived this way for over two years, one more
> release shouldn't be an issue

We'll hold it for next month then. But warrants a security release?
Comment 31 Martin Renvoize 2024-02-15 09:47:42 UTC
Pushed to master for 24.05.00