Skip to content

Commit d68712e

Browse files
bwswGabrielBrascher
bwsw
authored and
GabrielBrascher
committed
CLOUDSTACK-3049: Implemented role update for account. (apache#3058)
1 parent 97ddd8d commit d68712e

File tree

8 files changed

+500
-182
lines changed

8 files changed

+500
-182
lines changed

‎.dockerignore

+1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
# under the License.
1717

1818
cloudstack/tools/docker/Dockerfile
19+
cloudstack/tools/docker/Dockerfile.smokedev
1920
.dockerignore
2021
.idea
2122
.git

‎api/src/main/java/org/apache/cloudstack/api/command/admin/account/UpdateAccountCmd.java

+11-5
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import javax.inject.Inject;
2323

24+
import org.apache.cloudstack.api.response.RoleResponse;
2425
import org.apache.log4j.Logger;
2526

2627
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
@@ -48,24 +49,27 @@ public class UpdateAccountCmd extends BaseCmd {
4849
//////////////// API parameters /////////////////////
4950
/////////////////////////////////////////////////////
5051
@ACL(accessType = AccessType.OperateEntry)
51-
@Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "Account id")
52+
@Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "Account UUID")
5253
private Long id;
5354

54-
@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "the current account name")
55+
@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "Current account name")
5556
private String accountName;
5657

57-
@Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "the ID of the domain where the account exists")
58+
@Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The UUID of the domain where the account exists")
5859
private Long domainId;
5960

60-
@Parameter(name = ApiConstants.NEW_NAME, type = CommandType.STRING, required = true, description = "new name for the account")
61+
@Parameter(name = ApiConstants.ROLE_ID, type = CommandType.UUID, entityType = RoleResponse.class, description = "The UUID of the dynamic role to set for the account")
62+
private Long roleId;
63+
64+
@Parameter(name = ApiConstants.NEW_NAME, type = CommandType.STRING, description = "New name for the account")
6165
private String newName;
6266

6367
@Parameter(name = ApiConstants.NETWORK_DOMAIN,
6468
type = CommandType.STRING,
6569
description = "Network domain for the account's networks; empty string will update domainName with NULL value")
6670
private String networkDomain;
6771

68-
@Parameter(name = ApiConstants.ACCOUNT_DETAILS, type = CommandType.MAP, description = "details for account used to store specific parameters")
72+
@Parameter(name = ApiConstants.ACCOUNT_DETAILS, type = CommandType.MAP, description = "Details for the account used to store specific parameters")
6973
private Map details;
7074

7175
@Inject
@@ -87,6 +91,8 @@ public Long getDomainId() {
8791
return domainId;
8892
}
8993

94+
public Long getRoleId() { return roleId; }
95+
9096
public String getNewName() {
9197
return newName;
9298
}

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

+51-23
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,10 @@
3737
import javax.inject.Inject;
3838
import javax.naming.ConfigurationException;
3939

