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

OIDC dev services and ui changes #35324

Merged
merged 5 commits into from
Jan 14, 2025
Merged

OIDC dev services and ui changes #35324

merged 5 commits into from
Jan 14, 2025

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Aug 11, 2023

Fixes #31340.
Fixes #34577.
Fixes #35064.

  • Added an option for "lightweight" keycloak Dev Service, currently behind a config option. IMO this should be the default and users should require a full-blown keycloak dev service explicitly.
    • This supports the same alice/bob users by default with same roles
    • Extra users/roles can be configured with quarkus.keycloak.devservices.roles.john=reader,writer config (no need for the users.john key because no password is required
    • All existing users are presented in the login page with an easy 1-click and tooltip showing their roles
    • Custom form for specifying other user name/roles
    • This currently supports ID tokens, Access tokens but not refresh tokens, not sure if we need that
  • Fixes UTF-8 JWT token support in Dev UI
  • Display nice JWT token tooltips in Dev UI
  • Support web-app in OIDC Dev UI, including session cookies

I'll make this a Draft PR because docs are still needed, and also this requires review and aprouval, especially if this replaces the default Keycloak Dev UI.

Screenshot from 2023-08-11 16-40-23
Screenshot from 2023-08-11 16-41-44
Screenshot from 2023-08-11 16-41-18
Screenshot from 2023-08-11 16-40-38

@FroMage
Copy link
Member Author

FroMage commented Aug 11, 2023

Thanks to @phillip-kruger for his design in the new lightweight dev service. You can see at https://github.com/quarkusio/quarkus/pull/35324/files#diff-348da42b232118c4809edbac7a9ae8228b336bcee5e80cf3c8fe81f36d886a16R1223 what I was trying to do to decorate the output of JSON.stringify. Turns out I had to reimplement stringify partly :(

@sberyozkin
Copy link
Member

Hi Steph @FroMage, and @phillip-kruger, this is just super great, and it really opens it up for all OIDC users, not only those working with Keycloak. I'm just back from PTO, so I'll be commenting during the next few days.

IMO this should be the default and users should require a full-blown keycloak dev service explicitly.

Let it settle a bit and we can switch, but what I've also been thinking about, since it is a lightweight version, and hence not Keycloak specific, I'm having a doubt that it should be enabled/disabled with quarkus.keycloak.devservices.lightweight=true/false. Would it make sense to enable the lightweight version instead by adding an extension like quarkus-devservices-oidc - if it is included - it replaces the current one which loads Keycloak ?

@sberyozkin
Copy link
Member

sberyozkin commented Aug 15, 2023

This currently supports ID tokens, Access tokens but not refresh tokens, not sure if we need that

No, we don't need it, RTs are not supposed to be visible outside of the confidential OIDC clients - with Quarkus endpoints playing the role of such clients, since with SPA (public clients) having RT available is too much risk - in the vast majority of cases it would be binary in any case.

By the way, JWT is currently decoded with the base64 function, which is wrong, it should be base64url decoded, it nearly always works, but I noticed once, with Auth0 JWT tokens, it was failing to decode the tokens. I recall I could not find the ready function to use bas64url at the time, some custom JavaScript modules were needed, but may be now it is an easy replacement, have a look please, may be @phillip-kruger knows the function name

@sberyozkin
Copy link
Member

CC-ing @stuartwdouglas as well, as Stuart had been reviewing my original PR and we had a few iterations discussing how to package the (KC) devservices

@sberyozkin
Copy link
Member

sberyozkin commented Aug 16, 2023

@FroMage

I'm having a doubt that it should be enabled/disabled with quarkus.keycloak.devservices.lightweight=true/false. Would it make sense to enable the lightweight version instead by adding an extension like quarkus-devservices-oidc - if it is included - it replaces the current one which loads Keycloak

Or may be we can just have a new property in

It is a minor thing, ](https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/DevUiConfig.java), that would make keep it in the OIDC namespace, say quarkus.oidc.dev-ui.lightweight=true

@FroMage
Copy link
Member Author

FroMage commented Aug 21, 2023

Let it settle a bit and we can switch, but what I've also been thinking about, since it is a lightweight version, and hence not Keycloak specific, I'm having a doubt that it should be enabled/disabled with quarkus.keycloak.devservices.lightweight=true/false. Would it make sense to enable the lightweight version instead by adding an extension like quarkus-devservices-oidc - if it is included - it replaces the current one which loads Keycloak ?

I was thinking slightly differently, that since the extension is named quarkus-oidc it should come with the lightweight dev service, and the one from keycloak could come from quarkus-keycloak but aside from the dev service, I'm not sure what that extension would hold?

@sberyozkin
Copy link
Member

sberyozkin commented Aug 29, 2023

@FroMage It would be a breaking change for Keycloak users and for them having to include a new dep would be a problem. The current devservice is used a lot by KC users.
Lets use your idea of switching the dev service experience with the property, I'd only propose to make it .keycloak. namespace neutral, please see my prev comment, quarkus.oidc.dev-ui.lightweight=true instead of quarkus.keycloak.devservices.lightweight=true, in https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/DevUiConfig.java

@sberyozkin
Copy link
Member

@FroMage By the way, the current OIDC Dev UI can already be used to interact with OIDC providers configured with quarkus.oidc.auth-server-url in devmode , Keycloak, or Auth0, etc.

What happens if the lightweight option is enabled and quarkus.oidc.auth-server-url is configured ?

