Skip to content

API key pair restructure follow-ups #13828

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bernardodemarco wants to merge 5 commits into
base: main
Choose a base branch
from
Open

API key pair restructure follow-ups #13828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
20f5556
API key pair restructure follow-ups
bernardodemarco Aug 8, 2026
c0a3341
Fix count response field for the listUserKeys API
bernardodemarco Aug 8, 2026
b9c307b
fix API access check when all rules of the key pair have their corres…
bernardodemarco Aug 14, 2026
1f1ccc9
fix build
bernardodemarco Aug 14, 2026
9747ad0
revert: "Fix count response field for the listUserKeys API"
bernardodemarco Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/src/main/java/com/cloud/user/AccountService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,6 @@ User createUser(String userName, String password, String firstName, String lastN
String getAccessingApiKey(BaseCmd cmd);

List<RolePermissionEntity> getAllKeypairPermissions(String apiKey);

List<? extends ApiKeyPairPermission> getAllExplicitKeyPairPermissions(Long keyPairId);
}
5 changes: 3 additions & 2 deletions api/src/main/java/org/apache/cloudstack/acl/APIChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.cloud.user.Account;
import com.cloud.user.User;
import com.cloud.utils.component.Adapter;
import org.apache.cloudstack.acl.apikeypair.ApiKeyPair;
import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission;

