Skip to content

Commit b97bd3b

Browse files
DaanHooglandDaanHoogland
committed
fix quota resource access validation
1 parent 24d12f1 commit b97bd3b

File tree

9 files changed

+172
-37
lines changed

9 files changed

+172
-37
lines changed

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,8 @@ User createUser(String userName, String password, String firstName, String lastN
114114

115115
void checkAccess(Account account, AccessType accessType, boolean sameOwner, String apiName, ControlledEntity... entities) throws PermissionDeniedException;
116116

117+
void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource);
118+
117119
Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly);
118120

119121
/**

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
import javax.inject.Inject;
2323

24+
import com.cloud.user.Account;
25+
import org.apache.cloudstack.api.ACL;
2426
import org.apache.log4j.Logger;
2527
import org.apache.cloudstack.api.APICommand;
2628
import org.apache.cloudstack.api.ApiConstants;
@@ -42,6 +44,7 @@ public class QuotaBalanceCmd extends BaseCmd {
4244
@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Account Id for which statement needs to be generated")
4345
private String accountName;
4446

47+
@ACL
4548
@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.")
4649
private Long domainId;
4750

@@ -51,6 +54,7 @@ public class QuotaBalanceCmd extends BaseCmd {
5154
@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.")
5255
private Date startDate;
5356

57+
@ACL
5458
@Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "List usage records for the specified account")
5559
private Long accountId;
5660

@@ -104,7 +108,14 @@ public void setStartDate(Date startDate) {
104108

105109
@Override
106110
public long getEntityOwnerId() {
107-
return _accountService.getActiveAccountByName(accountName, domainId).getAccountId();
111+
if (accountId != null) {
112+
return accountId;
113+
}
114+
Account account = _accountService.getActiveAccountByName(accountName, domainId);
115+
if (account != null) {
116+
return account.getAccountId();
117+
}
118+
return Account.ACCOUNT_ID_SYSTEM;
108119
}
109120

110121
@Override

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.cloud.user.Account;
2020

21+
import org.apache.cloudstack.api.ACL;
2122
import org.apache.cloudstack.api.APICommand;
2223
import org.apache.cloudstack.api.ApiConstants;
2324
import org.apache.cloudstack.api.ApiErrorCode;
@@ -48,6 +49,7 @@ public class QuotaCreditsCmd extends BaseCmd {
4849
@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Account Id for which quota credits need to be added")
4950
private String accountName;
5051

52+
@ACL
5153
@Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = true, entityType = DomainResponse.class, description = "Domain for which quota credits need to be added")
5254
private Long domainId;
5355

@@ -132,6 +134,10 @@ public void execute() {
132134

133135
@Override
134136
public long getEntityOwnerId() {
137+
Account account = _accountService.getActiveAccountByName(accountName, domainId);
138+
if (account != null) {
139+
return account.getAccountId();
140+
}
135141
return Account.ACCOUNT_ID_SYSTEM;
136142
}
137143

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import javax.inject.Inject;
2323

24+
import org.apache.cloudstack.api.ACL;
2425
import org.apache.cloudstack.api.APICommand;
2526
import org.apache.cloudstack.api.ApiConstants;
2627
import org.apache.cloudstack.api.BaseCmd;
@@ -44,6 +45,7 @@ public class QuotaStatementCmd extends BaseCmd {
4445
@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Optional, Account Id for which statement needs to be generated")
4546
private String accountName;
4647

48+
@ACL
4749
@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.")
4850
private Long domainId;
4951

@@ -56,6 +58,7 @@ public class QuotaStatementCmd extends BaseCmd {
5658
@Parameter(name = ApiConstants.TYPE, type = CommandType.INTEGER, description = "List quota usage records for the specified usage type")
5759
private Integer usageType;
5860

61+
@ACL
5962
@Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "List usage records for the specified account")
6063
private Long accountId;
6164

@@ -112,6 +115,9 @@ public void setStartDate(Date startDate) {
112115

113116
@Override
114117
public long getEntityOwnerId() {
118+
if (accountId != null) {
119+
return accountId;
120+
}
115121
Account activeAccountByName = _accountService.getActiveAccountByName(accountName, domainId);
116122
if (activeAccountByName != null) {
117123
return activeAccountByName.getAccountId();

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,11 @@ public void checkAccess(Account account, AccessType accessType, boolean sameOwne
446446
// TODO Auto-generated method stub
447447
}
448448

449+
@Override
450+
public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
451+
// TODO Auto-generated method stub
452+
}
453+
449454
@Override
450455
public Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly) {
451456
// TODO Auto-generated method stub

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

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,6 @@
3232

3333
import javax.inject.Inject;
3434

35-
import org.apache.cloudstack.acl.ControlledEntity;
36-
import org.apache.cloudstack.acl.InfrastructureEntity;
3735
import org.apache.cloudstack.acl.SecurityChecker;
3836
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
3937
import org.apache.cloudstack.api.ACL;
@@ -288,40 +286,43 @@ public void processParameters(final BaseCmd cmd, final Map params) {
288286
doAccessChecks(cmd, entitiesToAccess);
289287
}
290288

291-
private void doAccessChecks(BaseCmd cmd, Map<Object, AccessType> entitiesToAccess) {
289+
protected void doAccessChecks(BaseCmd cmd, Map<Object, AccessType> entitiesToAccess) {
290+
Account[] owners = getEntityOwners(cmd);
291+
292292
Account caller = CallContext.current().getCallingAccount();
293+
if (cmd instanceof BaseAsyncCreateCmd) {
294+
// check that caller can access the owner account.
295+
_accountMgr.checkAccess(caller, null, false, owners);
296+
}
297+
298+
checkCallerAccessToEntities(caller, owners, entitiesToAccess);
299+
}
300+
301+
protected Account[] getEntityOwners(BaseCmd cmd) {
293302
List<Long> entityOwners = cmd.getEntityOwnerIds();
294-
Account[] owners = null;
295303
if (entityOwners != null) {
296-
owners = entityOwners.stream().map(id -> _accountMgr.getAccount(id)).toArray(Account[]::new);
304+
return entityOwners.stream().map(id -> _accountMgr.getAccount(id)).toArray(Account[]::new);
305+
}
306+
307+
if (cmd.getEntityOwnerId() == Account.ACCOUNT_ID_SYSTEM && cmd instanceof BaseAsyncCmd && cmd.getApiResourceType() == ApiCommandResourceType.Network) {
308+
s_logger.debug("Skipping access check on the network owner if the owner is ROOT/system.");
297309
} else {
298-
if (cmd.getEntityOwnerId() == Account.ACCOUNT_ID_SYSTEM && cmd instanceof BaseAsyncCmd && ((BaseAsyncCmd)cmd).getApiResourceType() == ApiCommandResourceType.Network) {
299-
if (s_logger.isDebugEnabled()) {
300-
s_logger.debug("Skipping access check on the network owner if the owner is ROOT/system.");
301-
}
302-
owners = new Account[]{};
303-
} else {
304-
owners = new Account[]{_accountMgr.getAccount(cmd.getEntityOwnerId())};
310+
Account owner = _accountMgr.getAccount(cmd.getEntityOwnerId());
311+
if (owner != null) {
312+
return new Account[]{owner};
305313
}
306314
}
315+
return new Account[]{};
316+
}
307317

308-
if (cmd instanceof BaseAsyncCreateCmd) {
309-
// check that caller can access the owner account.
310-
_accountMgr.checkAccess(caller, null, false, owners);
318+
protected void checkCallerAccessToEntities(Account caller, Account[] owners, Map<Object, AccessType> entitiesToAccess) {
319+
if (entitiesToAccess.isEmpty()) {
320+
return;
311321
}
312-
313-
if (!entitiesToAccess.isEmpty()) {
314-
// check that caller can access the owner account.
315-
_accountMgr.checkAccess(caller, null, false, owners);
316-
for (Map.Entry<Object,AccessType>entry : entitiesToAccess.entrySet()) {
317-
Object entity = entry.getKey();
318-
if (entity instanceof ControlledEntity) {
319-
_accountMgr.checkAccess(caller, entry.getValue(), true, (ControlledEntity) entity);
320-
} else if (entity instanceof InfrastructureEntity) {
321-
// FIXME: Move this code in adapter, remove code from
322-
// Account manager
323-
}
324-
}
322+
_accountMgr.checkAccess(caller, null, false, owners);
323+
for (Map.Entry<Object, AccessType> entry : entitiesToAccess.entrySet()) {
324+
Object entity = entry.getKey();
325+
_accountMgr.validateAccountHasAccessToResource(caller, entry.getValue(), entity);
325326
}
326327
}
327328

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242

4343
import org.apache.cloudstack.acl.APIChecker;
4444
import org.apache.cloudstack.acl.ControlledEntity;
45+
import org.apache.cloudstack.acl.InfrastructureEntity;
4546
import org.apache.cloudstack.acl.QuerySelector;
4647
import org.apache.cloudstack.acl.Role;
4748
import org.apache.cloudstack.acl.RoleService;
@@ -719,6 +720,19 @@ public void checkAccess(Account caller, AccessType accessType, boolean sameOwner
719720

720721
}
721722

723+
@Override
724+
public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
725+
Class<?> resourceClass = resource.getClass();
726+
if (ControlledEntity.class.isAssignableFrom(resourceClass)) {
727+
checkAccess(account, accessType, true, (ControlledEntity) resource);
728+
} else if (Domain.class.isAssignableFrom(resourceClass)) {
729+
checkAccess(account, (Domain) resource);
730+
} else if (InfrastructureEntity.class.isAssignableFrom(resourceClass)) {
731+
s_logger.trace("Validation of access to infrastructure entity has been disabled in CloudStack version 4.4.");
732+
}
733+
s_logger.debug(String.format("Account [%s] has access to resource.", account.getUuid()));
734+
}
735+
722736
@Override
723737
public Long checkAccessAndSpecifyAuthority(Account caller, Long zoneId) {
724738
// 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‎

Lines changed: 93 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
package com.cloud.api.dispatch;
2020

2121
import java.util.HashMap;
22+
import java.util.List;
23+
import java.util.Map;
2224

2325
import org.junit.After;
2426
import org.junit.Assert;
@@ -27,11 +29,16 @@
2729
import org.junit.runner.RunWith;
2830
import org.mockito.Mock;
2931
import org.mockito.Mockito;
30-
import org.mockito.runners.MockitoJUnitRunner;
32+
import org.mockito.InjectMocks;
33+
import org.mockito.Spy;
34+
35+
import org.mockito.junit.MockitoJUnitRunner;
3136

3237
import org.apache.cloudstack.api.BaseCmd;
3338
import org.apache.cloudstack.api.Parameter;
3439
import org.apache.cloudstack.api.ServerApiException;
40+
import org.apache.cloudstack.api.command.user.address.AssociateIPAddrCmd;
41+
import org.apache.cloudstack.acl.SecurityChecker;
3542
import org.apache.cloudstack.context.CallContext;
3643

3744
import com.cloud.exception.ConcurrentOperationException;
@@ -42,14 +49,33 @@
4249
import com.cloud.user.Account;
4350
import com.cloud.user.AccountManager;
4451
import com.cloud.user.User;
52+
import com.cloud.vm.VMInstanceVO;
4553

4654
@RunWith(MockitoJUnitRunner.class)
4755
public class ParamProcessWorkerTest {
4856

57+
@Spy
58+
@InjectMocks
59+
private ParamProcessWorker paramProcessWorkerSpy;
60+
61+
@Mock
62+
private AccountManager accountManagerMock;
63+
64+
@Mock
65+
private Account callingAccountMock;
66+
67+
@Mock
68+
private User callingUserMock;
69+
70+
@Mock
71+
private Account ownerAccountMock;
72+
4973
@Mock
50-
protected AccountManager accountManager;
74+
BaseCmd baseCmdMock;
5175

52-
protected ParamProcessWorker paramProcessWorker;
76+
private Account[] owners = new Account[]{ownerAccountMock};
77+
78+
private Map<Object, SecurityChecker.AccessType> entities = new HashMap<>();
5379

5480
public static class TestCmd extends BaseCmd {
5581

@@ -67,7 +93,7 @@ public static class TestCmd extends BaseCmd {
6793

6894
@Override
6995
public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException,
70-
ResourceAllocationException, NetworkRuleConflictException {
96+
ResourceAllocationException, NetworkRuleConflictException {
7197
// well documented nothing
7298
}
7399

@@ -85,9 +111,7 @@ public long getEntityOwnerId() {
85111

86112
@Before
87113
public void setup() {
88-
CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
89-
paramProcessWorker = new ParamProcessWorker();
90-
paramProcessWorker._accountMgr = accountManager;
114+
CallContext.register(callingUserMock, callingAccountMock);
91115
}
92116

93117
@After
@@ -103,10 +127,71 @@ public void processParameters() {
103127
params.put("boolparam1", "true");
104128
params.put("doubleparam1", "11.89");
105129
final TestCmd cmd = new TestCmd();
106-
paramProcessWorker.processParameters(cmd, params);
130+
paramProcessWorkerSpy.processParameters(cmd, params);
107131
Assert.assertEquals("foo", cmd.strparam1);
108132
Assert.assertEquals(100, cmd.intparam1);
109133
Assert.assertTrue(Double.compare(cmd.doubleparam1, 11.89) == 0);
134+
Mockito.verify(paramProcessWorkerSpy).doAccessChecks(Mockito.any(), Mockito.any());
135+
}
136+
137+
@Test
138+
public void doAccessChecksTestChecksCallerAccessToOwnerWhenCmdExtendsBaseAsyncCreateCmd() {
139+
Mockito.doReturn(owners).when(paramProcessWorkerSpy).getEntityOwners(Mockito.any());
140+
Mockito.doNothing().when(paramProcessWorkerSpy).checkCallerAccessToEntities(Mockito.any(), Mockito.any(), Mockito.any());
141+
142+
paramProcessWorkerSpy.doAccessChecks(new AssociateIPAddrCmd(), entities);
143+
144+
Mockito.verify(accountManagerMock).checkAccess(callingAccountMock, null, false, owners);
145+
}
146+
147+
@Test
148+
public void doAccessChecksTestChecksCallerAccessToEntities() {
149+
Mockito.doReturn(owners).when(paramProcessWorkerSpy).getEntityOwners(Mockito.any());
150+
Mockito.doNothing().when(paramProcessWorkerSpy).checkCallerAccessToEntities(Mockito.any(), Mockito.any(), Mockito.any());
151+
152+
paramProcessWorkerSpy.doAccessChecks(new AssociateIPAddrCmd(), entities);
153+
154+
Mockito.verify(paramProcessWorkerSpy).checkCallerAccessToEntities(callingAccountMock, owners, entities);
155+
}
156+
157+
@Test
158+
public void getEntityOwnersTestReturnsAccountsWhenCmdHasMultipleEntityOwners() {
159+
Mockito.when(baseCmdMock.getEntityOwnerIds()).thenReturn(List.of(1L, 2L));
160+
Mockito.doReturn(callingAccountMock).when(accountManagerMock).getAccount(1L);
161+
Mockito.doReturn(ownerAccountMock).when(accountManagerMock).getAccount(2L);
162+
163+
List<Account> result = List.of(paramProcessWorkerSpy.getEntityOwners(baseCmdMock));
164+
165+
Assert.assertEquals(List.of(callingAccountMock, ownerAccountMock), result);
166+
}
167+
168+
@Test
169+
public void getEntityOwnersTestReturnsAccountWhenCmdHasOneEntityOwner() {
170+
Mockito.when(baseCmdMock.getEntityOwnerId()).thenReturn(1L);
171+
Mockito.when(baseCmdMock.getEntityOwnerIds()).thenReturn(null);
172+
Mockito.doReturn(ownerAccountMock).when(accountManagerMock).getAccount(1L);
173+
174+
List<Account> result = List.of(paramProcessWorkerSpy.getEntityOwners(baseCmdMock));
175+
176+
Assert.assertEquals(List.of(ownerAccountMock), result);
177+
}
178+
179+
@Test
180+
public void checkCallerAccessToEntitiesTestChecksCallerAccessToOwners() {
181+
entities.put(ownerAccountMock, SecurityChecker.AccessType.UseEntry);
182+
183+
paramProcessWorkerSpy.checkCallerAccessToEntities(callingAccountMock, owners, entities);
184+
185+
Mockito.verify(accountManagerMock).checkAccess(callingAccountMock, null, false, owners);
110186
}
111187

188+
@Test
189+
public void checkCallerAccessToEntitiesTestChecksCallerAccessToResource() {
190+
VMInstanceVO vmInstanceVo = new VMInstanceVO();
191+
entities.put(vmInstanceVo, SecurityChecker.AccessType.UseEntry);
192+
193+
paramProcessWorkerSpy.checkCallerAccessToEntities(callingAccountMock, owners, entities);
194+
195+
Mockito.verify(accountManagerMock).validateAccountHasAccessToResource(callingAccountMock, SecurityChecker.AccessType.UseEntry, vmInstanceVo);
196+
}
112197
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,11 @@ public void checkAccess(Account account, AccessType accessType, boolean sameOwne
436436
// TODO Auto-generated method stub
437437
}
438438

439+
@Override
440+
public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
441+
// TODO Auto-generated method stub
442+
}
443+
439444
@Override
440445
public Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly) {
441446
// TODO Auto-generated method stub

0 commit comments

Comments
 (0)