Bug 40023 - Allow embedding `patron` when validating credentials
Summary: Allow embedding `patron` when validating credentials
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on: 30962
Blocks:
  Show dependency treegraph
 
Reported: 2025-05-28 18:48 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-05-30 00:50 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 40023: Unit tests (1.42 KB, patch)
2025-05-28 19:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40023: Add option to embed patron object on password validation response (3.06 KB, patch)
2025-05-28 19:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2025-05-28 18:48:13 UTC
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.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-05-28 19:24:53 UTC
Created attachment 182831 [details] [review]
Bug 40023: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2025-05-28 19:24:56 UTC
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>
Comment 3 David Cook 2025-05-29 00:00:39 UTC
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.)
Comment 4 Tomás Cohen Arazi (tcohen) 2025-05-29 13:55:54 UTC
(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.
Comment 5 David Cook 2025-05-30 00:28:17 UTC
(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).
Comment 6 David Cook 2025-05-30 00:31:39 UTC
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...
Comment 7 Tomás Cohen Arazi (tcohen) 2025-05-30 00:33:00 UTC
(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.
Comment 8 David Cook 2025-05-30 00:50:07 UTC
(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