Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
corneliusroemer opened this issue Nov 20, 2024 · 6 comments
Labels
dependencies Pull requests that update a dependency file discussion Open questions refactoring Code requires refactoring

Comments

@corneliusroemer
Copy link
Contributor

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 dependabot/dependabot-core#9330

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

Dependabot PR: #3102

@corneliusroemer corneliusroemer added dependencies Pull requests that update a dependency file discussion Open questions refactoring Code requires refactoring labels Nov 20, 2024
@corneliusroemer
Copy link
Contributor Author

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
Copy link
Contributor

There was some reason, probably @JonasKellerer knows?

@JonasKellerer
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

fhennig commented Nov 21, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file discussion Open questions refactoring Code requires refactoring
Projects
None yet
Development

No branches or pull requests

4 participants