Commit 931f1c82 authored by Patrick Dowler's avatar Patrick Dowler
Browse files

improve ldap query with minimal scope, remove redundant filters; fix...

improve ldap query with minimal scope, remove redundant filters; fix deleteUser to really delete pending users
parent 827c01f7
Loading
Loading
Loading
Loading
+1 −7
Original line number Diff line number Diff line
@@ -165,7 +165,7 @@ public abstract class LdapDAO
    {
        if (subjDN == null)
        {
            Subject callerSubject = getSubject();
            Subject callerSubject = Subject.getSubject(AccessController.getContext());
            if (callerSubject == null)
            {
                throw new AccessControlException("Caller not authenticated.");
@@ -238,10 +238,4 @@ public abstract class LdapDAO

        throw new RuntimeException("Ldap error (" + code.getName() + ")");
    }

    protected Subject getSubject()
    {
        return Subject.getSubject(AccessController.getContext());
    }

}
+90 −78
Original line number Diff line number Diff line
@@ -102,6 +102,7 @@ import com.unboundid.ldap.sdk.BindRequest;
import com.unboundid.ldap.sdk.BindResult;
import com.unboundid.ldap.sdk.Control;
import com.unboundid.ldap.sdk.DN;
import com.unboundid.ldap.sdk.DeleteRequest;
import com.unboundid.ldap.sdk.Filter;
import com.unboundid.ldap.sdk.LDAPException;
import com.unboundid.ldap.sdk.LDAPResult;
@@ -120,7 +121,11 @@ import com.unboundid.ldap.sdk.extensions.PasswordModifyExtendedRequest;
import com.unboundid.ldap.sdk.extensions.PasswordModifyExtendedResult;



/**
 * 
 * @author pdowler
 * @param <T> 
 */
public class LdapUserDAO<T extends Principal> extends LdapDAO
{
    private static final Logger logger = Logger.getLogger(LdapUserDAO.class);
@@ -154,7 +159,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
            LDAP_FIRST_NAME, LDAP_LAST_NAME, LDAP_ADDRESS, LDAP_CITY,
            LDAP_COUNTRY, LDAP_EMAIL, LDAP_INSTITUTE
    };
    private String[] memberAttribs = new String[]
    private String[] firstLastAttribs = new String[]
    {
            LDAP_FIRST_NAME, LDAP_LAST_NAME
    };
@@ -179,11 +184,11 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
                         userAttribs.length);
        userAttribs = tmp;

        tmp = new String[memberAttribs.length + princs.length];
        tmp = new String[firstLastAttribs.length + princs.length];
        System.arraycopy(princs, 0, tmp, 0, princs.length);
        System.arraycopy(memberAttribs, 0, tmp, princs.length,
                         memberAttribs.length);
        memberAttribs = tmp;
        System.arraycopy(firstLastAttribs, 0, tmp, princs.length,
                         firstLastAttribs.length);
        firstLastAttribs = tmp;
    }

    /**
@@ -243,7 +248,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO

            SearchRequest searchRequest =
                    new SearchRequest(config.getUsersDN(),
                                      SearchScope.SUB, filter, attributes);
                                      SearchScope.ONE, filter, attributes);

            SearchResult searchResult = null;
            try
@@ -311,6 +316,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
    public void addPendingUser(final UserRequest<T> userRequest)
            throws TransientException, UserAlreadyExistsException
    {
        // check current users
        try
        {
            getUser(userRequest.getUser().getUserID(), config.getUsersDN(), false);
@@ -318,8 +324,9 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
                                 " found in " + config.getUsersDN();
            throw new UserAlreadyExistsException(error);
        }
        catch (UserNotFoundException e1)
        {
        catch (UserNotFoundException ok) { }
        
        // check pending users
        try
        {
            getUser(userRequest.getUser().getUserID(), config.getUserRequestsDN(), false);
@@ -327,8 +334,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
                " found in " + config.getUserRequestsDN();
            throw new UserAlreadyExistsException(error);
        }
            catch (UserNotFoundException e2) {}
        }
        catch (UserNotFoundException ok) { }

        addUser(userRequest, config.getUserRequestsDN());
    }
@@ -473,7 +479,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
            logger.debug("search filter: " + filter);

            SearchRequest searchRequest = 
                    new SearchRequest(usersDN, SearchScope.SUB, filter, userAttribs);
                    new SearchRequest(usersDN, SearchScope.ONE, filter, userAttribs);

            if (proxy)
            {
@@ -497,25 +503,24 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
        }

        User<T> user = new User<T>(userID);
        user.getIdentities().add(new HttpPrincipal(
                searchResult.getAttributeValue(
                       userLdapAttrib.get(HttpPrincipal.class))));
        String username = searchResult.getAttributeValue(userLdapAttrib.get(HttpPrincipal.class));
        logger.debug("username: " + username);
        user.getIdentities().add(new HttpPrincipal(username));

        Integer numericID = searchResult.getAttributeValueAsInteger(
            userLdapAttrib.get(NumericPrincipal.class));
        logger.debug("Numeric id is: " + numericID);
        Integer numericID = searchResult.getAttributeValueAsInteger(userLdapAttrib.get(NumericPrincipal.class));
        logger.debug("Numeric id: " + numericID);
        if (numericID == null)
        {
            // If the numeric ID does not return it means the user
            // does not have permission
            throw new AccessControlException("Permission denied");
        }
        NumericPrincipal numericPrincipal = new NumericPrincipal(numericID);
        user.getIdentities().add(numericPrincipal);
        user.getIdentities().add(new NumericPrincipal(numericID));

        String x500str = searchResult.getAttributeValue(userLdapAttrib.get(X500Principal.class));
        logger.debug("x500principal: " + x500str);
        user.getIdentities().add(new X500Principal(x500str));

        user.getIdentities().add(new X500Principal(
                searchResult.getAttributeValue(
                        userLdapAttrib.get(X500Principal.class))));
        String fname = searchResult.getAttributeValue(LDAP_FIRST_NAME);
        String lname = searchResult.getAttributeValue(LDAP_LAST_NAME);
        PersonalDetails personaDetails = new PersonalDetails(fname, lname);
@@ -688,8 +693,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
        }
        catch (LDAPException e)
        {
            e.printStackTrace();
            logger.debug("Modify Exception: " + e, e);
            logger.debug("Modify Exception", e);
            LdapDAO.checkLdapResult(e.getResultCode());
        }
        try
@@ -748,8 +752,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
        }
        catch (LDAPException e)
        {
            e.printStackTrace();
            logger.debug("Modify Exception: " + e, e);
            logger.debug("Modify Exception", e);
            LdapDAO.checkLdapResult(e.getResultCode());
        }
        try
@@ -816,7 +819,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
            throws UserNotFoundException, TransientException,
                   AccessControlException
    {
        deleteUser(userID, config.getUsersDN());
        deleteUser(userID, config.getUsersDN(), true);
    }

    /**
@@ -831,16 +834,18 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
        throws UserNotFoundException, TransientException,
        AccessControlException
    {
        deleteUser(userID, config.getUserRequestsDN());
        deleteUser(userID, config.getUserRequestsDN(), false);
    }

    private void deleteUser(final T userID, final String usersDN)
    private void deleteUser(final T userID, final String usersDN, boolean markDelete)
        throws UserNotFoundException, AccessControlException, TransientException
    {
        getUser(userID, usersDN);
        try
        {
            DN userDN = getUserDN(userID.getName(), usersDN);
            if (markDelete)
            {
                List<Modification> modifs = new ArrayList<Modification>();
                modifs.add(new Modification(ModificationType.ADD, LDAP_NSACCOUNTLOCK, "true"));

@@ -852,6 +857,17 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
                LDAPResult result = getConnection().modify(modifyRequest);
                LdapDAO.checkLdapResult(result.getResultCode());
            }
            else // real delete
            {
                DeleteRequest delRequest = new DeleteRequest(userDN);
                delRequest.addControl(
                    new ProxiedAuthorizationV2RequestControl(
                        "dn:" + getSubjectDN().toNormalizedString()));
                
                LDAPResult result = getConnection().delete(delRequest);
                LdapDAO.checkLdapResult(result.getResultCode());
            }
        }
        catch (LDAPException e1)
        {
            logger.debug("Delete Exception: " + e1, e1);
@@ -892,23 +908,22 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
                        "Unsupported principal type " + userID.getClass());
            }

            User<T> user = getUser(userID);
            Filter filter = Filter.createANDFilter(
                    Filter.createEqualityFilter(searchField,
                                                user.getUserID().getName()),
                    Filter.createPresenceFilter(LDAP_MEMBEROF));
            //User<T> user = getUser(userID);
            
            SearchRequest searchRequest =
                    new SearchRequest(config.getUsersDN(), SearchScope.SUB,
                                      filter, LDAP_MEMBEROF);
            //Filter filter = Filter.createANDFilter(
            //        Filter.createEqualityFilter(searchField,
            //                                    user.getUserID().getName()),
            //        Filter.createPresenceFilter(LDAP_MEMBEROF));
            
            Filter filter = Filter.createEqualityFilter(searchField, userID.getName());
            SearchRequest searchRequest = new SearchRequest(
                    config.getUsersDN(), SearchScope.ONE, filter, LDAP_MEMBEROF);

            searchRequest.addControl(
                    new ProxiedAuthorizationV2RequestControl("dn:" +
                                                             getSubjectDN()
                                                                     .toNormalizedString()));
                            getSubjectDN().toNormalizedString()));

            SearchResultEntry searchResult =
                    getConnection().searchForEntry(searchRequest);
            SearchResultEntry searchResult = getConnection().searchForEntry(searchRequest);

            DN parentDN;
            if (isAdmin)
@@ -922,8 +937,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO

            if (searchResult != null)
            {
                String[] members = searchResult
                        .getAttributeValues(LDAP_MEMBEROF);
                String[] members = searchResult.getAttributeValues(LDAP_MEMBEROF);
                if (members != null)
                {
                    for (String member : members)
@@ -974,16 +988,14 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
                    Filter.createEqualityFilter(LDAP_MEMBEROF, groupID));

            SearchRequest searchRequest =
                    new SearchRequest(config.getUsersDN(), SearchScope.SUB,
                    new SearchRequest(config.getUsersDN(), SearchScope.ONE,
                                      filter, LDAP_COMMON_NAME);

            searchRequest.addControl(
                    new ProxiedAuthorizationV2RequestControl("dn:" +
                                                             getSubjectDN()
                                                                     .toNormalizedString()));
                            getSubjectDN().toNormalizedString()));

            SearchResultEntry searchResults =
                    getConnection().searchForEntry(searchRequest);
            SearchResultEntry searchResults = getConnection().searchForEntry(searchRequest);

            return (searchResults != null);
        }
@@ -996,7 +1008,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO

    /**
     * Returns a member user identified by the X500Principal only. The
     * returned object has the fields required by the GMS.
     * returned object has the fields required by the LdapGroupDAO.
     * Note that this method binds as a proxy user and not as the
     * subject.
     *
@@ -1014,7 +1026,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO

        SearchRequest searchRequest =
                new SearchRequest(config.getUsersDN(), SearchScope.ONE,
                                  filter, memberAttribs);
                                  filter, firstLastAttribs);

        SearchResultEntry searchResult =
                getConnection().searchForEntry(searchRequest);
@@ -1069,7 +1081,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
        try
        {
            SearchRequest searchRequest = new SearchRequest(
                config.getUsersDN(), SearchScope.SUB, filter, LDAP_ENTRYDN);
                config.getUsersDN(), SearchScope.ONE, filter, LDAP_ENTRYDN);
            searchResult = getConnection().searchForEntry(searchRequest);
        }
        catch (LDAPException e)
@@ -1147,7 +1159,7 @@ public class LdapUserDAO<T extends Principal> extends LdapDAO
     * Method to return a randomly generated user numeric ID. The default
     * implementation returns a value between 10000 and Integer.MAX_VALUE.
     * Services that support a different mechanism for generating numeric
     * IDs overide this method.
     * IDs override this method.
     * @return
     */
    protected int genNextNumericId()
