Skip to content

server: optimize account creation by pre-loading the role permissions #11137

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.

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

Draft
wants to merge 1 commit into
base: 4.20
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/src/main/java/org/apache/cloudstack/acl/APIChecker.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.cloud.exception.PermissionDeniedException;
import com.cloud.user.Account;
import com.cloud.user.User;
import com.cloud.utils.Pair;
import com.cloud.utils.component.Adapter;

import java.util.List;
Expand All @@ -43,4 +44,7 @@ public interface APIChecker extends Adapter {
*/
List<String> getApisAllowedToUser(Role role, User user, List<String> apiNames) throws PermissionDeniedException;
boolean isEnabled();

Pair<Role, List<RolePermission>> getRolePermissions(long roleId);
boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions);
}
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@
return accountService.getAccount(accountId);
}

protected Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
@Override
public Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
final Role accountRole = roleService.findRole(roleId);
if (accountRole == null || accountRole.getId() < 1L) {
return new Pair<>(null, null);
Expand Down Expand Up @@ -149,7 +150,7 @@
throw new PermissionDeniedException(String.format("Account role for user id [%s] cannot be found.", user.getUuid()));
}
if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId() == RoleType.Admin.getId()) {
logger.info("Account for user id {} is Root Admin or Domain Admin, all APIs are allowed.", user.getUuid());
logger.info("Account for user id {} is Root Admin, all APIs are allowed.", user.getUuid());
return true;
}
List<RolePermission> allPermissions = roleAndPermissions.second();
Expand Down Expand Up @@ -180,6 +181,25 @@
throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account));
}

@Override
public boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) {

Check warning on line 185 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java#L185

Added line #L185 was not covered by tests
if (accountRole == null) {
throw new PermissionDeniedException(String.format("The account [%s] has role null or unknown.", account));

Check warning on line 187 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java#L187

Added line #L187 was not covered by tests
}

if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId() == RoleType.Admin.getId()) {
if (logger.isTraceEnabled()) {
logger.trace(String.format("Account [%s] is Root Admin, all APIs are allowed.", account));

Check warning on line 192 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java#L192

Added line #L192 was not covered by tests
}
return true;

Check warning on line 194 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java#L194

Added line #L194 was not covered by tests
}

if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) {
return true;

Check warning on line 198 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java#L198

Added line #L198 was not covered by tests
}
throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account));
}

Check warning on line 201 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java#L200-L201

Added lines #L200 - L201 were not covered by tests

