Improve login time (#6412)

* Improve slow login

* Address review

* Address Daan's review

* Address Daniel reviews

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<String> getApisAllowedToUser(Role role, User user, List<String> apiNames) throws PermissionDeniedException;
     boolean isEnabled();
 }

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[");
+        return String.format("Project %s.", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "name", "uuid", "domainId"));
-        buf.append(id).append("|name=").append(name).append("|domainid=").append(domainId).append("]");
-        return buf.toString();
     }
 
     @Override

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

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

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");
+    }
 }

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<PluggableService> services;
     private Map<RoleType, Set<String>> annotationRoleBasedApisMap = new HashMap<RoleType, Set<String>>();
 
-    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 {
+    @Override
-        throw new PermissionDeniedException("The API " + commandName + " is denied for the account's role.");
+    public List<String> getApisAllowedToUser(Role role, User user, List<String> apiNames) throws PermissionDeniedException {
+        if (!isEnabled()) {
+            return apiNames;
         }
 
-    public boolean isDisabled() {
+        List<RolePermission> allPermissions = roleService.findAllPermissionsBy(role.getId());
-        return !roleService.isEnabled();
+        List<String> allowedApis = new ArrayList<>();
+        for (String api : apiNames) {
+            if (checkApiPermissionByRole(role, api, allPermissions)) {
+                allowedApis.add(api);
+            }
+        }
+        return allowedApis;
+    }
+
+    /**
+     * 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<RolePermission> 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
+        List<RolePermission> allPermissions = roleService.findAllPermissionsBy(accountRole.getId());
-        for (final RolePermission permission : roleService.findAllPermissionsBy(accountRole.getId())) {
+        if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) {
-            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)) {
             return true;
         }
-
+        throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account));
-        // Default deny all
-        throw new UnavailableCommandException("The API " + commandName + " does not exist or is not available for this 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();
     }
 

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<String> 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.doReturn(true).when(apiAccessCheckerSpy).isEnabled();
-        Mockito.doCallRealMethod().when(apiAccessChecker).checkAccess(Mockito.any(User.class), Mockito.anyString());
+        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"));
+        Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "SomeRole", RoleType.Admin, "default root admin role"));
-        assertTrue(apiAccessChecker.checkAccess(getTestUser(), "anyApi"));
+        assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), "anyApi"));
     }
 
     @Test
     public void testInvalidRolePermissionsCheckAccess() {
-        Mockito.when(roleService.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.<RolePermission>emptyList());
+        Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.<RolePermission>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));
+        Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission));
-        assertTrue(apiAccessChecker.checkAccess(getTestUser(), allowedApiName));
+        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));
+        Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission));
-        assertTrue(apiAccessChecker.checkAccess(getTestUser(), allowedApiName));
+        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);
+        apiAccessCheckerSpy.addApiToRoleBasedAnnotationsMap(getTestRole().getRoleType(), allowedApiName);
-        assertTrue(apiAccessChecker.checkAccess(getTestUser(), allowedApiName));
+        assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName));
     }
 
+    @Test
+    public void getApisAllowedToUserTestRoleServiceIsDisabledShouldReturnUnchangedList() {
+        Mockito.doReturn(false).when(apiAccessCheckerSpy).isEnabled();
+
+        List<String> 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<String> 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<String> apisReceived = apiAccessCheckerSpy.getApisAllowedToUser(getTestRole(), getTestUser(), apiNames);
+        Assert.assertEquals(0, apisReceived.size());
+    }
 }

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() {
+    @Override
-        return !isEnabled();
+    public List<String> getApisAllowedToUser(Role role, User user, List<String> 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())) {
+        List<ProjectRolePermission> allProjectRolePermissions = projectRoleService.findAllProjectRolePermissions(project.getId(), projectRole.getId());
-            if (permission.getRule().matches(apiCommandName)) {
+        for (String api : apiCommandNames) {
-                if (Permission.ALLOW.equals(permission.getPermission())) {
+            for (ProjectRolePermission permission : allProjectRolePermissions) {
-                    return true;
+                if (permission.getRule().matches(api)) {
-                } else {
+                    return Permission.ALLOW.equals(permission.getPermission());
-                    denyApiAccess(apiCommandName);
                 }
             }
         }

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<String> 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<String> 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<String> 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<String> 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<String> 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<String> apisReceived = projectRoleBasedApiAccessCheckerSpy.getApisAllowedToUser(null, getTestUser(), apiNames);
+        Assert.assertEquals(1, apisReceived.size());
+    }
+}

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();
+        if (roleService.isEnabled()) {
+            LOGGER.debug("RoleService is enabled. We will use it instead of StaticRoleBasedAPIAccessChecker.");
         }
-    public boolean isDisabled() {
         return roleService.isEnabled();
     }
 
+    @Override
+    public List<String> getApisAllowedToUser(Role role, User user, List<String> 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 =
+        if (isApiAllowed(commandName, roleType)) {
-            commandsPropertiesOverrides.contains(commandName) ? commandsPropertiesRoleBasedApisMap.get(roleType).contains(commandName) : annotationRoleBasedApisMap.get(
-                roleType).contains(commandName);
-
-        if (isAllowed) {
             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<String, Object> 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));
             }
         }
     }

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<APIChecker> _apiAccessCheckers = null;
     List<PluggableService> _services = null;
-    private static Map<String, ApiDiscoveryResponse> s_apiNameDiscoveryResponseMap = null;
+    protected static Map<String, ApiDiscoveryResponse> 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<? extends BaseResponse> listApis(User user, String name) {
-        ListResponse<ApiDiscoveryResponse> response = new ListResponse<ApiDiscoveryResponse>();
+        ListResponse<ApiDiscoveryResponse> response = new ListResponse<>();
-        List<ApiDiscoveryResponse> responseList = new ArrayList<ApiDiscoveryResponse>();
+        List<ApiDiscoveryResponse> responseList = new ArrayList<>();
+        List<String> apisAllowed = new ArrayList<>(s_apiNameDiscoveryResponseMap.keySet());
 
         if (user == null)
             return null;
@@ -245,23 +258,34 @@ 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()) {
+            Account account = accountService.getAccount(user.getAccountId());
-                boolean isAllowed = true;
+            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 {
+                    apisAllowed = apiChecker.getApisAllowedToUser(role, user, apisAllowed);
-                        apiChecker.checkAccess(user, apiName);
-                    } catch (Exception ex) {
-                        isAllowed = false;
                 }
             }
-                if (isAllowed)
+
+            for (String apiName: apisAllowed) {
                 responseList.add(s_apiNameDiscoveryResponseMap.get(apiName));
             }
         }

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.acl.Role;
-import org.apache.cloudstack.api.command.user.discovery.ListApisCmd;
+import org.apache.cloudstack.acl.RoleService;
-import org.apache.cloudstack.api.command.user.vm.ListVMsCmd;
+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.junit.Before;
-import org.apache.cloudstack.api.response.ListResponse;
-import org.apache.commons.lang3.StringUtils;
-import org.junit.BeforeClass;
 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.Map;
-import java.util.stream.Collectors;
 
-import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
-import static org.junit.Assert.assertFalse;
+import static org.mockito.ArgumentMatchers.anyList;
-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;
 
+@RunWith(MockitoJUnitRunner.class)
 public class ApiDiscoveryTest {
-    private static APIChecker s_apiChecker = mock(APIChecker.class);
+    @Mock
-    private static PluggableService s_pluggableService = mock(PluggableService.class);
+    AccountService accountServiceMock;
-    private static ApiDiscoveryServiceImpl s_discoveryService = new ApiDiscoveryServiceImpl();
 
-    private static Class<?> testCmdClass = ListApisCmd.class;
+    @Mock
-    private static Class<?> listVMsCmdClass = ListVMsCmd.class;
+    RoleService roleServiceMock;
-    private static User testUser;
-    private static String testApiName;
-    private static String listVmsCmdName;
-    private static String testApiDescription;
-    private static String testApiSince;
-    private static boolean testApiAsync;
 
-    @BeforeClass
+    @Mock
-    public static void setUp() throws ConfigurationException {
+    APIChecker apiCheckerMock;
 
-        listVmsCmdName = listVMsCmdClass.getAnnotation(APICommand.class).name();
+    @Mock
+    Map<String, ApiDiscoveryResponse> apiNameDiscoveryResponseMapMock;
 
-        testApiName = testCmdClass.getAnnotation(APICommand.class).name();
+    @Mock
-        testApiDescription = testCmdClass.getAnnotation(APICommand.class).description();
+    List<APIChecker> apiAccessCheckersMock;
-        testApiSince = testCmdClass.getAnnotation(APICommand.class).since();
-        testApiAsync = false;
-        testUser = new UserVO();
 
-        s_discoveryService._apiAccessCheckers = mock(List.class);
+    @Spy
-        s_discoveryService._services = mock(List.class);
+    @InjectMocks
+    ApiDiscoveryServiceImpl discoveryServiceSpy;
 
-        when(s_apiChecker.checkAccess(any(User.class), anyString())).thenReturn(true);
+    @Before
-        when(s_pluggableService.getCommands()).thenReturn(new ArrayList<Class<?>>());
+    public void setup() {
-        when(s_discoveryService._apiAccessCheckers.iterator()).thenReturn(Arrays.asList(s_apiChecker).iterator());
+        discoveryServiceSpy.s_apiNameDiscoveryResponseMap = apiNameDiscoveryResponseMapMock;
-        when(s_discoveryService._services.iterator()).thenReturn(Arrays.asList(s_pluggableService).iterator());
+        discoveryServiceSpy._apiAccessCheckers = apiAccessCheckersMock;
 
-        Set<Class<?>> cmdClasses = new HashSet<Class<?>>();
+        Mockito.when(discoveryServiceSpy._apiAccessCheckers.iterator()).thenReturn(Arrays.asList(apiCheckerMock).iterator());
-        cmdClasses.add(ListApisCmd.class);
+    }
-        cmdClasses.add(ListVMsCmd.class);
+
-        s_discoveryService.start();
+    private User getTestUser() {
-        s_discoveryService.cacheResponseMap(cmdClasses);
+        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 {
+    public void listApisTestDoesNotGetApisAllowedToUserOnAdminRole() throws PermissionDeniedException {
-        ListResponse<ApiDiscoveryResponse> responses = (ListResponse<ApiDiscoveryResponse>)s_discoveryService.listApis(testUser, testApiName);
+        AccountVO adminAccountVO = new AccountVO("some name", 1L, "network-domain", Account.Type.ADMIN, "some-uuid");
-        assertNotNull("Responses should not be null", responses);
+        RoleVO adminRoleVO = new RoleVO(1L,"name", RoleType.Admin, "description");
-        if (responses != null) {
+
-            ApiDiscoveryResponse response = responses.getResponses().get(0);
+        Mockito.when(accountServiceMock.getAccount(Mockito.anyLong())).thenReturn(adminAccountVO);
-            assertTrue("No. of response items should be one", responses.getCount() == 1);
+        Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(adminRoleVO);
-            assertEquals("Error in api name", testApiName, response.getName());
+
-            assertEquals("Error in api description", testApiDescription, response.getDescription());
+        discoveryServiceSpy.listApis(getTestUser(), null);
-            assertEquals("Error in api since", testApiSince, response.getSince());
+
-            assertEquals("Error in api isAsync", testApiAsync, response.getAsync());
+        Mockito.verify(apiCheckerMock, Mockito.times(0)).getApisAllowedToUser(any(Role.class), any(User.class), anyList());
-        }
     }
 
     @Test
-    public void verifyListApis() throws Exception {
+    public void listApisTestGetsApisAllowedToUserOnUserRole() throws PermissionDeniedException {
-        ListResponse<ApiDiscoveryResponse> responses = (ListResponse<ApiDiscoveryResponse>)s_discoveryService.listApis(testUser, null);
+        RoleVO userRoleVO = new RoleVO(4L, "name", RoleType.User, "description");
-        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());
-            }
-        }
-    }
 
-    @Test
+        Mockito.when(accountServiceMock.getAccount(Mockito.anyLong())).thenReturn(getNormalAccount());
-    public void verifyListVirtualMachinesTagsField() throws Exception {
+        Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(userRoleVO);
-        ListResponse<ApiDiscoveryResponse> responses = (ListResponse<ApiDiscoveryResponse>)s_discoveryService.listApis(testUser, listVmsCmdName);
+
-        assertNotNull("Response should not be null", responses);
+        discoveryServiceSpy.listApis(getTestUser(), null);
-        if (responses != null) {
+
-            assertEquals("No. of response items should be one", 1, (int) responses.getCount());
+        Mockito.verify(apiCheckerMock, Mockito.times(1)).getApisAllowedToUser(any(Role.class), any(User.class), anyList());
-            ApiDiscoveryResponse response = responses.getResponses().get(0);
-            List<ApiResponseResponse> 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);
-        }
     }
 }

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;
 
@@ -140,46 +142,78 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APIChecker,
     }
 
     @Override
-    public boolean checkAccess(User user, String apiCommandName) throws PermissionDeniedException {
+    public List<String> getApisAllowedToUser(Role role, User user, List<String> apiNames) throws PermissionDeniedException {
-        // check if api rate limiting is enabled or not
+        if (!isEnabled()) {
-        if (!enabled) {
+            return apiNames;
-            return true;
-        }
-        Long accountId = user.getAccountId();
-        Account account = _accountService.getAccount(accountId);
-        return checkAccess(account, apiCommandName);
         }
 
-    public boolean checkAccess(Account account, String commandName) {
+        for (int i = 0; i < apiNames.size(); i++) {
-        if (_accountService.isRootAdmin(account.getId())) {
+            if (hasApiRateLimitBeenExceeded(user.getAccountId())) {
-            // no API throttling on root admin
+                throwExceptionDueToApiRateLimitReached(user.getAccountId());
-            return true;
             }
-        StoreEntry entry = _store.get(account.getId());
+        }
-
+        return apiNames;
-        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. */
+    public void throwExceptionDueToApiRateLimitReached(Long accountId) throws RequestLimitException {
-        int current = entry.incrementAndGet();
+        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);
-        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);
     }
+
+    @Override
+    public boolean checkAccess(User user, String apiCommandName) throws PermissionDeniedException {
+        if (!isEnabled()) {
+            return true;
+        }
+
+        Account account = _accountService.getAccount(user.getAccountId());
+        return checkAccess(account, apiCommandName);
+    }
+
+    @Override
+    public boolean checkAccess(Account account, String commandName) {
+        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;
+        }
+        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) {
+            entry = _store.create(account.getId(), timeToLive);
+        }
+
+        int current = entry.incrementAndGet();
+
+        if (current <= maxAllowed) {
+            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;
 
     }
-
 }