search

loculus-project / loculus

An open-source software package to power microbial genomic databases
https://loculus.org
GNU Affero General Public License v3.0
37 stars 2 forks source link

Decide what to do with openid-client v6: it's a total rewrite, expensive migration, wait and see? #3260

Open corneliusroemer opened 2 days ago

corneliusroemer commented 2 days ago

I looked briefly into the major bump of openid-client from v5 to v6. v6 is a complete rewrite. Our current auth middleware is tightly coupled to the v5 API. So upgrading is almost the same as moving to another library - unless I'm missing something.

For now it seems v5 will still get limited support, so I'll silence dependabot for now and not worry as long as things are working - dependabot should hopefully still let us know about security fixes. Though it won't let us know about other v5 bumps per https://github.com/dependabot/dependabot-core/issues/9330

https://github.com/panva/openid-client

Dependabot PR: https://github.com/loculus-project/loculus/pull/3102

corneliusroemer commented 1 day ago

I wonder whether it might make sense to move to keycloak-js as we're using keycloak already, might be smoother. @fengelniederhammer what was the thinking behind using openid-client back in the day when you chose that library?

https://www.keycloak.org/securing-apps/javascript-adapter

fengelniederhammer commented 1 day ago

There was some reason, probably @JonasKellerer knows?

JonasKellerer commented 1 day ago

At that time we also looked into this. When I remender correctly, keycloak-js needed a "window", which we dont have in the astro-backend and we could not to get it working. On the frontend it worked well. Maybe they have changed something since then?

corneliusroemer commented 20 hours ago

Thanks that's useful to know! If you happen to find a PR or notes where you tried out keycloak-js that could be usefully linked here.

Did you look at other libraries? The current setup seems quite complicated, I think we could hopefully simplify it. Part of the complexity seems to be due to the client and now it looks like it'd get even worse. There must be a better way.

fengelniederhammer commented 20 hours ago

At the time when we integrated Keycloak, there was no package that could handle OIDC in Astro properly. There are some libraries that handle it on client-side quite well, and there is a package for express that seemed decent (but we didn't try it).

That's why we ended up implementing the flow ourselves (which is far from ideal!).

I doubt that the situation has changed, but it would be good to get rid of our own implementation.

fhennig commented 17 hours ago

I'm curious about this, but I think maybe I don't fully understand yet how it works. Maybe also something to discuss on monday? @chaoran-chen