Created attachment 127759 [details] [review] Bug 29510: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
Nice work.. signing off
Then we get a 404, is that expected?
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.
And, finally, what about /api/v1/checkouts with x-koha-embed = 'issuer'?
(sorry just noticed I should have put the comments on the other bug report)
(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.
(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.
(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?
(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 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?
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).
I think this is still rather important.. without it we're risking data leak.
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>
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>
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>
All previous comments got an answer?
Missing QA.
Looking here
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>
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>
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>
What is the risk assessment here with the upcoming releases in mind?
It's minor in my opinion. We've lived this way for over two years, one more release shouldn't be an issue
(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?
Pushed to master for 24.05.00