Skip to content

Commit 123b426

Browse files
DaanHooglandDaanHoogland
committed
fix quota resource access validation
1 parent 8a2f652 commit 123b426

File tree

9 files changed

+187
-50
lines changed

9 files changed

+187
-50
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
@@ -116,6 +116,8 @@ User createUser(String userName, String password, String firstName, String lastN
116116

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

119+
void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource);
120+
119121
Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly);
120122

121123
/**

‎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
@@ -451,6 +451,11 @@ public void checkAccess(Account account, AccessType accessType, boolean sameOwne
451451
// TODO Auto-generated method stub
452452
}
453453

454+
@Override
455+
public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
456+
// TODO Auto-generated method stub
457+
}
458+
454459
@Override
455460
public Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly) {
456461
// 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
@@ -43,6 +43,7 @@
4343

4444
import org.apache.cloudstack.acl.APIChecker;
4545
import org.apache.cloudstack.acl.ControlledEntity;
46+
import org.apache.cloudstack.acl.InfrastructureEntity;
4647
import org.apache.cloudstack.acl.QuerySelector;
4748
import org.apache.cloudstack.acl.Role;
4849
import org.apache.cloudstack.acl.RoleService;
@@ -722,6 +723,19 @@ public void checkAccess(Account caller, AccessType accessType, boolean sameOwner
722723

723724
}
724725

726+
@Override
727+
public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
728+
Class<?> resourceClass = resource.getClass();
729+
if (ControlledEntity.class.isAssignableFrom(resourceClass)) {
730+
checkAccess(account, accessType, true, (ControlledEntity) resource);
731+
} else if (Domain.class.isAssignableFrom(resourceClass)) {
732+
checkAccess(account, (Domain) resource);
733+
} else if (InfrastructureEntity.class.isAssignableFrom(resourceClass)) {
734+
s_logger.trace("Validation of access to infrastructure entity has been disabled in CloudStack version 4.4.");
735+
}
736+
s_logger.debug(String.format("Account [%s] has access to resource.", account.getUuid()));
737+
}
738+
725739
@Override
726740
public Long checkAccessAndSpecifyAuthority(Account caller, Long zoneId) {
727741
// We just care for resource domain admins for now, and they should be permitted to see only their zone.

0 commit comments

Comments
 (0)