From 82a5d40c1feebf4e8db92213da01bdfa69f00755 Mon Sep 17 00:00:00 2001 From: eric Date: Fri, 7 Apr 2023 15:22:13 +0200 Subject: [PATCH] fix: on RootProvider always rely on the Error page if unexpected error is raised fixes gravitee-io/issues#9000 fixes AM-535 (cherry picked from commit 39355e4635f43aa765f75c17239cfe439bfb18f7) --- .../mfa/MFAChallengeFailureHandler.java | 22 ++--- .../handler/error/AbstractErrorHandler.java | 91 +++++++++++++++++++ .../resources/handler/error/ErrorHandler.java | 30 +----- .../handler/login/LoginFailureHandler.java | 8 +- .../user/register/RegisterFailureHandler.java | 43 ++++----- .../mfa/MFAChallengeEndpointTest.java | 48 +++++++++- 6 files changed, 171 insertions(+), 71 deletions(-) create mode 100644 gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/AbstractErrorHandler.java diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeFailureHandler.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeFailureHandler.java index 08050b9e89d..03041bb6778 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeFailureHandler.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeFailureHandler.java @@ -21,6 +21,8 @@ import io.gravitee.am.common.web.UriBuilder; import io.gravitee.am.gateway.handler.common.vertx.utils.RequestUtils; import io.gravitee.am.gateway.handler.common.vertx.utils.UriBuilderRequest; +import io.gravitee.am.gateway.handler.root.RootProvider; +import io.gravitee.am.gateway.handler.root.resources.handler.error.AbstractErrorHandler; import io.gravitee.am.service.AuthenticationFlowContextService; import io.vertx.core.Handler; import io.vertx.reactivex.core.MultiMap; @@ -36,22 +38,21 @@ * @author GraviteeSource Team */ -public class MFAChallengeFailureHandler implements Handler { +public class MFAChallengeFailureHandler extends AbstractErrorHandler { private static final Logger LOGGER = LoggerFactory.getLogger(MFAChallengeFailureHandler.class); private static final String ERROR_CODE_VALUE = "send_challenge_failed"; private final AuthenticationFlowContextService authenticationFlowContextService; public MFAChallengeFailureHandler(AuthenticationFlowContextService authenticationFlowContextService) { + super(RootProvider.PATH_ERROR); this.authenticationFlowContextService = authenticationFlowContextService; } @Override - public void handle(RoutingContext routingContext) { - if (routingContext.failed()) { - Throwable throwable = routingContext.failure(); - handleException(routingContext, throwable == null ? "MFA Challenge failed for unexpected reason" : throwable.getMessage()); - } + public void doHandle(RoutingContext routingContext) { + Throwable throwable = routingContext.failure(); + handleException(routingContext, throwable == null ? "MFA Challenge failed for unexpected reason" : throwable.getMessage()); } private void handleException(RoutingContext context, String errorDescription) { @@ -70,7 +71,7 @@ private MultiMap updateQueryParams(RoutingContext context, String errorDescripti if (context.request().getParam(Parameters.LOGIN_HINT) != null) { // encode login_hint parameter (to not replace '+' sign by a space ' ') - queryParams.set(Parameters.LOGIN_HINT, UriBuilder.encodeURIComponent(context.request().getParam(ConstantKeys.USERNAME_PARAM_KEY))); + queryParams.set(Parameters.LOGIN_HINT, UriBuilder.encodeURIComponent(context.request().getParam(Parameters.LOGIN_HINT))); } return queryParams; @@ -87,11 +88,4 @@ private void logoutUser(RoutingContext context) { context.session().destroy(); } } - - private void doRedirect(HttpServerResponse response, String url) { - response - .putHeader(HttpHeaders.LOCATION, url) - .setStatusCode(302) - .end(); - } } diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/AbstractErrorHandler.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/AbstractErrorHandler.java new file mode 100644 index 00000000000..d6fdb87e510 --- /dev/null +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/AbstractErrorHandler.java @@ -0,0 +1,91 @@ +/** + * Copyright (C) 2015 The Gravitee team (http://gravitee.io) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.gravitee.am.gateway.handler.root.resources.handler.error; + +import io.gravitee.am.common.exception.oauth2.OAuth2Exception; +import io.gravitee.am.common.oauth2.Parameters; +import io.gravitee.am.common.utils.ConstantKeys; +import io.gravitee.am.gateway.handler.common.vertx.utils.RequestUtils; +import io.gravitee.am.gateway.handler.common.vertx.utils.UriBuilderRequest; +import io.gravitee.am.gateway.policy.PolicyChainException; +import io.gravitee.am.model.oidc.Client; +import io.gravitee.am.service.exception.AbstractManagementException; +import io.gravitee.common.http.HttpHeaders; +import io.vertx.core.Handler; +import io.vertx.ext.auth.webauthn.impl.attestation.AttestationException; +import io.vertx.ext.web.handler.HttpException; +import io.vertx.reactivex.core.MultiMap; +import io.vertx.reactivex.core.http.HttpServerRequest; +import io.vertx.reactivex.core.http.HttpServerResponse; +import io.vertx.reactivex.ext.web.RoutingContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static io.gravitee.am.common.utils.ConstantKeys.CLIENT_CONTEXT_KEY; +import static io.gravitee.am.common.utils.ConstantKeys.TOKEN_CONTEXT_KEY; +import static io.gravitee.am.gateway.handler.common.vertx.utils.UriBuilderRequest.CONTEXT_PATH; +import static java.util.Objects.isNull; + +/** + * @author Titouan COMPIEGNE (titouan.compiegne at graviteesource.com) + * @author GraviteeSource Team + */ +public abstract class AbstractErrorHandler implements Handler { + + protected final Logger logger = LoggerFactory.getLogger(this.getClass()); + protected final String errorPage; + + public AbstractErrorHandler(String errorPage) { + this.errorPage = errorPage; + } + + @Override + public final void handle(RoutingContext routingContext) { + if (routingContext.failed()) { + try { + doHandle(routingContext); + } catch (Exception e) { + logger.error("Unable to handle error response", e); + String errorPageURL = routingContext.get(CONTEXT_PATH) + errorPage; + + final HttpServerRequest request = routingContext.request(); + // prepare query parameters + MultiMap parameters = RequestUtils.getCleanedQueryParams(routingContext.request()); + // get client if exists + Client client = routingContext.get(CLIENT_CONTEXT_KEY); + if (client != null) { + parameters.set(Parameters.CLIENT_ID, client.getClientId()); + } else if (request.getParam(Parameters.CLIENT_ID) != null) { + parameters.set(Parameters.CLIENT_ID, (request.getParam(Parameters.CLIENT_ID))); + } + + // append error information + parameters.set(ConstantKeys.ERROR_PARAM_KEY, "server_error"); + parameters.set(ConstantKeys.ERROR_DESCRIPTION_PARAM_KEY, "Unexpected error occurred"); + + // redirect + String proxiedErrorPage = UriBuilderRequest.resolveProxyRequest(request, errorPageURL, parameters, true); + doRedirect(routingContext.response(), proxiedErrorPage); + } + } + } + + protected abstract void doHandle(RoutingContext routingContext); + + protected void doRedirect(HttpServerResponse response, String url) { + response.putHeader(HttpHeaders.LOCATION, url).setStatusCode(302).end(); + } +} diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/ErrorHandler.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/ErrorHandler.java index 077c566215d..0b18e54f472 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/ErrorHandler.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/ErrorHandler.java @@ -23,17 +23,11 @@ import io.gravitee.am.gateway.policy.PolicyChainException; import io.gravitee.am.model.oidc.Client; import io.gravitee.am.service.exception.AbstractManagementException; -import io.gravitee.common.http.HttpHeaders; -import io.gravitee.common.http.HttpStatusCode; -import io.vertx.core.Handler; import io.vertx.ext.auth.webauthn.impl.attestation.AttestationException; import io.vertx.ext.web.handler.HttpException; import io.vertx.reactivex.core.MultiMap; import io.vertx.reactivex.core.http.HttpServerRequest; -import io.vertx.reactivex.core.http.HttpServerResponse; import io.vertx.reactivex.ext.web.RoutingContext; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import static io.gravitee.am.common.utils.ConstantKeys.CLIENT_CONTEXT_KEY; import static io.gravitee.am.common.utils.ConstantKeys.TOKEN_CONTEXT_KEY; @@ -44,17 +38,14 @@ * @author Titouan COMPIEGNE (titouan.compiegne at graviteesource.com) * @author GraviteeSource Team */ -public class ErrorHandler implements Handler { - - private static final Logger logger = LoggerFactory.getLogger(ErrorHandler.class); - private final String errorPage; +public class ErrorHandler extends AbstractErrorHandler { public ErrorHandler(String errorPage) { - this.errorPage = errorPage; + super(errorPage); } @Override - public void handle(RoutingContext routingContext) { + protected void doHandle(RoutingContext routingContext) { if (routingContext.failed()) { Throwable throwable = routingContext.failure(); // management exception (resource not found, server error, ...) @@ -75,17 +66,7 @@ public void handle(RoutingContext routingContext) { handleException(routingContext, "technical_error", "Invalid WebAuthn attestation, make sure your device is compliant with the platform requirements"); } else { logger.error("An exception occurs while handling incoming request", throwable); - if (routingContext.statusCode() != -1) { - routingContext - .response() - .setStatusCode(routingContext.statusCode()) - .end(); - } else { - routingContext - .response() - .setStatusCode(HttpStatusCode.INTERNAL_SERVER_ERROR_500) - .end(); - } + handleException(routingContext, "unexpected_error", "An unexpected error occurs"); } } } @@ -125,7 +106,4 @@ private void handleException(RoutingContext routingContext, String errorCode, St } } - private void doRedirect(HttpServerResponse response, String url) { - response.putHeader(HttpHeaders.LOCATION, url).setStatusCode(302).end(); - } } diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/login/LoginFailureHandler.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/login/LoginFailureHandler.java index 848cc8e28f1..d17490b7615 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/login/LoginFailureHandler.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/login/LoginFailureHandler.java @@ -38,14 +38,12 @@ import org.slf4j.LoggerFactory; import java.net.URI; -import java.util.Collection; -import java.util.LinkedHashMap; -import java.util.Map; -import java.util.Optional; +import java.util.*; import static io.gravitee.am.common.utils.ConstantKeys.PARAM_CONTEXT_KEY; import static io.gravitee.am.service.utils.ResponseTypeUtils.isHybridFlow; import static io.gravitee.am.service.utils.ResponseTypeUtils.isImplicitFlow; +import static java.util.Objects.nonNull; /** * @author Titouan COMPIEGNE (titouan.compiegne at graviteesource.com) @@ -168,7 +166,7 @@ private void handleException(RoutingContext context, String errorCode, String er if (errorDescription != null) { queryParams.set(ConstantKeys.ERROR_DESCRIPTION_PARAM_KEY, errorDescription); } - if (context.request().getParam(Parameters.LOGIN_HINT) != null) { + if (nonNull(context.request().getParam(Parameters.LOGIN_HINT)) && nonNull(context.request().getParam(ConstantKeys.USERNAME_PARAM_KEY))) { // encode login_hint parameter (to not replace '+' sign by a space ' ') queryParams.set(Parameters.LOGIN_HINT, UriBuilder.encodeURIComponent(context.request().getParam(ConstantKeys.USERNAME_PARAM_KEY))); } diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/user/register/RegisterFailureHandler.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/user/register/RegisterFailureHandler.java index 0d661b17b13..992a6f2b860 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/user/register/RegisterFailureHandler.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/main/java/io/gravitee/am/gateway/handler/root/resources/handler/user/register/RegisterFailureHandler.java @@ -18,6 +18,8 @@ import io.gravitee.am.common.utils.ConstantKeys; import io.gravitee.am.gateway.handler.common.vertx.utils.RequestUtils; import io.gravitee.am.gateway.handler.common.vertx.utils.UriBuilderRequest; +import io.gravitee.am.gateway.handler.root.RootProvider; +import io.gravitee.am.gateway.handler.root.resources.handler.error.AbstractErrorHandler; import io.gravitee.am.service.exception.EmailFormatInvalidException; import io.gravitee.am.service.exception.InvalidUserException; import io.gravitee.common.http.HttpHeaders; @@ -34,27 +36,27 @@ * @author Titouan COMPIEGNE (titouan.compiegne at graviteesource.com) * @author GraviteeSource Team */ -public class RegisterFailureHandler implements Handler { +public class RegisterFailureHandler extends AbstractErrorHandler { - private static final Logger logger = LoggerFactory.getLogger(RegisterFailureHandler.class); + public RegisterFailureHandler() { + super(RootProvider.PATH_ERROR); + } @Override - public void handle(RoutingContext context) { - if (context.failed()) { - // prepare response - final MultiMap queryParams = RequestUtils.getCleanedQueryParams(context.request()); - // if failure, return to the register page with an error - Throwable cause = context.failure(); - if (cause instanceof InvalidUserException) { - queryParams.set(ConstantKeys.WARNING_PARAM_KEY, "invalid_user_information"); - } else if (cause instanceof EmailFormatInvalidException) { - queryParams.set(ConstantKeys.WARNING_PARAM_KEY, "invalid_email"); - } else { - logger.error("An error occurs while ending user registration", cause); - queryParams.set(ConstantKeys.ERROR_PARAM_KEY, "registration_failed"); - } - redirectToPage(context, queryParams, cause); + protected void doHandle(RoutingContext context) { + // prepare response + final MultiMap queryParams = RequestUtils.getCleanedQueryParams(context.request()); + // if failure, return to the register page with an error + Throwable cause = context.failure(); + if (cause instanceof InvalidUserException) { + queryParams.set(ConstantKeys.WARNING_PARAM_KEY, "invalid_user_information"); + } else if (cause instanceof EmailFormatInvalidException) { + queryParams.set(ConstantKeys.WARNING_PARAM_KEY, "invalid_email"); + } else { + logger.error("An error occurs while ending user registration", cause); + queryParams.set(ConstantKeys.ERROR_PARAM_KEY, "registration_failed"); } + redirectToPage(context, queryParams, cause); } private void redirectToPage(RoutingContext context, MultiMap queryParams, Throwable... exceptions) { @@ -72,11 +74,4 @@ private void redirectToPage(RoutingContext context, MultiMap queryParams, Throwa .end(); } } - - private void doRedirect(HttpServerResponse response, String url) { - response - .putHeader(HttpHeaders.LOCATION, url) - .setStatusCode(302) - .end(); - } } diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/test/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpointTest.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/test/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpointTest.java index 83376b6c225..745c65deec0 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/test/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpointTest.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-core/src/test/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpointTest.java @@ -29,6 +29,7 @@ import io.gravitee.am.model.factor.EnrolledFactor; import io.gravitee.am.model.factor.EnrolledFactorSecurity; import io.gravitee.am.model.oidc.Client; +import io.gravitee.am.service.AuthenticationFlowContextService; import io.gravitee.am.service.CredentialService; import io.gravitee.am.service.DeviceService; import io.gravitee.am.service.FactorService; @@ -98,6 +99,9 @@ public class MFAChallengeEndpointTest extends RxWebTestBase { @Mock private EmailService emailService; + @Mock + private AuthenticationFlowContextService authenticationFlowContextService; + private LocalSessionStore localSessionStore; @Override @@ -108,8 +112,7 @@ public void setUp() throws Exception { localSessionStore = LocalSessionStore.create(vertx); router.route("/mfa/challenge") .handler(SessionHandler.create(localSessionStore)) - .handler(BodyHandler.create()) - .failureHandler(rc -> rc.response().setStatusCode(500).end()); + .handler(BodyHandler.create()); } @Test @@ -164,4 +167,45 @@ public void shouldVerify_fido2Factor() throws Exception { }, HttpStatusCode.FOUND_302, "Found", null); } + + @Test + public void shouldRejectVerification() throws Exception { + when(factorManager.getFactor("factorId")).thenThrow(new NullPointerException()); + + router.route(HttpMethod.POST, "/mfa/challenge") + .handler(ctx -> { + User user = new User(); + user.setId("userId"); + EnrolledFactor enrolledFactor = new EnrolledFactor(); + EnrolledFactorSecurity enrolledFactorSecurity = new EnrolledFactorSecurity(); + enrolledFactorSecurity.setType(FactorSecurityType.WEBAUTHN_CREDENTIAL); + enrolledFactor.setFactorId("factorId"); + enrolledFactor.setSecurity(enrolledFactorSecurity); + user.setFactors(Collections.singletonList(enrolledFactor)); + Client client = new Client(); + client.setFactors(Collections.singleton("factorId")); + ctx.setUser(io.vertx.reactivex.ext.auth.User.newInstance(new io.gravitee.am.gateway.handler.common.vertx.web.auth.user.User(user))); + ctx.put(ConstantKeys.CLIENT_CONTEXT_KEY, client); + ctx.next(); + }) + .handler(new MFAChallengeEndpoint(factorManager, userService, engine, deviceService, applicationContext, domain, credentialService, factorService, rateLimiterService, verifyAttemptService, emailService)) + .failureHandler(new MFAChallengeFailureHandler(authenticationFlowContextService)); + + testRequest( + HttpMethod.POST, + "/mfa/challenge", + req -> { + Buffer buffer = Buffer.buffer(); + buffer.appendString("code={\"id\":\"credentialId\"}&factorId=factorId"); + req.headers().set("content-length", String.valueOf(buffer.length())); + req.headers().set("content-type", "application/x-www-form-urlencoded"); + req.write(buffer); + }, + resp -> { + String location = resp.headers().get("location"); + assertNotNull(location); + assertTrue(location.contains("/error")); + }, + HttpStatusCode.FOUND_302, "Found", null); + } }