Bug 28157

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.

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'?

Created attachment 119925 [details] [review]
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

Created attachment 119926 [details] [review]
Bug 28157: Regression tests

Created attachment 119927 [details] [review]
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

My attempt to this.

Created attachment 119930 [details] [review]
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>

Created attachment 119931 [details] [review]
Bug 28157: Regression tests

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 119932 [details] [review]
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>

Created attachment 119959 [details] [review]
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>

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.

Pushed to master for 21.05, thanks to everybody involved!

(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.

(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 :).

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.

(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!

(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.

> 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.

Enhancement not pushed to 20.11.x