Skip to content

Commit

Permalink
[2.13] Perform security checks eagerly in RR on inherited endpoints
Browse files Browse the repository at this point in the history
  • Loading branch information
michalvavrik committed Feb 25, 2024
1 parent bc0ca28 commit 2eb4659
Show file tree
Hide file tree
Showing 12 changed files with 160 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ public class DefaultRolesAllowedJaxRsTest {
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=admin\n"),
"application.properties"));
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ public class DenyAllJaxRsTest {
static QuarkusUnitTest runner = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(PermitAllResource.class, UnsecuredResource.class,
TestIdentityProvider.class,
TestIdentityController.class,
UnsecuredSubResource.class)
TestIdentityProvider.class, TestIdentityController.class, UnsecuredSubResource.class,
UnsecuredParentResource.class, UnsecuredResourceInterface.class)
.addAsResource(new StringAsset("quarkus.security.jaxrs.deny-unannotated-endpoints = true\n"),
"application.properties"));

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package io.quarkus.resteasy.reactive.server.test.security;

import static io.restassured.RestAssured.given;
import static io.restassured.RestAssured.when;
import static org.hamcrest.Matchers.is;

import java.io.IOException;
Expand All @@ -14,6 +16,7 @@
import javax.ws.rs.ext.MessageBodyReader;
import javax.ws.rs.ext.Provider;

import org.hamcrest.Matchers;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
Expand All @@ -31,7 +34,8 @@ public class RolesAllowedJaxRsTestCase {
.addClasses(RolesAllowedResource.class, UserResource.class, RolesAllowedBlockingResource.class,
SerializationEntity.class, SerializationRolesResource.class, TestIdentityProvider.class,
TestIdentityController.class, UnsecuredSubResource.class, RolesAllowedService.class,
RolesAllowedServiceResource.class));
RolesAllowedServiceResource.class, UnsecuredResource.class, UnsecuredParentResource.class,
UnsecuredResourceInterface.class));

