feat(sda-commons-client-jersey): add oidc startup validation toggle#4491
Conversation
JoergSiebahn
left a comment
JoergSiebahn
left a comment
There was a problem hiding this comment.
Hi @sabkuehn,
thanks for the contribution. It makes sense to have the option to validate the token retrieval on startup. There are some minor things I'm struggling with. Could you have a look at the comments below?
| * Disable retrieving a new token completely. Not meant for production! | ||
| * Example: `false` | ||
|
|
||
| - _OIDC_STARTUP_VALIDATION_DISABLED_ |
There was a problem hiding this comment.
In my opinion it's more intuitive when booleans have a default of false. What do you think about enableStartupValidation? I'm open for other suggestions.
There was a problem hiding this comment.
Sounds good. Changed to OIDC_ENABLE_STARTUP_VALIDATION.
| OidcResult oidcResult; | ||
| try { | ||
| oidcResult = oidcClient.createAccessToken(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
createAccessToken does not declare any exception. So we could catch RuntimeException here.
| } catch (Exception e) { | |
| } catch (RuntimeException e) { |
|
|
||
| private String scope; | ||
|
|
||
| private boolean startupValidationDisabled = true; |
There was a problem hiding this comment.
So above:
| private boolean startupValidationDisabled = true; | |
| private boolean enableStartupValidation; |
|
|
||
| - _OIDC_STARTUP_VALIDATION_DISABLED_ | ||
| * Disables startup validation of the OIDC configuration by skipping an initial access-token retrieval. | ||
| * Set to `false` to fail fast during startup if the OIDC provider is unreachable or misconfigured. |
There was a problem hiding this comment.
Is it worth to mention, that this validation only validates token retrieval but not if the token is suitable to access the resource server?
| * initialized, the token endpoint cannot be reached, or no valid access token can be | ||
| * retrieved | ||
| */ | ||
| public void validateOidcConfig() { |
There was a problem hiding this comment.
Does this method need to be public? I only can see it's called within the class. If it's private we don't need to keep the configuration as a member and we would not have another method that is bound to SemVer rules regarding breaking changes.
| return "OIDC startup validation failed. " | ||
| + detail | ||
| + " Configuration: issuerUrl='" | ||
| + oidcConfiguration.getIssuerUrl() | ||
| + "', grantType='" | ||
| + oidcConfiguration.getGrantType() | ||
| + "'. Check OIDC provider settings."; |
There was a problem hiding this comment.
NIT: This may use String format:
| return "OIDC startup validation failed. " | |
| + detail | |
| + " Configuration: issuerUrl='" | |
| + oidcConfiguration.getIssuerUrl() | |
| + "', grantType='" | |
| + oidcConfiguration.getGrantType() | |
| + "'. Check OIDC provider settings."; | |
| return "OIDC startup validation failed. %s Configuration: issuerUrl='%s', grantType='%s'. Check OIDC provider settings." | |
| .format(detail, oidcConfiguration.getIssuerUrl(), oidcConfiguration.getGrantType()); |
(The suggestion may not pass the code format requirement)
| buildOidcConfigValidationErrorMessage("Token retrieval failed."), e); | ||
| } | ||
|
|
||
| if (oidcResult == null || oidcResult.getState() != OidcState.OK) { |
There was a problem hiding this comment.
Shouldn't this check only for ERROR? The state SKIPPED indicates that OIDC is disabled. That should be a valid case for validation.
| if (oidcResult == null || oidcResult.getState() != OidcState.OK) { | |
| if (oidcResult == null || oidcResult.getState() == OidcState.ERROR) { |
Add OIDC_ENABLE_STARTUP_VALIDATION to control startup-time validation of OIDC token retrieval. The default keeps validation disabled for backward compatibility.
JoergSiebahn
left a comment
JoergSiebahn
left a comment
There was a problem hiding this comment.
Thank you, @sabkuehn
|
🎉 This PR is included in version 9.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
3 participants
Add OIDC_STARTUP_VALIDATION_DISABLED to control startup-time validation of OIDC token retrieval. The default keeps validation disabled for backward compatibility.
Motivation: error message was previously lost in the business request
Before this change, there was no startup validation for the OIDC configuration. When the token endpoint was misconfigured or unreachable, the failure only surfaced during an actual business request and even then, only as a missing Authorization header with a warning log, making the root cause hard to diagnose.
This PR adds startup validation (startupValidationDisabled = false) so that a misconfigured OIDC setup fails fast and loudly at application startup.