SEC-513: Minor work on LDAP UserDetailsManager implementation.

core/src/main/java/org/springframework/security/ldap/DefaultLdapUsernameToDnMapper.java

@@ -3,6 +3,11 @@ package org.springframework.security.ldap;
 import org.springframework.ldap.core.DistinguishedName;
 
 /**
+ * This implementation appends a name component to the <tt>userDnBase</tt> context using the
+ * <tt>usernameAttributeName</tt> property. So if the <tt>uid</tt> attribute is used to store the username, and the
+ * base DN is <tt>cn=users</tt> and we are creating a new user called "sam", then the DN will be
+ * <tt>uid=sam,cn=users</tt>.
+ *
  * @author Luke Taylor
  * @version $Id$
  */
@@ -11,11 +16,6 @@ public class DefaultLdapUsernameToDnMapper implements LdapUsernameToDnMapper {
     private String usernameAttribute;
 
    /**
-    * This implementation appends a name component to the <tt>userDnBase</tt> context using the
-    * <tt>usernameAttributeName</tt> property. So if the <tt>uid</tt> attribute is used to store the username, and the
-    * base DN is <tt>cn=users</tt> and we are creating a new user called "sam", then the DN will be
-    * <tt>uid=sam,cn=users</tt>.
-    *
     * @param userDnBase the base name of the DN
     * @param usernameAttribute the attribute to append for the username component.
     */
@@ -24,6 +24,9 @@ public class DefaultLdapUsernameToDnMapper implements LdapUsernameToDnMapper {
         this.usernameAttribute = usernameAttribute;
     }
 
+    /**
+     * Assembles the Distinguished Name that should be used the given username.
+     */
     public DistinguishedName buildDn(String username) {
         DistinguishedName dn = new DistinguishedName(userDnBase);
 

core/src/main/java/org/springframework/security/userdetails/ldap/LdapUserDetailsManager.java

@@ -119,7 +119,7 @@ public class LdapUserDetailsManager implements UserDetailsManager {
         }
     };
 
-    private String[] attributesToRetrieve = null;
+    private String[] attributesToRetrieve;
 
     public LdapUserDetailsManager(ContextSource contextSource) {
         template = new LdapTemplate(contextSource);
@@ -186,6 +186,7 @@ public class LdapUserDetailsManager implements UserDetailsManager {
                 ctx.removeFromEnvironment("com.sun.jndi.ldap.connect.pool");
                 ctx.addToEnvironment(Context.SECURITY_PRINCIPAL, LdapUtils.getFullDn(dn, ctx).toUrl());
                 ctx.addToEnvironment(Context.SECURITY_CREDENTIALS, oldPassword);
+                // TODO: reconnect doesn't appear to actually change the credentials
                 try {
                     ctx.reconnect(null);
                 } catch (javax.naming.AuthenticationException e) {
@@ -317,11 +318,11 @@ public class LdapUserDetailsManager implements UserDetailsManager {
         userDetailsMapper.mapUserToContext(user, ctx);
     }
 
-    private void addAuthorities(DistinguishedName userDn, GrantedAuthority[] authorities) {
+    protected void addAuthorities(DistinguishedName userDn, GrantedAuthority[] authorities) {
         modifyAuthorities(userDn, authorities, DirContext.ADD_ATTRIBUTE);
     }
 
-    private void removeAuthorities(DistinguishedName userDn, GrantedAuthority[] authorities) {
+    protected void removeAuthorities(DistinguishedName userDn, GrantedAuthority[] authorities) {
         modifyAuthorities(userDn, authorities, DirContext.REMOVE_ATTRIBUTE);
     }
 

core/src/test/java/org/springframework/security/userdetails/ldap/LdapUserDetailsManagerTests.java

@@ -22,7 +22,6 @@ import org.springframework.security.ldap.AbstractLdapIntegrationTests;
 import org.springframework.security.ldap.DefaultLdapUsernameToDnMapper;
 import org.springframework.security.ldap.SpringSecurityLdapTemplate;
 import org.springframework.security.providers.UsernamePasswordAuthenticationToken;
-import org.springframework.security.userdetails.UserDetails;
 import org.springframework.security.userdetails.UsernameNotFoundException;
 import org.springframework.ldap.core.DirContextAdapter;
 
@@ -90,23 +89,17 @@ public class LdapUserDetailsManagerTests extends AbstractLdapIntegrationTests {
     public void testLoadUserByUsernameReturnsCorrectData() {
         mgr.setUsernameMapper(new DefaultLdapUsernameToDnMapper("ou=people","uid"));
         mgr.setGroupSearchBase("ou=groups");
-        UserDetails bob = mgr.loadUserByUsername("bob");
+        LdapUserDetails bob = (LdapUserDetails) mgr.loadUserByUsername("bob");
         assertEquals("bob", bob.getUsername());
-        // password isn't read
+        assertEquals("uid=bob, ou=people, dc=springframework, dc=org", bob.getDn());
-        //assertEquals("bobspassword", bob.getPassword());
+        assertEquals("bobspassword", bob.getPassword());
 
         assertEquals(1, bob.getAuthorities().length);
     }
 
-    @Test
+    @Test(expected = UsernameNotFoundException.class)
     public void testLoadingInvalidUsernameThrowsUsernameNotFoundException() {
-
+        mgr.loadUserByUsername("jim");
-        try {
-            mgr.loadUserByUsername("jim");
-            fail("Expected UsernameNotFoundException for user 'jim'");
-        } catch(UsernameNotFoundException expected) {
-            // expected
-        }
     }
 
     @Test
@@ -123,6 +116,7 @@ public class LdapUserDetailsManagerTests extends AbstractLdapIntegrationTests {
     @Test
     public void testCreateNewUserSucceeds() {
         InetOrgPerson.Essence p = new InetOrgPerson.Essence();
+        p.setDn("whocares");
         p.setCn(new String[] {"Joe Smeth"});
         p.setSn("Smeth");
         p.setUid("joe");
@@ -134,6 +128,7 @@ public class LdapUserDetailsManagerTests extends AbstractLdapIntegrationTests {
     @Test
     public void testDeleteUserSucceeds() {
         InetOrgPerson.Essence p = new InetOrgPerson.Essence();
+        p.setDn("whocares");
         p.setCn(new String[] {"Don Smeth"});
         p.setSn("Smeth");
         p.setUid("don");
@@ -162,6 +157,7 @@ public class LdapUserDetailsManagerTests extends AbstractLdapIntegrationTests {
     @Test
     public void testPasswordChangeWithCorrectOldPasswordSucceeds() {
         InetOrgPerson.Essence p = new InetOrgPerson.Essence();
+        p.setDn("whocares");
         p.setCn(new String[] {"John Yossarian"});
         p.setSn("Yossarian");
         p.setUid("johnyossarian");
@@ -179,9 +175,10 @@ public class LdapUserDetailsManagerTests extends AbstractLdapIntegrationTests {
                 "userPassword", "yossariansnewpassword"));
     }
 
-    @Test
+    @Test(expected = BadCredentialsException.class)
     public void testPasswordChangeWithWrongOldPasswordFails() {
         InetOrgPerson.Essence p = new InetOrgPerson.Essence();
+        p.setDn("whocares");
         p.setCn(new String[] {"John Yossarian"});
         p.setSn("Yossarian");
         p.setUid("johnyossarian");
@@ -193,11 +190,6 @@ public class LdapUserDetailsManagerTests extends AbstractLdapIntegrationTests {
         SecurityContextHolder.getContext().setAuthentication(
                 new UsernamePasswordAuthenticationToken("johnyossarian", "yossarianspassword", TEST_AUTHORITIES));
 
-        try {
+        mgr.changePassword("wrongpassword", "yossariansnewpassword");
-            mgr.changePassword("wrongpassword", "yossariansnewpassword");
-            fail("Expected BadCredentialsException");
-        } catch (BadCredentialsException expected) {
-        }
-
     }
 }