Fixed CLOUDSTACK-6509 Cannot import multiple LDAP/AD users into a cloudstack account

Conflicts:
	api/src/com/cloud/user/AccountService.java
	plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java
	plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java

Signed-off-by: Koushik Das <koushik@apache.org>

api/src/com/cloud/user/AccountService.java

@@ -114,4 +114,12 @@ public interface AccountService {
     void checkAccess(Account caller, AccessType accessType, boolean sameOwner, PartOf... entities) throws PermissionDeniedException;
 
     Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly);
+
+    /**
+     * returns the user account object for a given user id
+     * @param userId user id
+     * @return useraccount object if it exists else null
+     */
+    UserAccount getUserAccountById(Long userId);
+
 }

plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java

@@ -95,6 +95,12 @@ public class MockAccountManager extends ManagerBase implements AccountManager {
 
     }
 
+    @Override
+    public UserAccount getUserAccountById(Long userId) {
+        // TODO Auto-generated method stub
+        return null;
+    }
+
     @Override
     public String[] createApiKeyAndSecretKey(RegisterCmd arg0) {
         // TODO Auto-generated method stub

plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java

@@ -23,9 +23,6 @@ import java.util.Map;
 import javax.inject.Inject;
 import javax.naming.NamingException;
 
-import org.apache.log4j.Logger;
-import org.bouncycastle.util.encoders.Base64;
-
 import org.apache.cloudstack.api.APICommand;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.api.ApiErrorCode;
@@ -38,13 +35,16 @@ import org.apache.cloudstack.api.response.DomainResponse;
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.ldap.LdapManager;
 import org.apache.cloudstack.ldap.LdapUser;
+import org.apache.log4j.Logger;
+import org.bouncycastle.util.encoders.Base64;
 
+import com.cloud.domain.DomainVO;
 import com.cloud.user.Account;
 import com.cloud.user.AccountService;
+import com.cloud.user.User;
 import com.cloud.user.UserAccount;
 
-@APICommand(name = "ldapCreateAccount", description = "Creates an account from an LDAP user", responseObject = AccountResponse.class, since = "4.2.0",
+@APICommand(name = "ldapCreateAccount", description = "Creates an account from an LDAP user", responseObject = AccountResponse.class, since = "4.2.0", requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
-        requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
 public class LdapCreateAccountCmd extends BaseCmd {
     public static final Logger s_logger = Logger.getLogger(LdapCreateAccountCmd.class.getName());
     private static final String s_name = "createaccountresponse";
@@ -52,23 +52,16 @@ public class LdapCreateAccountCmd extends BaseCmd {
     @Inject
     private LdapManager _ldapManager;
 
-    @Parameter(name = ApiConstants.ACCOUNT,
+    @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "Creates the user under the specified account. If no account is specified, the username will be used as the account name.")
-            type = CommandType.STRING,
-            description = "Creates the user under the specified account. If no account is specified, the username will be used as the account name.")
     private String accountName;
 
-    @Parameter(name = ApiConstants.ACCOUNT_TYPE,
+    @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account.  Specify 0 for user, 1 for root admin, and 2 for domain admin")
-            type = CommandType.SHORT,
-            required = true,
-            description = "Type of the account.  Specify 0 for user, 1 for root admin, and 2 for domain admin")
     private Short accountType;
 
     @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "Creates the user under the specified domain.")
     private Long domainId;
 
-    @Parameter(name = ApiConstants.TIMEZONE,
+    @Parameter(name = ApiConstants.TIMEZONE, type = CommandType.STRING, description = "Specifies a timezone for this command. For more information on the timezone parameter, see Time Zone Format.")
-            type = CommandType.STRING,
-            description = "Specifies a timezone for this command. For more information on the timezone parameter, see Time Zone Format.")
     private String timezone;
 
     @Parameter(name = ApiConstants.USERNAME, type = CommandType.STRING, required = true, description = "Unique username.")
@@ -96,22 +89,46 @@ public class LdapCreateAccountCmd extends BaseCmd {
         _accountService = accountService;
     }
 
-    UserAccount createCloudstackUserAccount(final LdapUser user) {
+    UserAccount createCloudstackUserAccount(final LdapUser user, String accountName, Long domainId) {
-        return _accountService.createUserAccount(username, generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(), timezone, accountName,
+        Account account = _accountService.getActiveAccountByName(accountName, domainId);
-                accountType, domainId, networkDomain, details, accountUUID, userUUID);
+        if (account == null) {
+            return _accountService.createUserAccount(username, generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(), timezone, accountName, accountType,
+                    domainId, networkDomain, details, accountUUID, userUUID);
+        } else {
+            User newUser = _accountService.createUser(username, generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(), timezone, accountName, domainId,
+                    userUUID);
+            return _accountService.getUserAccountById(newUser.getId());
+        }
+    }
+
+    private String getAccountName() {
+        String name = accountName;
+        if (accountName == null) {
+            name = username;
+        }
+        return name;
+    }
+
+    private Long getDomainId() {
+        Long id = domainId;
+        if (id == null) {
+            id = DomainVO.ROOT_DOMAIN;
+        }
+        return id;
     }
 
     @Override
     public void execute() throws ServerApiException {
         final CallContext callContext = getCurrentContext();
-        callContext.setEventDetails("Account Name: " + accountName + ", Domain Id:" + domainId);
+        String finalAccountName = getAccountName();
+        Long finalDomainId = getDomainId();
+        callContext.setEventDetails("Account Name: " + finalAccountName + ", Domain Id:" + finalDomainId);
         try {
             final LdapUser user = _ldapManager.getUser(username);
             validateUser(user);
-            final UserAccount userAccount = createCloudstackUserAccount(user);
+            final UserAccount userAccount = createCloudstackUserAccount(user, finalAccountName, finalDomainId);
             if (userAccount != null) {
-                final AccountResponse response = _responseGenerator
+                final AccountResponse response = _responseGenerator.createUserAccountResponse(ResponseView.Full, userAccount);
-                        .createUserAccountResponse(ResponseView.Full, userAccount);
                 response.setResponseName(getCommandName());
                 setResponseObject(response);
             } else {
@@ -159,4 +176,4 @@ public class LdapCreateAccountCmd extends BaseCmd {
         }
         return true;
     }
-}
+}

plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java

@@ -25,6 +25,7 @@ import java.util.UUID;
 
 import javax.inject.Inject;
 
+import com.cloud.user.Account;
 import org.apache.cloudstack.api.APICommand;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.api.ApiErrorCode;
@@ -87,6 +88,9 @@ public class LdapImportUsersCmd extends BaseListCmd {
 
     private Domain _domain;
 
+    @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "Creates the user under the specified account. If no account is specified, the username will be used as the account name.")
+    private String accountName;
+
     @Inject
     private LdapManager _ldapManager;
 
@@ -101,6 +105,17 @@ public class LdapImportUsersCmd extends BaseListCmd {
         _accountService = accountService;
     }
 
+    private void createCloudstackUserAccount(LdapUser user, String accountName, Domain domain) {
+        Account account = _accountService.getActiveAccountByName(accountName, domain.getId());
+        if (account == null) {
+            _accountService.createUserAccount(user.getUsername(), generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(), timezone, accountName, accountType,
+                    domain.getId(), domain.getNetworkDomain(), details, UUID.randomUUID().toString(), UUID.randomUUID().toString());
+        } else {
+            _accountService.createUser(user.getUsername(), generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(), timezone, accountName, domain.getId(),
+                    UUID.randomUUID().toString());
+        }
+    }
+
     @Override
     public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException,
         ResourceAllocationException, NetworkRuleConflictException {
@@ -122,8 +137,7 @@ public class LdapImportUsersCmd extends BaseListCmd {
         for (LdapUser user : users) {
             Domain domain = getDomain(user);
             try {
-                _accountService.createUserAccount(user.getUsername(), generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(), timezone,
+                createCloudstackUserAccount(user, getAccountName(user), domain);
-                    user.getUsername(), accountType, domain.getId(), domain.getNetworkDomain(), details, UUID.randomUUID().toString(), UUID.randomUUID().toString());
                 addedUsers.add(user);
             } catch (InvalidParameterValueException ex) {
                 s_logger.error("Failed to create user with username: " + user.getUsername() + " ::: " + ex.getMessage());
@@ -135,6 +149,14 @@ public class LdapImportUsersCmd extends BaseListCmd {
         setResponseObject(response);
     }
 
+    private String getAccountName(LdapUser user) {
+        String finalAccountName = accountName;
+        if(finalAccountName == null ) {
+            finalAccountName = user.getUsername();
+        }
+        return finalAccountName;
+    }
+
     private Domain getDomainForName(String name) {
         Domain domain = null;
         if (StringUtils.isNotBlank(name)) {

plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapImportUsersCmdSpec.groovy

@@ -19,9 +19,14 @@ package groovy.org.apache.cloudstack.ldap
 import com.cloud.domain.Domain
 import com.cloud.domain.DomainVO
 import com.cloud.user.AccountService
+import com.cloud.user.AccountVO
 import com.cloud.user.DomainService
+import com.cloud.user.UserAccountVO
+import com.cloud.user.UserVO
+import org.apache.cloudstack.api.command.LdapCreateAccountCmd
 import org.apache.cloudstack.api.command.LdapImportUsersCmd
 import org.apache.cloudstack.api.response.LdapUserResponse
+import org.apache.cloudstack.context.CallContext
 import org.apache.cloudstack.ldap.LdapManager
 import org.apache.cloudstack.ldap.LdapUser
 
@@ -193,4 +198,59 @@ class LdapImportUsersCmdSpec extends spock.lang.Specification {
 
     }
 
+
+    def "Test create ldap import account for an already existing cloudstack account"() {
+        given: "We have an LdapManager, DomainService, two users and a LdapImportUsersCmd"
+        def ldapManager = Mock(LdapManager)
+        List<LdapUser> users = new ArrayList()
+        users.add(new LdapUser("rmurphy", "rmurphy@test.com", "Ryan", "Murphy", "cn=rmurphy,ou=engineering,dc=cloudstack,dc=org", "engineering"))
+        ldapManager.getUsers() >> users
+        LdapUserResponse response1 = new LdapUserResponse("rmurphy", "rmurphy@test.com", "Ryan", "Murphy", "cn=rmurphy,ou=engineering,dc=cloudstack,dc=org", "engineering")
+        ldapManager.createLdapUserResponse(_) >>> response1
+
+        def domainService = Mock(DomainService)
+        1 * domainService.getDomain(1L) >> new DomainVO("DOMAIN", 1L, 1L, "DOMAIN", UUID.randomUUID().toString());;
+
+        def accountService = Mock(AccountService)
+        1 * accountService.getActiveAccountByName('ACCOUNT', 0) >>  Mock(AccountVO)
+        1 * accountService.createUser('rmurphy', _ , 'Ryan', 'Murphy', 'rmurphy@test.com', null, 'ACCOUNT', 0, _) >> Mock(UserVO)
+        0 * accountService.createUserAccount('rmurphy', _, 'Ryan', 'Murphy', 'rmurphy@test.com', null, 'ACCOUNT', 2, 0, 'DOMAIN', null, _, _)
+
+        def ldapImportUsersCmd = new LdapImportUsersCmd(ldapManager, domainService, accountService)
+        ldapImportUsersCmd.accountName = "ACCOUNT"
+        ldapImportUsersCmd.accountType = 2;
+        ldapImportUsersCmd.domainId = 1L;
+
+        when: "create account is called"
+        ldapImportUsersCmd.execute()
+        then: "expect 1 call on accountService createUser and 0 on account service create user account"
+    }
+
+    def "Test create ldap import account for a new cloudstack account"() {
+        given: "We have an LdapManager, DomainService, two users and a LdapImportUsersCmd"
+        def ldapManager = Mock(LdapManager)
+        List<LdapUser> users = new ArrayList()
+        users.add(new LdapUser("rmurphy", "rmurphy@test.com", "Ryan", "Murphy", "cn=rmurphy,ou=engineering,dc=cloudstack,dc=org", "engineering"))
+        ldapManager.getUsers() >> users
+        LdapUserResponse response1 = new LdapUserResponse("rmurphy", "rmurphy@test.com", "Ryan", "Murphy", "cn=rmurphy,ou=engineering,dc=cloudstack,dc=org", "engineering")
+        ldapManager.createLdapUserResponse(_) >>> response1
+
+        def domainService = Mock(DomainService)
+        1 * domainService.getDomain(1L) >> new DomainVO("DOMAIN", 1L, 1L, "DOMAIN", UUID.randomUUID().toString());;
+
+        def accountService = Mock(AccountService)
+        1 * accountService.getActiveAccountByName('ACCOUNT', 0) >>  null
+        0 * accountService.createUser('rmurphy', _ , 'Ryan', 'Murphy', 'rmurphy@test.com', null, 'ACCOUNT', 0, _)
+        1 * accountService.createUserAccount('rmurphy', _, 'Ryan', 'Murphy', 'rmurphy@test.com', null, 'ACCOUNT', 2, 0, 'DOMAIN', null, _, _)
+
+        def ldapImportUsersCmd = new LdapImportUsersCmd(ldapManager, domainService, accountService)
+        ldapImportUsersCmd.accountName = "ACCOUNT"
+        ldapImportUsersCmd.accountType = 2;
+        ldapImportUsersCmd.domainId = 1L;
+
+        when: "create account is called"
+        ldapImportUsersCmd.execute()
+        then: "expect 1 call on accountService createUser and 0 on account service create user account"
+    }
+
 }

server/src/com/cloud/user/AccountManagerImpl.java

@@ -2623,4 +2623,9 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
         }
         return null;
     }
+
+    @Override
+    public UserAccount getUserAccountById(Long userId) {
+        return _userAccountDao.findById(userId);
+    }
 }

server/test/com/cloud/user/MockAccountManagerImpl.java

@@ -226,6 +226,12 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco
 
     }
 
+    @Override
+    public UserAccount getUserAccountById(Long userId) {
+        // TODO Auto-generated method stub
+        return null;
+    }
+
     @Override
     public void logoutUser(long userId) {
         // TODO Auto-generated method stub

ui/scripts/accountsWizard.js

@@ -198,10 +198,10 @@
             array1.push("&domainid=" + args.data.domainid);
 
             var account = args.data.account;
-            if (account === null || account.length === 0) {
+
-                account = args.username;
+            if (account !== null && account.length > 0) {
+                array1.push("&account=" + account);
             }
-            array1.push("&account=" + account);
 
             var accountType = args.data.accounttype;
             if (args.data.accounttype == "1" && args.data.domainid != rootDomainId) { //if account type is admin, but domain is not Root domain