quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.73k stars 2.67k forks source link

DevServices for Keycloak UI is broken on 2.8-main #24329

Closed sberyozkin closed 2 years ago

sberyozkin commented 2 years ago

Describe the bug

Regression has been introduced on main, after starting mvn quarkus:dev and going to http://localhost:8080/q/dev/, the OIDC card does not have a Provider: Keycloak link to http://localhost:8080/q/dev/io.quarkus.quarkus-oidc/provider. http://localhost:8080/q/dev/io.quarkus.quarkus-oidc/provider does respond if it is accessed directly but the UI is wrong there as well, Service Path field is shown alongside the expected Log Into Single Page Application.

It took me a long time to trace it down to #23781. All works as expected with the immediately preceding commit (Stork doc improvements).

The existing smoke test detects http://localhost:8080/q/dev/io.quarkus.quarkus-oidc/provider which is not sufficient, I think one of the OIDC devmode tests will need to be updated to check that http://localhost:8080/q/dev has a Provider: Keycloak string.

Expected behavior

OIDC Card has a Provider: Keycloak and the provider page should only have a Log Into Single Page Application option, as shown here

Actual behavior

No response

How to Reproduce?

Do mvn quarkus:dev in quarkus-quickstarts/security-openid-connect-quickstart and go to http://localhost:8080/q/dev and observe an empty OIDC card without Provider: Keycloak, then go to http://localhost:8080/q/dev/io.quarkus.quarkus-oidc/provider and see a redundant Service Path field

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

quarkus-bot[bot] commented 2 years ago

/cc @pedroigor, @stuartwdouglas

sberyozkin commented 2 years ago

@mkouba @phillip-kruger Hi Martin, Phillip, can you have a look please, this branch in embedded.html does not work, info:oidcProviderName is set here, passed from here.

As far as the redundant Service Path field is concerned, it is defined in a few places in provider.html.The expected Log into Single Page Application is displayed correctly, it is defined here. There might be other issues with the template, but for the moment it is not even possible to test it

mkouba commented 2 years ago

Hm, it does not seem to be related to https://github.com/quarkusio/quarkus/pull/23781. I've followed the steps to reproduce and the relevant DevConsoleTemplateInfoBuildItem is not produced at all because the configProps is an empty optional here: https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/devservices/keycloak/KeycloakDevConsoleProcessor.java#L34. @sberyozkin which extension produces the KeycloakDevServicesConfigBuildItem?

sberyozkin commented 2 years ago

I'm sorry @mkouba, I rebuilt a few times yesterday and I was certain I located the PR which affected it, but in fact it is https://github.com/quarkusio/quarkus/commit/eff45901d57afa9fb12dc3da96bf65c6f5adef8d#diff-67404a43520250b1ebd5a0c945e070e60d70d4a3ba06b15e3fac747f820320cc, CC @ozangunalp, which affected it.

I'll see if I can fix it, sorry again @mkouba

ozangunalp commented 2 years ago

@sberyozkin That's my bad. I must have missed the usages of KeycloakDevServicesConfigBuildItem. I'll prepare a fix and modify the smoke test to check it.

sberyozkin commented 2 years ago

Hi @ozangunalp Np at all, I'm working on it right now, thanks. It was not something you could've detected as the smoke test was not providing enough evidence. I'll ask yourself and Martin to check my fix, due soon enough

ozangunalp commented 2 years ago

Ok! btw I tested this fix here: https://github.com/quarkusio/quarkus/compare/main...ozangunalp:fix_keycloak_dev_ui?expand=1

sberyozkin commented 2 years ago

@ozangunalp Thanks, this is how I started as well, but then realized it won't be enough as the DevServicesResultBuildItem will also be needed (to get some configProps). So instead of duplicating the properties again as I did originally, I thought of passing the extra properties with DevServicesResultBuildItem, should open a PR a bit later.

sberyozkin commented 2 years ago

I'll combine both items instead of using a single one