From f4779b4d0ce9fddc80c8df91b594e3991b56af58 Mon Sep 17 00:00:00 2001 From: Rajani Karuturi Date: Fri, 25 Apr 2014 16:42:39 +0530 Subject: [PATCH] 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 --- api/src/com/cloud/user/AccountService.java | 8 +++ .../management/MockAccountManager.java | 6 ++ .../api/command/LdapCreateAccountCmd.java | 63 ++++++++++++------- .../api/command/LdapImportUsersCmd.java | 26 +++++++- .../ldap/LdapImportUsersCmdSpec.groovy | 60 ++++++++++++++++++ .../com/cloud/user/AccountManagerImpl.java | 5 ++ .../cloud/user/MockAccountManagerImpl.java | 6 ++ ui/scripts/accountsWizard.js | 6 +- 8 files changed, 152 insertions(+), 28 deletions(-) diff --git a/api/src/com/cloud/user/AccountService.java b/api/src/com/cloud/user/AccountService.java index 71136bf836a..10be650d5c7 100755 --- a/api/src/com/cloud/user/AccountService.java +++ b/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); + } diff --git a/plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java b/plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java index cad5a503d9f..3517cc9c697 100644 --- a/plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java +++ b/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 diff --git a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java index 626bb8fee62..b753952aec4 100644 --- a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java +++ b/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", - requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) +@APICommand(name = "ldapCreateAccount", description = "Creates an account from an LDAP user", responseObject = AccountResponse.class, since = "4.2.0", 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, - 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.") + @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; - @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") + @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") 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, - type = CommandType.STRING, - description = "Specifies a timezone for this command. For more information on the timezone parameter, see Time Zone Format.") + @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.") 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) { - return _accountService.createUserAccount(username, generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(), timezone, accountName, - accountType, domainId, networkDomain, details, accountUUID, userUUID); + UserAccount createCloudstackUserAccount(final LdapUser user, String accountName, Long domainId) { + Account account = _accountService.getActiveAccountByName(accountName, domainId); + 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 - .createUserAccountResponse(ResponseView.Full, userAccount); + final AccountResponse response = _responseGenerator.createUserAccountResponse(ResponseView.Full, userAccount); response.setResponseName(getCommandName()); setResponseObject(response); } else { @@ -159,4 +176,4 @@ public class LdapCreateAccountCmd extends BaseCmd { } return true; } -} \ No newline at end of file +} diff --git a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java index 887ad009fd3..6f7be90fd2a 100644 --- a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java +++ b/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, - user.getUsername(), accountType, domain.getId(), domain.getNetworkDomain(), details, UUID.randomUUID().toString(), UUID.randomUUID().toString()); + createCloudstackUserAccount(user, getAccountName(user), domain); 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)) { diff --git a/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapImportUsersCmdSpec.groovy b/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapImportUsersCmdSpec.groovy index a66da1f3cff..79eba93d89e 100644 --- a/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapImportUsersCmdSpec.groovy +++ b/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 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 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" + } + } diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index 06c55aa1533..32e28f73dec 100755 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/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); + } } diff --git a/server/test/com/cloud/user/MockAccountManagerImpl.java b/server/test/com/cloud/user/MockAccountManagerImpl.java index 2f57abcefe4..81ee1a4e056 100644 --- a/server/test/com/cloud/user/MockAccountManagerImpl.java +++ b/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 diff --git a/ui/scripts/accountsWizard.js b/ui/scripts/accountsWizard.js index 6b4907c1cb1..d84fbfa59bd 100644 --- a/ui/scripts/accountsWizard.js +++ b/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