Merge pull request #5104 from geoand/#5051-take2

Ensure that final methods don't prevent CDI interceptors from being applied

extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcConfig.java

@@ -50,6 +50,16 @@ public class ArcConfig {
     @ConfigItem(defaultValue = "true")
     public boolean autoInjectFields;
 
+    /**
+     * If set to true, Arc will transform the bytecode of beans containing methods that need to be proxyable
+     * but have been declared as final. The transformation is simply a matter of removing final.
+     * This ensures that a proxy can be created properly.
+     * If the value is set to false, then an exception is thrown at build time indicating
+     * that a proxy could not be created because a method was final.
+     */
+    @ConfigItem(defaultValue = "true")
+    public boolean removeFinalForProxyableMethods;
+
     public final boolean isRemoveUnusedBeansFieldValid() {
         return ALLOWED_REMOVE_UNUSED_BEANS_VALUES.contains(removeUnusedBeans.toLowerCase());
     }

extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcProcessor.java

@@ -10,6 +10,7 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
@@ -34,6 +35,7 @@
 import io.quarkus.arc.processor.BeanInfo;
 import io.quarkus.arc.processor.BeanProcessor;
 import io.quarkus.arc.processor.BuiltinScope;
+import io.quarkus.arc.processor.BytecodeTransformer;
 import io.quarkus.arc.processor.ContextConfigurator;
 import io.quarkus.arc.processor.ContextRegistrar;
 import io.quarkus.arc.processor.ReflectionRegistration;
@@ -50,6 +52,7 @@
 import io.quarkus.deployment.builditem.AdditionalApplicationArchiveMarkerBuildItem;
 import io.quarkus.deployment.builditem.ApplicationArchivesBuildItem;
 import io.quarkus.deployment.builditem.ApplicationClassPredicateBuildItem;
+import io.quarkus.deployment.builditem.BytecodeTransformerBuildItem;
 import io.quarkus.deployment.builditem.CapabilityBuildItem;
 import io.quarkus.deployment.builditem.ExecutorBuildItem;
 import io.quarkus.deployment.builditem.FeatureBuildItem;
@@ -86,7 +89,7 @@ CapabilityBuildItem capability() {
         return new CapabilityBuildItem(Capabilities.CDI_ARC);
     }
 
-    // PHASE 1 - build BeanProcessor, register custom contexts 
+    // PHASE 1 - build BeanProcessor, register custom contexts
     @BuildStep
     public ContextRegistrationPhaseBuildItem initialize(
             ArcConfig arcConfig,
@@ -223,6 +226,7 @@ public boolean test(BeanInfo bean) {
                 }
             });
         }
+        builder.setRemoveFinalFromProxyableMethods(arcConfig.removeFinalForProxyableMethods);
 
         BeanProcessor beanProcessor = builder.build();
         ContextRegistrar.RegistrationContext context = beanProcessor.registerCustomContexts();
@@ -248,7 +252,8 @@ public BeanRegistrationPhaseBuildItem registerBeans(ContextRegistrationPhaseBuil
     // PHASE 3 - initialize and validate the bean deployment
     @BuildStep
     public ValidationPhaseBuildItem validate(BeanRegistrationPhaseBuildItem beanRegistrationPhase,
-            List<BeanConfiguratorBuildItem> beanConfigurators) {
+            List<BeanConfiguratorBuildItem> beanConfigurators,
+            BuildProducer<BytecodeTransformerBuildItem> bytecodeTransformer) {
 
         for (BeanConfiguratorBuildItem beanConfigurator : beanConfigurators) {
             for (BeanConfigurator<?> value : beanConfigurator.getValues()) {
@@ -257,7 +262,12 @@ public ValidationPhaseBuildItem validate(BeanRegistrationPhaseBuildItem beanRegi
             }
         }
 
-        beanRegistrationPhase.getBeanProcessor().initialize();
+        beanRegistrationPhase.getBeanProcessor().initialize(new Consumer<BytecodeTransformer>() {
+            @Override
+            public void accept(BytecodeTransformer t) {
+                bytecodeTransformer.produce(new BytecodeTransformerBuildItem(t.getClassToTransform(), t.getVisitorFunction()));
+            }
+        });
         return new ValidationPhaseBuildItem(beanRegistrationPhase.getBeanProcessor().validate(),
                 beanRegistrationPhase.getBeanProcessor());
     }
@@ -384,4 +394,4 @@ public boolean test(T t) {
             return false;
         }
     }
-}
+}

