[Bug 28157] New: Add the ability to set a library from which an API request pretends to come from
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 Bug ID: 28157 Summary: Add the ability to set a library from which an API request pretends to come from Change sponsored?: --- Product: Koha Version: master 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 Some actions results will depend on the logged-in library, in the Koha UI. For example, extended attributes are not displayed on a library if limits have been set. And the same applies for setting them. Our current code on the API front controller sets userenv with the logged-in patron library. We should add a way to override it through a header, and checks should prevent invalid combinations: i.e. IndependentBranches + patron not superlibrarian => reject the request. -- 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=28157 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com Status|NEW |ASSIGNED -- 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=28157 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- For reference: header.inc:152 [% IF !( Koha.Preference('IndependentBranches') && !CAN_user_superlibrarian && !CAN_user_editcatalogue_edit_catalogue ) %] is the only place this is checked so the current behavior, to replicate here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- To record a discussion Tomas and I had out of band.. we wondered if there were perhaps design flaws with our routing pattern.. should we not be nested under /libraries/{librar_id}/* for routes that require a 'logged in library' as an example.. rather than sending along a 'session header'? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 119925 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119925&action=edit Bug 28157: Add Koha::Patron->can_log_into This patch adds a new method that tells if the patron is allowed to be logged into certain library. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 119926 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119926&action=edit Bug 28157: Regression tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 119927 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119927&action=edit Bug 28157: Add handling for the x-koha-library header This patch introduces a new header handling. The key idea is that on Koha's base classes there's broad usage of C4::Context->userenv to determine the current library and make decisions based on that. API requests, on the other hand, might not be tied to sessions (stateless) which is the way current library is retrieved. So we need a way to properly specify what library the request is trying to act as coming from. To test: 1. Apply this patchset 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass! 3- Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- My attempt to this. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28002 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28002 [Bug 28002] Add optional extended_attributes param to POST /patrons -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119925|0 |1 is obsolete| | --- Comment #7 from David Nind <david@davidnind.com> --- Created attachment 119930 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119930&action=edit Bug 28157: Add Koha::Patron->can_log_into This patch adds a new method that tells if the patron is allowed to be logged into certain library. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119926|0 |1 is obsolete| | --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 119931 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119931&action=edit Bug 28157: Regression tests Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119927|0 |1 is obsolete| | --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 119932 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119932&action=edit Bug 28157: Add handling for the x-koha-library header This patch introduces a new header handling. The key idea is that on Koha's base classes there's broad usage of C4::Context->userenv to determine the current library and make decisions based on that. API requests, on the other hand, might not be tied to sessions (stateless) which is the way current library is retrieved. So we need a way to properly specify what library the request is trying to act as coming from. To test: 1. Apply this patchset 2. Run: $ kshell k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t => SUCCESS: Tests pass! 3- Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 119959 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119959&action=edit Bug 28157: Move swagger file to YAML format This patch changes the base OpenAPI file (swagger.json) into YAML. The motivation for this, is adding more documentation, in Markdown. JSON doesn't accept multiline strings, so this seems like a good move for readability. To test: 1. Verify your API is functional 2. Apply this patch 3. Repeat 1 => SUCCESS: No changes, really 4. Point your browser to /api/v1/.html => SUCCESS: Some nicely formatted description of the API can be seen. It includes information about x-koha-library. 5. 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=28157 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28189 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28189 [Bug 28189] Move the base swagger file to YAML -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119959|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #11 from David Cook <dcook@prosentient.com.au> --- I do not understand this at all. In terms of AuthN and AuthZ, you'd want to use the user, so the user session should determine the library... Why include the library in the route? I think that I must be missing something here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to David Cook from comment #11)
I do not understand this at all. In terms of AuthN and AuthZ, you'd want to use the user, so the user session should determine the library...
Why include the library in the route? I think that I must be missing something here.
Because... with a non-api login you have a cookie with context.. that context include a library for your current session.. it may, or may not match the users homebranch. (You can switch library after all.. assuming you've not set independent branches). In the API, we don't have such a context.. the user may be at their homebranch.. or they may be elsewhere.. so we need some way of conveying that the the API for routes that require that data. Tomas and I discussed is and agreed that long term.. API v2 long term.. we should actually move any routes that require such context under /libraries/library_id/whatever/action.. but that's a big change, so for v1 to get the functionality we opted to add an optional header for it.. which defaults to the users homebranch if not passed. Hope that helps clarify David. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize from comment #13)
(In reply to David Cook from comment #11)
I do not understand this at all. In terms of AuthN and AuthZ, you'd want to use the user, so the user session should determine the library...
Why include the library in the route? I think that I must be missing something here.
Because... with a non-api login you have a cookie with context.. that context include a library for your current session.. it may, or may not match the users homebranch. (You can switch library after all.. assuming you've not set independent branches).
In the API, we don't have such a context.. the user may be at their homebranch.. or they may be elsewhere.. so we need some way of conveying that the the API for routes that require that data.
I think that I understand what you're saying, but I still don't understand why it would matter. If it's about determining which rules to apply to which branch, surely that branch data should be set in the user session? Why embed it in the API?
Tomas and I discussed is and agreed that long term.. API v2 long term.. we should actually move any routes that require such context under /libraries/library_id/whatever/action.. but that's a big change, so for v1 to get the functionality we opted to add an optional header for it.. which defaults to the users homebranch if not passed.
Do you have any concrete examples? I can't really think of any similar API endpoints in other systems that would be like that?
Hope that helps clarify David.
Not really but I really appreciate you taking the time to try to clarify it for me :). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- Actually, after reviewing the patches again, I think that I get the idea. With an interactive session, we're logging in with username, password, and target branch. However, with an API session, we're logging in using only username and password, so we don't really have any way of providing a branch. So unless we require people to switch branches using an API endpoint first, the only other option is to provide that x-koha-library header or I suppose embed the library ID in the route like you've suggested. I suppose an alternative would be to pass the target branch in parameters but that would only work for certain endpoints. Hmm feels too abstract at the moment for me to really think of concrete examples. But I suppose the patch is already pushed so it doesn't really matter anymore heh. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to David Cook from comment #15)
Actually, after reviewing the patches again, I think that I get the idea.
With an interactive session, we're logging in with username, password, and target branch.
However, with an API session, we're logging in using only username and password, so we don't really have any way of providing a branch.
The catch is, the API is stateless. If it being used from a browser with a session cookie, then we're are all set. But the API is not designed with that only use case in mind.
So unless we require people to switch branches using an API endpoint first, the only other option is to provide that x-koha-library header or I suppose embed the library ID in the route like you've suggested.
Switching branch using a route implies state.
I suppose an alternative would be to pass the target branch in parameters but that would only work for certain endpoints.
The header is a parameter he he. If you mean query parameters, they are expected to be used for filtering purposes in a Restful design.
Hmm feels too abstract at the moment for me to really think of concrete examples.
This dev is actually written to solve a very concrete problem we noticed with extended attributes routes. Extended attributes visibility and setability are by design (in Koha) something that depends on the current library. Look at Koha::Patron::Attributes->filter_by_branch (don't remember the exact name, on the phone).
But I suppose the patch is already pushed so it doesn't really matter anymore heh.
It does! So next time you need to write routes, you can take advantage of it! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi from comment #16)
The catch is, the API is stateless. If it being used from a browser with a session cookie, then we're are all set. But the API is not designed with that only use case in mind.
I find the word "stateless" to be a bit misleading. The only difference between the OAuth token and the cookie (with a session ID) is where the state is stored. With the browser cookie, we let people specify the library they want. You could do the same thing with the OAuth token. When you make the token request, you could request a particular scope that would grant you access for a particular library. But... we don't use OAuth scopes, so we don't currently have that ability. Plus... we also support Basic auth which doesn't carry any state so fair enough. (Of course, with the basic auth, we could actually have a login endpoint where someone specifies a branch and it returns a JWT that they use going forward, but oh well.) I'll concede the point.
I suppose an alternative would be to pass the target branch in parameters but that would only work for certain endpoints.
The header is a parameter he he. If you mean query parameters, they are expected to be used for filtering purposes in a Restful design.
I was referring more so to the data sent to the particular endpoint. But I was working off abstract ideas and no concrete examples. I was thinking in terms of posting a JSON object to the API and one of the keys being the branch, but yeah it wouldn't be particularly RESTful. Personally, I find REST to be a broken concept. Last time I looked at the API, it wasn't RESTful in many ways, although that was a while ago. I find REST to be too dogmatic to be practical anyway though.
This dev is actually written to solve a very concrete problem we noticed with extended attributes routes. Extended attributes visibility and setability are by design (in Koha) something that depends on the current library. Look at Koha::Patron::Attributes->filter_by_branch (don't remember the exact name, on the phone).
That's still fairly abstract. What's the particular use case? (Also I couldn't find the method that you mentioned...guessing it must be something different.) With the example of the patron attribute branch limitation... is the idea that an API user has requested a list of available patron attributes and it should be limited by a branch? In the browser context, that's their login library. I can't think of a user-specific non-browser context. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 --- Comment #18 from David Cook <dcook@prosentient.com.au> ---
With the example of the patron attribute branch limitation... is the idea that an API user has requested a list of available patron attributes and it should be limited by a branch? In the browser context, that's their login library. I can't think of a user-specific non-browser context.
Ah wait maybe this is where it clicks. So the concrete example might be: /libraries/<library_id>/patron_attributes/list So you're listing all the patron attributes available for that library? But then that's not related to the user at all so I'm confused again haha. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27760 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27760 [Bug 27760] Add handling for x-koha-override -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28157 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #19 from Fridolin Somers <fridolin.somers@biblibre.com> --- Enhancement not pushed to 20.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org