cloudstack-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ro...@apache.org
Subject [cloudstack] branch master updated: CLOUDSTACK-9971: Bugfix/listaccounts parameter consistency (#2156)
Date Wed, 03 Jan 2018 11:30:03 GMT
This is an automated email from the ASF dual-hosted git repository.

rohit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git


The following commit(s) were added to refs/heads/master by this push:
     new 000ee36  CLOUDSTACK-9971: Bugfix/listaccounts parameter consistency (#2156)
000ee36 is described below

commit 000ee36224feb81fb86da2ffebb812f4d3d1e35a
Author: Daniel Carbone <daniel.p.carbone@gmail.com>
AuthorDate: Wed Jan 3 05:29:54 2018 -0600

    CLOUDSTACK-9971: Bugfix/listaccounts parameter consistency (#2156)
    
    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
---
 .../schema/src/com/cloud/user/dao/AccountDao.java  |  8 ++-
 .../src/com/cloud/user/dao/AccountDaoImpl.java     | 23 ++++---
 .../src/com/cloud/api/query/QueryManagerImpl.java  | 80 +++++++++++++---------
 3 files changed, 66 insertions(+), 45 deletions(-)

diff --git a/engine/schema/src/com/cloud/user/dao/AccountDao.java b/engine/schema/src/com/cloud/user/dao/AccountDao.java
index 374c9cc..b97f318 100644
--- a/engine/schema/src/com/cloud/user/dao/AccountDao.java
+++ b/engine/schema/src/com/cloud/user/dao/AccountDao.java
@@ -16,9 +16,6 @@
 // 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;
@@ -26,6 +23,9 @@ 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
index 2789150..956f8a8 100644
--- 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.SearchBuilder;
 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
{
@@ -189,6 +187,13 @@ public class AccountDaoImpl extends GenericDaoBase<AccountVO, Long>
implements A
     }
 
     @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);
         sc.setParameters("domainId", domainId);
diff --git a/server/src/com/cloud/api/query/QueryManagerImpl.java b/server/src/com/cloud/api/query/QueryManagerImpl.java
index 2a6919b..2d6fabc 100644
--- a/server/src/com/cloud/api/query/QueryManagerImpl.java
+++ b/server/src/com/cloud/api/query/QueryManagerImpl.java
@@ -1982,47 +1982,58 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase
implements Q
         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 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase
implements Q
         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 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase
implements Q
             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);

-- 
To stop receiving notification emails like this one, please contact
['"commits@cloudstack.apache.org" <commits@cloudstack.apache.org>'].

Mime
View raw message