Fail deployment if multiple security annotations are present

Fixes quarkusio#10483
gsmet · Jul 16, 2020 · b189411 · b189411
1 parent c32c54d
commit b189411
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 12 deletions.
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
@@ -216,15 +216,16 @@ private Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> gatherSecurityA
             Map<DotName, ClassInfo> additionalSecuredClasses, boolean denyUnannotated) {
 
         Map<MethodInfo, AnnotationInstance> methodToInstanceCollector = new HashMap<>();
+        Map<ClassInfo, AnnotationInstance> classAnnotations = new HashMap<>();
         Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> result = new HashMap<>(gatherSecurityAnnotations(
-                index, DotNames.ROLES_ALLOWED, methodToInstanceCollector,
+                index, DotNames.ROLES_ALLOWED, methodToInstanceCollector, classAnnotations,
                 (instance -> rolesAllowedSecurityCheck(instance.value().asStringArray()))));
-        result.putAll(gatherSecurityAnnotations(index, DotNames.PERMIT_ALL, methodToInstanceCollector,
+        result.putAll(gatherSecurityAnnotations(index, DotNames.PERMIT_ALL, methodToInstanceCollector, classAnnotations,
                 (instance -> permitAllSecurityCheck())));
-        result.putAll(gatherSecurityAnnotations(index, DotNames.AUTHENTICATED, methodToInstanceCollector,
+        result.putAll(gatherSecurityAnnotations(index, DotNames.AUTHENTICATED, methodToInstanceCollector, classAnnotations,
                 (instance -> authenticatedSecurityCheck())));
 
-        result.putAll(gatherSecurityAnnotations(index, DotNames.DENY_ALL, methodToInstanceCollector,
+        result.putAll(gatherSecurityAnnotations(index, DotNames.DENY_ALL, methodToInstanceCollector, classAnnotations,
                 (instance -> denyAllSecurityCheck())));
 
         /*
@@ -279,6 +280,7 @@ private boolean isPublicNonStaticNonConstructor(MethodInfo methodInfo) {
     private Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> gatherSecurityAnnotations(
             IndexView index, DotName dotName,
             Map<MethodInfo, AnnotationInstance> alreadyCheckedMethods,
+            Map<ClassInfo, AnnotationInstance> classLevelAnnotations,
             Function<AnnotationInstance, Function<BytecodeCreator, ResultHandle>> securityCheckInstanceCreator) {
 
         Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> result = new HashMap<>();
@@ -302,16 +304,22 @@ private Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> gatherSecurityA
             AnnotationTarget target = instance.target();
             if (target.kind() == AnnotationTarget.Kind.CLASS) {
                 List<MethodInfo> methods = target.asClass().methods();
-                for (MethodInfo methodInfo : methods) {
-                    AnnotationInstance alreadyExistingInstance = alreadyCheckedMethods.get(methodInfo);
-                    if ((alreadyExistingInstance == null)) {
-                        result.put(methodInfo, securityCheckInstanceCreator.apply(instance));
-                    } else if (alreadyExistingInstance.target().kind() == AnnotationTarget.Kind.CLASS) {
-                        throw new IllegalStateException(
-                                "Class " + methodInfo.declaringClass() + " is annotated with multiple security annotations");
+                AnnotationInstance existingClassInstance = classLevelAnnotations.get(target.asClass());
+                if (existingClassInstance == null) {
+                    classLevelAnnotations.put(target.asClass(), instance);
+                    for (MethodInfo methodInfo : methods) {
+                        AnnotationInstance alreadyExistingInstance = alreadyCheckedMethods.get(methodInfo);
+                        if ((alreadyExistingInstance == null)) {
+                            result.put(methodInfo, securityCheckInstanceCreator.apply(instance));
+                        }
                     }
+                } else {
+                    throw new IllegalStateException(
+                            "Class " + target.asClass() + " is annotated with multiple security annotations " + instance.name()
+                                    + " and " + existingClassInstance.name());
                 }
             }
+
         }
 
         return result;

diff --git a/extensions/security/deployment/src/test/java/io/quarkus/security/test/cdi/InvalidBean.java b/extensions/security/deployment/src/test/java/io/quarkus/security/test/cdi/InvalidBean.java
@@ -0,0 +1,12 @@
+package io.quarkus.security.test.cdi;
+
+import javax.annotation.security.RolesAllowed;
+import javax.enterprise.context.ApplicationScoped;
+
+import io.quarkus.security.Authenticated;
+
+@ApplicationScoped
+@Authenticated
+@RolesAllowed("admin")
+public class InvalidBean {
+}
diff --git a/...urity/deployment/src/test/java/io/quarkus/security/test/cdi/InvalidClassBeanTestCase.java b/...urity/deployment/src/test/java/io/quarkus/security/test/cdi/InvalidClassBeanTestCase.java
@@ -0,0 +1,34 @@
+package io.quarkus.security.test.cdi;
+
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.spec.JavaArchive;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.test.QuarkusUnitTest;
+
+public class InvalidClassBeanTestCase {
+    @RegisterExtension
+    static QuarkusUnitTest test = new QuarkusUnitTest()
+            .setArchiveProducer(new Supplier<JavaArchive>() {
+                @Override
+                public JavaArchive get() {
+                    return ShrinkWrap.create(JavaArchive.class)
+                            .addClass(InvalidBean.class);
+                }
+            }).assertException(new Consumer<Throwable>() {
+                @Override
+                public void accept(Throwable throwable) {
+                    Assertions.assertEquals(IllegalStateException.class, throwable.getClass());
+                }
+            });
+
+    @Test
+    public void testRejected() {
+        Assertions.fail();
+    }
+}
diff --git a/test-framework/junit5-internal/src/main/java/io/quarkus/test/QuarkusUnitTest.java b/test-framework/junit5-internal/src/main/java/io/quarkus/test/QuarkusUnitTest.java
@@ -66,6 +66,8 @@ public class QuarkusUnitTest
         implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback,
         InvocationInterceptor {
 
+    public static final String THE_BUILD_WAS_EXPECTED_TO_FAIL = "The build was expected to fail";
+
     static {
         System.setProperty("java.util.logging.manager", "org.jboss.logmanager.LogManager");
     }
@@ -405,7 +407,7 @@ public void execute(BuildContext context) {
                 //we restore the CL at the end of the test
                 Thread.currentThread().setContextClassLoader(runningQuarkusApplication.getClassLoader());
                 if (assertException != null) {
-                    fail("The build was expected to fail");
+                    fail(THE_BUILD_WAS_EXPECTED_TO_FAIL);
                 }
                 started = true;
                 System.setProperty("test.url", TestHTTPResourceManager.getUri(runningQuarkusApplication));
@@ -424,6 +426,10 @@ public void execute(BuildContext context) {
             } catch (Throwable e) {
                 started = false;
                 if (assertException != null) {
+                    if (e instanceof AssertionError && e.getMessage().equals(THE_BUILD_WAS_EXPECTED_TO_FAIL)) {
+                        //don't pass the 'no failure' assertion into the assert exception function
+                        throw e;
+                    }
                     if (e instanceof RuntimeException) {
                         Throwable cause = e.getCause();
                         if (cause != null && cause instanceof BuildException) {