fix: on RootProvider always rely on the Error page if unexpected erro…

…r is raised fixes gravitee-io/issues#9000 fixes AM-535 (cherry picked from commit 39355e4)
gravitee-io · Apr 13, 2023 · 82a5d40 · 82a5d40
1 parent f77003e
commit 82a5d40
Show file tree

Hide file tree

Showing 6 changed files with 171 additions and 71 deletions.
diff --git a/...o/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeFailureHandler.java b/...o/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<RoutingContext> {
+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/...ava/io/gravitee/am/gateway/handler/root/resources/handler/error/AbstractErrorHandler.java b/...ava/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<RoutingContext> {
+
+    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/...c/main/java/io/gravitee/am/gateway/handler/root/resources/handler/error/ErrorHandler.java b/...c/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<RoutingContext> {
-
-    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/...java/io/gravitee/am/gateway/handler/root/resources/handler/login/LoginFailureHandler.java b/...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/...vitee/am/gateway/handler/root/resources/handler/user/register/RegisterFailureHandler.java b/...vitee/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<RoutingContext> {
+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/.../io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpointTest.java b/.../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);
+    }
 }