BryanMLima commented on code in PR #6412:
URL: https://github.com/apache/cloudstack/pull/6412#discussion_r885031372
##########
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:
To be honest, the method `getApisAllowedToUser` is not needed in the
`ProjectRoleBasedApiAccessChecker` class. As the `listApis` is not called
within a project, as far as I know. However, I maintained to be compatible with
the last code in this section, as I did not tested thoroughly to remove it and
ensure it would work as intended. I pretend to refactor this code later on, and
remove further unnecessary checks, and validate my premise to remove this part.
Furthermore, this log is only called once, so I think is important to log
that there is no project in the context of the API call.
--
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]
