Skip to content

Commit

Permalink
Fix JAX-RS default security checks for inherited / transformed endpoints
Browse files Browse the repository at this point in the history
(cherry picked from commit 6f3d752d157cc8eb22cc86c521cd6f029f67f42f)
  • Loading branch information
michalvavrik authored and gsmet committed Jan 26, 2024
1 parent a180a8f commit 467cc9c
Show file tree
Hide file tree
Showing 24 changed files with 339 additions and 89 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ static Optional<AnnotationInstance> searchPathAnnotationOnInterfaces(CombinedInd
* @param resultAcc accumulator for tail-recursion
* @return Collection of all interfaces und their parents. Never null.
*/
private static Collection<ClassInfo> getAllClassInterfaces(
static Collection<ClassInfo> getAllClassInterfaces(
CombinedIndexBuildItem index,
Collection<ClassInfo> classInfos,
List<ClassInfo> resultAcc) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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<ClassInfo> interfaces = getAllClassInterfaces(index, List.of(classInfo), new ArrayList<>());
if (!interfaces.isEmpty()) {
final List<MethodInfo> 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<DotName> 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");
}
}
}
}
Expand All @@ -85,6 +122,27 @@ void setUpDenyAllJaxRs(CombinedIndexBuildItem index,
}
}

private static void addAllUnannotatedEndpoints(CombinedIndexBuildItem index, ClassInfo classInfo,
List<MethodInfo> 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<MethodInfo> methods) {
if (!hasSecurityAnnotation(classInfo)) {
for (MethodInfo methodInfo : classInfo.methods()) {
if (isRestEndpointMethod(index, methodInfo) && !hasSecurityAnnotation(methodInfo)) {
methods.add(methodInfo);
}
}
}
}

/**
* Install the JAX-RS security provider.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand All @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
@@ -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";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,18 @@
* @author Michal Szynkiewicz, [email protected]
*/
@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
Expand Down
Original file line number Diff line number Diff line change
@@ -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";
}

}
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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 {

Expand Down Expand Up @@ -134,46 +130,13 @@ void searchForProviders(Capabilities capabilities,
}

@BuildStep
void setUpDenyAllJaxRs(
CombinedIndexBuildItem index,
JaxRsSecurityConfig securityConfig,
Optional<ResourceScanningResultBuildItem> resteasyDeployment,
BeanArchiveIndexBuildItem beanArchiveIndexBuildItem,
ApplicationResultBuildItem applicationResultBuildItem,
BuildProducer<AdditionalSecuredMethodsBuildItem> additionalSecuredClasses) {

if (resteasyDeployment.isPresent()
&& (securityConfig.denyJaxRs() || securityConfig.defaultRolesAllowed().isPresent())) {
final List<MethodInfo> methods = new ArrayList<>();
Map<DotName, String> httpAnnotationToMethod = resteasyDeployment.get().getResult().getHttpAnnotationToMethod();
Set<DotName> 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<MethodInfo> 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<DefaultSecurityCheckBuildItem> defaultSecurityCheckProducer) {
if (securityConfig.denyJaxRs()) {
defaultSecurityCheckProducer.produce(DefaultSecurityCheckBuildItem.denyAll());
} else if (securityConfig.defaultRolesAllowed().isPresent()) {
defaultSecurityCheckProducer
.produce(DefaultSecurityCheckBuildItem.rolesAllowed(securityConfig.defaultRolesAllowed().get()));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));

Expand All @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down
Loading

0 comments on commit 467cc9c

Please sign in to comment.