Add quarkus-oidc-client-registration extension

[sberyozkin]

Fixes #38250.

This PR introduces experimental quarkus-oidc-client-registration extension to support OIDC Dynamic Client Registration. Client registration API provided by this extension may change after feedback from users, before it settles down.

OIDC Dynamic Client Registration is an important feature allowing to scale up registration and management of OIDC clients in combination with dynamic OIDC tenancy resolution.

I'm moving it out of the draft state because this PR is no longer trying to bring 2 new extensions (quarkus-oidc-client-registration, quarkus-oidc-client-registration-configuration-source) and focuses on the core OIDC client registration only with further enhancements to be added in due course.

There are about 3 updates in this PR which impact a large number of files, but only a single one is really about the OIDC client registration. The other two ones just try to make it easier to move ahead with the OIDC client registration work:

• quarkus.oidc.runtime.AbstractJsonObject got moved from quarkus-oidc down to quarkus-oidc-common, to avoid adding the same methods for supporting retrieving JSON for OIDC client registration ClientMetadata (and longer term, to support OIDC client enhancement request from Stian for OIDC client to support UserInfo, etc) - and this move causes a few import updates
• OIDC client registration is about talking to a discoverable OIDC client registration endpoint, and a lot of current OidcCommonConfig, which both quarkus-oidc OidcTenantConfig and quarkus-oidc-client OidcClientConfig extend, applies to these communications. But a good portion of OidcCommonConfig is only relevant to the work of quarkus-oidc OidcTenantConfig and quarkus-oidc-client OidcClientConfig. Therefore this PR moves such properties out of OidcCommonConfig to OidcClientCommonConfig leading to the following hierarchies: OidcTenantConfig -> OidcClientCommonConfig -> OidcCommonConfig, OidcClientConfig -> OidcClientCommonConfig -> OidcCommonConfig, but only OidcClientRegistrationConfig -> OidcCommonConfig. This leads to a good number of import updates as well. It is not strictly necessary, but then users who create client registrations would see configuration properties not relevant to the client registration process and that would not be great.

Finally, we have the actual quarkus-oidc-client-registration extension. I've tried to provide a summary of registration patterns it supports in this PR's doc update, please have a look. But here is a brief summary:

• Each OidcClientRegistration represents an OIDC client registration endpoint - it can be created from OidcClientRegistrationConfig set either in application.properties or dynamically.
• OidcClientRegistrationConfig may specify OIDC address only, but also can have metadata configured for a single client, if it does, then OidcClientRegistration will return RegisteredClient representing an already registered client at start-up
• Each OidcClientRegistration, in addition to returning client registered at startup, may accept requests to register one or more clients, using Uni and Multi response types respectively
• OidcClientRegistration can be used without having to use quarkus-oidc, but only to facilitate the dynamic client registration
• OIDC dynamic TenantConfigResolver is suited best for using dynamically registered clients and creating OIDC tenant configurations
• OidcClientRegistrations (as opposed to OidcClientRegistration) provides access to all OidcClientRegistrations created from configured OIDC client registration configs
• OidcClientRegistrations can be used to accept a new OidcClientRegistration which in turn can be used to register one or more clients
• RegisteredClient represents a registered client and supports interacting with this client-specific registration endpoint to read, update the metadata or delete this client
• ClientMetadata is a JSON wrapper which can be used to register new clients

OIDC dynamic client registration is a rather complex area and it is hard to capture all of its subtleties perfectly from the start. Therefore I mark it clearly as an experimental extension, in its metadata and the docs, and I propose to let users stress it after the review dealing with some obvious PR issues. It will become an important feature but it may need a few iterations to settle down.

I'd like to have at least 2 reviews and approvals. Thanks

[gastaldi]

Looks good overall, added some nice-to-have suggestions

[sberyozkin]

Hey @gastaldi Thanks for finding the time yesterday :-), I switched off my laptop shortly after I opened the PR. I'll handle some of the comments, but as far as ConfigMapping is concerned, OidcCommonConfig group would need to be reworked as part of #39185 eventually, we may need to do a breaking change there since this group can be managed directly by users in dynamic TenantConfigResolver... It is a tricky one. I'm not sure right now, that #39185 can be a prerequisite for this issue.

[sberyozkin]

@michalvavrik, what are your thoughts about an order in which ConfigMapping to OIDC and OIDc client extensions, and possibly to the OIDC client registration one should be added ?

This PR introduces OidcTenantConfig -> OidcCommonClientConfig -> OidcCommonConfig hierarchy by moving properties not relevant for OIDC client registration to OidcCommonClientConfig, with OidcClientRegistrationConfig extending OidcCommonConfig directly.

I don't mind much waiting for #39185 before finalizing this PR but completing #39815 may take a while and it would be more difficult afterwards to move some of the builder API support for OidcCommonConfig.

If that works for you, then, once everyone is happy with this PR, two of us can look at introducing the builder API for oidc, oidc-client and oidc-client-registration, and for the last one we will just replace OidcClientRegistrationConfig group with the mapping without even deprecating the group, since it is an experimental extension