/**
* Only one strategy should be used between StaticRoleBasedAPIAccessChecker and DynamicRoleBasedAPIAccessChecker
* Default behavior is to use the Dynamic version. The StaticRoleBasedAPIAccessChecker is the legacy version.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@

import javax.inject.Inject;
import javax.naming.ConfigurationException;
import org.apache.cloudstack.acl.RolePermissionEntity.Permission;

import org.apache.cloudstack.acl.RolePermissionEntity.Permission;
import org.apache.cloudstack.context.CallContext;

import com.cloud.exception.PermissionDeniedException;
Expand All @@ -33,6 +33,7 @@
import com.cloud.user.Account;
import com.cloud.user.AccountService;
import com.cloud.user.User;
import com.cloud.utils.Pair;
import com.cloud.utils.component.AdapterBase;
import com.cloud.utils.component.PluggableService;

Expand Down Expand Up @@ -195,4 +196,14 @@
public void setServices(List<PluggableService> services) {
this.services = services;
}

@Override
public Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
return null;
}

Check warning on line 203 in plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java#L201-L203

Added lines #L201 - L203 were not covered by tests

@Override
public boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) {
return false;
}

Check warning on line 208 in plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java#L206-L208

Added lines #L206 - L208 were not covered by tests
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import com.cloud.user.Account;
import com.cloud.user.AccountService;
import com.cloud.user.User;
import com.cloud.utils.Pair;
import com.cloud.utils.PropertiesUtil;
import com.cloud.utils.component.AdapterBase;
import com.cloud.utils.component.PluggableService;
Expand Down Expand Up @@ -200,4 +201,13 @@
this.commandPropertyFiles = commandPropertyFiles;
}

@Override
public Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
return null;

Check warning on line 206 in plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java#L206

Added line #L206 was not covered by tests
}

@Override
public boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) {
return false;

Check warning on line 211 in plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java#L211

Added line #L211 was not covered by tests
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import net.sf.ehcache.CacheManager;

import org.apache.cloudstack.acl.Role;
import org.apache.cloudstack.acl.RolePermission;
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
import org.springframework.stereotype.Component;

Expand All @@ -42,6 +43,7 @@
import com.cloud.user.Account;
import com.cloud.user.AccountService;
import com.cloud.user.User;
import com.cloud.utils.Pair;
import com.cloud.utils.component.AdapterBase;

@Component
Expand Down Expand Up @@ -256,4 +258,14 @@
this.enabled = enabled;

}

@Override
public Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
return null;
}

Check warning on line 265 in plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java

View check run for this annotation

Codecov / codecov/patch

plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java#L263-L265

Added lines #L263 - L265 were not covered by tests

@Override
public boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) {
return false;
}

Check warning on line 270 in plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java

View check run for this annotation

Codecov / codecov/patch

plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java#L268-L270

Added lines #L268 - L270 were not covered by tests
}
35 changes: 21 additions & 14 deletions server/src/main/java/com/cloud/user/AccountManagerImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.apache.cloudstack.acl.InfrastructureEntity;
import org.apache.cloudstack.acl.QuerySelector;
import org.apache.cloudstack.acl.Role;
import org.apache.cloudstack.acl.RolePermission;
import org.apache.cloudstack.acl.RoleService;
import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.acl.SecurityChecker;
Expand Down Expand Up @@ -1431,29 +1432,35 @@
requested.getUuid(),
requested.getRoleId()));
}
if (caller.getRoleId().equals(requested.getRoleId())) {
return;

Check warning on line 1436 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L1436 was not covered by tests
}
List<APIChecker> apiCheckers = getEnabledApiCheckers();
for (APIChecker apiChecker : apiCheckers) {
checkApiAccess(apiChecker, caller, requested);
}
}

Check warning on line 1442 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1440-L1442

Added lines #L1440 - L1442 were not covered by tests

private void checkApiAccess(APIChecker apiChecker, Account caller, Account requested) throws PermissionDeniedException {
Pair<Role, List<RolePermission>> roleAndPermissionsForCaller = apiChecker.getRolePermissions(caller.getRoleId());
Pair<Role, List<RolePermission>> roleAndPermissionsForRequested = apiChecker.getRolePermissions(requested.getRoleId());

Check warning on line 1446 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1444-L1446

Added lines #L1444 - L1446 were not covered by tests
for (String command : apiNameList) {
try {
checkApiAccess(apiCheckers, requested, command);
} catch (PermissionDeniedException pde) {
if (logger.isTraceEnabled()) {
logger.trace(String.format(
"Checking for permission to \"%s\" is irrelevant as it is not requested for %s [%s]",
command,
requested.getAccountName(),
requested.getUuid()
)
);
if (roleAndPermissionsForRequested == null) {
apiChecker.checkAccess(caller, command);

Check warning on line 1450 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L1450 was not covered by tests
} else {
apiChecker.checkAccess(caller, command, roleAndPermissionsForRequested.first(), roleAndPermissionsForRequested.second());

Check warning on line 1452 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L1452 was not covered by tests
}
} catch (PermissionDeniedException pde) {

Check warning on line 1454 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L1454 was not covered by tests
continue;
}
// so requested can, now make sure caller can as well
try {
if (logger.isTraceEnabled()) {
logger.trace(String.format("permission to \"%s\" is requested",
command));
if (roleAndPermissionsForCaller == null) {
apiChecker.checkAccess(caller, command);

Check warning on line 1460 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L1460 was not covered by tests
} else {
apiChecker.checkAccess(caller, command, roleAndPermissionsForCaller.first(), roleAndPermissionsForCaller.second());

Check warning on line 1462 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L1462 was not covered by tests
}
checkApiAccess(apiCheckers, caller, command);
} catch (PermissionDeniedException pde) {
String msg = String.format("User of Account %s and domain %s can not create an account with access to more privileges they have themself.",
caller, _domainMgr.getDomain(caller.getDomainId()));
Expand Down
Loading