[Koha-bugs] [Bug 30988] Add generic OpenIDConnect client implementation

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30988

--- Comment #46 from David Cook <dcook at prosentient.com.au> ---
(In reply to Martin Renvoize from comment #42)
> QA Fail
> * We're missing Unit Tests for the new module you introduce, I'm afraid
> that's a hard fail for now. 

I thought since the foundation Google OpenID Connect code (bug 10988) made it
through without unit tests, this one would be able to as well, since it's just
a slight modification on existing code in Koha.

> It also looks like you have a note to do
> validation here which is missing... little confused as it looks like you are
> passing in a json structure rather than the token string.. so I'd have
> expected that to already be verified?  We could perhaps just use an existing
> library for this Mojo::JWT for instance?

I think that the code comments are misleading. The Koha code does validate the
"iss", "aud", and "exp" claims in the ID token. The only thing it doesn't check
is the JWT signature. (See
https://developers.google.com/identity/protocols/oauth2/openid-connect#validatinganidtoken). 

Mojo::JWT is probably the way to go, but I wonder if it's scope creep though,
since the Google OpenID Connect code doesn't do it currently. 

> Future
> 1. I'd love to see this moved out of preferences and into it's own
> management page.. I could see multiple OpenIDConnect providers being
> configured at the same time and allowing the end user to 'Pick their poison'
> at login.
> 2. If we went the above route, I'd love to see the Google stuff pulled out
> and the preferences it uses also migrated into the above management area.

That's a goal of mine for sure. 

Locally, I have an endpoint called
/cgi-bin/koha/svc/login_openidc/<provider_id>. The controller is located at
"login_openidc" and it parses the remaining URL string to get the ID, and then
looks up the provider in its configuration. It would be very easy to do. 

> 3. The controller/svc script is pretty heavy right now.. I'd love to have
> seen more of this done 'in module' and have the controller lighter and
> perhaps in the REST side of things rather than a new /svc.. but I'm honestly
> not sure how it would fit in REST as yet so have no issue with it in /svc
> for now.

This code is pretty much a copy/paste of bug 10988 so it inherits its problems. 

One could argue that it shouldn't be in /svc or the REST API, because it's not
really an API. It's just an authentication endpoint. In terms of the
controller, it's really just redirecting a browser (and doing some backend work
which should be in a module for sure).

-- 
You are receiving this mail because:
You are watching all bug changes.