CLOUDSTACK-8596 addressed review comments In LdapUserManagerFactory moved the beans to a map used a Enum for LdapProvider and made the corresponding changes in LdapConfiguration and the callers.
Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/96cf0325 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/96cf0325 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/96cf0325 Branch: refs/heads/master Commit: 96cf0325e2b9542c18fde6f2c4f7191e6406cdb6 Parents: d42173a Author: Rajani Karuturi <[email protected]> Authored: Thu Jul 23 15:21:59 2015 +0530 Committer: Rajani Karuturi <[email protected]> Committed: Thu Jul 23 15:21:59 2015 +0530 ---------------------------------------------------------------------- .../cloudstack/ldap/ADLdapUserManagerImpl.java | 15 ++++---- .../cloudstack/ldap/LdapConfiguration.java | 11 +++++- .../apache/cloudstack/ldap/LdapUserManager.java | 4 +++ .../cloudstack/ldap/LdapUserManagerFactory.java | 37 ++++++++++++-------- .../ldap/LdapConfigurationSpec.groovy | 6 ++-- .../ldap/LdapUserManagerFactorySpec.groovy | 5 +-- 6 files changed, 52 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/96cf0325/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---------------------------------------------------------------------- diff --git a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java index e5e20c5..50f1fa0 100644 --- a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java +++ b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java @@ -32,21 +32,22 @@ import org.apache.log4j.Logger; public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager { public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName()); + private static final String MICROSOFT_AD_NESTED_MEMBERS_FILTER = "memberOf:1.2.840.113556.1.4.1941"; @Override public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException { - final SearchControls searchControls = new SearchControls(); - searchControls.setSearchScope(_ldapConfiguration.getScope()); - searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes()); + if (StringUtils.isBlank(groupName)) { + throw new IllegalArgumentException("ldap group name cannot be blank"); + } String basedn = _ldapConfiguration.getBaseDn(); if (StringUtils.isBlank(basedn)) { throw new IllegalArgumentException("ldap basedn is not configured"); } - if (StringUtils.isBlank(groupName)) { - throw new IllegalArgumentException("ldap group name cannot be blank"); - } + final SearchControls searchControls = new SearchControls(); + searchControls.setSearchScope(_ldapConfiguration.getScope()); + searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes()); NamingEnumeration<SearchResult> results = context.search(basedn, generateADGroupSearchFilter(groupName), searchControls); final List<LdapUser> users = new ArrayList<LdapUser>(); @@ -65,7 +66,7 @@ public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements Ld final StringBuilder memberOfFilter = new StringBuilder(); String groupCnName = _ldapConfiguration.getCommonNameAttribute() + "=" +groupName + "," + _ldapConfiguration.getBaseDn(); - memberOfFilter.append("(memberOf:1.2.840.113556.1.4.1941:="); + memberOfFilter.append("(" + MICROSOFT_AD_NESTED_MEMBERS_FILTER + ":="); memberOfFilter.append(groupCnName); memberOfFilter.append(")"); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/96cf0325/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java ---------------------------------------------------------------------- diff --git a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java index f247f40..a64899a 100644 --- a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java +++ b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java @@ -167,7 +167,16 @@ public class LdapConfiguration implements Configurable{ return ldapPageSize.value(); } - public String getLdapProvider() { return ldapProvider.value();} + public LdapUserManager.Provider getLdapProvider() { + LdapUserManager.Provider provider; + try { + provider = LdapUserManager.Provider.valueOf(ldapProvider.value().toUpperCase()); + } catch (IllegalArgumentException ex) { + //openldap is the default + provider = LdapUserManager.Provider.OPENLDAP; + } + return provider; + } @Override public String getConfigComponentName() { http://git-wip-us.apache.org/repos/asf/cloudstack/blob/96cf0325/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManager.java ---------------------------------------------------------------------- diff --git a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManager.java b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManager.java index 28fc74e..c1bfe74 100644 --- a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManager.java +++ b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManager.java @@ -26,6 +26,10 @@ import javax.naming.ldap.LdapContext; public interface LdapUserManager { + public enum Provider { + MICROSOFTAD, OPENLDAP; + } + public LdapUser getUser(final String username, final LdapContext context) throws NamingException, IOException; public List<LdapUser> getUsers(final LdapContext context) throws NamingException, IOException; http://git-wip-us.apache.org/repos/asf/cloudstack/blob/96cf0325/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java ---------------------------------------------------------------------- diff --git a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java index 35652fb..b7414c7 100644 --- a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java +++ b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java @@ -24,28 +24,37 @@ import org.springframework.beans.factory.config.AutowireCapableBeanFactory; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; +import java.util.HashMap; +import java.util.Map; + public class LdapUserManagerFactory implements ApplicationContextAware { + + public static final Logger s_logger = Logger.getLogger(LdapUserManagerFactory.class.getName()); - private static LdapUserManager adUserManager; - private static LdapUserManager openLdapUserManager; + private static Map<LdapUserManager.Provider, LdapUserManager> ldapUserManagerMap = new HashMap<>(); static ApplicationContext applicationCtx; - public LdapUserManager getInstance(String id) { - if ("microsoftad".equalsIgnoreCase(id)) { - if (adUserManager == null) { - adUserManager = new ADLdapUserManagerImpl(); - applicationCtx.getAutowireCapableBeanFactory().autowireBeanProperties(adUserManager, AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE, true); + public LdapUserManager getInstance(LdapUserManager.Provider provider) { + LdapUserManager ldapUserManager; + if (provider == LdapUserManager.Provider.MICROSOFTAD) { + ldapUserManager = ldapUserManagerMap.get(LdapUserManager.Provider.MICROSOFTAD); + if (ldapUserManager == null) { + ldapUserManager = new ADLdapUserManagerImpl(); + applicationCtx.getAutowireCapableBeanFactory().autowireBeanProperties(ldapUserManager, AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE, true); + ldapUserManagerMap.put(LdapUserManager.Provider.MICROSOFTAD, ldapUserManager); + } + } else { + //defaults to opendldap + ldapUserManager = ldapUserManagerMap.get(LdapUserManager.Provider.OPENLDAP); + if (ldapUserManager == null) { + ldapUserManager = new OpenLdapUserManagerImpl(); + applicationCtx.getAutowireCapableBeanFactory().autowireBeanProperties(ldapUserManager, AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE, true); + ldapUserManagerMap.put(LdapUserManager.Provider.OPENLDAP, ldapUserManager); } - return adUserManager; - } - //defaults to opendldap - if (openLdapUserManager == null) { - openLdapUserManager = new OpenLdapUserManagerImpl(); - applicationCtx.getAutowireCapableBeanFactory().autowireBeanProperties(openLdapUserManager, AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE, true); } - return openLdapUserManager; + return ldapUserManager; } @Override http://git-wip-us.apache.org/repos/asf/cloudstack/blob/96cf0325/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationSpec.groovy ---------------------------------------------------------------------- diff --git a/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationSpec.groovy b/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationSpec.groovy index a4cf3c2..4f93adf 100644 --- a/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationSpec.groovy +++ b/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationSpec.groovy @@ -25,6 +25,7 @@ import org.apache.cloudstack.framework.config.impl.ConfigurationVO import org.apache.cloudstack.ldap.LdapConfiguration import org.apache.cloudstack.ldap.LdapConfigurationVO import org.apache.cloudstack.ldap.LdapManager +import org.apache.cloudstack.ldap.LdapUserManager import org.apache.cloudstack.ldap.dao.LdapConfigurationDao import org.apache.cxf.common.util.StringUtils @@ -295,13 +296,14 @@ class LdapConfigurationSpec extends spock.lang.Specification { def ldapConfigurationDao = Mock(LdapConfigurationDao) LdapConfiguration ldapConfiguration = new LdapConfiguration(configDao, ldapConfigurationDao) - def expected = provider == null ? "openldap" : provider //"openldap" is the default value + def expected = provider.equalsIgnoreCase("microsoftad") ? LdapUserManager.Provider.MICROSOFTAD : LdapUserManager.Provider.OPENLDAP //"openldap" is the default value def result = ldapConfiguration.getLdapProvider() expect: + println "asserting for provider configuration: " + provider result == expected where: - provider << ["openldap", "microsoftad", null, "xyz"] + provider << ["openldap", "microsoftad", "", " ", "xyz", "MicrosoftAd", "OpenLdap", "MicrosoftAD"] } } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/96cf0325/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserManagerFactorySpec.groovy ---------------------------------------------------------------------- diff --git a/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserManagerFactorySpec.groovy b/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserManagerFactorySpec.groovy index 7615f9d..ca423d3 100644 --- a/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserManagerFactorySpec.groovy +++ b/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserManagerFactorySpec.groovy @@ -19,6 +19,7 @@ package groovy.org.apache.cloudstack.ldap import org.apache.cloudstack.ldap.ADLdapUserManagerImpl +import org.apache.cloudstack.ldap.LdapUserManager import org.apache.cloudstack.ldap.LdapUserManagerFactory import org.apache.cloudstack.ldap.OpenLdapUserManagerImpl import org.springframework.beans.factory.config.AutowireCapableBeanFactory @@ -42,7 +43,7 @@ class LdapUserManagerFactorySpec extends spock.lang.Specification { def result = ldapUserManagerFactory.getInstance(id); def expected; - if(id == "microsoftad") { + if(id == LdapUserManager.Provider.MICROSOFTAD) { expected = ADLdapUserManagerImpl.class; } else { expected = OpenLdapUserManagerImpl.class; @@ -51,6 +52,6 @@ class LdapUserManagerFactorySpec extends spock.lang.Specification { expect: assert result.class.is(expected) where: - id << ["openldap", "microsoftad", null, "xyz"] + id << [LdapUserManager.Provider.MICROSOFTAD, LdapUserManager.Provider.OPENLDAP, null] } }
