[Bug 40023] New: Allow embedding `patron` when validating credentials
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 Bug ID: 40023 Summary: Allow embedding `patron` when validating credentials Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org CC: tomascohen@gmail.com It would be handy for external systems to be able to specify the requirement to embed the patron object in the response of a successful patron credentials validation. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |30962 CC| |lucasmontoya@theke.io, | |nick@bywatersolutions.com Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30962 [Bug 30962] Add POST endpoint for validating a user password -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 182831 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182831&action=edit Bug 40023: Unit tests 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=40023 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 182832 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182832&action=edit Bug 40023: Add option to embed patron object on password validation response This patch adds the option to embed the patron object on a successful password validation. It does so by * Adding a new embed in the spec * Manually handling the embed in the controller * Calling `$c->objects->to_api` on the already available `$patron` object. To test: 1. Apply the unit tests 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/password_validation.t => FAIL: Embedding is not allowed by the spec, or implemented at all. 3. Apply this patch 4. Rebuild the bundled spec: k$ yarn api:bundle 5. Repeat 2 => SUCCESS: Tests pass! 6. You can also play with your favourite REST tool (Postman is mine) 7. 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=40023 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au Status|Needs Signoff |In Discussion --- Comment #3 from David Cook <dcook@prosentient.com.au> --- At the moment the permission required for this endpoint is 'borrowers: "1"', but that's way too permissive. As per bug 36561, this means that any patron with any "borrowers" permission can use this endpoint. With that in mind, someone with "send_messages_to_borrowers" could use bug 40023 to get access to all the patron information, which would be a personal information breach. When I do lock down this endpoint more, then it would be an even bigger disclosure. -- I suppose you could check for edit_borrowers and list_borrowers and return a filtered/censored patron object response. But the idea for this API endpoint was to have a way for third-party systems to authenticate a user using only an ID and password. Very minimal trust and disclosure. While it might be convenient to embed the patron object in the response, that really should be a second API call. (Although I think the argument that if they have edit_borrowers/list_borrowers then maybe they could get an embedded response. That said, this current patch would bypass "view_borrower_infos_from_any_libraries" I believe.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to David Cook from comment #3)
At the moment the permission required for this endpoint is 'borrowers: "1"', but that's way too permissive. As per bug 36561, this means that any patron with any "borrowers" permission can use this endpoint.
My interpretation was 'borrowers: 1' means the top-level permission, thus all borrower related permisions, so maybe too restrictive, even. That's why I proposed this. That said, I like the idea of a configuration patron profile endpoint. In the meantime... what permissions would you require to allow having access to the full patron object? Best regards. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #4)
My interpretation was 'borrowers: 1' means the top-level permission, thus all borrower related permisions, so maybe too restrictive, even. That's why I proposed this.
Yeah, I originally thought "borrowers: 1" meant top-level permission too when I wrote the original endpoint, but I am quite confident that it doesn't. I need to work on a re-vamped permission (and probably review all other API endpoints to be honest...)
That said, I like the idea of a configuration patron profile endpoint.
In the meantime... what permissions would you require to allow having access to the full patron object?
edit_borrowers and list_borrowers can lookup the full patron object via /patrons/{patron_id}, so in the controller you could check for those permissions for the user before embedding the patron object. That would keep it consistent with the /patrons/{patron_id} endpoint. But I'm curious what's the use case you have in mind? I could understand wanting to return something like the user's names, because a third-party self-service kiosk or third-party digital service website might want to login a person using their Koha credentials and display their name without doing a full borrower lookup using /patrons/{patron_id}. (Of course, I'd also argue that third-parties should not have access to /patrons/{patron_id} because that's a personal information disclosure security issue.) Consider the ILSDI API. It has AuthenticatePatron which takes a username/password and returns an ID. The REST API needed that same functionality, so I added it via this endpoint. Then consider "GetPatronInfo" from ILSDI. It hides a lot of personal information. It should probably hide more than it does, but it hides a lot (in a good way). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- Currently looking at the output of /api/v1/patrons/{patron_id} and noticing it includes "overdrive_auth_token" although that's not defined in Koha::Patron::to_api_mapping(). "login_attempts" as well. "staff_notes"... I'd say those shouldn't shared with a third-party system. -- The API is in a weird place at the moment. It's not clear what services are expected to be used only by Koha and which are meant to be used by third-parties, and I think that presents some of the issues around privacy and security... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #7 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to David Cook from comment #6)
Currently looking at the output of /api/v1/patrons/{patron_id} and noticing it includes "overdrive_auth_token" although that's not defined in Koha::Patron::to_api_mapping(). "login_attempts" as well. "staff_notes"... I'd say those shouldn't shared with a third-party system.
--
The API is in a weird place at the moment. It's not clear what services are expected to be used only by Koha and which are meant to be used by third-parties, and I think that presents some of the issues around privacy and security...
We need to move all credentials to their own table. The API exposes our design issues. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #7)
We need to move all credentials to their own table. The API exposes our design issues.
100% agree -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #5)
(In reply to Tomás Cohen Arazi (tcohen) from comment #4)
My interpretation was 'borrowers: 1' means the top-level permission, thus all borrower related permisions, so maybe too restrictive, even. That's why I proposed this.
Yeah, I originally thought "borrowers: 1" meant top-level permission too when I wrote the original endpoint, but I am quite confident that it doesn't. I need to work on a re-vamped permission (and probably review all other API endpoints to be honest...)
You were right, Tomas. I don't know why I was confused about '"borrowerws": "1"', but I've double-checked it and you're right. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #0)
It would be handy for external systems to be able to specify the requirement to embed the patron object in the response of a successful patron credentials validation.
I agree that it would be useful for third-party/external systems to be able to get more information back. I imagine it would be very useful to have fields like name and email. Maybe patron category. -- I'm reminded about OAuth2/OIDC scopes and scope mappers on IdPs. I suppose one way to do this in a Koha-esque way would be to have a system preference that lets you define which fields to include in the patron object response. That way it would be easily adaptable to different Koha instances. We could default to a reasonable minimum while also allowing libraries to easily expand the patron information they want to share with third-parties. If we did a YAML syspref, you could even make different scopes and require different ones from different third-parties. That would be even more powerful. Probably not the best way to do it overall/in the long run, but it would be an all right compromise. And it would be a compromise that would be easy to convert into a different feature down the road, because all the data would be explicit. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36561 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- I noticed that bug 36561 was pushed, and it's got me thinking about this again... Bug 36561 is a move in the right direction, but the API user would still need list_borrowers in order to get patron data for the authenticated patron, which is still a problem. But... I still don't love the idea of sharing the whole patron object. I think we could keep these patches from Tomas, but just add another one that allows for a more selective release of patron information. The only practical short-term solution I can think of is using a system preference to store the choices. (In future, if we had something more fit-to-purpose, we could migrate the syspref as a "default" profile.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40023 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- The "/auth/password/validation" endpoint has so much potential for use in third-party systems, and I would love for us to provide it in a usable yet secure way. I'd love to work with more third-party vendors on this one, but I can't do it from a corporate/vendor perspective - it needs to be done from a community perspective so that the whole world benefits (and so the vendor doesn't have to maintain many branches for different Koha vendors). But since third parties need to work quickly, they'll either require higher permissions for the REST API or use things like ILS-DI/SIP2. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org