Skip to content

OIDC: stop modifying deprecated fields and setters on former config classes marked for removal #47457

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

OIDC: stop modifying deprecated fields and setters on former config classes marked for removal #47457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...me/src/main/java/io/quarkus/oidc/common/runtime/config/OidcClientCommonConfigBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ public Credentials credentials() {
}
}

private Optional<String> tokenPath;
protected Optional<String> tokenPath;
private Optional<String> revokePath;
private Optional<String> clientId;
private Optional<String> clientName;
private Credentials credentials;
protected Credentials credentials;

protected OidcClientCommonConfigBuilder(OidcClientCommonConfig oidcClientCommonConfig) {
super(oidcClientCommonConfig);
Expand Down
4 changes: 2 additions & 2 deletions .../runtime/src/main/java/io/quarkus/oidc/common/runtime/config/OidcCommonConfigBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ public Tls tls() {
}
}

private Optional<String> authServerUrl;
private Optional<Boolean> discoveryEnabled;
protected Optional<String> authServerUrl;
protected Optional<Boolean> discoveryEnabled;
private Optional<String> registrationPath;
private Optional<Duration> connectionDelay;
private int connectionRetryCount;
Expand Down
16 changes: 4 additions & 12 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ public OidcTenantConfig() {

}

private OidcTenantConfig(io.quarkus.oidc.runtime.OidcTenantConfig mapping) {
/**
* This constructor is not part of a public API and can change or be removed at any time.
*/
protected OidcTenantConfig(io.quarkus.oidc.runtime.OidcTenantConfig mapping) {
super(mapping);
tenantId = mapping.tenantId();
tenantEnabled = mapping.tenantEnabled();
Expand Down Expand Up @@ -3023,17 +3026,6 @@ public static OidcTenantConfigBuilder builder(io.quarkus.oidc.runtime.OidcTenant
return new OidcTenantConfigBuilder(mapping);
}

/**
* Creates {@link OidcTenantConfig} from the {@code mapping}. This method is more efficient than
* the {@link #builder()} method if you don't need to modify the {@code mapping}.
*
* @param mapping tenant config as returned from the SmallRye Config; must not be null
* @return OidcTenantConfig
*/
public static OidcTenantConfig of(io.quarkus.oidc.runtime.OidcTenantConfig mapping) {
return new OidcTenantConfig(mapping);
}

/**
* Creates {@link OidcTenantConfigBuilder} builder populated with documented default values.
*
Expand Down
119 changes: 117 additions & 2 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfigBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
package io.quarkus.oidc;

import static io.quarkus.oidc.runtime.OidcUtils.createMutableTenantConfig;
import static io.quarkus.oidc.runtime.OidcUtils.needsToSetDefaults;
import static io.quarkus.oidc.runtime.OidcUtils.shouldEnableUserInfo;
import static io.quarkus.oidc.runtime.OidcUtils.shouldSetRefreshExpired;

import java.nio.file.Path;
import java.time.Duration;
import java.util.ArrayList;
Expand Down Expand Up @@ -31,6 +36,7 @@
import io.quarkus.oidc.runtime.builders.AuthenticationConfigBuilder;
import io.quarkus.oidc.runtime.builders.LogoutConfigBuilder;
import io.quarkus.oidc.runtime.builders.TokenConfigBuilder;
import io.quarkus.oidc.runtime.providers.KnownOidcProviders;
import io.smallrye.config.SmallRyeConfigBuilder;

/**
Expand Down Expand Up @@ -666,8 +672,22 @@ public io.quarkus.oidc.OidcTenantConfig build() {
if (tenantId.isEmpty()) {
tenantId(OidcUtils.DEFAULT_TENANT_ID);
}
var mapping = new OidcTenantConfigImpl(this);
return io.quarkus.oidc.OidcTenantConfig.of(mapping);

// keep 'needsToSetDefaults' even though it duplicates nested 'if' conditions so that we share logic that
// determines when to set defaults between this builder and the mutable tenant class
if (needsToSetDefaults(provider, applicationType, token, roles, authentication)) {
if (provider.isPresent()) {
mergeProvider(KnownOidcProviders.provider(provider.get()));
}
if (shouldSetRefreshExpired(applicationType, token)) {
token().refreshExpired().end();
}
if (shouldEnableUserInfo(roles, token, authentication, applicationType)) {
authentication().userInfoRequired().end();
}
}

return createMutableTenantConfig(new OidcTenantConfigImpl(this));
}

/**
Expand Down Expand Up @@ -1239,4 +1259,99 @@ public Token getToken() {
public Logout getLogout() {
return logout;
}

Copy link
Member

@sberyozkin sberyozkin Apr 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems to belong to the utility code, some properties here can not be even supported by this builder but by various sub-builders

Copy link
Member Author

Choose a reason for hiding this comment

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

First of all, I can change it, IMHO this is just matter of opinion where this should be. I don't have problem to change it if you are certain.

It seems to belong to the utility code

Here is why I chose this place:

  • we need one place where to handle defaults so that it is easy to find
  • doing this in some utility will mean that you can reuse it outside of this builder, but it is important to understand how config was build, so that you can verify whether defaults are correct; what this PR does is:
    • config mapping provided by SR Config is updated by this builder, because you cannot modify these interfaces provided by SR config anyway
    • deprecated config classes marked for removal are rebuild by this config; that doesn't matter because we are going to drop them anyway eventually, we will only keep names and turn them into records with deep copy
    • Oidc tenants created using the builder are updated by this

So there is really no other usages for this code outside of this builder. Why would we need them in a utility class?

some properties here can not be even supported by this builder but by various sub-builders

We work with OidcTenantConfig, only thing that users can provide to Quarkus OIDC is that config, they cannot provide Authentication or Roles etc. Yes, they can use individual builders, but once they want to build OidcTenantConfig, this code you are commenting on is applied.

Copy link
Member Author

@michalvavrik michalvavrik Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I hope I am not confusing you, but let me put it this way: with this PR we control all the entry points and if user changes OidcTenantConfig instance after they passed it to Quarkus OIDC code, it has no effect. Which means after we have immutable copy where we control these 2 changeable fields (user info and tenant enabled), we can only rely on methods and state of former config class fields will not be changed (because Quarkus OIDC won't do it and noone else can).

That means we don't care what users do with individual builders, because we have one way how OidcTenantConfig is build.

Copy link
Member

Choose a reason for hiding this comment

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

@michalvavrik

with this PR we control all the entry points and if user changes OidcTenantConfig instance after they passed it to Quarkus OIDC code, it has no effect.

The fact the test provider did it does not mean anyone would actually want to do it in the real application, why would they. And if they do, may be should fail instead, or log a message that the change happened and attempt to adapt... But it can bring so much complexity, without an obvious benefit, that I'm doubting we should do it

/**
* Merge the current tenant (represented by this builder) and well-known OpenId Connect provider configurations.
* Initialized properties take priority over uninitialized properties.
*
* Initialized properties in the current tenant configuration take priority
* over the same initialized properties in the well-known OpenId Connect provider configuration.
*
* @param provider well-known OpenId Connect provider configuration
*/
private void mergeProvider(io.quarkus.oidc.OidcTenantConfig provider) {
// root properties
if (authServerUrl.isEmpty() && provider.authServerUrl().isPresent()) {
authServerUrl(provider.authServerUrl().get());
}
if (applicationType.isEmpty() && provider.applicationType().isPresent()) {
applicationType(provider.applicationType().get());
}
if (discoveryEnabled.isEmpty() && provider.discoveryEnabled().isPresent()) {
discoveryEnabled(provider.discoveryEnabled().get());
}
if (authorizationPath.isEmpty() && provider.authorizationPath().isPresent()) {
authorizationPath(provider.authorizationPath().get());
}
if (jwksPath.isEmpty() && provider.jwksPath().isPresent()) {
jwksPath(provider.jwksPath().get());
}
if (tokenPath.isEmpty() && provider.tokenPath().isPresent()) {
tokenPath(provider.tokenPath().get());
}
if (userInfoPath.isEmpty() && provider.userInfoPath().isPresent()) {
userInfoPath(provider.userInfoPath().get());
}

// authentication
var providerAuth = provider.authentication();
var authBuilder = authentication();
if (authentication.idTokenRequired().isEmpty() && providerAuth.idTokenRequired().isPresent()) {
authBuilder.idTokenRequired(providerAuth.idTokenRequired().get());
}
if (authentication.userInfoRequired().isEmpty() && providerAuth.userInfoRequired().isPresent()) {
authBuilder.userInfoRequired(providerAuth.userInfoRequired().get());
}
if (authentication.pkceRequired().isEmpty() && providerAuth.pkceRequired().isPresent()) {
authBuilder.pkceRequired(providerAuth.pkceRequired().get());
}
if (authentication.scopes().isEmpty() && providerAuth.scopes().isPresent()) {
authBuilder.scopes(providerAuth.scopes().get());
}
if (authentication.scopeSeparator().isEmpty() && providerAuth.scopeSeparator().isPresent()) {
authBuilder.scopeSeparator(providerAuth.scopeSeparator().get());
}
if (authentication.addOpenidScope().isEmpty() && providerAuth.addOpenidScope().isPresent()) {
authBuilder.addOpenidScope(providerAuth.addOpenidScope().get());
}
if (authentication.forceRedirectHttpsScheme().isEmpty() && providerAuth.forceRedirectHttpsScheme().isPresent()) {
authBuilder.forceRedirectHttpsScheme(providerAuth.forceRedirectHttpsScheme().get());
}
if (authentication.responseMode().isEmpty() && providerAuth.responseMode().isPresent()) {
authBuilder.responseMode(providerAuth.responseMode().get());
}
if (authentication.redirectPath().isEmpty() && providerAuth.redirectPath().isPresent()) {
authBuilder.redirectPath(providerAuth.redirectPath().get());
}
authBuilder.end();

// credentials
var credentialsBuilder = credentials();
if (credentials.clientSecret().method().isEmpty()
&& provider.credentials().clientSecret().method().isPresent()) {
credentialsBuilder.clientSecret().method(provider.credentials().clientSecret().method().get()).end();
}
if (credentials.jwt().audience().isEmpty() && provider.credentials().jwt().audience().isPresent()) {
credentialsBuilder.jwt().audience(provider.credentials().jwt().audience().get()).end();
}
if (credentials.jwt().signatureAlgorithm().isEmpty()
&& provider.credentials().jwt().signatureAlgorithm().isPresent()) {
credentialsBuilder.jwt().signatureAlgorithm(provider.credentials().jwt().signatureAlgorithm().get()).end();
}
credentialsBuilder.end();

// token
var tokenBuilder = token();
if (token.issuer().isEmpty() && provider.token().issuer().isPresent()) {
tokenBuilder.issuer(provider.token().issuer().get());
}
if (token.principalClaim().isEmpty() && provider.token().principalClaim().isPresent()) {
tokenBuilder.principalClaim(provider.token().principalClaim().get());
}
if (token.verifyAccessTokenWithUserInfo().isEmpty()
&& provider.token().verifyAccessTokenWithUserInfo().isPresent()) {
tokenBuilder.verifyAccessTokenWithUserInfo(provider.token().verifyAccessTokenWithUserInfo().get());
}
tokenBuilder.end();
}
}
7 changes: 5 additions & 2 deletions ...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/DefaultTenantConfigResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ private Uni<OidcTenantConfig> getDynamicTenantConfig(RoutingContext context) {
//shouldn't happen, but guard against it anyway
oidcConfig = Uni.createFrom().nullItem();
} else {
oidcConfig = oidcConfig.onItem().transform(cfg -> OidcUtils.resolveProviderConfig(cfg));
oidcConfig = oidcConfig.map(MutableOidcTenantConfig::recreateIfNecessary);
}
context.put(CURRENT_DYNAMIC_TENANT_CONFIG, oidcConfig);
}
Expand All @@ -267,7 +267,10 @@ public Uni<? extends TenantConfigContext> apply(OidcTenantConfig tenantConfig) {
if (tenantContext == null) {
return tenantConfigBean.createDynamicTenantContext(tenantConfig);
} else {
return Uni.createFrom().item(tenantContext);
// tenant config resolver can return OidcTenantConfig based on current request context
// and if somewhere in the code we use config from the tenant context, we need to use it
var contextWithUpdatedConfig = new TenantConfigContextImpl(tenantContext, tenantConfig);
return Uni.createFrom().item(contextWithUpdatedConfig);
}
} else {
final String tenantId = context.get(OidcUtils.TENANT_ID_ATTRIBUTE);
Expand Down
6 changes: 6 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/LazyTenantConfigContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.quarkus.oidc.runtime;

import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

import javax.crypto.SecretKey;
Expand Down Expand Up @@ -84,4 +85,9 @@ public SecretKey getInternalIdTokenSecretKey() {
public List<OidcRedirectFilter> getOidcRedirectFilters(Redirect.Location loc) {
return delegate.getOidcRedirectFilters(loc);
}

@Override
public Map<Redirect.Location, List<OidcRedirectFilter>> getLocationToRedirectFilters() {
return delegate.getLocationToRedirectFilters();
}
}
Loading
Loading