fix(security): play framework upgrade (#6626)

* fix(security): play framework upgrade
datahub-project · Dec 9, 2022 · 27ea3bf · 27ea3bf
1 parent 121e9c2
commit 27ea3bf
Show file tree

Hide file tree

Showing 30 changed files with 455 additions and 205 deletions.
diff --git a/.gitignore b/.gitignore
@@ -72,6 +72,7 @@ temp/**
 
 # frontend assets
 datahub-frontend/public/**
+datahub-frontend/test/resources/public/**
 
 .remote*
 # Ignore runtime generated authenticatior/authorizer jar files

diff --git a/build.gradle b/build.gradle
@@ -10,6 +10,7 @@ buildscript {
   ext.testContainersVersion = '1.17.4'
   ext.jacksonVersion = '2.13.4'
   ext.jettyVersion = '9.4.46.v20220331'
+  ext.playVersion = '2.8.18'
   ext.log4jVersion = '2.19.0'
   ext.slf4jVersion = '1.7.32'
   ext.logbackClassic = '1.2.11'
@@ -50,7 +51,7 @@ project.ext.spec = [
 ]
 
 project.ext.externalDependency = [
-    'akkaHttp': 'com.typesafe.akka:akka-http-core_2.12:10.1.15',
+    'akkaHttp': 'com.typesafe.akka:akka-http-core_2.12:10.2.10',
     'antlr4Runtime': 'org.antlr:antlr4-runtime:4.7.2',
     'antlr4': 'org.antlr:antlr4:4.7.2',
     'assertJ': 'org.assertj:assertj-core:3.11.1',
@@ -82,7 +83,7 @@ project.ext.externalDependency = [
     'graphqlJava': 'com.graphql-java:graphql-java:' + graphQLJavaVersion,
     'graphqlJavaScalars': 'com.graphql-java:graphql-java-extended-scalars:' + graphQLJavaVersion,
     'gson': 'com.google.code.gson:gson:2.8.9',
-    'guice': 'com.google.inject:guice:4.2.2',
+    'guice': 'com.google.inject:guice:4.2.3',
     'guava': 'com.google.guava:guava:27.0.1-jre',
     'h2': 'com.h2database:h2:2.1.214',
     'hadoopClient': 'org.apache.hadoop:hadoop-client:3.2.1',
@@ -125,6 +126,7 @@ project.ext.externalDependency = [
     'log4jCore': "org.apache.logging.log4j:log4j-core:$log4jVersion",
     'log4jApi': "org.apache.logging.log4j:log4j-api:$log4jVersion",
     'log4j12Api': "org.slf4j:log4j-over-slf4j:$slf4jVersion",
+    'log4j2Api': "org.apache.logging.log4j:log4j-to-slf4j:$log4jVersion",
     'lombok': 'org.projectlombok:lombok:1.18.12',
     'mariadbConnector': 'org.mariadb.jdbc:mariadb-java-client:2.6.0',
     'mavenArtifact': "org.apache.maven:maven-artifact:$mavenVersion",
@@ -141,17 +143,18 @@ project.ext.externalDependency = [
     'opentracingJdbc':'io.opentracing.contrib:opentracing-jdbc:0.2.15',
     'parquet': 'org.apache.parquet:parquet-avro:1.12.3',
     'picocli': 'info.picocli:picocli:4.5.0',
-    'playCache': 'com.typesafe.play:play-cache_2.12:2.7.6',
-    'playEhcache': 'com.typesafe.play:play-ehcache_2.12:2.7.6',
-    'playWs': 'com.typesafe.play:play-ahc-ws-standalone_2.12:2.0.8',
-    'playDocs': 'com.typesafe.play:play-docs_2.12:2.7.6',
-    'playGuice': 'com.typesafe.play:play-guice_2.12:2.7.6',
-    'playJavaJdbc': 'com.typesafe.play:play-java-jdbc_2.12:2.7.6',
-    'playAkkaHttpServer': 'com.typesafe.play:play-akka-http-server_2.12:2.7.6',
-    'playServer': 'com.typesafe.play:play-server_2.12:2.7.6',
-    'playTest': 'com.typesafe.play:play-test_2.12:2.7.6',
-    'pac4j': 'org.pac4j:pac4j-oidc:3.6.0',
-    'playPac4j': 'org.pac4j:play-pac4j_2.12:8.0.2',
+    'playCache': "com.typesafe.play:play-cache_2.12:$playVersion",
+    'playEhcache': "com.typesafe.play:play-ehcache_2.12:$playVersion",
+    'playWs': 'com.typesafe.play:play-ahc-ws-standalone_2.12:2.1.10',
+    'playDocs': "com.typesafe.play:play-docs_2.12:$playVersion",
+    'playGuice': "com.typesafe.play:play-guice_2.12:$playVersion",
+    'playJavaJdbc': "com.typesafe.play:play-java-jdbc_2.12:$playVersion",
+    'playAkkaHttpServer': "com.typesafe.play:play-akka-http-server_2.12:$playVersion",
+    'playServer': "com.typesafe.play:play-server_2.12:$playVersion",
+    'playTest': "com.typesafe.play:play-test_2.12:$playVersion",
+    'playFilters': "com.typesafe.play:filters-helpers_2.12:$playVersion",
+    'pac4j': 'org.pac4j:pac4j-oidc:4.5.7',
+    'playPac4j': 'org.pac4j:play-pac4j_2.12:9.0.2',
     'postgresql': 'org.postgresql:postgresql:42.3.8',
     'protobuf': 'com.google.protobuf:protobuf-java:3.19.6',
     'rangerCommons': 'org.apache.ranger:ranger-plugins-common:2.3.0',
@@ -203,7 +206,6 @@ configure(subprojects.findAll {! it.name.startsWith('spark-lineage')}) {
     exclude group: "io.netty", module: "netty"
     exclude group: "log4j", module: "log4j"
     exclude group: "org.springframework.boot", module: "spring-boot-starter-logging"
-    exclude group: "org.apache.logging.log4j", module: "log4j-to-slf4j"
     exclude group: "com.vaadin.external.google", module: "android-json"
     exclude group: "org.slf4j", module: "slf4j-reload4j"
     exclude group: "org.slf4j", module: "slf4j-log4j12"
@@ -230,7 +232,7 @@ subprojects {
     dependencies {
       testCompile externalDependency.testng
       constraints {
-        implementation('io.netty:netty-all:4.1.68.Final')
+        implementation('io.netty:netty-all:4.1.85.Final')
         implementation('org.apache.commons:commons-compress:1.21')
         implementation('org.apache.velocity:velocity-engine-core:2.3')
         implementation('org.hibernate:hibernate-validator:6.0.20.Final')

diff --git a/datahub-frontend/README.md b/datahub-frontend/README.md
@@ -7,7 +7,7 @@ DataHub frontend is a [Play](https://www.playframework.com/) service written in
 between [DataHub GMS](../metadata-service) which is the backend service and [DataHub Web](../datahub-web-react/README.md).
 
 ## Pre-requisites
-* You need to have [JDK8](https://www.oracle.com/java/technologies/jdk8-downloads.html)
+* You need to have [JDK11](https://openjdk.org/projects/jdk/11/)
 installed on your machine to be able to build `DataHub Frontend`.
 * You need to have [Chrome](https://www.google.com/chrome/) web browser
 installed to be able to build because UI tests have a dependency on `Google Chrome`.

diff --git a/datahub-frontend/app/auth/AuthModule.java b/datahub-frontend/app/auth/AuthModule.java
@@ -89,7 +89,7 @@ protected void configure() {
                 final String aesKeyHash = DigestUtils.sha1Hex(aesKeyBase.getBytes(StandardCharsets.UTF_8));
                 final String aesEncryptionKey = aesKeyHash.substring(0, 16);
                 playCacheCookieStore = new PlayCookieSessionStore(
-                        new ShiroAesDataEncrypter(aesEncryptionKey));
+                        new ShiroAesDataEncrypter(aesEncryptionKey.getBytes()));
             } catch (Exception e) {
                 throw new RuntimeException("Failed to instantiate Pac4j cookie session store!", e);
             }

diff --git a/datahub-frontend/app/auth/AuthUtils.java b/datahub-frontend/app/auth/AuthUtils.java
@@ -63,8 +63,8 @@ public class AuthUtils {
      *
      * Returns true if the request is eligible to be forwarded to GMS, false otherwise.
      */
-    public static boolean isEligibleForForwarding(Http.Context ctx) {
-        return hasValidSessionCookie(ctx) || hasAuthHeader(ctx);
+    public static boolean isEligibleForForwarding(Http.Request req) {
+        return hasValidSessionCookie(req) || hasAuthHeader(req);
     }
 
     /**
@@ -75,17 +75,17 @@ public static boolean isEligibleForForwarding(Http.Context ctx) {
      * Note that we depend on the presence of 2 cookies, one accessible to the browser and one not,
      * as well as their agreement to determine authentication status.
      */
-    public static boolean hasValidSessionCookie(final Http.Context ctx) {
-        return ctx.session().containsKey(ACTOR)
-                && ctx.request().cookie(ACTOR) != null
-                && ctx.session().get(ACTOR).equals(ctx.request().cookie(ACTOR).value());
+    public static boolean hasValidSessionCookie(final Http.Request req) {
+        return req.session().data().containsKey(ACTOR)
+                && req.cookie(ACTOR) != null
+                && req.session().data().get(ACTOR).equals(req.cookie(ACTOR).value());
     }
 
     /**
      * Returns true if a request includes the Authorization header, false otherwise
      */
-    public static boolean hasAuthHeader(final Http.Context ctx) {
-        return ctx.request().getHeaders().contains(Http.HeaderNames.AUTHORIZATION);
+    public static boolean hasAuthHeader(final Http.Request req) {
+        return req.getHeaders().contains(Http.HeaderNames.AUTHORIZATION);
     }
 
     /**

diff --git a/datahub-frontend/app/auth/Authenticator.java b/datahub-frontend/app/auth/Authenticator.java
@@ -28,40 +28,21 @@ public Authenticator(@Nonnull Config config) {
     }
 
     @Override
-    public String getUsername(@Nonnull Http.Context ctx) {
+    public Optional<String> getUsername(@Nonnull Http.Request req) {
         if (this.metadataServiceAuthEnabled) {
             // If Metadata Service auth is enabled, we only want to verify presence of the
             // "Authorization" header OR the presence of a frontend generated session cookie.
             // At this time, the actor is still considered to be unauthenicated.
-            return AuthUtils.isEligibleForForwarding(ctx) ? "urn:li:corpuser:UNKNOWN" : null;
+            return Optional.ofNullable(AuthUtils.isEligibleForForwarding(req) ? "urn:li:corpuser:UNKNOWN" : null);
         } else {
             // If Metadata Service auth is not enabled, verify the presence of a valid session cookie.
-            return AuthUtils.hasValidSessionCookie(ctx) ? ctx.session().get(ACTOR) : null;
-        }
-    }
-
-    @Override
-    public Optional<String> getUsername(@Nonnull Http.Request request) {
-        Http.Context ctx = Http.Context.current();
-        if (this.metadataServiceAuthEnabled) {
-            // If Metadata Service auth is enabled, we only want to verify presence of the
-            // "Authorization" header OR the presence of a frontend generated session cookie.
-            // At this time, the actor is still considered to be unauthenicated.
-            return Optional.ofNullable(AuthUtils.isEligibleForForwarding(ctx) ? "urn:li:corpuser:UNKNOWN" : null);
-        } else {
-            // If Metadata Service auth is not enabled, verify the presence of a valid session cookie.
-            return Optional.ofNullable(AuthUtils.hasValidSessionCookie(ctx) ? ctx.session().get(ACTOR) : null);
+            return Optional.ofNullable(AuthUtils.hasValidSessionCookie(req) ? req.session().data().get(ACTOR) : null);
         }
     }
 
     @Override
     @Nonnull
-    public Result onUnauthorized(@Nullable Http.Context ctx) {
-        return unauthorized();
-    }
-
-    @Override
-    public Result onUnauthorized(Http.Request req) {
+    public Result onUnauthorized(@Nullable Http.Request req) {
         return unauthorized();
     }
 }
diff --git a/datahub-frontend/app/auth/sso/SsoProvider.java b/datahub-frontend/app/auth/sso/SsoProvider.java
@@ -1,6 +1,7 @@
 package auth.sso;
 
 import org.pac4j.core.client.Client;
+import org.pac4j.core.credentials.Credentials;
 
 /**
  * A thin interface over a Pac4j {@link Client} object and its
@@ -40,6 +41,6 @@ public String getCommonName() {
   /**
    * Retrieves an initialized Pac4j {@link Client}.
    */
-  Client<?, ?> client();
+  Client<? extends Credentials> client();
 
 }
diff --git a/datahub-frontend/app/auth/sso/oidc/OidcAuthorizationGenerator.java b/datahub-frontend/app/auth/sso/oidc/OidcAuthorizationGenerator.java
@@ -1,11 +1,13 @@
 package auth.sso.oidc;
 
 import java.util.Map.Entry;
+import java.util.Optional;
 
 import org.pac4j.core.authorization.generator.AuthorizationGenerator;
 import org.pac4j.core.context.WebContext;
 import org.pac4j.core.profile.AttributeLocation;
 import org.pac4j.core.profile.CommonProfile;
+import org.pac4j.core.profile.UserProfile;
 import org.pac4j.core.profile.definition.ProfileDefinition;
 import org.pac4j.oidc.profile.OidcProfile;
 import org.slf4j.Logger;
@@ -18,33 +20,38 @@ public class OidcAuthorizationGenerator implements AuthorizationGenerator {
 
     private static final Logger logger = LoggerFactory.getLogger(OidcAuthorizationGenerator.class);
 
-    private final ProfileDefinition profileDef;
+    private final ProfileDefinition<?> profileDef;
 
     private final OidcConfigs oidcConfigs;
 
-    public OidcAuthorizationGenerator(final ProfileDefinition profileDef, final OidcConfigs oidcConfigs) {
+    public OidcAuthorizationGenerator(final ProfileDefinition<?> profileDef, final OidcConfigs oidcConfigs) {
         this.profileDef = profileDef;
         this.oidcConfigs = oidcConfigs;
     }
 
     @Override
-    public CommonProfile generate(WebContext context, CommonProfile profile) {
+    public Optional<UserProfile> generate(WebContext context, UserProfile profile) {
         if (oidcConfigs.getExtractJwtAccessTokenClaims().orElse(false)) {
             try {
                 final JWT jwt = JWTParser.parse(((OidcProfile) profile).getAccessToken().getValue());
+
+                CommonProfile commonProfile = new CommonProfile();
 
                 for (final Entry<String, Object> entry : jwt.getJWTClaimsSet().getClaims().entrySet()) {
                     final String claimName = entry.getKey();
+
                     if (profile.getAttribute(claimName) == null) {
-                        profileDef.convertAndAdd(profile, AttributeLocation.PROFILE_ATTRIBUTE, claimName, entry.getValue());
+                        profileDef.convertAndAdd(commonProfile, AttributeLocation.PROFILE_ATTRIBUTE, claimName, entry.getValue());
                     }
                 }
+
+                return Optional.of(commonProfile);
             } catch (Exception e) {
                 logger.warn("Cannot parse access token claims", e);
             }
         }
 
-        return profile;
+        return Optional.ofNullable(profile);
     }
 
 }
diff --git a/datahub-frontend/app/auth/sso/oidc/OidcCallbackLogic.java b/datahub-frontend/app/auth/sso/oidc/OidcCallbackLogic.java
@@ -52,13 +52,16 @@
 import org.pac4j.core.http.adapter.HttpActionAdapter;
 import org.pac4j.core.profile.CommonProfile;
 import org.pac4j.core.profile.ProfileManager;
+import org.pac4j.core.profile.UserProfile;
 import org.pac4j.play.PlayWebContext;
 import play.mvc.Result;
 import auth.sso.SsoManager;
 
+import static auth.AuthUtils.ACCESS_TOKEN;
+import static auth.AuthUtils.ACTOR;
+import static auth.AuthUtils.createActorCookie;
 import static com.linkedin.metadata.Constants.*;
-import static play.mvc.Results.*;
-import static auth.AuthUtils.*;
+import static play.mvc.Results.internalServerError;
 
 
 /**
@@ -101,17 +104,17 @@ public Result perform(PlayWebContext context, Config config,
 
     // By this point, we know that OIDC is the enabled provider.
     final OidcConfigs oidcConfigs = (OidcConfigs) _ssoManager.getSsoProvider().configs();
-    return handleOidcCallback(oidcConfigs, result, context, getProfileManager(context, config));
+    return handleOidcCallback(oidcConfigs, result, context, getProfileManager(context));
   }
 
   private Result handleOidcCallback(final OidcConfigs oidcConfigs, final Result result, final PlayWebContext context,
-      final ProfileManager<CommonProfile> profileManager) {
+      final ProfileManager<UserProfile> profileManager) {
 
     log.debug("Beginning OIDC Callback Handling...");
 
     if (profileManager.isAuthenticated()) {
       // If authenticated, the user should have a profile.
-      final CommonProfile profile = profileManager.get(true).get();
+      final CommonProfile profile = (CommonProfile) profileManager.get(true).get();
       log.debug(String.format("Found authenticated user with profile %s", profile.getAttributes().toString()));
 
       // Extract the User name required to log into DataHub.
@@ -149,8 +152,8 @@ private Result handleOidcCallback(final OidcConfigs oidcConfigs, final Result re
 
       // Successfully logged in - Generate GMS login token
       final String accessToken = _authClient.generateSessionTokenForUser(corpUserUrn.getId());
-      context.getJavaSession().put(ACCESS_TOKEN, accessToken);
-      context.getJavaSession().put(ACTOR, corpUserUrn.toString());
+      context.getNativeSession().adding(ACCESS_TOKEN, accessToken);
+      context.getNativeSession().adding(ACTOR, corpUserUrn.toString());
       return result.withCookies(createActorCookie(corpUserUrn.toString(), oidcConfigs.getSessionTtlInHours()));
     }
     return internalServerError(

diff --git a/datahub-frontend/app/auth/sso/oidc/OidcProvider.java b/datahub-frontend/app/auth/sso/oidc/OidcProvider.java
@@ -8,7 +8,6 @@
 import org.pac4j.core.http.callback.PathParameterCallbackUrlResolver;
 import org.pac4j.oidc.config.OidcConfiguration;
 import org.pac4j.oidc.credentials.OidcCredentials;
-import org.pac4j.oidc.profile.OidcProfile;
 import org.pac4j.oidc.profile.OidcProfileDefinition;
 
 
@@ -27,15 +26,15 @@ public class OidcProvider implements SsoProvider<OidcConfigs> {
   private static final String OIDC_CLIENT_NAME = "oidc";
 
   private final OidcConfigs _oidcConfigs;
-  private final Client<OidcCredentials, OidcProfile> _oidcClient; // Used primarily for redirecting to IdP.
+  private final Client<OidcCredentials> _oidcClient; // Used primarily for redirecting to IdP.
 
   public OidcProvider(final OidcConfigs configs) {
     _oidcConfigs = configs;
     _oidcClient = createPac4jClient();
   }
 
   @Override
-  public Client<OidcCredentials, OidcProfile> client() {
+  public Client<OidcCredentials> client() {
     return _oidcClient;
   }
 
@@ -49,7 +48,7 @@ public SsoProtocol protocol() {
     return SsoProtocol.OIDC;
   }
 
-  private Client<OidcCredentials, OidcProfile> createPac4jClient() {
+  private Client<OidcCredentials> createPac4jClient() {
     final OidcConfiguration oidcConfiguration = new OidcConfiguration();
     oidcConfiguration.setClientId(_oidcConfigs.getClientId());
     oidcConfiguration.setSecret(_oidcConfigs.getClientSecret());

diff --git a/datahub-frontend/app/auth/sso/oidc/OidcResponseErrorHandler.java b/datahub-frontend/app/auth/sso/oidc/OidcResponseErrorHandler.java
@@ -5,6 +5,8 @@
 import org.slf4j.LoggerFactory;
 import play.mvc.Result;
 
+import java.util.Optional;
+
 import static play.mvc.Results.internalServerError;
 import static play.mvc.Results.unauthorized;
 
@@ -26,7 +28,7 @@ public static Result handleError(final PlayWebContext context) {
                 getError(context),
                 getErrorDescription(context));
 
-        if (getError(context).equals("access_denied")) {
+        if (getError(context).isPresent() && getError(context).get().equals("access_denied")) {
             return unauthorized(String.format("Access denied. "
                     + "The OIDC service responded with 'Access denied'. "
                     + "It seems that you don't have access to this application yet. Please apply for access. \n\n"
@@ -38,18 +40,18 @@ public static Result handleError(final PlayWebContext context) {
 
         return internalServerError(
                 String.format("Internal server error. The OIDC service responded with an error: '%s'.\n"
-                        + "Error description: '%s'", getError(context), getErrorDescription(context)));
+                        + "Error description: '%s'", getError(context).orElse(""), getErrorDescription(context).orElse("")));
     }
 
     public static boolean isError(final PlayWebContext context) {
-        return getError(context) != null && !getError(context).isEmpty();
+        return getError(context).isPresent() && !getError(context).get().isEmpty();
     }
 
-    public static String getError(final PlayWebContext context) {
+    public static Optional<String> getError(final PlayWebContext context) {
         return context.getRequestParameter(ERROR_FIELD_NAME);
     }
 
-    public static String getErrorDescription(final PlayWebContext context) {
+    public static Optional<String> getErrorDescription(final PlayWebContext context) {
         return context.getRequestParameter(ERROR_DESCRIPTION_FIELD_NAME);
     }
 }