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.
Created attachment 182831 [details] [review] Bug 40023: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 182832 [details] [review] 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>
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.)
(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.
(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).
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...
(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.
(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