Repository: vert-x3/vertx-web
Issue: OAuth handler should not redirect the callback route

when the oauth handle is set to protect everything e.g.: `router.route().handler(...` it always covers the `callback` handler and this will enter an infinite loop of redirects to the oauth provider and back. This needs to be whitelisted.

Changed files:
vertx-web/src/test/java/io/vertx/ext/web/handler/OAuth2AuthHandlerTest.java, vertx-web/src/main/java/io/vertx/ext/web/handler/impl/OAuth2AuthHandlerImpl.java

diff --git a/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/OAuth2AuthHandlerImpl.java b/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/OAuth2AuthHandlerImpl.java
index 414f08356..fb18a2b86 100644
--- a/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/OAuth2AuthHandlerImpl.java
+++ b/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/OAuth2AuthHandlerImpl.java
@@ -23,6 +23,8 @@ import io.vertx.core.http.HttpHeaders;
 import io.vertx.core.http.HttpMethod;
 import io.vertx.core.json.JsonArray;
 import io.vertx.core.json.JsonObject;
+import io.vertx.core.logging.Logger;
+import io.vertx.core.logging.LoggerFactory;
 import io.vertx.ext.auth.AuthProvider;
 import io.vertx.ext.auth.oauth2.OAuth2Auth;
 import io.vertx.ext.web.Route;
@@ -43,6 +45,8 @@ import static io.vertx.ext.auth.oauth2.OAuth2FlowType.AUTH_CODE;
  */
 public class OAuth2AuthHandlerImpl extends AuthorizationAuthHandler implements OAuth2AuthHandler {
 
+  private static final Logger log = LoggerFactory.getLogger(OAuth2AuthHandlerImpl.class);
+
   /**
    * This is a verification step, it can abort the instantiation by
    * throwing a RuntimeException
@@ -116,8 +120,22 @@ public class OAuth2AuthHandlerImpl extends AuthorizationAuthHandler implements O
           handler.handle(Future.failedFuture("callback route is not configured."));
           return;
         }
-        // the redirect is processed as a failure to abort the chain
-        handler.handle(Future.failedFuture(new HttpStatusException(302, authURI(context.request().uri()))));
+        // when this handle is mounted as a catch all, the callback route must be configured before,
+        // as it would shade the callback route. When a request matches the callback path and has the
+        // method GET the exceptional case should not redirect to the oauth2 server as it would become
+        // an infinite redirect loop. In this case an exception must be raised.
+        if (
+          context.request().method() == HttpMethod.GET &&
+            context.normalisedPath().equals(callback.getPath())) {
+
+          if (log.isWarnEnabled()) {
+            log.warn("The callback route is shaded by the OAuth2AuthHandler, ensure the callback route is added BEFORE the OAuth2AuthHandler route!");
+          }
+          handler.handle(Future.failedFuture(new HttpStatusException(500, "Infinite redirect loop [oauth2 callback]")));
+        } else {
+          // the redirect is processed as a failure to abort the chain
+          handler.handle(Future.failedFuture(new HttpStatusException(302, authURI(context.request().uri()))));
+        }
       } else {
         // attempt to decode the token and handle it as a user
         ((OAuth2Auth) authProvider).decodeToken(token, decodeToken -> {
diff --git a/vertx-web/src/test/java/io/vertx/ext/web/handler/OAuth2AuthHandlerTest.java b/vertx-web/src/test/java/io/vertx/ext/web/handler/OAuth2AuthHandlerTest.java
index 68ae3631b..71e2256ef 100644
--- a/vertx-web/src/test/java/io/vertx/ext/web/handler/OAuth2AuthHandlerTest.java
+++ b/vertx-web/src/test/java/io/vertx/ext/web/handler/OAuth2AuthHandlerTest.java
@@ -109,6 +109,82 @@ public class OAuth2AuthHandlerTest extends WebTestBase {
     server.close();
   }
 
+  @Test
+  public void testAuthCodeFlowBadSetup() throws Exception {
+
+    // lets mock a oauth2 server using code auth code flow
+    OAuth2Auth oauth2 = OAuth2Auth.create(vertx, OAuth2FlowType.AUTH_CODE, new OAuth2ClientOptions()
+      .setClientID("client-id")
+      .setClientSecret("client-secret")
+      .setSite("http://localhost:10000"));
+
+    final CountDownLatch latch = new CountDownLatch(1);
+
+    HttpServer server = vertx.createHttpServer().requestHandler(req -> {
+      if (req.method() == HttpMethod.POST && "/oauth/token".equals(req.path())) {
+        req.setExpectMultipart(true).bodyHandler(buffer -> req.response().putHeader("Content-Type", "application/json").end(fixture.encode()));
+      } else if (req.method() == HttpMethod.POST && "/oauth/revoke".equals(req.path())) {
+        req.setExpectMultipart(true).bodyHandler(buffer -> req.response().end());
+      } else {
+        req.response().setStatusCode(400).end();
+      }
+    }).listen(10000, ready -> {
+      if (ready.failed()) {
+        throw new RuntimeException(ready.cause());
+      }
+      // ready
+      latch.countDown();
+    });
+
+    latch.await();
+
+    // protect everything. This has the bad sideffect that it will also shade the callback route which is computed
+    // after this handler, the proper way to fix this would be create the route before
+    router.route().handler(OAuth2AuthHandler.create(oauth2, "http://localhost:8080/callback").setupCallback(router.route()));
+    // mount some handler under the protected zone
+    router.route("/protected/somepage").handler(rc -> {
+      assertNotNull(rc.user());
+      rc.response().end("Welcome to the protected resource!");
+    });
+
+
+    testRequest(HttpMethod.GET, "/protected/somepage", null, resp -> {
+      // in this case we should get a redirect
+      redirectURL = resp.getHeader("Location");
+      assertNotNull(redirectURL);
+    }, 302, "Found", null);
+
+    // fake the redirect
+    testRequest(HttpMethod.GET, "/callback?state=/protected/somepage&code=1", null, resp -> {
+    }, 500, "Internal Server Error", "Internal Server Error");
+
+    // second attempt with proper config
+    router.clear();
+
+    // protect everything.
+    OAuth2AuthHandler oauth2Handler = OAuth2AuthHandler.create(oauth2, "http://localhost:8080/callback").setupCallback(router.route());
+    // now the callback is registered before as it should
+    router.route().handler(oauth2Handler);
+    // mount some handler under the protected zone
+    router.route("/protected/somepage").handler(rc -> {
+      assertNotNull(rc.user());
+      rc.response().end("Welcome to the protected resource!");
+    });
+
+
+    testRequest(HttpMethod.GET, "/protected/somepage", null, resp -> {
+      // in this case we should get a redirect
+      redirectURL = resp.getHeader("Location");
+      assertNotNull(redirectURL);
+    }, 302, "Found", null);
+
+    // fake the redirect
+    testRequest(HttpMethod.GET, "/callback?state=/protected/somepage&code=1", null, resp -> {
+    }, 200, "OK", "Welcome to the protected resource!");
+
+    server.close();
+  }
+
   @Test
   public void testPasswordFlow() throws Exception {