Copilot commented on code in PR #11137:
URL: https://github.com/apache/cloudstack/pull/11137#discussion_r3043288136
##########
api/src/main/java/org/apache/cloudstack/acl/APIChecker.java:
##########
@@ -43,4 +44,7 @@ public interface APIChecker extends Adapter {
*/
List<String> getApisAllowedToUser(Role role, User user, List<String>
apiNames) throws PermissionDeniedException;
boolean isEnabled();
+
+ default Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
return null; }
+ default boolean checkAccess(Account account, String commandName, Role
accountRole, List<RolePermission> allPermissions) { return false; }
Review Comment:
The new `checkAccess(Account, String, Role, List<RolePermission>)` overload
and `getRolePermissions(long)` are role/ACL-specific, but they were added to
the base `APIChecker` interface (which is also used by non-ACL checkers like
rate limiting). To avoid coupling unrelated checkers to role permission
concepts, consider moving these new methods to `APIAclChecker` (or another
ACL-specific interface) instead of `APIChecker`.
```suggestion
/**
* ACL-specific extension for API checkers that work with roles and role
permissions.
*/
interface APIAclChecker extends APIChecker {
default Pair<Role, List<RolePermission>> getRolePermissions(long
roleId) { return null; }
default boolean checkAccess(Account account, String commandName,
Role accountRole, List<RolePermission> allPermissions) { return false; }
}
```
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -1431,29 +1432,35 @@ protected void checkRoleEscalation(Account caller,
Account requested) {
requested.getUuid(),
requested.getRoleId()));
}
+ if (caller.getRoleId().equals(requested.getRoleId())) {
+ return;
+ }
List<APIChecker> apiCheckers = getEnabledApiCheckers();
+ for (APIChecker apiChecker : apiCheckers) {
+ checkApiAccess(apiChecker, caller, requested);
+ }
Review Comment:
`checkRoleEscalation` iterates over every enabled `APIChecker`. This
includes non-ACL checkers like `ApiRateLimitServiceImpl` which increments
per-account counters in `checkAccess(Account, String)` (and may throw
`RequestLimitException`). That means creating an account can unexpectedly
consume/trigger API rate limits and also interfere with the “requested can
access” probing (exceptions are treated as “irrelevant”). Consider filtering
the checkers used here to `APIAclChecker` only (or otherwise excluding rate
limiting and other non-ACL checkers) so role escalation validation only
evaluates permissions.
##########
plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java:
##########
@@ -180,6 +181,25 @@ public boolean checkAccess(Account account, String
commandName) {
throw new UnavailableCommandException(String.format("The API [%s] does
not exist or is not available for the account %s.", commandName, account));
}
+ @Override
+ public boolean checkAccess(Account account, String commandName, Role
accountRole, List<RolePermission> allPermissions) {
+ if (accountRole == null) {
+ throw new PermissionDeniedException(String.format("The account
[%s] has role null or unknown.", account));
+ }
+
+ if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId()
== RoleType.Admin.getId()) {
+ if (logger.isTraceEnabled()) {
+ logger.trace(String.format("Account [%s] is Root Admin, all
APIs are allowed.", account));
+ }
+ return true;
+ }
+
+ if (checkApiPermissionByRole(accountRole, commandName,
allPermissions)) {
+ return true;
+ }
+ throw new UnavailableCommandException(String.format("The API [%s] does
not exist or is not available for the account %s.", commandName, account));
+ }
Review Comment:
`DynamicRoleBasedAPIAccessChecker` adds a new `checkAccess(Account, String,
Role, List<RolePermission>)` path, but the existing unit tests in this module
only cover `checkAccess(User, String)` / `checkAccess(Account, String)`. Adding
tests for the new overload (allow/deny and root-admin cases) would help prevent
regressions in the new preloaded-permissions flow used during account creation.
##########
api/src/main/java/org/apache/cloudstack/acl/APIChecker.java:
##########
@@ -43,4 +44,7 @@ public interface APIChecker extends Adapter {
*/
List<String> getApisAllowedToUser(Role role, User user, List<String>
apiNames) throws PermissionDeniedException;
boolean isEnabled();
+
+ default Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
return null; }
+ default boolean checkAccess(Account account, String commandName, Role
accountRole, List<RolePermission> allPermissions) { return false; }
Review Comment:
`APIChecker.checkAccess(Account, String, Role, List<RolePermission>)` has a
default implementation that returns `false`. In
`AccountManagerImpl.checkApiAccess(...)` the return value is ignored and only
exceptions deny access, so a `false` default will effectively behave like
“allowed” if any implementation ever returns a non-null value from
`getRolePermissions(...)` but forgets to override this new `checkAccess(...)`
overload. Consider changing the default implementation to delegate to the
existing `checkAccess(Account, String)` (or throw an
`UnsupportedOperationException`) so the safe/legacy behavior is preserved by
default.
```suggestion
default boolean checkAccess(Account account, String commandName, Role
accountRole, List<RolePermission> allPermissions) {
return checkAccess(account, commandName);
}
```
--
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]
