-
Notifications
You must be signed in to change notification settings - Fork 693
Bugfix: failure to start if OIDC provider is unavailable #36639
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
Bugfix: failure to start if OIDC provider is unavailable #36639
Conversation
c3abe01
to
084f548
Compare
The aim of the test is to test unauthorized behaviour So in this case the setup is not really used anywhere in this test.
Adding this dependency as it going to be used in tests.
This class will be used in two places, so I've decided to make it generic: - for `ClientRegistrationRepository` bean creation - for `JwtDecoder` bean creation The aim of this class is to ensure eventual creation of the object. What that means: - we will try to create the object on a startup - in case of failure it will keep retrying the creation in async way until it succeeds Open question: what is the best value for default retry delay?
Tests two scenarios: - initialisation on startup - initialisation after retries
The main purpose of the wrapper is to ensure `ClientRegistrationRepository` is created rather on a startup or later during retries: - on a startup it will attempt to create a repository - if this fails, it'll create `SafeInitProxy` (see previous commits) and pass 2 arguments: a supplier to create a `ClientRegistrationRepository` and en error listener (log entry will be present) - startup will succeed even when IdP is not available - created proxy will keep attempting to create the repository in the background - at the same time, if client makes a call (any), `findByRegistrationId` will attempt to get a value from the proxy (if successful that means the IdP is available). If the value is not present (meaning IdP is still not available) it'll throw an exception.
Same as for the previous wrapper,the main purpose of this is to ensure `JwtDecoder` is created rather on a startup or later during retries: - on a startup it will attempt to create a decoder - if this fails, it'll create `SafeInitProxy` instance (see previous commits) and pass 2 arguments: a supplier to create a `JwtDecoder` and en error listener (log entry will be present) - this will schedule async retries of creation - startup will succeed - created proxy will keep attempting to create the decoder in the background - at the same time, if client makes a call (any), `decode` will attempt to get a value from the proxy (if successful that means the IdP is available). If the value is not present (meaning IdP is still not available) it'll throw an exception that will be returned to the client. Open question: what the error message should be here? JwtDecoder relies on ClientRegistrationRepository so I guess the error here should probably be the same?
…ecoder` Also, add `.authenticationEntryPoint(authFailureHandler)` (that acts as error handler) to: `getTokenValidator` and `oidcWebappSecurity`. This is done so we could customize the error message for the client and specify the details. This would return our standard error response instead of a default Spring error response.
As this is a recoverable failure, log level should be "warn" for the repository. As decoder error is following the repository error, it can be `debug`
This test: - sets up the application to point to a proxy instead of keycloak first - verifies that application is up but client request fails with the expected message - points proxy to keycloak - verifies that client request succeeds please note, setting up keycloak takes time.
825c215
to
c4848fb
Compare
d92310a
to
bd5bc87
Compare
return proxy | ||
.orElseThrow( | ||
() -> | ||
new RuntimeException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in SafeInitJwtDecoder
it would be useful to provide an Exception class that is more semantically appropriate.
I was thinking Spring's BeanInitializationException
as we're building a Bean, but actually we throw here on access, not on initialization.
Perhaps then the Java built-in IllegalStateException
makes most sense.
We could add our own Exception, but that is probably just noise if we're not handling it specifically elsewhere.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with IllegalStateException
exception for SafeInitClientRegistrationRepository
However, for SafeInitJwtDecoder
we have to have AuthenticationCredentialsNotFoundException
in order for Spring to pass it to our error handler (unless I am missing something, but I think I am correct). There are other exceptions that could be used to achieve the same but the choice is limited as they have to extend AuthenticationException
🙂
UPD: I've just verified my assumption - yes, this is how it currently works.
For reference : please see AuthFailureHandler
, AuthenticationEntryPoint
and corresponding AuthenticationException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could potentially introduce our own Exception what would extends AuthenticationException
but it seems like an overkill to me (especially in the timeline that we currently have).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity of what's happening you can also look at BearerTokenAuthenticationFilter
line 160:
catch (AuthenticationException failed) {
this.securityContextHolderStrategy.clearContext();
this.logger.trace("Failed to process authentication request", failed);
this.authenticationFailureHandler.onAuthenticationFailure(request, response, failed);
}
And line 83:
private AuthenticationFailureHandler authenticationFailureHandler = new AuthenticationEntryPointFailureHandler(
(request, response, exception) -> this.authenticationEntryPoint.commence(request, response, exception));
So if we throw something that is not AuthenticationException
, it is not gonna be handled properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the investigation and explanation!
Then I'd propose:
- let's change the
RuntimeException
to anIllegalStateException
for clarity - add a comment on top of
AuthenticationCredentialsNotFoundException
explaining, in short, why the exception type is there as you just explained to me - it will be very useful for the next person not to scratch their head :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the nice implementation and addressing the comments!
…6789) Reverts #36639 This fix caused an issue that led Spring Security to fall back to the default login endpoint due to missing `Iterable` Implementation in the `SafeInitClientRegistrationRepository`. The real solution needs further root cause discussion on how we deal with startup in the multi-module setup. See internal discussion: https://camunda.slack.com/archives/C08MRKHJ0CD/p1755164727531609
Description
The bug:
Expected behaviour:
Solution (please see commit messages for more details):
Two places are affected by this bug:
clientRegistrationRepository
bean creationjwtDecoder
bean creation (as it needs theclientRegistrationRepository
)Lets introduce custom
ClientRegistrationRepository
andJwtDecoder
implementations that:InMemoryClientRegistrationRepository
anddecoder
the same way as it is done now (on application startup). Let's attempt it once. This would ensure that if there are no issues we are ready to authenticate after startup straight away.Integration test with the below scenario was also added:
❓ Open question:
Checklist
Related issues
closes #34189