Bug 40023

Summary: Allow embedding `patron` when validating credentials
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook, lucasmontoya, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36561
GIT URL: Initiative type: ---
Sponsorship status: --- Crowdfunding goal: 0
Patch complexity: Trivial patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 30962    
Bug Blocks:    
Attachments: Bug 40023: Unit tests
Bug 40023: Add option to embed patron object on password validation response

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
Comment 9 David Cook 2025-07-17 00:04:44 UTC
(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.
Comment 10 David Cook 2025-07-17 00:17:22 UTC
(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.
Comment 11 David Cook 2025-09-17 03:22:20 UTC
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.)
Comment 12 David Cook 2025-09-17 03:23:45 UTC
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.