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

Support Keycloak Dev Service when OIDC client is used without OIDC extension #43609

Merged

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Sep 30, 2024

  • Closes: Make Keycloak dev service independent of OIDC extension #43086
  • OIDC Client will be started and preconfigured without the OIDC extension being present
  • removing OidcDevServicesBuildItem; the class wasn't final, therefore could never be used as SimpleBuildItem must be final
    throw new IllegalArgumentException("Build item class must be leaf (final) types: " + getClass());
    ; extensions can set quarkus.keycloak.devservices.enabled with SR Config during the build time
  • grant was deprecated on Dev Svc config from 2021 and since I am moving this config, I am also dropping this property
  • OIDC Client tab is only added and contains Keycloak Admin URL:
    image

@michalvavrik michalvavrik changed the title Feature/OIDC dev svc for client Support Keycloak Dev Service when OIDC client is used without OIDC extension Sep 30, 2024
@michalvavrik michalvavrik force-pushed the feature/oidc-dev-svc-for-client branch 2 times, most recently from beb0279 to ac7233d Compare September 30, 2024 21:48
@quarkus-bot

This comment has been minimized.

@michalvavrik
Copy link
Member Author

cc @sberyozkin but let's see how CI goes, I run couple of extension unit tests locally, but not every integration test

@quarkus-bot

This comment has been minimized.

Copy link

github-actions bot commented Sep 30, 2024

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik marked this pull request as draft October 1, 2024 08:27
@michalvavrik
Copy link
Member Author

OidcClientTooManyJwtCredentialKeyPropsTestCase became flaky and I cannot say if it is consequence of this PR, but I'll have a look into it. The integration-tests/oidc-client-wiremock I didn't run yesterday so I'll look into them.

@michalvavrik
Copy link
Member Author

michalvavrik commented Oct 1, 2024

So OidcClientTooManyJwtCredentialKeyPropsTestCase was about missing quarkus.oidc.auth-server-url being thrown sooner than other expected validation exception. And integration-tests/oidc-client-wiremock should not start Dev Service because it is a wiremock. I'll push fixes. Run it 5 times in row and cannot reproduce it anymore, we will see if there is any other race.

@michalvavrik
Copy link
Member Author

I'll wait for #43601 merge before I push fixes because this needs to adapt changes there anyway (I am moving file that is edited there).

@sberyozkin
Copy link
Member

sberyozkin commented Oct 1, 2024

Thanks very much @michalvavrik,
I think having an OIDC client dedicate card is worth it, even if OIDC card is present, it is just another link to the Keycloak provider for now which is not really conflicting and then users will start asking for some more OIDC client specific things to add there, so we will be evolving it....

What happens when both quarkus-oidc and quarkus-oidc-client are loaded ?

@michalvavrik
Copy link
Member Author

What happens when both quarkus-oidc and quarkus-oidc-client are loaded ?

DEV UI wise - card is not produced in the OIDC Client, so you have OIDC card that also contains that link
DEV Svc wise - sadly it is almost everywhere in our tests (only module that I know of that contains OIDC client and not OIDC is the OIDC CLient wiremock) so you can see it -> nothing bad, dev svc is started just right as previously it did

@sberyozkin
Copy link
Member

@michalvavrik

DEV UI wise - card is not produced in the OIDC Client, so you have OIDC card that also contains that link

But we don't have to start hiding it, I think it is shown now on main anyway when OIDC is loaded, with the text only, so this PR only has a KC link added to it.

dev svc is started just right as previously it did

Sure, I got confused for a moment if it would start twice, but no, it will be a single instance only

@michalvavrik
Copy link
Member Author

But we don't have to start hiding it, I think it is shown now on main anyway when OIDC is loaded, with the text only, so this PR only has a KC link added to it.

I am not sure we understand each other, let me try again:

  • OIDC CLlient has own DEV UI card with KC Link
  • OIDC has own DEV UI card with KC link and link to the OIDC DEV UI component

I hide OIDC CLient card when OIDC Is present. If you are saying "let's show it" I think it's alright and I'll do it (please confirm!). I somehow misunderstood your email that said I guess though when no quarkus-oidc is involved, there should be maybe Keycloak card only offering a Keycloak admin link ? It can be done independently though. I suppose I overthink it.

@michalvavrik michalvavrik force-pushed the feature/oidc-dev-svc-for-client branch from 91ffec0 to 085e7e8 Compare October 1, 2024 17:00
@michalvavrik michalvavrik marked this pull request as ready for review October 1, 2024 17:00
@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Oct 1, 2024
@michalvavrik michalvavrik force-pushed the feature/oidc-dev-svc-for-client branch 2 times, most recently from 2069d6a to 2fafdc1 Compare October 1, 2024 20:05
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/oidc-dev-svc-for-client branch from 2fafdc1 to 813565a Compare October 1, 2024 21:24
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

@michalvavrik It looks quite perfect, minor suggestions are there only, I'll run your PR a bit later as well and then we can merge. Later, I'll follow the same pattern for OIDC dynamic client registration as well and eventually we can cover keycloak-admin-clients

@michalvavrik michalvavrik force-pushed the feature/oidc-dev-svc-for-client branch from 813565a to a8f21be Compare October 2, 2024 12:42
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 2, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit a8f21be.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested Michal's PR on my laptop, with quarkus-quickstarts/security-openid-connect-quickstart to confirm a KC DevService is started in dev and test modes, and that OIDC DevUI works as usual.
Additionally, I've confirmed OIDc Dev UI offers Auth0 login experience with quarkus.oidc.auth-server-url pointing to an Auth0 domain

@sberyozkin sberyozkin added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Oct 2, 2024
@sberyozkin sberyozkin merged commit bcc0bc9 into quarkusio:main Oct 2, 2024
50 of 53 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Oct 2, 2024
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Oct 2, 2024
@michalvavrik michalvavrik deleted the feature/oidc-dev-svc-for-client branch October 2, 2024 15:28
@sberyozkin
Copy link
Member

@michalvavrik I thought for a second if it would make sense to keep the base devservice abstract and have extension specific extensions which would deal with their own properties (oidc, oidc-client), etc and have some build time coordination informing each other the the dev service is started.
For example, we also have keycloak-authorization, which is OIDC plus KC authorization policy.
But then I thought we can rework easily if necessary

@michalvavrik
Copy link
Member Author

@michalvavrik I thought for a second if it would make sense to keep the base devservice abstract and have extension specific extensions which would deal with their own properties (oidc, oidc-client), etc and have some build time coordination informing each other the the dev service is started. For example, we also have keycloak-authorization, which is OIDC plus KC authorization policy. But then I thought we can rework easily if necessary

I am all for loose coupling, did it this way because it feels easier to understand than new level of abstraction. FWIW if it will be me who introduce this for other extensions, I'll move these config properties to the extensions and enhance keycloakdevservicerequired build item. I can draw it if you have issue to migrate some service, for example keycloak-authorization.

@sberyozkin
Copy link
Member

@michalvavrik Sure, we can look at it all later, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Keycloak dev service independent of OIDC extension
3 participants