Copilot commented on code in PR #11390:
URL: https://github.com/apache/cloudstack/pull/11390#discussion_r2257695733
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -598,21 +598,26 @@ public boolean isAdmin(Long accountId) {
@Override
public boolean isRootAdmin(Long accountId) {
if (accountId != null) {
- AccountVO acct = _accountDao.findById(accountId);
- if (acct == null) {
- return false; //account is deleted or does not exist
- }
- for (SecurityChecker checker : _securityCheckers) {
- try {
- if (checker.checkAccess(acct, null, null,
"SystemCapability")) {
- if (logger.isTraceEnabled()) {
- logger.trace("Root Access granted to " + acct + "
by " + checker.getName());
- }
- return true;
+ return isRootAdmin(_accountDao.findById(accountId));
+ }
+ return false;
+ }
+
Review Comment:
The method `isRootAdmin(Account)` should include proper JavaDoc
documentation explaining the purpose, parameters, return value, and how it
differs from the existing `isRootAdmin(Long)` method.
```suggestion
/**
* Determines if the given {@link Account} has root administrator
privileges.
*
* @param account the {@link Account} object to check for root admin
rights; may be {@code null}
* @return {@code true} if the account is a root admin, {@code false}
otherwise
*
* <p>
* This method differs from {@link #isRootAdmin(Long)} in that it
accepts an {@link Account} object directly,
* whereas {@code isRootAdmin(Long)} takes an account ID and performs a
lookup to retrieve the account.
* Use this method when you already have the {@link Account} object to
avoid unnecessary database lookups.
* </p>
*/
```
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -598,21 +598,26 @@ public boolean isAdmin(Long accountId) {
@Override
public boolean isRootAdmin(Long accountId) {
if (accountId != null) {
- AccountVO acct = _accountDao.findById(accountId);
- if (acct == null) {
- return false; //account is deleted or does not exist
- }
- for (SecurityChecker checker : _securityCheckers) {
- try {
- if (checker.checkAccess(acct, null, null,
"SystemCapability")) {
- if (logger.isTraceEnabled()) {
- logger.trace("Root Access granted to " + acct + "
by " + checker.getName());
- }
- return true;
+ return isRootAdmin(_accountDao.findById(accountId));
+ }
+ return false;
+ }
+
+ @Override
+ public boolean isRootAdmin(Account account) {
+ if (account == null) {
+ return false; //account is deleted or does not exist
Review Comment:
The comment should be updated to reflect that this handles null account
objects, not just deleted/non-existent accounts, since the parameter is already
an Account object rather than an ID.
```suggestion
return false; // account is null (may be deleted, non-existent,
or null reference)
```
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
