From 71bc088a707b3e9ba526f6185182f156d20a9e94 Mon Sep 17 00:00:00 2001 From: Bryan Lima <42067040+BryanMLima@users.noreply.github.com> Date: Wed, 20 Jul 2022 03:00:17 -0300 Subject: [PATCH] Improve login time (#6412) * Improve slow login * Address review * Address Daan's review * Address Daniel reviews --- .../org/apache/cloudstack/acl/APIChecker.java | 15 +- .../java/com/cloud/projects/ProjectVO.java | 5 +- .../main/java/com/cloud/user/AccountVO.java | 3 +- .../src/main/java/com/cloud/user/UserVO.java | 3 +- .../org/apache/cloudstack/acl/RoleVO.java | 6 + .../acl/DynamicRoleBasedAPIAccessChecker.java | 84 ++++++--- .../DynamicRoleBasedAPIAccessCheckerTest.java | 84 ++++++--- .../acl/ProjectRoleBasedApiAccessChecker.java | 83 ++++++--- .../ProjectRoleBasedApiAccessCheckerTest.java | 154 ++++++++++++++++ .../acl/StaticRoleBasedAPIAccessChecker.java | 55 ++++-- .../discovery/ApiDiscoveryServiceImpl.java | 52 ++++-- .../discovery/ApiDiscoveryTest.java | 174 +++++++++--------- .../ratelimit/ApiRateLimitServiceImpl.java | 71 +++++-- 13 files changed, 578 insertions(+), 211 deletions(-) create mode 100644 plugins/acl/project-role-based/src/main/test/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessCheckerTest.java diff --git a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java index 6cf45456e78..660f64f43ef 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -21,7 +21,11 @@ import com.cloud.user.Account; import com.cloud.user.User; import com.cloud.utils.component.Adapter; -// APIChecker checks the ownership and access control to API requests +import java.util.List; + +/** + * APICheckers is designed to verify the ownership of resources and to control the access to APIs. + */ public interface APIChecker extends Adapter { // Interface for checking access for a role using apiname // If true, apiChecker has checked the operation @@ -29,5 +33,14 @@ public interface APIChecker extends Adapter { // On exception, checkAccess failed don't allow boolean checkAccess(User user, String apiCommandName) throws PermissionDeniedException; boolean checkAccess(Account account, String apiCommandName) throws PermissionDeniedException; + /** + * Verifies if the account has permission for the given list of APIs and returns only the allowed ones. + * + * @param role of the user to be verified + * @param user to be verified + * @param apiNames the list of apis to be verified + * @return the list of allowed apis for the given user + */ + List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException; boolean isEnabled(); } diff --git a/engine/schema/src/main/java/com/cloud/projects/ProjectVO.java b/engine/schema/src/main/java/com/cloud/projects/ProjectVO.java index 77eed40d7ba..c8faa00812c 100644 --- a/engine/schema/src/main/java/com/cloud/projects/ProjectVO.java +++ b/engine/schema/src/main/java/com/cloud/projects/ProjectVO.java @@ -30,6 +30,7 @@ import javax.persistence.Table; import org.apache.cloudstack.api.Identity; import org.apache.cloudstack.api.InternalIdentity; +import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import com.cloud.utils.NumbersUtil; import com.cloud.utils.db.GenericDao; @@ -116,9 +117,7 @@ public class ProjectVO implements Project, Identity, InternalIdentity { @Override public String toString() { - StringBuilder buf = new StringBuilder("Project["); - buf.append(id).append("|name=").append(name).append("|domainid=").append(domainId).append("]"); - return buf.toString(); + return String.format("Project %s.", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "name", "uuid", "domainId")); } @Override diff --git a/engine/schema/src/main/java/com/cloud/user/AccountVO.java b/engine/schema/src/main/java/com/cloud/user/AccountVO.java index 5a6170ac7aa..50793fbd208 100644 --- a/engine/schema/src/main/java/com/cloud/user/AccountVO.java +++ b/engine/schema/src/main/java/com/cloud/user/AccountVO.java @@ -18,6 +18,7 @@ package com.cloud.user; import com.cloud.utils.db.GenericDao; import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import javax.persistence.Column; import javax.persistence.Entity; @@ -189,7 +190,7 @@ public class AccountVO implements Account { @Override public String toString() { - return String.format("Acct[%s-%s] -- Account {\"id\": %s, \"name\": \"%s\", \"uuid\": \"%s\"}", uuid, accountName, id, accountName, uuid); + return String.format("Account [%s]", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "uuid","accountName", "id")); } @Override diff --git a/engine/schema/src/main/java/com/cloud/user/UserVO.java b/engine/schema/src/main/java/com/cloud/user/UserVO.java index 027020fc546..94e61ff14a8 100644 --- a/engine/schema/src/main/java/com/cloud/user/UserVO.java +++ b/engine/schema/src/main/java/com/cloud/user/UserVO.java @@ -30,6 +30,7 @@ import javax.persistence.Table; import org.apache.cloudstack.api.Identity; import org.apache.cloudstack.api.InternalIdentity; +import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import com.cloud.user.Account.State; import com.cloud.utils.db.Encrypt; @@ -283,7 +284,7 @@ public class UserVO implements User, Identity, InternalIdentity { @Override public String toString() { - return new StringBuilder("User[").append(id).append("-").append(username).append("]").toString(); + return String.format("User %s.", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "username", "uuid")); } @Override diff --git a/engine/schema/src/main/java/org/apache/cloudstack/acl/RoleVO.java b/engine/schema/src/main/java/org/apache/cloudstack/acl/RoleVO.java index 823dc69e870..f5a0cebc75f 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/acl/RoleVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/acl/RoleVO.java @@ -18,6 +18,7 @@ package org.apache.cloudstack.acl; import com.cloud.utils.db.GenericDao; +import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import javax.persistence.Column; import javax.persistence.Entity; @@ -114,4 +115,9 @@ public class RoleVO implements Role { public boolean isDefault() { return isDefault; } + + @Override + public String toString() { + return ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "name", "uuid", "roleType"); + } } diff --git a/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java b/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java index a73a5c0a075..cca9e338868 100644 --- a/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java +++ b/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java @@ -16,6 +16,7 @@ // under the License. package org.apache.cloudstack.acl; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -48,7 +49,7 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API private List services; private Map> annotationRoleBasedApisMap = new HashMap>(); - private static final Logger logger = Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class.getName()); + private static final Logger LOGGER = Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class.getName()); protected DynamicRoleBasedAPIAccessChecker() { super(); @@ -57,22 +58,58 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API } } - private void denyApiAccess(final String commandName) throws PermissionDeniedException { - throw new PermissionDeniedException("The API " + commandName + " is denied for the account's role."); + @Override + public List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException { + if (!isEnabled()) { + return apiNames; + } + + List allPermissions = roleService.findAllPermissionsBy(role.getId()); + List allowedApis = new ArrayList<>(); + for (String api : apiNames) { + if (checkApiPermissionByRole(role, api, allPermissions)) { + allowedApis.add(api); + } + } + return allowedApis; } - public boolean isDisabled() { - return !roleService.isEnabled(); + /** + * Checks if the given Role of an Account has the allowed permission for the given API. + * + * @param role to be used on the verification + * @param apiName to be verified + * @param allPermissions list of role permissions for the given role + * @return if the role has the permission for the API + */ + public boolean checkApiPermissionByRole(Role role, String apiName, List allPermissions) { + for (final RolePermission permission : allPermissions) { + if (!permission.getRule().matches(apiName)) { + continue; + } + + if (!Permission.ALLOW.equals(permission.getPermission())) { + return false; + } + + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(String.format("The API [%s] is allowed for the role %s by the permission [%s].", apiName, role, permission.getRule().toString())); + } + return true; + } + return annotationRoleBasedApisMap.get(role.getRoleType()) != null && + annotationRoleBasedApisMap.get(role.getRoleType()).contains(apiName); } @Override public boolean checkAccess(User user, String commandName) throws PermissionDeniedException { - if (isDisabled()) { + if (!isEnabled()) { return true; } + Account account = accountService.getAccount(user.getAccountId()); if (account == null) { - throw new PermissionDeniedException("The account id=" + user.getAccountId() + "for user id=" + user.getId() + "is null"); + throw new PermissionDeniedException(String.format("The account id [%s] for user id [%s] is null.", user.getAccountId(), user.getUuid())); } return checkAccess(account, commandName); @@ -81,37 +118,32 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API public boolean checkAccess(Account account, String commandName) { final Role accountRole = roleService.findRole(account.getRoleId()); if (accountRole == null || accountRole.getId() < 1L) { - denyApiAccess(commandName); + throw new PermissionDeniedException(String.format("The account [%s] has role null or unknown.", account)); } - // Allow all APIs for root admins if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId() == RoleType.Admin.getId()) { + LOGGER.info(String.format("Account [%s] is Root Admin or Domain Admin, all APIs are allowed.", account)); return true; } - // Check against current list of permissions - for (final RolePermission permission : roleService.findAllPermissionsBy(accountRole.getId())) { - if (permission.getRule().matches(commandName)) { - if (Permission.ALLOW.equals(permission.getPermission())) { - return true; - } else { - denyApiAccess(commandName); - } - } - } - - // Check annotations - if (annotationRoleBasedApisMap.get(accountRole.getRoleType()) != null - && annotationRoleBasedApisMap.get(accountRole.getRoleType()).contains(commandName)) { + List allPermissions = roleService.findAllPermissionsBy(accountRole.getId()); + if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) { return true; } - - // Default deny all - throw new UnavailableCommandException("The API " + commandName + " does not exist or is not available for this account."); + throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account)); } + /** + * Only one strategy should be used between StaticRoleBasedAPIAccessChecker and DynamicRoleBasedAPIAccessChecker + * Default behavior is to use the Dynamic version. The StaticRoleBasedAPIAccessChecker is the legacy version. + * If roleService is enabled, then it uses the DynamicRoleBasedAPIAccessChecker, otherwise, it will use the + * StaticRoleBasedAPIAccessChecker. + */ @Override public boolean isEnabled() { + if (!roleService.isEnabled()) { + LOGGER.trace("RoleService is disabled. We will not use DynamicRoleBasedAPIAccessChecker."); + } return roleService.isEnabled(); } diff --git a/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java b/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java index 5204bd8c040..375aaa707e6 100644 --- a/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java +++ b/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java @@ -17,14 +17,20 @@ package org.apache.cloudstack.acl; import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; +import java.util.List; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; -import org.mockito.runners.MockitoJUnitRunner; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; import com.cloud.exception.PermissionDeniedException; import com.cloud.user.Account; @@ -43,9 +49,12 @@ public class DynamicRoleBasedAPIAccessCheckerTest extends TestCase { @Mock private AccountService accountService; @Mock - private RoleService roleService; + private RoleService roleServiceMock; + @Spy + @InjectMocks + private DynamicRoleBasedAPIAccessChecker apiAccessCheckerSpy; - private DynamicRoleBasedAPIAccessChecker apiAccessChecker; + List apiNames = new ArrayList<>(Arrays.asList("apiName")); private User getTestUser() { return new UserVO(12L, "some user", "password", "firstName", "lastName", @@ -69,23 +78,19 @@ public class DynamicRoleBasedAPIAccessCheckerTest extends TestCase { @Override @Before public void setUp() throws NoSuchFieldException, IllegalAccessException { - apiAccessChecker = Mockito.spy(new DynamicRoleBasedAPIAccessChecker()); - setupMockField(apiAccessChecker, "accountService", accountService); - setupMockField(apiAccessChecker, "roleService", roleService); - Mockito.when(accountService.getAccount(Mockito.anyLong())).thenReturn(getTestAccount()); - Mockito.when(roleService.findRole(Mockito.anyLong())).thenReturn((RoleVO) getTestRole()); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn((RoleVO) getTestRole()); // Enabled plugin - Mockito.doReturn(false).when(apiAccessChecker).isDisabled(); - Mockito.doCallRealMethod().when(apiAccessChecker).checkAccess(Mockito.any(User.class), Mockito.anyString()); + Mockito.doReturn(true).when(apiAccessCheckerSpy).isEnabled(); + Mockito.doCallRealMethod().when(apiAccessCheckerSpy).checkAccess(Mockito.any(User.class), Mockito.anyString()); } @Test public void testInvalidAccountCheckAccess() { Mockito.when(accountService.getAccount(Mockito.anyLong())).thenReturn(null); try { - apiAccessChecker.checkAccess(getTestUser(), "someApi"); + apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi"); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -93,9 +98,9 @@ public class DynamicRoleBasedAPIAccessCheckerTest extends TestCase { @Test public void testInvalidAccountRoleCheckAccess() { - Mockito.when(roleService.findRole(Mockito.anyLong())).thenReturn(null); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(null); try { - apiAccessChecker.checkAccess(getTestUser(), "someApi"); + apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi"); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -104,15 +109,15 @@ public class DynamicRoleBasedAPIAccessCheckerTest extends TestCase { @Test public void testDefaultRootAdminAccess() { Mockito.when(accountService.getAccount(Mockito.anyLong())).thenReturn(new AccountVO("root admin", 1L, null, Account.Type.ADMIN, "some-uuid")); - Mockito.when(roleService.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "SomeRole", RoleType.Admin, "default root admin role")); - assertTrue(apiAccessChecker.checkAccess(getTestUser(), "anyApi")); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "SomeRole", RoleType.Admin, "default root admin role")); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), "anyApi")); } @Test public void testInvalidRolePermissionsCheckAccess() { - Mockito.when(roleService.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.emptyList()); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.emptyList()); try { - apiAccessChecker.checkAccess(getTestUser(), "someApi"); + apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi"); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -122,25 +127,25 @@ public class DynamicRoleBasedAPIAccessCheckerTest extends TestCase { public void testValidAllowRolePermissionApiCheckAccess() { final String allowedApiName = "someAllowedApi"; final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null); - Mockito.when(roleService.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); - assertTrue(apiAccessChecker.checkAccess(getTestUser(), allowedApiName)); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName)); } @Test public void testValidAllowRolePermissionWildcardCheckAccess() { final String allowedApiName = "someAllowedApi"; final RolePermission permission = new RolePermissionVO(1L, "some*", Permission.ALLOW, null); - Mockito.when(roleService.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); - assertTrue(apiAccessChecker.checkAccess(getTestUser(), allowedApiName)); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName)); } @Test public void testValidDenyRolePermissionApiCheckAccess() { final String denyApiName = "someDeniedApi"; final RolePermission permission = new RolePermissionVO(1L, denyApiName, Permission.DENY, null); - Mockito.when(roleService.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); try { - apiAccessChecker.checkAccess(getTestUser(), denyApiName); + apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -150,9 +155,9 @@ public class DynamicRoleBasedAPIAccessCheckerTest extends TestCase { public void testValidDenyRolePermissionWildcardCheckAccess() { final String denyApiName = "someDenyApi"; final RolePermission permission = new RolePermissionVO(1L, "*Deny*", Permission.DENY, null); - Mockito.when(roleService.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); try { - apiAccessChecker.checkAccess(getTestUser(), denyApiName); + apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -161,8 +166,33 @@ public class DynamicRoleBasedAPIAccessCheckerTest extends TestCase { @Test public void testAnnotationFallbackCheckAccess() { final String allowedApiName = "someApiWithAnnotations"; - apiAccessChecker.addApiToRoleBasedAnnotationsMap(getTestRole().getRoleType(), allowedApiName); - assertTrue(apiAccessChecker.checkAccess(getTestUser(), allowedApiName)); + apiAccessCheckerSpy.addApiToRoleBasedAnnotationsMap(getTestRole().getRoleType(), allowedApiName); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName)); } + @Test + public void getApisAllowedToUserTestRoleServiceIsDisabledShouldReturnUnchangedList() { + Mockito.doReturn(false).when(apiAccessCheckerSpy).isEnabled(); + + List apisReceived = apiAccessCheckerSpy.getApisAllowedToUser(null, getTestUser(), apiNames); + Assert.assertEquals(1, apisReceived.size()); + } + + @Test + public void getApisAllowedToUserTestPermissionAllowForGivenApiShouldReturnUnchangedList() { + final RolePermission permission = new RolePermissionVO(1L, "apiName", Permission.ALLOW, null); + Mockito.doReturn(Collections.singletonList(permission)).when(roleServiceMock).findAllPermissionsBy(Mockito.anyLong()); + + List apisReceived = apiAccessCheckerSpy.getApisAllowedToUser(getTestRole(), getTestUser(), apiNames); + Assert.assertEquals(1, apisReceived.size()); + } + + @Test + public void getApisAllowedToUserTestPermissionDenyForGivenApiShouldReturnEmptyList() { + final RolePermission permission = new RolePermissionVO(1L, "apiName", Permission.DENY, null); + Mockito.doReturn(Collections.singletonList(permission)).when(roleServiceMock).findAllPermissionsBy(Mockito.anyLong()); + + List apisReceived = apiAccessCheckerSpy.getApisAllowedToUser(getTestRole(), getTestUser(), apiNames); + Assert.assertEquals(0, apisReceived.size()); + } } \ No newline at end of file diff --git a/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java b/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java index 4d33b26c428..9363ebd2379 100644 --- a/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java +++ b/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java @@ -60,51 +60,91 @@ public class ProjectRoleBasedApiAccessChecker extends AdapterBase implements AP @Override public boolean isEnabled() { + if (!roleService.isEnabled()) { + LOGGER.trace("RoleService is disabled. We will not use ProjectRoleBasedApiAccessChecker."); + } return roleService.isEnabled(); } - public boolean isDisabled() { - return !isEnabled(); + @Override + public List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException { + if (!isEnabled()) { + return apiNames; + } + + Project project = CallContext.current().getProject(); + if (project == null) { + LOGGER.warn(String.format("Project is null, ProjectRoleBasedApiAccessChecker only applies to projects, returning APIs [%s] for user [%s] as allowed.", apiNames, user)); + return apiNames; + } + + long accountID = user.getAccountId(); + ProjectAccount projectUser = projectAccountDao.findByProjectIdUserId(project.getId(), accountID, user.getId()); + if (projectUser != null) { + if (projectUser.getAccountRole() != ProjectAccount.Role.Admin) { + apiNames.removeIf(apiName -> !isPermitted(project, projectUser, apiName)); + } + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(String.format("Returning APIs [%s] as allowed for user [%s].", apiNames, user)); + } + return apiNames; + } + + ProjectAccount projectAccount = projectAccountDao.findByProjectIdAccountId(project.getId(), accountID); + if (projectAccount == null) { + throw new PermissionDeniedException(String.format("The user [%s] does not belong to the project [%s].", user, project)); + } + + if (projectAccount.getAccountRole() != ProjectAccount.Role.Admin) { + apiNames.removeIf(apiName -> !isPermitted(project, projectAccount, apiName)); + } + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(String.format("Returning APIs [%s] as allowed for user [%s].", apiNames, user)); + } + return apiNames; } @Override public boolean checkAccess(User user, String apiCommandName) throws PermissionDeniedException { - if (isDisabled()) { + if (!isEnabled()) { + return true; + } + + Project project = CallContext.current().getProject(); + if (project == null) { + LOGGER.warn(String.format("Project is null, ProjectRoleBasedApiAccessChecker only applies to projects, returning API [%s] for user [%s] as allowed.", apiCommandName, + user)); return true; } Account userAccount = accountService.getAccount(user.getAccountId()); - Project project = CallContext.current().getProject(); - if (project == null) { - return true; - } - if (accountService.isRootAdmin(userAccount.getId()) || accountService.isDomainAdmin(userAccount.getAccountId())) { + LOGGER.info(String.format("Account [%s] is Root Admin or Domain Admin, all APIs are allowed.", userAccount.getAccountName())); return true; } ProjectAccount projectUser = projectAccountDao.findByProjectIdUserId(project.getId(), userAccount.getAccountId(), user.getId()); if (projectUser != null) { - if (projectUser.getAccountRole() == ProjectAccount.Role.Admin) { + if (projectUser.getAccountRole() == ProjectAccount.Role.Admin || isPermitted(project, projectUser, apiCommandName)) { return true; - } else { - return isPermitted(project, projectUser, apiCommandName); } + denyApiAccess(apiCommandName); } ProjectAccount projectAccount = projectAccountDao.findByProjectIdAccountId(project.getId(), userAccount.getAccountId()); if (projectAccount != null) { - if (projectAccount.getAccountRole() == ProjectAccount.Role.Admin) { + if (projectAccount.getAccountRole() == ProjectAccount.Role.Admin || isPermitted(project, projectAccount, apiCommandName)) { return true; - } else { - return isPermitted(project, projectAccount, apiCommandName); } + denyApiAccess(apiCommandName); } + // Default deny all if ("updateProjectInvitation".equals(apiCommandName)) { return true; } - throw new UnavailableCommandException("The API " + apiCommandName + " does not exist or is not available for this account/user in project "+project.getUuid()); + + throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for this account/user in project [%s].", apiCommandName, project.getUuid())); } @Override @@ -112,7 +152,7 @@ public class ProjectRoleBasedApiAccessChecker extends AdapterBase implements AP return true; } - private boolean isPermitted(Project project, ProjectAccount projectUser, String apiCommandName) { + public boolean isPermitted(Project project, ProjectAccount projectUser, String ... apiCommandNames) { ProjectRole projectRole = null; if(projectUser.getProjectRoleId() != null) { projectRole = projectRoleService.findProjectRole(projectUser.getProjectRoleId(), project.getId()); @@ -122,12 +162,11 @@ public class ProjectRoleBasedApiAccessChecker extends AdapterBase implements AP return true; } - for (ProjectRolePermission permission : projectRoleService.findAllProjectRolePermissions(project.getId(), projectRole.getId())) { - if (permission.getRule().matches(apiCommandName)) { - if (Permission.ALLOW.equals(permission.getPermission())) { - return true; - } else { - denyApiAccess(apiCommandName); + List allProjectRolePermissions = projectRoleService.findAllProjectRolePermissions(project.getId(), projectRole.getId()); + for (String api : apiCommandNames) { + for (ProjectRolePermission permission : allProjectRolePermissions) { + if (permission.getRule().matches(api)) { + return Permission.ALLOW.equals(permission.getPermission()); } } } diff --git a/plugins/acl/project-role-based/src/main/test/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessCheckerTest.java b/plugins/acl/project-role-based/src/main/test/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessCheckerTest.java new file mode 100644 index 00000000000..3d053e5b285 --- /dev/null +++ b/plugins/acl/project-role-based/src/main/test/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessCheckerTest.java @@ -0,0 +1,154 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.acl; + +import com.cloud.exception.PermissionDeniedException; +import com.cloud.projects.Project; +import com.cloud.projects.ProjectAccount; +import com.cloud.projects.ProjectAccountVO; +import com.cloud.projects.ProjectVO; +import com.cloud.projects.dao.ProjectAccountDao; +import com.cloud.user.User; +import com.cloud.user.UserVO; + +import org.apache.cloudstack.context.CallContext; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import junit.framework.TestCase; + +@RunWith(PowerMockRunner.class) +@PrepareForTest(CallContext.class) +public class ProjectRoleBasedApiAccessCheckerTest extends TestCase { + @Mock + ProjectAccountDao projectAccountDaoMock; + + @Mock + RoleService roleServiceMock; + + @Mock + ProjectAccountVO projectAccountVOMock; + + @Mock + CallContext callContextMock; + + @InjectMocks + ProjectRoleBasedApiAccessChecker projectRoleBasedApiAccessCheckerSpy = Mockito.spy(ProjectRoleBasedApiAccessChecker.class); + + List apiNames = new ArrayList<>(Arrays.asList("apiName")); + + @Before + public void setup() { + + Mockito.doReturn(true).when(roleServiceMock).isEnabled(); + } + + public Project getTestProject() { + return new ProjectVO("Teste", "Teste", 1L, 1L); + } + + private User getTestUser() { + return new UserVO(12L, "some user", "password", "firstName", "lastName", + "email@gmail.com", "GMT", "uuid", User.Source.UNKNOWN); + } + + @Test + public void getApisAllowedToUserTestRoleServiceIsDisabledShouldReturnUnchangedApiList() { + Mockito.doReturn(false).when(roleServiceMock).isEnabled(); + + List apisReceived = projectRoleBasedApiAccessCheckerSpy.getApisAllowedToUser(null, getTestUser(), apiNames); + Assert.assertEquals(1, apisReceived.size()); + } + + @Test + public void getApisAllowedToUserTestProjectIsNullShouldReturnUnchangedApiList() { + PowerMockito.mockStatic(CallContext.class); + PowerMockito.when(CallContext.current()).thenReturn(callContextMock); + + Mockito.doReturn(null).when(callContextMock).getProject(); + + List apisReceived = projectRoleBasedApiAccessCheckerSpy.getApisAllowedToUser(null, getTestUser(), apiNames); + Assert.assertEquals(1, apisReceived.size()); + } + + @Test (expected = PermissionDeniedException.class) + public void getApisAllowedToUserTestProjectAccountIsNullThrowPermissionDeniedException() { + PowerMockito.mockStatic(CallContext.class); + PowerMockito.when(CallContext.current()).thenReturn(callContextMock); + + Mockito.when(callContextMock.getProject()).thenReturn(getTestProject()); + Mockito.when(projectAccountDaoMock.findByProjectIdAccountId(Mockito.anyLong(), Mockito.anyLong())).thenReturn(null); + Mockito.when(projectAccountDaoMock.findByProjectIdUserId(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong())).thenReturn(null); + + projectRoleBasedApiAccessCheckerSpy.getApisAllowedToUser(null, getTestUser(), apiNames); + } + + @Test + public void getApisAllowedToUserTestProjectAccountHasAdminRoleReturnsUnchangedApiList() { + PowerMockito.mockStatic(CallContext.class); + PowerMockito.when(CallContext.current()).thenReturn(callContextMock); + + Mockito.doReturn(getTestProject()).when(callContextMock).getProject(); + Mockito.doReturn(projectAccountVOMock).when(projectAccountDaoMock).findByProjectIdUserId(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong()); + Mockito.doReturn(ProjectAccount.Role.Admin).when(projectAccountVOMock).getAccountRole(); + + List apisReceived = projectRoleBasedApiAccessCheckerSpy.getApisAllowedToUser(null, getTestUser(), apiNames); + Assert.assertEquals(1, apisReceived.size()); + } + + @Test + public void getApisAllowedToUserTestProjectAccountNotPermittedForTheApiListShouldReturnEmptyList() { + PowerMockito.mockStatic(CallContext.class); + PowerMockito.when(CallContext.current()).thenReturn(callContextMock); + + Mockito.doReturn(getTestProject()).when(callContextMock).getProject(); + Mockito.doReturn(projectAccountVOMock).when(projectAccountDaoMock).findByProjectIdUserId(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong()); + Mockito.doReturn(ProjectAccount.Role.Regular).when(projectAccountVOMock).getAccountRole(); + Mockito.doReturn(false).when(projectRoleBasedApiAccessCheckerSpy).isPermitted(Mockito.any(Project.class), Mockito.any(ProjectAccount.class), Mockito.anyString()); + + + List apisReceived = projectRoleBasedApiAccessCheckerSpy.getApisAllowedToUser(null, getTestUser(), apiNames); + Assert.assertTrue(apisReceived.isEmpty()); + } + + @Test + public void getApisAllowedToUserTestProjectAccountPermittedForTheApiListShouldReturnTheSameList() { + PowerMockito.mockStatic(CallContext.class); + PowerMockito.when(CallContext.current()).thenReturn(callContextMock); + + Mockito.doReturn(getTestProject()).when(callContextMock).getProject(); + Mockito.doReturn(projectAccountVOMock).when(projectAccountDaoMock).findByProjectIdUserId(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong()); + Mockito.doReturn(ProjectAccount.Role.Regular).when(projectAccountVOMock).getAccountRole(); + Mockito.doReturn(true).when(projectRoleBasedApiAccessCheckerSpy).isPermitted(Mockito.any(Project.class), Mockito.any(ProjectAccount.class), Mockito.anyString()); + + + List apisReceived = projectRoleBasedApiAccessCheckerSpy.getApisAllowedToUser(null, getTestUser(), apiNames); + Assert.assertEquals(1, apisReceived.size()); + } +} \ No newline at end of file diff --git a/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java b/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java index 2b2464f720c..27f8305f579 100644 --- a/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java +++ b/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java @@ -65,45 +65,74 @@ public class StaticRoleBasedAPIAccessChecker extends AdapterBase implements APIA } } + /** + * Only one strategy should be used between StaticRoleBasedAPIAccessChecker and DynamicRoleBasedAPIAccessChecker + * Default behavior is to use the Dynamic version. The StaticRoleBasedAPIAccessChecker is the legacy version. + * If roleService is enabled, then it uses the DynamicRoleBasedAPIAccessChecker, otherwise, it will use the + * StaticRoleBasedAPIAccessChecker. + */ @Override public boolean isEnabled() { - return !isDisabled(); - } - public boolean isDisabled() { + if (roleService.isEnabled()) { + LOGGER.debug("RoleService is enabled. We will use it instead of StaticRoleBasedAPIAccessChecker."); + } return roleService.isEnabled(); } + @Override + public List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException { + if (isEnabled()) { + return apiNames; + } + + RoleType roleType = role.getRoleType(); + apiNames.removeIf(apiName -> !isApiAllowed(apiName, roleType)); + + return apiNames; + } + @Override public boolean checkAccess(User user, String commandName) throws PermissionDeniedException { - if (isDisabled()) { + if (isEnabled()) { return true; } Account account = accountService.getAccount(user.getAccountId()); if (account == null) { - throw new PermissionDeniedException("The account id=" + user.getAccountId() + "for user id=" + user.getId() + "is null"); + throw new PermissionDeniedException(String.format("The account with id [%s] for user with uuid [%s] is null.", user.getAccountId(), user.getUuid())); } return checkAccess(account, commandName); } + @Override public boolean checkAccess(Account account, String commandName) { RoleType roleType = accountService.getRoleType(account); - boolean isAllowed = - commandsPropertiesOverrides.contains(commandName) ? commandsPropertiesRoleBasedApisMap.get(roleType).contains(commandName) : annotationRoleBasedApisMap.get( - roleType).contains(commandName); - - if (isAllowed) { + if (isApiAllowed(commandName, roleType)) { return true; } if (commandNames.contains(commandName)) { - throw new PermissionDeniedException("The API is denied. Role type=" + roleType.toString() + " is not allowed to request the api: " + commandName); + throw new PermissionDeniedException(String.format("Request to API [%s] was denied. The role type [%s] is not allowed to request it.", commandName, roleType.toString())); } else { - throw new UnavailableCommandException("The API " + commandName + " does not exist or is not available for this account."); + throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for this account.", commandName)); } } + /** + * Verifies if the API is allowed for the given RoleType. + * + * @param apiName API command to be verified + * @param roleType to be verified + * @return 'true' if the API is allowed for the given RoleType, otherwise, 'false'. + */ + public boolean isApiAllowed(String apiName, RoleType roleType) { + if (commandsPropertiesOverrides.contains(apiName)) { + return commandsPropertiesRoleBasedApisMap.get(roleType).contains(apiName); + } + return annotationRoleBasedApisMap.get(roleType).contains(apiName); + } + @Override public boolean configure(String name, Map params) throws ConfigurationException { super.configure(name, params); @@ -147,7 +176,7 @@ public class StaticRoleBasedAPIAccessChecker extends AdapterBase implements APIA commandsPropertiesRoleBasedApisMap.get(roleType).add(apiName); } } catch (NumberFormatException nfe) { - LOGGER.info("Malformed key=value pair for entry: " + entry.toString()); + LOGGER.error(String.format("Malformed key=value pair for entry: [%s].", entry)); } } } diff --git a/plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java b/plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java index 378f3540f94..3bdf2e9ce92 100644 --- a/plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java +++ b/plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java @@ -27,7 +27,6 @@ import java.util.Set; import javax.inject.Inject; -import com.cloud.user.Account; import org.apache.cloudstack.acl.APIChecker; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.BaseAsyncCmd; @@ -35,17 +34,24 @@ import org.apache.cloudstack.api.BaseAsyncCreateCmd; import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.BaseResponse; import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RoleService; +import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.command.user.discovery.ListApisCmd; import org.apache.cloudstack.api.response.ApiDiscoveryResponse; import org.apache.cloudstack.api.response.ApiParameterResponse; import org.apache.cloudstack.api.response.ApiResponseResponse; import org.apache.cloudstack.api.response.ListResponse; +import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.reflections.ReflectionUtils; import org.springframework.stereotype.Component; +import com.cloud.exception.PermissionDeniedException; import com.cloud.serializer.Param; +import com.cloud.user.Account; +import com.cloud.user.AccountService; import com.cloud.user.User; import com.cloud.utils.ReflectUtil; import com.cloud.utils.component.ComponentLifecycleBase; @@ -58,7 +64,13 @@ public class ApiDiscoveryServiceImpl extends ComponentLifecycleBase implements A List _apiAccessCheckers = null; List _services = null; - private static Map s_apiNameDiscoveryResponseMap = null; + protected static Map s_apiNameDiscoveryResponseMap = null; + + @Inject + AccountService accountService; + + @Inject + RoleService roleService; protected ApiDiscoveryServiceImpl() { super(); @@ -231,8 +243,9 @@ public class ApiDiscoveryServiceImpl extends ComponentLifecycleBase implements A @Override public ListResponse listApis(User user, String name) { - ListResponse response = new ListResponse(); - List responseList = new ArrayList(); + ListResponse response = new ListResponse<>(); + List responseList = new ArrayList<>(); + List apisAllowed = new ArrayList<>(s_apiNameDiscoveryResponseMap.keySet()); if (user == null) return null; @@ -245,24 +258,35 @@ public class ApiDiscoveryServiceImpl extends ComponentLifecycleBase implements A try { apiChecker.checkAccess(user, name); } catch (Exception ex) { - s_logger.debug("API discovery access check failed for " + name + " with " + ex.getMessage()); + s_logger.error(String.format("API discovery access check failed for [%s] with error [%s].", name, ex.getMessage()), ex); return null; } } responseList.add(s_apiNameDiscoveryResponseMap.get(name)); } else { - for (String apiName : s_apiNameDiscoveryResponseMap.keySet()) { - boolean isAllowed = true; + Account account = accountService.getAccount(user.getAccountId()); + if (account == null) { + throw new PermissionDeniedException(String.format("The account with id [%s] for user [%s] is null.", user.getAccountId(), user)); + } + + final Role role = roleService.findRole(account.getRoleId()); + if (role == null || role.getId() < 1L) { + throw new PermissionDeniedException(String.format("The account [%s] has role null or unknown.", + ReflectionToStringBuilderUtils.reflectOnlySelectedFields(account, "accountName", "uuid"))); + } + + if (role.getRoleType() == RoleType.Admin && role.getId() == RoleType.Admin.getId()) { + s_logger.info(String.format("Account [%s] is Root Admin, all APIs are allowed.", + ReflectionToStringBuilderUtils.reflectOnlySelectedFields(account, "accountName", "uuid"))); + } else { for (APIChecker apiChecker : _apiAccessCheckers) { - try { - apiChecker.checkAccess(user, apiName); - } catch (Exception ex) { - isAllowed = false; - } + apisAllowed = apiChecker.getApisAllowedToUser(role, user, apisAllowed); } - if (isAllowed) - responseList.add(s_apiNameDiscoveryResponseMap.get(apiName)); + } + + for (String apiName: apisAllowed) { + responseList.add(s_apiNameDiscoveryResponseMap.get(apiName)); } } response.setResponses(responseList); diff --git a/plugins/api/discovery/src/test/java/org/apache/cloudstack/discovery/ApiDiscoveryTest.java b/plugins/api/discovery/src/test/java/org/apache/cloudstack/discovery/ApiDiscoveryTest.java index b0ca23245de..752eb1ac2ff 100644 --- a/plugins/api/discovery/src/test/java/org/apache/cloudstack/discovery/ApiDiscoveryTest.java +++ b/plugins/api/discovery/src/test/java/org/apache/cloudstack/discovery/ApiDiscoveryTest.java @@ -16,113 +16,119 @@ // under the License. package org.apache.cloudstack.discovery; +import com.cloud.exception.PermissionDeniedException; +import com.cloud.user.Account; +import com.cloud.user.AccountService; +import com.cloud.user.AccountVO; import com.cloud.user.User; import com.cloud.user.UserVO; -import com.cloud.utils.component.PluggableService; + import org.apache.cloudstack.acl.APIChecker; -import org.apache.cloudstack.api.APICommand; -import org.apache.cloudstack.api.command.user.discovery.ListApisCmd; -import org.apache.cloudstack.api.command.user.vm.ListVMsCmd; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RoleService; +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.acl.RoleVO; import org.apache.cloudstack.api.response.ApiDiscoveryResponse; -import org.apache.cloudstack.api.response.ApiResponseResponse; -import org.apache.cloudstack.api.response.ListResponse; -import org.apache.commons.lang3.StringUtils; -import org.junit.BeforeClass; +import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; -import javax.naming.ConfigurationException; -import java.util.ArrayList; import java.util.Arrays; -import java.util.HashSet; import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; +import java.util.Map; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyString; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyList; +@RunWith(MockitoJUnitRunner.class) public class ApiDiscoveryTest { - private static APIChecker s_apiChecker = mock(APIChecker.class); - private static PluggableService s_pluggableService = mock(PluggableService.class); - private static ApiDiscoveryServiceImpl s_discoveryService = new ApiDiscoveryServiceImpl(); + @Mock + AccountService accountServiceMock; - private static Class testCmdClass = ListApisCmd.class; - private static Class listVMsCmdClass = ListVMsCmd.class; - private static User testUser; - private static String testApiName; - private static String listVmsCmdName; - private static String testApiDescription; - private static String testApiSince; - private static boolean testApiAsync; + @Mock + RoleService roleServiceMock; - @BeforeClass - public static void setUp() throws ConfigurationException { + @Mock + APIChecker apiCheckerMock; - listVmsCmdName = listVMsCmdClass.getAnnotation(APICommand.class).name(); + @Mock + Map apiNameDiscoveryResponseMapMock; - testApiName = testCmdClass.getAnnotation(APICommand.class).name(); - testApiDescription = testCmdClass.getAnnotation(APICommand.class).description(); - testApiSince = testCmdClass.getAnnotation(APICommand.class).since(); - testApiAsync = false; - testUser = new UserVO(); + @Mock + List apiAccessCheckersMock; - s_discoveryService._apiAccessCheckers = mock(List.class); - s_discoveryService._services = mock(List.class); + @Spy + @InjectMocks + ApiDiscoveryServiceImpl discoveryServiceSpy; - when(s_apiChecker.checkAccess(any(User.class), anyString())).thenReturn(true); - when(s_pluggableService.getCommands()).thenReturn(new ArrayList>()); - when(s_discoveryService._apiAccessCheckers.iterator()).thenReturn(Arrays.asList(s_apiChecker).iterator()); - when(s_discoveryService._services.iterator()).thenReturn(Arrays.asList(s_pluggableService).iterator()); + @Before + public void setup() { + discoveryServiceSpy.s_apiNameDiscoveryResponseMap = apiNameDiscoveryResponseMapMock; + discoveryServiceSpy._apiAccessCheckers = apiAccessCheckersMock; - Set> cmdClasses = new HashSet>(); - cmdClasses.add(ListApisCmd.class); - cmdClasses.add(ListVMsCmd.class); - s_discoveryService.start(); - s_discoveryService.cacheResponseMap(cmdClasses); + Mockito.when(discoveryServiceSpy._apiAccessCheckers.iterator()).thenReturn(Arrays.asList(apiCheckerMock).iterator()); + } + + private User getTestUser() { + return new UserVO(12L, "some user", "password", "firstName", "lastName", + "email@gmail.com", "GMT", "uuid", User.Source.UNKNOWN); + } + + private Account getNormalAccount() { + return new AccountVO("some name", 1L, "network-domain", Account.Type.NORMAL, "some-uuid"); + } + + @Test (expected = PermissionDeniedException.class) + public void listApisTestThrowPermissionDeniedExceptionOnAccountNull() throws PermissionDeniedException { + Mockito.when(accountServiceMock.getAccount(Mockito.anyLong())).thenReturn(null); + discoveryServiceSpy.listApis(getTestUser(), null); + } + + @Test (expected = PermissionDeniedException.class) + public void listApisTestThrowPermissionDeniedExceptionOnRoleNull() throws PermissionDeniedException { + Mockito.when(accountServiceMock.getAccount(Mockito.anyLong())).thenReturn(getNormalAccount()); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(null); + + discoveryServiceSpy.listApis(getTestUser(), null); + } + + @Test (expected = PermissionDeniedException.class) + public void listApisTestThrowPermissionDeniedExceptionOnRoleUnknown() throws PermissionDeniedException { + RoleVO unknownRoleVO = new RoleVO(-1L,"name", RoleType.Unknown, "description"); + + Mockito.when(accountServiceMock.getAccount(Mockito.anyLong())).thenReturn(getNormalAccount()); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(unknownRoleVO); + + discoveryServiceSpy.listApis(getTestUser(), null); } @Test - public void verifyListSingleApi() throws Exception { - ListResponse responses = (ListResponse)s_discoveryService.listApis(testUser, testApiName); - assertNotNull("Responses should not be null", responses); - if (responses != null) { - ApiDiscoveryResponse response = responses.getResponses().get(0); - assertTrue("No. of response items should be one", responses.getCount() == 1); - assertEquals("Error in api name", testApiName, response.getName()); - assertEquals("Error in api description", testApiDescription, response.getDescription()); - assertEquals("Error in api since", testApiSince, response.getSince()); - assertEquals("Error in api isAsync", testApiAsync, response.getAsync()); - } + public void listApisTestDoesNotGetApisAllowedToUserOnAdminRole() throws PermissionDeniedException { + AccountVO adminAccountVO = new AccountVO("some name", 1L, "network-domain", Account.Type.ADMIN, "some-uuid"); + RoleVO adminRoleVO = new RoleVO(1L,"name", RoleType.Admin, "description"); + + Mockito.when(accountServiceMock.getAccount(Mockito.anyLong())).thenReturn(adminAccountVO); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(adminRoleVO); + + discoveryServiceSpy.listApis(getTestUser(), null); + + Mockito.verify(apiCheckerMock, Mockito.times(0)).getApisAllowedToUser(any(Role.class), any(User.class), anyList()); } @Test - public void verifyListApis() throws Exception { - ListResponse responses = (ListResponse)s_discoveryService.listApis(testUser, null); - assertNotNull("Responses should not be null", responses); - if (responses != null) { - assertTrue("No. of response items > 2", responses.getCount().intValue() == 2); - for (ApiDiscoveryResponse response : responses.getResponses()) { - assertFalse("API name is empty", response.getName().isEmpty()); - assertFalse("API description is empty", response.getDescription().isEmpty()); - } - } - } + public void listApisTestGetsApisAllowedToUserOnUserRole() throws PermissionDeniedException { + RoleVO userRoleVO = new RoleVO(4L, "name", RoleType.User, "description"); - @Test - public void verifyListVirtualMachinesTagsField() throws Exception { - ListResponse responses = (ListResponse)s_discoveryService.listApis(testUser, listVmsCmdName); - assertNotNull("Response should not be null", responses); - if (responses != null) { - assertEquals("No. of response items should be one", 1, (int) responses.getCount()); - ApiDiscoveryResponse response = responses.getResponses().get(0); - List tagsResponse = response.getApiResponse().stream().filter(resp -> StringUtils.equals(resp.getName(), "tags")).collect(Collectors.toList()); - assertEquals("Tags field should be present in listVirtualMachines response fields", tagsResponse.size(), 1); - } + Mockito.when(accountServiceMock.getAccount(Mockito.anyLong())).thenReturn(getNormalAccount()); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(userRoleVO); + + discoveryServiceSpy.listApis(getTestUser(), null); + + Mockito.verify(apiCheckerMock, Mockito.times(1)).getApisAllowedToUser(any(Role.class), any(User.class), anyList()); } -} +} \ No newline at end of file diff --git a/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java b/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java index 3ed3551414a..9fe3cb23e98 100644 --- a/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java +++ b/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java @@ -26,6 +26,8 @@ import javax.naming.ConfigurationException; import net.sf.ehcache.Cache; import net.sf.ehcache.CacheManager; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -139,47 +141,79 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APIChecker, return true; } + @Override + public List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException { + if (!isEnabled()) { + return apiNames; + } + + for (int i = 0; i < apiNames.size(); i++) { + if (hasApiRateLimitBeenExceeded(user.getAccountId())) { + throwExceptionDueToApiRateLimitReached(user.getAccountId()); + } + } + return apiNames; + } + + public void throwExceptionDueToApiRateLimitReached(Long accountId) throws RequestLimitException { + long expireAfter = _store.get(accountId).getExpireDuration(); + String msg = String.format("The given user has reached his/her account api limit, please retry after [%s] ms.", expireAfter); + s_logger.warn(msg); + throw new RequestLimitException(msg); + } + @Override public boolean checkAccess(User user, String apiCommandName) throws PermissionDeniedException { - // check if api rate limiting is enabled or not - if (!enabled) { + if (!isEnabled()) { return true; } - Long accountId = user.getAccountId(); - Account account = _accountService.getAccount(accountId); + + Account account = _accountService.getAccount(user.getAccountId()); return checkAccess(account, apiCommandName); } + @Override public boolean checkAccess(Account account, String commandName) { - if (_accountService.isRootAdmin(account.getId())) { - // no API throttling on root admin + Long accountId = account.getAccountId(); + if (_accountService.isRootAdmin(accountId)) { + s_logger.info(String.format("Account [%s] is Root Admin, in this case, API limit does not apply.", + ReflectionToStringBuilderUtils.reflectOnlySelectedFields(account, "accountName", "uuid"))); return true; } - StoreEntry entry = _store.get(account.getId()); + if (hasApiRateLimitBeenExceeded(accountId)) { + throwExceptionDueToApiRateLimitReached(accountId); + } + return true; + } + + /** + * Verifies if the API limit was exceeded by the account. + * + * @param accountId the id of the account to be verified + * @return if the API limit was exceeded by the account + */ + public boolean hasApiRateLimitBeenExceeded(Long accountId) { + Account account = _accountService.getAccount(accountId); + StoreEntry entry = _store.get(accountId); if (entry == null) { - - /* Populate the entry, thus unlocking any underlying mutex */ entry = _store.create(account.getId(), timeToLive); } - /* Increment the client count and see whether we have hit the maximum allowed clients yet. */ int current = entry.incrementAndGet(); if (current <= maxAllowed) { - s_logger.trace("account (" + account.getAccountId() + "," + account.getAccountName() + ") has current count = " + current); - return true; - } else { - long expireAfter = entry.getExpireDuration(); - // for this exception, we can just show the same message to user and admin users. - String msg = "The given user has reached his/her account api limit, please retry after " + expireAfter + " ms."; - s_logger.warn(msg); - throw new RequestLimitException(msg); + s_logger.trace(String.format("Account %s has current count [%s].", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(account, "uuid", "accountName"), current)); + return false; } + return true; } @Override public boolean isEnabled() { + if (!enabled) { + s_logger.debug("API rate limiting is disabled. We will not use ApiRateLimitService."); + } return enabled; } @@ -207,5 +241,4 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APIChecker, this.enabled = enabled; } - }