From 5d59fc7f5a70a4d1fc44a9118d8d582f14563095 Mon Sep 17 00:00:00 2001 From: Min Chen Date: Fri, 4 Apr 2014 18:44:32 -0700 Subject: [PATCH] Fix RoleBasedQuerySelector to handle new listAll semantics. If listAll=true, show all resources that caller (or impersonater) has ListEntry access type; otherwise, show all resources that caller (or impersonater) has UseEntry access type. --- .../apache/cloudstack/acl/QuerySelector.java | 10 ++-- .../com/cloud/user/AccountManagerImpl.java | 32 ++++++----- .../iam/RoleBasedEntityQuerySelector.java | 53 +++++++++++++++---- .../apache/cloudstack/iam/api/IAMService.java | 3 +- .../cloudstack/iam/server/IAMServiceImpl.java | 9 ++-- .../server/dao/IAMPolicyPermissionDao.java | 2 +- .../dao/IAMPolicyPermissionDaoImpl.java | 18 +++---- test/integration/smoke/test_vm_iam.py | 33 ++++-------- 8 files changed, 94 insertions(+), 66 deletions(-) diff --git a/api/src/org/apache/cloudstack/acl/QuerySelector.java b/api/src/org/apache/cloudstack/acl/QuerySelector.java index b89aa4e9ef5..229ce9866f9 100644 --- a/api/src/org/apache/cloudstack/acl/QuerySelector.java +++ b/api/src/org/apache/cloudstack/acl/QuerySelector.java @@ -18,6 +18,8 @@ package org.apache.cloudstack.acl; import java.util.List; +import org.apache.cloudstack.acl.SecurityChecker.AccessType; + import com.cloud.user.Account; import com.cloud.utils.component.Adapter; @@ -33,7 +35,7 @@ public interface QuerySelector extends Adapter { * @param action action * @return list of domain Ids granted to the caller account. */ - List getAuthorizedDomains(Account caller, String action); + List getAuthorizedDomains(Account caller, String action, AccessType accessType); /** * List granted accounts for the caller, given a specific action. @@ -42,7 +44,7 @@ public interface QuerySelector extends Adapter { * @param action action. * @return list of domain Ids granted to the caller account. */ - List getAuthorizedAccounts(Account caller, String action); + List getAuthorizedAccounts(Account caller, String action, AccessType accessType); /** @@ -52,7 +54,7 @@ public interface QuerySelector extends Adapter { * @param action action. * @return list of domain Ids granted to the caller account. */ - List getAuthorizedResources(Account caller, String action); + List getAuthorizedResources(Account caller, String action, AccessType accessType); /** * Check if this account is associated with a policy with scope of ALL @@ -60,7 +62,7 @@ public interface QuerySelector extends Adapter { * @param action action. * @return true if this account is attached with a policy for the given action of ALL scope. */ - boolean isGrantedAll(Account caller, String action); + boolean isGrantedAll(Account caller, String action, AccessType accessType); /** * List of ACL group the given account belongs to diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index d4b67311d13..047f5475ced 100755 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/server/src/com/cloud/user/AccountManagerImpl.java @@ -2165,6 +2165,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M List permittedResources, Ternary domainIdRecursiveListProject, boolean listAll, boolean forProjectInvitation, String action) { + Account owner = null; // for impersonation Long domainId = domainIdRecursiveListProject.first(); if (id == null) { // if id is specified, it will ignore all other parameters @@ -2178,7 +2179,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M checkAccess(caller, domain); } - // specific account is specified + // specific account is specified, we need to impersonate that account instead of caller if (accountName != null) { if (projectId != null) { throw new InvalidParameterValueException("Account and projectId can't be specified together"); @@ -2197,14 +2198,10 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M if (userAccount != null) { //check permissions checkAccess(caller, null, userAccount); - permittedAccounts.add(userAccount.getId()); + owner = userAccount; } 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()); } } @@ -2229,6 +2226,15 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } } else { + if (owner == null) { + // no impersonation, so we directly check permission for caller + owner = caller; + } + AccessType accessType = AccessType.UseEntry; + if (listAll) { + // listAll = true should show all resources that owner has ListEntry access type + accessType = AccessType.ListEntry; + } domainIdRecursiveListProject.third(Project.ListProjectResourcesCriteria.SkipProjectResources); // search for policy permissions associated with caller to get all his authorized domains, accounts, and resources @@ -2239,18 +2245,18 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M return; // no futher filtering QuerySelector qs = _querySelectors.get(0); - boolean grantedAll = qs.isGrantedAll(caller, action); + boolean grantedAll = qs.isGrantedAll(owner, action, accessType); if ( grantedAll ){ - if (permittedAccounts.isEmpty() && domainId != null) { + if (domainId != null) { permittedDomains.add(domainId); } } else { - List grantedDomains = qs.getAuthorizedDomains(caller, action); - List grantedAccounts = qs.getAuthorizedAccounts(caller, action); - List grantedResources = qs.getAuthorizedResources(caller, action); + List grantedDomains = qs.getAuthorizedDomains(owner, action, accessType); + List grantedAccounts = qs.getAuthorizedAccounts(owner, action, accessType); + List grantedResources = qs.getAuthorizedResources(owner, action, accessType); - if (permittedAccounts.isEmpty() && domainId != null) { + if (domainId != null) { // specific domain and no account is specified if (grantedDomains.contains(domainId)) { permittedDomains.add(domainId); @@ -2268,7 +2274,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // like NetworkACLItem permittedResources.addAll(grantedResources); } - } else if (permittedAccounts.isEmpty()) { + } else { // neither domain nor account is not specified permittedDomains.addAll(grantedDomains); permittedAccounts.addAll(grantedAccounts); diff --git a/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java b/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java index 23c57a16880..a56940368d1 100644 --- a/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java +++ b/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java @@ -25,6 +25,7 @@ import org.apache.log4j.Logger; import org.apache.cloudstack.acl.PermissionScope; import org.apache.cloudstack.acl.QuerySelector; +import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.iam.api.IAMGroup; import org.apache.cloudstack.iam.api.IAMPolicy; import org.apache.cloudstack.iam.api.IAMPolicyPermission; @@ -41,14 +42,22 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe IAMService _iamService; @Override - public List getAuthorizedDomains(Account caller, String action) { + public List getAuthorizedDomains(Account caller, String action, AccessType accessType) { long accountId = caller.getAccountId(); + if (accessType == null) { + accessType = AccessType.UseEntry; // default always show resources authorized to use + } // Get the static Policies of the Caller List policies = _iamService.listIAMPolicies(accountId); // for each policy, find granted permission with Domain scope List domainIds = new ArrayList(); for (IAMPolicy policy : policies) { - List pp = _iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.DOMAIN.toString()); + List pp = new ArrayList(); + for (AccessType type : AccessType.values()) { + if (type.ordinal() >= accessType.ordinal()) { + pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.DOMAIN.toString(), type)); + } + } if (pp != null) { for (IAMPolicyPermission p : pp) { if (p.getScopeId() != null) { @@ -65,14 +74,22 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe } @Override - public List getAuthorizedAccounts(Account caller, String action) { + public List getAuthorizedAccounts(Account caller, String action, AccessType accessType) { long accountId = caller.getAccountId(); + if (accessType == null) { + accessType = AccessType.UseEntry; // default always show resources authorized to use + } // Get the static Policies of the Caller List policies = _iamService.listIAMPolicies(accountId); // for each policy, find granted permission with Account scope List accountIds = new ArrayList(); for (IAMPolicy policy : policies) { - List pp = _iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.ACCOUNT.toString()); + List pp = new ArrayList(); + for (AccessType type : AccessType.values()) { + if (type.ordinal() >= accessType.ordinal()) { + pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.ACCOUNT.toString(), type)); + } + } if (pp != null) { for (IAMPolicyPermission p : pp) { if (p.getScopeId() != null) { @@ -89,8 +106,11 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe } @Override - public List getAuthorizedResources(Account caller, String action) { + public List getAuthorizedResources(Account caller, String action, AccessType accessType) { long accountId = caller.getAccountId(); + if (accessType == null) { + accessType = AccessType.UseEntry; // default always show resources authorized to use + } // Get the static Policies of the Caller List policies = _iamService.listIAMPolicies(accountId); @@ -107,7 +127,12 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe // for each policy, find granted permission with Resource scope List entityIds = new ArrayList(); for (IAMPolicy policy : policies) { - List pp = _iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.RESOURCE.toString()); + List pp = new ArrayList(); + for (AccessType type : AccessType.values()) { + if (type.ordinal() >= accessType.ordinal()) { + pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.RESOURCE.toString(), type)); + } + } if (pp != null) { for (IAMPolicyPermission p : pp) { if (p.getScopeId() != null) { @@ -120,15 +145,23 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe } @Override - public boolean isGrantedAll(Account caller, String action) { + public boolean isGrantedAll(Account caller, String action, AccessType accessType) { long accountId = caller.getAccountId(); + if (accessType == null) { + accessType = AccessType.UseEntry; // default always show resources authorized to use + } // Get the static Policies of the Caller List policies = _iamService.listIAMPolicies(accountId); // for each policy, find granted permission with ALL scope for (IAMPolicy policy : policies) { - List pp = _iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.ALL.toString()); - if (pp != null && pp.size() > 0) { - return true; + List pp = new ArrayList(); + for (AccessType type : AccessType.values()) { + if (type.ordinal() >= accessType.ordinal()) { + pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.ALL.toString(), type)); + } + if (pp != null && pp.size() > 0) { + return true; + } } } return false; diff --git a/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java b/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java index 74a08851506..c396fa922fe 100644 --- a/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java +++ b/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java @@ -18,6 +18,7 @@ package org.apache.cloudstack.iam.api; import java.util.List; +import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.iam.api.IAMPolicyPermission.Permission; import com.cloud.utils.Pair; @@ -72,7 +73,7 @@ public interface IAMService { List listPolicyPermissions(long policyId); - List listPolicyPermissionsByScope(long policyId, String action, String scope); + List listPolicyPermissionsByScope(long policyId, String action, String scope, AccessType accessType); List listPolicyPermissionByActionAndEntity(long policyId, String action, String entityType); diff --git a/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java b/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java index 56c931a9d10..d6a61a10457 100644 --- a/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java +++ b/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java @@ -25,6 +25,7 @@ import javax.inject.Inject; import org.apache.log4j.Logger; import org.apache.cloudstack.acl.PermissionScope; +import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.iam.api.IAMGroup; import org.apache.cloudstack.iam.api.IAMPolicy; import org.apache.cloudstack.iam.api.IAMPolicyPermission; @@ -742,8 +743,8 @@ public class IAMServiceImpl extends ManagerBase implements IAMService, Manager { // for each policy, find granted permission within the given scope List entityIds = new ArrayList(); for (IAMPolicy policy : policies) { - List pp = _policyPermissionDao.listGrantedByActionAndScope(policy.getId(), action, - scope); + List pp = _policyPermissionDao.listByPolicyActionAndScope(policy.getId(), action, + scope, null); if (pp != null) { for (IAMPolicyPermissionVO p : pp) { if (p.getScopeId() != null) { @@ -765,9 +766,9 @@ public class IAMServiceImpl extends ManagerBase implements IAMService, Manager { @SuppressWarnings("unchecked") @Override - public List listPolicyPermissionsByScope(long policyId, String action, String scope) { + public List listPolicyPermissionsByScope(long policyId, String action, String scope, AccessType accessType) { @SuppressWarnings("rawtypes") - List pp = _policyPermissionDao.listGrantedByActionAndScope(policyId, action, scope); + List pp = _policyPermissionDao.listByPolicyActionAndScope(policyId, action, scope, accessType.toString()); return pp; } diff --git a/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java b/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java index cdcb02b1dee..6e6099c6648 100644 --- a/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java +++ b/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java @@ -29,7 +29,7 @@ public interface IAMPolicyPermissionDao extends GenericDao listGrantedByActionAndScope(long policyId, String action, String scope); + List listByPolicyActionAndScope(long policyId, String action, String scope, String accessType); List listByPolicyActionAndEntity(long policyId, String action, String entityType); diff --git a/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java b/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java index 3f976cfefce..9cc1af07e8c 100644 --- a/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java +++ b/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java @@ -33,7 +33,6 @@ public class IAMPolicyPermissionDaoImpl extends GenericDaoBase policyIdSearch; private SearchBuilder fullSearch; - private SearchBuilder actionScopeSearch; private SearchBuilder entitySearch; @Override @@ -54,13 +53,6 @@ public class IAMPolicyPermissionDaoImpl extends GenericDaoBase listGrantedByActionAndScope(long policyId, String action, String scope) { - SearchCriteria sc = actionScopeSearch.create(); + public List listByPolicyActionAndScope(long policyId, String action, String scope, String accessType) { + SearchCriteria sc = fullSearch.create(); sc.setParameters("policyId", policyId); sc.setParameters("action", action); sc.setParameters("scope", scope); sc.setParameters("permission", Permission.Allow); + if ( accessType != null ){ + // accessType can be optional, used mainly in list apis with ListEntry and UseEntry distinction + sc.setParameters("accessType", accessType); + } return listBy(sc); } @@ -120,7 +116,7 @@ public class IAMPolicyPermissionDaoImpl extends GenericDaoBase listByEntity(String entityType, Long entityId) { - SearchCriteria sc = fullSearch.create(); + SearchCriteria sc = entitySearch.create(); sc.setParameters("entityType", entityType); sc.setParameters("scopeId", entityId); return listBy(sc); diff --git a/test/integration/smoke/test_vm_iam.py b/test/integration/smoke/test_vm_iam.py index bdbe75a1742..80c049b0cd9 100644 --- a/test/integration/smoke/test_vm_iam.py +++ b/test/integration/smoke/test_vm_iam.py @@ -289,8 +289,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -313,8 +312,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -338,8 +336,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -381,8 +378,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -430,8 +426,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -484,8 +479,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -528,8 +522,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -570,8 +563,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -618,8 +610,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -662,8 +653,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list), @@ -705,8 +695,7 @@ 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, - listall=True + self.apiclient ) self.assertEqual( isinstance(list_vm_response, list),