fix quota resource access validation

api/src/main/java/com/cloud/user/AccountService.java

@@ -114,6 +114,8 @@ public interface AccountService {
 
     void checkAccess(Account account, AccessType accessType, boolean sameOwner, String apiName, ControlledEntity... entities) throws PermissionDeniedException;
 
+    void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource);
+
     Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly);
 
     /**

plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaBalanceCmd.java

@@ -21,6 +21,8 @@ import java.util.List;
 
 import javax.inject.Inject;
 
+import com.cloud.user.Account;
+import org.apache.cloudstack.api.ACL;
 import org.apache.log4j.Logger;
 import org.apache.cloudstack.api.APICommand;
 import org.apache.cloudstack.api.ApiConstants;
@@ -42,6 +44,7 @@ public class QuotaBalanceCmd extends BaseCmd {
     @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Account Id for which statement needs to be generated")
     private String accountName;
 
+    @ACL
     @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = true, entityType = DomainResponse.class, description = "If domain Id is given and the caller is domain admin then the statement is generated for domain.")
     private Long domainId;
 
@@ -51,6 +54,7 @@ public class QuotaBalanceCmd extends BaseCmd {
     @Parameter(name = ApiConstants.START_DATE, type = CommandType.DATE, description = "Start date range quota query. Use yyyy-MM-dd as the date format, e.g. startDate=2009-06-01.")
     private Date startDate;
 
+    @ACL
     @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "List usage records for the specified account")
     private Long accountId;
 
@@ -104,7 +108,14 @@ public class QuotaBalanceCmd extends BaseCmd {
 
     @Override
     public long getEntityOwnerId() {
-       return _accountService.getActiveAccountByName(accountName, domainId).getAccountId();
+        if (accountId != null) {
+            return accountId;
+        }
+        Account account = _accountService.getActiveAccountByName(accountName, domainId);
+        if (account != null) {
+            return account.getAccountId();
+        }
+        return Account.ACCOUNT_ID_SYSTEM;
     }
 
     @Override

plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaCreditsCmd.java

@@ -18,6 +18,7 @@ package org.apache.cloudstack.api.command;
 
 import com.cloud.user.Account;
 
+import org.apache.cloudstack.api.ACL;
 import org.apache.cloudstack.api.APICommand;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.api.ApiErrorCode;
@@ -48,6 +49,7 @@ public class QuotaCreditsCmd extends BaseCmd {
     @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Account Id for which quota credits need to be added")
     private String accountName;
 
+    @ACL
     @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = true, entityType = DomainResponse.class, description = "Domain for which quota credits need to be added")
     private Long domainId;
 
@@ -132,6 +134,10 @@ public class QuotaCreditsCmd extends BaseCmd {
 
     @Override
     public long getEntityOwnerId() {
+        Account account = _accountService.getActiveAccountByName(accountName, domainId);
+        if (account != null) {
+            return account.getAccountId();
+        }
         return Account.ACCOUNT_ID_SYSTEM;
     }
 

plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java

@@ -21,6 +21,7 @@ import java.util.List;
 
 import javax.inject.Inject;
 
+import org.apache.cloudstack.api.ACL;
 import org.apache.cloudstack.api.APICommand;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.api.BaseCmd;
@@ -44,6 +45,7 @@ public class QuotaStatementCmd extends BaseCmd {
     @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Optional, Account Id for which statement needs to be generated")
     private String accountName;
 
+    @ACL
     @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = true, entityType = DomainResponse.class, description = "Optional, If domain Id is given and the caller is domain admin then the statement is generated for domain.")
     private Long domainId;
 
@@ -56,6 +58,7 @@ public class QuotaStatementCmd extends BaseCmd {
     @Parameter(name = ApiConstants.TYPE, type = CommandType.INTEGER, description = "List quota usage records for the specified usage type")
     private Integer usageType;
 
+    @ACL
     @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "List usage records for the specified account")
     private Long accountId;
 
@@ -112,6 +115,9 @@ public class QuotaStatementCmd extends BaseCmd {
 
     @Override
     public long getEntityOwnerId() {
+        if (accountId != null) {
+            return accountId;
+        }
         Account activeAccountByName = _accountService.getActiveAccountByName(accountName, domainId);
         if (activeAccountByName != null) {
             return activeAccountByName.getAccountId();

plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java

@@ -446,6 +446,11 @@ public class MockAccountManager extends ManagerBase implements AccountManager {
         // TODO Auto-generated method stub
     }
 
+    @Override
+    public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
+        // TODO Auto-generated method stub
+    }
+
     @Override
     public Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly) {
         // TODO Auto-generated method stub

server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java

@@ -32,8 +32,6 @@ import java.util.regex.Matcher;
 
 import javax.inject.Inject;
 
-import org.apache.cloudstack.acl.ControlledEntity;
-import org.apache.cloudstack.acl.InfrastructureEntity;
 import org.apache.cloudstack.acl.SecurityChecker;
 import org.apache.cloudstack.acl.SecurityChecker.AccessType;
 import org.apache.cloudstack.api.ACL;
@@ -288,41 +286,44 @@ public class ParamProcessWorker implements DispatchWorker {
         doAccessChecks(cmd, entitiesToAccess);
     }
 
-    private void doAccessChecks(BaseCmd cmd, Map<Object, AccessType> entitiesToAccess) {
+    protected void doAccessChecks(BaseCmd cmd, Map<Object, AccessType> entitiesToAccess) {
-        Account caller = CallContext.current().getCallingAccount();
+        Account[] owners = getEntityOwners(cmd);
-        List<Long> entityOwners = cmd.getEntityOwnerIds();
-        Account[] owners = null;
-        if (entityOwners != null) {
-            owners = entityOwners.stream().map(id -> _accountMgr.getAccount(id)).toArray(Account[]::new);
-        } else {
-            if (cmd.getEntityOwnerId() == Account.ACCOUNT_ID_SYSTEM && cmd instanceof BaseAsyncCmd && ((BaseAsyncCmd)cmd).getApiResourceType() == ApiCommandResourceType.Network) {
-                if (s_logger.isDebugEnabled()) {
-                    s_logger.debug("Skipping access check on the network owner if the owner is ROOT/system.");
-                }
-                owners = new Account[]{};
-            } else {
-                owners = new Account[]{_accountMgr.getAccount(cmd.getEntityOwnerId())};
-            }
-        }
 
+        Account caller = CallContext.current().getCallingAccount();
         if (cmd instanceof BaseAsyncCreateCmd) {
             // check that caller can access the owner account.
             _accountMgr.checkAccess(caller, null, false, owners);
         }
 
-        if (!entitiesToAccess.isEmpty()) {
+        checkCallerAccessToEntities(caller, owners, entitiesToAccess);
-            // check that caller can access the owner account.
+    }
-            _accountMgr.checkAccess(caller, null, false, owners);
+
-            for (Map.Entry<Object,AccessType>entry : entitiesToAccess.entrySet()) {
+    protected Account[] getEntityOwners(BaseCmd cmd) {
-                Object entity = entry.getKey();
+        List<Long> entityOwners = cmd.getEntityOwnerIds();
-                if (entity instanceof ControlledEntity) {
+        if (entityOwners != null) {
-                    _accountMgr.checkAccess(caller, entry.getValue(), true, (ControlledEntity) entity);
+            return entityOwners.stream().map(id -> _accountMgr.getAccount(id)).toArray(Account[]::new);
-                } else if (entity instanceof InfrastructureEntity) {
+        }
-                    // FIXME: Move this code in adapter, remove code from
+
-                    // Account manager
+        if (cmd.getEntityOwnerId() == Account.ACCOUNT_ID_SYSTEM && cmd instanceof BaseAsyncCmd && cmd.getApiResourceType() == ApiCommandResourceType.Network) {
-                }
+            s_logger.debug("Skipping access check on the network owner if the owner is ROOT/system.");
+        } else {
+            Account owner = _accountMgr.getAccount(cmd.getEntityOwnerId());
+            if (owner != null) {
+                return new Account[]{owner};
             }
         }
+        return new Account[]{};
+    }
+
+    protected void checkCallerAccessToEntities(Account caller, Account[] owners, Map<Object, AccessType> entitiesToAccess) {
+        if (entitiesToAccess.isEmpty()) {
+            return;
+        }
+        _accountMgr.checkAccess(caller, null, false, owners);
+        for (Map.Entry<Object, AccessType> entry : entitiesToAccess.entrySet()) {
+            Object entity = entry.getKey();
+            _accountMgr.validateAccountHasAccessToResource(caller, entry.getValue(), entity);
+        }
     }
 
     @SuppressWarnings({"unchecked", "rawtypes"})

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

@@ -42,6 +42,7 @@ import javax.naming.ConfigurationException;
 
 import org.apache.cloudstack.acl.APIChecker;
 import org.apache.cloudstack.acl.ControlledEntity;
+import org.apache.cloudstack.acl.InfrastructureEntity;
 import org.apache.cloudstack.acl.QuerySelector;
 import org.apache.cloudstack.acl.Role;
 import org.apache.cloudstack.acl.RoleService;
@@ -719,6 +720,19 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
 
     }
 
+    @Override
+    public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
+        Class<?> resourceClass = resource.getClass();
+        if (ControlledEntity.class.isAssignableFrom(resourceClass)) {
+            checkAccess(account, accessType, true, (ControlledEntity) resource);
+        } else if (Domain.class.isAssignableFrom(resourceClass)) {
+            checkAccess(account, (Domain) resource);
+        } else if (InfrastructureEntity.class.isAssignableFrom(resourceClass)) {
+            s_logger.trace("Validation of access to infrastructure entity has been disabled in CloudStack version 4.4.");
+        }
+        s_logger.debug(String.format("Account [%s] has access to resource.", account.getUuid()));
+    }
+
     @Override
     public Long checkAccessAndSpecifyAuthority(Account caller, Long zoneId) {
         // We just care for resource domain admins for now, and they should be permitted to see only their zone.

server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java

@@ -19,6 +19,8 @@
 package com.cloud.api.dispatch;
 
 import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
 
 import org.junit.After;
 import org.junit.Assert;
@@ -27,11 +29,16 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.Mockito;
-import org.mockito.runners.MockitoJUnitRunner;
+import org.mockito.InjectMocks;
+import org.mockito.Spy;
+
+import org.mockito.junit.MockitoJUnitRunner;
 
 import org.apache.cloudstack.api.BaseCmd;
 import org.apache.cloudstack.api.Parameter;
 import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.command.user.address.AssociateIPAddrCmd;
+import org.apache.cloudstack.acl.SecurityChecker;
 import org.apache.cloudstack.context.CallContext;
 
 import com.cloud.exception.ConcurrentOperationException;
@@ -42,14 +49,33 @@ import com.cloud.exception.ResourceUnavailableException;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
 import com.cloud.user.User;
+import com.cloud.vm.VMInstanceVO;
 
 @RunWith(MockitoJUnitRunner.class)
 public class ParamProcessWorkerTest {
 
-    @Mock
+    @Spy
-    protected AccountManager accountManager;
+    @InjectMocks
+    private ParamProcessWorker paramProcessWorkerSpy;
 
-    protected ParamProcessWorker paramProcessWorker;
+    @Mock
+    private AccountManager accountManagerMock;
+
+    @Mock
+    private Account callingAccountMock;
+
+    @Mock
+    private User callingUserMock;
+
+    @Mock
+    private Account ownerAccountMock;
+
+    @Mock
+    BaseCmd baseCmdMock;
+
+    private Account[] owners = new Account[]{ownerAccountMock};
+
+    private Map<Object, SecurityChecker.AccessType> entities = new HashMap<>();
 
     public static class TestCmd extends BaseCmd {
 
@@ -67,7 +93,7 @@ public class ParamProcessWorkerTest {
 
         @Override
         public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException,
-            ResourceAllocationException, NetworkRuleConflictException {
+                ResourceAllocationException, NetworkRuleConflictException {
             // well documented nothing
         }
 
@@ -85,9 +111,7 @@ public class ParamProcessWorkerTest {
 
     @Before
     public void setup() {
-        CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
+        CallContext.register(callingUserMock, callingAccountMock);
-        paramProcessWorker = new ParamProcessWorker();
-        paramProcessWorker._accountMgr = accountManager;
     }
 
     @After
@@ -103,10 +127,71 @@ public class ParamProcessWorkerTest {
         params.put("boolparam1", "true");
         params.put("doubleparam1", "11.89");
         final TestCmd cmd = new TestCmd();
-        paramProcessWorker.processParameters(cmd, params);
+        paramProcessWorkerSpy.processParameters(cmd, params);
         Assert.assertEquals("foo", cmd.strparam1);
         Assert.assertEquals(100, cmd.intparam1);
         Assert.assertTrue(Double.compare(cmd.doubleparam1, 11.89) == 0);
+        Mockito.verify(paramProcessWorkerSpy).doAccessChecks(Mockito.any(), Mockito.any());
     }
 
+    @Test
+    public void doAccessChecksTestChecksCallerAccessToOwnerWhenCmdExtendsBaseAsyncCreateCmd() {
+        Mockito.doReturn(owners).when(paramProcessWorkerSpy).getEntityOwners(Mockito.any());
+        Mockito.doNothing().when(paramProcessWorkerSpy).checkCallerAccessToEntities(Mockito.any(), Mockito.any(), Mockito.any());
+
+        paramProcessWorkerSpy.doAccessChecks(new AssociateIPAddrCmd(), entities);
+
+        Mockito.verify(accountManagerMock).checkAccess(callingAccountMock, null, false, owners);
+    }
+
+    @Test
+    public void doAccessChecksTestChecksCallerAccessToEntities() {
+        Mockito.doReturn(owners).when(paramProcessWorkerSpy).getEntityOwners(Mockito.any());
+        Mockito.doNothing().when(paramProcessWorkerSpy).checkCallerAccessToEntities(Mockito.any(), Mockito.any(), Mockito.any());
+
+        paramProcessWorkerSpy.doAccessChecks(new AssociateIPAddrCmd(), entities);
+
+        Mockito.verify(paramProcessWorkerSpy).checkCallerAccessToEntities(callingAccountMock, owners, entities);
+    }
+
+    @Test
+    public void getEntityOwnersTestReturnsAccountsWhenCmdHasMultipleEntityOwners() {
+        Mockito.when(baseCmdMock.getEntityOwnerIds()).thenReturn(List.of(1L, 2L));
+        Mockito.doReturn(callingAccountMock).when(accountManagerMock).getAccount(1L);
+        Mockito.doReturn(ownerAccountMock).when(accountManagerMock).getAccount(2L);
+
+        List<Account> result = List.of(paramProcessWorkerSpy.getEntityOwners(baseCmdMock));
+
+        Assert.assertEquals(List.of(callingAccountMock, ownerAccountMock), result);
+    }
+
+    @Test
+    public void getEntityOwnersTestReturnsAccountWhenCmdHasOneEntityOwner() {
+        Mockito.when(baseCmdMock.getEntityOwnerId()).thenReturn(1L);
+        Mockito.when(baseCmdMock.getEntityOwnerIds()).thenReturn(null);
+        Mockito.doReturn(ownerAccountMock).when(accountManagerMock).getAccount(1L);
+
+        List<Account> result = List.of(paramProcessWorkerSpy.getEntityOwners(baseCmdMock));
+
+        Assert.assertEquals(List.of(ownerAccountMock), result);
+    }
+
+    @Test
+    public void checkCallerAccessToEntitiesTestChecksCallerAccessToOwners() {
+        entities.put(ownerAccountMock, SecurityChecker.AccessType.UseEntry);
+
+        paramProcessWorkerSpy.checkCallerAccessToEntities(callingAccountMock, owners, entities);
+
+        Mockito.verify(accountManagerMock).checkAccess(callingAccountMock, null, false, owners);
+    }
+
+    @Test
+    public void checkCallerAccessToEntitiesTestChecksCallerAccessToResource() {
+        VMInstanceVO vmInstanceVo = new VMInstanceVO();
+        entities.put(vmInstanceVo, SecurityChecker.AccessType.UseEntry);
+
+        paramProcessWorkerSpy.checkCallerAccessToEntities(callingAccountMock, owners, entities);
+
+        Mockito.verify(accountManagerMock).validateAccountHasAccessToResource(callingAccountMock, SecurityChecker.AccessType.UseEntry, vmInstanceVo);
+    }
 }

server/src/test/java/com/cloud/user/MockAccountManagerImpl.java

@@ -436,6 +436,11 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco
         // TODO Auto-generated method stub
     }
 
+    @Override
+    public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
+        // TODO Auto-generated method stub
+    }
+
     @Override
     public Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly) {
         // TODO Auto-generated method stub