Bug 36094

Summary: svc/authentication needs adjustments
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: David Cook <dcook>
Status: ASSIGNED --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36117
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36084
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 36194, 36560    
Attachments: Bug 36094: Remove svc/authentication

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.
Comment 26 David Cook 2024-05-22 02:05:45 UTC
Note that I've updated the Koha wiki documentation for the "Koha /svc/ HTTP API", and I've raised a Github issue for "koha_api" for MarcEdit.

I don't know when MarcEdit will get an update though, so I suspect that there will be some time where MarcEdit users aren't able to directly interface with Koha 24.05+.

But doing my best to try to coordinate this change...
Comment 27 Katrin Fischer 2024-05-22 07:10:10 UTC
(In reply to David Cook from comment #26)
> Note that I've updated the Koha wiki documentation for the "Koha /svc/ HTTP
> API", and I've raised a Github issue for "koha_api" for MarcEdit.
> 
> I don't know when MarcEdit will get an update though, so I suspect that
> there will be some time where MarcEdit users aren't able to directly
> interface with Koha 24.05+.
> 
> But doing my best to try to coordinate this change...

Hi David, thanks for doing this.

Should we put some warning in the release notes? I'd be happy to have some help there.
Comment 28 David Cook 2024-05-22 23:15:17 UTC
(In reply to Katrin Fischer from comment #27)
> Should we put some warning in the release notes? I'd be happy to have some
> help there.

Yeah, I suppose that we should just to be extra explicit. 

I'll put some notes into bug 36084 and we can mark this one as invalid?
Comment 29 Jonathan Druart 2024-05-23 06:15:45 UTC
(In reply to David Cook from comment #28)
> (In reply to Katrin Fischer from comment #27)
> > Should we put some warning in the release notes? I'd be happy to have some
> > help there.
> 
> Yeah, I suppose that we should just to be extra explicit. 
> 
> I'll put some notes into bug 36084 and we can mark this one as invalid?

Can you provide a tldr version explaining why we finally don't want to adjust svc/authentication please?
Comment 30 Katrin Fischer 2024-05-23 07:10:38 UTC
I just wanted to ask the same question - I think a fix would still be good?
Comment 31 Katrin Fischer 2024-05-23 07:11:02 UTC
... but if we don't make it before release, letting people know about that might be good.
Comment 32 David Cook 2024-05-23 07:12:32 UTC
(In reply to Jonathan Druart from comment #29)
> Can you provide a tldr version explaining why we finally don't want to
> adjust svc/authentication please?

(In reply to Katrin Fischer from comment #30)
> I just wanted to ask the same question - I think a fix would still be good?

Because it's already fixed?

As part of bug 36084, I added commit 0fe82b601b4b06ce0e9fcc5ca677810886f26717 on Feb 21 2024. 

So it should be all good.

Unless I'm misunderstanding the questions?
Comment 33 David Cook 2024-05-23 07:13:21 UTC
(In reply to Katrin Fischer from comment #31)
> ... but if we don't make it before release, letting people know about that
> might be good.

If you look at the wiki (I'm too scared to paste the link here because I always get auto-banned when I do), I think that I describe the new workflow reasonably well.

If I don't, I'd be happy to improve my wording.

Thanks :D
Comment 34 Jonathan Druart 2024-05-23 07:31:24 UTC
We don't have cud-, requiring POST, or whatever. The curl command from comment 17 is still valid, everybody can request it.
Comment 35 David Cook 2024-05-24 00:20:25 UTC
(In reply to Jonathan Druart from comment #34)
> We don't have cud-, requiring POST, or whatever. The curl command from
> comment 17 is still valid, everybody can request it.

I don't think cud- applies for the /svc API - at least not the original one.

As for requiring POST and comment 17 for /svc/authentication... that's a good point but that is more so an issue with check_api_auth() and changing that has wide-reaching consequences beyond /svc. 

As for /svc/bib, /svc/new_bib, and /svc/import_bib, they already require POSTs, so they should be fine (not withstanding the same issue with being able to login using a GET via check_api_auth()).
Comment 36 David Cook 2024-05-24 00:31:09 UTC
(In reply to David Cook from comment #35)
> As for requiring POST and comment 17 for /svc/authentication... that's a
> good point but that is more so an issue with check_api_auth() and changing
> that has wide-reaching consequences beyond /svc. 

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

Technically, C4::Auth::get_template_and_user() requires a POST and op=cud-login, which works for a CGI script, because that's the first auth related function called. 

But for /svc scripts (and scripts like them), they call check_api_auth(), so that's the function that would need to be updated. It's unfortunate that it's so wide spread and used in different ways.

Practically, we probably could just have check_api_auth() require a POST when it's performing login functionality. But... we're too late in the release cycle for that I think. 

--

After 24.05 is released, I'd be happy to write a patch that requires POST when check_api_auth() is doing a login. Then we'll have a whole release cycle to find any bugs in that.