40+
4041
import org.apache.cloudstack.acl.ControlledEntity;
4142
import org.apache.cloudstack.acl.QuerySelector;
43+
import org.apache.cloudstack.acl.Role;
4244
import org.apache.cloudstack.acl.RoleType;
4345
import org.apache.cloudstack.acl.SecurityChecker;
4446
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
@@ -1019,8 +1021,11 @@ public UserAccount createUserAccount(final String userName, final String passwor
10191021
@DB
10201022
@ActionEvents({@ActionEvent(eventType = EventTypes.EVENT_ACCOUNT_CREATE, eventDescription = "creating Account"),
10211023
@ActionEvent(eventType = EventTypes.EVENT_USER_CREATE, eventDescription = "creating User")})
1022-
public UserAccount createUserAccount(final String userName, final String password, final String firstName, final String lastName, final String email, final String timezone, String accountName,
1023-
final short accountType, final Long roleId, Long domainId, final String networkDomain, final Map<String, String> details, String accountUUID, final String userUUID,
1024+
public UserAccount createUserAccount(final String userName, final String password, final String firstName,
1025+
final String lastName, final String email, final String timezone,
1026+
String accountName, final short accountType, final Long roleId, Long domainId,
1027+
final String networkDomain, final Map<String, String> details,
1028+
String accountUUID, final String userUUID,
10241029
final User.Source source) {
10251030

10261031
if (accountName == null) {
@@ -1155,7 +1160,7 @@ public UserAccount updateUser(UpdateUserCmd updateUserCmd) {
11551160
UserVO user = retrieveAndValidateUser(updateUserCmd);
11561161
s_logger.debug("Updating user with Id: " + user.getUuid());
11571162

1158-
validateAndUpdatApiAndSecretKeyIfNeeded(updateUserCmd, user);
1163+
validateAndUpdateApiAndSecretKeyIfNeeded(updateUserCmd, user);
11591164
Account account = retrieveAndValidateAccount(user);
11601165

11611166
validateAndUpdateFirstNameIfNeeded(updateUserCmd, user);
@@ -1344,7 +1349,7 @@ protected Account getCurrentCallingAccount() {
13441349
* <li>If a pair of keys is provided, we validate to see if there is an user already using the provided API key. If there is someone else using, we throw an {@link InvalidParameterValueException} because two users cannot have the same API key.
13451350
* </ul>
13461351
*/
1347-
protected void validateAndUpdatApiAndSecretKeyIfNeeded(UpdateUserCmd updateUserCmd, UserVO user) {
1352+
protected void validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmd updateUserCmd, UserVO user) {
13481353
String apiKey = updateUserCmd.getApiKey();
13491354
String secretKey = updateUserCmd.getSecretKey();
13501355

@@ -1687,6 +1692,7 @@ public AccountVO disableAccount(String accountName, Long domainId, Long accountI
16871692
public AccountVO updateAccount(UpdateAccountCmd cmd) {
16881693
Long accountId = cmd.getId();
16891694
Long domainId = cmd.getDomainId();
1695+
Long roleId = cmd.getRoleId();
16901696
String accountName = cmd.getAccountName();
16911697
String newAccountName = cmd.getNewName();
16921698
String networkDomain = cmd.getNetworkDomain();
@@ -1700,6 +1706,8 @@ public AccountVO updateAccount(UpdateAccountCmd cmd) {
17001706
account = _accountDao.findEnabledAccount(accountName, domainId);
17011707
}
17021708

1709+
final AccountVO acctForUpdate = _accountDao.findById(account.getId());
1710+
17031711
// Check if account exists
17041712
if (account == null || account.getType() == Account.ACCOUNT_TYPE_PROJECT) {
17051713
s_logger.error("Unable to find account by accountId: " + accountId + " OR by name: " + accountName + " in domain " + domainId);
@@ -1712,25 +1720,48 @@ public AccountVO updateAccount(UpdateAccountCmd cmd) {
17121720
}
17131721

17141722
// Check if user performing the action is allowed to modify this account
1715-
checkAccess(getCurrentCallingAccount(), _domainMgr.getDomain(account.getDomainId()));
1723+
Account caller = getCurrentCallingAccount();
1724+
checkAccess(caller, _domainMgr.getDomain(account.getDomainId()));
17161725

1717-
// check if the given account name is unique in this domain for updating
1718-
Account duplicateAcccount = _accountDao.findActiveAccount(newAccountName, domainId);
1719-
if (duplicateAcccount != null && duplicateAcccount.getId() != account.getId()) {
1720-
throw new InvalidParameterValueException(
1721-
"There already exists an account with the name:" + newAccountName + " in the domain:" + domainId + " with existing account id:" + duplicateAcccount.getId());
1726+
if(newAccountName != null) {
1727+
1728+
if (newAccountName.isEmpty()) {
1729+
throw new InvalidParameterValueException("The new account name for account '" + account.getUuid() + "' " +
1730+
"within domain '" + domainId + "' is empty string. Account will be not renamed.");
1731+
}
1732+
1733+
// check if the new proposed account name is absent in the domain
1734+
Account existingAccount = _accountDao.findActiveAccount(newAccountName, domainId);
1735+
if (existingAccount != null && existingAccount.getId() != account.getId()) {
1736+
throw new InvalidParameterValueException("The account with the proposed name '" +
1737+
newAccountName + "' exists in the domain '" +
1738+
domainId + "' with existing account id '" + existingAccount.getId() + "'");
1739+
}
1740+
1741+
acctForUpdate.setAccountName(newAccountName);
17221742
}
17231743

17241744
if (networkDomain != null && !networkDomain.isEmpty()) {
17251745
if (!NetUtils.verifyDomainName(networkDomain)) {
1726-
throw new InvalidParameterValueException(
1727-
"Invalid network domain. Total length shouldn't exceed 190 chars. Each domain label must be between 1 and 63 characters long, can contain ASCII letters 'a' through 'z', the digits '0' through '9', "
1746+
throw new InvalidParameterValueException("Invalid network domain or format. " +
1747+
"Total length shouldn't exceed 190 chars. Every domain part must be between 1 and 63 " +
1748+
"characters long, can contain ASCII letters 'a' through 'z', the digits '0' through '9', "
17281749
+ "and the hyphen ('-'); can't start or end with \"-\"");
17291750
}
17301751
}
17311752

1732-
final AccountVO acctForUpdate = _accountDao.findById(account.getId());
1733-
acctForUpdate.setAccountName(newAccountName);
1753+
1754+
if (roleId != null) {
1755+
final List<Role> roles = cmd.roleService.listRoles();
1756+
final boolean roleNotFound = roles.stream().filter(r -> r.getId() == roleId).count() == 0;
1757+
if (roleNotFound) {
1758+
throw new InvalidParameterValueException("Role with ID '" + roleId.toString() + "' " +
1759+
"is not found or not available for the account '" + account.getUuid() + "' " +
1760+
"in the domain '" + domainId + "'.");
1761+
}
1762+
1763+
acctForUpdate.setRoleId(roleId);
1764+
}
17341765

17351766
if (networkDomain != null) {
17361767
if (networkDomain.isEmpty()) {
@@ -1741,17 +1772,14 @@ public AccountVO updateAccount(UpdateAccountCmd cmd) {
17411772
}
17421773

17431774
final Account accountFinal = account;
1744-
success = Transaction.execute(new TransactionCallback<Boolean>() {
1745-
@Override
1746-
public Boolean doInTransaction(TransactionStatus status) {
1747-
boolean success = _accountDao.update(accountFinal.getId(), acctForUpdate);
1748-
1749-
if (details != null && success) {
1750-
_accountDetailsDao.update(accountFinal.getId(), details);
1751-
}
1775+
success = Transaction.execute((TransactionCallback<Boolean>) status -> {
1776+
boolean success1 = _accountDao.update(accountFinal.getId(), acctForUpdate);
17521777

1753-
return success;
1778+
if (details != null && success1) {
1779+
_accountDetailsDao.update(accountFinal.getId(), details);
17541780
}
1781+
1782+
return success1;
17551783
});
17561784

17571785
if (success) {

‎server/src/test/java/com/cloud/user/AccountManagerImplTest.java

+7-7
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ public void updateUserTestTimeZoneAndEmailNotNull() {
226226

227227
private void prepareMockAndExecuteUpdateUserTest(int numberOfExpectedCallsForSetEmailAndSetTimeZone) {
228228
Mockito.doReturn(userVoMock).when(accountManagerImpl).retrieveAndValidateUser(UpdateUserCmdMock);
229-
Mockito.doNothing().when(accountManagerImpl).validateAndUpdatApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
229+
Mockito.doNothing().when(accountManagerImpl).validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
230230
Mockito.doReturn(accountMock).when(accountManagerImpl).retrieveAndValidateAccount(userVoMock);
231231

232232
Mockito.doNothing().when(accountManagerImpl).validateAndUpdateFirstNameIfNeeded(UpdateUserCmdMock, userVoMock);
@@ -242,7 +242,7 @@ private void prepareMockAndExecuteUpdateUserTest(int numberOfExpectedCallsForSet
242242
InOrder inOrder = Mockito.inOrder(userVoMock, accountManagerImpl, userDaoMock, userAccountDaoMock);
243243

244244
inOrder.verify(accountManagerImpl).retrieveAndValidateUser(UpdateUserCmdMock);
245-
inOrder.verify(accountManagerImpl).validateAndUpdatApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
245+
inOrder.verify(accountManagerImpl).validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
246246
inOrder.verify(accountManagerImpl).retrieveAndValidateAccount(userVoMock);
247247

248248
inOrder.verify(accountManagerImpl).validateAndUpdateFirstNameIfNeeded(UpdateUserCmdMock, userVoMock);
@@ -275,7 +275,7 @@ public void retrieveAndValidateUserTestUserIsFound() {
275275

276276
@Test
277277
public void validateAndUpdatApiAndSecretKeyIfNeededTestNoKeys() {
278-
accountManagerImpl.validateAndUpdatApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
278+
accountManagerImpl.validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
279279

280280
Mockito.verify(accountDaoMock, Mockito.times(0)).findUserAccountByApiKey(Mockito.anyString());
281281
}
@@ -284,14 +284,14 @@ public void validateAndUpdatApiAndSecretKeyIfNeededTestNoKeys() {
284284
public void validateAndUpdatApiAndSecretKeyIfNeededTestOnlyApiKeyInformed() {
285285
Mockito.doReturn("apiKey").when(UpdateUserCmdMock).getApiKey();
286286

287-
accountManagerImpl.validateAndUpdatApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
287+
accountManagerImpl.validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
288288
}
289289

290290
@Test(expected = InvalidParameterValueException.class)
291291
public void validateAndUpdatApiAndSecretKeyIfNeededTestOnlySecretKeyInformed() {
292292
Mockito.doReturn("secretKey").when(UpdateUserCmdMock).getSecretKey();
293293

294-
accountManagerImpl.validateAndUpdatApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
294+
accountManagerImpl.validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
295295
}
296296

297297
@Test(expected = InvalidParameterValueException.class)
@@ -308,7 +308,7 @@ public void validateAndUpdatApiAndSecretKeyIfNeededTestApiKeyAlreadyUsedBySomeon
308308
Pair<User, Account> pairUserAccountMock = new Pair<User, Account>(otherUserMock, Mockito.mock(Account.class));
309309
Mockito.doReturn(pairUserAccountMock).when(accountDaoMock).findUserAccountByApiKey(apiKey);
310310

311-
accountManagerImpl.validateAndUpdatApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
311+
accountManagerImpl.validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
312312
}
313313

314314
@Test
@@ -327,7 +327,7 @@ public void validateAndUpdatApiAndSecretKeyIfNeededTest() {
327327
Pair<User, Account> pairUserAccountMock = new Pair<User, Account>(otherUserMock, Mockito.mock(Account.class));
328328
Mockito.doReturn(pairUserAccountMock).when(accountDaoMock).findUserAccountByApiKey(apiKey);
329329

330-
accountManagerImpl.validateAndUpdatApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
330+
accountManagerImpl.validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock);
331331

332332
Mockito.verify(accountDaoMock).findUserAccountByApiKey(apiKey);
333333
Mockito.verify(userVoMock).setApiKey(apiKey);

‎test/integration/smoke/test_accounts.py

+53-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
User,
3131
NATRule,
3232
Template,
33-
PublicIPAddress)
33+
PublicIPAddress, Role)
3434
from marvin.lib.common import (get_domain,
3535
get_zone,
3636
get_test_template,
@@ -67,6 +67,11 @@ def __init__(self):
6767
# username
6868
"password": "fr3sca",
6969
},
70+
"role": {
71+
"name": "MarvinFake Role",
72+
"type": "User",
73+
"description": "Fake Role created by Marvin test"
74+
},
7075
"user": {
7176
"email": "[email protected]",
7277
"firstname": "User",
@@ -261,6 +266,53 @@ def test_01_create_account(self):
261266
return
262267

263268

269+
@attr(tags=["advanced", "basic", "eip", "advancedns", "sg"],
270+
required_hardware="false")
271+
def test_02_update_account(self):
272+
"""
273+
Tests that accounts can be updated with new name, network domain, dynamic role
274+
:return:
275+
"""
276+
dynamic_roles_active = self.apiclient.listCapabilities(listCapabilities.listCapabilitiesCmd()).dynamicrolesenabled
277+
if not dynamic_roles_active:
278+
self.skipTest("Dynamic Role-Based API checker not enabled, skipping test")
279+
280+
ts = str(time.time())
281+
network_domain = 'mycloud.com'
282+
283+
account = Account.create(self.apiclient, self.services['account'])
284+
self.cleanup.append(account)
285+
286+
role = Role.create(self.apiclient, self.services['role'])
287+
self.cleanup.append(role)
288+
289+
account.update(self.apiclient, newname=account.name + ts)
290+
account.update(self.apiclient, roleid=role.id)
291+
account.update(self.apiclient, networkdomain=network_domain)
292+
293+
list_accounts_response = list_accounts(self.apiclient, id=account.id)
294+
test_account = list_accounts_response[0]
295+
296+
self.assertEqual(
297+
test_account.roleid, role.id,
298+
"Check the role for the account is changed")
299+
300+
self.assertEqual(
301+
test_account.networkdomain, network_domain,
302+
"Check the domain for the account is changed")
303+
304+
self.assertEqual(
305+
test_account.name, account.name + ts,
306+
"Check the name for the account is changed")
307+
308+
try:
309+
account.update(self.apiclient, newname="")
310+
self.fail("Account name change to empty name succeeded. Must be error.")
311+
except CloudstackAPIException:
312+
pass
313+
314+
315+
264316
class TestRemoveUserFromAccount(cloudstackTestCase):
265317

266318
@classmethod

0 commit comments

Comments
 (0)