This is an automated email from the ASF dual-hosted git repository.
dahn pushed a commit to branch 4.19
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 7d70e32378f0d5141d54fa3c95df855f147e66a8
Author: Daan Hoogland <d...@onecht.net>
AuthorDate: Tue Aug 20 15:12:54 2024 +0200
fix quota resource access validation
---
.../main/java/com/cloud/user/AccountService.java | 2 +
.../cloudstack/api/command/QuotaBalanceCmd.java | 13 ++-
.../cloudstack/api/command/QuotaCreditsCmd.java | 6 +
.../cloudstack/api/command/QuotaStatementCmd.java | 6 +
.../contrail/management/MockAccountManager.java | 5 +
.../com/cloud/api/dispatch/ParamProcessWorker.java | 57 ++++-----
.../java/com/cloud/user/AccountManagerImpl.java | 14 +++
.../cloud/api/dispatch/ParamProcessWorkerTest.java | 129 +++++++++++++++++----
.../com/cloud/user/MockAccountManagerImpl.java | 5 +
9 files changed, 187 insertions(+), 50 deletions(-)
diff --git a/api/src/main/java/com/cloud/user/AccountService.java
b/api/src/main/java/com/cloud/user/AccountService.java
index 63f5455cfd0..60db7abb734 100644
--- a/api/src/main/java/com/cloud/user/AccountService.java
+++ b/api/src/main/java/com/cloud/user/AccountService.java
@@ -116,6 +116,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);
/**
diff --git
a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaBalanceCmd.java
b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaBalanceCmd.java
index f4e248855fd..ecb6531d0cf 100644
---
a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaBalanceCmd.java
+++
b/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
diff --git
a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaCreditsCmd.java
b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaCreditsCmd.java
index c47c0ad2d76..e83d52f94f4 100644
---
a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaCreditsCmd.java
+++
b/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;
}
diff --git
a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java
b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java
index 4d1c233c37a..3219ba1448f 100644
---
a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java
+++
b/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();
diff --git
a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
index 67cfe1df3e1..a3fac2c8be9 100644
---
a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
+++
b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
@@ -451,6 +451,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
diff --git
a/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java
b/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java
index 9f07db4b033..b11a5282e62 100644
--- a/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java
+++ b/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,40 +286,43 @@ 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[] owners = getEntityOwners(cmd);
+
Account caller = CallContext.current().getCallingAccount();
+ if (cmd instanceof BaseAsyncCreateCmd) {
+ // check that caller can access the owner account.
+ _accountMgr.checkAccess(caller, null, false, owners);
+ }
+
+ checkCallerAccessToEntities(caller, owners, entitiesToAccess);
+ }
+
+ protected Account[] getEntityOwners(BaseCmd cmd) {
List<Long> entityOwners = cmd.getEntityOwnerIds();
- Account[] owners = null;
if (entityOwners != null) {
- owners = entityOwners.stream().map(id ->
_accountMgr.getAccount(id)).toArray(Account[]::new);
+ return entityOwners.stream().map(id ->
_accountMgr.getAccount(id)).toArray(Account[]::new);
+ }
+
+ 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 {
- 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 owner = _accountMgr.getAccount(cmd.getEntityOwnerId());
+ if (owner != null) {
+ return new Account[]{owner};
}
}
+ return new Account[]{};
+ }
- if (cmd instanceof BaseAsyncCreateCmd) {
- // check that caller can access the owner account.
- _accountMgr.checkAccess(caller, null, false, owners);
+ protected void checkCallerAccessToEntities(Account caller, Account[]
owners, Map<Object, AccessType> entitiesToAccess) {
+ if (entitiesToAccess.isEmpty()) {
+ return;
}
-
- if (!entitiesToAccess.isEmpty()) {
- // check that caller can access the owner account.
- _accountMgr.checkAccess(caller, null, false, owners);
- for (Map.Entry<Object,AccessType>entry :
entitiesToAccess.entrySet()) {
- Object entity = entry.getKey();
- if (entity instanceof ControlledEntity) {
- _accountMgr.checkAccess(caller, entry.getValue(), true,
(ControlledEntity) entity);
- } else if (entity instanceof InfrastructureEntity) {
- // FIXME: Move this code in adapter, remove code from
- // Account manager
- }
- }
+ _accountMgr.checkAccess(caller, null, false, owners);
+ for (Map.Entry<Object, AccessType> entry :
entitiesToAccess.entrySet()) {
+ Object entity = entry.getKey();
+ _accountMgr.validateAccountHasAccessToResource(caller,
entry.getValue(), entity);
}
}
diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java
b/server/src/main/java/com/cloud/user/AccountManagerImpl.java
index 0a0d9265ac4..c7ceb00cb57 100644
--- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java
+++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java
@@ -43,6 +43,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;
@@ -722,6 +723,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.
diff --git
a/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java
b/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java
index cf2a43eceda..0604405f4a5 100644
--- a/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java
+++ b/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java
@@ -18,18 +18,10 @@
*/
package com.cloud.api.dispatch;
-import com.cloud.exception.ConcurrentOperationException;
-import com.cloud.exception.InsufficientCapacityException;
-import com.cloud.exception.NetworkRuleConflictException;
-import com.cloud.exception.ResourceAllocationException;
-import com.cloud.exception.ResourceUnavailableException;
-import com.cloud.user.Account;
-import com.cloud.user.AccountManager;
-import com.cloud.user.User;
-import org.apache.cloudstack.api.BaseCmd;
-import org.apache.cloudstack.api.Parameter;
-import org.apache.cloudstack.api.ServerApiException;
-import org.apache.cloudstack.context.CallContext;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
@@ -37,17 +29,53 @@ 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 java.util.HashMap;
+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;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+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 {
+ @Spy
+ @InjectMocks
+ private ParamProcessWorker paramProcessWorkerSpy;
+
+ @Mock
+ private AccountManager accountManagerMock;
+
+ @Mock
+ private Account callingAccountMock;
+
@Mock
- protected AccountManager accountManager;
+ private User callingUserMock;
- protected ParamProcessWorker paramProcessWorker;
+ @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 {
@@ -65,7 +93,7 @@ public class ParamProcessWorkerTest {
@Override
public void execute() throws ResourceUnavailableException,
InsufficientCapacityException, ServerApiException, ConcurrentOperationException,
- ResourceAllocationException, NetworkRuleConflictException {
+ ResourceAllocationException, NetworkRuleConflictException {
// well documented nothing
}
@@ -83,9 +111,7 @@ public class ParamProcessWorkerTest {
@Before
public void setup() {
- CallContext.register(Mockito.mock(User.class),
Mockito.mock(Account.class));
- paramProcessWorker = new ParamProcessWorker();
- paramProcessWorker._accountMgr = accountManager;
+ CallContext.register(callingUserMock, callingAccountMock);
}
@After
@@ -101,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);
+ }
}
diff --git a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java
b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java
index fe7748b8581..f8558992440 100644
--- a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java
+++ b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java
@@ -438,6 +438,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