extensions/security/deployment/src/test/java/io/quarkus/security/test/cdi/BeanWithSecuredFinalMethod.java

@@ -0,0 +1,19 @@
+package io.quarkus.security.test.cdi;
+
+import javax.annotation.security.DenyAll;
+import javax.annotation.security.RolesAllowed;
+import javax.inject.Singleton;
+
+@Singleton
+public class BeanWithSecuredFinalMethod {
+
+    @RolesAllowed("admin")
+    public final String securedMethod() {
+        return "accessibleForAdminOnly";
+    }
+
+    @DenyAll
+    public final String otherSecuredMethod(String input) {
+        return "denied";
+    }
+}

extensions/security/deployment/src/test/java/io/quarkus/security/test/cdi/SecurityAnnotationOnFinalMethodTest.java

@@ -0,0 +1,45 @@
+package io.quarkus.security.test.cdi;
+
+import static io.quarkus.security.test.cdi.SecurityTestUtils.assertFailureFor;
+import static io.quarkus.security.test.cdi.SecurityTestUtils.assertSuccess;
+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 javax.inject.Inject;
+
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.spec.JavaArchive;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.security.ForbiddenException;
+import io.quarkus.security.UnauthorizedException;
+import io.quarkus.security.test.utils.AuthData;
+import io.quarkus.security.test.utils.IdentityMock;
+import io.quarkus.test.QuarkusUnitTest;
+
+public class SecurityAnnotationOnFinalMethodTest {
+
+    @RegisterExtension
+    static final QuarkusUnitTest config = new QuarkusUnitTest()
+            .setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)
+                    .addClasses(BeanWithSecuredFinalMethod.class, IdentityMock.class,
+                            AuthData.class, SecurityTestUtils.class));
+
+    @Inject
+    BeanWithSecuredFinalMethod bean;
+
+    @Test
+    public void shouldRestrictAccessToSpecificRole() {
+        assertFailureFor(() -> bean.securedMethod(), UnauthorizedException.class, ANONYMOUS);
+        assertSuccess(() -> bean.securedMethod(), "accessibleForAdminOnly", ADMIN);
+    }
+
+    @Test
+    public void shouldFailToAccessCompletely() {
+        assertFailureFor(() -> bean.otherSecuredMethod("whatever"), UnauthorizedException.class, ANONYMOUS);
+        assertFailureFor(() -> bean.otherSecuredMethod("whatever"), ForbiddenException.class, USER, ADMIN);
+    }
+
+}

extensions/security/deployment/src/test/java/io/quarkus/security/test/cdi/SecurityAnnotationOnFinalMethodWithDisableFinalRemovalTest.java

@@ -0,0 +1,39 @@
+package io.quarkus.security.test.cdi;
+
+import static org.junit.jupiter.api.Assertions.fail;
+
+import javax.enterprise.inject.spi.DeploymentException;
+import javax.inject.Inject;
+
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.asset.StringAsset;
+import org.jboss.shrinkwrap.api.spec.JavaArchive;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.security.test.utils.AuthData;
+import io.quarkus.security.test.utils.IdentityMock;
+import io.quarkus.test.QuarkusUnitTest;
+
+public class SecurityAnnotationOnFinalMethodWithDisableFinalRemovalTest {
+
+    @RegisterExtension
+    static final QuarkusUnitTest config = new QuarkusUnitTest()
+            .setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)
+                    .addClasses(BeanWithSecuredFinalMethod.class, IdentityMock.class,
+                            AuthData.class, SecurityTestUtils.class)
+                    .addAsResource(new StringAsset(
+                            "quarkus.arc.remove-final-for-proxyable-methods=false"),
+                            "application.properties"))
+            .setExpectedException(DeploymentException.class);
+
+    @Inject
+    BeanWithSecuredFinalMethod bean;
+
+    @Test
+    public void test() {
+        // should never be executed since the application should not be built
+        fail();
+    }
+
+}

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanDeployment.java

