This is an automated email from the ASF dual-hosted git repository.
harikrishna pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/main by this push:
new 71bc088a70 Improve login time (#6412)
71bc088a70 is described below
commit 71bc088a707b3e9ba526f6185182f156d20a9e94
Author: Bryan Lima <[email protected]>
AuthorDate: Wed Jul 20 03:00:17 2022 -0300
Improve login time (#6412)
* Improve slow login
* Address review
* Address Daan's review
* Address Daniel reviews
---
.../java/org/apache/cloudstack/acl/APIChecker.java | 15 +-
.../main/java/com/cloud/projects/ProjectVO.java | 5 +-
.../src/main/java/com/cloud/user/AccountVO.java | 3 +-
.../src/main/java/com/cloud/user/UserVO.java | 3 +-
.../java/org/apache/cloudstack/acl/RoleVO.java | 6 +
.../acl/DynamicRoleBasedAPIAccessChecker.java | 84 ++++++---
.../acl/DynamicRoleBasedAPIAccessCheckerTest.java | 84 ++++++---
.../acl/ProjectRoleBasedApiAccessChecker.java | 75 ++++++--
.../acl/ProjectRoleBasedApiAccessCheckerTest.java | 154 +++++++++++++++++
.../acl/StaticRoleBasedAPIAccessChecker.java | 55 ++++--
.../discovery/ApiDiscoveryServiceImpl.java | 52 ++++--
.../cloudstack/discovery/ApiDiscoveryTest.java | 188 +++++++++++----------
.../ratelimit/ApiRateLimitServiceImpl.java | 71 +++++---
13 files changed, 581 insertions(+), 214 deletions(-)
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 6cf45456e7..660f64f43e 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<String> getApisAllowedToUser(Role role, User user, List<String>
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 77eed40d7b..c8faa00812 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 5a6170ac7a..50793fbd20 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 027020fc54..94e61ff14a 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 823dc69e87..f5a0cebc75 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 a73a5c0a07..cca9e33886 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<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 {
- 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 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<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();
}
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 5204bd8c04..375aaa707e 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<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.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.<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));
- 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<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());
+ }
}
\ 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 4d33b26c42..9363ebd237 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<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;
}
- Account userAccount = accountService.getAccount(user.getAccountId());
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());
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<ProjectRolePermission> 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 0000000000..3d053e5b28
--- /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<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 protected]", "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());
+ }
+}
\ 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 2b2464f720..27f8305f57 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,43 +65,72 @@ 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<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 =
- 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
@@ -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 378f3540f9..3bdf2e9ce9 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<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>();
- List<ApiDiscoveryResponse> responseList = new
ArrayList<ApiDiscoveryResponse>();
+ ListResponse<ApiDiscoveryResponse> response = new ListResponse<>();
+ List<ApiDiscoveryResponse> responseList = new ArrayList<>();
+ List<String> 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 b0ca23245d..752eb1ac2f 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 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 java.util.Map;
+
+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();
-
- 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;
-
- @BeforeClass
- public static void setUp() throws ConfigurationException {
-
- listVmsCmdName =
listVMsCmdClass.getAnnotation(APICommand.class).name();
-
- testApiName = testCmdClass.getAnnotation(APICommand.class).name();
- testApiDescription =
testCmdClass.getAnnotation(APICommand.class).description();
- testApiSince = testCmdClass.getAnnotation(APICommand.class).since();
- testApiAsync = false;
- testUser = new UserVO();
-
- s_discoveryService._apiAccessCheckers = mock(List.class);
- s_discoveryService._services = mock(List.class);
-
- when(s_apiChecker.checkAccess(any(User.class),
anyString())).thenReturn(true);
- when(s_pluggableService.getCommands()).thenReturn(new
ArrayList<Class<?>>());
-
when(s_discoveryService._apiAccessCheckers.iterator()).thenReturn(Arrays.asList(s_apiChecker).iterator());
-
when(s_discoveryService._services.iterator()).thenReturn(Arrays.asList(s_pluggableService).iterator());
-
- Set<Class<?>> cmdClasses = new HashSet<Class<?>>();
- cmdClasses.add(ListApisCmd.class);
- cmdClasses.add(ListVMsCmd.class);
- s_discoveryService.start();
- s_discoveryService.cacheResponseMap(cmdClasses);
+ @Mock
+ AccountService accountServiceMock;
+
+ @Mock
+ RoleService roleServiceMock;
+
+ @Mock
+ APIChecker apiCheckerMock;
+
+ @Mock
+ Map<String, ApiDiscoveryResponse> apiNameDiscoveryResponseMapMock;
+
+ @Mock
+ List<APIChecker> apiAccessCheckersMock;
+
+ @Spy
+ @InjectMocks
+ ApiDiscoveryServiceImpl discoveryServiceSpy;
+
+ @Before
+ public void setup() {
+ discoveryServiceSpy.s_apiNameDiscoveryResponseMap =
apiNameDiscoveryResponseMapMock;
+ discoveryServiceSpy._apiAccessCheckers = apiAccessCheckersMock;
+
+
Mockito.when(discoveryServiceSpy._apiAccessCheckers.iterator()).thenReturn(Arrays.asList(apiCheckerMock).iterator());
}
- @Test
- public void verifyListSingleApi() throws Exception {
- ListResponse<ApiDiscoveryResponse> responses =
(ListResponse<ApiDiscoveryResponse>)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());
- }
+ private User getTestUser() {
+ return new UserVO(12L, "some user", "password", "firstName",
"lastName",
+ "[email protected]", "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 verifyListApis() throws Exception {
- ListResponse<ApiDiscoveryResponse> responses =
(ListResponse<ApiDiscoveryResponse>)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 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 verifyListVirtualMachinesTagsField() throws Exception {
- ListResponse<ApiDiscoveryResponse> responses =
(ListResponse<ApiDiscoveryResponse>)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<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);
- }
+ public void listApisTestGetsApisAllowedToUserOnUserRole() throws
PermissionDeniedException {
+ RoleVO userRoleVO = new RoleVO(4L, "name", RoleType.User,
"description");
+
+
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 3ed3551414..9fe3cb23e9 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<String> getApisAllowedToUser(Role role, User user,
List<String> 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;
+ }
- if (entry == null) {
+ /**
+ * 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);
- /* Populate the entry, thus unlocking any underlying mutex */
+ if (entry == null) {
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;
}
-
}
