Merge pull request #10811 from sberyozkin/refresh_token_instead_of_be…

…arer Check OIDC token type and return 401 for invalid bearer tokens
quarkusio · Jul 20, 2020 · dd3b5e4 · dd3b5e4
2 parents 5af22eb + 75f43b9
commit dd3b5e4
Show file tree

Hide file tree

Showing 10 changed files with 94 additions and 17 deletions.
diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
@@ -768,6 +768,12 @@ public static Token fromAudience(String... audience) {
         @ConfigItem
         public Optional<List<String>> audience = Optional.empty();
 
+        /**
+         * Expected token type
+         */
+        @ConfigItem
+        public Optional<String> tokenType = Optional.empty();
+
         /**
          * Life span grace period in seconds.
          * When checking token expiry, current time is allowed to be later than token expiration time by at most the configured
@@ -849,6 +855,14 @@ public Duration getForcedJwkRefreshInterval() {
         public void setForcedJwkRefreshInterval(Duration forcedJwkRefreshInterval) {
             this.forcedJwkRefreshInterval = forcedJwkRefreshInterval;
         }
+
+        public Optional<String> getTokenType() {
+            return tokenType;
+        }
+
+        public void setTokenType(String tokenType) {
+            this.tokenType = Optional.of(tokenType);
+        }
     }
 
     @ConfigGroup

diff --git a/...ons/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/BearerAuthenticationMechanism.java b/...ons/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/BearerAuthenticationMechanism.java
@@ -15,8 +15,6 @@ public class BearerAuthenticationMechanism extends AbstractOidcAuthenticationMec
     private static final String BEARER = "Bearer";
     protected static final ChallengeData UNAUTHORIZED_CHALLENGE = new ChallengeData(HttpResponseStatus.UNAUTHORIZED.code(),
             null, null);
-    protected static final ChallengeData FORBIDDEN_CHALLENGE = new ChallengeData(HttpResponseStatus.FORBIDDEN.code(), null,
-            null);
 
     public Uni<SecurityIdentity> authenticate(RoutingContext context,
             IdentityProviderManager identityProviderManager,
@@ -31,13 +29,7 @@ public Uni<SecurityIdentity> authenticate(RoutingContext context,
     }
 
     public Uni<ChallengeData> getChallenge(RoutingContext context, DefaultTenantConfigResolver resolver) {
-        String bearerToken = extractBearerToken(context);
-
-        if (bearerToken == null) {
-            return Uni.createFrom().item(UNAUTHORIZED_CHALLENGE);
-        }
-
-        return Uni.createFrom().item(FORBIDDEN_CHALLENGE);
+        return Uni.createFrom().item(UNAUTHORIZED_CHALLENGE);
     }
 
     private String extractBearerToken(RoutingContext context) {

diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcIdentityProvider.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcIdentityProvider.java
@@ -114,6 +114,7 @@ public void handle(AsyncResult<AccessToken> event) {
                                     userInfo = getUserInfo(event.result(), (String) vertxContext.get("access_token"));
                                 }
                                 if (tokenJson != null) {
+                                    OidcUtils.validatePrimaryJwtTokenType(resolvedContext.oidcConfig.token, tokenJson);
                                     JsonObject rolesJson = getRolesJson(vertxContext, resolvedContext, tokenCred, tokenJson,
                                             userInfo);
                                     try {

diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcUtils.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcUtils.java
@@ -191,4 +191,16 @@ public static void setSecurityIdentityUserInfo(QuarkusSecurityIdentity.Builder b
             builder.addAttribute("userinfo", new UserInfo(userInfo.encode()));
         }
     }
+
+    public static void validatePrimaryJwtTokenType(OidcTenantConfig.Token tokenConfig, JsonObject tokenJson) {
+        if (tokenJson.containsKey("typ")) {
+            String type = tokenJson.getString("typ");
+            if (tokenConfig.getTokenType().isPresent() && !tokenConfig.getTokenType().get().equals(type)) {
+                throw new OIDCException("Invalid token type");
+            } else if ("Refresh".equals(type)) {
+                // At least check it is not a refresh token issued by Keycloak
+                throw new OIDCException("Refresh token can only be used with the refresh token grant");
+            }
+        }
+    }
 }
diff --git a/extensions/oidc/runtime/src/test/java/io/quarkus/oidc/runtime/OidcUtilsTest.java b/extensions/oidc/runtime/src/test/java/io/quarkus/oidc/runtime/OidcUtilsTest.java
@@ -26,6 +26,41 @@
 
 public class OidcUtilsTest {
 
+    @Test
+    public void testCorrectTokenType() throws Exception {
+        OidcTenantConfig.Token tokenClaims = new OidcTenantConfig.Token();
+        tokenClaims.setTokenType("access_token");
+        JsonObject json = new JsonObject();
+        json.put("typ", "access_token");
+        OidcUtils.validatePrimaryJwtTokenType(tokenClaims, json);
+    }
+
+    @Test
+    public void testWrongTokenType() throws Exception {
+        OidcTenantConfig.Token tokenClaims = new OidcTenantConfig.Token();
+        tokenClaims.setTokenType("access_token");
+        JsonObject json = new JsonObject();
+        json.put("typ", "refresh_token");
+        try {
+            OidcUtils.validatePrimaryJwtTokenType(tokenClaims, json);
+            fail("Exception expected: wrong token type");
+        } catch (OIDCException ex) {
+            // expected
+        }
+    }
+
+    @Test
+    public void testKeycloakRefreshTokenType() throws Exception {
+        JsonObject json = new JsonObject();
+        json.put("typ", "Refresh");
+        try {
+            OidcUtils.validatePrimaryJwtTokenType(new OidcTenantConfig.Token(), json);
+            fail("Exception expected: wrong token type");
+        } catch (OIDCException ex) {
+            // expected
+        }
+    }
+
     @Test
     public void testTokenWithCorrectIssuer() throws Exception {
         OidcTenantConfig.Token tokenClaims = OidcTenantConfig.Token.fromIssuer("https://server.example.com");

diff --git a/...tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java b/...tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java
@@ -103,11 +103,11 @@ public void testResolveTenantIdentifier() {
                 .statusCode(200)
                 .body(equalTo("tenant-b:alice"));
 
-        // should give a 403 given that access token from issuer b can not access tenant c
+        // should give a 401 given that access token from issuer b can not access tenant c
         RestAssured.given().auth().oauth2(getAccessToken("alice", "b"))
                 .when().get("/tenant/tenant-c/api/user")
                 .then()
-                .statusCode(403);
+                .statusCode(401);
     }
 
     @Test
@@ -118,11 +118,11 @@ public void testResolveTenantConfig() {
                 .statusCode(200)
                 .body(equalTo("tenant-d:alice"));
 
-        // should give a 403 given that access token from issuer b can not access tenant c
+        // should give a 401 given that access token from issuer b can not access tenant c
         RestAssured.given().auth().oauth2(getAccessToken("alice", "b"))
                 .when().get("/tenant/tenant-d/api/user")
                 .then()
-                .statusCode(403);
+                .statusCode(401);
     }
 
     @Test
@@ -164,11 +164,11 @@ public Boolean call() throws Exception {
                 .body(equalTo("tenant-oidc:alice"));
 
         // Get a token with kid '3' - it can only be verified via the introspection fallback since OIDC returns JWK set with kid '2'
-        // 403 since the introspection is not enabled
+        // 401 since the introspection is not enabled
         RestAssured.given().auth().oauth2(getAccessTokenFromSimpleOidc("3"))
                 .when().get("/tenant/tenant-oidc/api/user")
                 .then()
-                .statusCode(403);
+                .statusCode(401);
 
         // Enable introspection
         RestAssured.when().post("/oidc/introspection").then().body(equalTo("true"));

diff --git a/...ion-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/ServicePublicKeyTestCase.java b/...ion-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/ServicePublicKeyTestCase.java
@@ -28,6 +28,6 @@ public void testModifiedSignature() throws IOException, InterruptedException {
         Response r = RestAssured.given().auth()
                 .oauth2(jwt + "1")
                 .get("/service/tenant-public-key");
-        Assertions.assertEquals(403, r.getStatusCode());
+        Assertions.assertEquals(401, r.getStatusCode());
     }
 }
diff --git a/integration-tests/oidc/src/main/resources/application.properties b/integration-tests/oidc/src/main/resources/application.properties
@@ -1,6 +1,7 @@
 # Configuration file
 quarkus.oidc.auth-server-url=${keycloak.ssl.url}/realms/quarkus/
 quarkus.oidc.client-id=quarkus-app
+quarkus.oidc.credentials.secret=secret
 quarkus.oidc.token.principal-claim=email
 quarkus.http.cors=true
 quarkus.oidc.tls.verification=none
diff --git a/...gration-tests/oidc/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java b/...gration-tests/oidc/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java
@@ -1,6 +1,7 @@
 package io.quarkus.it.keycloak;
 
 import static io.quarkus.it.keycloak.KeycloakRealmResourceManager.getAccessToken;
+import static io.quarkus.it.keycloak.KeycloakRealmResourceManager.getRefreshToken;
 import static org.awaitility.Awaitility.await;
 import static org.hamcrest.Matchers.equalTo;
 
@@ -77,6 +78,14 @@ public void testAccessAdminResource() {
                 .body(Matchers.containsString("granted:admin"));
     }
 
+    @Test
+    public void testAccessAdminResourceWithRefreshToken() {
+        RestAssured.given().auth().oauth2(getRefreshToken("admin"))
+                .when().get("/api/admin")
+                .then()
+                .statusCode(401);
+    }
+
     @Test
     public void testPermissionHttpInformationProvider() {
         RestAssured.given().auth().oauth2(getAccessToken("alice"))
@@ -119,6 +128,6 @@ public void testExpiredBearerToken() throws InterruptedException {
                 .pollDelay(3, TimeUnit.SECONDS)
                 .atMost(5, TimeUnit.SECONDS).until(
                         () -> RestAssured.given().auth().oauth2(token).when()
-                                .get("/api/users/me").thenReturn().statusCode() == 403);
+                                .get("/api/users/me").thenReturn().statusCode() == 401);
     }
 }
diff --git a/...gration-tests/oidc/src/test/java/io/quarkus/it/keycloak/KeycloakRealmResourceManager.java b/...gration-tests/oidc/src/test/java/io/quarkus/it/keycloak/KeycloakRealmResourceManager.java
@@ -141,4 +141,17 @@ public static String getAccessToken(String userName) {
                 .post(KEYCLOAK_SERVER_URL + "/realms/" + KEYCLOAK_REALM + "/protocol/openid-connect/token")
                 .as(AccessTokenResponse.class).getToken();
     }
+
+    public static String getRefreshToken(String userName) {
+        return RestAssured
+                .given()
+                .param("grant_type", "password")
+                .param("username", userName)
+                .param("password", userName)
+                .param("client_id", "quarkus-app")
+                .param("client_secret", "secret")
+                .when()
+                .post(KEYCLOAK_SERVER_URL + "/realms/" + KEYCLOAK_REALM + "/protocol/openid-connect/token")
+                .as(AccessTokenResponse.class).getRefreshToken();
+    }
 }