Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid "Failed to index" warnings produced during @SecureField annotation detection #40448

Merged
merged 1 commit into from
May 10, 2024
Merged

Avoid "Failed to index" warnings produced during @SecureField annotation detection #40448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions docs/src/main/asciidoc/rest.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1458,8 +1458,12 @@
----
package org.acme.rest;

import static jakarta.ws.rs.core.MediaType.APPLICATION_JSON;

import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.core.Response;

@Path("person")
public class Person {
Expand All @@ -1469,8 +1473,25 @@
public Person getPerson(Long id) {
return new Person(id, "foo", "bar", "Brick Lane");
}

@Produces(APPLICATION_JSON) <1>
@Path("/friend/{id}")
@GET
public Response getPersonFriend(Long id) {
var person = new Person(id, "foo", "bar", "Brick Lane");
return Response.ok(person).build();
}
}
----
<1> The `@SecureField` annotation is only effective when Quarkus recognizes that produced content type is the 'application/json' type.

WARNING: Currently you cannot use the `@SecureField` annotation to secure your data returned from resource methods returning the `io.smallrye.mutiny.Multi` reactive type.

[IMPORTANT]
====
All resource methods returning data secured with the `@SecureField` annotation should be tested.

Check warning on line 1492 in docs/src/main/asciidoc/rest.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale 

[vale] reported by reviewdog 🐶
[Quarkus.TermsWarnings] Consider using 'verify' rather than 'make sure' unless updating existing content that uses the term.

Raw Output:
{"message": "[Quarkus.TermsWarnings] Consider using 'verify' rather than 'make sure' unless updating existing content that uses the term.", "location": {"path": "docs/src/main/asciidoc/rest.adoc", "range": {"start": {"line": 1492, "column": 94}}}, "severity": "WARNING"}
Please make sure data are secured as you intended.
====

Assuming security has been set up for the application (see our xref:security-overview.adoc[guide] for more details), when a user with the `admin` role
performs an HTTP GET on `/person/1` they will receive:
Expand Down
39 changes: 33 additions & 6 deletions ...rkus/resteasy/reactive/jackson/deployment/processor/ResteasyReactiveJacksonProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.quarkus.resteasy.reactive.jackson.deployment.processor;

import static io.quarkus.resteasy.reactive.common.deployment.QuarkusResteasyReactiveDotNames.JSON_IGNORE;
import static io.quarkus.security.spi.RolesAllowedConfigExpResolverBuildItem.isSecurityConfigExpressionCandidate;
import static org.jboss.resteasy.reactive.common.util.RestMediaType.APPLICATION_NDJSON;
import static org.jboss.resteasy.reactive.common.util.RestMediaType.APPLICATION_STREAM_JSON;
Expand All @@ -15,6 +16,7 @@
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.BiConsumer;
import java.util.function.Predicate;
import java.util.function.Supplier;

