quarkusio / quarkus

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

Keycloak dev services uses wrong token url for client_credentials grant type #35029

Closed domkun closed 1 year ago

domkun commented 1 year ago

Describe the bug

Using the keycloak-provider (http://localhost:8080/q/dev-ui/io.quarkus.quarkus-oidc/keycloak-provider) to test and invoke a service using the client_credentials grant type. The dev services fail to get a token.

The token request is sent to propertiesState.keycloakAdminUrl + "/realms/" + this._selectedRealm + "/protocol/openid-connect/auth" (see qwc-oidc-provider.js _getKeycloakTokenUrl() method.). Keycloak responds with "Missing parameter: response_type" and no token is issued.

My OIDC config is:

quarkus.oidc.client-id=myrealm-client
quarkus.oidc.credentials.secret=lx7ThS5aYggdsMm42BP3wMrVqKm9WpNY
quarkus.keycloak.devservices.realm-path=myrealm-realm.json
quarkus.oidc.devui.grant.type=CLIENT
quarkus.keycloak.devservices.realm-name=myrealm
quarkus.keycloak.devservices.port=9999

I think the correct url should end with /token not /auth

Expected behavior

When clicking on "Test service" button of the keycloak provider in the DevUI, a token should be fetched using the configured client id and secret and the service should be invoked.

Actual behavior

When clicking on "Test service" button of the keycloak provider in the DevUI, an error is shown:

"Failed to test service. Error message: Method [io.quarkus.quarkus-oidc.testServiceWithClientCred] failed: null"

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

19

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.2.Final

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

maven

Additional information

No response

quarkus-bot[bot] commented 1 year ago

/cc @geoand (devservices), @pedroigor (keycloak), @sberyozkin (keycloak), @stuartwdouglas (devservices)

sberyozkin commented 1 year ago

Right, the problem this URL is assumed to be the one required for the sign in:

https://github.com/quarkusio/quarkus/blob/3.2.2.Final/extensions/oidc/deployment/src/main/resources/dev-ui/qwc-oidc-provider.js#L857

So we can fix it by having _getKeycloakAuthUrl instead.

@michalvavrik I can do a quick fix and ask you to review

michalvavrik commented 1 year ago

ok @sberyozkin . I'll look compare it for myself later with old DEV UI, because I thought I didn't change logic anywhere. I guess I was wrong, I'll check. Thanks

sberyozkin commented 1 year ago

@michalvavrik Yeah, it was working OK in the old one: https://github.com/quarkusio/quarkus/blob/3.2.2.Final/extensions/oidc/deployment/src/main/resources/dev-templates/provider.html#L443

But a lot has changed, so it might happen

michalvavrik commented 1 year ago

@sberyozkin I trust you that you are right (and TBH I made no effort to try it in old DEV UI for that reason and for today, I'm very busy), I can see absolutely no difference between new and old DEV UI code.

Old line: https://github.com/quarkusio/quarkus/blob/b5c974d46983e18e4f30cc590a8c20053d51bade/extensions/oidc/deployment/src/main/resources/dev-templates/provider.html#L108

New line: https://github.com/quarkusio/quarkus/blob/b5c974d46983e18e4f30cc590a8c20053d51bade/extensions/oidc/deployment/src/main/resources/dev-ui/qwc-oidc-provider.js#L1228

Yeah, it was working OK in the old one: https://github.com/quarkusio/quarkus/blob/3.2.2.Final/extensions/oidc/deployment/src/main/resources/dev-templates/provider.html#L443

Yeah, but previously you mentioned this line https://github.com/quarkusio/quarkus/blob/3.2.2.Final/extensions/oidc/deployment/src/main/resources/dev-ui/qwc-oidc-provider.js#L857 in the new DEV UI, and it's counterpart in the old dev ui is what I mention above ^^ (line 108 in old dev ui). Maybe you meant that from within https://github.com/quarkusio/quarkus/blob/b5c974d46983e18e4f30cc590a8c20053d51bade/extensions/oidc/deployment/src/main/resources/dev-ui/qwc-oidc-provider.js#L1219 it should end with /token? I agree, made mistake. Thank you for looking at it.

sberyozkin commented 1 year ago

@michalvavrik Np at all, like I said, given the amount of changes you did making a minor typo like that one was possible,

I'm about to open a simple PR...