DaanHoogland commented on code in PR #6412:
URL: https://github.com/apache/cloudstack/pull/6412#discussion_r884457530
##########
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:
```suggestion
if (LOGGER.isTraceEnabled()) {
LOGGER.trace(String.format("Returning APIs [%s] as allowed for
user [%s].", apiNames, user));
}
```
##########
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));
Review Comment:
isn´t this a normal flow? why make this a `WARN` level message?
##########
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:
seems like the logic is reversed. was this on purpose?
it seems the `StaticRoleBasedAPIAccessChecker` shhould return false if
`roleService.isEnabled() == true`
##########
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:
again, is this really worth a `WARN`?
##########
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()) {
Review Comment:
I think this one should be trace. It could be called a huge amount of times.
##########
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:
```suggestion
if (LOGGER.isTraceEnabled()) {
LOGGER.trace(String.format("Returning APIs [%s] as allowed
for user [%s].", apiNames, user));
}
```
##########
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:
why is this `@Before` now? can't it be `@BeforeClass`?
--
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]