import jakarta.inject.Singleton;
Expand Down Expand Up @@ -59,6 +61,7 @@
import io.quarkus.deployment.builditem.ShutdownContextBuildItem;
import io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem;
import io.quarkus.resteasy.reactive.common.deployment.JaxRsResourceIndexBuildItem;
import io.quarkus.resteasy.reactive.common.deployment.QuarkusResteasyReactiveDotNames;
import io.quarkus.resteasy.reactive.common.deployment.ResourceScanningResultBuildItem;
import io.quarkus.resteasy.reactive.common.deployment.ServerDefaultProducesHandlerBuildItem;
import io.quarkus.resteasy.reactive.jackson.CustomDeserialization;
Expand Down Expand Up @@ -372,7 +375,12 @@ public void handleFieldSecurity(ResteasyReactiveResourceMethodEntriesBuildItem r
JaxRsResourceIndexBuildItem index,
BuildProducer<ResourceMethodCustomSerializationBuildItem> producer) {
IndexView indexView = index.getIndexView();
Map<String, Boolean> typeToHasSecureField = new HashMap<>();
boolean noSecureFieldDetected = indexView.getAnnotations(SECURE_FIELD).isEmpty();
if (noSecureFieldDetected) {
return;
}

Map<String, Boolean> typeToHasSecureField = new HashMap<>(getTypesWithSecureField());
List<ResourceMethodCustomSerializationBuildItem> result = new ArrayList<>();
for (ResteasyReactiveResourceMethodEntriesBuildItem.Entry entry : resourceMethodEntries.getEntries()) {
MethodInfo methodInfo = entry.getMethodInfo();
Expand Down Expand Up @@ -425,7 +433,7 @@ public void handleFieldSecurity(ResteasyReactiveResourceMethodEntriesBuildItem r
}

ClassInfo effectiveReturnClassInfo = indexView.getClassByName(effectiveReturnType.name());
if ((effectiveReturnClassInfo == null) || effectiveReturnClassInfo.name().equals(ResteasyReactiveDotNames.OBJECT)) {
if (effectiveReturnClassInfo == null) {
continue;
}
AtomicBoolean needToDeleteCache = new AtomicBoolean(false);
Expand All @@ -443,6 +451,7 @@ public void handleFieldSecurity(ResteasyReactiveResourceMethodEntriesBuildItem r
}
if (needToDeleteCache.get()) {
typeToHasSecureField.clear();
typeToHasSecureField.putAll(getTypesWithSecureField());
}
}
if (!result.isEmpty()) {
Expand All @@ -452,6 +461,13 @@ public void handleFieldSecurity(ResteasyReactiveResourceMethodEntriesBuildItem r
}
}

private static Map<String, Boolean> getTypesWithSecureField() {
// if any of following types is detected as an endpoint return type or a field of endpoint return type,
// we always need to apply security serialization as any type can be represented with them
return Map.of(ResteasyReactiveDotNames.OBJECT.toString(), Boolean.TRUE, ResteasyReactiveDotNames.RESPONSE.toString(),
Boolean.TRUE);
}

private static boolean hasSecureFields(IndexView indexView, ClassInfo currentClassInfo,
Map<String, Boolean> typeToHasSecureField, AtomicBoolean needToDeleteCache) {
// use cached result if there is any
Expand Down Expand Up @@ -479,10 +495,20 @@ private static boolean hasSecureFields(IndexView indexView, ClassInfo currentCla
.anyMatch(ci -> hasSecureFields(indexView, ci, typeToHasSecureField, needToDeleteCache));
} else {
// figure if any field or parent / subclass field is secured
hasSecureFields = hasSecureFields(currentClassInfo)
|| anyFieldHasSecureFields(indexView, currentClassInfo, typeToHasSecureField, needToDeleteCache)
|| anySubclassHasSecureFields(indexView, currentClassInfo, typeToHasSecureField, needToDeleteCache)
|| anyParentClassHasSecureFields(indexView, currentClassInfo, typeToHasSecureField, needToDeleteCache);
if (hasSecureFields(currentClassInfo)) {
hasSecureFields = true;
} else {
Predicate<DotName> ignoredTypesPredicate = QuarkusResteasyReactiveDotNames.IGNORE_TYPE_FOR_REFLECTION_PREDICATE;
if (ignoredTypesPredicate.test(currentClassInfo.name())) {
hasSecureFields = false;
} else {
hasSecureFields = anyFieldHasSecureFields(indexView, currentClassInfo, typeToHasSecureField,
needToDeleteCache)
|| anySubclassHasSecureFields(indexView, currentClassInfo, typeToHasSecureField, needToDeleteCache)
|| anyParentClassHasSecureFields(indexView, currentClassInfo, typeToHasSecureField,
needToDeleteCache);
}
}
}
typeToHasSecureField.put(className, hasSecureFields);
return hasSecureFields;
Expand Down Expand Up @@ -513,6 +539,7 @@ private static boolean anyFieldHasSecureFields(IndexView indexView, ClassInfo cu
return currentClassInfo
.fields()
.stream()
.filter(fieldInfo -> !fieldInfo.hasAnnotation(JSON_IGNORE))
.map(FieldInfo::type)
.anyMatch(fieldType -> fieldTypeHasSecureFields(fieldType, indexView, typeToHasSecureField, needToDeleteCache));
}
Expand Down
3 changes: 3 additions & 0 deletions ...src/test/java/io/quarkus/resteasy/reactive/jackson/deployment/test/MultipartResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
import org.jboss.resteasy.reactive.RestForm;
import org.jboss.resteasy.reactive.multipart.FileUpload;

import io.quarkus.resteasy.reactive.jackson.DisableSecureSerialization;
import io.smallrye.common.annotation.Blocking;

@Path("/multipart")
public class MultipartResource {

@DisableSecureSerialization // Person has @SecureField but we want to inspect all data
@POST
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.MULTIPART_FORM_DATA)
Expand All @@ -45,6 +47,7 @@ public Map<String, Object> greeting(@Valid @BeanParam FormData formData) {
return result;
}

@DisableSecureSerialization // Person has @SecureField but we want to inspect all data
@POST
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.MULTIPART_FORM_DATA)
Expand Down
57 changes: 57 additions & 0 deletions ...ment/src/test/java/io/quarkus/resteasy/reactive/jackson/deployment/test/ResponseType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package io.quarkus.resteasy.reactive.jackson.deployment.test;

import jakarta.ws.rs.core.Response;

import org.jboss.resteasy.reactive.RestResponse;

import io.quarkus.resteasy.reactive.jackson.SecureField;
import io.smallrye.mutiny.Multi;
import io.smallrye.mutiny.Uni;

public enum ResponseType {
/**
* Returns DTOs directly.
*/
PLAIN(true, "plain"),
/**
* Returns {@link Multi} with DTOs.
*/
// TODO: enable when https://github.com/quarkusio/quarkus/issues/40447 gets fixed
//MULTI(true, "multi"),
/**
* Returns {@link Uni} with DTOs.
*/
UNI(true, "uni"),
/**
* Returns {@link Object} that is either DTO with a {@link SecureField} or not.
*/
OBJECT(false, "object"), // we must always assume it can contain SecureField
/**
* Returns {@link Response} that is either DTO with a {@link SecureField} or not.
*/
RESPONSE(false, "response"), // we must always assume it can contain SecureField
/**
* Returns {@link RestResponse} with DTOs.
*/
REST_RESPONSE(true, "rest-response"),
/**
* Returns {@link RestResponse} with DTOs.
*/
COLLECTION(true, "collection");

private final boolean secureFieldDetectable;
private final String resourceSubPath;

ResponseType(boolean secureFieldDetectable, String resourceSubPath) {
this.secureFieldDetectable = secureFieldDetectable;
this.resourceSubPath = resourceSubPath;
}

boolean isSecureFieldDetectable() {
return secureFieldDetectable;
}

String getResourceSubPath() {
return resourceSubPath;
}
}
Loading
Loading