@@ -25,6 +25,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
@@ -88,11 +89,12 @@ public class BeanDeployment {
     private final Map<ScopeInfo, Function<MethodCreator, ResultHandle>> customContexts;
 
     private final Collection<BeanDefiningAnnotation> beanDefiningAnnotations;
+    private final boolean removeFinalForProxyableMethods;
 
     BeanDeployment(IndexView index, Collection<BeanDefiningAnnotation> additionalBeanDefiningAnnotations,
             List<AnnotationsTransformer> annotationTransformers) {
         this(index, additionalBeanDefiningAnnotations, annotationTransformers, Collections.emptyList(), Collections.emptyList(),
-                null, false, null, Collections.emptyMap(), Collections.emptyList());
+                null, false, null, Collections.emptyMap(), Collections.emptyList(), false);
     }
 
     BeanDeployment(IndexView index, Collection<BeanDefiningAnnotation> additionalBeanDefiningAnnotations,
@@ -101,7 +103,7 @@ public class BeanDeployment {
             Collection<DotName> resourceAnnotations,
             BuildContextImpl buildContext, boolean removeUnusedBeans, List<Predicate<BeanInfo>> unusedExclusions,
             Map<DotName, Collection<AnnotationInstance>> additionalStereotypes,
-            List<InterceptorBindingRegistrar> bindingRegistrars) {
+            List<InterceptorBindingRegistrar> bindingRegistrars, boolean removeFinalForProxyableMethods) {
         this.buildContext = buildContext;
         Set<BeanDefiningAnnotation> beanDefiningAnnotations = new HashSet<>();
         if (additionalBeanDefiningAnnotations != null) {
@@ -154,6 +156,7 @@ public class BeanDeployment {
 
         this.beanResolver = new BeanResolver(this);
         this.interceptorResolver = new InterceptorResolver(this);
+        this.removeFinalForProxyableMethods = removeFinalForProxyableMethods;
     }
 
     ContextRegistrar.RegistrationContext registerCustomContexts(List<ContextRegistrar> contextRegistrars) {
@@ -203,19 +206,19 @@ BeanRegistrar.RegistrationContext registerBeans(List<BeanRegistrar> beanRegistra
         return registerSyntheticBeans(beanRegistrars, buildContext);
     }
 
-    void init() {
+    void init(Consumer<BytecodeTransformer> bytecodeTransformerConsumer) {
         long start = System.currentTimeMillis();
 
         // Collect dependency resolution errors
         List<Throwable> errors = new ArrayList<>();
         for (BeanInfo bean : beans) {
-            bean.init(errors);
+            bean.init(errors, bytecodeTransformerConsumer, removeFinalForProxyableMethods);
         }
         for (ObserverInfo observer : observers) {
             observer.init(errors);
         }
         for (InterceptorInfo interceptor : interceptors) {
-            interceptor.init(errors);
+            interceptor.init(errors, bytecodeTransformerConsumer, removeFinalForProxyableMethods);
         }
         processErrors(errors);
 

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java

@@ -398,7 +398,8 @@ void validate(List<Throwable> errors, List<BeanDeploymentValidator> validators)
         }
     }
 
-    void init(List<Throwable> errors) {
+    void init(List<Throwable> errors, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
+            boolean removeFinalForProxyableMethods) {
         for (Injection injection : injections) {
             for (InjectionPointInfo injectionPoint : injection.injectionPoints) {
                 Beans.resolveInjectionPoint(beanDeployment, this, injectionPoint, errors);
@@ -407,7 +408,7 @@ void init(List<Throwable> errors) {
         if (disposer != null) {
             disposer.init(errors);
         }
-        interceptedMethods.putAll(initInterceptedMethods(errors));
+        interceptedMethods.putAll(initInterceptedMethods(errors, bytecodeTransformerConsumer, removeFinalForProxyableMethods));
         if (errors.isEmpty()) {
             lifecycleInterceptors.putAll(initLifecycleInterceptors());
         }
@@ -425,7 +426,8 @@ protected String getType() {
         }
     }
 
-    private Map<MethodInfo, InterceptionInfo> initInterceptedMethods(List<Throwable> errors) {
+    private Map<MethodInfo, InterceptionInfo> initInterceptedMethods(List<Throwable> errors,
+            Consumer<BytecodeTransformer> bytecodeTransformerConsumer, boolean removeFinalForProxyableMethods) {
         if (!isInterceptor() && isClassBean()) {
             Map<MethodInfo, InterceptionInfo> interceptedMethods = new HashMap<>();
             Map<MethodKey, Set<AnnotationInstance>> candidates = new HashMap<>();
@@ -439,7 +441,7 @@ private Map<MethodInfo, InterceptionInfo> initInterceptedMethods(List<Throwable>
             }
 
             Set<MethodInfo> finalMethods = Methods.addInterceptedMethodCandidates(beanDeployment, target.get().asClass(),
-                    candidates, classLevelBindings);
+                    candidates, classLevelBindings, bytecodeTransformerConsumer, removeFinalForProxyableMethods);
             if (!finalMethods.isEmpty()) {
                 errors.add(new DeploymentException(String.format(
                         "Intercepted methods of the bean %s may not be declared final:\n\t- %s", getBeanClass(),

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanProcessor.java

@@ -15,6 +15,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Consumer;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import org.jboss.jandex.AnnotationInstance;
@@ -29,7 +30,7 @@
  * <ol>
  * <li>{@link #registerCustomContexts()}</li>
  * <li>{@link #registerBeans()}</li>
- * <li>{@link #initialize()}</li>
+ * <li>{@link #initialize(Consumer)}</li>
  * <li>{@link #validate()}</li>
  * <li>{@link #processValidationErrors(io.quarkus.arc.processor.BeanDeploymentValidator.ValidationContext)}</li>
  * <li>{@link #generateResources(ReflectionRegistration)}</li>
@@ -73,7 +74,8 @@ private BeanProcessor(String name, IndexView index, Collection<BeanDefiningAnnot
             List<BeanDeploymentValidator> beanDeploymentValidators, Predicate<DotName> applicationClassPredicate,
             boolean unusedBeansRemovalEnabled,
             List<Predicate<BeanInfo>> unusedExclusions, Map<DotName, Collection<AnnotationInstance>> additionalStereotypes,
-            List<InterceptorBindingRegistrar> interceptorBindingRegistrars) {
+            List<InterceptorBindingRegistrar> interceptorBindingRegistrars,
+            boolean removeFinalForProxyableMethods) {
         this.reflectionRegistration = reflectionRegistration;
         this.applicationClassPredicate = applicationClassPredicate;
         this.name = name;
@@ -91,7 +93,7 @@ private BeanProcessor(String name, IndexView index, Collection<BeanDefiningAnnot
                 initAndSort(annotationTransformers, buildContext),
                 initAndSort(injectionPointsTransformers, buildContext), resourceAnnotations, buildContext,
                 unusedBeansRemovalEnabled, unusedExclusions,
-                additionalStereotypes, interceptorBindingRegistrars);
+                additionalStereotypes, interceptorBindingRegistrars, removeFinalForProxyableMethods);
     }
 
     public ContextRegistrar.RegistrationContext registerCustomContexts() {
@@ -102,8 +104,8 @@ public BeanRegistrar.RegistrationContext registerBeans() {
         return beanDeployment.registerBeans(beanRegistrars);
     }
 
-    public void initialize() {
-        beanDeployment.init();
+    public void initialize(Consumer<BytecodeTransformer> bytecodeTransformerConsumer) {
+        beanDeployment.init(bytecodeTransformerConsumer);
     }
 
     public BeanDeploymentValidator.ValidationContext validate() {
@@ -201,7 +203,12 @@ public BeanDeployment getBeanDeployment() {
     public BeanDeployment process() throws IOException {
         registerCustomContexts();
         registerBeans();
-        initialize();
+        initialize(new Consumer<BytecodeTransformer>() {
+            @Override
+            public void accept(BytecodeTransformer transformer) {
+
+            }
+        });
         ValidationContext validationContext = validate();
         processValidationErrors(validationContext);
         generateResources(null);
@@ -242,6 +249,8 @@ public boolean test(DotName dotName) {
             }
         };
 
+        private boolean removeFinalForProxyableMethods;
+
         public Builder setName(String name) {
             this.name = name;
             return this;
@@ -353,12 +362,17 @@ public Builder addRemovalExclusion(Predicate<BeanInfo> exclusion) {
             return this;
         }
 
+        public Builder setRemoveFinalFromProxyableMethods(boolean removeFinalForProxyableMethods) {
+            this.removeFinalForProxyableMethods = removeFinalForProxyableMethods;
+            return this;
+        }
+
         public BeanProcessor build() {
             return new BeanProcessor(name, index, additionalBeanDefiningAnnotations, output, sharedAnnotationLiterals,
                     reflectionRegistration, annotationTransformers, injectionPointTransformers, resourceAnnotations,
                     beanRegistrars, contextRegistrars, beanDeploymentValidators,
                     applicationClassPredicate, removeUnusedBeans, removalExclusions, additionalStereotypes,
-                    additionalInterceptorBindingRegistrars);
+                    additionalInterceptorBindingRegistrars, removeFinalForProxyableMethods);
         }
 
     }