Skip to content

Commit bab6d6d

Browse files
bernardodemarcobernardodemarco
committed
consider implicit rules
1 parent 521c48a commit bab6d6d

File tree

4 files changed

+66
-46
lines changed

4 files changed

+66
-46
lines changed

‎plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java‎

Lines changed: 43 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public List<String> getApisAllowedToUser(Role role, User user, List<String> apiN
7575

7676
List<String> allowedApis = new ArrayList<>();
7777
for (String api : apiNames) {
78-
if (checkApiPermissionByRole(role, api, allPermissionEntities, false)) {
78+
if (checkApiPermissionByRole(role, api, allPermissionEntities)) {
7979
allowedApis.add(api);
8080
}
8181
}
@@ -90,7 +90,7 @@ public List<String> getApisAllowedToUser(Role role, User user, List<String> apiN
9090
* @param allPermissions list of role permissions for the given role
9191
* @return if the role has the permission for the API
9292
*/
93-
public boolean checkApiPermissionByRole(Role role, String apiName, List<RolePermissionEntity> allPermissions, boolean keyPairOverride) {
93+
public boolean checkApiPermissionByRole(Role role, String apiName, List<RolePermissionEntity> allPermissions) {
9494
for (RolePermissionEntity permission : allPermissions) {
9595
if (!permission.getRule().matches(apiName)) {
9696
continue;
@@ -106,9 +106,19 @@ public boolean checkApiPermissionByRole(Role role, String apiName, List<RolePerm
106106
return true;
107107
}
108108

109-
return annotationRoleBasedApisMap.get(role.getRoleType()) != null &&
110-
annotationRoleBasedApisMap.get(role.getRoleType()).contains(apiName) &&
111-
!keyPairOverride;
109+
return isApiImplicitlyAllowedForRoleType(role.getRoleType(), apiName);
110+
}
111+
112+
/**
113+
* Returns whether an API is implicitly allowed for a role type. Access to APIs can be
114+
* implicitly managed through the {@link APICommand#authorized()} field of the {@link APICommand} annotation.
115+
* @param roleType role type to check implicit API access.
116+
* @param apiName the name of the API whose implicit access will be checked.
117+
* @return whether the given API is implicitly allowed for the role type.
118+
*/
119+
public boolean isApiImplicitlyAllowedForRoleType(RoleType roleType, String apiName) {
120+
return annotationRoleBasedApisMap.get(roleType) != null &&
121+
annotationRoleBasedApisMap.get(roleType).contains(apiName);
112122
}
113123

114124
protected Account getAccountFromId(long accountId) {
@@ -143,67 +153,59 @@ protected Account getAccountFromIdUsingCache(long accountId) {
143153
}
144154

145155
@Override
146-
public boolean checkAccess(User user, String commandName, ApiKeyPairPermission ... apiKeyPairPermissions) throws PermissionDeniedException {
156+
public boolean checkAccess(User user, String commandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException {
147157
if (!isEnabled()) {
148158
return true;
149159
}
150160
Account account = getAccountFromIdUsingCache(user.getAccountId());
151161
if (account == null) {
152162
throw new PermissionDeniedException(String.format("Account for user id [%s] cannot be found", user.getUuid()));
153163
}
154-
Pair<Role, List<RolePermission>> roleAndPermissions = getRolePermissionsUsingCache(account.getRoleId());
155-
final Role accountRole = roleAndPermissions.first();
156-
if (accountRole == null) {
157-
throw new PermissionDeniedException(String.format("Account role for user id [%s] cannot be found.", user.getUuid()));
158-
}
159-
if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId() == RoleType.Admin.getId()) {
160-
logger.info("Account for user id {} is Root Admin or Domain Admin, all APIs are allowed.", user.getUuid());
161-
return true;
162-
}
163-
164-
List<RolePermissionEntity> allRules = defineNewKeypairRules(accountRole, apiKeyPairPermissions);
165-
boolean override = apiKeyPairPermissions.length != 0;
164+
return checkApisPermissions(account, commandName, apiKeyPairPermissions);
165+
}
166166

167-
if (checkApiPermissionByRole(accountRole, commandName, allRules, override)) {
168-
return true;
169-
}
170-
throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account for user id [%s].", commandName, user.getUuid()));
167+
@Override
168+
public boolean checkAccess(Account account, String commandName, ApiKeyPairPermission... apiKeyPairPermissions) {
169+
return checkApisPermissions(account, commandName, apiKeyPairPermissions);
171170
}
172171

173-
public boolean checkAccess(Account account, String commandName, ApiKeyPairPermission ... apiKeyPairPermissions) {
172+
protected boolean checkApisPermissions(Account account, String commandName, ApiKeyPairPermission... apiKeyPairPermissions) {
174173
Pair<Role, List<RolePermission>> roleAndPermissions = getRolePermissionsUsingCache(account.getRoleId());
175174
final Role accountRole = roleAndPermissions.first();
176175
if (accountRole == null) {
177-
throw new PermissionDeniedException(String.format("The account [%s] has role null or unknown.", account));
176+
throw new PermissionDeniedException(String.format("The role of the account [%s] is null or unknown.", account));
178177
}
179178

180179
if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId() == RoleType.Admin.getId() && apiKeyPairPermissions.length == 0) {
181-
if (logger.isTraceEnabled()) {
182-
logger.trace(String.format("Account [%s] is Root Admin or Domain Admin, all APIs are allowed.", account));
183-
}
180+
logger.trace("Account [{}] is Root Admin and there are not API key pair permissions, so all APIs are allowed.", account);
184181
return true;
185182
}
186183

187-
List<RolePermissionEntity> allRules = defineNewKeypairRules(accountRole, apiKeyPairPermissions);
188-
189-
boolean override = apiKeyPairPermissions.length != 0;
190-
191-
if (checkApiPermissionByRole(accountRole, commandName, allRules, override)) {
184+
List<RolePermissionEntity> allRules = getExplicitPermissions(accountRole, apiKeyPairPermissions);
185+
if (checkApiPermissionByRole(accountRole, commandName, allRules)) {
192186
return true;
193187
}
194188
throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account.getAccountName()));
195189
}
196190

197-
public List<RolePermissionEntity> defineNewKeypairRules(Role accountRole, ApiKeyPairPermission ... apiKeyPairPermissions) {
198-
List<RolePermissionEntity> allPermissions;
199-
if (apiKeyPairPermissions.length == 0) {
200-
List<RolePermission> allRolePermissions = roleService.findAllPermissionsBy(accountRole.getId());
201-
allPermissions = allRolePermissions.stream().map(permission -> (RolePermissionEntity) permission)
202-
.collect(Collectors.toList());
203-
} else {
204-
allPermissions = Arrays.asList(apiKeyPairPermissions);
191+
/**
192+
* Retrieves the explicit permissions associated with the caller. If
193+
* the authentication was established via an API key pair, the permissions from the key pair
194+
* will be returned. If not, then the caller's account role permissions will be considered.
195+
* @param accountRole role of the account whose permissions will be retrieved
196+
* @param apiKeyPairPermissions permissions of the API key pair that will be retrieved
197+
* @return the permissions associated with the caller's account
198+
*/
199+
protected List<RolePermissionEntity> getExplicitPermissions(Role accountRole, ApiKeyPairPermission... apiKeyPairPermissions) {
200+
boolean retrieveRulesFromKeyPair = apiKeyPairPermissions.length > 0;
201+
if (retrieveRulesFromKeyPair) {
202+
return Arrays.asList(apiKeyPairPermissions);
205203
}
206-
return allPermissions;
204+
205+
List<RolePermission> allRolePermissions = roleService.findAllPermissionsBy(accountRole.getId());
206+
return allRolePermissions.stream()
207+
.map(permission -> (RolePermissionEntity) permission)
208+
.collect(Collectors.toList());
207209
}
208210

209211
/**

‎plugins/api/discovery/pom.xml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,12 @@
4545
<version>${project.version}</version>
4646
<scope>compile</scope>
4747
</dependency>
48+
<dependency>
49+
<groupId>org.apache.cloudstack</groupId>
50+
<artifactId>cloud-plugin-acl-dynamic-role-based</artifactId>
51+
<version>${project.version}</version>
52+
<scope>compile</scope>
53+
</dependency>
4854
</dependencies>
4955
<build>
5056
<plugins>

‎plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import javax.inject.Inject;
3232

3333
import org.apache.cloudstack.acl.APIChecker;
34+
import org.apache.cloudstack.acl.DynamicRoleBasedAPIAccessChecker;
3435
import org.apache.cloudstack.acl.Role;
3536
import org.apache.cloudstack.acl.RoleService;
3637
import org.apache.cloudstack.acl.RoleType;
@@ -342,11 +343,11 @@ protected List<ApiDiscoveryResponse> listApisForKeyPair(String apiKey, String ap
342343
.map(apiKeyPairPermission -> (RolePermissionEntity) apiKeyPairPermission).collect(Collectors.toList());
343344

344345
List<String> filteredApis = new ArrayList<>();
345-
if (apiName != null && isApiAllowedForKey(rolePermissionEntities, apiName)) {
346+
if (apiName != null && isApiAllowedForKey(rolePermissionEntities, apiName, role.getRoleType())) {
346347
filteredApis = List.of(apiName);
347348
} else {
348349
for (String api : apisAllowed) {
349-
if (isApiAllowedForKey(rolePermissionEntities, api)) {
350+
if (isApiAllowedForKey(rolePermissionEntities, api, role.getRoleType())) {
350351
filteredApis.add(api);
351352
}
352353
}
@@ -357,14 +358,16 @@ protected List<ApiDiscoveryResponse> listApisForKeyPair(String apiKey, String ap
357358
return filteredApis.stream().map(api -> s_apiNameDiscoveryResponseMap.get(api)).collect(Collectors.toList());
358359
}
359360

360-
protected boolean isApiAllowedForKey(List<RolePermissionEntity> rolePermissionEntities, String apiName) {
361+
protected boolean isApiAllowedForKey(List<RolePermissionEntity> rolePermissionEntities, String apiName, RoleType roleType) {
361362
for (RolePermissionEntity rolePermissionEntity : rolePermissionEntities) {
362363
if (!rolePermissionEntity.getRule().matches(apiName)) {
363364
continue;
364365
}
365366
return rolePermissionEntity.getPermission().equals(RolePermissionEntity.Permission.ALLOW);
366367
}
367-
return false;
368+
369+
DynamicRoleBasedAPIAccessChecker dynamicRoleBasedAPIAccessChecker = getDynamicRoleBasedAPIAccessChecker();
370+
return dynamicRoleBasedAPIAccessChecker != null && dynamicRoleBasedAPIAccessChecker.isApiImplicitlyAllowedForRoleType(roleType, apiName);
368371
}
369372

370373
private void checkRateLimit(User user, Role role, List<String> apiNames) {
@@ -377,6 +380,15 @@ private void checkRateLimit(User user, Role role, List<String> apiNames) {
377380
}
378381
}
379382

383+
private DynamicRoleBasedAPIAccessChecker getDynamicRoleBasedAPIAccessChecker() {
384+
for (APIChecker apiChecker : _apiAccessCheckers) {
385+
if (apiChecker instanceof DynamicRoleBasedAPIAccessChecker) {
386+
return (DynamicRoleBasedAPIAccessChecker) apiChecker;
387+
}
388+
}
389+
return null;
390+
}
391+
380392
public List<APIChecker> getApiAccessCheckers() {
381393
return _apiAccessCheckers;
382394
}

‎server/src/main/java/com/cloud/user/AccountManagerImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3511,7 +3511,7 @@ private ApiKeyPairVO validateAndPersistKeyPairAndPermissions(Account account, Ap
35113511
permissions.forEach(permission -> {
35123512
ApiKeyPairPermissionVO permissionVO = (ApiKeyPairPermissionVO) permission;
35133513
permissionVO.setApiKeyPairId(savedApiKeyPair.getId());
3514-
apiKeyPairPermissionsDao.persist(permissionVO);
3514+
apiKeyPairPermissionsDao.persist(permissionVO);
35153515
});
35163516
return savedApiKeyPair;
35173517
}

0 commit comments

Comments
 (0)