Align all properties to have the same format

as SAML properties https://www.pivotaltracker.com/story/show/106326628 [#106326628]
cloudfoundry · Oct 27, 2015 · bb0688c · bb0688c
1 parent d22af90
commit bb0688c
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 28 deletions.
diff --git a/.../org/cloudfoundry/identity/uaa/authentication/manager/LdapLoginAuthenticationManager.java b/.../org/cloudfoundry/identity/uaa/authentication/manager/LdapLoginAuthenticationManager.java
@@ -27,8 +27,6 @@
 import org.springframework.util.MultiValueMap;
 
 import java.util.Arrays;
-import java.util.Collections;
-import java.util.List;
 import java.util.Map;
 
 public class LdapLoginAuthenticationManager extends ExternalLoginAuthenticationManager {
@@ -41,24 +39,21 @@ public void setProvisioning(IdentityProviderProvisioning provisioning) {
         this.provisioning = provisioning;
     }
 
-    public static final List<String> ALREADY_MAPPED_ATTRS =
-        Collections.unmodifiableList(Arrays.asList("first_name", "family_name", "phone_number"));
-
     @Override
     protected MultiValueMap<String, String> getUserAttributes(UserDetails request) {
         MultiValueMap<String, String> result = super.getUserAttributes(request);
         if (provisioning!=null) {
             IdentityProvider provider = provisioning.retrieveByOrigin(getOrigin(), IdentityZoneHolder.get().getId());
             if (request instanceof ExtendedLdapUserDetails) {
                 ExtendedLdapUserDetails ldapDetails = ((ExtendedLdapUserDetails) request);
-                for (Map.Entry<String, Object> entry : provider.getConfigValue(LdapIdentityProviderDefinition.class).getAttributeMappings().entrySet()) {
+                LdapIdentityProviderDefinition ldapIdentityProviderDefinition = provider.getConfigValue(LdapIdentityProviderDefinition.class);
+                Map<String, Object> providerMappings = ldapIdentityProviderDefinition.getAttributeMappings();
+                for (Map.Entry<String, Object> entry : providerMappings.entrySet()) {
                     if (entry.getKey().startsWith(USER_ATTRIBUTE_PREFIX) && entry.getValue() != null) {
                         String key = entry.getKey().substring(USER_ATTRIBUTE_PREFIX.length());
-                        if (! ALREADY_MAPPED_ATTRS.contains(key)) {
-                            String[] values = ldapDetails.getAttribute((String) entry.getValue(), false);
-                            if (values != null && values.length > 0) {
-                                result.put(key, Arrays.asList(values));
-                            }
+                        String[] values = ldapDetails.getAttribute((String) entry.getValue(), false);
+                        if (values != null && values.length > 0) {
+                            result.put(key, Arrays.asList(values));
                         }
                     }
                 }

diff --git a/common/src/main/java/org/cloudfoundry/identity/uaa/ldap/LdapIdentityProviderDefinition.java b/common/src/main/java/org/cloudfoundry/identity/uaa/ldap/LdapIdentityProviderDefinition.java
@@ -192,8 +192,10 @@ public static LdapIdentityProviderDefinition searchAndBindMapGroupToScopes(
         return definition;
     }
 
+    /**
+     * Load a LDAP definition from the Yaml config (IdentityProviderBootstrap)
+     */
     public static LdapIdentityProviderDefinition fromConfig(Map<String, Object> ldapConfig) {
-
         LdapIdentityProviderDefinition definition = new LdapIdentityProviderDefinition();
         if (ldapConfig==null || ldapConfig.isEmpty()) {
             return definition;
@@ -261,9 +263,13 @@ public static LdapIdentityProviderDefinition fromConfig(Map<String, Object> ldap
             definition.setAutoAddGroups((Boolean) ldapConfig.get(LDAP_GROUPS_AUTO_ADD));
             definition.setGroupRoleAttribute((String) ldapConfig.get(LDAP_GROUPS_GROUP_ROLE_ATTRIBUTE));
         }
-        final String LDAP_ATTR_MAP_PREFIX = "ldap."+ATTRIBUTE_MAPPINGS+".";
+
+        //if flat attributes are set in the properties
+        final String LDAP_ATTR_MAP_PREFIX = LDAP_ATTRIBUTE_MAPPINGS+".";
         for (Map.Entry<String,Object> entry : ldapConfig.entrySet()) {
-            if (!LDAP_PROPERTY_NAMES.contains(entry.getKey()) && entry.getKey().startsWith(LDAP_ATTR_MAP_PREFIX+USER_ATTRIBUTE_PREFIX)) {
+            if (!LDAP_PROPERTY_NAMES.contains(entry.getKey()) &&
+                entry.getKey().startsWith(LDAP_ATTR_MAP_PREFIX) &&
+                entry.getValue() instanceof String) {
                 definition.addAttributeMapping(entry.getKey().substring(LDAP_ATTR_MAP_PREFIX.length()), entry.getValue());
             }
         }

diff --git a/docs/UAA-LDAP.md b/docs/UAA-LDAP.md
@@ -704,7 +704,8 @@ ldap:
     first_name: givenname
     family_name: sn
     phone_number: telephonenumber
-    email: mail
-    user.attribute.employeeCostCenter: costCenter
-    user.attribute.terribleBosses: manager
+    user:
+      attribute:
+        employeeCostCenter: costCenter
+        terribleBosses: manager
 </pre>
diff --git a/uaa/src/main/resources/ldap-integration.xml b/uaa/src/main/resources/ldap-integration.xml
@@ -48,9 +48,9 @@
 
     <bean id="extendedLdapUserDetailsMapper" class="org.cloudfoundry.identity.uaa.ldap.ExtendedLdapUserMapper">
         <property name="mailAttributeName" value="${ldap.base.mailAttributeName:mail}"/>
-        <property name="givenNameAttributeName" value="${ldap.attributeMappings.user.attribute.first_name:givenname}"/>
-        <property name="familyNameAttributeName" value="${ldap.attributeMappings.user.attribute.family_name:sn}"/>
-        <property name="phoneNumberAttributeName" value="${ldap.attributeMappings.user.attribute.phone_number:telephonenumber}"/>
+        <property name="givenNameAttributeName" value="${ldap.attributeMappings.first_name:givenname}"/>
+        <property name="familyNameAttributeName" value="${ldap.attributeMappings.family_name:sn}"/>
+        <property name="phoneNumberAttributeName" value="${ldap.attributeMappings.phone_number:telephonenumber}"/>
         <property name="mailSubstitute" value="${ldap.base.mailSubstitute:null}"/>
         <property name="mailSubstituteOverridesLdap" value="${ldap.base.mailSubstituteOverridesLdap:false}"/>
     </bean>

diff --git a/uaa/src/main/resources/uaa.yml b/uaa/src/main/resources/uaa.yml
@@ -95,8 +95,9 @@
 #  emailDomain:
 #    - example.com
 #  attributeMappings:
-#    given_name: firstName
-#    family_name: surname
+#    given_name: givenname
+#    family_name: sn
+#    phone_number: telephonenumber
 #    user.attribute.employeeCostCenter: costCenter
 #    user.attribute.terribleBosses: uaaManager
 

diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/ldap/LdapMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/ldap/LdapMockMvcTests.java
@@ -80,6 +80,7 @@
 import java.util.Set;
 
 import static org.cloudfoundry.identity.uaa.ExternalIdentityProviderDefinition.ATTRIBUTE_MAPPINGS;
+import static org.cloudfoundry.identity.uaa.ldap.LdapIdentityProviderDefinition.LDAP_ATTRIBUTE_MAPPINGS;
 import static org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils.CookieCsrfPostProcessor.cookieCsrf;
 import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
 import static org.hamcrest.Matchers.containsInAnyOrder;
@@ -241,10 +242,9 @@ public void testCustomUserAttributes() throws Exception {
         mockEnvironment.setProperty("ldap."+ATTRIBUTE_MAPPINGS+".user.attribute."+COST_CENTERS, COST_CENTER);
 
         //test to remap the user/person properties
-        mockEnvironment.setProperty("ldap."+ATTRIBUTE_MAPPINGS+".user.attribute."+FIRST_NAME, "sn");
-        mockEnvironment.setProperty("ldap."+ATTRIBUTE_MAPPINGS+".user.attribute."+PHONE_NUMBER, "givenname");
-        mockEnvironment.setProperty("ldap."+ATTRIBUTE_MAPPINGS+".user.attribute."+FAMILY_NAME, "telephonenumber");
-        mockEnvironment.setProperty("ldap."+ATTRIBUTE_MAPPINGS+".user.attribute."+EMAIL, "mail");
+        mockEnvironment.setProperty(LDAP_ATTRIBUTE_MAPPINGS+"."+FIRST_NAME, "sn");
+        mockEnvironment.setProperty(LDAP_ATTRIBUTE_MAPPINGS+"."+PHONE_NUMBER, "givenname");
+        mockEnvironment.setProperty(LDAP_ATTRIBUTE_MAPPINGS+"."+FAMILY_NAME, "telephonenumber");
 
         setUp();
 
@@ -254,7 +254,7 @@ public void testCustomUserAttributes() throws Exception {
 
         UaaAuthentication authentication = (UaaAuthentication) ((SecurityContext) result.getRequest().getSession().getAttribute(SPRING_SECURITY_CONTEXT_KEY)).getAuthentication();
 
-        assertEquals("Expected two user attributes", 3, authentication.getUserAttributes().size());
+        assertEquals("Expected two user attributes", 2, authentication.getUserAttributes().size());
         assertNotNull("Expected cost center attribute", authentication.getUserAttributes().get(COST_CENTERS));
         assertEquals(DENVER_CO, authentication.getUserAttributes().getFirst(COST_CENTERS));
 
@@ -265,7 +265,6 @@ public void testCustomUserAttributes() throws Exception {
         assertEquals("8885550986", getFamilyName(username));
         assertEquals("Marissa", getPhoneNumber(username));
         assertEquals("Marissa9", getGivenName(username));
-        assertThat(authentication.getUserAttributes().get(EMAIL), containsInAnyOrder("marissa9@test.com", "marissa9-custom@test.com"));
     }
 
     @Test