Merge pull request quarkusio#43367 from manovotn/issue33666

Make all security annotations inherited; correct how Arc processes non-inherited interceptor bindings
bschuhmann · Sep 20, 2024 · e9af5d0 · e9af5d0
2 parents c99a22d + 7db7753
commit e9af5d0
Show file tree

Hide file tree

Showing 15 changed files with 334 additions and 7 deletions.
diff --git a/extensions/security/deployment/src/main/java/io/quarkus/security/deployment/DotNames.java b/extensions/security/deployment/src/main/java/io/quarkus/security/deployment/DotNames.java
@@ -1,5 +1,7 @@
 package io.quarkus.security.deployment;
 
+import java.lang.annotation.Inherited;
+
 import jakarta.annotation.security.DenyAll;
 import jakarta.annotation.security.PermitAll;
 import jakarta.annotation.security.RolesAllowed;
@@ -17,6 +19,9 @@ public final class DotNames {
     public static final DotName DENY_ALL = DotName.createSimple(DenyAll.class.getName());
     public static final DotName PERMIT_ALL = DotName.createSimple(PermitAll.class.getName());
 
+    // used to make the above annotations appear as @Inherited to Arc
+    public static final DotName INHERITED = DotName.createSimple(Inherited.class.getName());
+
     private DotNames() {
     }
 }
diff --git a/...s/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java b/...s/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java
@@ -4,6 +4,7 @@
 import static io.quarkus.gizmo.MethodDescriptor.ofMethod;
 import static io.quarkus.security.deployment.DotNames.AUTHENTICATED;
 import static io.quarkus.security.deployment.DotNames.DENY_ALL;
+import static io.quarkus.security.deployment.DotNames.INHERITED;
 import static io.quarkus.security.deployment.DotNames.PERMISSIONS_ALLOWED;
 import static io.quarkus.security.deployment.DotNames.PERMIT_ALL;
 import static io.quarkus.security.deployment.DotNames.ROLES_ALLOWED;
@@ -559,6 +560,17 @@ void transformSecurityAnnotations(BuildProducer<AnnotationsTransformerBuildItem>
         }
     }
 
