From d86e062a0e65b5577c148876b9a37fb74b16924e Mon Sep 17 00:00:00 2001 From: Vlasta Ramik Date: Thu, 16 Nov 2023 13:50:56 +0100 Subject: [PATCH] Removal of retry blocks introduced for CRDB Closes #24095 Signed-off-by: vramik Signed-off-by: Alexander Schwartz Co-authored-by: Alexander Schwartz --- .../models/utils/KeycloakModelUtils.java | 74 ------------------- .../AuthenticationProcessor.java | 10 --- .../oidc/endpoints/AuthorizationEndpoint.java | 34 +-------- .../oidc/endpoints/TokenEndpoint.java | 21 ------ .../resources/LoginActionsService.java | 20 ----- .../resources/admin/ComponentResource.java | 73 ++++++++---------- .../testsuite/model/KeycloakModelTest.java | 43 +++-------- 7 files changed, 45 insertions(+), 230 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index 15369079cd..579b8d57f3 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -62,13 +62,11 @@ import java.security.KeyPair; import java.security.PrivateKey; import java.security.PublicKey; import java.security.cert.X509Certificate; -import java.sql.SQLException; import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Optional; -import java.util.Random; import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; @@ -390,78 +388,6 @@ public final class KeycloakModelUtils { return result; } - /** - * Creates a new {@link KeycloakSession} and runs the specified callable in a new transaction. If the transaction fails - * with a SQL retriable error, the method re-executes the specified callable until it either succeeds or the maximum number - * of attempts is reached, leaving some increasing random delay milliseconds between the invocations. It uses the exponential - * backoff + jitter algorithm to compute the delay, which is limited to {@code attemptsCount * retryIntervalMillis}. - * More details https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/ - * - * @param factory a reference to the {@link KeycloakSessionFactory}. - * @param callable a reference to the {@link KeycloakSessionTaskWithResult} that will be executed in a retriable way. - * @param attemptsCount the maximum number of attempts to execute the callable. - * @param retryIntervalMillis the base interval value in millis used to compute the delay. - * @param the type returned by the callable. - * @return the value computed by the callable. - */ - public static V runJobInRetriableTransaction(final KeycloakSessionFactory factory, final KeycloakSessionTaskWithResult callable, - final int attemptsCount, final int retryIntervalMillis) { - int retryCount = 0; - Random rand = new Random(); - while (true) { - try (KeycloakSession session = factory.create()) { - session.getTransactionManager().begin(); - return callable.run(session); - } catch (RuntimeException re) { - if (isExceptionRetriable(re) && ++retryCount < attemptsCount) { - int delay = Math.min(retryIntervalMillis * attemptsCount, (1 << retryCount) * retryIntervalMillis) - + rand.nextInt(retryIntervalMillis); - logger.debugf("Caught retriable exception, retrying request. Retry count = %s, retry delay = %s", retryCount, delay); - try { - Thread.sleep(delay); - } catch (InterruptedException ie) { - ie.addSuppressed(re); - throw new RuntimeException(ie); - } - } else { - if (retryCount == attemptsCount) { - logger.debug("Exhausted all retry attempts for request."); - throw new RuntimeException("retries exceeded", re); - } - throw re; - } - } - } - } - - /** - * Checks if the specified exception is retriable or not. A retriable exception must be an instance of {@code SQLException} - * and must have a 40001 SQL retriable state. This is a standard SQL state as defined in SQL standard, and across the - * implementations its meaning boils down to "deadlock" (applies to Postgres, MSSQL, Oracle, MySQL, and others). - * - * @param exception the exception to be checked. - * @return {@code true} if the exception is retriable; {@code false} otherwise. - */ - public static boolean isExceptionRetriable(final Throwable exception) { - Objects.requireNonNull(exception); - // first find the root cause and check if it is a SQLException - Throwable rootCause = exception; - while (rootCause.getCause() != null && rootCause.getCause() != rootCause) { - rootCause = rootCause.getCause(); - } - // JTA transaction handler might add multiple suppressed exceptions to the root cause, evaluate each of those - for (Throwable suppressed : rootCause.getSuppressed()) { - if (isExceptionRetriable(suppressed)) { - return true; - } - }; - if (rootCause instanceof SQLException) { - // check if the exception state is a recoverable one (40001) - return "40001".equals(((SQLException) rootCause).getSQLState()); - } - return false; - } - /** * Wrap given runnable job into KeycloakTransaction. Set custom timeout for the JTA transaction (in case we're in the environment with JTA enabled) * diff --git a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java index 022e65d3b8..b206a4a1a7 100755 --- a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java +++ b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java @@ -39,7 +39,6 @@ import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionModel; import org.keycloak.models.utils.AuthenticationFlowResolver; import org.keycloak.models.utils.FormMessage; -import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.protocol.LoginProtocol; import org.keycloak.protocol.LoginProtocol.Error; import org.keycloak.protocol.oidc.TokenManager; @@ -47,7 +46,6 @@ import org.keycloak.services.ErrorPage; import org.keycloak.services.ErrorPageException; import org.keycloak.services.ServicesLogger; import org.keycloak.services.managers.AuthenticationManager; -import org.keycloak.services.managers.AuthenticationSessionManager; import org.keycloak.services.managers.BruteForceProtector; import org.keycloak.services.managers.ClientSessionCode; import org.keycloak.services.managers.UserSessionManager; @@ -829,14 +827,6 @@ public class AuthenticationProcessor { if (e.getResponse() != null) return e.getResponse(); return ErrorPage.error(session, authenticationSession, Response.Status.BAD_REQUEST, Messages.INVALID_USER); } - - } else if (KeycloakModelUtils.isExceptionRetriable(failure)) { - // let calling code decide if whole action should be retried. - if (failure instanceof RuntimeException) { - throw (RuntimeException) failure; - } else { - throw new RuntimeException(failure); - } } else { ServicesLogger.LOGGER.failedAuthentication(failure); event.error(Errors.INVALID_USER_CREDENTIALS); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java index ea46dbf661..977516905e 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java @@ -21,7 +21,6 @@ import org.jboss.logging.Logger; import org.keycloak.OAuth2Constants; import org.keycloak.authentication.AuthenticationProcessor; import org.keycloak.common.Profile; -import org.keycloak.common.util.ResponseSessionTask; import org.keycloak.constants.AdapterConstants; import org.keycloak.events.Details; import org.keycloak.events.Errors; @@ -32,7 +31,6 @@ import org.keycloak.models.AuthenticationFlowModel; import org.keycloak.models.ClientModel; import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; -import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.protocol.AuthorizationEndpointBase; import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper; import org.keycloak.protocol.oidc.OIDCLoginProtocol; @@ -102,22 +100,17 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase { event.event(EventType.LOGIN); } - private AuthorizationEndpoint(final KeycloakSession session, final EventBuilder event, final Action action) { - this(session, event); - this.action = action; - } - @POST @Consumes(MediaType.APPLICATION_FORM_URLENCODED) public Response buildPost() { logger.trace("Processing @POST request"); - return processInRetriableTransaction(httpRequest.getDecodedFormParameters()); + return process(httpRequest.getDecodedFormParameters()); } @GET public Response buildGet() { logger.trace("Processing @GET request"); - return processInRetriableTransaction(session.getContext().getUri().getQueryParameters()); + return process(session.getContext().getUri().getQueryParameters()); } /** @@ -131,28 +124,7 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase { return new DeviceEndpoint(session, event); } - /** - * Process the request in a retriable transaction. - */ - private Response processInRetriableTransaction(final MultivaluedMap formParameters) { - if (Profile.isFeatureEnabled(Profile.Feature.MAP_STORAGE)) { - return KeycloakModelUtils.runJobInRetriableTransaction(session.getKeycloakSessionFactory(), new ResponseSessionTask(session) { - @Override - public Response runInternal(KeycloakSession session) { - session.getContext().getHttpResponse().setWriteCookiesOnTransactionComplete(); - // create another instance of the endpoint to isolate each run. - AuthorizationEndpoint other = new AuthorizationEndpoint(session, - new EventBuilder(session.getContext().getRealm(), session, clientConnection), action); - // process the request in the created instance. - return other.process(formParameters); - } - }, 10, 100); - } else { - return process(formParameters); - } - } - - private Response process(MultivaluedMap params) { + private Response process(final MultivaluedMap params) { String clientId = AuthorizationEndpointRequestParserProcessor.getClientId(event, session, params); checkSsl(); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java index c1d717da24..e867a054e8 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java @@ -31,7 +31,6 @@ import org.keycloak.common.Profile; import org.keycloak.common.VerificationException; import org.keycloak.common.constants.ServiceAccountConstants; import org.keycloak.common.util.KeycloakUriBuilder; -import org.keycloak.common.util.ResponseSessionTask; import org.keycloak.constants.AdapterConstants; import org.keycloak.events.Details; import org.keycloak.events.Errors; @@ -49,7 +48,6 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionModel; import org.keycloak.models.utils.AuthenticationFlowResolver; -import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.TokenExchangeContext; @@ -178,25 +176,6 @@ public class TokenEndpoint { @Consumes(MediaType.APPLICATION_FORM_URLENCODED) @POST public Response processGrantRequest() { - if (Profile.isFeatureEnabled(Profile.Feature.MAP_STORAGE)) { - // grant request needs to be run in a retriable transaction as concurrent execution of this action can lead to - // exceptions on DBs with SERIALIZABLE isolation level. - return KeycloakModelUtils.runJobInRetriableTransaction(session.getKeycloakSessionFactory(), new ResponseSessionTask(session) { - @Override - public Response runInternal(KeycloakSession session) { - // create another instance of the endpoint to isolate each run. - TokenEndpoint other = new TokenEndpoint(session, tokenManager, - new EventBuilder(session.getContext().getRealm(), session, clientConnection)); - // process the request in the created instance. - return other.processGrantRequestInternal(); - } - }, 10, 100); - } else { - return processGrantRequestInternal(); - } - } - - private Response processGrantRequestInternal() { cors = Cors.add(request).auth().allowedMethods("POST").auth().exposedHeaders(Cors.ACCESS_CONTROL_ALLOW_METHODS); MultivaluedMap formParameters = request.getDecodedFormParameters(); diff --git a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java index 8e34da5dc7..a40d09ed2a 100755 --- a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java +++ b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java @@ -17,8 +17,6 @@ package org.keycloak.services.resources; import org.jboss.logging.Logger; -import org.keycloak.common.Profile; -import org.keycloak.common.util.ResponseSessionTask; import org.keycloak.forms.login.LoginFormsProvider; import org.keycloak.forms.login.MessageType; import org.keycloak.forms.login.freemarker.DetachedInfoStateChecker; @@ -311,25 +309,7 @@ public class LoginActionsService { @QueryParam(Constants.EXECUTION) String execution, @QueryParam(Constants.CLIENT_ID) String clientId, @QueryParam(Constants.TAB_ID) String tabId) { - if (Profile.isFeatureEnabled(Profile.Feature.MAP_STORAGE)) { - return KeycloakModelUtils.runJobInRetriableTransaction(session.getKeycloakSessionFactory(), new ResponseSessionTask(session) { - @Override - public Response runInternal(KeycloakSession session) { - // create another instance of the endpoint to isolate each run. - session.getContext().getHttpResponse().setWriteCookiesOnTransactionComplete(); - LoginActionsService other = new LoginActionsService(session, new EventBuilder(session.getContext().getRealm(), session, clientConnection)); - // process the request in the created instance. - return other.authenticateInternal(authSessionId, code, execution, clientId, tabId); - } - }, 10, 100); - } else { - return authenticateInternal(authSessionId, code, execution, clientId, tabId); - } - } - - private Response authenticateInternal(final String authSessionId, final String code, final String execution, - final String clientId, final String tabId) { event.event(EventType.LOGIN); SessionCodeChecks checks = checksForCode(authSessionId, code, execution, clientId, tabId, AUTHENTICATE_PATH); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ComponentResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ComponentResource.java index 95de2151f5..0a5b2e81b6 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/ComponentResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ComponentResource.java @@ -31,7 +31,6 @@ import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; -import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.models.utils.RepresentationToModel; import org.keycloak.models.utils.StripSecretsUtils; @@ -44,7 +43,6 @@ import org.keycloak.representations.idm.ConfigPropertyRepresentation; import org.keycloak.services.ErrorResponse; import org.keycloak.services.resources.KeycloakOpenAPI; import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator; -import org.keycloak.utils.LockObjectsForModification; import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.Consumes; @@ -136,22 +134,19 @@ public class ComponentResource { @Operation() public Response create(ComponentRepresentation rep) { auth.realm().requireManageRealm(); - return KeycloakModelUtils.runJobInRetriableTransaction(session.getKeycloakSessionFactory(), kcSession -> { - RealmModel realmModel = LockObjectsForModification.lockRealmsForModification(kcSession, () -> kcSession.realms().getRealm(realm.getId())); - try { - ComponentModel model = RepresentationToModel.toModel(kcSession, rep); - if (model.getParentId() == null) model.setParentId(realmModel.getId()); + try { + ComponentModel model = RepresentationToModel.toModel(session, rep); + if (model.getParentId() == null) model.setParentId(realm.getId()); - model = realmModel.addComponentModel(model); + model = realm.addComponentModel(model); - adminEvent.operation(OperationType.CREATE).resourcePath(kcSession.getContext().getUri(), model.getId()).representation(StripSecretsUtils.strip(kcSession, rep)).success(); - return Response.created(kcSession.getContext().getUri().getAbsolutePathBuilder().path(model.getId()).build()).build(); - } catch (ComponentValidationException e) { - return localizedErrorResponse(e); - } catch (IllegalArgumentException e) { - throw new BadRequestException(e); - } - }, 10, 100); + adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri(), model.getId()).representation(StripSecretsUtils.strip(session, rep)).success(); + return Response.created(session.getContext().getUri().getAbsolutePathBuilder().path(model.getId()).build()).build(); + } catch (ComponentValidationException e) { + return localizedErrorResponse(e); + } catch (IllegalArgumentException e) { + throw new BadRequestException(e); + } } @GET @@ -177,23 +172,20 @@ public class ComponentResource { @Operation() public Response updateComponent(@PathParam("id") String id, ComponentRepresentation rep) { auth.realm().requireManageRealm(); - return KeycloakModelUtils.runJobInRetriableTransaction(session.getKeycloakSessionFactory(), kcSession -> { - RealmModel realmModel = LockObjectsForModification.lockRealmsForModification(kcSession, () -> kcSession.realms().getRealm(realm.getId())); - try { - ComponentModel model = realmModel.getComponent(id); - if (model == null) { - throw new NotFoundException("Could not find component"); - } - RepresentationToModel.updateComponent(kcSession, rep, model, false); - adminEvent.operation(OperationType.UPDATE).resourcePath(kcSession.getContext().getUri()).representation(StripSecretsUtils.strip(kcSession, rep)).success(); - realmModel.updateComponent(model); - return Response.noContent().build(); - } catch (ComponentValidationException e) { - return localizedErrorResponse(e); - } catch (IllegalArgumentException e) { - throw new BadRequestException(); + try { + ComponentModel model = realm.getComponent(id); + if (model == null) { + throw new NotFoundException("Could not find component"); } - }, 10, 100); + RepresentationToModel.updateComponent(session, rep, model, false); + adminEvent.operation(OperationType.UPDATE).resourcePath(session.getContext().getUri()).representation(StripSecretsUtils.strip(session, rep)).success(); + realm.updateComponent(model); + return Response.noContent().build(); + } catch (ComponentValidationException e) { + return localizedErrorResponse(e); + } catch (IllegalArgumentException e) { + throw new BadRequestException(); + } } @DELETE @Path("{id}") @@ -201,17 +193,12 @@ public class ComponentResource { @Operation() public void removeComponent(@PathParam("id") String id) { auth.realm().requireManageRealm(); - KeycloakModelUtils.runJobInRetriableTransaction(session.getKeycloakSessionFactory(), kcSession -> { - RealmModel realmModel = LockObjectsForModification.lockRealmsForModification(kcSession, () -> kcSession.realms().getRealm(realm.getId())); - - ComponentModel model = realmModel.getComponent(id); - if (model == null) { - throw new NotFoundException("Could not find component"); - } - adminEvent.operation(OperationType.DELETE).resourcePath(kcSession.getContext().getUri()).success(); - realmModel.removeComponent(model); - return null; - }, 10 , 100); + ComponentModel model = realm.getComponent(id); + if (model == null) { + throw new NotFoundException("Could not find component"); + } + adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).success(); + realm.removeComponent(model); } private Response localizedErrorResponse(ComponentValidationException cve) { diff --git a/testsuite/model/src/test/java/org/keycloak/testsuite/model/KeycloakModelTest.java b/testsuite/model/src/test/java/org/keycloak/testsuite/model/KeycloakModelTest.java index 39b8a2e390..a7a338de68 100644 --- a/testsuite/model/src/test/java/org/keycloak/testsuite/model/KeycloakModelTest.java +++ b/testsuite/model/src/test/java/org/keycloak/testsuite/model/KeycloakModelTest.java @@ -116,8 +116,6 @@ import org.keycloak.models.DeploymentStateProviderFactory; * @author hmlnarik */ public abstract class KeycloakModelTest { - public static final String KEYCLOAK_MODELTESTS_RETRY_TRANSACTIONS = "keycloak.modeltests.retry-transactions"; - private static final Logger LOG = Logger.getLogger(KeycloakModelParameters.class); private static final AtomicInteger FACTORY_COUNT = new AtomicInteger(); protected final Logger log = Logger.getLogger(getClass()); @@ -571,37 +569,20 @@ public abstract class KeycloakModelTest { } protected R inComittedTransaction(T parameter, BiFunction what, BiConsumer onCommit, BiConsumer onRollback) { - if (Boolean.parseBoolean(System.getProperty(KEYCLOAK_MODELTESTS_RETRY_TRANSACTIONS, "false"))) { - return KeycloakModelUtils.runJobInRetriableTransaction(getFactory(), session -> { - session.getTransactionManager().enlistAfterCompletion(new AbstractKeycloakTransaction() { - @Override - protected void commitImpl() { - if (onCommit != null) { onCommit.accept(session, parameter); } - } + return KeycloakModelUtils.runJobInTransactionWithResult(getFactory(), session -> { + session.getTransactionManager().enlistAfterCompletion(new AbstractKeycloakTransaction() { + @Override + protected void commitImpl() { + if (onCommit != null) { onCommit.accept(session, parameter); } + } - @Override - protected void rollbackImpl() { - if (onRollback != null) { onRollback.accept(session, parameter); } - } - }); - return what.apply(session, parameter); - }, 5, 100); - } else { - return KeycloakModelUtils.runJobInTransactionWithResult(getFactory(), session -> { - session.getTransactionManager().enlistAfterCompletion(new AbstractKeycloakTransaction() { - @Override - protected void commitImpl() { - if (onCommit != null) { onCommit.accept(session, parameter); } - } - - @Override - protected void rollbackImpl() { - if (onRollback != null) { onRollback.accept(session, parameter); } - } - }); - return what.apply(session, parameter); + @Override + protected void rollbackImpl() { + if (onRollback != null) { onRollback.accept(session, parameter); } + } }); - } + return what.apply(session, parameter); + }); } /**