-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Ensure transaction is rolled back upon error #19488
Conversation
ceba687
to
b6b8b97
Compare
@hmlnarik Thanks for the fix! LGTM as long as tests are OK |
@hmlnarik @mposolda is this included in the latest nightly docker image? I've tried to migrate my test from my previous MR ( #17646 ) just to be sure, but:
The test I try to run: @Test
public void testRegisterShouldFailBeforeUserCreationWhenUserIsInContext() throws Exception {
try (AutoCloseable c = new RealmAttributeUpdater(testRealmResource())
.updateWith(r -> {
Map<String, String> config = new HashMap<>();
config.put("from", "auto@keycloak.org");
config.put("host", "localhost");
config.put("port", "3025");
r.setSmtpServer(config);
r.setRegistrationAllowed(true);
r.setVerifyEmail(true);
r.setResetPasswordAllowed(true);
r.setRegistrationEmailAsUsername(true);
})
.update()) {
UserRepresentation userWhoPreExistsInRealm = new UserRepresentation();
userWhoPreExistsInRealm.setEmail("keycloak-dev@realcity.io");
ApiUtil.createUserAndResetPasswordWithAdminClient(testRealmResource(), userWhoPreExistsInRealm, "password");
testRealmAccountPage.navigateTo();
loginPage.clickRegister();
registerPage.clickBackToLogin();
loginPage.assertCurrent(testRealmResource().toRepresentation().getRealm());
loginPage.resetPassword();
resetPage.assertCurrent();
resetPage.changePassword("keycloak-dev@realcity.io");
driver.navigate().back();
driver.navigate().back();
driver.navigate().back();
registerPage.assertCurrent();
registerPage.registerWithEmailAsUsername(
"Vilmos",
"Szabó-Nagy",
"vilmos.nagy@realcity.io",
"TestPassword123",
"TestPassword123"
);
errorPage.assertCurrent();
assertEquals("The error page is shown", "Invalid username or password.", errorPage.getError()); // it does not fail here, the error page is shown
// the user entity should not be created if the registration flow is not executed till the end
final UserRepresentation userByUsername = ApiUtil.findUserByUsername(testRealmResource(), "vilmos.nagy@realcity.io");
assertNull(userByUsername); // it fails here
}
} The git commit which I run the test: And I'm still able to reproduce my error on the latest nightly docker image, this one: https://quay.io/repository/keycloak/keycloak/manifest/sha256:1fa2e183aa62f223724a827d07a5023141ae6eb6d4d1319f39d6fa5228aea206 |
I've created the #19556 PR to demonstrate the failing test case. |
registration Adding a failing test from keycloak#17644. The keycloak#19488 PR seems not to solve it
registration Adding a failing test from keycloak#17644. The keycloak#19488 PR seems not to solve it
This change ensures that an exception is thrown from endpoints upon error rather than returning a valid response which would commit an transaction that could have recorded changes to an object. Thrown transaction then sets the code to rollback. To ensure that events are recorded in the case of error, errors are recorded in separate transaction.
This has revealed an issue in the client policy test, which has been fixed by the second commit.
Closes: #17644
Closes: #19487