Skip to content
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

Don't treat colon as a permission-to-action separator in @PermissionChecker value attribute #45364

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+514 −58
Open

Don't treat colon as a permission-to-action separator in @PermissionChecker value attribute #45364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Allow colon in @permissionchecker annotation
  • Loading branch information
@michalvavrik
michalvavrik committed Jan 4, 2025
commit 474d17a625eb4cfd2755d1de76ebf50a282b2fbd
44 changes: 44 additions & 0 deletions docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,6 +1122,50 @@ 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 a string equality.
There are no `java.security.Permission` actions and the colon is not the permission to action separator, it's a character as any other.

[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
Open in desktop
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
Open in desktop
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
Loading