Issue #5486 PropertyFileLoginModule retains PropertyUserStores (#5518)

* Issue #5486 PropertyFileLoginModule retains PropertyUserStores

Signed-off-by: Jan Bartel <janb@webtide.com>

jetty-jaas/src/main/java/org/eclipse/jetty/jaas/JAASLoginService.java

@@ -20,15 +20,16 @@
 
 import java.io.IOException;
 import java.security.Principal;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Set;
 import javax.security.auth.Subject;
 import javax.security.auth.callback.Callback;
 import javax.security.auth.callback.CallbackHandler;
 import javax.security.auth.callback.NameCallback;
-import javax.security.auth.callback.PasswordCallback;
 import javax.security.auth.callback.UnsupportedCallbackException;
 import javax.security.auth.login.Configuration;
 import javax.security.auth.login.FailedLoginException;
@@ -37,17 +38,14 @@
 import javax.servlet.ServletRequest;
 
 import org.eclipse.jetty.jaas.callback.DefaultCallbackHandler;
-import org.eclipse.jetty.jaas.callback.ObjectCallback;
-import org.eclipse.jetty.jaas.callback.RequestParameterCallback;
-import org.eclipse.jetty.jaas.callback.ServletRequestCallback;
 import org.eclipse.jetty.security.DefaultIdentityService;
 import org.eclipse.jetty.security.IdentityService;
 import org.eclipse.jetty.security.LoginService;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.UserIdentity;
 import org.eclipse.jetty.util.ArrayUtil;
 import org.eclipse.jetty.util.Loader;
-import org.eclipse.jetty.util.component.AbstractLifeCycle;
+import org.eclipse.jetty.util.component.ContainerLifeCycle;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 
@@ -58,13 +56,14 @@
  * Implementation of jetty's LoginService that works with JAAS for
  * authorization and authentication.
  */
