CLOUDSTACK-9971: Bugfix/listaccounts parameter consistency (#2156)

dcarbone · yadvr · commit 000ee36224fe · 2018-01-03T16:59:54.000+05:30
Ran into an issue today where we passed both the "id" and "domainid" parameters into "listAccounts" and received a response despite the account id passed not belonging to the domainid passed.

Allow usage of "domainid" AND "id" in "listAccounts"
- Adding "AccountDoa::findActiveAccountById"
- Adding "AccountDaoImpl::findActiveAccountById"
- Removing seemingly pointless "listForDomain" parameter
- Updating "typeNEQ" value from "5" to "Account.ACCOUNT_TYPE_PROJECT"
  (which is "5")
- Only attempt to load domain for "path" query parameter once

"searchForAccountsInternal" input validation logic pseudo-code:
  - If "domainid" set, check immediately
  - If "id" not set:
    - and user is admin and "listall" is true
      - if "domainid" not set, use caller domain id
      - force "isrecursive" true
    - else use caller account id
  - Else if "domainid" and "name" set
    - verify existence of account and that user has access
  - Else:
    - if "domainid" not set, locate account by "id"
    - else, locate account by "id" and "domainid"
    - verify account found and caller has access rights
diff --git a/engine/schema/src/com/cloud/user/dao/AccountDao.java b/engine/schema/src/com/cloud/user/dao/AccountDao.java
@@ -16,16 +16,16 @@
 // under the License.
 package com.cloud.user.dao;
 
-import java.util.Date;
-import java.util.List;
-
 import com.cloud.user.Account;
 import com.cloud.user.AccountVO;
 import com.cloud.user.User;
 import com.cloud.utils.Pair;
 import com.cloud.utils.db.Filter;
 import com.cloud.utils.db.GenericDao;
 
+import java.util.Date;
+import java.util.List;
+
 public interface AccountDao extends GenericDao<AccountVO, Long> {
     Pair<User, Account> findUserAccountByApiKey(String apiKey);
 
@@ -62,6 +62,8 @@ public interface AccountDao extends GenericDao<AccountVO, Long> {
     //returns only non-removed account
     Account findActiveAccount(String accountName, Long domainId);
 
+    Account findActiveAccountById(Long accountId, Long domainId);
+
     Account findActiveNonProjectAccount(String accountName, Long domainId);
 
     List<Long> getAccountIdsForDomains(List<Long> ids);
diff --git a/engine/schema/src/com/cloud/user/dao/AccountDaoImpl.java b/engine/schema/src/com/cloud/user/dao/AccountDaoImpl.java
@@ -16,15 +16,6 @@
 // under the License.
 package com.cloud.user.dao;
 
-import java.sql.PreparedStatement;
-import java.sql.ResultSet;
-import java.util.Date;
-import java.util.List;
-
-
-import org.apache.log4j.Logger;
-import org.springframework.stereotype.Component;
-
 import com.cloud.user.Account;
 import com.cloud.user.Account.State;
 import com.cloud.user.AccountVO;
@@ -39,6 +30,13 @@
 import com.cloud.utils.db.SearchCriteria;
 import com.cloud.utils.db.SearchCriteria.Op;
 import com.cloud.utils.db.TransactionLegacy;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.util.Date;
+import java.util.List;
 
 @Component
 public class AccountDaoImpl extends GenericDaoBase<AccountVO, Long> implements AccountDao {
@@ -188,6 +186,13 @@ public Account findActiveAccount(String accountName, Long domainId) {
         return findOneBy(sc);
     }
 
+    @Override
+    public Account findActiveAccountById(Long accountId, Long domainId) {
+        SearchCriteria<AccountVO> sc = AllFieldsSearch.create("id", accountId);
+        sc.setParameters("domainId", domainId);
+        return findOneBy(sc);
+    }
+
     @Override
     public Account findActiveNonProjectAccount(String accountName, Long domainId) {
         SearchCriteria<AccountVO> sc = NonProjectAccountSearch.create("accountName", accountName);
diff --git a/server/src/com/cloud/api/query/QueryManagerImpl.java b/server/src/com/cloud/api/query/QueryManagerImpl.java
@@ -1982,47 +1982,58 @@ private Pair<List<AccountJoinVO>, Integer> searchForAccountsInternal(ListAccount
         String accountName = cmd.getSearchName();
         boolean isRecursive = cmd.isRecursive();
         boolean listAll = cmd.listAll();
-        Boolean listForDomain = false;
-
-        if (accountId != null) {
-            Account account = _accountDao.findById(accountId);
-            if (account == null || account.getId() == Account.ACCOUNT_ID_SYSTEM) {
-                throw new InvalidParameterValueException("Unable to find account by id " + accountId);
-            }
-
-            _accountMgr.checkAccess(caller, null, true, account);
-        }
+        boolean callerIsAdmin = _accountMgr.isAdmin(caller.getId());
+        Account account;
+        Domain domain = null;
 
+        // if "domainid" specified, perform validation
         if (domainId != null) {
-            Domain domain = _domainDao.findById(domainId);
+            // ensure existence...
+            domain = _domainDao.findById(domainId);
             if (domain == null) {
                 throw new InvalidParameterValueException("Domain id=" + domainId + " doesn't exist");
             }
-
+            // ... and check access rights.
             _accountMgr.checkAccess(caller, domain);
-
-            if (accountName != null) {
-                Account account = _accountDao.findActiveAccount(accountName, domainId);
-                if (account == null || account.getId() == Account.ACCOUNT_ID_SYSTEM) {
-                    throw new InvalidParameterValueException("Unable to find account by name " + accountName
-                            + " in domain " + domainId);
-                }
-                _accountMgr.checkAccess(caller, null, true, account);
-            }
         }
 
+        // if no "id" specified...
         if (accountId == null) {
-            if (_accountMgr.isAdmin(caller.getId()) && listAll && domainId == null) {
-                listForDomain = true;
-                isRecursive = true;
+            // listall only has significance if they are an admin
+            if (listAll && callerIsAdmin) {
+                // if no domain id specified, use caller's domain
                 if (domainId == null) {
                     domainId = caller.getDomainId();
                 }
-            } else if (_accountMgr.isAdmin(caller.getId()) && domainId != null) {
-                listForDomain = true;
-            } else {
+                // mark recursive
+                isRecursive = true;
+            } else if (!callerIsAdmin || domainId == null) {
                 accountId = caller.getAccountId();
             }
+        } else if (domainId != null && accountName != null) {
+            // if they're looking for an account by name
+            account = _accountDao.findActiveAccount(accountName, domainId);
+            if (account == null || account.getId() == Account.ACCOUNT_ID_SYSTEM) {
+                throw new InvalidParameterValueException(
+                        "Unable to find account by name " + accountName + " in domain " + domainId
+                );
+            }
+            _accountMgr.checkAccess(caller, null, true, account);
+        } else {
+            // if they specified an "id"...
+            if (domainId == null) {
+                account = _accountDao.findById(accountId);
+            } else {
+                account = _accountDao.findActiveAccountById(accountId, domainId);
+            }
+            if (account == null || account.getId() == Account.ACCOUNT_ID_SYSTEM) {
+                throw new InvalidParameterValueException(
+                        "Unable to find account by id "
+                                + accountId
+                                + (domainId == null ? "" : " in domain " + domainId)
+                );
+            }
+            _accountMgr.checkAccess(caller, null, true, account);
         }
 
         Filter searchFilter = new Filter(AccountJoinVO.class, "id", true, cmd.getStartIndex(), cmd.getPageSizeVal());
@@ -2042,12 +2053,15 @@ private Pair<List<AccountJoinVO>, Integer> searchForAccountsInternal(ListAccount
         sb.and("typeNEQ", sb.entity().getType(), SearchCriteria.Op.NEQ);
         sb.and("idNEQ", sb.entity().getId(), SearchCriteria.Op.NEQ);
 
-        if (listForDomain && isRecursive) {
+        if (domainId != null && isRecursive) {
             sb.and("path", sb.entity().getDomainPath(), SearchCriteria.Op.LIKE);
         }
 
         SearchCriteria<AccountJoinVO> sc = sb.create();
 
+        // don't return account of type project to the end user
+        sc.setParameters("typeNEQ", Account.ACCOUNT_TYPE_PROJECT);
+        // don't return system account...
         sc.setParameters("idNEQ", Account.ACCOUNT_ID_SYSTEM);
 
         if (keyword != null) {
@@ -2073,16 +2087,16 @@ private Pair<List<AccountJoinVO>, Integer> searchForAccountsInternal(ListAccount
             sc.setParameters("accountName", accountName);
         }
 
-        // don't return account of type project to the end user
-        sc.setParameters("typeNEQ", 5);
-
         if (accountId != null) {
             sc.setParameters("id", accountId);
         }
 
-        if (listForDomain) {
+        if (domainId != null) {
             if (isRecursive) {
-                Domain domain = _domainDao.findById(domainId);
+                // will happen if no "domainid" was specified in the request...
+                if (domain == null) {
+                    domain = _domainDao.findById(domainId);
+                }
                 sc.setParameters("path", domain.getPath() + "%");
             } else {
                 sc.setParameters("domainId", domainId);