@sberyozkin
Copy link
Member

Steph, lets continue the discussion

@FroMage
Copy link
Member Author

FroMage commented Oct 17, 2023

I don't have much time ATM, but sure, let's finish this. What question did you have?

@FroMage
Copy link
Member Author

FroMage commented Jan 19, 2024

@sberyozkin OK, I rebased it. Shall we resume the discussion and get it merged? Can you remember what you wanted me to change? Thanks.

This comment has been minimized.

@FroMage
Copy link
Member Author

FroMage commented Jan 26, 2024

Ping @sberyozkin

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

@sberyozkin any chance we can get this moving?

@FroMage
Copy link
Member Author

FroMage commented Mar 21, 2024

I've rebased this.

Does this need any tests written? Documentation somewhere?

@sberyozkin
Copy link
Member

@FroMage Sorry for the delay, I miss many GitHub pings, ping me please at Zulip if I don't respond for weeks/months :-).

Please address the comment at #35324 (comment).

Related to it is the namespace for the lightweight mode, it feels quarkus.oidc is the right one since the lightweight option does not depend on Keycloak at all, see #35324 (comment)

Look forward to us merging this PR soon

@michalvavrik
Copy link
Member

This has been more than a year now and IIUC @FroMage activities around Panache 2, you won't have time this month? I'd like to make sure this gets into 3.18 because it's fantastic feature.

How about we divide this PR into smaller pieces? I am mostly interested in this piece:

  • Added an option for "lightweight" keycloak Dev Service, currently behind a config option. IMO this should be the default and users should require a full-blown keycloak dev service explicitly.

And I think you need to:

  • re Does this need any tests written? Documentation somewhere? yes, tests and docs is needed
  • we migrated dev svc to different module and now it also starts for other extensions, it will take some coordination and adaptation when to start which service
  • address Sergeys comment regarding config namespace

Overall, it should be easy to finish this part as you have done all the work, problem is that I can't help because I can't push into your PR. WDYT is there a chance you will have time for this or you see a chance for me to help?

@FroMage
Copy link
Member Author

FroMage commented Dec 6, 2024

This has been more than a year now and IIUC @FroMage activities around Panache 2, you won't have time this month?

I certainly won't :(

I'd like to make sure this gets into 3.18 because it's fantastic feature.

How about we divide this PR into smaller pieces? I am mostly interested in this piece:

  • Added an option for "lightweight" keycloak Dev Service, currently behind a config option. IMO this should be the default and users should require a full-blown keycloak dev service explicitly.

That'd be great.

And I think you need to:

  • re Does this need any tests written? Documentation somewhere? yes, tests and docs is needed

Well, sure, but I won't have time

  • we migrated dev svc to different module and now it also starts for other extensions, it will take some coordination and adaptation when to start which service

I have no idea what this even means :(

  • address Sergeys comment regarding config namespace

I don't care about which namespace this uses, the bigger question was whether this should be on by default or not.

Overall, it should be easy to finish this part as you have done all the work, problem is that I can't help because I can't push into your PR. WDYT is there a chance you will have time for this or you see a chance for me to help?

I've added you as collaborator.

@michalvavrik
Copy link
Member

I have no idea what this even means :(

Sorry, there is a mechanism we use to determine when Keycloak Dev Service should be started because we need to coordinate when OIDC extension, OIDC Client, OIDC Client Registration, Keycloak Admin client need this dev service and when should we not start it. It's not important to explain here. I'll take care of it.

I don't care about which namespace this uses, the bigger question was whether this should be on by default or not.

I am +1 for turning this by default, but I think we will end up doing it later based on Sergey's feedback. I think it would be nice to get a feedback from users that tried it.

I've added you as collaborator.

Thank you, I'll be working on it during next week.

@michalvavrik
Copy link
Member

I just resolved merge conflicts with the current main and pushed to see if I can. Please ignore that push for now, thanks.

@michalvavrik
Copy link
Member

hey @sberyozkin, once again ready for a review :-)

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.

@michalvavrik @FroMage, thank you for this awesome feature

@sberyozkin sberyozkin added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed area/panache area/core area/arc Issue related to ARC (dependency injection) area/security area/resteasy-classic area/smallrye area/vertx area/testing area/reactive-messaging area/mongodb area/platform Issues related to definition and interaction with Quarkus Platform area/kubernetes area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/graphql area/rest area/mailer area/dependencies Pull requests that update a dependency file labels Jan 14, 2025
Copy link

quarkus-bot bot commented Jan 14, 2025

Status for workflow Quarkus Documentation CI

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

✅ 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

quarkus-bot bot commented Jan 14, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 0461ec3.

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

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/opentelemetry-quickstart

io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled - History

  • Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled(OpenTelemetryDisabledTest.java:29)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

@gastaldi gastaldi merged commit 46c3051 into quarkusio:main Jan 14, 2025
55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Jan 14, 2025
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jan 14, 2025
@michalvavrik michalvavrik deleted the oidc-devui branch January 14, 2025 20:45
@maxandersen
Copy link
Member

related to #35324

@michalvavrik
Copy link
Member

michalvavrik commented Jan 15, 2025

related to #35324

hey @maxandersen , a PR is always related to itself. I think that was a typo. I am curious what you meant that was related to this PR. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review area/documentation area/oidc kind/enhancement New feature or request release/noteworthy-feature triage/flaky-test
Projects
None yet
6 participants