Re-factored and fixed unit test

projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/GroupPersistence.java

@@ -68,16 +68,16 @@
  */
 package ca.nrc.cadc.ac.server;
 
+import java.security.AccessControlException;
+import java.security.Principal;
+import java.util.Collection;
+
 import ca.nrc.cadc.ac.Group;
 import ca.nrc.cadc.ac.GroupAlreadyExistsException;
 import ca.nrc.cadc.ac.GroupNotFoundException;
-import ca.nrc.cadc.ac.IdentityType;
 import ca.nrc.cadc.ac.Role;
 import ca.nrc.cadc.ac.UserNotFoundException;
 import ca.nrc.cadc.net.TransientException;
-import java.security.AccessControlException;
-import java.security.Principal;
-import java.util.Collection;
 
 public abstract interface GroupPersistence<T extends Principal>
 {

projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/PluginFactory.java

@@ -125,8 +125,8 @@ public class PluginFactory
         {
             try
             {
-                Class c = Class.forName(cname);
-                ret = (GroupPersistence) c.newInstance();
+                Class<?> c = Class.forName(cname);
+                ret = (GroupPersistence<T>) c.newInstance();
             }
             catch (Exception ex)
             {
@@ -149,8 +149,8 @@ public class PluginFactory
         {
             try
             {
-                Class c = Class.forName(cname);
-                ret = (UserPersistence) c.newInstance();
+                Class<?> c = Class.forName(cname);
+                ret = (UserPersistence<T>) c.newInstance();
             }
             catch (Exception ex)
             {

projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/ldap/LdapDAO.java

@@ -68,22 +68,26 @@
  */
 package ca.nrc.cadc.ac.server.ldap;
 
+import java.security.AccessControlException;
+import java.security.AccessController;
+import java.security.Principal;
+import java.util.Set;
+
+import javax.security.auth.Subject;
+import javax.security.auth.x500.X500Principal;
+
 import ca.nrc.cadc.auth.HttpPrincipal;
 import ca.nrc.cadc.auth.NumericPrincipal;
 import ca.nrc.cadc.auth.OpenIdPrincipal;
+import ca.nrc.cadc.net.TransientException;
+
 import com.unboundid.ldap.sdk.DN;
 import com.unboundid.ldap.sdk.LDAPConnection;
 import com.unboundid.ldap.sdk.LDAPException;
+import com.unboundid.ldap.sdk.ResultCode;
 import com.unboundid.ldap.sdk.SearchResult;
 import com.unboundid.ldap.sdk.SearchResultEntry;
 import com.unboundid.ldap.sdk.SearchScope;
-import java.security.AccessControlException;
-import java.security.AccessController;
-import java.security.Principal;
-import java.util.List;
-import java.util.Set;
-import javax.security.auth.Subject;
-import javax.security.auth.x500.X500Principal;
 
 public abstract class LdapDAO
 {
@@ -116,7 +120,15 @@ public abstract class LdapDAO
         {
             conn = new LDAPConnection(config.getServer(), config.getPort());
             conn.bind(config.getAdminUserDN(), config.getAdminPasswd());
+        }
 
+        return conn;
+    }
+
+    protected DN getSubjectDN() throws LDAPException
+    {
+        if (subjDN == null)
+        {
             Subject callerSubject = 
                     Subject.getSubject(AccessController.getContext());
             if (callerSubject == null)
@@ -161,7 +173,7 @@ public abstract class LdapDAO
             }
 
             SearchResult searchResult = 
-                    conn.search(config.getUsersDN(), SearchScope.ONE, 
+                    getConnection().search(config.getUsersDN(), SearchScope.ONE, 
                                 ldapField, new String[] {"entrydn"});
 
             if (searchResult.getEntryCount() < 1)
@@ -173,17 +185,50 @@ public abstract class LdapDAO
             subjDN = ((SearchResultEntry) searchResult.getSearchEntries()
                     .get(0)).getAttributeValueAsDN("entrydn");
         }
-
-        return conn;
+        return subjDN;
     }
-
-    protected DN getSubjectDN() throws LDAPException
+    
+    /**
+     * Checks the Ldap result code, and if the result is not SUCCESS,
+     * throws an appropriate exception. This is the place to decide on 
+     * mapping between ldap errors and exception types
+     * @param code
+     * @param errorMsg
+     * @throws TransientException 
+     */
+    protected static void checkLdapResult(ResultCode code, String errorMsg) 
+            throws TransientException
     {
-        if (subjDN == null)
+        String msg = "";
+        if (errorMsg != null)
         {
-            getConnection();
+            msg = "(" + errorMsg + ")";
+        }
+        if (code == ResultCode.INSUFFICIENT_ACCESS_RIGHTS)
+        {
+            throw new AccessControlException("Not authorized " + msg);
+        }
+        else if (code == ResultCode.INVALID_CREDENTIALS)
+        {
+            throw new AccessControlException("Invalid credentials " + msg);
+        }
+        else if (code == ResultCode.SUCCESS)
+        {
+            // all good. nothing to do
+        }
+        else if (code == ResultCode.PARAM_ERROR)
+        {
+            throw new IllegalArgumentException("Error in Ldap parameters " + msg);
+        }
+        else if (code == ResultCode.BUSY ||
+                 code == ResultCode.CONNECT_ERROR )
+        {
+            throw new TransientException("Connection problems " + msg );
+        }
+        else
+        {
+            throw new RuntimeException("Ldap error" + msg);
         }
-        return subjDN;
     }
 
 }

projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/ldap/LdapUserDAO.java

@@ -84,7 +84,6 @@ import ca.nrc.cadc.ac.PersonalDetails;
 import ca.nrc.cadc.ac.User;
 import ca.nrc.cadc.ac.UserNotFoundException;
 import ca.nrc.cadc.auth.HttpPrincipal;
-import ca.nrc.cadc.auth.NumericPrincipal;
 import ca.nrc.cadc.net.TransientException;
 
 import com.unboundid.ldap.sdk.CompareRequest;
@@ -167,7 +166,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
         }
         catch (LDAPException e)
         {
-            e.printStackTrace();
+            LdapDAO.checkLdapResult(e.getResultCode(), e.getDiagnosticMessage());
         }
 
         if (searchResult == null)
@@ -202,6 +201,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
     public Collection<Group> getUserGroups(T userID)
         throws UserNotFoundException, TransientException, AccessControlException
     {
+        Collection<Group> groups = new HashSet<Group>();
         try
         {
             String searchField = (String) userLdapAttrib.get(userID.getClass());
@@ -227,8 +227,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
 
             SearchResultEntry searchResult = 
                     getConnection().searchForEntry(searchRequest);
-            
-            Collection<Group> groups = new HashSet<Group>();
+                       
             if (searchResult != null)
             {
                 String[] members = 
@@ -248,16 +247,13 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
                         catch (IllegalArgumentException ignore) { }
                     }
                 }
-            }
-            return groups;
+            }           
         }
         catch (LDAPException e)
         {
-            // TODO check which LDAP exceptions are transient and which
-            // ones are
-            // access control
-            throw new TransientException("Error getting user groups", e);
+            LdapDAO.checkLdapResult(e.getResultCode(), e.getDiagnosticMessage());
         }
+        return groups;
     }
     
     /**
@@ -310,11 +306,9 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
         }
         catch (LDAPException e1)
         {
-            // TODO check which LDAP exceptions are transient and which
-            // ones are
-            // access control
-            throw new TransientException("Error getting the user", e1);
+            LdapDAO.checkLdapResult(e1.getResultCode(), e1.getDiagnosticMessage());
         }
+        return false;
     }
     
     public boolean isMember(T userID, String groupID)
@@ -347,11 +341,9 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
         }
         catch (LDAPException e)
         {
-            // TODO check which LDAP exceptions are transient and which
-            // ones are
-            // access control
-            throw new TransientException("Error getting the user", e);
+            LdapDAO.checkLdapResult(e.getResultCode(), e.getDiagnosticMessage());
         }
+        return false;
     }
     
     /**
@@ -401,7 +393,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
     
 
     DN getUserDN(User<? extends Principal> user)
-        throws LDAPException, UserNotFoundException
+        throws UserNotFoundException, TransientException
     {
         String searchField = (String) userLdapAttrib.get(user.getUserID().getClass());
         if (searchField == null)
@@ -413,17 +405,22 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
         searchField = "(" + searchField + "=" + 
                       user.getUserID().getName() + ")";
 
-        SearchRequest searchRequest = 
-                new SearchRequest(this.config.getUsersDN(), SearchScope.SUB, 
-                                 searchField, new String[] {"entrydn"});
+        SearchResultEntry searchResult = null;
+        try
+        {
+            SearchRequest searchRequest = new SearchRequest(this.config.getUsersDN(), SearchScope.SUB, 
+                             searchField, new String[] {"entrydn"});
         
-//        searchRequest.addControl(
-//                    new ProxiedAuthorizationV2RequestControl("dn:" + 
-//                            getSubjectDN().toNormalizedString()));
 
-        SearchResultEntry searchResult = 
+            searchResult = 
                 getConnection().searchForEntry(searchRequest);
 
+        } catch (LDAPException e)
+        {
+            LdapDAO.checkLdapResult(e.getResultCode(), e.getDiagnosticMessage());
+        }
+        
+
         if (searchResult == null)
         {
             String msg = "User not found " + user.getUserID().toString();

projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java

@@ -89,7 +89,8 @@ public class LdapGroupDAOTest
     static User<X500Principal> unknownUser;
     static User<X500Principal> adminUser;
     
-    static Subject authSubject;
+    static Subject authSubject1;
+    static Subject authSubject2;
     static Subject anonSubject;
     
     static LdapConfig config;
@@ -110,8 +111,11 @@ public class LdapGroupDAOTest
         unknownUser = new User<X500Principal>(unknownPrincipal);
         adminUser = new User<X500Principal>(adminPrincipal);
         
-        authSubject = new Subject();
-        authSubject.getPrincipals().add(daoTestUser1.getUserID());
+        authSubject1 = new Subject();
+        authSubject1.getPrincipals().add(daoTestUser1.getUserID());
+        
+        authSubject2 = new Subject();
+        authSubject2.getPrincipals().add(daoTestUser2.getUserID());
         
         anonSubject = new Subject();
         anonSubject.getPrincipals().add(unknownUser.getUserID());
@@ -134,7 +138,7 @@ public class LdapGroupDAOTest
     public void testOneGroup() throws Exception
     {
         // do everything as owner
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -218,7 +222,7 @@ public class LdapGroupDAOTest
     public void testSearchOwnerGroups() throws Exception
     {
         // do everything as owner
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -263,7 +267,7 @@ public class LdapGroupDAOTest
     public void testSearchMemberGroups() throws Exception
     {
         // do everything as owner
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -310,7 +314,7 @@ public class LdapGroupDAOTest
     public void testSearchRWGroups() throws Exception
     {
         // do everything as owner
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -371,7 +375,7 @@ public class LdapGroupDAOTest
             }
         });
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -405,7 +409,7 @@ public class LdapGroupDAOTest
     {
         final String groupID = getGroupID();
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -437,7 +441,7 @@ public class LdapGroupDAOTest
             }
         });
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {   
@@ -452,7 +456,7 @@ public class LdapGroupDAOTest
     {        
         final String groupID = getGroupID();
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -484,7 +488,7 @@ public class LdapGroupDAOTest
             }
         });
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {               
@@ -499,7 +503,7 @@ public class LdapGroupDAOTest
     {
         final String groupID = getGroupID();
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -531,7 +535,7 @@ public class LdapGroupDAOTest
             }
         });
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {                
@@ -546,7 +550,7 @@ public class LdapGroupDAOTest
     {        
         final String groupID = getGroupID();
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {
@@ -589,7 +593,7 @@ public class LdapGroupDAOTest
             }
         });
         
-        Subject.doAs(authSubject, new PrivilegedExceptionAction<Object>()
+        Subject.doAs(authSubject1, new PrivilegedExceptionAction<Object>()
         {
             public Object run() throws Exception
             {               
@@ -597,6 +601,33 @@ public class LdapGroupDAOTest
                 return null;
             }
         });
+        
+        
+        // change the user
+        
+        Subject.doAs(authSubject2, new PrivilegedExceptionAction<Object>()
+        {
+            
+            public Object run() throws Exception
+            {
+
+                
+                try
+                {
+                    Group group = getGroupDAO().getGroup(groupID);
+                    assertTrue(group == null);
+                    
+                    fail("searchGroups with unknown user should throw " + 
+                         "GroupNotFoundException");
+                }
+                catch (GroupNotFoundException ignore) 
+                {
+
+                }
+
+                return null;
+            }
+        });
     }
 
     private void assertGroupsEqual(Group gr1, Group gr2)