From 0392b1060f122285a767e14afcb0dc9d1933cd24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Vav=C5=99=C3=ADk?= Date: Wed, 10 Aug 2022 11:07:32 +0200 Subject: [PATCH] RESTEasy Reactive - Prevent repeated security checks closes: #26536 --- .../deployment/ResteasyReactiveProcessor.java | 12 +++ .../security/RolesAllowedJaxRsTestCase.java | 15 +++- .../test/security/RolesAllowedService.java | 16 ++++ .../security/RolesAllowedServiceResource.java | 21 +++++ .../StandardSecurityCheckInterceptor.java | 80 +++++++++++++++++++ .../security/EagerSecurityHandler.java | 14 +++- .../spi/runtime/SecurityHandlerConstants.java | 15 ++++ .../runtime/interceptor/SecurityHandler.java | 8 +- 8 files changed, 172 insertions(+), 9 deletions(-) create mode 100644 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedService.java create mode 100644 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedServiceResource.java create mode 100644 extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/StandardSecurityCheckInterceptor.java create mode 100644 extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/SecurityHandlerConstants.java diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/main/java/io/quarkus/resteasy/reactive/server/deployment/ResteasyReactiveProcessor.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/main/java/io/quarkus/resteasy/reactive/server/deployment/ResteasyReactiveProcessor.java index 26d9d40125420b..531d8a8fded239 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/main/java/io/quarkus/resteasy/reactive/server/deployment/ResteasyReactiveProcessor.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/main/java/io/quarkus/resteasy/reactive/server/deployment/ResteasyReactiveProcessor.java @@ -143,6 +143,7 @@ import io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveRecorder; import io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveRuntimeRecorder; import io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveServerRuntimeConfig; +import io.quarkus.resteasy.reactive.server.runtime.StandardSecurityCheckInterceptor; import io.quarkus.resteasy.reactive.server.runtime.exceptionmappers.AuthenticationCompletionExceptionMapper; import io.quarkus.resteasy.reactive.server.runtime.exceptionmappers.AuthenticationFailedExceptionMapper; import io.quarkus.resteasy.reactive.server.runtime.exceptionmappers.AuthenticationRedirectExceptionMapper; @@ -1131,6 +1132,17 @@ public void visit(int version, int access, String name, String signature, } + @BuildStep + void registerSecurityInterceptors(Capabilities capabilities, + BuildProducer beans) { + if (capabilities.isPresent(Capability.SECURITY)) { + // Register interceptors for standard security annotations to prevent repeated security checks + beans.produce(new AdditionalBeanBuildItem(StandardSecurityCheckInterceptor.RolesAllowedInterceptor.class, + StandardSecurityCheckInterceptor.AuthenticatedInterceptor.class, + StandardSecurityCheckInterceptor.PermitAllInterceptor.class)); + } + } + private T consumeStandardSecurityAnnotations(MethodInfo methodInfo, ClassInfo classInfo, IndexView index, Function function) { if (SecurityTransformerUtils.hasStandardSecurityAnnotation(methodInfo)) { diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedJaxRsTestCase.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedJaxRsTestCase.java index c4f23ed4397f05..5621fb9fe90677 100644 --- a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedJaxRsTestCase.java +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedJaxRsTestCase.java @@ -29,10 +29,9 @@ public class RolesAllowedJaxRsTestCase { static QuarkusUnitTest runner = new QuarkusUnitTest() .withApplicationRoot((jar) -> jar .addClasses(RolesAllowedResource.class, UserResource.class, RolesAllowedBlockingResource.class, - SerializationEntity.class, SerializationRolesResource.class, - TestIdentityProvider.class, - TestIdentityController.class, - UnsecuredSubResource.class)); + SerializationEntity.class, SerializationRolesResource.class, TestIdentityProvider.class, + TestIdentityController.class, UnsecuredSubResource.class, RolesAllowedService.class, + RolesAllowedServiceResource.class)); @BeforeAll public static void setupUsers() { @@ -61,6 +60,14 @@ public void testUser() { RestAssured.given().auth().preemptive().basic("user", "user").get("/user").then().body(is("user")); } + @Test + public void testNestedRolesAllowed() { + // there are 2 different checks in place: user & admin on resource, admin on service + RestAssured.given().auth().basic("admin", "admin").get("/roles-service/hello").then().statusCode(200) + .body(is(RolesAllowedService.SERVICE_HELLO)); + RestAssured.given().auth().basic("user", "user").get("/roles-service/hello").then().statusCode(403); + } + @Test public void testSecurityRunsBeforeValidation() { read = false; diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedService.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedService.java new file mode 100644 index 00000000000000..716baa62472d0d --- /dev/null +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedService.java @@ -0,0 +1,16 @@ +package io.quarkus.resteasy.reactive.server.test.security; + +import javax.annotation.security.RolesAllowed; +import javax.enterprise.context.ApplicationScoped; + +@ApplicationScoped +public class RolesAllowedService { + + public static final String SERVICE_HELLO = "Hello from Service!"; + + @RolesAllowed("admin") + public String hello() { + return SERVICE_HELLO; + } + +} diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedServiceResource.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedServiceResource.java new file mode 100644 index 00000000000000..b67faaa6b9317a --- /dev/null +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/RolesAllowedServiceResource.java @@ -0,0 +1,21 @@ +package io.quarkus.resteasy.reactive.server.test.security; + +import javax.annotation.security.RolesAllowed; +import javax.inject.Inject; +import javax.ws.rs.GET; +import javax.ws.rs.Path; + +@Path("/roles-service") +public class RolesAllowedServiceResource { + + @Inject + RolesAllowedService rolesAllowedService; + + @Path("/hello") + @RolesAllowed({ "user", "admin" }) + @GET + public String getServiceHello() { + return rolesAllowedService.hello(); + } + +} diff --git a/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/StandardSecurityCheckInterceptor.java b/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/StandardSecurityCheckInterceptor.java new file mode 100644 index 00000000000000..6b94b33b4a1269 --- /dev/null +++ b/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/StandardSecurityCheckInterceptor.java @@ -0,0 +1,80 @@ +package io.quarkus.resteasy.reactive.server.runtime; + +import static io.quarkus.security.spi.runtime.SecurityHandlerConstants.EXECUTED; +import static io.quarkus.security.spi.runtime.SecurityHandlerConstants.SECURITY_HANDLER; + +import java.lang.reflect.Method; + +import javax.annotation.Priority; +import javax.annotation.security.DenyAll; +import javax.annotation.security.PermitAll; +import javax.annotation.security.RolesAllowed; +import javax.inject.Inject; +import javax.interceptor.AroundInvoke; +import javax.interceptor.Interceptor; +import javax.interceptor.InvocationContext; + +import org.jboss.resteasy.reactive.server.core.CurrentRequestManager; + +import io.quarkus.security.Authenticated; +import io.quarkus.security.spi.runtime.AuthorizationController; +import io.quarkus.security.spi.runtime.MethodDescription; + +/** + * Security checks for RBAC annotations on endpoints are done by + * the {@link io.quarkus.resteasy.reactive.server.runtime.security.EagerSecurityHandler}, + * this interceptor propagates the information to the SecurityHandler to prevent repeated checks. The {@link DenyAll} + * security check is performed just once. + */ +public abstract class StandardSecurityCheckInterceptor { + + public static final String STANDARD_SECURITY_CHECK_INTERCEPTOR = StandardSecurityCheckInterceptor.class.getName(); + + @Inject + AuthorizationController controller; + + @AroundInvoke + public Object intercept(InvocationContext ic) throws Exception { + if (controller.isAuthorizationEnabled() && CurrentRequestManager.get() != null + && alreadyDoneByEagerSecurityHandler( + CurrentRequestManager.get().getProperty(STANDARD_SECURITY_CHECK_INTERCEPTOR), ic.getMethod())) { + ic.getContextData().put(SECURITY_HANDLER, EXECUTED); + } + return ic.proceed(); + } + + private boolean alreadyDoneByEagerSecurityHandler(Object methodWithFinishedChecks, Method method) { + // compare methods: EagerSecurityHandler only intercept endpoints, we still want SecurityHandler run for CDI beans + return methodWithFinishedChecks != null && MethodDescription.ofMethod(method).equals(methodWithFinishedChecks); + } + + /** + * Prevent the SecurityHandler from performing {@link RolesAllowed} security checks + */ + @Interceptor + @RolesAllowed("") + @Priority(Interceptor.Priority.PLATFORM_BEFORE) + public static final class RolesAllowedInterceptor extends StandardSecurityCheckInterceptor { + + } + + /** + * Prevent the SecurityHandler from performing {@link javax.annotation.security.PermitAll} security checks + */ + @Interceptor + @PermitAll + @Priority(Interceptor.Priority.PLATFORM_BEFORE) + public static final class PermitAllInterceptor extends StandardSecurityCheckInterceptor { + + } + + /** + * Prevent the SecurityHandler from performing {@link Authenticated} security checks + */ + @Interceptor + @Authenticated + @Priority(Interceptor.Priority.PLATFORM_BEFORE) + public static final class AuthenticatedInterceptor extends StandardSecurityCheckInterceptor { + + } +} 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 6105e74315671f..2da1c299374659 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 @@ -1,5 +1,7 @@ package io.quarkus.resteasy.reactive.server.runtime.security; +import static io.quarkus.resteasy.reactive.server.runtime.StandardSecurityCheckInterceptor.STANDARD_SECURITY_CHECK_INTERCEPTOR; + import java.lang.reflect.Method; import java.util.Collections; import java.util.List; @@ -75,7 +77,9 @@ public void handle(ResteasyReactiveRequestContext requestContext) throws Excepti requestContext.requireCDIRequestScope(); SecurityCheck theCheck = check; - if (!theCheck.isPermitAll()) { + if (theCheck.isPermitAll()) { + preventRepeatedSecurityChecks(requestContext, methodDescription); + } else { requestContext.suspend(); Uni deferredIdentity = getCurrentIdentityAssociation().get().getDeferredIdentity(); @@ -95,6 +99,7 @@ public void handle(ResteasyReactiveRequestContext requestContext) throws Excepti public Object apply(SecurityIdentity securityIdentity) { theCheck.apply(securityIdentity, methodDescription, requestContext.getParameters()); + preventRepeatedSecurityChecks(requestContext, methodDescription); return null; } }) @@ -117,6 +122,13 @@ public void onFailure(Throwable failure) { } } + private void preventRepeatedSecurityChecks(ResteasyReactiveRequestContext requestContext, + MethodDescription methodDescription) { + // propagate information that security check has been performed on this method to the SecurityHandler + // via io.quarkus.resteasy.reactive.server.runtime.StandardSecurityCheckInterceptor + requestContext.setProperty(STANDARD_SECURITY_CHECK_INTERCEPTOR, methodDescription); + } + private InjectableInstance getCurrentIdentityAssociation() { InjectableInstance identityAssociation = this.currentIdentityAssociation; if (identityAssociation == null) { diff --git a/extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/SecurityHandlerConstants.java b/extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/SecurityHandlerConstants.java new file mode 100644 index 00000000000000..72a84b34e92116 --- /dev/null +++ b/extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/SecurityHandlerConstants.java @@ -0,0 +1,15 @@ +package io.quarkus.security.spi.runtime; + +public class SecurityHandlerConstants { + + /** + * Invocation context data key used by the SecurityHandler to save a security checks state + */ + public static final String SECURITY_HANDLER = "io.quarkus.security.securityHandler"; + + /** + * The SecurityHandler keep a state of security checks in the Invocation context data to prevent repeated checks. + * `executed` means the check has already been done. + */ + public static final String EXECUTED = "executed"; +} diff --git a/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityHandler.java b/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityHandler.java index ab0153df22bc39..6c960be600c191 100644 --- a/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityHandler.java +++ b/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityHandler.java @@ -1,5 +1,8 @@ package io.quarkus.security.runtime.interceptor; +import static io.quarkus.security.spi.runtime.SecurityHandlerConstants.EXECUTED; +import static io.quarkus.security.spi.runtime.SecurityHandlerConstants.SECURITY_HANDLER; + import java.util.concurrent.CompletionStage; import java.util.function.Function; @@ -16,9 +19,6 @@ @Singleton public class SecurityHandler { - private static final String HANDLER_NAME = SecurityHandler.class.getName(); - private static final String EXECUTED = "executed"; - @Inject SecurityConstrainer constrainer; @@ -49,7 +49,7 @@ public Object handle(InvocationContext ic) throws Exception { } private boolean alreadyHandled(InvocationContext ic) { - return ic.getContextData().put(HANDLER_NAME, EXECUTED) != null; + return ic.getContextData().put(SECURITY_HANDLER, EXECUTED) != null; } private static class UniContinuation implements Function> {