[NOTASK]

refactor/cleanup: clean up old dev patterns, clean up warnings (deprecations, unchecked, etc), centralize log factory update pac4j (5.7.1 -> 5.7.2) add developer notes
internet2 · Dec 4, 2023 · 052c427 · 052c427
1 parent 2c454f4
commit 052c427
Showing 9 changed files with 70 additions and 87 deletions.
diff --git a/DEVNOTES.adoc b/DEVNOTES.adoc
@@ -0,0 +1,14 @@
+= Developer Notes
+
+* When building, there will be warnings about `Bundle edu.internet2.middleware.grouper.plugins:grouper-authentication-plugin:bundle:0.0.1-SNAPSHOT : Classes found in the wrong directory`. This can be safely ignored
+
+* During tests, there will be log messages like
++
+----
+2023-12-01T10:30:12,364: [main] WARN  ConfigUtils.checkConfig(93) - [] - you are using the config key `external.authentication.saml.keyStoreAlias`; this should be changed to `external.authentication.saml.keystoreAlias`
+2023-12-01T10:30:12,368: [main] WARN  ConfigUtils.checkConfig(93) - [] - you are using the config key `external.authentication.saml.keyStoreType`; this should be changed to `external.authentication.saml.keystoreType`
+----
++
+These can usually be safely ignored.
++
+* SPIs will not work properly if the jar is built with inline; this should be considered if this configuration is changed. Some of the included libraries/frameworks depend on this, notably opensaml.
diff --git a/pom.xml b/pom.xml
@@ -34,7 +34,8 @@
     <properties>
         <maven.compiler.source>17</maven.compiler.source>
         <maven.compiler.target>17</maven.compiler.target>
-        <pac4j.version>5.7.1</pac4j.version>
+        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+        <pac4j.version>5.7.2</pac4j.version>
         <javaee-pac4j.version>7.1.0</javaee-pac4j.version>
         <grouper.version>4.7.0</grouper.version>
     </properties>
@@ -69,7 +70,7 @@
         </dependency>
         <dependency>
             <groupId>org.pac4j</groupId>
-            <artifactId>pac4j-cas</artifactId>
+            <artifactId>pac4j-cas-clientv4</artifactId>
             <version>${pac4j.version}</version>
         </dependency>
         <dependency>
@@ -79,7 +80,7 @@
         </dependency>
         <dependency>
             <groupId>org.pac4j</groupId>
-            <artifactId>pac4j-saml</artifactId>
+            <artifactId>pac4j-saml-opensamlv5</artifactId>
             <version>${pac4j.version}</version>
             <exclusions>
                 <exclusion>
@@ -165,9 +166,20 @@
 
     <build>
         <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-compiler-plugin</artifactId>
+                <version>3.11.0</version>
+                <configuration>
+                    <compilerArgs>
+                        <arg>-Xlint:unchecked</arg>
+                    </compilerArgs>
+                </configuration>
+            </plugin>
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-jar-plugin</artifactId>
+                <version>3.3.0</version>
                 <configuration>
                     <archive>
                         <index>true</index>
@@ -185,14 +197,16 @@
             <plugin>
                 <groupId>org.apache.felix</groupId>
                 <artifactId>maven-bundle-plugin</artifactId>
+                <version>5.1.9</version>
                 <extensions>true</extensions>
                 <configuration>
                     <instructions>
                         <Bundle-SymbolicName>${project.groupId}.${project.artifactId}</Bundle-SymbolicName>
                         <Bundle-Name>${project.artifactId}-bundle</Bundle-Name>
                         <Bundle-Version>${project.version}</Bundle-Version>
                         <Bundle-Activator>edu.internet2.middleware.grouper.authentication.plugin.GrouperAuthentication</Bundle-Activator>
-                        <Export-Package>edu.internet2.middleware.grouper.authentication.plugin.filter</Export-Package>
+                        <!-- <Export-Package>edu.internet2.middleware.grouper.authentication.plugin.filter</Export-Package> -->
+                        <Export-Package>!*</Export-Package>
                         <Private-Package>edu.internet2.middleware.grouper.authentication.plugin.*</Private-Package>
                         <Embed-Dependency>*;scope=compile|runtime;inline=false</Embed-Dependency>
                         <Embed-Transitive>true</Embed-Transitive>

