Refactor jaas login sequence to only fetch role data if user is authenticated according to that module.

5 files changed, 158 insertions, 39 deletions

diff --git a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/AbstractDatabaseLoginModule.java b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/AbstractDatabaseLoginModule.java
index 1a2e141bca..6fd2660e6b 100644
--- a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/AbstractDatabaseLoginModule.java
+++ b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/AbstractDatabaseLoginModule.java
@@ -21,7 +21,6 @@ package org.eclipse.jetty.jaas.spi;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
-import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -59,6 +58,24 @@ public abstract class AbstractDatabaseLoginModule extends AbstractLoginModule
      * @throws Exception if unable to get the connection
      */
     public abstract Connection getConnection () throws Exception;
+    
+    
+    public class JDBCUserInfo extends UserInfo
+    {
+        public JDBCUserInfo (String userName, Credential credential)
+        {
+            super(userName, credential);
+        }
+        
+        
+        
+        @Override
+        public List<String> doFetchRoles ()
+        throws Exception
+        {
+           return getRoles(getUserName());
+        }
+    }
 
 
 
@@ -92,8 +109,22 @@ public abstract class AbstractDatabaseLoginModule extends AbstractLoginModule
                 return null;
             }
 
+          
+
+            return new JDBCUserInfo (userName, Credential.getCredential(dbCredential));
+        }
+    }
+    
+    
+    public List<String>  getRoles (String userName)
+    throws Exception
+    {
+        List<String> roles = new ArrayList<String>();
+        
+        try (Connection connection = getConnection())
+        {
             //query for role names
-            List<String> roles = new ArrayList<String>();
+
             try (PreparedStatement statement = connection.prepareStatement (rolesQuery))
             {
                 statement.setString (1, userName);
@@ -106,10 +137,13 @@ public abstract class AbstractDatabaseLoginModule extends AbstractLoginModule
                     }
                 }
             }
-
-            return new UserInfo (userName, Credential.getCredential(dbCredential), roles);
+          
         }
+
+        return roles;
     }
