From d802748128cd1932279b7c334f3792d481814ef5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Vav=C5=99=C3=ADk?= Date: Tue, 23 Jan 2024 16:48:20 +0100 Subject: [PATCH] Fix JAX-RS default security checks for inherited / transformed endpoints (cherry picked from commit 6f3d752d157cc8eb22cc86c521cd6f029f67f42f) --- .../RestPathAnnotationProcessor.java | 2 +- .../deployment/ResteasyBuiltinsProcessor.java | 66 ++++++++++++++- .../security/AbstractSecurityEventTest.java | 3 +- .../DefaultRolesAllowedJaxRsTest.java | 16 +++- .../DefaultRolesAllowedStarJaxRsTest.java | 4 +- .../test/security/DenyAllJaxRsTest.java | 16 +++- .../security/UnsecuredParentResource.java | 14 ++++ .../test/security/UnsecuredResource.java | 7 +- .../security/UnsecuredResourceInterface.java | 14 ++++ .../ResteasyReactiveCommonProcessor.java | 53 ++---------- .../security/AbstractSecurityEventTest.java | 3 +- .../DefaultRolesAllowedJaxRsTest.java | 16 +++- .../DefaultRolesAllowedStarJaxRsTest.java | 4 +- .../test/security/DenyAllJaxRsTest.java | 84 ++++++++++++++++++- .../security/UnsecuredParentResource.java | 14 ++++ .../test/security/UnsecuredResource.java | 2 +- .../security/UnsecuredResourceInterface.java | 14 ++++ .../security/EagerSecurityHandler.java | 16 +++- .../deployment/SecurityProcessor.java | 11 +++ .../spi/runtime/SecurityCheckStorage.java | 5 ++ .../runtime/SecurityCheckRecorder.java | 4 + .../SecurityCheckStorageBuilder.java | 13 +++ .../spi/DefaultSecurityCheckBuildItem.java | 28 +++++++ .../common/processor/EndpointIndexer.java | 19 ----- 24 files changed, 339 insertions(+), 89 deletions(-) create mode 100644 extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredParentResource.java create mode 100644 extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResourceInterface.java create mode 100644 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredParentResource.java create mode 100644 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredResourceInterface.java create mode 100644 extensions/security/spi/src/main/java/io/quarkus/security/spi/DefaultSecurityCheckBuildItem.java diff --git a/extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/RestPathAnnotationProcessor.java b/extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/RestPathAnnotationProcessor.java index 5959d9e09c199..94b3a0993718a 100644 --- a/extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/RestPathAnnotationProcessor.java +++ b/extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/RestPathAnnotationProcessor.java @@ -182,7 +182,7 @@ static Optional searchPathAnnotationOnInterfaces(CombinedInd * @param resultAcc accumulator for tail-recursion * @return Collection of all interfaces und their parents. Never null. */ - private static Collection getAllClassInterfaces( + static Collection getAllClassInterfaces( CombinedIndexBuildItem index, Collection classInfos, List resultAcc) { diff --git a/extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/ResteasyBuiltinsProcessor.java b/extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/ResteasyBuiltinsProcessor.java index 2001747fbb72b..e525ee2caddfd 100644 --- a/extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/ResteasyBuiltinsProcessor.java +++ b/extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/ResteasyBuiltinsProcessor.java @@ -1,17 +1,21 @@ package io.quarkus.resteasy.deployment; import static io.quarkus.deployment.annotations.ExecutionTime.STATIC_INIT; +import static io.quarkus.resteasy.deployment.RestPathAnnotationProcessor.getAllClassInterfaces; import static io.quarkus.resteasy.deployment.RestPathAnnotationProcessor.isRestEndpointMethod; import static io.quarkus.security.spi.SecurityTransformerUtils.hasSecurityAnnotation; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Objects; +import java.util.function.Predicate; import java.util.stream.Collectors; import org.jboss.jandex.ClassInfo; import org.jboss.jandex.DotName; import org.jboss.jandex.MethodInfo; +import org.jboss.logging.Logger; import io.quarkus.arc.deployment.AdditionalBeanBuildItem; import io.quarkus.deployment.Capabilities; @@ -50,6 +54,7 @@ public class ResteasyBuiltinsProcessor { protected static final String META_INF_RESOURCES = "META-INF/resources"; + private static final Logger LOG = Logger.getLogger(ResteasyBuiltinsProcessor.class); @BuildStep void setUpDenyAllJaxRs(CombinedIndexBuildItem index, @@ -65,10 +70,42 @@ void setUpDenyAllJaxRs(CombinedIndexBuildItem index, ClassInfo classInfo = index.getIndex().getClassByName(DotName.createSimple(className)); if (classInfo == null) throw new IllegalStateException("Unable to find class info for " + className); - if (!hasSecurityAnnotation(classInfo)) { - for (MethodInfo methodInfo : classInfo.methods()) { - if (isRestEndpointMethod(index, methodInfo) && !hasSecurityAnnotation(methodInfo)) { - methods.add(methodInfo); + // add unannotated class endpoints as well as parent class unannotated endpoints + addAllUnannotatedEndpoints(index, classInfo, methods); + + // interface endpoints implemented on resources are already in, now we need to resolve default interface + // methods as there, CDI interceptors won't work, therefore neither will our additional secured methods + Collection interfaces = getAllClassInterfaces(index, List.of(classInfo), new ArrayList<>()); + if (!interfaces.isEmpty()) { + final List interfaceEndpoints = new ArrayList<>(); + for (ClassInfo anInterface : interfaces) { + addUnannotatedEndpoints(index, anInterface, interfaceEndpoints); + } + // look for implementors as implementors on resource classes are secured by CDI interceptors + if (!interfaceEndpoints.isEmpty()) { + interfaceBlock: for (MethodInfo interfaceEndpoint : interfaceEndpoints) { + if (interfaceEndpoint.isDefault()) { + for (MethodInfo endpoint : methods) { + boolean nameParamsMatch = endpoint.name().equals(interfaceEndpoint.name()) + && (interfaceEndpoint.parameterTypes().equals(endpoint.parameterTypes())); + if (nameParamsMatch) { + // whether matched method is declared on class that implements interface endpoint + Predicate isEndpointInterface = interfaceEndpoint.declaringClass() + .name()::equals; + if (endpoint.declaringClass().interfaceNames().stream().anyMatch(isEndpointInterface)) { + continue interfaceBlock; + } + } + } + String configProperty = config.denyJaxRs ? "quarkus.security.jaxrs.deny-unannotated-endpoints" + : "quarkus.security.jaxrs.default-roles-allowed"; + // this is logging only as I'm a bit worried about false positives and breaking things + // for what is very much edge case + LOG.warn("Default interface method '" + interfaceEndpoint + + "' cannot be secured with the '" + configProperty + + "' configuration property. Please implement this method for CDI " + + "interceptor binding to work"); + } } } } @@ -85,6 +122,27 @@ void setUpDenyAllJaxRs(CombinedIndexBuildItem index, } } + private static void addAllUnannotatedEndpoints(CombinedIndexBuildItem index, ClassInfo classInfo, + List methods) { + if (classInfo == null) { + return; + } + addUnannotatedEndpoints(index, classInfo, methods); + if (classInfo.superClassType() != null && !classInfo.superClassType().name().equals(DotName.OBJECT_NAME)) { + addAllUnannotatedEndpoints(index, index.getIndex().getClassByName(classInfo.superClassType().name()), methods); + } + } + + private static void addUnannotatedEndpoints(CombinedIndexBuildItem index, ClassInfo classInfo, List methods) { + if (!hasSecurityAnnotation(classInfo)) { + for (MethodInfo methodInfo : classInfo.methods()) { + if (isRestEndpointMethod(index, methodInfo) && !hasSecurityAnnotation(methodInfo)) { + methods.add(methodInfo); + } + } + } + } + /** * Install the JAX-RS security provider. */ diff --git a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/AbstractSecurityEventTest.java b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/AbstractSecurityEventTest.java index ee7c11733afc8..007fbb74b414e 100644 --- a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/AbstractSecurityEventTest.java +++ b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/AbstractSecurityEventTest.java @@ -37,7 +37,8 @@ public abstract class AbstractSecurityEventTest { protected static final Class[] TEST_CLASSES = { RolesAllowedResource.class, TestIdentityProvider.class, TestIdentityController.class, - UnsecuredResource.class, UnsecuredSubResource.class, EventObserver.class + UnsecuredResource.class, UnsecuredSubResource.class, EventObserver.class, UnsecuredResourceInterface.class, + UnsecuredParentResource.class }; @Inject diff --git a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedJaxRsTest.java b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedJaxRsTest.java index fb13a3d50cbf6..b9e18eed5475c 100644 --- a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedJaxRsTest.java +++ b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedJaxRsTest.java @@ -22,8 +22,8 @@ public class DefaultRolesAllowedJaxRsTest { static QuarkusUnitTest runner = new QuarkusUnitTest() .withApplicationRoot((jar) -> jar .addClasses(PermitAllResource.class, UnsecuredResource.class, - TestIdentityProvider.class, - TestIdentityController.class, + TestIdentityProvider.class, UnsecuredResourceInterface.class, + TestIdentityController.class, UnsecuredParentResource.class, UnsecuredSubResource.class, HelloResource.class) .addAsResource(new StringAsset("quarkus.security.jaxrs.default-roles-allowed = admin\n"), "application.properties")); @@ -41,6 +41,18 @@ public void shouldDenyUnannotated() { assertStatus(path, 200, 403, 401); } + @Test + public void shouldDenyUnannotatedOnParentClass() { + String path = "/unsecured/defaultSecurityParent"; + assertStatus(path, 200, 403, 401); + } + + @Test + public void shouldDenyUnannotatedOnInterface() { + String path = "/unsecured/defaultSecurityInterface"; + assertStatus(path, 200, 403, 401); + } + @Test public void shouldDenyDenyAllMethod() { String path = "/unsecured/denyAll"; diff --git a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedStarJaxRsTest.java b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedStarJaxRsTest.java index 4e0fd8c7dd808..ddcc31f5ae87e 100644 --- a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedStarJaxRsTest.java +++ b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedStarJaxRsTest.java @@ -17,8 +17,8 @@ public class DefaultRolesAllowedStarJaxRsTest { static QuarkusUnitTest runner = new QuarkusUnitTest() .withApplicationRoot((jar) -> jar .addClasses(PermitAllResource.class, UnsecuredResource.class, - TestIdentityProvider.class, - TestIdentityController.class, + TestIdentityProvider.class, UnsecuredParentResource.class, + TestIdentityController.class, UnsecuredResourceInterface.class, UnsecuredSubResource.class) .addAsResource(new StringAsset("quarkus.security.jaxrs.default-roles-allowed = **\n"), "application.properties")); diff --git a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DenyAllJaxRsTest.java b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DenyAllJaxRsTest.java index 90cd2a9f77390..8ed4d8cbf445c 100644 --- a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DenyAllJaxRsTest.java +++ b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DenyAllJaxRsTest.java @@ -26,8 +26,8 @@ public class DenyAllJaxRsTest { static QuarkusUnitTest runner = new QuarkusUnitTest() .withApplicationRoot((jar) -> jar .addClasses(PermitAllResource.class, UnsecuredResource.class, - TestIdentityProvider.class, - TestIdentityController.class, + TestIdentityProvider.class, UnsecuredParentResource.class, + TestIdentityController.class, UnsecuredResourceInterface.class, UnsecuredSubResource.class, HelloResource.class) .addAsResource(new StringAsset("quarkus.security.jaxrs.deny-unannotated-endpoints = true\n"), "application.properties")); @@ -58,6 +58,18 @@ public void shouldDenyUnannotated() { assertStatus(path, 403, 401); } + @Test + public void shouldDenyUnannotatedOnParentClass() { + String path = "/unsecured/defaultSecurityParent"; + assertStatus(path, 403, 401); + } + + @Test + public void shouldDenyUnannotatedOnInterface() { + String path = "/unsecured/defaultSecurityInterface"; + assertStatus(path, 403, 401); + } + @Test public void shouldDenyDenyAllMethod() { String path = "/unsecured/denyAll"; diff --git a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredParentResource.java b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredParentResource.java new file mode 100644 index 0000000000000..abf5b385e9a0d --- /dev/null +++ b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredParentResource.java @@ -0,0 +1,14 @@ +package io.quarkus.resteasy.test.security; + +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; + +public class UnsecuredParentResource { + + @Path("/defaultSecurityParent") + @GET + public String defaultSecurityParent() { + return "defaultSecurityParent"; + } + +} diff --git a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java index 116f041ba538d..7339c9c1eda07 100644 --- a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java +++ b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java @@ -12,13 +12,18 @@ * @author Michal Szynkiewicz, michal.l.szynkiewicz@gmail.com */ @Path("/unsecured") -public class UnsecuredResource { +public class UnsecuredResource extends UnsecuredParentResource implements UnsecuredResourceInterface { @Path("/defaultSecurity") @GET public String defaultSecurity() { return "defaultSecurity"; } + @Override + public String defaultSecurityInterface() { + return UnsecuredResourceInterface.super.defaultSecurityInterface(); + } + @Path("/permitAllPathParam/{index}") @GET @PermitAll diff --git a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResourceInterface.java b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResourceInterface.java new file mode 100644 index 0000000000000..d2498d46a8c63 --- /dev/null +++ b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResourceInterface.java @@ -0,0 +1,14 @@ +package io.quarkus.resteasy.test.security; + +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; + +public interface UnsecuredResourceInterface { + + @Path("/defaultSecurityInterface") + @GET + default String defaultSecurityInterface() { + return "defaultSecurityInterface"; + } + +} diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive-common/deployment/src/main/java/io/quarkus/resteasy/reactive/common/deployment/ResteasyReactiveCommonProcessor.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive-common/deployment/src/main/java/io/quarkus/resteasy/reactive/common/deployment/ResteasyReactiveCommonProcessor.java index ff52973878de5..272d25e5b98f1 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive-common/deployment/src/main/java/io/quarkus/resteasy/reactive/common/deployment/ResteasyReactiveCommonProcessor.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive-common/deployment/src/main/java/io/quarkus/resteasy/reactive/common/deployment/ResteasyReactiveCommonProcessor.java @@ -1,13 +1,9 @@ package io.quarkus.resteasy.reactive.common.deployment; -import static io.quarkus.security.spi.SecurityTransformerUtils.hasSecurityAnnotation; import static org.jboss.resteasy.reactive.common.model.ResourceInterceptor.FILTER_SOURCE_METHOD_METADATA_KEY; -import static org.jboss.resteasy.reactive.common.processor.EndpointIndexer.collectClassEndpoints; import java.io.ByteArrayInputStream; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -76,7 +72,7 @@ import io.quarkus.resteasy.reactive.spi.MessageBodyWriterOverrideBuildItem; import io.quarkus.resteasy.reactive.spi.ReaderInterceptorBuildItem; import io.quarkus.resteasy.reactive.spi.WriterInterceptorBuildItem; -import io.quarkus.security.spi.AdditionalSecuredMethodsBuildItem; +import io.quarkus.security.spi.DefaultSecurityCheckBuildItem; public class ResteasyReactiveCommonProcessor { @@ -134,46 +130,13 @@ void searchForProviders(Capabilities capabilities, } @BuildStep - void setUpDenyAllJaxRs( - CombinedIndexBuildItem index, - JaxRsSecurityConfig securityConfig, - Optional resteasyDeployment, - BeanArchiveIndexBuildItem beanArchiveIndexBuildItem, - ApplicationResultBuildItem applicationResultBuildItem, - BuildProducer additionalSecuredClasses) { - - if (resteasyDeployment.isPresent() - && (securityConfig.denyJaxRs() || securityConfig.defaultRolesAllowed().isPresent())) { - final List methods = new ArrayList<>(); - Map httpAnnotationToMethod = resteasyDeployment.get().getResult().getHttpAnnotationToMethod(); - Set resourceClasses = resteasyDeployment.get().getResult().getScannedResourcePaths().keySet(); - - for (DotName className : resourceClasses) { - ClassInfo classInfo = index.getIndex().getClassByName(className); - if (classInfo == null) - throw new IllegalStateException("Unable to find class info for " + className); - if (!hasSecurityAnnotation(classInfo)) { - // collect class endpoints - Collection classEndpoints = collectClassEndpoints(classInfo, httpAnnotationToMethod, - beanArchiveIndexBuildItem.getIndex(), applicationResultBuildItem.getResult()); - - // add endpoints - for (MethodInfo classEndpoint : classEndpoints) { - if (!hasSecurityAnnotation(classEndpoint)) { - methods.add(classEndpoint); - } - } - } - } - - if (!methods.isEmpty()) { - if (securityConfig.denyJaxRs()) { - additionalSecuredClasses.produce(new AdditionalSecuredMethodsBuildItem(methods)); - } else { - additionalSecuredClasses - .produce(new AdditionalSecuredMethodsBuildItem(methods, securityConfig.defaultRolesAllowed())); - } - } + void setUpDenyAllJaxRs(JaxRsSecurityConfig securityConfig, + BuildProducer defaultSecurityCheckProducer) { + if (securityConfig.denyJaxRs()) { + defaultSecurityCheckProducer.produce(DefaultSecurityCheckBuildItem.denyAll()); + } else if (securityConfig.defaultRolesAllowed().isPresent()) { + defaultSecurityCheckProducer + .produce(DefaultSecurityCheckBuildItem.rolesAllowed(securityConfig.defaultRolesAllowed().get())); } } diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/AbstractSecurityEventTest.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/AbstractSecurityEventTest.java index cc8d3a9db7199..1d3366fd2220a 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/AbstractSecurityEventTest.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/AbstractSecurityEventTest.java @@ -40,7 +40,8 @@ public abstract class AbstractSecurityEventTest { protected static final Class[] TEST_CLASSES = { RolesAllowedResource.class, RolesAllowedBlockingResource.class, TestIdentityProvider.class, TestIdentityController.class, UnsecuredResource.class, UnsecuredSubResource.class, RolesAllowedService.class, - RolesAllowedServiceResource.class, EventObserver.class + RolesAllowedServiceResource.class, EventObserver.class, UnsecuredResourceInterface.class, + UnsecuredParentResource.class }; @Inject diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DefaultRolesAllowedJaxRsTest.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DefaultRolesAllowedJaxRsTest.java index 4590ee9a9d02f..ca34474ad9052 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DefaultRolesAllowedJaxRsTest.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DefaultRolesAllowedJaxRsTest.java @@ -18,9 +18,9 @@ public class DefaultRolesAllowedJaxRsTest { static QuarkusUnitTest runner = new QuarkusUnitTest() .withApplicationRoot((jar) -> jar .addClasses(PermitAllResource.class, UnsecuredResource.class, - TestIdentityProvider.class, + TestIdentityProvider.class, UnsecuredResourceInterface.class, TestIdentityController.class, - UnsecuredSubResource.class, HelloResource.class) + UnsecuredSubResource.class, HelloResource.class, UnsecuredParentResource.class) .addAsResource(new StringAsset("quarkus.security.jaxrs.default-roles-allowed=admin\n"), "application.properties")); @@ -37,6 +37,18 @@ public void shouldDenyUnannotated() { assertStatus(path, 200, 403, 401); } + @Test + public void shouldDenyUnannotatedParent() { + String path = "/unsecured/defaultSecurityParent"; + assertStatus(path, 200, 403, 401); + } + + @Test + public void shouldDenyUnannotatedInterface() { + String path = "/unsecured/defaultSecurityInterface"; + assertStatus(path, 200, 403, 401); + } + @Test public void shouldDenyDenyAllMethod() { String path = "/unsecured/denyAll"; diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DefaultRolesAllowedStarJaxRsTest.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DefaultRolesAllowedStarJaxRsTest.java index dcad10e88e0ce..6f77ef2fff88e 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DefaultRolesAllowedStarJaxRsTest.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DefaultRolesAllowedStarJaxRsTest.java @@ -17,8 +17,8 @@ public class DefaultRolesAllowedStarJaxRsTest { static QuarkusUnitTest runner = new QuarkusUnitTest() .withApplicationRoot((jar) -> jar .addClasses(PermitAllResource.class, UnsecuredResource.class, - TestIdentityProvider.class, - TestIdentityController.class, + TestIdentityProvider.class, UnsecuredResourceInterface.class, + TestIdentityController.class, UnsecuredParentResource.class, UnsecuredSubResource.class) .addAsResource(new StringAsset("quarkus.security.jaxrs.default-roles-allowed = **\n"), "application.properties")); diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DenyAllJaxRsTest.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DenyAllJaxRsTest.java index 2e10fb07015e4..76f23ddcc8056 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DenyAllJaxRsTest.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/DenyAllJaxRsTest.java @@ -3,12 +3,25 @@ import static io.restassured.RestAssured.given; import static io.restassured.RestAssured.when; +import java.lang.reflect.Modifier; + +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; + import org.hamcrest.Matchers; +import org.jboss.jandex.AnnotationTarget; +import org.jboss.jandex.AnnotationValue; +import org.jboss.jandex.ClassInfo; +import org.jboss.jandex.MethodInfo; +import org.jboss.resteasy.reactive.common.processor.transformation.AnnotationsTransformer; import org.jboss.shrinkwrap.api.asset.StringAsset; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import io.quarkus.builder.BuildContext; +import io.quarkus.builder.BuildStep; +import io.quarkus.resteasy.reactive.server.spi.AnnotationsTransformerBuildItem; import io.quarkus.security.test.utils.TestIdentityController; import io.quarkus.security.test.utils.TestIdentityProvider; import io.quarkus.test.QuarkusUnitTest; @@ -21,11 +34,43 @@ public class DenyAllJaxRsTest { static QuarkusUnitTest runner = new QuarkusUnitTest() .withApplicationRoot((jar) -> jar .addClasses(PermitAllResource.class, UnsecuredResource.class, - TestIdentityProvider.class, - TestIdentityController.class, - UnsecuredSubResource.class, HelloResource.class) + TestIdentityProvider.class, UnsecuredResourceInterface.class, + TestIdentityController.class, SpecialResource.class, + UnsecuredSubResource.class, HelloResource.class, UnsecuredParentResource.class) .addAsResource(new StringAsset("quarkus.security.jaxrs.deny-unannotated-endpoints = true\n"), - "application.properties")); + "application.properties")) + .addBuildChainCustomizer(builder -> { + builder.addBuildStep(new BuildStep() { + @Override + public void execute(BuildContext context) { + // Here we add an AnnotationsTransformer in order to make sure that the security layer + // uses the proper set of transformers + context.produce( + new AnnotationsTransformerBuildItem( + AnnotationsTransformer.builder().appliesTo(AnnotationTarget.Kind.METHOD) + .transform(transformerContext -> { + // This transformer auto-adds @GET and @Path if missing, thus emulating Renarde + MethodInfo methodInfo = transformerContext.getTarget().asMethod(); + ClassInfo declaringClass = methodInfo.declaringClass(); + if (declaringClass.name().toString().equals(SpecialResource.class.getName()) + && !methodInfo.isConstructor() + && !Modifier.isStatic(methodInfo.flags())) { + if (methodInfo.declaredAnnotation(GET.class.getName()) == null) { + // auto-add it + transformerContext.transform().add(GET.class).done(); + } + if (methodInfo.declaredAnnotation(Path.class.getName()) == null) { + // auto-add it + transformerContext.transform().add(Path.class, + AnnotationValue.createStringValue("value", + methodInfo.name())) + .done(); + } + } + }))); + } + }).produces(AnnotationsTransformerBuildItem.class).build(); + }); @BeforeAll public static void setupUsers() { @@ -40,6 +85,18 @@ public void shouldDenyUnannotated() { assertStatus(path, 403, 401); } + @Test + public void shouldDenyUnannotatedOnParentClass() { + String path = "/unsecured/defaultSecurityParent"; + assertStatus(path, 403, 401); + } + + @Test + public void shouldDenyUnannotatedOnInterface() { + String path = "/unsecured/defaultSecurityInterface"; + assertStatus(path, 403, 401); + } + @Test public void shouldDenyUnannotatedNonBlocking() { String path = "/unsecured/defaultSecurityNonBlocking"; @@ -90,6 +147,14 @@ public void testServerExceptionMapper() { .body(Matchers.equalTo("unauthorizedExceptionMapper")); } + @Test + public void shouldDenyUnannotatedWithAnnotationTransformer() { + String path = "/special/explicit"; + assertStatus(path, 403, 401); + path = "/special/implicit"; + assertStatus(path, 403, 401); + } + private void assertStatus(String path, int status, int anonStatus) { given().auth().preemptive() .basic("admin", "admin").get(path) @@ -105,4 +170,15 @@ private void assertStatus(String path, int status, int anonStatus) { } + @Path("/special") + public static class SpecialResource { + @GET + public String explicit() { + return "explicit"; + } + + public String implicit() { + return "implicit"; + } + } } diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredParentResource.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredParentResource.java new file mode 100644 index 0000000000000..8250d5a9bf9a6 --- /dev/null +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredParentResource.java @@ -0,0 +1,14 @@ +package io.quarkus.resteasy.reactive.server.test.security; + +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; + +public class UnsecuredParentResource { + + @Path("/defaultSecurityParent") + @GET + public String defaultSecurityParent() { + return "defaultSecurityParent"; + } + +} diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredResource.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredResource.java index bca5025db0a5f..82622c3e585de 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredResource.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredResource.java @@ -12,7 +12,7 @@ * @author Michal Szynkiewicz, michal.l.szynkiewicz@gmail.com */ @Path("/unsecured") -public class UnsecuredResource { +public class UnsecuredResource extends UnsecuredParentResource implements UnsecuredResourceInterface { @Path("/defaultSecurity") @GET public String defaultSecurity() { diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredResourceInterface.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredResourceInterface.java new file mode 100644 index 0000000000000..7be652f15ef8d --- /dev/null +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/UnsecuredResourceInterface.java @@ -0,0 +1,14 @@ +package io.quarkus.resteasy.reactive.server.test.security; + +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; + +public interface UnsecuredResourceInterface { + + @Path("/defaultSecurityInterface") + @GET + default String defaultSecurityInterface() { + return "defaultSecurityInterface"; + } + +} diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/security/EagerSecurityHandler.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/security/EagerSecurityHandler.java index 00b25676a4bd4..f94c369070cf0 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/security/EagerSecurityHandler.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/security/EagerSecurityHandler.java @@ -66,9 +66,14 @@ public void handle(ResteasyReactiveRequestContext requestContext) throws Excepti ResteasyReactiveResourceInfo lazyMethod = requestContext.getTarget().getLazyMethod(); MethodDescription methodDescription = lazyMethodToMethodDescription(lazyMethod); if (check == null) { - check = Arc.container().instance(SecurityCheckStorage.class).get().getSecurityCheck(methodDescription); + SecurityCheckStorage storage = Arc.container().instance(SecurityCheckStorage.class).get(); + check = storage.getSecurityCheck(methodDescription); if (check == null) { - check = NULL_SENTINEL; + if (storage.getDefaultSecurityCheck() == null || isRequestAlreadyChecked(requestContext)) { + check = NULL_SENTINEL; + } else { + check = storage.getDefaultSecurityCheck(); + } } this.check = check; } @@ -193,6 +198,13 @@ private void preventRepeatedSecurityChecks(ResteasyReactiveRequestContext reques requestContext.setProperty(STANDARD_SECURITY_CHECK_INTERCEPTOR, methodDescription); } + private boolean isRequestAlreadyChecked(ResteasyReactiveRequestContext requestContext) { + // when request has already been checked at least once (by another instance of this handler) + // then default security checks, like denied access to all JAX-RS resources by default + // shouldn't be applied; this doesn't mean security checks registered for methods shouldn't be applied + return requestContext.getProperty(STANDARD_SECURITY_CHECK_INTERCEPTOR) != null; + } + private InjectableInstance getCurrentIdentityAssociation() { InjectableInstance identityAssociation = this.currentIdentityAssociation; if (identityAssociation == null) { diff --git a/extensions/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java b/extensions/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java index d781085f835ac..8aa777f640b67 100644 --- a/extensions/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java +++ b/extensions/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java @@ -105,6 +105,7 @@ import io.quarkus.security.runtime.interceptor.SecurityHandler; import io.quarkus.security.spi.AdditionalSecuredClassesBuildItem; import io.quarkus.security.spi.AdditionalSecuredMethodsBuildItem; +import io.quarkus.security.spi.DefaultSecurityCheckBuildItem; import io.quarkus.security.spi.RolesAllowedConfigExpResolverBuildItem; import io.quarkus.security.spi.runtime.AuthorizationController; import io.quarkus.security.spi.runtime.DevModeDisabledAuthorizationController; @@ -527,6 +528,7 @@ void gatherSecurityChecks(BuildProducer syntheticBeans, BuildProducer configBuilderProducer, List additionalSecuredMethods, SecurityCheckRecorder recorder, + Optional defaultSecurityCheckBuildItem, BuildProducer reflectiveClassBuildItemBuildProducer, List additionalSecurityChecks, SecurityBuildTimeConfig config) { classPredicate.produce(new ApplicationClassPredicateBuildItem(new SecurityCheckStorageAppPredicate())); @@ -559,6 +561,15 @@ void gatherSecurityChecks(BuildProducer syntheticBeans, recorder.addMethod(builder, method.declaringClass().name().toString(), method.name(), params, methodEntry.getValue()); } + + if (defaultSecurityCheckBuildItem.isPresent()) { + var roles = defaultSecurityCheckBuildItem.get().getRolesAllowed(); + if (roles == null) { + recorder.registerDefaultSecurityCheck(builder, recorder.denyAll()); + } else { + recorder.registerDefaultSecurityCheck(builder, recorder.rolesAllowed(roles.toArray(new String[0]))); + } + } recorder.create(builder); syntheticBeans.produce( diff --git a/extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/SecurityCheckStorage.java b/extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/SecurityCheckStorage.java index 3cfeaa2b44b80..2d72f5f8dc2e2 100644 --- a/extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/SecurityCheckStorage.java +++ b/extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/SecurityCheckStorage.java @@ -10,4 +10,9 @@ default SecurityCheck getSecurityCheck(Method method) { SecurityCheck getSecurityCheck(MethodDescription methodDescription); + /** + * {@link SecurityCheck} that should be applied when there is no other check applied on incoming request. + */ + SecurityCheck getDefaultSecurityCheck(); + } diff --git a/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/SecurityCheckRecorder.java b/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/SecurityCheckRecorder.java index 8661d06f3f4bd..545b3249cd9b0 100644 --- a/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/SecurityCheckRecorder.java +++ b/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/SecurityCheckRecorder.java @@ -351,4 +351,8 @@ private Class loadClass(String className) { throw new RuntimeException("Unable to load class '" + className + "' for creating permission", e); } } + + public void registerDefaultSecurityCheck(RuntimeValue builder, SecurityCheck securityCheck) { + builder.getValue().registerDefaultSecurityCheck(securityCheck); + } } diff --git a/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityCheckStorageBuilder.java b/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityCheckStorageBuilder.java index 6be2f134e0057..25cecdb398e78 100644 --- a/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityCheckStorageBuilder.java +++ b/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityCheckStorageBuilder.java @@ -9,6 +9,7 @@ public class SecurityCheckStorageBuilder { private final Map securityChecks = new HashMap<>(); + private SecurityCheck defaultSecurityCheck; public void registerCheck(String className, String methodName, @@ -17,12 +18,24 @@ public void registerCheck(String className, securityChecks.put(new MethodDescription(className, methodName, parameterTypes), securityCheck); } + public void registerDefaultSecurityCheck(SecurityCheck defaultSecurityCheck) { + if (this.defaultSecurityCheck != null) { + throw new IllegalStateException("Default SecurityCheck has already been registered"); + } + this.defaultSecurityCheck = defaultSecurityCheck; + } + public SecurityCheckStorage create() { return new SecurityCheckStorage() { @Override public SecurityCheck getSecurityCheck(MethodDescription methodDescription) { return securityChecks.get(methodDescription); } + + @Override + public SecurityCheck getDefaultSecurityCheck() { + return defaultSecurityCheck; + } }; } } diff --git a/extensions/security/spi/src/main/java/io/quarkus/security/spi/DefaultSecurityCheckBuildItem.java b/extensions/security/spi/src/main/java/io/quarkus/security/spi/DefaultSecurityCheckBuildItem.java new file mode 100644 index 0000000000000..ed3dafe18de0d --- /dev/null +++ b/extensions/security/spi/src/main/java/io/quarkus/security/spi/DefaultSecurityCheckBuildItem.java @@ -0,0 +1,28 @@ +package io.quarkus.security.spi; + +import java.util.List; +import java.util.Objects; + +import io.quarkus.builder.item.SimpleBuildItem; + +public final class DefaultSecurityCheckBuildItem extends SimpleBuildItem { + + public final List rolesAllowed; + + private DefaultSecurityCheckBuildItem(List rolesAllowed) { + this.rolesAllowed = rolesAllowed; + } + + public static DefaultSecurityCheckBuildItem denyAll() { + return new DefaultSecurityCheckBuildItem(null); + } + + public static DefaultSecurityCheckBuildItem rolesAllowed(List rolesAllowed) { + Objects.requireNonNull(rolesAllowed); + return new DefaultSecurityCheckBuildItem(List.copyOf(rolesAllowed)); + } + + public List getRolesAllowed() { + return rolesAllowed; + } +} diff --git a/independent-projects/resteasy-reactive/common/processor/src/main/java/org/jboss/resteasy/reactive/common/processor/EndpointIndexer.java b/independent-projects/resteasy-reactive/common/processor/src/main/java/org/jboss/resteasy/reactive/common/processor/EndpointIndexer.java index 41f8bcaa557ef..a16267eab7b54 100644 --- a/independent-projects/resteasy-reactive/common/processor/src/main/java/org/jboss/resteasy/reactive/common/processor/EndpointIndexer.java +++ b/independent-projects/resteasy-reactive/common/processor/src/main/java/org/jboss/resteasy/reactive/common/processor/EndpointIndexer.java @@ -422,25 +422,6 @@ protected List createEndpoints(ClassInfo currentClassInfo, return ret; } - /** - * Return endpoints defined directly on classInfo. - * - * @param classInfo resource class - * @return classInfo endpoint method info - */ - public static Collection collectClassEndpoints(ClassInfo classInfo, - Map httpAnnotationToMethod, IndexView index, ApplicationScanningResult applicationScanningResult) { - Collection endpoints = collectEndpoints(classInfo, classInfo, new HashSet<>(), new HashSet<>(), true, - httpAnnotationToMethod, index, applicationScanningResult, new AnnotationStore(null)); - Collection ret = new HashSet<>(); - for (FoundEndpoint endpoint : endpoints) { - if (endpoint.classInfo.equals(classInfo)) { - ret.add(endpoint.methodInfo); - } - } - return ret; - } - private static List collectEndpoints(ClassInfo currentClassInfo, ClassInfo actualEndpointInfo, Set seenMethods, Set existingClassNameBindings, boolean considerApplication, Map httpAnnotationToMethod, IndexView index, ApplicationScanningResult applicationScanningResult,