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

Introduce a config builder for OIDC Client Registration #44429

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Nov 11, 2024

This is but part of phase 2 agreed here #39185 (comment).
We agreed to start with the OIDC Client Registration because its API is experimental, therefore we can make changes on it and it's better to make breaking changes sooner than later.
This PR:

  • introduces builder for OidcClientRegistrationConfig config group that users will use when they register new OIDC Client
  • migrates io.quarkus.oidc.client.registration.OidcClientRegistrations from former config group class to config group interface
  • OidcCommonConfig now implements @ConfigMapping config group interface, which means we can use the interface without converting it to the class when not necessary (and we are not breaking anything); this needs to be done so that OIDC Common utils can accept both former class and new config

When this PR is merged, it will be easier to follow with similar changes for the OIDC and OIDC Client extensions because they share OidcCommonConfig.

Copy link

github-actions bot commented Nov 11, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

This comment has been minimized.

@sberyozkin
Copy link
Member

Hi @michalvavrik Looks good, thanks, how will a more complex sequence look like, where some metadata are supplied as well ?

@michalvavrik
Copy link
Member Author

Hi @michalvavrik Looks good, thanks, how will a more complex sequence look like, where some metadata are supplied as well ?

It's flat structure because metadata has so little properties that I thought it was overkill to create extra .metadata().clientName("name").redirect.....

You could find some examples in tests that I added, but basically this is how it works:

var config = OidcClientRegistrationConfig.builder()
                .extraProperty("metadata-extra-prop-key-1", "metadata-extra-prop-val-1")
                // or
                .extraProps(Map.of("one", "one", "two", "two"))
                .clientName("metadata-client-name")
                .redirectUri("metadata-redirect-uri")
                .postLogoutUri("metadata-post-logout-uri")
                // and now continue with wehatever you like
                .id("client-id-1")
                .build();

@sberyozkin
Copy link
Member

@michalvavrik See, there will be a lot of such sub-interfaces for OidcTenantConfig, so now we have an opportunity to work out now how to make it as easy as possible for the users. I'm not too keen on having everything flat, IMHO, metadata should have its own build support to be distinctive from the rest of the key properties like registration url, and what would be nice is to be able to do either metadata().clientName()... as part of the main builder sequence, or for the main builder sequence to do .metadata(metadata) where metadata was built independently.

I'd also like to suggest to offer shortcuts to let users minimize a number of .builder().
For example, OidcClientRegistrationConfig.registrationUri(someUrl) returns a Builder...

@michalvavrik
Copy link
Member Author

@michalvavrik See, there will be a lot of such sub-interfaces for OidcTenantConfig, so now we have an opportunity to work out now how to make it as easy as possible for the users. I'm not too keen on having everything flat, IMHO, metadata should have its own build support to be distinctive from the rest of the key properties like registration url, and what would be nice is to be able to do either metadata().clientName()... as part of the main builder sequence, or for the main builder sequence to do .metadata(metadata) where metadata was built independently.

I'd also like to suggest to offer shortcuts to let users minimize a number of .builder(). For example, OidcClientRegistrationConfig.registrationUri(someUrl) returns a Builder...

Alright, I'll look into it.

@michalvavrik michalvavrik force-pushed the feature/oidc-client-registration-config-builder branch from b488c31 to 09aa6c3 Compare November 15, 2024 17:45
@michalvavrik
Copy link
Member Author

Hello @sberyozkin , I believe I have addressed your feedback.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/oidc-client-registration-config-builder branch from 09aa6c3 to fea29ab Compare November 19, 2024 14:13
Copy link

quarkus-bot bot commented Nov 19, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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 Nov 19, 2024

Status for workflow Quarkus CI

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

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

@sberyozkin sberyozkin self-requested a review November 19, 2024 15:15
@sberyozkin sberyozkin merged commit d2ba09f into quarkusio:main Nov 19, 2024
26 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Nov 19, 2024
@michalvavrik michalvavrik deleted the feature/oidc-client-registration-config-builder branch November 19, 2024 15:17
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.

2 participants