+    
+    
 
 
     public void initialize(Subject subject,
diff --git a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/AbstractLoginModule.java b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/AbstractLoginModule.java
index 67b2f5484b..0bcdd54c17 100644
--- a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/AbstractLoginModule.java
+++ b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/AbstractLoginModule.java
@@ -54,6 +54,12 @@ public abstract class AbstractLoginModule implements LoginModule
     private JAASUserInfo currentUser;
     private Subject subject;
 
+    /**
+     * JAASUserInfo
+     *
+     * This class unites the UserInfo data with jaas concepts
+     * such as Subject and Principals
+     */
     public class JAASUserInfo
     {
         private UserInfo user;
@@ -62,7 +68,8 @@ public abstract class AbstractLoginModule implements LoginModule
 
         public JAASUserInfo (UserInfo u)
         {
-            setUserInfo(u);
+            this.user = u;
+            this.principal = new JAASPrincipal(u.getUserName());
         }
 
         public String getUserName ()
@@ -75,19 +82,7 @@ public abstract class AbstractLoginModule implements LoginModule
             return this.principal;
         }
 
-        public void setUserInfo (UserInfo u)
-        {
-            this.user = u;
-            this.principal = new JAASPrincipal(u.getUserName());
-            this.roles = new ArrayList<JAASRole>();
-            if (u.getRoleNames() != null)
-            {
-                Iterator<String> itor = u.getRoleNames().iterator();
-                while (itor.hasNext())
-                    this.roles.add(new JAASRole((String)itor.next()));
-            }
-        }
-
+ 
         public void setJAASInfo (Subject subject)
         {
             subject.getPrincipals().add(this.principal);
@@ -106,6 +101,18 @@ public abstract class AbstractLoginModule implements LoginModule
         {
             return this.user.checkCredential(suppliedCredential);
         }
+        
+        public void fetchRoles() throws Exception
+        {
+            this.user.fetchRoles();
+            this.roles = new ArrayList<JAASRole>();
+            if (this.user.getRoleNames() != null)
+            {
+                Iterator<String> itor = this.user.getRoleNames().iterator();
+                while (itor.hasNext())
+                    this.roles.add(new JAASRole((String)itor.next()));
+            }
+        }
     }
 
     public Subject getSubject ()
@@ -174,7 +181,6 @@ public abstract class AbstractLoginModule implements LoginModule
      */
     public boolean commit() throws LoginException
     {
-
         if (!isAuthenticated())
         {
             currentUser = null;
@@ -252,7 +258,10 @@ public abstract class AbstractLoginModule implements LoginModule
             setAuthenticated(currentUser.checkCredential(webCredential));
           
             if (isAuthenticated())
+            {
+                currentUser.fetchRoles();
                 return true;
+            }
             else
                 throw new FailedLoginException();
         }
diff --git a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/LdapLoginModule.java b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/LdapLoginModule.java
index c66ff431fd..73ccb914dd 100644
--- a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/LdapLoginModule.java
+++ b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/LdapLoginModule.java
@@ -176,6 +176,28 @@ public class LdapLoginModule extends AbstractLoginModule
 
     private DirContext _rootContext;
 
+    
+    public class LDAPUserInfo extends UserInfo
+    {
+
+        /**
+         * @param userName
+         * @param credential
+         */
+        public LDAPUserInfo(String userName, Credential credential)
+        {
+            super(userName, credential);
+        }
+
+        @Override
+        public List<String> doFetchRoles() throws Exception
+        {
+            return getUserRoles(_rootContext, getUserName());
+        }
+        
+    }
+    
+    
     /**
      * get the available information about the user
      * <p>
@@ -199,9 +221,7 @@ public class LdapLoginModule extends AbstractLoginModule
 
         pwdCredential = convertCredentialLdapToJetty(pwdCredential);
         Credential credential = Credential.getCredential(pwdCredential);
-        List<String> roles = getUserRoles(_rootContext, username);
-
-        return new UserInfo(username, credential, roles);
+        return new LDAPUserInfo(username, credential);
     }
 
     protected String doRFC2254Encoding(String inputString)
@@ -411,12 +431,17 @@ public class LdapLoginModule extends AbstractLoginModule
 
             setCurrentUser(new JAASUserInfo(userInfo));
 
+            boolean authed = false;            
             if (webCredential instanceof String)
-            {
-                return credentialLogin(Credential.getCredential((String) webCredential));
-            }
-
-            return credentialLogin(webCredential);
+                authed = credentialLogin(Credential.getCredential((String) webCredential));
+            else
+                authed = credentialLogin(webCredential);
+            
+            //only fetch roles if authenticated
+            if (authed)
+                getCurrentUser().fetchRoles();
+            
+            return authed;
         }
         catch (UnsupportedCallbackException e)
         {
@@ -496,16 +521,18 @@ public class LdapLoginModule extends AbstractLoginModule
 
         String filter = "(&(objectClass={0})({1}={2}))";
 
-        LOG.info("Searching for users with filter: \'" + filter + "\'" + " from base dn: " + _userBaseDn);
+        if (LOG.isDebugEnabled())
+            LOG.debug("Searching for users with filter: \'" + filter + "\'" + " from base dn: " + _userBaseDn);
 
         Object[] filterArguments = new Object[]{
-            _userObjectClass,
-            _userIdAttribute,
-            username
+                                                _userObjectClass,
+                                                _userIdAttribute,
+                                                username
         };
         NamingEnumeration<SearchResult> results = _rootContext.search(_userBaseDn, filter, filterArguments, ctls);
 
-        LOG.info("Found user?: " + results.hasMoreElements());
+        if (LOG.isDebugEnabled())
+            LOG.debug("Found user?: " + results.hasMoreElements());
 
         if (!results.hasMoreElements())
         {
diff --git a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/PropertyFileLoginModule.java b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/PropertyFileLoginModule.java
index 097d943121..3560f2b57b 100644
--- a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/PropertyFileLoginModule.java
+++ b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/PropertyFileLoginModule.java
@@ -101,7 +101,7 @@ public class PropertyFileLoginModule extends AbstractLoginModule
     }
 
     /**
-     * Don't implement this as we want to pre-fetch all of the users.
+     * 
      *
      * @param userName the user name
      * @throws Exception if unable to get the user information
@@ -117,6 +117,8 @@ public class PropertyFileLoginModule extends AbstractLoginModule
         if (userIdentity==null)
             return null;
 
+        //TODO in future versions change the impl of PropertyUserStore so its not
+        //storing Subjects etc, just UserInfo
         Set<Principal> principals = userIdentity.getSubject().getPrincipals();
 
         List<String> roles = new ArrayList<String>();
diff --git a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/UserInfo.java b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/UserInfo.java
index c15e3ba185..c13061d1af 100644
--- a/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/UserInfo.java
+++ b/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/spi/UserInfo.java
@@ -19,6 +19,7 @@
 package org.eclipse.jetty.jaas.spi;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 
 import org.eclipse.jetty.util.security.Credential;
@@ -29,24 +30,70 @@ import org.eclipse.jetty.util.security.Credential;
  * This is the information read from the external source
  * about a user.
  * 
- * Can be cached by a UserInfoCache implementation
+ * Can be cached.
  */
 public class UserInfo
 {
     
     private String _userName;
     private Credential _credential;
-    private List<String> _roleNames;
+    protected List<String> _roleNames = new ArrayList<>();
+    protected boolean _rolesLoaded = false;
     
     
+    /**
+     * @param userName
+     * @param credential
+     * @param roleNames
+     */
     public UserInfo (String userName, Credential credential, List<String> roleNames)
     {
         _userName = userName;
         _credential = credential;
-        _roleNames = new ArrayList<String>();
         if (roleNames != null)
         {
-            _roleNames.addAll(roleNames);
+            synchronized (_roleNames)
+            {
+                _roleNames.addAll(roleNames);
+                _rolesLoaded = true;
+            }
+        }
+    }
+    
+    
+    /**
+     * @param userName
+     * @param credential
+     */
+    public UserInfo (String userName, Credential credential)
+    {
+        this (userName, credential, null);
+    }
+    
+    
+    
+    /**
+     * Should be overridden by subclasses to obtain
+     * role info
+     * 
+     * @return
+     * @throws Exception
+     */
+    public List<String> doFetchRoles ()
+    throws Exception
+    {
+        return Collections.emptyList();
+    }
+    
+    public void fetchRoles () throws Exception
+    {
+        synchronized (_roleNames)
+        {
+            if (!_rolesLoaded)
+            {
+                _roleNames.addAll(doFetchRoles());
+                _rolesLoaded = true;
+            }
         }
     }
     
@@ -56,8 +103,8 @@ public class UserInfo
     }
     
     public List<String> getRoleNames ()
-    {
-        return new ArrayList<String>(_roleNames);
+    {   
+        return Collections.unmodifiableList(_roleNames);
     }
     
     public boolean checkCredential (Object suppliedCredential)