[sberyozkin]

Hey @pedroigor @gastaldi, I've temporarily moved it to Draft to look at tests till the end of the week, as I have to sign off.

So, the main remaining issue, as far as I see, is what can be done with the already registered clients after a restart, to avoid reregistering them again.
This PR supports several modes of registration, independently of OIDC, on demand, all is good here.
But there is one mode where the new client metadata are pre-configured in application.properties, and until the latest update, it would be registered on every restart.

The first thing I've tried is to check what Keycloak does if the new client metadata contain the client name and the client with the same name has already been registered. Right now, indeed, Keycloak, would just keep creating new clients, they would all have unique ids but the same name. I've opened keycloak/keycloak#32133 with some proposals how to avoid such duplications. However, I'm not 100% sure the team will agree, and the other thing is that as far as the OIDC dynamic reg spec is concerned, it is all totally undefined as to what happens in such cases, so even if we optimize it for Keycloak, the duplication may be still happening for other providers.

So, the only thing I could think of is to let users postpone such pre configured client registration until the runtime, so that they can get a chance to restore the already registered client's metadata. To do so, the users would have to persist RegisteredClient registration URI and token, and then, at runtime, request OidcClientRegistration to read the existing client instead using this URI and token - it should avoid duplications as well.

It is getting quite complex though so my proposal would be to give users a chance to start experimenting with all the options once I get the PR green. Will ping a bit later

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

@gastaldi @pedroigor

I've fixed the tests and added new content to the docs, showing a simple example of how an already registered client can read its own metadata, ex, when it needs to refresh it and I'll try update with Keycloak 25.0.3 or later.

More importantly, I've added one more doc section suggesting a strategy for avoiding the duplicate client registrations at startup in cases when client metadata are preconfigured:

• Delay the registration until it is necessary with a register-early=false property
• On shutdown: persist in DB, cache, the client's registration URI and token
• On startup: check if the registration URI and token for a given client are available, if yes, read the existing registration, otherwise initiate the delayed new client registration

Other good ideas from Pedro, 1) support named OidcClientRegistration injections 2) Have a closer integration with quarkus-oidc, can definitely be looked at, I'm just not sure we should do it now, given the overall complexity, I'd rather get users experiment with it for a while first. The 2nd item will also require a transition from ConfigRoot to ConfigMapping

Thanks

[sberyozkin]

Hey @pedroigor @gastaldi

With Keycloak 25.0.4, I was able to update the test to show the individual registered client's update also works, see the code starting from here.

So we check there first that the registered client's name is Default Client, and then we update the name to Default Client Updated which is expected in the test here.

That should get enough space for users to start experimenting. The extension is clearly marked as experimental even in the docs, so we'll have an easy way to tune the API for a while. Once this one is merged, I'll open follow up enhancement requests: named OidcClientRegistration injection, and closer integration with quarkus-oidc, which was in the original PR draft but like I said I thought it looked a bit raw and I decided not to try to do too much immediately - it feels to me the dynamic OIDC client registration is really an async admin process, unrelated to the work of quarkus-oidc itself, though we can try to do something nice for some simple quarkus-oidc scenarios later.

[gastaldi]

LGTM

[quarkus-bot]

Status for workflow Quarkus Documentation CI

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

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

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 36d84b4.

✅ 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

⚙️ Maven Tests - JDK 17 Windows

📦 integration-tests/maven

✖ io.quarkus.maven.it.TestMojoIT.testThatTheTestsAreReRunMultiModule - History

• Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes.
	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.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils.waitForNextCompletion(TestModeContinuousTestingMavenTestUtils.java:50)
	at io.quarkus.maven.it.LaunchMojoTestBase.testThatTheTestsAreReRunMultiModule(LaunchMojoTestBase.java:56)

✖ io.quarkus.maven.it.TestMojoIT.testThatTheTestsAreReRunMultiModule - History

• Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes.
	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.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils.waitForNextCompletion(TestModeContinuousTestingMavenTestUtils.java:50)
	at io.quarkus.maven.it.LaunchMojoTestBase.testThatTheTestsAreReRunMultiModule(LaunchMojoTestBase.java:56)

[sberyozkin]

Hi @gastaldi @pedroigor

I'm getting ready to merge it, as Pedro may not have time. I'll definitely though follow up with enhancement requests based on Pedro's feedback.

In the very short term I'd like users to try it and I think I may need to tweak ClientMetadata a bit to make it easier to avoid having to deal with JSON and let users add array properties to the builder, etc. It is too complex now to keep adding new things to it at the moment IMHO :-)

[michalvavrik]

FYI I have experienced failures in my PR here #42935 that are caused by this. When I reverted this PR locally, it's fixed. No need to reaction, I just wanted to link it here. I'll comment more in #42935.

[sberyozkin]

Hi @michalvavrik That was fixed in response to #43106

[michalvavrik]

Hi @michalvavrik That was fixed in response to #43106

it was fixed here #42947, nevertheless, it works. thanks for the fix