import java.util.List;
Expand All @@ -32,8 +33,8 @@ public interface APIChecker extends Adapter {
// If true, apiChecker has checked the operation
// If false, apiChecker is unable to handle the operation or not implemented
// On exception, checkAccess failed don't allow
boolean checkAccess(User user, String apiCommandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException;
boolean checkAccess(Account account, String apiCommandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException;
boolean checkAccess(User user, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException;
boolean checkAccess(Account account, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException;
/**
* Verifies if the account has permission for the given list of APIs and returns only the allowed ones.
*
Expand Down
10 changes: 6 additions & 4 deletions ...-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import javax.inject.Inject;
import javax.naming.ConfigurationException;

import org.apache.cloudstack.acl.apikeypair.ApiKeyPair;
import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission;
import org.apache.cloudstack.acl.RolePermissionEntity.Permission;
import org.apache.cloudstack.api.APICommand;
Expand Down Expand Up @@ -141,7 +142,7 @@ protected Account getAccountFromIdUsingCache(long accountId) {
}

@Override
public boolean checkAccess(User user, String commandName, ApiKeyPairPermission ... apiKeyPairPermissions) throws PermissionDeniedException {
public boolean checkAccess(User user, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException {
if (!isEnabled()) {
return true;
}
Expand All @@ -150,11 +151,11 @@ public boolean checkAccess(User user, String commandName, ApiKeyPairPermission .
throw new PermissionDeniedException(String.format("Account for user with ID [%s] cannot be found", user.getUuid()));
}

return checkAccess(account, commandName, apiKeyPairPermissions);
return checkAccess(account, commandName, keyPair, apiKeyPairPermissions);
}

@Override
public boolean checkAccess(Account account, String commandName, ApiKeyPairPermission ... apiKeyPairPermissions) {
public boolean checkAccess(Account account, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission ... apiKeyPairPermissions) {
Pair<Role, List<RolePermission>> roleAndPermissions = getRolePermissionsUsingCache(account.getRoleId());
final Role accountRole = roleAndPermissions.first();
if (accountRole == null) {
Expand All @@ -166,7 +167,8 @@ public boolean checkAccess(Account account, String commandName, ApiKeyPairPermis
return true;
}

boolean considerKeyPairPermissions = apiKeyPairPermissions.length > 0;
boolean keyPairHasExplicitPermissions = keyPair != null && !accountService.getAllExplicitKeyPairPermissions(keyPair.getId()).isEmpty();
boolean considerKeyPairPermissions = apiKeyPairPermissions.length > 0 || keyPairHasExplicitPermissions;
List<RolePermissionEntity> allRules = considerKeyPairPermissions ? Arrays.asList(apiKeyPairPermissions) : new ArrayList<>(roleAndPermissions.second());
if (checkApiPermissionByRole(accountRole, commandName, allRules, considerKeyPairPermissions)) {
return true;
Expand Down
34 changes: 17 additions & 17 deletions ...e-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,14 @@ public void setUp() throws NoSuchFieldException, IllegalAccessException {

// Enabled plugin
Mockito.doReturn(true).when(apiAccessCheckerSpy).isEnabled();
Mockito.doCallRealMethod().when(apiAccessCheckerSpy).checkAccess(Mockito.any(User.class), Mockito.anyString());
Mockito.doCallRealMethod().when(apiAccessCheckerSpy).checkAccess(Mockito.any(User.class), Mockito.anyString(), Mockito.any());
}

@Test
public void testInvalidAccountCheckAccess() {
Mockito.when(accountService.getAccount(Mockito.anyLong())).thenReturn(null);
try {
apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi");
apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi", null);
fail("Exception was expected");
} catch (PermissionDeniedException ignored) {
}
Expand All @@ -102,7 +102,7 @@ public void testInvalidAccountCheckAccess() {
public void testInvalidAccountRoleCheckAccess() {
Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(null);
try {
apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi");
apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi", null);
fail("Exception was expected");
} catch (PermissionDeniedException ignored) {
}
Expand All @@ -112,14 +112,14 @@ public void testInvalidAccountRoleCheckAccess() {
public void testDefaultRootAdminAccess() {
Mockito.when(accountService.getAccount(Mockito.anyLong())).thenReturn(new AccountVO("root admin", 1L, null, Account.Type.ADMIN, "some-uuid"));
Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "SomeRole", RoleType.Admin, "default root admin role"));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), "anyApi"));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), "anyApi", null));
}

@Test
public void testInvalidRolePermissionsCheckAccess() {
Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.<RolePermission>emptyList());
try {
apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi");
apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi", null);
fail("Exception was expected");
} catch (PermissionDeniedException ignored) {
}
Expand All @@ -130,15 +130,15 @@ public void testValidAllowRolePermissionApiCheckAccess() {
final String allowedApiName = "someAllowedApi";
final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null);
Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName, null));
}

@Test
public void testValidAllowRolePermissionWildcardCheckAccess() {
final String allowedApiName = "someAllowedApi";
final RolePermission permission = new RolePermissionVO(1L, "some*", Permission.ALLOW, null);
Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName, null));
}

@Test
Expand All @@ -147,7 +147,7 @@ public void testValidDenyRolePermissionApiCheckAccess() {
final RolePermission permission = new RolePermissionVO(1L, denyApiName, Permission.DENY, null);
Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission));
try {
apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName);
apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName, null);
fail("Exception was expected");
} catch (PermissionDeniedException ignored) {
}
Expand All @@ -159,7 +159,7 @@ public void testValidDenyRolePermissionWildcardCheckAccess() {
final RolePermission permission = new RolePermissionVO(1L, "*Deny*", Permission.DENY, null);
Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission));
try {
apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName);
apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName, null);
fail("Exception was expected");
} catch (PermissionDeniedException ignored) {
}
Expand All @@ -169,7 +169,7 @@ public void testValidDenyRolePermissionWildcardCheckAccess() {
public void testAnnotationFallbackCheckAccess() {
final String allowedApiName = "someApiWithAnnotations";
apiAccessCheckerSpy.addApiToRoleBasedAnnotationsMap(getTestRole().getRoleType(), allowedApiName);
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName, null));
}

@Test
Expand Down Expand Up @@ -202,21 +202,21 @@ public void getApisAllowedToUserTestPermissionDenyForGivenApiShouldReturnEmptyLi
public void checkAccessTestInvalidApiKeyPairPermission() {
final String api = "someDeniedApi";
final ApiKeyPairPermission permission = new ApiKeyPairPermissionVO(1L, api, Permission.DENY, null);
assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permission));
assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permission));
}

@Test(expected = UnavailableCommandException.class)
public void checkAccessTestUnrelatedApiKeyPairPermission() {
final String api = "someDeniedApi";
final ApiKeyPairPermission permission = new ApiKeyPairPermissionVO(1L, "apiName", Permission.ALLOW, null);
assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permission));
assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permission));
}

@Test
public void checkAccessTestValidApiKeyPairPermission() {
final String api = "someAllowedApi";
final ApiKeyPairPermission permission = new ApiKeyPairPermissionVO(1L, api, Permission.ALLOW, null);
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permission));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permission));
}

@Test
Expand All @@ -226,7 +226,7 @@ public void checkAccessTestValidMultipleApiKeyPermissions() {
new ApiKeyPairPermissionVO(1L, "someDeniedApi", Permission.DENY, null),
new ApiKeyPairPermissionVO(1L, api, Permission.ALLOW, null)
};
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permissions));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permissions));
}

@Test(expected = UnavailableCommandException.class)
Expand All @@ -236,7 +236,7 @@ public void checkAccessTestInvalidMultipleApiKeyPermissions() {
new ApiKeyPairPermissionVO(1L, "someAllowedApi", Permission.ALLOW, null),
new ApiKeyPairPermissionVO(1L, api, Permission.DENY, null)
};
assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permissions));
assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permissions));
}


Expand All @@ -247,7 +247,7 @@ public void checkAccessTestValidApiKeyPairPermissionWithNullOverride() {
final RolePermission permission = new RolePermissionVO(1L, api, Permission.ALLOW, null);
Mockito.doReturn(Collections.singletonList(permission)).when(roleServiceMock).findAllPermissionsBy(Mockito.anyLong());

assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, emptyPermissionArray));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, emptyPermissionArray));
Mockito.verify(roleServiceMock).findAllPermissionsBy(Mockito.anyLong());
}

Expand All @@ -258,7 +258,7 @@ public void checkAccessTestInvalidApiKeyPairPermissionWithNullOverride() {
final RolePermission permission = new RolePermissionVO(1L, api, Permission.DENY, null);
Mockito.doReturn(Collections.singletonList(permission)).when(roleServiceMock).findAllPermissionsBy(Mockito.anyLong());

assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, emptyPermissionArray));
assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, emptyPermissionArray));
Mockito.verify(roleServiceMock, Mockito.times(1)).findAllPermissionsBy(Mockito.anyLong());
}
}
5 changes: 3 additions & 2 deletions ...-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import javax.naming.ConfigurationException;
import org.apache.cloudstack.acl.RolePermissionEntity.Permission;

import org.apache.cloudstack.acl.apikeypair.ApiKeyPair;
import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission;
import org.apache.cloudstack.context.CallContext;

Expand Down Expand Up @@ -106,7 +107,7 @@ public List<String> getApisAllowedToUser(Role role, User user, List<String> apiN
}

@Override
public boolean checkAccess(User user, String apiCommandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException {
public boolean checkAccess(User user, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException {
if (!isEnabled()) {
return true;
}
Expand Down Expand Up @@ -151,7 +152,7 @@ public boolean checkAccess(User user, String apiCommandName, ApiKeyPairPermissio
}

@Override
public boolean checkAccess(Account account, String apiCommandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException {
public boolean checkAccess(Account account, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException {
return true;
}

Expand Down
7 changes: 4 additions & 3 deletions ...c-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import javax.naming.ConfigurationException;

import com.cloud.exception.UnavailableCommandException;
import org.apache.cloudstack.acl.apikeypair.ApiKeyPair;
import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission;

import org.apache.cloudstack.api.APICommand;
Expand Down Expand Up @@ -92,7 +93,7 @@ public List<String> getApisAllowedToUser(Role role, User user, List<String> apiN
}

@Override
public boolean checkAccess(User user, String commandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException {
public boolean checkAccess(User user, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException {
if (!isEnabled()) {
return true;
}
Expand All @@ -102,11 +103,11 @@ public boolean checkAccess(User user, String commandName, ApiKeyPairPermission..
throw new PermissionDeniedException(String.format("The account with id [%s] for user with uuid [%s] is null.", user.getAccountId(), user.getUuid()));
}

return checkAccess(account, commandName);
return checkAccess(account, commandName, keyPair);
}

@Override
public boolean checkAccess(Account account, String commandName, ApiKeyPairPermission... apiKeyPairPermissions) {
public boolean checkAccess(Account account, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) {
if (!isEnabled()) {
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions .../api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ public List<String> listApiNames(Account account) {
boolean isAllowed = true;
for (APIChecker apiChecker : _apiAccessCheckers) {
try {
apiChecker.checkAccess(account, apiName);
apiChecker.checkAccess(account, apiName, null);
} catch (Exception ex) {
isAllowed = false;
}
Expand Down Expand Up @@ -287,7 +287,7 @@ public ListResponse<? extends BaseResponse> listApis(User user, String name, Lis

for (APIChecker apiChecker : _apiAccessCheckers) {
try {
apiChecker.checkAccess(user, name);
apiChecker.checkAccess(user, name, null);
} catch (Exception ex) {
logger.error(String.format("API discovery access check failed for [%s] with error [%s].", name, ex.getMessage()), ex);
return null;
Expand Down
7 changes: 4 additions & 3 deletions ...api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.cloudstack.acl.Role;
import org.apache.cloudstack.acl.RolePermissionEntity;
import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.acl.apikeypair.ApiKeyPair;
import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission;
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
import org.springframework.stereotype.Component;
Expand Down Expand Up @@ -164,17 +165,17 @@ public void throwExceptionDueToApiRateLimitReached(Long accountId) throws Reques
}

@Override
public boolean checkAccess(User user, String apiCommandName, ApiKeyPairPermission ... apiKeyPairPermissions) throws PermissionDeniedException {
public boolean checkAccess(User user, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission ... apiKeyPairPermissions) throws PermissionDeniedException {
if (!isEnabled()) {
return true;
}

Account account = _accountService.getAccount(user.getAccountId());
return checkAccess(account, apiCommandName, apiKeyPairPermissions);
return checkAccess(account, apiCommandName, keyPair, apiKeyPairPermissions);
}

@Override
public boolean checkAccess(Account account, String commandName, ApiKeyPairPermission ... apiKeyPairPermissions) {
public boolean checkAccess(Account account, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission ... apiKeyPairPermissions) {
Long accountId = account.getAccountId();
if (_accountService.isRootAdmin(accountId)) {
logger.info(String.format("Account [%s] is Root Admin, in this case, API limit does not apply.",
Expand Down
2 changes: 1 addition & 1 deletion plugins/api/rate-limit/src/test/java/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ private User createFakeUser() {

private boolean isUnderLimit(User key) {
try {
s_limitService.checkAccess(key, null);
s_limitService.checkAccess(key, null, null);
return true;
} catch (RequestLimitException ex) {
return false;
Expand Down
5 changes: 5 additions & 0 deletions ...l/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,4 +632,9 @@ public User getOneActiveUserForAccount(Account account) {
public Account getAccountByUuid(String accountUuid) {
return null;
}

@Override
public List<? extends ApiKeyPairPermission> getAllExplicitKeyPairPermissions(Long keyPairId) {
return null;
}
}
Loading
Loading