Skip to content

Commit

Permalink
Merge pull request #45364 from michalvavrik/feature/permissions-check…
Browse files Browse the repository at this point in the history 
…er-so-called-actions

Don't treat colon as a permission-to-action separator in @permissionchecker value attribute
  • Loading branch information
@sberyozkin
sberyozkin authored Jan 14, 2025
2 parents 03782b0 + 6f52091 commit 39af742
Show file tree
Hide file tree
Showing 5 changed files with 513 additions and 58 deletions.
43 changes: 43 additions & 0 deletions docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1122,6 +1122,49 @@ public class ProjectPermissionChecker {
TIP: Permission checks run by default on event loops.
Annotate a permission checker method with the `io.smallrye.common.annotation.Blocking` annotation if you want to run the check on a worker thread.

Matching between the `@PermissionsAllowed` values and the `@PermissionChecker` value is based on string equality as shown in the example below:

[source,java]
----
package org.acme.security;
import io.quarkus.security.PermissionChecker;
import io.quarkus.security.PermissionsAllowed;
import jakarta.enterprise.context.ApplicationScoped;
@ApplicationScoped
public class FileService {
@PermissionsAllowed({ "delete:all", "delete:dir" }) <1>
void deleteDirectory(Path directoryPath) {
// delete directory
}
@PermissionsAllowed(value = { "delete:service", "delete:file" }, inclusive = true) <2>
void deleteServiceFile(Path serviceFilePath) {
// delete service file
}
@PermissionChecker("delete:all")
boolean canDeleteAllDirectories(SecurityIdentity identity) {
String filePermissions = identity.getAttribute("user-group-file-permissions");
return filePermissions != null && filePermissions.contains("w");
}
@PermissionChecker("delete:service")
boolean canDeleteService(SecurityIdentity identity) {
return identity.hasRole("admin");
}
@PermissionChecker("delete:file")
boolean canDeleteFile(Path serviceFilePath) {
return serviceFilePath != null && !serviceFilePath.endsWith("critical");
}
}
----
<1> The permission checker method `canDeleteAllDirectories` grants access to the `deleteDirectory` because the `delete:all` values are equal.
<2> There must be exactly two permission checker methods, one for the `delete:service` permission and other for the `delete:file` permission.

[[permission-meta-annotation]]
==== Create permission meta-annotations

Expand Down
147 changes: 90 additions & 57 deletions ...ity/deployment/src/main/java/io/quarkus/security/deployment/PermissionSecurityChecks.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,22 +134,13 @@ private static Map<String, PermissionCheckerMetadata> getPermissionCheckers(Inde
"supported return types are 'boolean' and 'Uni<Boolean>'. ")
.formatted(toString(checkerMethod), checkerMethod.returnType().name()));
}
var permissionToActions = parsePermissionToActions(annotationInstance.value().asString(), new HashMap<>())
.entrySet().iterator().next();

