You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by karuturi <gi...@git.apache.org> on 2015/07/20 14:38:38 UTC

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

GitHub user karuturi opened a pull request:

    https://github.com/apache/cloudstack/pull/609

    CLOUDSTACK-8596 ability to query nested groups for Microsoft AD

    added a new configuration to select the appropriate ldap implementation
    incase of microsoft AD enabled nested querying of group members
    
    moved LdapUserManager to an interface and added separate implementations
    for openLdap and microsoft AD
    Added unit tests
    
    ![screen shot 2015-07-20 at 6 06 54 pm](https://cloud.githubusercontent.com/assets/186833/8775899/4c3fa36a-2f0a-11e5-8280-d128cd1af02f.png)


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/karuturi/cloudstack CLOUDSTACK-8596

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/cloudstack/pull/609.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #609
    
----
commit 4e57cc62d0838f74b09f06d741c9b7cf532abd56
Author: Rajani Karuturi <ra...@gmail.com>
Date:   2015-07-20T12:26:54Z

    CLOUDSTACK-8596 ability to query nested groups for Microsoft AD
    
    added a new configuration to select the appropriate ldap implementation
    incase of microsoft AD enabled nested querying of group members
    
    moved LdapUserManager to an interface and added separate implementations
    for openLdap and microsoft AD
    Added unit tests

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35293493
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java ---
    @@ -0,0 +1,55 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import org.apache.log4j.Logger;
    +import org.springframework.beans.BeansException;
    +import org.springframework.beans.factory.config.AutowireCapableBeanFactory;
    +import org.springframework.context.ApplicationContext;
    +import org.springframework.context.ApplicationContextAware;
    +
    +public class LdapUserManagerFactory implements ApplicationContextAware {
    +    public static final Logger s_logger = Logger.getLogger(LdapUserManagerFactory.class.getName());
    +
    +    private static LdapUserManager adUserManager;
    --- End diff --
    
    Looks like both providers can co-exists. In that case it is better to have a map <name, manager>.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:

    https://github.com/apache/cloudstack/pull/609#issuecomment-124052158
  
    ran tests from command line. code looks good. I use eclipse and did not manage to run tests in the ide yet (no problem with the PR). HInts welcome :) :+1:  waiting on travis and the analisys builds


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35292541
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---
    @@ -0,0 +1,81 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.LdapContext;
    +
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager {
    +    public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName());
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        final SearchControls searchControls = new SearchControls();
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        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");
    +        }
    +
    +        NamingEnumeration<SearchResult> results = context.search(basedn, generateADGroupSearchFilter(groupName), searchControls);
    +        final List<LdapUser> users = new ArrayList<LdapUser>();
    +        while (results.hasMoreElements()) {
    +            final SearchResult result = results.nextElement();
    +            users.add(createUser(result));
    +        }
    +        return users;
    +    }
    +
    +    private String generateADGroupSearchFilter(String groupName) {
    +        final StringBuilder userObjectFilter = new StringBuilder();
    +        userObjectFilter.append("(objectClass=");
    +        userObjectFilter.append(_ldapConfiguration.getUserObject());
    +        userObjectFilter.append(")");
    +
    +        final StringBuilder memberOfFilter = new StringBuilder();
    +        String groupCnName =  _ldapConfiguration.getCommonNameAttribute() + "=" +groupName + "," +  _ldapConfiguration.getBaseDn();
    +        memberOfFilter.append("(memberOf:1.2.840.113556.1.4.1941:=");
    --- End diff --
    
    What does the numbers mean? Can we have a meaningful name for the constant?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35188707
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
    @@ -57,17 +57,22 @@
         private LdapContextFactory _ldapContextFactory;
     
         @Inject
    -    private LdapUserManager _ldapUserManager;
    +    private LdapConfiguration _ldapConfiguration;
    +
    +    @Inject LdapUserManagerFactory _ldapUserManagerFactory;
    +
     
         public LdapManagerImpl() {
             super();
         }
     
    -    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManager ldapUserManager) {
    +    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManagerFactory ldapUserManagerFactory,
    +                           final LdapConfiguration ldapConfiguration) {
    --- End diff --
    
    And why not use injection for that as well? Is this a groovy thing?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35308512
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---
    @@ -0,0 +1,81 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.LdapContext;
    +
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager {
    +    public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName());
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        final SearchControls searchControls = new SearchControls();
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        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");
    +        }
    +
    +        NamingEnumeration<SearchResult> results = context.search(basedn, generateADGroupSearchFilter(groupName), searchControls);
    +        final List<LdapUser> users = new ArrayList<LdapUser>();
    +        while (results.hasMoreElements()) {
    +            final SearchResult result = results.nextElement();
    +            users.add(createUser(result));
    +        }
    +        return users;
    +    }
    +
    +    private String generateADGroupSearchFilter(String groupName) {
    +        final StringBuilder userObjectFilter = new StringBuilder();
    +        userObjectFilter.append("(objectClass=");
    +        userObjectFilter.append(_ldapConfiguration.getUserObject());
    +        userObjectFilter.append(")");
    +
    +        final StringBuilder memberOfFilter = new StringBuilder();
    +        String groupCnName =  _ldapConfiguration.getCommonNameAttribute() + "=" +groupName + "," +  _ldapConfiguration.getBaseDn();
    +        memberOfFilter.append("(memberOf:1.2.840.113556.1.4.1941:=");
    --- End diff --
    
    done



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35291594
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java ---
    @@ -36,6 +36,8 @@
     
         private static final ConfigKey<Integer> ldapPageSize = new ConfigKey<Integer>(Integer.class, "ldap.request.page.size", "Advanced", "1000",
                                                                                    "page size sent to ldap server on each request to get user", true, ConfigKey.Scope.Global, 1);
    +    private static final ConfigKey<String> ldapProvider = new ConfigKey<String>(String.class, "ldap.provider", "Advanced", "openldap", "ldap provider ex:openldap, microsoftad",
    +                                                                                true, ConfigKey.Scope.Global, null);
    --- End diff --
    
    no. multiple providers are not possible. 
    I havent touched any of the existing configurations as I will be refactoring them along with https://cwiki.apache.org/confluence/display/CLOUDSTACK/WIP%3A+LDAP%3A+Trust+AD+and+Auto+Import


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35292860
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---
    @@ -0,0 +1,81 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.LdapContext;
    +
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager {
    +    public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName());
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        final SearchControls searchControls = new SearchControls();
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        String basedn = _ldapConfiguration.getBaseDn();
    +        if (StringUtils.isBlank(basedn)) {
    +            throw new IllegalArgumentException("ldap basedn is not configured");
    +        }
    +
    +        if (StringUtils.isBlank(groupName)) {
    --- End diff --
    
    will do


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:

    https://github.com/apache/cloudstack/pull/609#issuecomment-123668417
  
    ```
    root@Rajani's MS ~/source/cloudstack/plugins/user-authenticators/ldap (CLOUDSTACK-8596)$ mvn clean test
    [INFO] Scanning for projects...
    [INFO]
    [INFO] ------------------------------------------------------------------------
    [INFO] Building Apache CloudStack Plugin - User Authenticator LDAP 4.6.0-SNAPSHOT
    [INFO] ------------------------------------------------------------------------
    [INFO]
    [INFO] --- maven-clean-plugin:2.5:clean (default-clean) @ cloud-plugin-user-authenticator-ldap ---
    [INFO] Deleting /root/source/cloudstack/plugins/user-authenticators/ldap/target (includes = [**/*], excludes = [])
    [INFO] Deleting /root/source/cloudstack/plugins/user-authenticators/ldap (includes = [target, dist], excludes = [])
    [INFO]
    [INFO] --- maven-checkstyle-plugin:2.11:check (cloudstack-checkstyle) @ cloud-plugin-user-authenticator-ldap ---
    [INFO] Starting audit...
    Audit done.
    
    [INFO]
    [INFO] --- maven-remote-resources-plugin:1.3:process (default) @ cloud-plugin-user-authenticator-ldap ---
    [INFO]
    [INFO] --- maven-resources-plugin:2.5:resources (default-resources) @ cloud-plugin-user-authenticator-ldap ---
    [debug] execute contextualize
    [INFO] Using 'UTF-8' encoding to copy filtered resources.
    [INFO] Copying 2 resources
    [INFO] Copying 3 resources
    [INFO]
    [INFO] --- maven-compiler-plugin:3.2:compile (default-compile) @ cloud-plugin-user-authenticator-ldap ---
    [INFO] Changes detected - recompiling the module!
    [INFO] Compiling 29 source files to /root/source/cloudstack/plugins/user-authenticators/ldap/target/classes
    [INFO]
    [INFO] --- gmaven-plugin:1.3:compile (default) @ cloud-plugin-user-authenticator-ldap ---
    [INFO] Compiled 38 Groovy classes
    [INFO]
    [INFO] --- maven-resources-plugin:2.5:testResources (default-testResources) @ cloud-plugin-user-authenticator-ldap ---
    [debug] execute contextualize
    [INFO] Using 'UTF-8' encoding to copy filtered resources.
    [INFO] Copying 1 resource
    [INFO] Copying 3 resources
    [INFO]
    [INFO] --- maven-compiler-plugin:3.2:testCompile (default-testCompile) @ cloud-plugin-user-authenticator-ldap ---
    [INFO] Nothing to compile - all classes are up to date
    [INFO]
    [INFO] --- gmaven-plugin:1.3:testCompile (default) @ cloud-plugin-user-authenticator-ldap ---
    [INFO] Compiled 38 Groovy classes
    [INFO]
    [INFO] --- maven-surefire-plugin:2.18.1:test (default-test) @ cloud-plugin-user-authenticator-ldap ---
    [INFO] Surefire report directory: /root/source/cloudstack/plugins/user-authenticators/ldap/target/surefire-reports
    
    -------------------------------------------------------
     T E S T S
    -------------------------------------------------------
    Running groovy.org.apache.cloudstack.ldap.NoLdapUserMatchingQueryExceptionSpec
    Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.304 sec - in groovy.org.apache.cloudstack.ldap.NoLdapUserMatchingQueryExceptionSpec
    Running groovy.org.apache.cloudstack.ldap.LdapManagerImplSpec
    log4j:WARN No appenders could be found for logger (org.apache.cloudstack.ldap.LdapManagerImpl).
    log4j:WARN Please initialize the log4j system properly.
    log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
    Tests run: 22, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.38 sec - in groovy.org.apache.cloudstack.ldap.LdapManagerImplSpec
    Running groovy.org.apache.cloudstack.ldap.LdapListUsersCmdSpec
    Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.046 sec - in groovy.org.apache.cloudstack.ldap.LdapListUsersCmdSpec
    Running groovy.org.apache.cloudstack.ldap.LdapAddConfigurationCmdSpec
    Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.028 sec - in groovy.org.apache.cloudstack.ldap.LdapAddConfigurationCmdSpec
    Running groovy.org.apache.cloudstack.ldap.LdapUserSpec
    Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.018 sec - in groovy.org.apache.cloudstack.ldap.LdapUserSpec
    Running groovy.org.apache.cloudstack.ldap.LdapAuthenticatorSpec
    Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.035 sec - in groovy.org.apache.cloudstack.ldap.LdapAuthenticatorSpec
    Running groovy.org.apache.cloudstack.ldap.LdapConfigurationVOSpec
    Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.005 sec - in groovy.org.apache.cloudstack.ldap.LdapConfigurationVOSpec
    Running groovy.org.apache.cloudstack.ldap.OpenLdapUserManagerSpec
    Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.095 sec - in groovy.org.apache.cloudstack.ldap.OpenLdapUserManagerSpec
    Running groovy.org.apache.cloudstack.ldap.LdapDeleteConfigurationCmdSpec
    Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.011 sec - in groovy.org.apache.cloudstack.ldap.LdapDeleteConfigurationCmdSpec
    Running groovy.org.apache.cloudstack.ldap.LdapUserResponseSpec
    Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.011 sec - in groovy.org.apache.cloudstack.ldap.LdapUserResponseSpec
    Running groovy.org.apache.cloudstack.ldap.LdapUserManagerFactorySpec
    Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.03 sec - in groovy.org.apache.cloudstack.ldap.LdapUserManagerFactorySpec
    Running groovy.org.apache.cloudstack.ldap.ADLdapUserManagerImplSpec
    Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.011 sec - in groovy.org.apache.cloudstack.ldap.ADLdapUserManagerImplSpec
    Running groovy.org.apache.cloudstack.ldap.LdapCreateAccountCmdSpec
    Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.201 sec - in groovy.org.apache.cloudstack.ldap.LdapCreateAccountCmdSpec
    Running groovy.org.apache.cloudstack.ldap.LdapImportUsersCmdSpec
    Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.075 sec - in groovy.org.apache.cloudstack.ldap.LdapImportUsersCmdSpec
    Running groovy.org.apache.cloudstack.ldap.LdapSearchUserCmdSpec
    Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.011 sec - in groovy.org.apache.cloudstack.ldap.LdapSearchUserCmdSpec
    Running groovy.org.apache.cloudstack.ldap.LdapListConfigurationCmdSpec
    Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.01 sec - in groovy.org.apache.cloudstack.ldap.LdapListConfigurationCmdSpec
    Running groovy.org.apache.cloudstack.ldap.NoSuchLdapUserExceptionSpec
    Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.006 sec - in groovy.org.apache.cloudstack.ldap.NoSuchLdapUserExceptionSpec
    Running groovy.org.apache.cloudstack.ldap.LdapConfigurationResponseSpec
    Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.006 sec - in groovy.org.apache.cloudstack.ldap.LdapConfigurationResponseSpec
    Running groovy.org.apache.cloudstack.ldap.LdapConfigurationSpec
    Tests run: 19, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.057 sec - in groovy.org.apache.cloudstack.ldap.LdapConfigurationSpec
    Running groovy.org.apache.cloudstack.ldap.LdapContextFactorySpec
    Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.082 sec - in groovy.org.apache.cloudstack.ldap.LdapContextFactorySpec
    Running groovy.org.apache.cloudstack.ldap.LdapConfigurationDaoImplSpec
    Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.028 sec - in groovy.org.apache.cloudstack.ldap.LdapConfigurationDaoImplSpec
    Running groovy.org.apache.cloudstack.ldap.LdapUtilsSpec
    Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.009 sec - in groovy.org.apache.cloudstack.ldap.LdapUtilsSpec
    
    Results :
    
    Tests run: 138, Failures: 0, Errors: 0, Skipped: 0
    
    [INFO] ------------------------------------------------------------------------
    [INFO] BUILD SUCCESS
    [INFO] ------------------------------------------------------------------------
    [INFO] Total time: 11.759 s
    [INFO] Finished at: 2015-07-22T16:28:47+05:30
    [INFO] Final Memory: 40M/717M
    [INFO] ------------------------------------------------------------------------
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35184816
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
    @@ -57,17 +57,22 @@
         private LdapContextFactory _ldapContextFactory;
     
         @Inject
    -    private LdapUserManager _ldapUserManager;
    +    private LdapConfiguration _ldapConfiguration;
    +
    +    @Inject LdapUserManagerFactory _ldapUserManagerFactory;
    +
     
         public LdapManagerImpl() {
             super();
         }
     
    -    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManager ldapUserManager) {
    +    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManagerFactory ldapUserManagerFactory,
    +                           final LdapConfiguration ldapConfiguration) {
    --- End diff --
    
    constructor initialized ones were used in the unit tests to supply mocks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/cloudstack/pull/609


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35293673
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java ---
    @@ -0,0 +1,55 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import org.apache.log4j.Logger;
    +import org.springframework.beans.BeansException;
    +import org.springframework.beans.factory.config.AutowireCapableBeanFactory;
    +import org.springframework.context.ApplicationContext;
    +import org.springframework.context.ApplicationContextAware;
    +
    +public class LdapUserManagerFactory implements ApplicationContextAware {
    +    public static final Logger s_logger = Logger.getLogger(LdapUserManagerFactory.class.getName());
    +
    +    private static LdapUserManager adUserManager;
    +    private static LdapUserManager openLdapUserManager;
    +
    +    static ApplicationContext applicationCtx;
    +
    +    public LdapUserManager getInstance(String id) {
    --- End diff --
    
    The issue with that is, if the provider configuration is changed, management server restart is required.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35292815
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---
    @@ -0,0 +1,81 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.LdapContext;
    +
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager {
    +    public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName());
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        final SearchControls searchControls = new SearchControls();
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        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");
    +        }
    +
    +        NamingEnumeration<SearchResult> results = context.search(basedn, generateADGroupSearchFilter(groupName), searchControls);
    +        final List<LdapUser> users = new ArrayList<LdapUser>();
    +        while (results.hasMoreElements()) {
    +            final SearchResult result = results.nextElement();
    +            users.add(createUser(result));
    +        }
    +        return users;
    +    }
    +
    +    private String generateADGroupSearchFilter(String groupName) {
    +        final StringBuilder userObjectFilter = new StringBuilder();
    +        userObjectFilter.append("(objectClass=");
    +        userObjectFilter.append(_ldapConfiguration.getUserObject());
    +        userObjectFilter.append(")");
    +
    +        final StringBuilder memberOfFilter = new StringBuilder();
    +        String groupCnName =  _ldapConfiguration.getCommonNameAttribute() + "=" +groupName + "," +  _ldapConfiguration.getBaseDn();
    +        memberOfFilter.append("(memberOf:1.2.840.113556.1.4.1941:=");
    --- End diff --
    
    thats the AD filter to get nested group users. will move it to a constant.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35181750
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java ---
    @@ -43,14 +45,14 @@
         private ConfigurationDao _configDao;
     
         @Inject
    -    private LdapManager _ldapManager;
    +    private LdapConfigurationDao _ldapConfigurationDao;
     
         public LdapConfiguration() {
         }
     
    -    public LdapConfiguration(final ConfigurationDao configDao, final LdapManager ldapManager) {
    +    public LdapConfiguration(final ConfigurationDao configDao, final LdapConfigurationDao ldapConfigurationDao) {
             _configDao = configDao;
    -        _ldapManager = ldapManager;
    +        _ldapConfigurationDao = ldapConfigurationDao;
    --- End diff --
    
    Why inject and pass on in the constructor. Do both have usage scenarios?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35308492
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java ---
    @@ -0,0 +1,55 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import org.apache.log4j.Logger;
    +import org.springframework.beans.BeansException;
    +import org.springframework.beans.factory.config.AutowireCapableBeanFactory;
    +import org.springframework.context.ApplicationContext;
    +import org.springframework.context.ApplicationContextAware;
    +
    +public class LdapUserManagerFactory implements ApplicationContextAware {
    +    public static final Logger s_logger = Logger.getLogger(LdapUserManagerFactory.class.getName());
    +
    +    private static LdapUserManager adUserManager;
    --- End diff --
    
    done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35291227
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java ---
    @@ -36,6 +36,8 @@
     
         private static final ConfigKey<Integer> ldapPageSize = new ConfigKey<Integer>(Integer.class, "ldap.request.page.size", "Advanced", "1000",
                                                                                    "page size sent to ldap server on each request to get user", true, ConfigKey.Scope.Global, 1);
    +    private static final ConfigKey<String> ldapProvider = new ConfigKey<String>(String.class, "ldap.provider", "Advanced", "openldap", "ldap provider ex:openldap, microsoftad",
    +                                                                                true, ConfigKey.Scope.Global, null);
    --- End diff --
    
    As part of this change please move all LDAP related config to use the ConfigKey/Configurable mechanism of publishing them. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35292608
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
    @@ -57,17 +57,22 @@
         private LdapContextFactory _ldapContextFactory;
     
         @Inject
    -    private LdapUserManager _ldapUserManager;
    +    private LdapConfiguration _ldapConfiguration;
    +
    +    @Inject LdapUserManagerFactory _ldapUserManagerFactory;
    +
     
         public LdapManagerImpl() {
             super();
         }
     
    -    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManager ldapUserManager) {
    +    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManagerFactory ldapUserManagerFactory,
    --- End diff --
    
    Please put a comment that this is test-only


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35291585
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---
    @@ -0,0 +1,81 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.LdapContext;
    +
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager {
    +    public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName());
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        final SearchControls searchControls = new SearchControls();
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        String basedn = _ldapConfiguration.getBaseDn();
    +        if (StringUtils.isBlank(basedn)) {
    +            throw new IllegalArgumentException("ldap basedn is not configured");
    --- End diff --
    
    Isn't Basedn a required config? In that case it should be validated while configuring the ldap manager and not every time with these operations.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35293580
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java ---
    @@ -0,0 +1,55 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import org.apache.log4j.Logger;
    +import org.springframework.beans.BeansException;
    +import org.springframework.beans.factory.config.AutowireCapableBeanFactory;
    +import org.springframework.context.ApplicationContext;
    +import org.springframework.context.ApplicationContextAware;
    +
    +public class LdapUserManagerFactory implements ApplicationContextAware {
    +    public static final Logger s_logger = Logger.getLogger(LdapUserManagerFactory.class.getName());
    +
    +    private static LdapUserManager adUserManager;
    +    private static LdapUserManager openLdapUserManager;
    +
    +    static ApplicationContext applicationCtx;
    +
    +    public LdapUserManager getInstance(String id) {
    --- End diff --
    
    Can we rewrite like
    
    LdapUserManager manager;
    if (microsoft) {}
    else {}
    applicationCtx.getAutowire…
    return manager;


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35181833
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
    @@ -57,17 +57,22 @@
         private LdapContextFactory _ldapContextFactory;
     
         @Inject
    -    private LdapUserManager _ldapUserManager;
    +    private LdapConfiguration _ldapConfiguration;
    +
    +    @Inject LdapUserManagerFactory _ldapUserManagerFactory;
    +
     
         public LdapManagerImpl() {
             super();
         }
     
    -    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManager ldapUserManager) {
    +    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManagerFactory ldapUserManagerFactory,
    +                           final LdapConfiguration ldapConfiguration) {
    --- End diff --
    
    why inject and pass into constructor? is the ldap-provider-selector going to change on runtime?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35292477
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---
    @@ -0,0 +1,81 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.LdapContext;
    +
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager {
    +    public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName());
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        final SearchControls searchControls = new SearchControls();
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        String basedn = _ldapConfiguration.getBaseDn();
    +        if (StringUtils.isBlank(basedn)) {
    +            throw new IllegalArgumentException("ldap basedn is not configured");
    +        }
    +
    +        if (StringUtils.isBlank(groupName)) {
    --- End diff --
    
    This should be at the beginning.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:

    https://github.com/apache/cloudstack/pull/609#issuecomment-123998336
  
    Once the comments are addressed, LGTM.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35293652
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/OpenLdapUserManagerImpl.java ---
    @@ -0,0 +1,233 @@
    +// Licensed to the Apache Software Foundation (ASF) under one
    +// or more contributor license agreements.  See the NOTICE file
    +// distributed with this work for additional information
    +// regarding copyright ownership.  The ASF licenses this file
    +// to you under the Apache License, Version 2.0 (the
    +// "License"); you may not use this file except in compliance
    +// with the License.  You may obtain a copy of the License at
    +//
    +//   http://www.apache.org/licenses/LICENSE-2.0
    +//
    +// Unless required by applicable law or agreed to in writing,
    +// software distributed under the License is distributed on an
    +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +// KIND, either express or implied.  See the License for the
    +// specific language governing permissions and limitations
    +// under the License.
    +package org.apache.cloudstack.ldap;
    +
    +import java.io.IOException;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import javax.inject.Inject;
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.Attribute;
    +import javax.naming.directory.Attributes;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.Control;
    +import javax.naming.ldap.LdapContext;
    +import javax.naming.ldap.PagedResultsControl;
    +import javax.naming.ldap.PagedResultsResponseControl;
    +
    +import org.apache.commons.collections.CollectionUtils;
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class OpenLdapUserManagerImpl implements LdapUserManager {
    +    private static final Logger s_logger = Logger.getLogger(OpenLdapUserManagerImpl.class.getName());
    +
    +    @Inject
    +    protected LdapConfiguration _ldapConfiguration;
    +
    +    public OpenLdapUserManagerImpl() {
    +    }
    +
    +    public OpenLdapUserManagerImpl(final LdapConfiguration ldapConfiguration) {
    +        _ldapConfiguration = ldapConfiguration;
    +    }
    +
    +    protected LdapUser createUser(final SearchResult result) throws NamingException {
    +        final Attributes attributes = result.getAttributes();
    +
    +        final String username = LdapUtils.getAttributeValue(attributes, _ldapConfiguration.getUsernameAttribute());
    +        final String email = LdapUtils.getAttributeValue(attributes, _ldapConfiguration.getEmailAttribute());
    +        final String firstname = LdapUtils.getAttributeValue(attributes, _ldapConfiguration.getFirstnameAttribute());
    +        final String lastname = LdapUtils.getAttributeValue(attributes, _ldapConfiguration.getLastnameAttribute());
    +        final String principal = result.getNameInNamespace();
    +
    +        String domain = principal.replace("cn=" + LdapUtils.getAttributeValue(attributes, _ldapConfiguration.getCommonNameAttribute()) + ",", "");
    +        domain = domain.replace("," + _ldapConfiguration.getBaseDn(), "");
    +        domain = domain.replace("ou=", "");
    +
    +        return new LdapUser(username, email, firstname, lastname, principal, domain);
    +    }
    +
    +    private String generateSearchFilter(final String username) {
    +        final StringBuilder userObjectFilter = new StringBuilder();
    +        userObjectFilter.append("(objectClass=");
    +        userObjectFilter.append(_ldapConfiguration.getUserObject());
    +        userObjectFilter.append(")");
    +
    +        final StringBuilder usernameFilter = new StringBuilder();
    +        usernameFilter.append("(");
    +        usernameFilter.append(_ldapConfiguration.getUsernameAttribute());
    +        usernameFilter.append("=");
    +        usernameFilter.append((username == null ? "*" : username));
    +        usernameFilter.append(")");
    +
    +        final StringBuilder memberOfFilter = new StringBuilder();
    +        if (_ldapConfiguration.getSearchGroupPrinciple() != null) {
    +            memberOfFilter.append("(memberof=");
    +            memberOfFilter.append(_ldapConfiguration.getSearchGroupPrinciple());
    +            memberOfFilter.append(")");
    +        }
    +
    +        final StringBuilder result = new StringBuilder();
    +        result.append("(&");
    +        result.append(userObjectFilter);
    +        result.append(usernameFilter);
    +        result.append(memberOfFilter);
    +        result.append(")");
    +
    +        return result.toString();
    +    }
    +
    +    private String generateGroupSearchFilter(final String groupName) {
    +        final StringBuilder groupObjectFilter = new StringBuilder();
    +        groupObjectFilter.append("(objectClass=");
    +        groupObjectFilter.append(_ldapConfiguration.getGroupObject());
    +        groupObjectFilter.append(")");
    +
    +        final StringBuilder groupNameFilter = new StringBuilder();
    +        groupNameFilter.append("(");
    +        groupNameFilter.append(_ldapConfiguration.getCommonNameAttribute());
    +        groupNameFilter.append("=");
    +        groupNameFilter.append((groupName == null ? "*" : groupName));
    +        groupNameFilter.append(")");
    +
    +        final StringBuilder result = new StringBuilder();
    +        result.append("(&");
    +        result.append(groupObjectFilter);
    +        result.append(groupNameFilter);
    +        result.append(")");
    +
    +        return result.toString();
    +    }
    +
    +    @Override
    +    public LdapUser getUser(final String username, final LdapContext context) throws NamingException, IOException {
    +        List<LdapUser> result = searchUsers(username, context);
    +        if (result!= null && result.size() == 1) {
    +            return result.get(0);
    +        } else {
    +            throw new NamingException("No user found for username " + username);
    +        }
    +    }
    +
    +    @Override
    +    public List<LdapUser> getUsers(final LdapContext context) throws NamingException, IOException {
    +        return getUsers(null, context);
    +    }
    +
    +    @Override
    +    public List<LdapUser> getUsers(final String username, final LdapContext context) throws NamingException, IOException {
    +        List<LdapUser> users = searchUsers(username, context);
    +
    +        if (CollectionUtils.isNotEmpty(users)) {
    +            Collections.sort(users);
    +        }
    +        return users;
    +    }
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        String attributeName = _ldapConfiguration.getGroupUniqueMemeberAttribute();
    +        final SearchControls controls = new SearchControls();
    +        controls.setSearchScope(_ldapConfiguration.getScope());
    +        controls.setReturningAttributes(new String[] {attributeName});
    +
    +        NamingEnumeration<SearchResult> result = context.search(_ldapConfiguration.getBaseDn(), generateGroupSearchFilter(groupName), controls);
    +
    +        final List<LdapUser> users = new ArrayList<LdapUser>();
    +        //Expecting only one result which has all the users
    +        if (result.hasMoreElements()) {
    +            Attribute attribute = result.nextElement().getAttributes().get(attributeName);
    +            NamingEnumeration<?> values = attribute.getAll();
    +
    +            while (values.hasMoreElements()) {
    +                String userdn = String.valueOf(values.nextElement());
    +                try{
    +                    users.add(getUserForDn(userdn, context));
    +                } catch (NamingException e){
    +                    s_logger.info("Userdn: " + userdn + " Not Found:: Exception message: " + e.getMessage());
    +                }
    +            }
    +        }
    +
    +        Collections.sort(users);
    +
    +        return users;
    +    }
    +
    +    private LdapUser getUserForDn(String userdn, LdapContext context) throws NamingException {
    +        final SearchControls controls = new SearchControls();
    +        controls.setSearchScope(_ldapConfiguration.getScope());
    +        controls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        NamingEnumeration<SearchResult> result = context.search(userdn, "(objectClass=" + _ldapConfiguration.getUserObject() + ")", controls);
    +        if (result.hasMoreElements()) {
    +            return createUser(result.nextElement());
    +        } else {
    +            throw new NamingException("No user found for dn " + userdn);
    +        }
    +    }
    +
    +    @Override
    +    public List<LdapUser> searchUsers(final LdapContext context) throws NamingException, IOException {
    +        return searchUsers(null, context);
    +    }
    +
    +    @Override
    +    public List<LdapUser> searchUsers(final String username, final LdapContext context) throws NamingException, IOException {
    +
    +        final SearchControls searchControls = new SearchControls();
    +
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        String basedn = _ldapConfiguration.getBaseDn();
    --- End diff --
    
    Same as one of the previous comments


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35308521
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---
    @@ -0,0 +1,81 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.LdapContext;
    +
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager {
    +    public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName());
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        final SearchControls searchControls = new SearchControls();
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        String basedn = _ldapConfiguration.getBaseDn();
    +        if (StringUtils.isBlank(basedn)) {
    +            throw new IllegalArgumentException("ldap basedn is not configured");
    +        }
    +
    +        if (StringUtils.isBlank(groupName)) {
    --- End diff --
    
    done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35184810
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java ---
    @@ -43,14 +45,14 @@
         private ConfigurationDao _configDao;
     
         @Inject
    -    private LdapManager _ldapManager;
    +    private LdapConfigurationDao _ldapConfigurationDao;
     
         public LdapConfiguration() {
         }
     
    -    public LdapConfiguration(final ConfigurationDao configDao, final LdapManager ldapManager) {
    +    public LdapConfiguration(final ConfigurationDao configDao, final LdapConfigurationDao ldapConfigurationDao) {
             _configDao = configDao;
    -        _ldapManager = ldapManager;
    +        _ldapConfigurationDao = ldapConfigurationDao;
    --- End diff --
    
    constructor initialized ones were used in the unit tests to supply mocks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:

    https://github.com/apache/cloudstack/pull/609#issuecomment-124330906
  
    2 LGTMs and travis is green. I am merging this to master in sometime


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35292915
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
    @@ -57,17 +57,22 @@
         private LdapContextFactory _ldapContextFactory;
     
         @Inject
    -    private LdapUserManager _ldapUserManager;
    +    private LdapConfiguration _ldapConfiguration;
    +
    +    @Inject LdapUserManagerFactory _ldapUserManagerFactory;
    +
     
         public LdapManagerImpl() {
             super();
         }
     
    -    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManager ldapUserManager) {
    +    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManagerFactory ldapUserManagerFactory,
    --- End diff --
    
    this is not added now. its an existing one which is modified. I am planning on removing them in the future commits as there are other ways to inject them to tests.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35294032
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManagerFactory.java ---
    @@ -0,0 +1,55 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import org.apache.log4j.Logger;
    +import org.springframework.beans.BeansException;
    +import org.springframework.beans.factory.config.AutowireCapableBeanFactory;
    +import org.springframework.context.ApplicationContext;
    +import org.springframework.context.ApplicationContextAware;
    +
    +public class LdapUserManagerFactory implements ApplicationContextAware {
    +    public static final Logger s_logger = Logger.getLogger(LdapUserManagerFactory.class.getName());
    +
    +    private static LdapUserManager adUserManager;
    --- End diff --
    
    I will move them to a map


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:

    https://github.com/apache/cloudstack/pull/609#issuecomment-123954109
  
    @DaanHoogland @abhinandanprateek @runseb @wilderrodrigues @bhaisaab Can you please review this PR?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35291761
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java ---
    @@ -0,0 +1,81 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.cloudstack.ldap;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import javax.naming.NamingEnumeration;
    +import javax.naming.NamingException;
    +import javax.naming.directory.SearchControls;
    +import javax.naming.directory.SearchResult;
    +import javax.naming.ldap.LdapContext;
    +
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.log4j.Logger;
    +
    +public class ADLdapUserManagerImpl extends OpenLdapUserManagerImpl implements LdapUserManager {
    +    public static final Logger s_logger = Logger.getLogger(ADLdapUserManagerImpl.class.getName());
    +
    +    @Override
    +    public List<LdapUser> getUsersInGroup(String groupName, LdapContext context) throws NamingException {
    +        final SearchControls searchControls = new SearchControls();
    +        searchControls.setSearchScope(_ldapConfiguration.getScope());
    +        searchControls.setReturningAttributes(_ldapConfiguration.getReturnAttributes());
    +
    +        String basedn = _ldapConfiguration.getBaseDn();
    +        if (StringUtils.isBlank(basedn)) {
    +            throw new IllegalArgumentException("ldap basedn is not configured");
    --- End diff --
    
    basedn comes from global config. Currently, there is no way to ensure that this configuration is not null when ldap is enabled. This will be handled once I move all the ldap configurations to the ldap_configuration table and not the global configuration table.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/609#discussion_r35191354
  
    --- Diff: plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java ---
    @@ -57,17 +57,22 @@
         private LdapContextFactory _ldapContextFactory;
     
         @Inject
    -    private LdapUserManager _ldapUserManager;
    +    private LdapConfiguration _ldapConfiguration;
    +
    +    @Inject LdapUserManagerFactory _ldapUserManagerFactory;
    +
     
         public LdapManagerImpl() {
             super();
         }
     
    -    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManager ldapUserManager) {
    +    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManagerFactory ldapUserManagerFactory,
    +                           final LdapConfiguration ldapConfiguration) {
    --- End diff --
    
    It was initially done that way. We could inject as well. I did that in the new class.
    
    -----Original Message-----
    From: "Daan Hoogland" <no...@github.com>
    Sent: ‎22-‎07-‎2015 13:18
    To: "apache/cloudstack" <cl...@noreply.github.com>
    Cc: "Rajani Karuturi" <ra...@gmail.com>
    Subject: Re: [cloudstack] CLOUDSTACK-8596 ability to query nested groups forMicrosoft AD (#609)
    
    In plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java:
    >  
    >      public LdapManagerImpl() {
    >          super();
    >      }
    >  
    > -    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManager ldapUserManager) {
    > +    public LdapManagerImpl(final LdapConfigurationDao ldapConfigurationDao, final LdapContextFactory ldapContextFactory, final LdapUserManagerFactory ldapUserManagerFactory,
    > +                           final LdapConfiguration ldapConfiguration) {
    And why not use injection for that as well? Is this a groovy thing?
    —
    Reply to this email directly or view it on GitHub.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:

    https://github.com/apache/cloudstack/pull/609#issuecomment-124053986
  
    to run from eclipse, you need to install Groovy eclipse plugin https://code.google.com/p/spock/wiki/GettingStarted#Eclipse


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8596 ability to query nested g...

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:

    https://github.com/apache/cloudstack/pull/609#issuecomment-123668544
  
    @DaanHoogland now you can run the tests from the command line or ide 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---