https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff CC| |agustinmoyano@theke.io, | |jonathan.druart+koha@gmail. | |com, | |joonas.kylmala@helsinki.fi, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 127887 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127887&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 127889 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127889&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- We should consider this one for 21.11. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127886|0 |1 is obsolete| | --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 127991 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127991&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127888|0 |1 is obsolete| | --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 127993 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127993&action=edit Bug 29523: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #10 from David Nind <david@davidnind.com> --- Testing notes: 1. Step 6: had to give Henry circulate and catalogue permissions. 2. Step 8: URL should be ../vi/biblios/... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127991|0 |1 is obsolete| | --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 128042 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128042&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127993|0 |1 is obsolete| | --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 128044 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128044&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 128046 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128046&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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.. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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!>? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #21 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Martin Renvoize from comment #18)

Created attachment 128048 [details] [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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #23 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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` -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128042|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128044|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128046|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128048|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #27 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128689&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #29 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128691 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128691&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #31 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128693 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128693&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #33 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #35 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Thx Tomas, go back to your Sunday :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #37 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Out of scope, but heh: libraries_where_can_see_patrons This is a really awful name -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #39 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- - 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #41 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- +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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30019 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128694|0 |1 is obsolete| | --- Comment #44 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 131331 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=131331&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #48 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128689|0 |1 is obsolete| | --- Comment #50 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 136685 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136685&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128691|0 |1 is obsolete| | --- Comment #52 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 136687 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136687&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128693|0 |1 is obsolete| | --- Comment #54 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 136689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136689&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #131332|0 |1 is obsolete| | --- Comment #56 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 136691 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136691&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136692|0 |1 is obsolete| | --- Comment #59 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 136706 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136706&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #61 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #63 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #64 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Comment 60 is waiting for an answer. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #66 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136685|0 |1 is obsolete| | --- Comment #68 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148737 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148737&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136687|0 |1 is obsolete| | --- Comment #70 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148739 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148739&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136689|0 |1 is obsolete| | --- Comment #72 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148741 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148741&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136691|0 |1 is obsolete| | --- Comment #74 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148743 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148743&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136714|0 |1 is obsolete| | --- Comment #76 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148745 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148745&action=edit Bug 29523: Remove no longer required methods -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #78 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #80 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Code for this test has been moved during the rebase somehow. Please fix. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #148737|0 |1 is obsolete| | --- Comment #82 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 150990 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150990&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #148739|0 |1 is obsolete| | --- Comment #84 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 150992 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150992&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #148741|0 |1 is obsolete| | --- Comment #86 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 150994 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150994&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #148743|0 |1 is obsolete| | --- Comment #88 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 150996 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150996&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #148745|0 |1 is obsolete| | --- Comment #90 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 150998 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150998&action=edit Bug 29523: Remove no longer required methods -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #92 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #93 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #94 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #96 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- How can we move this forward? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #98 from David Cook <dcook@prosentient.com.au> --- 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). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #100 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #102 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #104 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #106 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150990|0 |1 is obsolete| | --- Comment #108 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 156940 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156940&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150992|0 |1 is obsolete| | --- Comment #110 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 156942 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156942&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150994|0 |1 is obsolete| | --- Comment #112 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 156944 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156944&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150996|0 |1 is obsolete| | --- Comment #114 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 156946 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156946&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150998|0 |1 is obsolete| | --- Comment #116 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 156948 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156948&action=edit Bug 29523: Remove no longer required methods -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #118 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #120 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 156965 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156965&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #122 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #124 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157049 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157049&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #125 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Running tests and doing a little squashing here at the moment.. should be ready for a final QA run imminently. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156940|0 |1 is obsolete| | --- Comment #127 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157072 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157072&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156942|0 |1 is obsolete| | --- Comment #129 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157074 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157074&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156944|0 |1 is obsolete| | --- Comment #131 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157076 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157076&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156946|0 |1 is obsolete| | --- Comment #133 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157078 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157078&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156948|0 |1 is obsolete| | --- Comment #135 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157080 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157080&action=edit Bug 29523: Remove no longer required methods -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157049|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157071|0 |1 is obsolete| | Attachment #157072|0 |1 is obsolete| | Attachment #157073|0 |1 is obsolete| | Attachment #157074|0 |1 is obsolete| | Attachment #157075|0 |1 is obsolete| | Attachment #157076|0 |1 is obsolete| | Attachment #157077|0 |1 is obsolete| | Attachment #157078|0 |1 is obsolete| | Attachment #157079|0 |1 is obsolete| | Attachment #157080|0 |1 is obsolete| | Attachment #157081|0 |1 is obsolete| | --- Comment #138 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 157406 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157406&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #140 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 157408 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157408&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #142 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 157410 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157410&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #144 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 157412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157412&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #146 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 157414 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157414&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #148 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 157416 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157416&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #150 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #152 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #154 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #156 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #158 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #160 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157673 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157673&action=edit Bug 29523: (follow-up) Comprehensive tests for redaction -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157679|0 |1 is obsolete| | --- Comment #162 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157683 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157683&action=edit Bug 29523: (follow-up) Comprehensive tests for redaction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #164 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Resuming here -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #166 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #168 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Waiting for feedback now. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #170 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157683|0 |1 is obsolete| | --- Comment #172 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 157799 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157799&action=edit Bug 29523: (follow-up) Comprehensive tests for redaction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #174 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Short description of my problem with embeds now: /api/v1/biblios/1/checkouts with x-koha-embed==patron Get complete patron 1 For this same staff user /api/v1/patrons/1 gives a 403 Since he has not edit_borrowers -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157406|0 |1 is obsolete| | --- Comment #176 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157881 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157881&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157408|0 |1 is obsolete| | --- Comment #178 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157883 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157883&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157410|0 |1 is obsolete| | --- Comment #180 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157885 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157885&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157412|0 |1 is obsolete| | --- Comment #182 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157887 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157887&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157414|0 |1 is obsolete| | --- Comment #184 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157889 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157889&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157416|0 |1 is obsolete| | --- Comment #186 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157891 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157891&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #188 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157893 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157893&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA --- Comment #190 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523 --- Comment #193 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 158013 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158013&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.