This is an automated email from the ASF dual-hosted git repository. rzo1 pushed a commit to branch tomee-10.x in repository https://gitbox.apache.org/repos/asf/tomee.git
commit dccb325d3b2b7e402a2da6884ae367653bc83841 Author: Richard Zowalla <[email protected]> AuthorDate: Tue Oct 13 08:47:09 2026 +0200 Resolve MP-JWT security constraints per resource class (#3068) Inherited resource methods no longer share role sets across subclasses, and an empty @RolesAllowed now denies all callers. (cherry picked from commit c89dcc5da35132f732ded44898ae072ef4a3352c) --- .../jaxrs/MPJWTSecurityAnnotationsInterceptor.java | 40 ++-- ...PJWTSecurityAnnotationsInterceptorsFeature.java | 74 ++----- ...SecurityAnnotationsInterceptorsFeatureTest.java | 237 +++++++++++++++++++++ 3 files changed, 280 insertions(+), 71 deletions(-) diff --git a/mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptor.java b/mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptor.java index fadace9557..104110c703 100644 --- a/mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptor.java +++ b/mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptor.java @@ -23,45 +23,49 @@ import jakarta.ws.rs.container.ContainerRequestFilter; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.SecurityContext; import java.io.IOException; -import java.lang.reflect.Method; import java.net.HttpURLConnection; +import java.util.Collections; +import java.util.LinkedHashSet; import java.util.Set; -import java.util.concurrent.ConcurrentMap; +/** + * Enforces the security constraint resolved for a single resource class / resource method pair. + */ public class MPJWTSecurityAnnotationsInterceptor implements ContainerRequestFilter { - private final jakarta.ws.rs.container.ResourceInfo resourceInfo; - private final ConcurrentMap<Method, Set<String>> rolesAllowed; - private final Set<Method> denyAll; - private final Set<Method> permitAll; + private final Set<String> rolesAllowed; + private final boolean denyAll; + private final boolean permitAll; - public MPJWTSecurityAnnotationsInterceptor(final jakarta.ws.rs.container.ResourceInfo resourceInfo, - final ConcurrentMap<Method, Set<String>> rolesAllowed, - final Set<Method> denyAll, - final Set<Method> permitAll) { - this.resourceInfo = resourceInfo; - this.rolesAllowed = rolesAllowed; + /** + * @param rolesAllowed the allowed roles, or {@code null} if no {@code @RolesAllowed} applies; + * an empty set denies every caller + * @param denyAll whether {@code @DenyAll} applies + * @param permitAll whether {@code @PermitAll} applies + */ + public MPJWTSecurityAnnotationsInterceptor(final Set<String> rolesAllowed, + final boolean denyAll, + final boolean permitAll) { + this.rolesAllowed = rolesAllowed == null ? null : Collections.unmodifiableSet(new LinkedHashSet<>(rolesAllowed)); this.denyAll = denyAll; this.permitAll = permitAll; } @Override public void filter(final ContainerRequestContext requestContext) throws IOException { - if (permitAll.contains(resourceInfo.getResourceMethod())) { + if (permitAll) { return; } - if (denyAll.contains(resourceInfo.getResourceMethod())) { + if (denyAll) { forbidden(requestContext); return; } - final Set<String> roles = rolesAllowed.get(resourceInfo.getResourceMethod()); - - if (roles != null && !roles.isEmpty()) { + if (rolesAllowed != null) { final SecurityContext securityContext = requestContext.getSecurityContext(); boolean hasAtLeasOneValidRole = false; - for (String role : roles) { + for (String role : rolesAllowed) { if (securityContext.isUserInRole(role)) { hasAtLeasOneValidRole = true; break; diff --git a/mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptorsFeature.java b/mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptorsFeature.java index 1cfa5e71c4..b4afaef9ff 100644 --- a/mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptorsFeature.java +++ b/mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptorsFeature.java @@ -30,28 +30,25 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; @Provider public class MPJWTSecurityAnnotationsInterceptorsFeature implements DynamicFeature { - private final ConcurrentMap<Method, Set<String>> rolesAllowed = new ConcurrentHashMap<>(); - private final Set<Method> denyAll = new HashSet<>(); - private final Set<Method> permitAll = new HashSet<>(); - @Override public void configure(final ResourceInfo resourceInfo, final FeatureContext context) { - final boolean hasSecurity = processSecurityAnnotations(resourceInfo.getResourceClass(), resourceInfo.getResourceMethod()); + // the constraint is resolved per resource class: an inherited resource method is the same + // java.lang.reflect.Method for every subclass, so it cannot be used as a shared key + final MPJWTSecurityAnnotationsInterceptor interceptor = + processSecurityAnnotations(resourceInfo.getResourceClass(), resourceInfo.getResourceMethod()); - if (hasSecurity) { // no need to add interceptor on the resources that don(t have any security requirements to enforce - context.register(new MPJWTSecurityAnnotationsInterceptor(resourceInfo, rolesAllowed, denyAll, permitAll)); + if (interceptor != null) { // no need to add interceptor on the resources that don(t have any security requirements to enforce + context.register(interceptor); } } - private boolean processSecurityAnnotations(final Class clazz, final Method method) { + private MPJWTSecurityAnnotationsInterceptor processSecurityAnnotations(final Class clazz, final Method method) { final List<Class<? extends Annotation>[]> classSecurityAnnotations = hasClassLevelAnnotations(clazz, RolesAllowed.class, PermitAll.class, DenyAll.class); @@ -60,7 +57,7 @@ public class MPJWTSecurityAnnotationsInterceptorsFeature implements DynamicFeatu RolesAllowed.class, PermitAll.class, DenyAll.class); if (classSecurityAnnotations.isEmpty() && methodSecurityAnnotations.isEmpty()) { - return false; // nothing to do + return null; // nothing to do } /* @@ -74,51 +71,22 @@ public class MPJWTSecurityAnnotationsInterceptorsFeature implements DynamicFeatu throw new IllegalStateException(method.toString() + " has more than one security annotation (RolesAllowed, PermitAll, DenyAll)."); } - if (methodSecurityAnnotations.isEmpty()) { // no need to deal with class level annotations if the method has some - final RolesAllowed classRolesAllowed = (RolesAllowed) clazz.getAnnotation(RolesAllowed.class); - final PermitAll classPermitAll = (PermitAll) clazz.getAnnotation(PermitAll.class); - final DenyAll classDenyAll = (DenyAll) clazz.getAnnotation(DenyAll.class); - - if (classRolesAllowed != null) { - Set<String> roles = new HashSet<>(); - final Set<String> previous = rolesAllowed.putIfAbsent(method, roles); - if (previous != null) { - roles = previous; - } - roles.addAll(Arrays.asList(classRolesAllowed.value())); - } - - if (classPermitAll != null) { - permitAll.add(method); - } - - if (classDenyAll != null) { - denyAll.add(method); - } - } - - final RolesAllowed mthdRolesAllowed = method.getAnnotation(RolesAllowed.class); - final PermitAll mthdPermitAll = method.getAnnotation(PermitAll.class); - final DenyAll mthdDenyAll = method.getAnnotation(DenyAll.class); - - if (mthdRolesAllowed != null) { - Set<String> roles = new HashSet<>(); - final Set<String> previous = rolesAllowed.putIfAbsent(method, roles); - if (previous != null) { - roles = previous; - } - roles.addAll(Arrays.asList(mthdRolesAllowed.value())); + if (!methodSecurityAnnotations.isEmpty()) { // method level annotations override class level ones + return toInterceptor(method.getAnnotation(RolesAllowed.class), + method.isAnnotationPresent(DenyAll.class), + method.isAnnotationPresent(PermitAll.class)); } - if (mthdPermitAll != null) { - permitAll.add(method); - } - - if (mthdDenyAll != null) { - denyAll.add(method); - } + return toInterceptor((RolesAllowed) clazz.getAnnotation(RolesAllowed.class), + clazz.isAnnotationPresent(DenyAll.class), + clazz.isAnnotationPresent(PermitAll.class)); + } - return true; + private MPJWTSecurityAnnotationsInterceptor toInterceptor(final RolesAllowed rolesAllowed, + final boolean denyAll, + final boolean permitAll) { + final Set<String> roles = rolesAllowed == null ? null : new HashSet<>(Arrays.asList(rolesAllowed.value())); + return new MPJWTSecurityAnnotationsInterceptor(roles, denyAll, permitAll); } private List<Class<? extends Annotation>[]> hasClassLevelAnnotations(final Class clazz, final Class<? extends Annotation>... annotationsToCheck) { diff --git a/mp-jwt/src/test/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptorsFeatureTest.java b/mp-jwt/src/test/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptorsFeatureTest.java new file mode 100644 index 0000000000..b8e74c77fe --- /dev/null +++ b/mp-jwt/src/test/java/org/apache/tomee/microprofile/jwt/jaxrs/MPJWTSecurityAnnotationsInterceptorsFeatureTest.java @@ -0,0 +1,237 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomee.microprofile.jwt.jaxrs; + +import jakarta.annotation.security.DenyAll; +import jakarta.annotation.security.PermitAll; +import jakarta.annotation.security.RolesAllowed; +import jakarta.ws.rs.container.ContainerRequestContext; +import jakarta.ws.rs.container.ContainerRequestFilter; +import jakarta.ws.rs.container.ResourceInfo; +import jakarta.ws.rs.core.FeatureContext; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.SecurityContext; +import org.junit.Test; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.net.HttpURLConnection; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; + +public class MPJWTSecurityAnnotationsInterceptorsFeatureTest { + + public static class BaseResource { + public String get() { + return "ok"; + } + + @RolesAllowed("override") + public String overridden() { + return "ok"; + } + + @PermitAll + public String open() { + return "ok"; + } + + @DenyAll + public String closed() { + return "ok"; + } + + public String unprotected() { + return "ok"; + } + } + + @RolesAllowed("admin") + public static class AdminResource extends BaseResource { + } + + @RolesAllowed("user") + public static class UserResource extends BaseResource { + } + + @RolesAllowed({}) + public static class NobodyResource extends BaseResource { + } + + @PermitAll + public static class PublicResource extends BaseResource { + } + + @DenyAll + public static class LockedResource extends BaseResource { + } + + public static class NoAnnotationResource extends BaseResource { + } + + public static class EmptyMethodRolesResource { + @RolesAllowed({}) + public String get() { + return "ok"; + } + } + + public static class ConflictingResource { + @RolesAllowed("admin") + @PermitAll + public String get() { + return "ok"; + } + } + + @Test + public void inheritedMethodKeepsRolesPerResourceClass() throws Exception { + final MPJWTSecurityAnnotationsInterceptorsFeature feature = new MPJWTSecurityAnnotationsInterceptorsFeature(); + final ContainerRequestFilter admin = configure(feature, AdminResource.class, "get"); + final ContainerRequestFilter user = configure(feature, UserResource.class, "get"); + + assertForbidden(admin, "user"); + assertAllowed(admin, "admin"); + assertForbidden(user, "admin"); + assertAllowed(user, "user"); + } + + @Test + public void emptyRolesAllowedDeniesEveryone() throws Exception { + final MPJWTSecurityAnnotationsInterceptorsFeature feature = new MPJWTSecurityAnnotationsInterceptorsFeature(); + + final ContainerRequestFilter classLevel = configure(feature, NobodyResource.class, "get"); + assertForbidden(classLevel); + assertForbidden(classLevel, "admin", "user"); + + final ContainerRequestFilter methodLevel = configure(feature, EmptyMethodRolesResource.class, "get"); + assertForbidden(methodLevel); + assertForbidden(methodLevel, "admin", "user"); + } + + @Test + public void methodLevelOverridesClassLevel() throws Exception { + final MPJWTSecurityAnnotationsInterceptorsFeature feature = new MPJWTSecurityAnnotationsInterceptorsFeature(); + + final ContainerRequestFilter overridden = configure(feature, AdminResource.class, "overridden"); + assertForbidden(overridden, "admin"); + assertAllowed(overridden, "override"); + + assertAllowed(configure(feature, AdminResource.class, "open")); + assertForbidden(configure(feature, AdminResource.class, "closed"), "admin"); + + assertAllowed(configure(feature, LockedResource.class, "open")); + assertForbidden(configure(feature, PublicResource.class, "closed")); + } + + @Test + public void permitAllAndDenyAll() throws Exception { + final MPJWTSecurityAnnotationsInterceptorsFeature feature = new MPJWTSecurityAnnotationsInterceptorsFeature(); + + assertAllowed(configure(feature, PublicResource.class, "get")); + assertForbidden(configure(feature, LockedResource.class, "get"), "admin"); + assertAllowed(configure(feature, NoAnnotationResource.class, "open")); + assertForbidden(configure(feature, NoAnnotationResource.class, "closed"), "admin"); + } + + @Test + public void unprotectedResourceHasNoInterceptor() throws Exception { + final MPJWTSecurityAnnotationsInterceptorsFeature feature = new MPJWTSecurityAnnotationsInterceptorsFeature(); + assertNull(configure(feature, NoAnnotationResource.class, "unprotected")); + } + + @Test(expected = IllegalStateException.class) + public void moreThanOneSecurityAnnotationIsRejected() throws Exception { + configure(new MPJWTSecurityAnnotationsInterceptorsFeature(), ConflictingResource.class, "get"); + } + + private static ContainerRequestFilter configure(final MPJWTSecurityAnnotationsInterceptorsFeature feature, + final Class<?> resourceClass, final String methodName) throws Exception { + final Method method = resourceClass.getMethod(methodName); + final ResourceInfo resourceInfo = new ResourceInfo() { + @Override + public Method getResourceMethod() { + return method; + } + + @Override + public Class<?> getResourceClass() { + return resourceClass; + } + }; + + final AtomicReference<ContainerRequestFilter> registered = new AtomicReference<>(); + final FeatureContext context = proxy(FeatureContext.class, (p, m, args) -> { + if ("register".equals(m.getName()) && args != null && args[0] instanceof ContainerRequestFilter) { + registered.set((ContainerRequestFilter) args[0]); + } + return p; + }); + + feature.configure(resourceInfo, context); + return registered.get(); + } + + private static void assertAllowed(final ContainerRequestFilter filter, final String... roles) throws Exception { + assertNotNull(filter); + assertNull(filter(filter, roles)); + } + + private static void assertForbidden(final ContainerRequestFilter filter, final String... roles) throws Exception { + assertNotNull(filter); + final Response response = filter(filter, roles); + assertNotNull(response); + assertEquals(HttpURLConnection.HTTP_FORBIDDEN, response.getStatus()); + } + + private static Response filter(final ContainerRequestFilter filter, final String... roles) throws Exception { + final Set<String> userRoles = new HashSet<>(Arrays.asList(roles)); + final SecurityContext securityContext = proxy(SecurityContext.class, (p, m, args) -> { + if ("isUserInRole".equals(m.getName())) { + return userRoles.contains((String) args[0]); + } + return null; + }); + + final AtomicReference<Response> aborted = new AtomicReference<>(); + final ContainerRequestContext requestContext = proxy(ContainerRequestContext.class, (p, m, args) -> { + if ("getSecurityContext".equals(m.getName())) { + return securityContext; + } + if ("abortWith".equals(m.getName())) { + aborted.set((Response) args[0]); + } + return null; + }); + + filter.filter(requestContext); + return aborted.get(); + } + + @SuppressWarnings("unchecked") + private static <T> T proxy(final Class<T> type, final InvocationHandler handler) { + return (T) Proxy.newProxyInstance(MPJWTSecurityAnnotationsInterceptorsFeatureTest.class.getClassLoader(), + new Class<?>[]{type}, handler); + } +}
