Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added recursive fetch of child domains for listUsageRecords API call #4717

Merged
merged 10 commits into from Apr 10, 2021
Merged

Added recursive fetch of child domains for listUsageRecords API call #4717

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0da721a
Added recursive fetch of domains for listUsageRecords API call
Spaceman1984 Feb 22, 2021
e66d92b
Added security check for domain admin
Spaceman1984 Feb 22, 2021
7a70230
Review changes
Spaceman1984 Feb 26, 2021
9270df4
Changed account check
Spaceman1984 Mar 1, 2021
5d43c1c
Removed unused var
Spaceman1984 Mar 1, 2021
71c7c71
Adding recursive check back
Spaceman1984 Mar 1, 2021
19fd612
Changed logic around setting the account id
Spaceman1984 Mar 1, 2021
a45fcee
Fixed null account issue
Spaceman1984 Mar 16, 2021
83c9b0a
Refactored code
Spaceman1984 Mar 17, 2021
80860ee
Added message regarding account id
Spaceman1984 Mar 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListUsageRecordsCmd.java
View file
Open in desktop
Expand Up @@ -85,6 +85,11 @@ public class ListUsageRecordsCmd extends BaseListCmd {
@Parameter(name = ApiConstants.OLD_FORMAT, type = CommandType.BOOLEAN, description = "Flag to enable description rendered in old format which uses internal database IDs instead of UUIDs. False by default.")
private Boolean oldFormat;

@Parameter(name = ApiConstants.IS_RECURSIVE, type = CommandType.BOOLEAN,
description = "Specify if usage records should be fetched recursively per domain.",
since = "4.15")
private Boolean recursive = false;

/////////////////////////////////////////////////////
/////////////////// Accessors ///////////////////////
/////////////////////////////////////////////////////
Expand Down Expand Up @@ -153,6 +158,10 @@ public boolean getOldFormat() {
return oldFormat != null && oldFormat;
}

public Boolean isRecursive() {
return recursive;
}

/////////////////////////////////////////////////////
/////////////// API Implementation///////////////////
/////////////////////////////////////////////////////
Expand Down
66 changes: 51 additions & 15 deletions server/src/main/java/com/cloud/usage/UsageServiceImpl.java
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@
import javax.inject.Inject;
import javax.naming.ConfigurationException;

import com.cloud.domain.Domain;
import org.apache.cloudstack.api.command.admin.usage.GenerateUsageRecordsCmd;
import org.apache.cloudstack.api.command.admin.usage.ListUsageRecordsCmd;
import org.apache.cloudstack.api.command.admin.usage.RemoveRawUsageRecordsCmd;
Expand Down Expand Up @@ -201,7 +202,8 @@ public Pair<List<? extends Usage>, Integer> getUsageRecords(ListUsageRecordsCmd
}

boolean isAdmin = false;
Spaceman1984 marked this conversation as resolved.
Show resolved Hide resolved
boolean isDomainAdmin = false;
boolean isDomainAdmin = _accountService.isDomainAdmin(caller.getId());
boolean isNormalUser = _accountService.isNormalUser(caller.getId());

//If accountId couldn't be found using accountName and domainId, get it from userContext
if (accountId == null) {
Expand All @@ -210,12 +212,45 @@ public Pair<List<? extends Usage>, Integer> getUsageRecords(ListUsageRecordsCmd
//If account_id or account_name is explicitly mentioned, list records for the specified account only even if the caller is of type admin
if (_accountService.isRootAdmin(caller.getId())) {
isAdmin = true;
} else if (_accountService.isDomainAdmin(caller.getId())) {
isDomainAdmin = true;
}
s_logger.debug("Account details not available. Using userContext accountId: " + accountId);
}

// Check if a domain admin is allowed to access the requested domain id
if (isDomainAdmin) {
if (domainId != null) {
Account callerAccount = _accountService.getAccount(caller.getId());
Domain domain = _domainDao.findById(domainId);
_accountService.checkAccess(callerAccount, domain);
} else {
// Domain admins can only access their own domain's usage records.
// Set the domain if not specified.
domainId = caller.getDomainId();
}

// Check if a domain admin is allowed to access the requested account info.
Account account = _accountService.getAccount(accountId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Spaceman1984 should we do a checkAccess() here as well with the provided caller and account?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhtyd Using checkAccess() here will result in querying the database for every child domain. I'm querying the database once by asking for all the child domains of the current domain and checking if the requested account belongs to anyone of them.

Domain domain = _domainDao.findById(caller.getDomainId());
_accountService.checkAccess(account, domain);
Spaceman1984 marked this conversation as resolved.
Show resolved Hide resolved
}

Spaceman1984 marked this conversation as resolved.
Show resolved Hide resolved
// By default users do not have access to this API.
// Adding checks here in case someone changes the default access.
if (isNormalUser) {
// A user can only access their own account records
if (caller.getId() != accountId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

caller.getAccountId() (in case getAccountId is not same as getId)

throw new PermissionDeniedException("Users are only allowed to list usage records for their own account.");
}
// Users cannot get recursive records
if (cmd.isRecursive()) {
throw new PermissionDeniedException("Users are not allowed to list usage records recursively.");
}
// Users cannot get domain records
if (cmd.getDomainId() != null) {
throw new PermissionDeniedException("Users are not allowed to list usage records for a domain");
}
}

Date startDate = cmd.getStartDate();
Date endDate = cmd.getEndDate();
if (startDate.after(endDate)) {
Expand All @@ -234,22 +269,23 @@ public Pair<List<? extends Usage>, Integer> getUsageRecords(ListUsageRecordsCmd

SearchCriteria<UsageVO> sc = _usageDao.createSearchCriteria();

if (accountId != -1 && accountId != Account.ACCOUNT_ID_SYSTEM && !isAdmin && !isDomainAdmin) {
if (accountId != -1 && accountId != Account.ACCOUNT_ID_SYSTEM && !isAdmin && !cmd.isRecursive()) {
Spaceman1984 marked this conversation as resolved.
Show resolved Hide resolved
sc.addAnd("accountId", SearchCriteria.Op.EQ, accountId);
}

if (isDomainAdmin) {
SearchCriteria<DomainVO> sdc = _domainDao.createSearchCriteria();
sdc.addOr("path", SearchCriteria.Op.LIKE, _domainDao.findById(caller.getDomainId()).getPath() + "%");
List<DomainVO> domains = _domainDao.search(sdc, null);
List<Long> domainIds = new ArrayList<Long>();
for (DomainVO domain : domains)
domainIds.add(domain.getId());
sc.addAnd("domainId", SearchCriteria.Op.IN, domainIds.toArray());
}

if (domainId != null) {
sc.addAnd("domainId", SearchCriteria.Op.EQ, domainId);
if (cmd.isRecursive()) {
SearchCriteria<DomainVO> sdc = _domainDao.createSearchCriteria();
sdc.addOr("path", SearchCriteria.Op.LIKE, _domainDao.findById(domainId).getPath() + "%");
List<DomainVO> domains = _domainDao.search(sdc, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify, reuse existing DomainDao::findAllChildren

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhtyd Using findAllChildren here would not pass the top domain.

List<Long> domainIds = new ArrayList<Long>();
for (DomainVO domain : domains) {
domainIds.add(domain.getId());
}
sc.addAnd("domainId", SearchCriteria.Op.IN, domainIds.toArray());
} else {
sc.addAnd("domainId", SearchCriteria.Op.EQ, domainId);
}
}

if (usageType != null) {
Expand Down