Description
David Cook
2024-04-10 04:37:09 UTC
Then again... maybe I'm wrong. Maybe this endpoint just needs a subpermission of "validate_borrowers". That would be a very easy change to make, and it would be backwards compatible, since "borrowers: 1" would already include "validate_borrowers". -- Of course, at some point, we'd need an ILS-DI GetPatronInfo replacement which doesn't reveal everything about the patron, but gives a third-party system enough to work with. Martin has been interested in this topic in terms of scopes that users consent to. Of course, part of the issue for these third-party systems is that their API user does things "outside of the context of a user". It would be great to have integrations where they redirect you to Koha, you consent and gives access to particular scopes, and then it redirects you back to the third-party system. And they act on behalf of you as the user but within your context as a user. So those API calls could place holds but only for you as an authenticated user. Created attachment 165447 [details] [review] Bug 36561: Add "validate_borrowers" permission for /api/v1/auth/password/validation This change adds a "validate_borrowers" permission which allows a user to only validate borrowers by using the /api/v1/auth/password/validation endpoint. This avoids scenarios where you want third-parties to authenticate a user without giving them full permissions to perform CRUD operations on user data. To test: 1. Apply patch 2. Run "koha-upgrade-schema kohadev" 3. koha-plack --reload kohadev 4. prove -v t/db_dependent/api/v1/password_validation.t 5. Visit http://localhost:8081/cgi-bin/koha/members/member-flags.pl?member=51 6. Note that a new subpermission "validate_borrowers" appears under the "borrowers" permission (In reply to David Cook from comment #1) > Maybe this endpoint just needs a subpermission of "validate_borrowers". > > That would be a very easy change to make, and it would be backwards > compatible, since "borrowers: 1" would already include "validate_borrowers". This is the option I've opted for, since it was the most straightforward and backwards compatible. Hi David. I'm not seeing the permission. All I see under "Add, modify and view patron information (borrowers)" are: Delete patrons (delete_borrowers) Add, modify and view patron information (edit_borrowers) Search, list and view patrons (list_borrowers) I see the new permission if I do a reset_all in KTD. Otherwise, as far as I can tell, step 2 of the test plan isn't doing anything (for me anyway). Does this means it needs a database update for existing installations? Not really sure how that should work... David (In reply to David Nind from comment #5) > I'm not seeing the permission. All I see under "Add, modify and view patron > information (borrowers)" are: Thanks, David! It looks like I missed adding the database update to the commit! Created attachment 165713 [details] [review] Bug 36561: Add "validate_borrowers" permission for /api/v1/auth/password/validation This change adds a "validate_borrowers" permission which allows a user to only validate borrowers by using the /api/v1/auth/password/validation endpoint. This avoids scenarios where you want third-parties to authenticate a user without giving them full permissions to perform CRUD operations on user data. To test: 1. Apply patch 2. Run "koha-upgrade-schema kohadev" 3. koha-plack --reload kohadev 4. prove -v t/db_dependent/api/v1/password_validation.t 5. Visit http://localhost:8081/cgi-bin/koha/members/member-flags.pl?member=51 6. Note that a new subpermission "validate_borrowers" appears under the "borrowers" permission Created attachment 165718 [details] [review] Bug 36561: Add "validate_borrowers" permission for /api/v1/auth/password/validation This change adds a "validate_borrowers" permission which allows a user to only validate borrowers by using the /api/v1/auth/password/validation endpoint. This avoids scenarios where you want third-parties to authenticate a user without giving them full permissions to perform CRUD operations on user data. To test: 1. Apply patch 2. Run "koha-upgrade-schema kohadev" 3. koha-plack --reload kohadev 4. prove -v t/db_dependent/api/v1/password_validation.t 5. Visit http://localhost:8081/cgi-bin/koha/members/member-flags.pl?member=51 6. Note that a new subpermission "validate_borrowers" appears under the "borrowers" permission Signed-off-by: David Nind <david@davidnind.com> Created attachment 166545 [details] [review] Bug 36561: Add "validate_borrowers" permission for /api/v1/auth/password/validation This change adds a "validate_borrowers" permission which allows a user to only validate borrowers by using the /api/v1/auth/password/validation endpoint. This avoids scenarios where you want third-parties to authenticate a user without giving them full permissions to perform CRUD operations on user data. To test: 1. Apply patch 2. Run "koha-upgrade-schema kohadev" 3. koha-plack --reload kohadev 4. prove -v t/db_dependent/api/v1/password_validation.t 5. Visit http://localhost:8081/cgi-bin/koha/members/member-flags.pl?member=51 6. Note that a new subpermission "validate_borrowers" appears under the "borrowers" permission Signed-off-by: David Nind <david@davidnind.com> Created attachment 166546 [details] [review] Bug 36561: (QA follow-up) Add exec flag to installer, tidy, fix comment Created attachment 166547 [details] [review] Bug 36561: (QA follow-up) Be consistent with other permission requirements Most subpermissions are not added as a list - updating to mathc others in the file Added a follow-up to match the way other permissions are required in the file - let me know if you had a reason to use a list David Created attachment 166549 [details] [review] Bug 36561: Add "validate_borrowers" permission for /api/v1/auth/password/validation This change adds a "validate_borrowers" permission which allows a user to only validate borrowers by using the /api/v1/auth/password/validation endpoint. This avoids scenarios where you want third-parties to authenticate a user without giving them full permissions to perform CRUD operations on user data. To test: 1. Apply patch 2. Run "koha-upgrade-schema kohadev" 3. koha-plack --reload kohadev 4. prove -v t/db_dependent/api/v1/password_validation.t 5. Visit http://localhost:8081/cgi-bin/koha/members/member-flags.pl?member=51 6. Note that a new subpermission "validate_borrowers" appears under the "borrowers" permission Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 166550 [details] [review] Bug 36561: (QA follow-up) Add exec flag to installer, tidy, fix comment Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 166551 [details] [review] Bug 36561: (QA follow-up) Be consistent with other permission requirements Most subpermissions are not added as a list - updating to mathc others in the file Signed-off-by: Nick Clemens <nick@bywatersolutions.com> I feel like the permission description as is might cause some confusion around staff, especially as it's currently only used on the API and has no effect in the interface. Suggestion: Verify user credentials via REST API I am not super happy about having a permission that only really makes sense for the API right now, but not sure what else we could do? Also: Should we do a database update assigning this permission to users with borrowers permission currently? (In reply to Katrin Fischer from comment #16) > I feel like the permission description as is might cause some confusion > around staff, especially as it's currently only used on the API and has no > effect in the interface. > > Suggestion: > Verify user credentials via REST API That's a good point. I'd be happy with that suggestion. > I am not super happy about having a permission that only really makes sense > for the API right now, but not sure what else we could do? I agree that it feels weird, but I think over time it'll make more sense, especially if we grouped them. The other day I was setting up a SIP2 user, and I had no idea what permissions it should have. I patterned it off a previous one I'd set up, but it got me thinking how many unintended consequences can occur from our current permissions, especially in terms of interplay with the API. Realistically, you just want a SIP user to be able to connect to the SIP server and do SIP operations. I think many of us know our permission system is overdue for an overhaul, but no one is sure what direction to go. (I recall Martin suggesting how the Koha Foundation getting a consultant could be handy for architectural directions, and this is one that could be a good target...) > > Also: Should we do a database update assigning this permission to users with > borrowers permission currently? No, because the "borrowers" permission is already the top level, and they implicitly have this permission. We'd just want to encourage people to use this fine-grained permission moving forward. I think we're a bit stuck here at the moment. What are we doing next to move this along? (In reply to David Cook from comment #18) > I think we're a bit stuck here at the moment. What are we doing next to move > this along? Maybe we're better off exploring bug 37144 for now. My initial reasoning for this validate_borrowers permission was that I was uncomfortable with giving full blown "borrowers" permission to a third-party. But if it was a limited view... that wouldn't be so bad. Maybe we could change from "borrowers: 1" to "bororwers: list_borrowers" coupled with bug 37144... that could be doable. (Then the main issue I see is being able to enumerate the borrowers using an increasing integer, but I don't think we're going to replace borrowernumber with a UUID any time soon...) (In reply to David Cook from comment #19) > Maybe we're better off exploring bug 37144 for now. My initial reasoning for > this validate_borrowers permission was that I was uncomfortable with giving > full blown "borrowers" permission to a third-party. Of course, I think bug 37144 has a long way to go before it's ready... (In reply to David Cook from comment #17) > (In reply to Katrin Fischer from comment #16) > > I feel like the permission description as is might cause some confusion > > around staff, especially as it's currently only used on the API and has no > > effect in the interface. > > > > Suggestion: > > Verify user credentials via REST API > > That's a good point. I'd be happy with that suggestion. I was thinking about this again... and I think that this is the approach I'll take. I thought about created an "External Web Services" top-level permission, but "validate_borrowers" is the only permission I could think to add to it. Looking at /cgi-bin/koha/ilsdi.pl and everything else is really covered by our existing permissions. "AuthenticatePatron" is the only exception. That said, sometimes, I think about discovery interfaces, and think it would be great to have permissions for doing things on behalf of an authenticated patron. For instance, Patron A supplies username and password to VuFind, and VuFind does the lookup using "/api/v1/auth/password/validation", and then VuFind is authorized to do things on behalf of only that patron. Of course, that's the point of OAuth2/SSO. VuFind should redirect to Koha, the user logs into Koha, gives consent to certain scopes, and then is redirected back to VuFind with an OAuth2 token, which can then be used for Koha API operations *as the borrower/patron*. Regardless, we do still have systems (like my Keycloak Koha backend SSO extension) that need to do system to system lookups without user involvement. -- Beyond "validate_borrowers", I could imagine a "lookup_borrower" for the ILS-DI LookupPatron type operation. You'd first use "/api/v1/auth/password/validation" and then lookup "/api/v1/patrons/51" (or perhaps a third-party specific endpoint) but only get a minimal record back. -- Anyway, for now we may as well just put them under "borrowers" with a special description... we can always move them under a "external_web_services" permission or something later. (In reply to Katrin Fischer from comment #16) > Also: Should we do a database update assigning this permission to users with > borrowers permission currently? Actually, I think you're right here. I think that I've misunderstood what the permission 'borrowers: "1"' means! I thought that it meant that you had to have the top level "borrowers" permission, but it actually means that you can have *any* borrowers permission. Looking at the rest of the API spec... I get the feeling that we've made mistakes for other endpoints too... In the wild, we typically see third-parties using ILS-DI "AuthenticatePatron" then ILS-DI "GetPatronInfo", and really... that's leaking more information than I'd like. We could try to replace it with "/api/v1/auth/password/validation" and "/api/v1/patrons/XXX" (which is what I use in my Keycloak SSO extension which uses Koha as the user database), but "/api/v1/patrons/XXX" also leaks a lot of data plus with x-koha-embed I think it has the capacity to leak even more data than we realize. I think that we've been so focused on the REST API in terms of Koha CRUD and convenience that we haven't really thought about how it can be used (securely) by third-parties. Going back to bug 37144... maybe we should have an endpoint "/api/v1/patron_profiles/XXX". The patron profile would sit in front of the actual patron data, and ideally be configurable... |