Improve login time (apache#6412)

* Improve slow login

* Address review

* Address Daan's review

* Address Daniel reviews

Author: BryanMLima

api/src/main/java/org/apache/cloudstack/acl/APIChecker.java

@@ -21,13 +21,26 @@
 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
     // If false, apiChecker is unable to handle the operation or not implemented
     // 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 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 Date getRemoved() {
 
     @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

engine/schema/src/main/java/com/cloud/user/AccountVO.java

@@ -18,6 +18,7 @@
 
 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 long getAccountId() {
 
     @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 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 void setRegistered(boolean registered) {
 
     @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 void setDescription(String description) {
     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 @@ protected DynamicRoleBasedAPIAccessChecker() {
         }
     }
 
-    private void denyApiAccess(final String commandName) throws PermissionDeniedException {
-        throw new PermissionDeniedException("The API " + commandName + " is denied for the account's role.");
+    @Override
+    public List<String> getApisAllowedToUser(Role role, User user, List<String> apiNames) throws PermissionDeniedException {
+        if (!isEnabled()) {
+            return apiNames;
+        }
+
+        List<RolePermission> allPermissions = roleService.findAllPermissionsBy(role.getId());
+        List<String> 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<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 boolean checkAccess(User user, String commandName) throws PermissionDenie
     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<RolePermission> 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();
     }