Bug 36094 - svc/authentication needs adjustments
Summary: svc/authentication needs adjustments
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 36194 36560
  Show dependency treegraph
 
Reported: 2024-02-14 12:46 UTC by Jonathan Druart
Modified: 2024-04-10 12:34 UTC (History)
1 user (show)

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


Attachments
Bug 36094: Remove svc/authentication (2.50 KB, patch)
2024-02-14 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-02-14 12:46:58 UTC
Looks like we can remove it.
Comment 1 Jonathan Druart 2024-02-14 12:47:43 UTC
Created attachment 162146 [details] [review]
Bug 36094: Remove svc/authentication

Looks like this script is not used by Koha internally, we can remove it.
Comment 2 Katrin Fischer 2024-02-14 19:01:11 UTC
I believe it was not intended for internal but for external use. IIRC it might be used by MARCEdit ior other external cataloguing editors that allow you to update records in Koha (unless they have been updated recently). See: https://wiki.koha-community.org/wiki/Koha_/svc/_HTTP_API
Comment 3 Katrin Fischer 2024-02-14 19:04:09 UTC
https://blog.reeset.net/archives/1123
Comment 4 David Cook 2024-02-15 04:44:21 UTC
(In reply to Katrin Fischer from comment #2)
> I believe it was not intended for internal but for external use. IIRC it
> might be used by MARCEdit ior other external cataloguing editors that allow
> you to update records in Koha (unless they have been updated recently). See:
> https://wiki.koha-community.org/wiki/Koha_/svc/_HTTP_API

I think that you're right. I think that MarcEdit still works that way. 

Due to necessary changes, I think that we will be breaking that integration soon though one way or another. 

But I think while we still have the /svc/ API then we'll need to keep this script. I think we'd only be able to remove it when we remove the whole thing.
Comment 5 David Cook 2024-02-15 04:58:41 UTC
misc/bin/connexion_import_daemon.pl should probably use svc/authentication instead of abusing /cgi-bin/koha/mainpage.pl to login...
Comment 6 Jonathan Druart 2024-02-15 08:19:28 UTC
There is no reason to work on improving svc, we need to provide similar features from the REST API. Are you willing to work on this (soon and fast)?
Comment 7 Katrin Fischer 2024-02-15 08:42:47 UTC
I am not against removing, but I think we should not just delete the script.
Ideally we need to deprecate and announce first to give programs like MARCEdit a chance to catch up. We could break people's workflow quite badly and suddenly, if they rely on the integration. 
 And we need to make sure the same things can be achieved with the REST API. I think we got all pieces in place there now, but not for that long.
Comment 8 David Cook 2024-02-15 22:31:12 UTC
(In reply to Katrin Fischer from comment #7)
> I am not against removing, but I think we should not just delete the script.
> Ideally we need to deprecate and announce first to give programs like
> MARCEdit a chance to catch up. We could break people's workflow quite badly
> and suddenly, if they rely on the integration. 
>  And we need to make sure the same things can be achieved with the REST API.
> I think we got all pieces in place there now, but not for that long.

+1
Comment 9 David Cook 2024-02-15 23:26:46 UTC
(In reply to David Cook from comment #8)
> (In reply to Katrin Fischer from comment #7)
> > I am not against removing, but I think we should not just delete the script.
> > Ideally we need to deprecate and announce first to give programs like
> > MARCEdit a chance to catch up. We could break people's workflow quite badly
> > and suddenly, if they rely on the integration. 
> >  And we need to make sure the same things can be achieved with the REST API.
> > I think we got all pieces in place there now, but not for that long.
> 
> +1

Actually thinking about this again...

Even if we don't delete the script, soon it won't work like it does today. Even without deprecating it... the API is going to break and we are going to break people's workflow quite badly.

As for the suddenness... I think it'll only break on a major version release (although I could be wrong). We'll need to add big warnings on that release.

Unfortunately, since MarcEdit is closed source, we don't really know how it interacts with Koha. We can offer Terry Reese assistance in updating MarcEdit to match the changes to Koha, but he might not accept that offer. 

I will send him an email to gauge his willingness to make changes.
Comment 10 David Cook 2024-02-15 23:28:41 UTC
(In reply to David Cook from comment #9)
> Unfortunately, since MarcEdit is closed source, we don't really know how it
> interacts with Koha. We can offer Terry Reese assistance in updating
> MarcEdit to match the changes to Koha, but he might not accept that offer. 
> 
> I will send him an email to gauge his willingness to make changes.

I think Tomas has previously reached out and hasn't heard back.

Sometimes I receive responses and sometimes I don't.
Comment 11 David Cook 2024-02-16 00:44:37 UTC
Terry has responded saying he's happy to update MarcEdit. He'll just need a sandbox to test against.

So once we've got the changes ready. We can give him a sandbox to test against. 

Hopefully that should solve the problem. 

(But it also means that I think we should close this report as invalid.)
Comment 12 Jonathan Druart 2024-02-16 07:24:12 UTC
(In reply to David Cook from comment #11)
> Terry has responded saying he's happy to update MarcEdit. He'll just need a
> sandbox to test against.
> 
> So once we've got the changes ready. We can give him a sandbox to test
> against. 
> 
> Hopefully that should solve the problem. 
> 
> (But it also means that I think we should close this report as invalid.)

It's not clear to me what's the plan here then.
Comment 13 David Cook 2024-02-21 04:27:31 UTC
It looks like misc/migration_tools/koha-svc.pl actually uses svc/authentication.

I'll come up with something for this.
Comment 14 David Cook 2024-02-21 05:13:05 UTC
(In reply to David Cook from comment #13)
> I'll come up with something for this.

Of course, my thoughts were based off the expectation that check_api_auth() returned a cookie and session ID for all responses. 

Here's the proposed flow:

1. GET /svc/authentication
If authenticated, returns OK and session cookie for authenticated session plus X-CSRF_TOKEN header in response

If not authenticated, returns failed/anon and session cookie for anonymous session plus X-CSRF_TOKEN header in response

2. POST /svc/authentication
If not authenticated, perform authentication using X-CSRF_TOKEN and session cookie for anonymous session

3. POST /svc/new_bib
Do work using X-CSRF_TOKEN from previous response (or meta tag if it's in a web app) and session cookie 

--

Technically, the POST /svc/authentication can use whatever session cookie, so I suppose I'll just create an anonymous session if one is missing. It means extra logic in /svc/authentication, which is suboptimal but it beats creating yet another "check_*" function in C4/Auth.pm or changing the API of our internal auth functions...
Comment 15 David Cook 2024-02-21 05:43:47 UTC
Except check_api_auth() calls check_cookie_auth() and returns its status instead of processing the actual login POST :|.

(I mean a function that starts with "check_*" shouldn't be performing a login in the first place anyway. Koha's auth processes are so overloaded and over-complicated for something that doesn't need to be very hard...)

Ah the issue is because of the "Bug 36102: (follow-up) Add cud-login to the login form" thing. It's checking for "userid" when it should be looking for "login_userid"...
Comment 16 David Cook 2024-02-21 06:17:36 UTC
All good. I worked it out in the end.

See bug 36084.
Comment 17 Jonathan Druart 2024-02-26 15:01:02 UTC
(In reply to David Cook from comment #16)
> All good. I worked it out in the end.
> 
> See bug 36084.

It's not secure.

% curl 'http://localhost:8081/cgi-bin/koha/svc/authentication?login_userid=koha&login_password=koha' 

<?xml version='1.0' standalone='yes'?>
<response>
  <status>ok</status>
</response>
Comment 18 David Cook 2024-02-26 22:09:34 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to David Cook from comment #16)
> > All good. I worked it out in the end.
> > 
> > See bug 36084.
> 
> It's not secure.
> 
> % curl
> 'http://localhost:8081/cgi-bin/koha/svc/
> authentication?login_userid=koha&login_password=koha' 
> 
> <?xml version='1.0' standalone='yes'?>
> <response>
>   <status>ok</status>
> </response>

Yeah, that's a problem with check_api_auth(), which I figured was outside the scope of this particular change. 

I suppose if it's a GET we might be able to delete the credentials out of the $query object before passing it to check_api_auth(). Without doing a lot of refactoring, I think we're probably going to be left with a hacky option like that.
Comment 19 David Cook 2024-02-26 22:10:50 UTC
(In reply to David Cook from comment #18)
> Yeah, that's a problem with check_api_auth(), which I figured was outside
> the scope of this particular change. 
> 
> I suppose if it's a GET we might be able to delete the credentials out of
> the $query object before passing it to check_api_auth(). Without doing a lot
> of refactoring, I think we're probably going to be left with a hacky option
> like that.

I suppose we could add that $op = cud-login like we've done elsewhere too. I was hoping to make minimal changes for the SVC, but that's another idea.
Comment 20 Jonathan Druart 2024-02-27 07:29:35 UTC
why not simply reject if the request_method ne "POST"?
Comment 21 David Cook 2024-02-27 22:29:17 UTC
(In reply to Jonathan Druart from comment #20)
> why not simply reject if the request_method ne "POST"?

Because the GET is used to obtain the CSRF in order to do the POST like I describe in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36094#c14

If MarcEdit is trying to use the SVC API, they'll first do a GET against svc/authentication to get the initial CSRF token and to check if they're authenticated already (since the SVC API uses cookie auth). 

If they're not authenticated, then they POST to svc/authentication to login using the CSRF token they got from the previous GET. 

They could then use the CSRF token they get back from the POST to do the next operation. (In theory all the SVC endpoints should return a CSRF token in their response headers, but I haven't gotten that far. I think you've mentioned elsewhere that the primary concern is Koha's internal use of SVC API so fair enough. But as Katrin mentions in Comment 2 we do need to think about external users too, so I've still got it on my mind.)
Comment 22 David Cook 2024-04-10 02:40:33 UTC
I've bumped into this again with the ILS-DI API (see bug 36560).

But it reminds me that the SVC API won't work for MarcEdit either.
Comment 23 David Cook 2024-04-10 02:52:52 UTC
(In reply to David Cook from comment #22)
> I've bumped into this again with the ILS-DI API (see bug 36560).
> 
> But it reminds me that the SVC API won't work for MarcEdit either.

While "GET /svc/authentication" would work with the SVC API, the ILS-DI API doesn't have an equivalent option. It doesn't use cookie auth; it uses IP auth. 

--

Since it uses IP auth, we can't use our cookie/session-based CSRF tokens with it.

I think for the ILS-DI, we're going to have to just put in an exception. Eventually, we'll want to get rid of the ILS-DI all together, and replace it with equivalent REST API endpoints.

The "/api/v1/auth/password/validation" effectively replaces the "AuthenticatePatron" services in ILS-DI anyway. 

Of course, at some point, we'll add CSRF protection to the REST API. For that, we'll either require OAuth2 which doesn't need the CSRF tokens, or if they are using Basic Auth, we'll need to require people to use cookies and do something like 'GET /svc/authentication". 

Anyway... in summary, I think the ILS-DI needs an exception, since it doesn't use cookie-based auth.
Comment 24 David Cook 2024-04-10 02:55:02 UTC
(In reply to David Cook from comment #23)
> Anyway... in summary, I think the ILS-DI needs an exception, since it
> doesn't use cookie-based auth.

Alternatively, I suppose this could be the death of the ILS-DI API, and we instead focus on making sure there's equivalent functionality in the REST API, so that all consumers can switch over to that API...
Comment 25 David Cook 2024-04-10 03:32:14 UTC
(In reply to David Cook from comment #23)
> Of course, at some point, we'll add CSRF protection to the REST API. For
> that, we'll either require OAuth2 which doesn't need the CSRF tokens, or if
> they are using Basic Auth, we'll need to require people to use cookies and
> do something like 'GET /svc/authentication". 

Actually, looking at Koha/REST/V1/Auth.pm, it looks like neither OAuth2 or Basic Auth would need the CSRF. It's just the cookie auth that would need the CSRF protection, and that would just be from within Koha.