Looks promising @mabartos

@rmartinc @shawkins @ahus1 Could anyone review it, please? Thanks!

@mabartos have there been inconsistencies for a while?

Looking at the current undertow logic, which seems to be what was used in Wildfly as well, on any uncaught exception the transaction would be rolled back -

That would have rolled back anything that throw any WebApplicationException regardless of whether it contained a Response.

If I understand correctly the current state is that WebApplicationExceptions thrown containing a Response, will not roll back the transaction.

So the changes here attempt to make it more like it used to be - any RollbackWebApplicationException or exception handled by the KeycloakErrorHandler will cause the transaction to be rolled back.

Are there some WebApplicationExceptions thrown where we don't expect a rollback? Or do we need to make sure that all the WebApplicationException we throw use RollbackWebApplicationException as the base class?

most of them already run in a separate TX that does the commit, but for some of them, we needed to wrap them in a separate TX as well

Ideally we should avoid the paradigm of suspending and starting a new transaction. Is it possible that this could be handled as an enlistAfterCompletion transaction in the rollback method?

@shawkins Thanks for the review!

have there been inconsistencies for a while?

Yes, we detected the behavior in the #50850, so there might be more cases where it can provide such a security issue - IMO, that's why we need to make it more robust.

That would have rolled back anything that throw any WebApplicationException regardless of whether it contained a Response. If I understand correctly the current state is that WebApplicationExceptions thrown containing a Response, will not roll back the transaction.

Yes, exactly. Our KeycloakErrorHandler is not called when the WebApplicationException contains a response; otherwise the rollback is set for every error on KC side.

So the changes here attempt to make it more like it used to be - any RollbackWebApplicationException or exception handled by the KeycloakErrorHandler will cause the transaction to be rolled back.

Yes, we attempt to cover all error cases and always set TX rollback.

Are there some WebApplicationExceptions thrown where we don't expect a rollback? Or do we need to make sure that all the WebApplicationException we throw use RollbackWebApplicationException as the base class?

I did an analysis, and there should not be any, where we explicitly expect a commit of the main transaction. There are some places, where we need to store soma data along the way - brute force detector, error events, UMA, session invalidations. It'd be good if any new KC WAE exception would extend the RollbackWebApplicationException. And if there's some case the rollback is not wanted, it can be workarounded by the plain WAE. Maybe we could fill an issue on the RESTEasy side to always execute exception mappers even for the WAE with response.

Ideally we should avoid the paradigm of suspending and starting a new transaction. Is it possible that this could be handled as an enlistAfterCompletion transaction in the rollback method?

Yes, that might be a more suitable approach. I'll look into it. Thanks!

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesMetadataTest

Keycloak CI - Adapter IT Strict Cookies

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestNoKeyNameInKeyInfo

Keycloak CI - Adapter IT Strict Cookies

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostEncTest

Keycloak CI - Adapter IT Strict Cookies

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

@shawkins Should be ready for review. Thanks!

That feels more like a sure thing. Otherwise the only other thought is whether our CloseSessionFilter could handle the detection of this situation - some way to see if the context has been aborted for example.

@shawkins Thanks! I'll create an issue on their side. I thought about the CloseSessionFilter as well, but for now, I just chose to have an approach with more freedom (the Rollback WAE can help you but if you need to do some special stuff...) to not break existing deployments in the community. Maybe we can revisit later if we find some more problems.

@shawkins Some additional context: It seems we're quite tied up, as the behavior does not come from the RESTEasy, but rather from the JAX-RS spec:

"If the response property of the exception does not contain an entity and an exception mapping provider is available for WebApplicationException or the corresponding subclass, an implementation MUST use the provider to create a new Response instance, otherwise the response property is used directly." https://jakarta.ee/specifications/restful-ws/3.0/jakarta-restful-ws-spec-3.0.html#method_exc

So, we cannot break the compliance on the RESTEasy side, and probably not worth to go over the JAX-RS spec itself.