+23 −11
Original line number Diff line number Diff line
@@ -74,6 +74,8 @@ import ca.nrc.cadc.auth.NumericPrincipal;
import ca.nrc.cadc.util.Log4jInit;
import com.unboundid.ldap.sdk.DN;
import com.unboundid.ldap.sdk.LDAPConnection;
import com.unboundid.ldap.sdk.LDAPException;
import java.security.PrivilegedAction;
import org.apache.log4j.Level;
import org.junit.BeforeClass;
import org.junit.Test;
@@ -81,6 +83,7 @@ import org.junit.Test;
import javax.security.auth.Subject;
import javax.security.auth.x500.X500Principal;
import java.security.PrivilegedExceptionAction;
import org.junit.Assert;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
@@ -179,21 +182,30 @@ public class LdapDAOTest extends AbstractLdapDAOTest
        DN expected = new DN("uid=foo,ou=bar,dc=net");
        final DNPrincipal dnPrincipal = new DNPrincipal(expected.toNormalizedString());

        LdapConfig config = LdapConfig.getLdapConfig("LdapConfig.test.properties");
        LdapDAO ldapDAO = new LdapDAO(config)
        {
            @Override
            protected Subject getSubject()
            {
        Subject subject = new Subject();
        subject.getPrincipals().add(new HttpPrincipal("foo"));
        subject.getPrincipals().add(new X500Principal("uid=foo,o=bar"));
        subject.getPrincipals().add(dnPrincipal);
                return subject;
                
        LdapConfig config = LdapConfig.getLdapConfig("LdapConfig.test.properties");
        final LdapDAO ldapDAO = new LdapDAO(config) { }; // abstract
        
        DN actual = Subject.doAs(subject, new PrivilegedAction<DN>()
        {
            public DN run()
            {
                try
                {
                    return ldapDAO.getSubjectDN();
                }
                catch(LDAPException ex)
                {
                    Assert.fail("getSubjectDN threw " + ex);
                }
        };
                return null;
            }
        } );
        
        DN actual = ldapDAO.getSubjectDN();
        assertNotNull("DN is null", actual);
        assertEquals("DN's do not match", expected.toNormalizedString(), actual.toNormalizedString());
    }