+    /*
+     * Transform all security annotations to be {@code @Inherited}
+     */
+    @BuildStep
+    void makeSecurityAnnotationsInherited(BuildProducer<AnnotationsTransformerBuildItem> transformer) {
+        Set<DotName> securityAnnotationNames = Set.of(PERMIT_ALL, DENY_ALL, AUTHENTICATED, PERMISSIONS_ALLOWED, ROLES_ALLOWED);
+        transformer.produce(new AnnotationsTransformerBuildItem(AnnotationTransformation.forClasses()
+                .whenClass(c -> securityAnnotationNames.contains(c.name()))
+                .transform(c -> c.add(AnnotationInstance.builder(INHERITED).build()))));
+    }
+
     @BuildStep
     PermissionsAllowedMetaAnnotationBuildItem transformPermissionsAllowedMetaAnnotations(
             BeanArchiveIndexBuildItem beanArchiveBuildItem,

diff --git a/...y/deployment/src/test/java/io/quarkus/security/test/cdi/CdiClassLevelInheritanceTest.java b/...y/deployment/src/test/java/io/quarkus/security/test/cdi/CdiClassLevelInheritanceTest.java
@@ -0,0 +1,93 @@
+package io.quarkus.security.test.cdi;
+
+import static io.quarkus.security.test.utils.IdentityMock.ADMIN;
+import static io.quarkus.security.test.utils.IdentityMock.ANONYMOUS;
+import static io.quarkus.security.test.utils.IdentityMock.USER;
+import static io.quarkus.security.test.utils.SecurityTestUtils.assertFailureFor;
+import static io.quarkus.security.test.utils.SecurityTestUtils.assertSuccess;
+
+import java.util.Collections;
+import java.util.Set;
+
+import jakarta.inject.Inject;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.security.ForbiddenException;
+import io.quarkus.security.StringPermission;
+import io.quarkus.security.UnauthorizedException;
+import io.quarkus.security.test.cdi.app.TestException;
+import io.quarkus.security.test.cdi.inheritance.AuthenticatedBean;
+import io.quarkus.security.test.cdi.inheritance.DenyAllBean;
+import io.quarkus.security.test.cdi.inheritance.PermissionsAllowedBean;
+import io.quarkus.security.test.cdi.inheritance.RolesAllowedBean;
+import io.quarkus.security.test.cdi.inheritance.SubclassAuthenticatedBean;
+import io.quarkus.security.test.cdi.inheritance.SubclassDenyAllBean;
+import io.quarkus.security.test.cdi.inheritance.SubclassPermissionsAllowedBean;
+import io.quarkus.security.test.cdi.inheritance.SubclassRolesAllowedBean;
+import io.quarkus.security.test.utils.AuthData;
+import io.quarkus.security.test.utils.IdentityMock;
+import io.quarkus.security.test.utils.SecurityTestUtils;
+import io.quarkus.test.QuarkusUnitTest;
+
+public class CdiClassLevelInheritanceTest {
+
+    @Inject
+    SubclassDenyAllBean subclassDenyAll;
+
+    @Inject
+    SubclassRolesAllowedBean subclassRolesAllowed;
+
+    @Inject
+    SubclassAuthenticatedBean subclassAuthenticated;
+
+    @Inject
+    SubclassPermissionsAllowedBean subclassPermissionsAllowed;
+
+    @RegisterExtension
+    static final QuarkusUnitTest config = new QuarkusUnitTest()
+            .withApplicationRoot((jar) -> jar
+                    .addClasses(IdentityMock.class,
+                            AuthData.class,
+                            DenyAllBean.class,
+                            RolesAllowedBean.class,
+                            AuthenticatedBean.class,
+                            PermissionsAllowedBean.class,
+                            SubclassPermissionsAllowedBean.class,
+                            SubclassDenyAllBean.class,
+                            SubclassRolesAllowedBean.class,
+                            SubclassAuthenticatedBean.class,
+                            TestException.class,
+                            SecurityTestUtils.class));
+
+    @Test
+    public void testDenyAllInherited() {
+        assertFailureFor(() -> subclassDenyAll.ping(), UnauthorizedException.class, ANONYMOUS);
+        assertFailureFor(() -> subclassDenyAll.ping(), ForbiddenException.class, USER, ADMIN);
+    }
+
+    @Test
+    public void testRolesAllowedInherited() {
+        assertFailureFor(() -> subclassRolesAllowed.ping(), UnauthorizedException.class, ANONYMOUS);
+        assertFailureFor(() -> subclassRolesAllowed.ping(), ForbiddenException.class, USER);
+        assertSuccess(() -> subclassRolesAllowed.ping(), RolesAllowedBean.class.getSimpleName(), ADMIN);
+    }
+
+    @Test
+    public void testAuthenticatedInherited() {
+        assertFailureFor(() -> subclassAuthenticated.ping(), UnauthorizedException.class, ANONYMOUS);
+        assertSuccess(() -> subclassAuthenticated.ping(), AuthenticatedBean.class.getSimpleName(), USER);
+        assertSuccess(() -> subclassAuthenticated.ping(), AuthenticatedBean.class.getSimpleName(), ADMIN);
+    }
+
+    @Test
+    public void testPermissionAllowedInherited() {
+        AuthData USER_READ = new AuthData(Collections.singleton("user_read"), false, "user_read",
+                Set.of(new StringPermission("read")));
+        AuthData USER_WRITE = new AuthData(Collections.singleton("user_write"), false, "user_write",
+                Set.of(new StringPermission("write")));
+        assertSuccess(() -> subclassPermissionsAllowed.ping(), PermissionsAllowedBean.class.getSimpleName(), USER_READ);
+        assertFailureFor(() -> subclassPermissionsAllowed.ping(), ForbiddenException.class, USER_WRITE);
+    }
+}
diff --git a/.../deployment/src/test/java/io/quarkus/security/test/cdi/inheritance/AuthenticatedBean.java b/.../deployment/src/test/java/io/quarkus/security/test/cdi/inheritance/AuthenticatedBean.java
@@ -0,0 +1,14 @@
+package io.quarkus.security.test.cdi.inheritance;
+
+import jakarta.enterprise.context.ApplicationScoped;
+
+import io.quarkus.security.Authenticated;
+
+@ApplicationScoped
+@Authenticated
+public class AuthenticatedBean {
+
+    public String ping() {
+        return AuthenticatedBean.class.getSimpleName();
+    }
+}
diff --git a/...curity/deployment/src/test/java/io/quarkus/security/test/cdi/inheritance/DenyAllBean.java b/...curity/deployment/src/test/java/io/quarkus/security/test/cdi/inheritance/DenyAllBean.java
@@ -0,0 +1,13 @@
+package io.quarkus.security.test.cdi.inheritance;
+
+import jakarta.annotation.security.DenyAll;
+import jakarta.enterprise.context.ApplicationScoped;
+
+@ApplicationScoped
+@DenyAll
+public class DenyAllBean {
+
+    public String ping() {
+        return DenyAllBean.class.getSimpleName();
+    }
+}
diff --git a/...oyment/src/test/java/io/quarkus/security/test/cdi/inheritance/PermissionsAllowedBean.java b/...oyment/src/test/java/io/quarkus/security/test/cdi/inheritance/PermissionsAllowedBean.java
@@ -0,0 +1,14 @@
+package io.quarkus.security.test.cdi.inheritance;
+
+import jakarta.enterprise.context.ApplicationScoped;
+
+import io.quarkus.security.PermissionsAllowed;
+
+@ApplicationScoped
+@PermissionsAllowed("read")
+public class PermissionsAllowedBean {
+
+    public String ping() {
+        return PermissionsAllowedBean.class.getSimpleName();
+    }
+}
diff --git a/...y/deployment/src/test/java/io/quarkus/security/test/cdi/inheritance/RolesAllowedBean.java b/...y/deployment/src/test/java/io/quarkus/security/test/cdi/inheritance/RolesAllowedBean.java
@@ -0,0 +1,13 @@
+package io.quarkus.security.test.cdi.inheritance;
+
+import jakarta.annotation.security.RolesAllowed;
+import jakarta.enterprise.context.ApplicationScoped;
+
+@ApplicationScoped
+@RolesAllowed("admin")
+public class RolesAllowedBean {
+
+    public String ping() {
+        return RolesAllowedBean.class.getSimpleName();
+    }
+}
diff --git a/...ent/src/test/java/io/quarkus/security/test/cdi/inheritance/SubclassAuthenticatedBean.java b/...ent/src/test/java/io/quarkus/security/test/cdi/inheritance/SubclassAuthenticatedBean.java
@@ -0,0 +1,7 @@
+package io.quarkus.security.test.cdi.inheritance;
+
+import jakarta.enterprise.context.ApplicationScoped;
+
+@ApplicationScoped
+public class SubclassAuthenticatedBean extends AuthenticatedBean {
+}
diff --git a/...eployment/src/test/java/io/quarkus/security/test/cdi/inheritance/SubclassDenyAllBean.java b/...eployment/src/test/java/io/quarkus/security/test/cdi/inheritance/SubclassDenyAllBean.java
@@ -0,0 +1,7 @@
+package io.quarkus.security.test.cdi.inheritance;
+
+import jakarta.enterprise.context.ApplicationScoped;
+
+@ApplicationScoped
+public class SubclassDenyAllBean extends DenyAllBean {
+}
diff --git a/...rc/test/java/io/quarkus/security/test/cdi/inheritance/SubclassPermissionsAllowedBean.java b/...rc/test/java/io/quarkus/security/test/cdi/inheritance/SubclassPermissionsAllowedBean.java
@@ -0,0 +1,7 @@
+package io.quarkus.security.test.cdi.inheritance;
+
+import jakarta.enterprise.context.ApplicationScoped;
+
+@ApplicationScoped
+public class SubclassPermissionsAllowedBean extends PermissionsAllowedBean {
+}
diff --git a/...ment/src/test/java/io/quarkus/security/test/cdi/inheritance/SubclassRolesAllowedBean.java b/...ment/src/test/java/io/quarkus/security/test/cdi/inheritance/SubclassRolesAllowedBean.java
@@ -0,0 +1,7 @@
+package io.quarkus.security.test.cdi.inheritance;
+
+import jakarta.enterprise.context.ApplicationScoped;
+
+@ApplicationScoped
+public class SubclassRolesAllowedBean extends RolesAllowedBean {
+}
diff --git a/...pendent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanDeployment.java b/...pendent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanDeployment.java
@@ -686,11 +686,29 @@ Collection<AnnotationInstance> extractQualifiers(AnnotationInstance annotation)
      * @return a collection of interceptor bindings or an empty collection
      */
     public Collection<AnnotationInstance> extractInterceptorBindings(AnnotationInstance annotation) {
+        return extractInterceptorBindings(annotation, false);
+    }
+
+    /**
+     * Behaves exactly as {@link #extractInterceptorBindings(AnnotationInstance)}, but if {@code onlyInherited == true},
+     * then only {@code @Inherited} annotations are returned. This filtering does <em>not</em> apply to transitive
+     * bindings, those are always returned regardless of their {@code @Inherited} status.
+     */
+    Collection<AnnotationInstance> extractInterceptorBindings(AnnotationInstance annotation, boolean onlyInherited) {
         Collection<AnnotationInstance> result = extractAnnotations(annotation, interceptorBindings,
                 repeatingInterceptorBindingAnnotations);
         if (result.isEmpty()) {
             return result;
         }
+        if (onlyInherited) {
+            Set<AnnotationInstance> modifiedResult = new HashSet<>();
+            for (AnnotationInstance ann : result) {
+                if (hasAnnotation(getInterceptorBinding(ann.name()), DotNames.INHERITED)) {
+                    modifiedResult.add(ann);
+                }
+            }
+            result = modifiedResult;
+        }
         Set<AnnotationInstance> transitive = transitiveInterceptorBindings.get(annotation.name());
         if (transitive != null) {
             result = new HashSet<>(result);

diff --git a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java
@@ -922,17 +922,13 @@ private void addClassLevelBindings(ClassInfo targetClass, Collection<AnnotationI
     private void doAddClassLevelBindings(ClassInfo classInfo, Collection<AnnotationInstance> bindings, Set<DotName> skip,
             boolean onlyInherited) {
         beanDeployment.getAnnotations(classInfo).stream()
-                .flatMap(a -> beanDeployment.extractInterceptorBindings(a).stream())
+                .flatMap(a -> beanDeployment.extractInterceptorBindings(a, onlyInherited).stream())
                 .filter(a -> !skip.contains(a.name()))
-                .filter(a -> !onlyInherited
-                        || beanDeployment.hasAnnotation(beanDeployment.getInterceptorBinding(a.name()), DotNames.INHERITED))
                 .forEach(bindings::add);
         if (classInfo.superClassType() != null && !classInfo.superClassType().name().equals(DotNames.OBJECT)) {
             ClassInfo superClass = getClassByName(beanDeployment.getBeanArchiveIndex(), classInfo.superName());
             if (superClass != null) {
-                // proper interceptor binding inheritance only in strict mode, due to Quarkus expecting security
-                // annotations (such as `@RolesAllowed`) to be inherited, even though they are not `@Inherited`
-                doAddClassLevelBindings(superClass, bindings, skip, beanDeployment.strictCompatibility);
+                doAddClassLevelBindings(superClass, bindings, skip, true);
             }
         }
     }

diff --git a/.../java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedBindingOnBeanTest.java b/.../java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedBindingOnBeanTest.java
@@ -29,7 +29,6 @@ public class InheritedBindingOnBeanTest {
     @RegisterExtension
     public ArcTestContainer container = new ArcTestContainer.Builder()
             .beanClasses(MyBean.class, FooBinding.class, BarBinding.class, FooInterceptor.class, BarInterceptor.class)
-            .strictCompatibility(true) // correct interceptor binding inheritance
             .build();
 
     @Test