diff --git a/src/main/java/edu/internet2/middleware/grouper/authentication/plugin/ConfigUtils.java b/src/main/java/edu/internet2/middleware/grouper/authentication/plugin/ConfigUtils.java
@@ -17,10 +17,12 @@
 import java.lang.reflect.Type;
 import java.time.Period;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.List;
 
 public class ConfigUtils {
     private static final Map<String, String> PROPERTY_RENAMES = new HashMap<>();
@@ -49,8 +51,9 @@ public static ConfigPropertiesCascadeBase getBestGrouperConfiguration() {
 
     public static ConfigPropertiesCascadeBase getConfigPropertiesCascadeBase(String type) {
         try {
-            ServiceReference<ConfigPropertiesCascadeBase> serviceReference = (ServiceReference<ConfigPropertiesCascadeBase>) bundleContext.getServiceReferences(ConfigPropertiesCascadeBase.class, "(type=" + type + ")").toArray()[0];
-            return bundleContext.getService(serviceReference);
+            List<ServiceReference<ConfigPropertiesCascadeBase>> serviceReferenceList = (List<ServiceReference<ConfigPropertiesCascadeBase>>) bundleContext.getServiceReferences(ConfigPropertiesCascadeBase.class, "(type=" + type + ")");
+            ServiceReference<?> serviceReference = serviceReferenceList.get(0);
+            return (ConfigPropertiesCascadeBase) bundleContext.getService(serviceReference);
         } catch (InvalidSyntaxException e) {
             throw new RuntimeException(e);
         }
@@ -127,7 +130,11 @@ private static Method getSetter(Class clazz, String name) throws NoSuchMethodExc
 
     private static Object getProperty(ConfigPropertiesCascadeBase configPropertiesCascadeBase, Type type, String propName) throws ClassNotFoundException {
         if (Enum.class.isAssignableFrom((Class<?>) type)) {
-            return Enum.valueOf((Class<Enum>)type, configPropertiesCascadeBase.propertyValueString(propName));
+            // there are a few properties that are enums (e.g., CAS protocol)
+            // TODO: can this be checked?
+            @SuppressWarnings("unchecked")
+            Enum<?> e = Enum.valueOf((Class) type, configPropertiesCascadeBase.propertyValueString(propName));
+            return e;
         } else {
             switch (type.getTypeName()) {
                 case "java.lang.String": {
@@ -154,14 +161,13 @@ private static Object getProperty(ConfigPropertiesCascadeBase configPropertiesCa
                     return Arrays.asList(configPropertiesCascadeBase.propertyValueString(propName).split(","));
                 }
                 case "java.util.Set": {
-                    Set set = new HashSet();
-                    for (String prop : configPropertiesCascadeBase.propertyValueString(propName).split(",")) {
-                        set.add(prop);
-                    }
+                    //TODO: hopefully string is all that is needed, but might need to revisit
+                    Set<String> set = new HashSet<>();
+                    Collections.addAll(set, configPropertiesCascadeBase.propertyValueString(propName).split(","));
                     return set;
                 }
                 case "java.util.Map": {
-                    Map<String, String> map = new HashMap();
+                    Map<String, String> map = new HashMap<>();
                     for (String pairs : configPropertiesCascadeBase.propertyValueString(propName).split(",")) {
                         String[] keyValue = pairs.split("=");
                         map.put(keyValue[0].trim(), keyValue[1].trim());

diff --git a/...ware/grouper/authentication/plugin/ExternalAuthenticationServletContainerInitializer.java b/...ware/grouper/authentication/plugin/ExternalAuthenticationServletContainerInitializer.java
@@ -3,10 +3,6 @@
 import edu.internet2.middleware.grouper.authentication.plugin.filter.CallbackFilterDecorator;
 import edu.internet2.middleware.grouper.authentication.plugin.filter.SecurityFilterDecorator;
 import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-import org.osgi.framework.BundleContext;
-import org.osgi.framework.InvalidSyntaxException;
-import org.osgi.framework.ServiceReference;
 
 import javax.servlet.FilterRegistration;
 import javax.servlet.ServletContainerInitializer;
@@ -15,17 +11,7 @@
 import java.util.Set;
 
 public class ExternalAuthenticationServletContainerInitializer implements ServletContainerInitializer {
-    private final Log log;
-
-    public ExternalAuthenticationServletContainerInitializer(BundleContext bundleContext) {
-        try {
-            //TODO: figure out why this is weird
-            ServiceReference<LogFactory> logfactoryReference = (ServiceReference<LogFactory>) bundleContext.getAllServiceReferences("org.apache.commons.logging.LogFactory", null)[0];
-            log = bundleContext.getService(logfactoryReference).getInstance(ExternalAuthenticationServletContainerInitializer.class);
-        } catch (InvalidSyntaxException e) {
-            throw new RuntimeException(e);
-        }
-    }
+    private static final Log log = GrouperAuthentication.getLogFactory().getInstance(ExternalAuthenticationServletContainerInitializer.class);
 
     @Override
     public void onStartup(Set<Class<?>> c, ServletContext ctx) throws ServletException {

diff --git a/...in/java/edu/internet2/middleware/grouper/authentication/plugin/GrouperAuthentication.java b/...in/java/edu/internet2/middleware/grouper/authentication/plugin/GrouperAuthentication.java
@@ -15,13 +15,15 @@
 
 public class GrouperAuthentication implements BundleActivator {
     private Map<String, ServiceReference> referenceMap = new HashMap<>();
-    private Map<String, ServiceRegistration> registrationMap = new HashMap<>();
+    private final Map<String, ServiceRegistration<?>> registrationMap = new HashMap<>();
 
     private static final LogFactory LOG_FACTORY;
     static {
         try {
             BundleContext bundleContext = FrameworkUtil.getBundle(GrouperAuthentication.class).getBundleContext();
             //TODO: figure out why this is weird
+            // TODO: check if this can be checked
+            @SuppressWarnings("unchecked")
             ServiceReference<LogFactory> logfactoryReference = (ServiceReference<LogFactory>) bundleContext.getAllServiceReferences("org.apache.commons.logging.LogFactory", null)[0];
             bundleContext.getServiceReference("org.apache.commons.logging.LogFactory");
             if (bundleContext.getService(logfactoryReference) != null) {
@@ -40,7 +42,7 @@ public static LogFactory getLogFactory() {
 
     @Override
     public void start(BundleContext context) throws Exception {
-        ExternalAuthenticationServletContainerInitializer externalAuthenticationServletContainerInitializer = new ExternalAuthenticationServletContainerInitializer(context);
+        ExternalAuthenticationServletContainerInitializer externalAuthenticationServletContainerInitializer = new ExternalAuthenticationServletContainerInitializer();
         ServiceRegistration easciRegistration = context.registerService(ServletContainerInitializer.class, externalAuthenticationServletContainerInitializer, new Hashtable<>());
         registrationMap.put(ExternalAuthenticationServletContainerInitializer.class.getCanonicalName(), easciRegistration);
     }

diff --git a/src/main/java/edu/internet2/middleware/grouper/authentication/plugin/Pac4jConfigFactory.java b/src/main/java/edu/internet2/middleware/grouper/authentication/plugin/Pac4jConfigFactory.java
@@ -5,30 +5,16 @@
 import edu.internet2.middleware.grouperClient.config.ConfigPropertiesCascadeBase;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-import org.osgi.framework.BundleContext;
-import org.osgi.framework.FrameworkUtil;
-import org.osgi.framework.InvalidSyntaxException;
-import org.osgi.framework.ServiceReference;
 import org.pac4j.core.client.Client;
 import org.pac4j.core.client.Clients;
 import org.pac4j.core.config.Config;
 import org.pac4j.core.config.ConfigFactory;
 import org.pac4j.core.matching.matcher.PathMatcher;
 
+import java.lang.reflect.InvocationTargetException;
+
 public class Pac4jConfigFactory implements ConfigFactory {
-    // private static final Logger LOGGER = Logger.getLogger(Pac4jConfigFactory.class);
-    private static final Log LOGGER;
-    static {
-        try {
-            BundleContext bundleContext = FrameworkUtil.getBundle(GrouperAuthentication.class).getBundleContext();
-            //TODO: figure out why this is weird
-            ServiceReference<LogFactory> logfactoryReference = (ServiceReference<LogFactory>) bundleContext.getAllServiceReferences("org.apache.commons.logging.LogFactory", null)[0];
-            LOGGER = bundleContext.getService(logfactoryReference).getInstance(ExternalAuthenticationServletContainerInitializer.class);
-        } catch (InvalidSyntaxException e) {
-            throw new RuntimeException(e);
-        }
-    }
+    private static final Log LOGGER = GrouperAuthentication.getLogFactory().getInstance(Pac4jConfigFactory.class);
 
     @Override
     public Config build(Object... parameters) {
@@ -61,14 +47,16 @@ public Config build(Object... parameters) {
 
             config.addMatcher("securityExclusions", pathMatcher);
             return config;
-        } catch (IllegalAccessException|InstantiationException e) {
+        } catch (IllegalAccessException | InstantiationException | NoSuchMethodException | InvocationTargetException e) {
             throw new RuntimeException("problem configuring pac4j", e);
         } finally {
             Thread.currentThread().setContextClassLoader(classLoader);
         }
     }
 
-    private static Client getClient(String provider) throws IllegalAccessException, InstantiationException {
+    //TODO: can this be checked
+    @SuppressWarnings("unchecked")
+    private static Client getClient(String provider) throws IllegalAccessException, InstantiationException, NoSuchMethodException, InvocationTargetException {
         Class<? extends ClientProvider> providerClass;
         //TODO: might be a better way of doing this
         try {
@@ -80,6 +68,6 @@ private static Client getClient(String provider) throws IllegalAccessException,
                 throw new RuntimeException(classNotFoundException);
             }
         }
-        return providerClass.newInstance().getClient();
+        return providerClass.getDeclaredConstructor().newInstance().getClient();
     }
 }
diff --git a/...ava/edu/internet2/middleware/grouper/authentication/plugin/config/OidcClientProvider.java b/...ava/edu/internet2/middleware/grouper/authentication/plugin/config/OidcClientProvider.java
@@ -1,32 +1,18 @@
 package edu.internet2.middleware.grouper.authentication.plugin.config;
 
 import edu.internet2.middleware.grouper.authentication.plugin.ConfigUtils;
-import edu.internet2.middleware.grouper.authentication.plugin.ExternalAuthenticationServletContainerInitializer;
 import edu.internet2.middleware.grouper.authentication.plugin.GrouperAuthentication;
 import edu.internet2.middleware.grouper.authentication.plugin.oidc.client.ClaimAsUsernameOidcClient;
 import edu.internet2.middleware.grouper.authentication.plugin.oidc.config.ClaimAsUsernameOidcConfiguration;
 import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-import org.osgi.framework.BundleContext;
-import org.osgi.framework.FrameworkUtil;
-import org.osgi.framework.InvalidSyntaxException;
-import org.osgi.framework.ServiceReference;
 import org.pac4j.core.client.Client;
 import org.pac4j.oidc.client.OidcClient;
 import org.pac4j.oidc.config.OidcConfiguration;
 
+import java.lang.reflect.InvocationTargetException;
+
 public class OidcClientProvider implements ClientProvider {
-    private static final Log LOGGER;
-    static {
-        try {
-            BundleContext bundleContext = FrameworkUtil.getBundle(GrouperAuthentication.class).getBundleContext();
-            //TODO: figure out why this is weird
-            ServiceReference<LogFactory> logfactoryReference = (ServiceReference<LogFactory>) bundleContext.getAllServiceReferences("org.apache.commons.logging.LogFactory", null)[0];
-            LOGGER = bundleContext.getService(logfactoryReference).getInstance(ExternalAuthenticationServletContainerInitializer.class);
-        } catch (InvalidSyntaxException e) {
-            throw new RuntimeException(e);
-        }
-    }
+    private static final Log LOGGER = GrouperAuthentication.getLogFactory().getInstance(OidcClientProvider.class);
 
     @Override
     public boolean supports(String type) {
@@ -39,9 +25,11 @@ public Client getClient() {
         String implementation = ConfigUtils.getBestGrouperConfiguration().propertyValueString("external.authentication.mechanism.oidc.clientImplementation");
         if (implementation != null && !implementation.isEmpty()) {
             try {
-                OidcConfiguration configuration = (OidcConfiguration) Class.forName(implementation).newInstance();
+                OidcConfiguration configuration = (OidcConfiguration) Class.forName(implementation).getDeclaredConstructor().newInstance();
                 client = new OidcClient(configuration);
-            } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) {
+            } catch (InstantiationException | IllegalAccessException | ClassNotFoundException | NoSuchMethodException |
+                     InvocationTargetException e) {
+                //TODO: check whether this is actually a good idea and/or approapriate. there are a few cases that might need to be handled differently (e.g., wrong classname)
                 LOGGER.warn("problem loading pac4j client implementation; using a default", e);
                 client = getClaimAsUsernameOidcClient();
             }

diff --git a/...va/edu/internet2/middleware/grouper/authentication/plugin/filter/ReinitializingTimer.java b/...va/edu/internet2/middleware/grouper/authentication/plugin/filter/ReinitializingTimer.java
@@ -3,32 +3,16 @@
 import edu.internet2.middleware.grouper.authentication.plugin.ConfigUtils;
 import edu.internet2.middleware.grouper.authentication.plugin.ExternalAuthenticationServletContainerInitializer;
 import edu.internet2.middleware.grouper.authentication.plugin.GrouperAuthentication;
-import edu.internet2.middleware.grouper.authentication.plugin.Pac4jConfigFactory;
 import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-import org.osgi.framework.BundleContext;
-import org.osgi.framework.FrameworkUtil;
-import org.osgi.framework.InvalidSyntaxException;
-import org.osgi.framework.ServiceReference;
 
 import java.util.Map;
 import java.util.TimerTask;
 import java.util.regex.Pattern;
 
 public class ReinitializingTimer extends TimerTask {
-    private static final Log LOGGER;
-    static {
-        try {
-            BundleContext bundleContext = FrameworkUtil.getBundle(GrouperAuthentication.class).getBundleContext();
-            //TODO: figure out why this is weird
-            ServiceReference<LogFactory> logfactoryReference = (ServiceReference<LogFactory>) bundleContext.getAllServiceReferences("org.apache.commons.logging.LogFactory", null)[0];
-            LOGGER = bundleContext.getService(logfactoryReference).getInstance(ExternalAuthenticationServletContainerInitializer.class);
-        } catch (InvalidSyntaxException e) {
-            throw new RuntimeException(e);
-        }
-    }
+    private static final Log LOGGER = GrouperAuthentication.getLogFactory().getInstance(ExternalAuthenticationServletContainerInitializer.class);
 
-    private Map config;
+    private Map<String, String> config;
     private final Reinitializable initTarget;
 
     public ReinitializingTimer(Reinitializable initTarget) {
@@ -45,7 +29,7 @@ protected static boolean areEqual(Map<String, String> first, Map<String, String>
 
     @Override
     public void run() {
-        Map curConfig = ConfigUtils.getBestGrouperConfiguration().propertiesMap(Pattern.compile("^external\\.authentication\\.([^.]+)$"));
+        Map<String, String> curConfig = ConfigUtils.getBestGrouperConfiguration().propertiesMap(Pattern.compile("^external\\.authentication\\.([^.]+)$"));
         if (!areEqual(config, curConfig)) {
             config = curConfig;
             initTarget.initDecorator();

diff --git a/src/test/java/edu/internet2/middleware/grouper/authentication/Pac4JConfigFactoryTest.java b/src/test/java/edu/internet2/middleware/grouper/authentication/Pac4JConfigFactoryTest.java
@@ -39,6 +39,7 @@
 public class Pac4JConfigFactoryTest {
     MockedStatic<FrameworkUtil> frameworkUtilMockedStatic;
 
+    @SuppressWarnings("unchecked")
     @Before
     public void setup() throws Exception {
         this.frameworkUtilMockedStatic = Mockito.mockStatic(FrameworkUtil.class);