Accept http URLs in OAuth2 Token Exchange (unitycatalog#731)

Fixes unitycatalog#730 and adds the following: 1. Uses built-in claim names (`JwtClaim.EMAIL`, `JwtClaim.SUBJECT`) 1. Uses built-in claim accessors (`decodedJWT.getIssuer()`, `decodedJWT.getKeyId()`, `decodedJWT.getSubject()`) 1. More DEBUG logging in OAuth2 execution paths Signed-off-by: Jacek Laskowski <[email protected]>
PostHog · Nov 20, 2024 · d02929b · d02929b
1 parent 9d68cd8
commit d02929b
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 21 deletions.
diff --git a/server/src/main/java/io/unitycatalog/server/security/SecurityContext.java b/server/src/main/java/io/unitycatalog/server/security/SecurityContext.java
@@ -72,9 +72,10 @@ public SecurityContext(
   public String createAccessToken(DecodedJWT decodedJWT) {
 
     String subject =
-        decodedJWT.getClaim(JwtClaim.EMAIL.key()).isMissing()
-            ? decodedJWT.getClaim(JwtClaim.SUBJECT.key()).asString()
-            : decodedJWT.getClaim(JwtClaim.EMAIL.key()).asString();
+        decodedJWT
+            .getClaims()
+            .getOrDefault(JwtClaim.EMAIL.key(), decodedJWT.getClaim(JwtClaim.SUBJECT.key()))
+            .asString();
 
     return JWT.create()
         .withSubject(serviceName)

diff --git a/server/src/main/java/io/unitycatalog/server/service/AuthDecorator.java b/server/src/main/java/io/unitycatalog/server/service/AuthDecorator.java
@@ -17,7 +17,6 @@
 import io.unitycatalog.server.exception.AuthorizationException;
 import io.unitycatalog.server.exception.ErrorCode;
 import io.unitycatalog.server.persist.UserRepository;
-import io.unitycatalog.server.security.JwtClaim;
 import io.unitycatalog.server.security.SecurityContext;
 import io.unitycatalog.server.utils.JwksOperations;
 import org.slf4j.Logger;
@@ -68,10 +67,10 @@ public HttpResponse serve(HttpService delegate, ServiceRequestContext ctx, HttpR
     DecodedJWT decodedJWT =
         JWT.decode(getAccessTokenFromCookieOrAuthHeader(authorizationHeader, authorizationCookie));
 
-    String issuer = decodedJWT.getClaim(JwtClaim.ISSUER.key()).asString();
-    String keyId = decodedJWT.getHeaderClaim(JwtClaim.KEY_ID.key()).asString();
+    String issuer = decodedJWT.getIssuer();
+    String keyId = decodedJWT.getKeyId();
 
-    LOGGER.debug("Validating access-token for issuer: {}", issuer);
+    LOGGER.debug("Validating access-token for issuer: {} and keyId: {}", issuer, keyId);
 
     if (!issuer.equals(INTERNAL)) {
       throw new AuthorizationException(ErrorCode.PERMISSION_DENIED, "Invalid access token.");
@@ -80,17 +79,20 @@ public HttpResponse serve(HttpService delegate, ServiceRequestContext ctx, HttpR
     JWTVerifier jwtVerifier = jwksOperations.verifierForIssuerAndKey(issuer, keyId);
     decodedJWT = jwtVerifier.verify(decodedJWT);
 
+    String subject = decodedJWT.getSubject();
+
     User user;
     try {
-      user = USER_REPOSITORY.getUserByEmail(decodedJWT.getClaim(JwtClaim.SUBJECT.key()).asString());
+      user = USER_REPOSITORY.getUserByEmail(subject);
     } catch (Exception e) {
+      LOGGER.debug("User not found: {}", subject);
       user = null;
     }
     if (user == null || user.getState() != User.StateEnum.ENABLED) {
-      throw new AuthorizationException(ErrorCode.PERMISSION_DENIED, "User not allowed.");
+      throw new AuthorizationException(ErrorCode.PERMISSION_DENIED, "User not allowed: " + subject);
     }
 
-    LOGGER.debug("Access allowed for subject: {}", decodedJWT.getClaim(JwtClaim.SUBJECT.key()));
+    LOGGER.debug("Access allowed for subject: {}", subject);
 
     ctx.setAttr(DECODED_JWT_ATTR, decodedJWT);
 

diff --git a/server/src/main/java/io/unitycatalog/server/service/AuthService.java b/server/src/main/java/io/unitycatalog/server/service/AuthService.java
@@ -139,9 +139,10 @@ public HttpResponse grantToken(
     }
 
     DecodedJWT decodedJWT = JWT.decode(request.getSubjectToken());
-    String issuer = decodedJWT.getClaim("iss").asString();
-    String keyId = decodedJWT.getHeaderClaim("kid").asString();
-    LOGGER.debug("Validating token for issuer: {}", issuer);
+    String issuer = decodedJWT.getIssuer();
+    String keyId = decodedJWT.getKeyId();
+
+    LOGGER.debug("Validating token for issuer: {} and keyId: {}", issuer, keyId);
 
     JWTVerifier jwtVerifier = jwksOperations.verifierForIssuerAndKey(issuer, keyId);
     decodedJWT = jwtVerifier.verify(decodedJWT);
@@ -193,17 +194,22 @@ public HttpResponse logout(HttpRequest request) {
 
   private static void verifyPrincipal(DecodedJWT decodedJWT) {
     String subject =
-        decodedJWT.getClaim(JwtClaim.EMAIL.key()).isMissing()
-            ? decodedJWT.getClaim(JwtClaim.SUBJECT.key()).asString()
-            : decodedJWT.getClaim(JwtClaim.EMAIL.key()).asString();
+        decodedJWT
+            .getClaims()
+            .getOrDefault(JwtClaim.EMAIL.key(), decodedJWT.getClaim(JwtClaim.SUBJECT.key()))
+            .asString();
+
+    LOGGER.debug("Validating principal: {}", subject);
 
     if (subject.equals("admin")) {
+      LOGGER.debug("admin always allowed");
       return;
     }
 
     try {
       User user = USER_REPOSITORY.getUserByEmail(subject);
       if (user != null && user.getState() == User.StateEnum.ENABLED) {
+        LOGGER.debug("Principal {} is enabled", subject);
         return;
       }
     } catch (Exception e) {

diff --git a/server/src/main/java/io/unitycatalog/server/utils/JwksOperations.java b/server/src/main/java/io/unitycatalog/server/utils/JwksOperations.java
@@ -20,14 +20,19 @@
 import java.security.interfaces.RSAPublicKey;
 import java.util.Map;
 
+import io.unitycatalog.server.service.AuthService;
 import lombok.SneakyThrows;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class JwksOperations {
 
   private final WebClient webClient = WebClient.builder().build();
   private static final ObjectMapper mapper = new ObjectMapper();
   private final SecurityContext securityContext;
 
+  private static final Logger LOGGER = LoggerFactory.getLogger(JwksOperations.class);
+
   public JwksOperations(SecurityContext securityContext) {
     this.securityContext = securityContext;
   }
@@ -61,7 +66,7 @@ private Algorithm algorithmForJwk(Jwk jwk) {
 
   @SneakyThrows
   public JwkProvider loadJwkProvider(String issuer) {
-
+    LOGGER.debug("Loading JwkProvider for issuer '{}'", issuer);
     if (issuer.equals(INTERNAL)) {
       // Return our own "self-signed" provider, for easy mode.
       // TODO: This should be configurable
@@ -71,7 +76,7 @@ public JwkProvider loadJwkProvider(String issuer) {
       // Get the JWKS from the OIDC well-known location described here
       // https://openid.net/specs/openid-connect-discovery-1_0-21.html#ProviderConfig
 
-      if (!issuer.startsWith("https://")) {
+      if (!issuer.startsWith("https://") && !issuer.startsWith("http://")) {
         issuer = "https://" + issuer;
       }
 
@@ -81,15 +86,17 @@ public JwkProvider loadJwkProvider(String issuer) {
         wellKnownConfigUrl += "/";
       }
 
+      var path = wellKnownConfigUrl + ".well-known/openid-configuration";
+      LOGGER.debug("path: {}", path);
+
       String response = webClient
-              .get(wellKnownConfigUrl + ".well-known/openid-configuration")
+              .get(path)
               .aggregate()
               .join()
               .contentUtf8();
 
       // TODO: We should cache this. No need to fetch it each time.
-      Map<String, Object> configMap = mapper.readValue(response, new TypeReference<>() {
-      });
+      Map<String, Object> configMap = mapper.readValue(response, new TypeReference<>() {});
 
       if (configMap == null || configMap.isEmpty()) {
         throw new OAuthInvalidRequestException(ErrorCode.ABORTED, "Could not get issuer configuration");