-public class JAASLoginService extends AbstractLifeCycle implements LoginService
+public class JAASLoginService extends ContainerLifeCycle implements LoginService
 {
     private static final Logger LOG = Log.getLogger(JAASLoginService.class);
 
     public static final String DEFAULT_ROLE_CLASS_NAME = "org.eclipse.jetty.jaas.JAASRole";
     public static final String[] DEFAULT_ROLE_CLASS_NAMES = {DEFAULT_ROLE_CLASS_NAME};
-
+    public static final ThreadLocal<JAASLoginService> INSTANCE = new ThreadLocal<>();
+
     protected String[] _roleClassNames = DEFAULT_ROLE_CLASS_NAMES;
     protected String _callbackHandlerClass;
     protected String _realmName;
@@ -183,6 +182,7 @@ protected void doStart() throws Exception
     {
         if (_identityService == null)
             _identityService = new DefaultIdentityService();
+        addBean(new PropertyUserStoreManager());
         super.doStart();
     }
 
@@ -193,59 +193,27 @@ public UserIdentity login(final String username, final Object credentials, final
         {
             CallbackHandler callbackHandler = null;
             if (_callbackHandlerClass == null)
-            {
-                callbackHandler = new CallbackHandler()
-                {
-                    @Override
-                    public void handle(Callback[] callbacks) throws IOException, UnsupportedCallbackException
-                    {
-                        for (Callback callback : callbacks)
-                        {
-                            if (callback instanceof NameCallback)
-                            {
-                                ((NameCallback)callback).setName(username);
-                            }
-                            else if (callback instanceof PasswordCallback)
-                            {
-                                ((PasswordCallback)callback).setPassword(credentials.toString().toCharArray());
-                            }
-                            else if (callback instanceof ObjectCallback)
-                            {
-                                ((ObjectCallback)callback).setObject(credentials);
-                            }
-                            else if (callback instanceof RequestParameterCallback)
-                            {
-                                RequestParameterCallback rpc = (RequestParameterCallback)callback;
-                                if (request != null)
-                                    rpc.setParameterValues(Arrays.asList(request.getParameterValues(rpc.getParameterName())));
-                            }
-                            else if (callback instanceof ServletRequestCallback)
-                            {
-                                ((ServletRequestCallback)callback).setRequest(request);
-                            }
-                            else
-                                throw new UnsupportedCallbackException(callback);
-                        }
-                    }
-                };
-            }
+                callbackHandler = new DefaultCallbackHandler();
             else
             {
                 Class<?> clazz = Loader.loadClass(_callbackHandlerClass);
                 callbackHandler = (CallbackHandler)clazz.getDeclaredConstructor().newInstance();
-                if (DefaultCallbackHandler.class.isAssignableFrom(clazz))
-                {
-                    DefaultCallbackHandler dch = (DefaultCallbackHandler)callbackHandler;
-                    if (request instanceof Request)
-                        dch.setRequest((Request)request);
-                    dch.setCredential(credentials);
-                    dch.setUserName(username);
-                }
+            }
+
+            if (callbackHandler instanceof DefaultCallbackHandler)
+            {
+                DefaultCallbackHandler dch = (DefaultCallbackHandler)callbackHandler;
+                if (request instanceof Request)
+                    dch.setRequest((Request)request);
+                dch.setCredential(credentials);
+                dch.setUserName(username);
             }
 
             //set up the login context
             Subject subject = new Subject();
-            LoginContext loginContext = (_configuration == null ? new LoginContext(_loginModuleName, subject, callbackHandler)
+            INSTANCE.set(this);
+            LoginContext loginContext = 
+                (_configuration == null ? new LoginContext(_loginModuleName, subject, callbackHandler)
                 : new LoginContext(_loginModuleName, subject, callbackHandler, _configuration));
 
             loginContext.login();
@@ -265,6 +233,10 @@ else if (callback instanceof ServletRequestCallback)
         {
             LOG.ignore(e);
         }
+        finally
+        {
+            INSTANCE.remove();
+        }
         return null;
     }
 
@@ -306,52 +278,36 @@ public void logout(UserIdentity user)
     protected String[] getGroups(Subject subject)
     {
         Collection<String> groups = new LinkedHashSet<>();
-        Set<Principal> principals = subject.getPrincipals();
-        for (Principal principal : principals)
+        for (Principal principal : subject.getPrincipals())
         {
-            Class<?> c = principal.getClass();
-            while (c != null)
-            {
-                if (roleClassNameMatches(c.getName()))
-                {
-                    groups.add(principal.getName());
-                    break;
-                }
-
-                boolean added = false;
-                for (Class<?> ci : c.getInterfaces())
-                {
-                    if (roleClassNameMatches(ci.getName()))
-                    {
-                        groups.add(principal.getName());
-                        added = true;
-                        break;
-                    }
-                }
-
-                if (!added)
-                {
-                    c = c.getSuperclass();
-                }
-                else
-                    break;
-            }
+            if (isRoleClass(principal.getClass(), Arrays.asList(getRoleClassNames())))
+                groups.add(principal.getName());
         }
 
         return groups.toArray(new String[groups.size()]);
     }
-
-    private boolean roleClassNameMatches(String classname)
+
+    /**
+     * Check whether the class, its superclasses or any interfaces they implement
+     * is one of the classes that represents a role.
+     * 
+     * @param clazz the class to check
+     * @param roleClassNames the list of classnames that represent roles
+     * @return true if the class is a role class
+     */
+    private static boolean isRoleClass(Class<?> clazz, List<String> roleClassNames)
     {
-        boolean result = false;
-        for (String roleClassName : getRoleClassNames())
+        Class<?> c = clazz;
+
+        //add the class, its interfaces and superclasses to the list to test
+        List<String> classnames = new ArrayList<>();
+        while (c != null)
         {
-            if (roleClassName.equals(classname))
-            {
-                result = true;
-                break;
-            }
+            classnames.add(c.getName());
+            Arrays.stream(c.getInterfaces()).map(Class::getName).forEach(classnames::add);
+            c = c.getSuperclass();
         }
-        return result;
+
+        return roleClassNames.stream().anyMatch(classnames::contains);
     }
 }

jetty-jaas/src/main/java/org/eclipse/jetty/jaas/PropertyUserStoreManager.java

@@ -0,0 +1,101 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.jaas;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+import org.eclipse.jetty.jaas.spi.PropertyFileLoginModule;
+import org.eclipse.jetty.security.PropertyUserStore;
+import org.eclipse.jetty.util.component.AbstractLifeCycle;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
+
+/**
+ * PropertyUserStoreManager
+ *
+ * Maintains a map of PropertyUserStores, keyed off the location of the property file containing
+ * the authentication and authorization information.
+ * 
+ * This class is used to enable the PropertyUserStores to be cached and shared. This is essential
+ * for the PropertyFileLoginModules, whose lifecycle is controlled by the JAAS api and instantiated
+ * afresh whenever a user needs to be authenticated. Without this class, every PropertyFileLoginModule
+ * instantiation would re-read and reload in all the user information just to authenticate a single user.
+ */
+public class PropertyUserStoreManager extends AbstractLifeCycle
+{
+    private static final Logger LOG = Log.getLogger(PropertyFileLoginModule.class);
+
+    /**
+     * Map of user authentication and authorization information loaded in from a property file.
+     * The map is keyed off the location of the file.
+     */
+    private Map<String, PropertyUserStore> _propertyUserStores; 
+
+    public PropertyUserStore getPropertyUserStore(String file)
+    {
+        synchronized (this)
+        {
+            if (_propertyUserStores == null)
+                return null;
+
+            return _propertyUserStores.get(file);
+        }
+    }
+
+    public PropertyUserStore addPropertyUserStore(String file, PropertyUserStore store)
+    {
+        synchronized (this)
+        {
+            Objects.requireNonNull(_propertyUserStores);
+            PropertyUserStore existing = _propertyUserStores.get(file);
+            if (existing != null)
+                return existing;
+
+            _propertyUserStores.put(file, store);
+            return store;
+        }
+    }
+
+    @Override
+    protected void doStart() throws Exception
+    {
+        _propertyUserStores = new HashMap<String, PropertyUserStore>();
+        super.doStart();
+    }
+
+    @Override
+    protected void doStop() throws Exception
+    {
+        for (Map.Entry<String,PropertyUserStore> entry: _propertyUserStores.entrySet())
+        {
+            try
+            {
+                entry.getValue().stop();
+            }
+            catch (Exception e)
+            {
+                LOG.warn("Error stopping PropertyUserStore at {}", entry.getKey(), e);
+            }
+        }
+        _propertyUserStores = null;
+        super.doStop();
+    }
+}

jetty-jaas/src/main/java/org/eclipse/jetty/jaas/callback/DefaultCallbackHandler.java

@@ -26,7 +26,6 @@
 import javax.security.auth.callback.UnsupportedCallbackException;
 
 import org.eclipse.jetty.server.Request;
-import org.eclipse.jetty.util.security.Password;
 
 /**
  * DefaultCallbackHandler
@@ -47,38 +46,34 @@ public void setRequest(Request request)
     public void handle(Callback[] callbacks)
         throws IOException, UnsupportedCallbackException
     {
-        for (int i = 0; i < callbacks.length; i++)
+        for (Callback callback : callbacks)
         {
-            if (callbacks[i] instanceof NameCallback)
+            if (callback instanceof NameCallback)
             {
-                ((NameCallback)callbacks[i]).setName(getUserName());
+                ((NameCallback)callback).setName(getUserName());
             }
-            else if (callbacks[i] instanceof ObjectCallback)
+            else if (callback instanceof ObjectCallback)
             {
-                ((ObjectCallback)callbacks[i]).setObject(getCredential());
+                ((ObjectCallback)callback).setObject(getCredential());
             }
-            else if (callbacks[i] instanceof PasswordCallback)
+            else if (callback instanceof PasswordCallback)
             {
-                if (getCredential() instanceof Password)
-                    ((PasswordCallback)callbacks[i]).setPassword(getCredential().toString().toCharArray());
-                else if (getCredential() instanceof String)
-                {
-                    ((PasswordCallback)callbacks[i]).setPassword(((String)getCredential()).toCharArray());
-                }
-                else
-                    throw new UnsupportedCallbackException(callbacks[i], "User supplied credentials cannot be converted to char[] for PasswordCallback: try using an ObjectCallback instead");
+                ((PasswordCallback)callback).setPassword(getCredential().toString().toCharArray());
             }
-            else if (callbacks[i] instanceof RequestParameterCallback)
+            else if (callback instanceof RequestParameterCallback)
             {
-                RequestParameterCallback callback = (RequestParameterCallback)callbacks[i];
-                callback.setParameterValues(Arrays.asList(_request.getParameterValues(callback.getParameterName())));
+                if (_request != null)
+                {
+                    RequestParameterCallback rpc = (RequestParameterCallback)callback;
+                    rpc.setParameterValues(Arrays.asList(_request.getParameterValues(rpc.getParameterName())));
+                }
             }
-            else if (callbacks[i] instanceof ServletRequestCallback)
+            else if (callback instanceof ServletRequestCallback)
             {
-                ((ServletRequestCallback)callbacks[i]).setRequest(_request);
+                ((ServletRequestCallback)callback).setRequest(_request);
             }
             else
-                throw new UnsupportedCallbackException(callbacks[i]);
+                throw new UnsupportedCallbackException(callback);
         }
     }
 }