@BeforeAll
public static void setupUsers() {
Expand Down Expand Up @@ -90,6 +94,53 @@ public void testSecurityRunsBeforeValidation() {
Assertions.assertFalse(read);
}

@Test
public void shouldAllowAnnotatedParentEndpoint() {
// the endpoint has @RolesAllowed, therefore default JAX-RS security should not be applied
String path = "/unsecured/parent-annotated";
assertStatus(path, 200, 200, 401);
}

@Test
public void shouldAllowAnnotatedEndpointOnInterface() {
// the endpoint has @RolesAllowed, therefore default JAX-RS security should not be applied
String path = "/unsecured/interface-annotated";
assertStatus(path, 200, 200, 401);
}

@Test
public void shouldDenyUnannotatedOverriddenOnInterfaceImplementor() {
// @RolesAllowed on interface, however implementor overridden the endpoint method with @Path @GET
String path = "/unsecured/interface-overridden-declared-on-implementor";
assertStatus(path, 200, 200, 200);
}

@Test
public void shouldAllowAnnotatedOverriddenEndpointDeclaredOnInterface() {
// @RolesAllowed on interface and implementor didn't declare endpoint declaring annotations @GET
String path = "/unsecured/interface-overridden-declared-on-interface";
assertStatus(path, 200, 200, 401);
// check that response comes from the overridden method
given().auth().preemptive()
.basic("admin", "admin").get(path)
.then()
.body(Matchers.is("implementor-response"));
}

private void assertStatus(String path, int adminStatus, int userStatus, int anonStatus) {
given().auth().preemptive()
.basic("admin", "admin").get(path)
.then()
.statusCode(adminStatus);
given().auth().preemptive()
.basic("user", "user").get(path)
.then()
.statusCode(userStatus);
when().get(path)
.then()
.statusCode(anonStatus);
}

static volatile boolean read = false;

@Provider
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package io.quarkus.resteasy.reactive.server.test.security;

import javax.annotation.security.RolesAllowed;
import javax.ws.rs.GET;
import javax.ws.rs.Path;

public class UnsecuredParentResource {

@Path("/defaultSecurityParent")
@GET
public String defaultSecurityParent() {
return "defaultSecurityParent";
}

@RolesAllowed({ "admin", "user" })
@GET
@Path("/parent-annotated")
public String parentAnnotated() {
return "parent-annotated";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* @author Michal Szynkiewicz, [email protected]
*/
@Path("/unsecured")
public class UnsecuredResource {
public class UnsecuredResource extends UnsecuredParentResource implements UnsecuredResourceInterface {
@Path("/defaultSecurity")
@GET
public String defaultSecurity() {
Expand Down Expand Up @@ -49,4 +49,16 @@ public UnsecuredSubResource sub() {
public UnsecuredSubResource permitAllSub() {
return new UnsecuredSubResource();
}

@Override
public String interfaceOverriddenDeclaredOnInterface() {
return "implementor-response";
}

@GET
@Path("/interface-overridden-declared-on-implementor")
@Override
public String interfaceOverriddenDeclaredOnImplementor() {
return "implementor-response";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package io.quarkus.resteasy.reactive.server.test.security;

import javax.annotation.security.RolesAllowed;
import javax.ws.rs.GET;
import javax.ws.rs.Path;

public interface UnsecuredResourceInterface {

@Path("/defaultSecurityInterface")
@GET
default String defaultSecurityInterface() {
return "defaultSecurityInterface";
}

@RolesAllowed({ "admin", "user" })
@GET
@Path("/interface-annotated")
default String interfaceAnnotated() {
return "interface-annotated";
}

@RolesAllowed({ "admin", "user" })
@GET
@Path("/interface-overridden-declared-on-interface")
default String interfaceOverriddenDeclaredOnInterface() {
// this interface is overridden without @GET and @Path
return "interface-overridden-declared-on-interface";
}

@RolesAllowed({ "admin", "user" })
@GET
@Path("/interface-overridden-declared-on-implementor")
default String interfaceOverriddenDeclaredOnImplementor() {
// this interface is overridden with @GET and @Path
return "interface-overridden-declared-on-implementor";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public void handle(ResteasyReactiveRequestContext requestContext) throws Excepti
}
SecurityCheck check = this.check;
ResteasyReactiveResourceInfo lazyMethod = requestContext.getTarget().getLazyMethod();
MethodDescription methodDescription = new MethodDescription(lazyMethod.getResourceClass().getName(),
MethodDescription methodDescription = new MethodDescription(lazyMethod.getActualDeclaringClassName(),
lazyMethod.getName(), MethodDescription.typesAsStrings(lazyMethod.getParameterTypes()));
if (check == null) {
check = Arc.container().instance(SecurityCheckStorage.class).get().getSecurityCheck(methodDescription);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ protected ServerResourceMethod createResourceMethod(MethodInfo methodInfo, Class
}
}
serverResourceMethod.setHandlerChainCustomizers(methodCustomizers);
serverResourceMethod.setActualDeclaringClassName(methodInfo.declaringClass().name().toString());
return serverResourceMethod;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ public RuntimeResource buildResourceMethod(ResourceClass clazz,

ResteasyReactiveResourceInfo lazyMethod = new ResteasyReactiveResourceInfo(method.getName(), resourceClass,
parameterDeclaredUnresolvedTypes, classAnnotationNames, method.getMethodAnnotationNames(),
!defaultBlocking && !method.isBlocking());
!defaultBlocking && !method.isBlocking(), method.getActualDeclaringClassName());

RuntimeInterceptorDeployment.MethodInterceptorContext interceptorDeployment = runtimeInterceptorDeployment
.forMethod(method, lazyMethod);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public class ServerResourceMethod extends ResourceMethod {

private List<HandlerChainCustomizer> handlerChainCustomizers = new ArrayList<>();
private ParameterExtractor customerParameterExtractor;
private String actualDeclaringClassName;

public ServerResourceMethod() {
}
Expand Down Expand Up @@ -69,4 +70,12 @@ public ServerResourceMethod setCustomerParameterExtractor(ParameterExtractor cus
this.customerParameterExtractor = customerParameterExtractor;
return this;
}

public String getActualDeclaringClassName() {
return actualDeclaringClassName;
}

public void setActualDeclaringClassName(String actualDeclaringClassName) {
this.actualDeclaringClassName = actualDeclaringClassName;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,33 @@ public class ResteasyReactiveResourceInfo implements ResourceInfo {
* If it's non-blocking method within the runtime that won't always default to blocking
*/
public final boolean isNonBlocking;

/**
* This class name will only differ from {@link this#declaringClass} name when the {@link this#method} was inherited.
*/
private final String actualDeclaringClassName;
private volatile Annotation[] classAnnotations;
private volatile Method method;
private volatile Annotation[] annotations;
private volatile Type returnType;
private volatile String methodId;

@Deprecated
public ResteasyReactiveResourceInfo(String name, Class<?> declaringClass, Class[] parameterTypes,
Set<String> classAnnotationNames, Set<String> methodAnnotationNames, boolean isNonBlocking) {
this(name, declaringClass, parameterTypes, classAnnotationNames, methodAnnotationNames, isNonBlocking,
declaringClass.getName());
}

public ResteasyReactiveResourceInfo(String name, Class<?> declaringClass, Class[] parameterTypes,
Set<String> classAnnotationNames, Set<String> methodAnnotationNames, boolean isNonBlocking,
String actualDeclaringClassName) {
this.name = name;
this.declaringClass = declaringClass;
this.parameterTypes = parameterTypes;
this.classAnnotationNames = classAnnotationNames;
this.methodAnnotationNames = methodAnnotationNames;
this.isNonBlocking = isNonBlocking;
this.actualDeclaringClassName = actualDeclaringClassName;
}

public String getName() {
Expand Down Expand Up @@ -119,4 +131,8 @@ public String getMethodId() {
}
return methodId;
}

public String getActualDeclaringClassName() {
return actualDeclaringClassName;
}
}

0 comments on commit 2eb4659

Please sign in to comment.