From 7796128372aa7dd142050dccfacb37d7aeb2e8d0 Mon Sep 17 00:00:00 2001 From: Min Chen Date: Fri, 4 Apr 2014 11:48:55 -0700 Subject: [PATCH] Handle listAll flag in IAM buildAclSearchParameters. --- .../api/BaseListDomainResourcesCmd.java | 3 +- .../com/cloud/user/AccountManagerImpl.java | 73 +++++++++++-------- test/integration/smoke/test_vm_iam.py | 33 ++++++--- 3 files changed, 67 insertions(+), 42 deletions(-) diff --git a/api/src/org/apache/cloudstack/api/BaseListDomainResourcesCmd.java b/api/src/org/apache/cloudstack/api/BaseListDomainResourcesCmd.java index 79f7edc2297..3257d65d3b4 100644 --- a/api/src/org/apache/cloudstack/api/BaseListDomainResourcesCmd.java +++ b/api/src/org/apache/cloudstack/api/BaseListDomainResourcesCmd.java @@ -39,7 +39,8 @@ public abstract class BaseListDomainResourcesCmd extends BaseListCmd { } public boolean isRecursive() { - return recursive == null ? false : recursive; + // if listAll is true, recursive is not specified, then recursive should default to true. + return recursive == null ? (listAll() ? true : false) : recursive; } public Long getDomainId() { diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index bd7f6a62838..d4b67311d13 100755 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/server/src/com/cloud/user/AccountManagerImpl.java @@ -2164,39 +2164,47 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M public void buildACLSearchParameters(Account caller, Long id, String accountName, Long projectId, List permittedDomains, List permittedAccounts, List permittedResources, Ternary domainIdRecursiveListProject, boolean listAll, boolean forProjectInvitation, String action) { - //TODO: need to handle listAll flag + Long domainId = domainIdRecursiveListProject.first(); - if (domainId != null) { - // look for entity in the given domain - Domain domain = _domainDao.findById(domainId); - if (domain == null) { - throw new InvalidParameterValueException("Unable to find domain by id " + domainId); - } - // check permissions - checkAccess(caller, domain); - } - - if (accountName != null) { - if (projectId != null) { - throw new InvalidParameterValueException("Account and projectId can't be specified together"); - } - - Account userAccount = null; - Domain domain = null; + if (id == null) { + // if id is specified, it will ignore all other parameters if (domainId != null) { - userAccount = _accountDao.findActiveAccount(accountName, domainId); - domain = _domainDao.findById(domainId); - } else { - userAccount = _accountDao.findActiveAccount(accountName, caller.getDomainId()); - domain = _domainDao.findById(caller.getDomainId()); + // look for entity in the given domain + Domain domain = _domainDao.findById(domainId); + if (domain == null) { + throw new InvalidParameterValueException("Unable to find domain by id " + domainId); + } + // check permissions + checkAccess(caller, domain); } - if (userAccount != null) { - //check permissions - checkAccess(caller, null, userAccount); - permittedAccounts.add(userAccount.getId()); - } else { - throw new InvalidParameterValueException("could not find account " + accountName + " in domain " + domain.getUuid()); + // specific account is specified + if (accountName != null) { + if (projectId != null) { + throw new InvalidParameterValueException("Account and projectId can't be specified together"); + } + + Account userAccount = null; + Domain domain = null; + if (domainId != null) { + userAccount = _accountDao.findActiveAccount(accountName, domainId); + domain = _domainDao.findById(domainId); + } else { + userAccount = _accountDao.findActiveAccount(accountName, caller.getDomainId()); + domain = _domainDao.findById(caller.getDomainId()); + } + + if (userAccount != null) { + //check permissions + checkAccess(caller, null, userAccount); + permittedAccounts.add(userAccount.getId()); + } else { + throw new InvalidParameterValueException("could not find account " + accountName + " in domain " + domain.getUuid()); + } + } else if (!listAll && domainId == null) { + // listAll=false with no domain specified will only show caller owned resources + // if domainId is specified, we will filter the list based caller visible resources (owned + granted) + permittedAccounts.add(caller.getId()); } } @@ -2233,7 +2241,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M QuerySelector qs = _querySelectors.get(0); boolean grantedAll = qs.isGrantedAll(caller, action); if ( grantedAll ){ - if ( domainId != null ){ + if (permittedAccounts.isEmpty() && domainId != null) { permittedDomains.add(domainId); } } @@ -2253,6 +2261,11 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M permittedAccounts.add(acctId); } } + //TODO: we should also filter granted resources based on domainId passed. + // potential bug, if domainId is passed, it may show some granted resources that may not be in that domain. + // to fix this, we need to change the interface to also pass ControlledEntity class to use EntityManager to find + // ControlledEntity instance to check domainId. But this has some issues for those non controlled entities, + // like NetworkACLItem permittedResources.addAll(grantedResources); } } else if (permittedAccounts.isEmpty()) { diff --git a/test/integration/smoke/test_vm_iam.py b/test/integration/smoke/test_vm_iam.py index 80c049b0cd9..bdbe75a1742 100644 --- a/test/integration/smoke/test_vm_iam.py +++ b/test/integration/smoke/test_vm_iam.py @@ -289,7 +289,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1A_apikey self.apiclient.connection.securityKey = self.user_1A_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -312,7 +313,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -336,7 +338,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_2A_apikey self.apiclient.connection.securityKey = self.user_2A_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -378,7 +381,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -426,7 +430,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -479,7 +484,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -522,7 +528,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -563,7 +570,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -610,7 +618,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -653,7 +662,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list), @@ -695,7 +705,8 @@ class TestVMIam(cloudstackTestCase): self.apiclient.connection.apiKey = self.user_1B_apikey self.apiclient.connection.securityKey = self.user_1B_secretkey list_vm_response = list_virtual_machines( - self.apiclient + self.apiclient, + listall=True ) self.assertEqual( isinstance(list_vm_response, list),