Migrate OIDC to the new Dev UI

[michalvavrik]

closes: #32031

[sberyozkin]

Hey @michalvavrik Great stuff, will look at it more likely on Mon, enjoy the rest of the weekend, cheers !

[michalvavrik]

Rebased this PR onto #33648

[sberyozkin]

Hey @michalvavrik First of all, major thanks for making this PR happen, appreciated.
I've tried it today, works fine in the new Dev UI, but there are a few glitches and I'm not quite sure if these are Dev UI or Dev Services for Keycloak problems. FYI, I've been testing it with quarkus-quickstarts/security-openid-connect-quickstart.

Initially I was keeping getting 500, after logging in as alice:alice and then typing /tokens in the service path area and choosing Test with Access token - with the server reporting it failed to connect to Keycloak - but then after a few restarts it was gone and I could no longer reproduce, and in this case I'd get 404, or when typed correct /api/users/me - 200, /api/admin - 403. Please give it a few tries, it felt like %prod was ignored in application.properties since I was seeing io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: localhost/127.0.0.1:8180

The other problem I have noticed is that after Keycloak restarts, I can't reconnect to it without clearing all History in the browser, for example, after mvn quarkus:dev and then signing in to Keycloak, if you add quarkus-smallrye-openapi to pom.xml (in order to test Swagger UI from within OIDC Dev UI), then you'll see Keycloak is restarting - just because pom.xml has changed (or you can for example remove quarkus.keycloak.devservices.realm-path=quarkus-realm.json from application.properties which will cause a restart), and then try to go Dev UI and select a Provider: Keycloak provider again and it will fail to connect because Dev UI is not aware that a new quarkus.oidc.auth-server-url is not set

Have a look at the above 2 issues please when you get a chance

Thanks

[michalvavrik]

Hey @michalvavrik First of all, major thanks for making this PR happen, appreciated. I've tried it today, works fine in the new Dev UI, but there are a few glitches and I'm not quite sure if these are Dev UI or Dev Services for Keycloak problems. FYI, I've been testing it with quarkus-quickstarts/security-openid-connect-quickstart.

Initially I was keeping getting 500, after logging in as alice:alice and then typing /tokens in the service path area and choosing Test with Access token - with the server reporting it failed to connect to Keycloak - but then after a few restarts it was gone and I could no longer reproduce, and in this case I'd get 404, or when typed correct /api/users/me - 200, /api/admin - 403. Please give it a few tries, it felt like %prod was ignored in application.properties since I was seeing io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: localhost/127.0.0.1:8180

Interesting, I tested every scenario with 3 quickstarts (sometimes I adjusted it little bit for my needs) and quarkus-quickstarts/security-openid-connect-quickstart was one of them, never got 500. I'll try to reproduce and get back to you with fix or additional questions on reproducer.

[michalvavrik]

The other problem I have noticed is that after Keycloak restarts, I can't reconnect to it without clearing all History in the browser, for example, after mvn quarkus:dev and then signing in to Keycloak, if you add quarkus-smallrye-openapi to pom.xml (in order to test Swagger UI from within OIDC Dev UI), then you'll see Keycloak is restarting - just because pom.xml has changed (or you can for example remove quarkus.keycloak.devservices.realm-path=quarkus-realm.json from application.properties which will cause a restart), and then try to go Dev UI and select a Provider: Keycloak provider again and it will fail to connect because Dev UI is not aware that a new quarkus.oidc.auth-server-url is not set

that makes sense, if component was already created, it has no way to find out it should recall getProperties. I actually thought about this scenario before and my conclusion always was that user is supposed to reload single page application. I can add handling like streaming and keep checking for properties, but it is expensive as hell. @phillip-kruger how do you usually handle need for connection callback after back end reload?

[phillip-kruger]

I think you can replace LitElement with QwcHotReloadElement and implement reload method. If you scratch around you will find an example

[michalvavrik]

I think you can replace LitElement with QwcHotReloadElement and implement reload method. If you scratch around you will find an example

thanks!

[sberyozkin]

Yeah, in the old Dev UI, the reloading was happening indirectly, as it would involve an HTTP request, great though Phillip has prepared a lot of tools like QwcHotReloadElement :-)

[michalvavrik]

@sberyozkin couldn't reproduce 500 and:

• /api/users/me returns 200 for alice, she has role user, so correct
• api/admin returns 400 for alice and 200 for admin, that seems correct to me as only admin has admin role and old DEV UI behaves same
• /api/tokens I can't find this endpoint, so 404 seems okay too

Did you meant that 500 was wrong, but then it worked as expected, or is something I mentioned above incorrect?

it felt like %prod was ignored in application.properties since I was seeing io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: localhost/127.0.0.1:8180

I'd be surprised if ti wasn't, prod profile should only be active when running JAR runner or executable or when activated directly. So when I run quarkus dev -Dquarkus.profile=prod can I reproduce it, but it seems like correct as there is no 8180 running (I didn't start it manually and DEV services don't start container as I specified URL).

Can you please provide steps to reproduce (high level, just so that I understand at which configuration and what is that I'm looking for)

[sberyozkin]

Hi @michalvavrik

Did you meant that 500 was wrong, but then it worked as expected, or is something I mentioned above incorrect?

everything you mentioned there is correct (400 was a typo I guess :-), and yes, I just typed /tokens by mistake first).