var permissionName = permissionToActions.getKey();
var permissionName = annotationInstance.value().asString();
if (permissionName.isBlank()) {
throw new IllegalArgumentException(
"@PermissionChecker annotation placed on the '%s' attribute 'value' must not be blank"
.formatted(toString(checkerMethod)));
}
var permissionActions = permissionToActions.getValue();
if (permissionActions != null && !permissionActions.isEmpty()) {
throw new IllegalArgumentException("""
@PermissionChecker annotation instance placed on the '%s' has attribute 'value' with
permission name '%s' and actions '%s', however actions are currently not supported
""".formatted(toString(checkerMethod), permissionName, permissionActions));
}
boolean isBlocking = checkerMethod.hasDeclaredAnnotation(BLOCKING);
if (isBlocking && isReactive) {
throw new IllegalArgumentException("""
Expand Down Expand Up @@ -588,9 +579,47 @@ private <T extends AnnotationTarget> void gatherPermissionKeys(AnnotationInstanc
List<PermissionKey> cache, Map<T, List<List<PermissionKey>>> targetToPermissionKeys) {
// @PermissionsAllowed value is in format permission:action, permission2:action, permission:action2, permission3
// here we transform it to permission -> actions
final var permissionToActions = new HashMap<String, Set<String>>();
for (String permissionToAction : instance.value().asStringArray()) {
parsePermissionToActions(permissionToAction, permissionToActions);
record PermissionNameAndChecker(String permissionName, PermissionCheckerMetadata checker) {
}
boolean foundPermissionChecker = false;
final var permissionToActions = new HashMap<PermissionNameAndChecker, Set<String>>();
for (String permissionValExpression : instance.value().asStringArray()) {
final PermissionCheckerMetadata checker = permissionNameToChecker.get(permissionValExpression);
if (checker != null) {
// matched @PermissionAllowed("value") with @PermissionChecker("value")
foundPermissionChecker = true;
final var permissionNameKey = new PermissionNameAndChecker(permissionValExpression, checker);
if (!permissionToActions.containsKey(permissionNameKey)) {
permissionToActions.put(permissionNameKey, Collections.emptySet());
}
} else if (permissionValExpression.contains(PERMISSION_TO_ACTION_SEPARATOR)) {

// expected format: permission:action
final String[] permissionToActionArr = permissionValExpression.split(PERMISSION_TO_ACTION_SEPARATOR);
if (permissionToActionArr.length != 2) {
throw new RuntimeException(String.format(
"PermissionsAllowed value '%s' contains more than one separator '%2$s', expected format is 'permissionName%2$saction'",
permissionValExpression, PERMISSION_TO_ACTION_SEPARATOR));
}
final PermissionNameAndChecker permissionNameKey = new PermissionNameAndChecker(permissionToActionArr[0],
null);
final String action = permissionToActionArr[1];
if (permissionToActions.containsKey(permissionNameKey)) {
permissionToActions.get(permissionNameKey).add(action);
} else {
final Set<String> actions = new HashSet<>();
actions.add(action);
permissionToActions.put(permissionNameKey, actions);
}
} else {

// expected format: permission
final PermissionNameAndChecker permissionNameKey = new PermissionNameAndChecker(permissionValExpression,
null);
if (!permissionToActions.containsKey(permissionNameKey)) {
permissionToActions.put(permissionNameKey, new HashSet<>());
}
}
}

if (permissionToActions.isEmpty()) {
Expand All @@ -611,12 +640,54 @@ private <T extends AnnotationTarget> void gatherPermissionKeys(AnnotationInstanc
: instance.value("params").asStringArray();
final Type classType = getPermissionClass(instance);
final boolean inclusive = instance.value("inclusive") != null && instance.value("inclusive").asBoolean();

if (inclusive && foundPermissionChecker) {
// @PermissionsAllowed({ "read", "read:all", "read:it", "write" } && @PermissionChecker("read")
// require @PermissionChecker for all 'read:action' because determining expected behavior would be too
// complex; similarly for @PermissionChecker("read:all") require 'read' and 'read:it' have checker as well
List<PermissionNameAndChecker> checkerPermissions = permissionToActions.keySet().stream()
.filter(k -> k.checker != null).toList();
for (PermissionNameAndChecker checkerPermission : checkerPermissions) {
// read -> read
// read:all -> read
String permissionName = checkerPermission.permissionName.contains(PERMISSION_TO_ACTION_SEPARATOR)
? checkerPermission.permissionName.split(PERMISSION_TO_ACTION_SEPARATOR)[0]
: checkerPermission.permissionName;
for (var e : permissionToActions.entrySet()) {
PermissionNameAndChecker permissionNameKey = e.getKey();
// look for permission names that match our permission checker value (before action-to-perm separator)
// for example: read:it
if (permissionNameKey.checker == null && permissionNameKey.permissionName.equals(permissionName)) {
boolean hasActions = e.getValue() != null && !e.getValue().isEmpty();
final String permissionsJoinedWithActions;
if (hasActions) {
permissionsJoinedWithActions = e.getValue()
.stream()
.map(action -> permissionNameKey.permissionName + PERMISSION_TO_ACTION_SEPARATOR
+ action)
.collect(Collectors.joining(", "));
} else {
permissionsJoinedWithActions = permissionNameKey.permissionName;
}
throw new RuntimeException(
"""
@PermissionsAllowed annotation placed on the '%s' has inclusive relation between its permissions.
The '%s' permission has been matched with @PermissionChecker '%s', therefore you must also define
a @PermissionChecker for '%s' permissions.
"""
.formatted(toString(annotationTarget), permissionName,
toString(checkerPermission.checker.checkerMethod),
permissionsJoinedWithActions));
}
}
}
}

for (var permissionToAction : permissionToActions.entrySet()) {
final var permissionName = permissionToAction.getKey();
final var permissionNameKey = permissionToAction.getKey();
final var permissionActions = permissionToAction.getValue();
final var permissionChecker = findPermissionChecker(permissionName, permissionActions);
final var key = new PermissionKey(permissionName, permissionActions, params, classType, inclusive,
permissionChecker, annotationTarget);
final var key = new PermissionKey(permissionNameKey.permissionName, permissionActions, params, classType,
inclusive, permissionNameKey.checker, annotationTarget);
final int i = cache.indexOf(key);
if (i == -1) {
orPermissions.add(key);
Expand All @@ -632,44 +703,6 @@ private <T extends AnnotationTarget> void gatherPermissionKeys(AnnotationInstanc
.add(List.copyOf(orPermissions));
}

private static HashMap<String, Set<String>> parsePermissionToActions(String permissionToAction,
HashMap<String, Set<String>> permissionToActions) {
if (permissionToAction.contains(PERMISSION_TO_ACTION_SEPARATOR)) {

// expected format: permission:action
final String[] permissionToActionArr = permissionToAction.split(PERMISSION_TO_ACTION_SEPARATOR);
if (permissionToActionArr.length != 2) {
throw new RuntimeException(String.format(
"PermissionsAllowed value '%s' contains more than one separator '%2$s', expected format is 'permissionName%2$saction'",
permissionToAction, PERMISSION_TO_ACTION_SEPARATOR));
}
final String permissionName = permissionToActionArr[0];
final String action = permissionToActionArr[1];
if (permissionToActions.containsKey(permissionName)) {
permissionToActions.get(permissionName).add(action);
} else {
final Set<String> actions = new HashSet<>();
actions.add(action);
permissionToActions.put(permissionName, actions);
}
} else {

// expected format: permission
if (!permissionToActions.containsKey(permissionToAction)) {
permissionToActions.put(permissionToAction, new HashSet<>());
}
}
return permissionToActions;
}

private PermissionCheckerMetadata findPermissionChecker(String permissionName, Set<String> permissionActions) {
if (permissionActions != null && !permissionActions.isEmpty()) {
// only permission name is supported for now
return null;
}
return permissionNameToChecker.get(permissionName);
}

private static Type getPermissionClass(AnnotationInstance instance) {
return instance.value(PERMISSION_ATTR) == null ? Type.create(STRING_PERMISSION, Type.Kind.CLASS)
: instance.value(PERMISSION_ATTR).asClass();
Expand Down Expand Up @@ -1361,8 +1394,8 @@ private static SecMethodAndPermCtorIdx[] matchPermCtorParamIdxBasedOnNameMatch(M
: constructor.declaringClass().name().toString();
throw new RuntimeException(String.format(
"No '%s' formal parameter name matches '%s' Permission %s parameter name '%s'",
securedMethod.name(), matchTarget, isQuarkusPermission ? "checker" : "constructor",
constructorParamName));
PermissionSecurityChecksBuilder.toString(securedMethod), matchTarget,
isQuarkusPermission ? "checker" : "constructor", constructorParamName));
}
}
return matches;
Expand Down
45 changes: 45 additions & 0 deletions ...test/permissionsallowed/checker/MissingCheckerForInclusivePermsValidationFailureTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package io.quarkus.security.test.permissionsallowed.checker;

import jakarta.inject.Singleton;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.security.PermissionChecker;
import io.quarkus.security.PermissionsAllowed;
import io.quarkus.security.identity.SecurityIdentity;
import io.quarkus.test.QuarkusUnitTest;

public class MissingCheckerForInclusivePermsValidationFailureTest {

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.assertException(t -> {
Assertions.assertEquals(RuntimeException.class, t.getClass(), t.getMessage());
Assertions.assertTrue(t.getMessage().contains("@PermissionsAllowed annotation placed on"));
Assertions.assertTrue(
t.getMessage().contains("SecuredBean#securedBean' has inclusive relation between its permissions"));
Assertions.assertTrue(t.getMessage().contains("you must also define"));
Assertions.assertTrue(t.getMessage().contains("@PermissionChecker for 'checker:missing' permissions"));
});

@Test
public void test() {
Assertions.fail();
}

@Singleton
public static class SecuredBean {

@PermissionsAllowed(value = { "checker", "checker:missing" }, inclusive = true)
public void securedBean() {
// EMPTY
}

@PermissionChecker("checker")
public boolean check(SecurityIdentity identity) {
return false;
}
}
}
Loading

0 comments on commit 39af742

Please sign in to comment.