BryanMLima commented on code in PR #6412:
URL: https://github.com/apache/cloudstack/pull/6412#discussion_r885034219
##########
plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java:
##########
@@ -60,59 +60,95 @@ private void denyApiAccess(final String commandName) throws
PermissionDeniedExce
@Override
public boolean isEnabled() {
+ if (!roleService.isEnabled()) {
+ LOGGER.debug("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));
+ }
+ 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));
+ }
+ 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));
Review Comment:
In this case, it makes sense, as this is the method that actually can be
called within a project context.
##########
plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java:
##########
@@ -65,43 +65,72 @@ public StaticRoleBasedAPIAccessChecker() {
}
}
+ /**
+ * 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();
Review Comment:
Yes, it is on purpose. I changed the logic in the lines `84` and `96`. It
makes more sense to me, to check if RoleService is enabled. If it is, then it
is using the dynamic version.
##########
plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java:
##########
@@ -60,59 +60,95 @@ private void denyApiAccess(final String commandName) throws
PermissionDeniedExce
@Override
public boolean isEnabled() {
+ if (!roleService.isEnabled()) {
+ LOGGER.debug("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));
+ }
+ 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));
+ }
+ LOGGER.trace(String.format("Returning APIs [%s] as allowed for user
[%s].", apiNames, user));
Review Comment:
Same here, is there any advantages to add an `isTraceEnabled`?
##########
plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java:
##########
@@ -60,59 +60,95 @@ private void denyApiAccess(final String commandName) throws
PermissionDeniedExce
@Override
public boolean isEnabled() {
+ if (!roleService.isEnabled()) {
+ LOGGER.debug("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));
+ }
+ LOGGER.trace(String.format("Returning APIs [%s] as allowed for
user [%s].", apiNames, user));
Review Comment:
As every var in the log is already allocated, is there any advantages to add
an `isTraceEnabled`?
##########
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() {
Review Comment:
The problem with `@BeforeClass` here it is because is requires a static
method. And thus, requires static fields.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