Can you please provide steps to reproduce (high level, just so that I understand at which configuration and what is that I'm looking for)

I literally just built your PR branch, then went to quarkus-quickstarts/security-openid-connect-quickstarts, reverted some previous changes I had locally for testing something else and run mvn quarkus:dev. Then after a few retries the problem was gone. It was probably related to some old state, as before testing I had that demo setup to point to the localhost:8180 Keycloak instance, most likely, yeah, sorry about the false alarm

[michalvavrik]

I literally just built your PR branch, then went to quarkus-quickstarts/security-openid-connect-quickstarts, reverted some previous changes I had locally for testing something else and run mvn quarkus:dev. Then after a few retries the problem was gone. It was probably related to some old state, as before testing I had that demo setup to point to the localhost:8180 Keycloak instance, most likely, yeah, sorry about the false alarm

It worries me, but can't reproduce. I'll fix hot reload thing and we shall see if you run into this again when doing review then. Thanks for response.

[michalvavrik]

So QwcHotReloadElement is cool, but it is not exclusively called only when back end reloaded. I didn't find a pattern to (what I call for myself bug, but ...) this, but very roughly in 10 of 50 cases I experienced situation that:

• add dependency
• wait
• hot reload is called
• now I perform new log in as Keycloak container was also restarted
• I'm redirected back
• I'm redirected to router and than back with query params
• web component constructor is called
• connected callback is called
• hot reload is invoked again

I guess it is not a problem for other component, however we need to update query params (remove them) when Keycloak container was restarted as code and state etc. would be invalid. I won't open issue because I don't know how to reproduce it. Sometimes when you really need to reproduce it, you just can't and then it happens few times in row.

Anyway - I introduced properties state which is workaround, but has nice side affect that we will know when properties could have changed (BE changed and we need to perform change detection) and won't perform any actions if nothing changed. IMHO this workaround should be kept anyway.

[phillip-kruger]

I am not sure I follow, but I am sure we can solve your issue. Can you record it somehow ? Or do you want a quick meet to discuss ?

[michalvavrik]

@phillip-kruger gave me few tips on Zulip, I'll try them tomorrow

[sberyozkin]

Thank you, @michalvavrik, and @phillip-kruger.

Michal, we can simplify it, for example, if I've logged in as bob and now Keycloak has reloaded while I'm logged in as bob then it would be OK if it it were not possible to logout as bob in the already restarted container, or similarly, if the container has restarted while the code flow is in transit, it would not be a main problem - it did not work before as well.

So, lets say, I'm testing bob's tokens and now the container has restarted - then, it would be enough, if I were able to click on DevUI icon on the left, which would show an OIDC card again, and then I'll just start a new login and it works - right now it fails as the old quarkus.oidc.* properties persist, but otherwise, if we can make this part only work, then it would be nice

[michalvavrik]

Thank you, @michalvavrik, and @phillip-kruger.

Michal, we can simplify it, for example, if I've logged in as bob and now Keycloak has reloaded while I'm logged in as bob then it would be OK if it it were not possible to logout as bob in the already restarted container, or similarly, if the container has restarted while the code flow is in transit, it would not be a main problem - it did not work before as well.

So, lets say, I'm testing bob's tokens and now the container has restarted - then, it would be enough, if I were able to click on DevUI icon on the left, which would show an OIDC card again, and then I'll just start a new login and it works - right now it fails as the old quarkus.oidc.* properties persist, but otherwise, if we can make this part only work, then it would be nice

I believe what you describe already work (or TBH it works better than what you describe as it requires no action at all from you), I'll just reflect changes suggested by Phillip and ping you here when the PR is ready for the review.

[sberyozkin]

It did not work for me :-), but yeah, I'll try again when the PR gets updated

[michalvavrik]

It did not work for me :-), but yeah, I'll try again when the PR gets updated

I updated change detection, there could be done more (probably easiest way would be https://lit.dev/docs/templates/directives/#keyed or couple watched properties under one object), but IMO it is resource-friendly enough. If you run into any scenario that is not according to your expectations or bug, just write me steps how to reproduce it and I'll fix it. Thanks

[michalvavrik]

Also something must be done about this section https://quarkus.io/guides/security-openid-connect-dev-services#dev-services-and-ui-support-for-other-openid-connect-providers I didn't update it in this PR intentionally as I'll need your feedback. If you want to keep previous behavior, then we probably have to introduce build item in OIDC depl. that if present, we don't create our own OIDC provider. Also I'll need to check that other extension can deliver Page to OIDC extension card.

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[sberyozkin]

Hi @michalvavrik Works all as expected, thanks for checking everything, things can be tuned further as needed going forward.

Also something must be done about this section https://quarkus.io/guides/security-openid-connect-dev-services#dev-services-and-ui-support-for-other-openid-connect-providers I didn't update it in this PR intentionally as I'll need your feedback. If you want to keep previous behavior, then we probably have to introduce build item in OIDC depl. that if present, we don't create our own OIDC provider.

We already have it,

quarkus/extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/devservices/keycloak/KeycloakDevServicesProcessor.java

Line 144 in 5b1acea

if (oidcProviderBuildItem.isPresent()) {

Also I'll need to check that other extension can deliver Page to OIDC extension card.

We can do it later, Steph can probably confirm it too once he starts testing his alternative OIDC Dev UI extension (Testing OIDC endpoints from UI without having to login to Keycloak)