This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch 4.1.x-fixes in repository https://gitbox.apache.org/repos/asf/cxf.git
commit 6f7688f4f054796f86889bfd369e74cd3f6e0cf1 Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Mon Oct 26 08:03:03 2026 +0000 Resolve OAuthScopesFilter annotation scopes by method signature (#3627) * Resolve OAuthScopesFilter annotation scopes by method signature * Review comments (cherry picked from commit 832021f8e623721ec9e17e522f173fc915a366ee) --- .../org/apache/cxf/common/util/ReflectionUtil.java | 17 ++ .../security/oauth2/filters/OAuthScopesFilter.java | 87 +++++-- .../oauth2/filters/OAuthScopesFilterTest.java | 275 +++++++++++++++++++++ 3 files changed, 363 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/apache/cxf/common/util/ReflectionUtil.java b/core/src/main/java/org/apache/cxf/common/util/ReflectionUtil.java index 146d7fc5762..92f89082aa1 100644 --- a/core/src/main/java/org/apache/cxf/common/util/ReflectionUtil.java +++ b/core/src/main/java/org/apache/cxf/common/util/ReflectionUtil.java @@ -170,6 +170,23 @@ public final class ReflectionUtil { } } + /** + * Returns the name and parameter types of a method, e.g. "foo(java.lang.String,int)". + * Overloaded methods have distinct signatures, while a method and the method it + * overrides (with the same erased parameter types) share one. + */ + public static String getMethodSignature(final Method method) { + StringBuilder sb = new StringBuilder(method.getName()).append('('); + Class<?>[] paramTypes = method.getParameterTypes(); + for (int i = 0; i < paramTypes.length; i++) { + if (i > 0) { + sb.append(','); + } + sb.append(paramTypes[i].getName()); + } + return sb.append(')').toString(); + } + public static Field[] getDeclaredFields(final Class<?> cls) { return AccessController.doPrivileged(new PrivilegedAction<Field[]>() { public Field[] run() { diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilter.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilter.java index f3fa59887a5..a43cb02d6c1 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilter.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilter.java @@ -20,7 +20,9 @@ package org.apache.cxf.rs.security.oauth2.filters; import java.io.IOException; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedList; @@ -37,6 +39,7 @@ import jakarta.ws.rs.container.ContainerRequestFilter; import jakarta.ws.rs.core.Context; import org.apache.cxf.common.logging.LogUtils; import org.apache.cxf.common.util.ClassHelper; +import org.apache.cxf.common.util.ReflectionUtil; import org.apache.cxf.jaxrs.ext.MessageContext; import org.apache.cxf.jaxrs.utils.ExceptionUtils; import org.apache.cxf.rs.security.oauth2.common.OAuthContext; @@ -52,18 +55,29 @@ public class OAuthScopesFilter implements ContainerRequestFilter { @Context private MessageContext mc; + // Scopes configured explicitly via setScopesMap/setScopesStringMap, keyed by method name private Map<String, List<String>> scopesMap = new HashMap<>(); private Map<String, Boolean> scopesMatchAllMap = new HashMap<>(); + // Scopes discovered from @Scopes annotations. Method-level and class-level annotations + // are keyed by method signature so that overloaded methods do not share a rule and an + // annotation on an overriding method is not replaced by one from a superclass or interface + private final Map<String, Scopes> methodScopesBySignature = new HashMap<>(); + private final Map<String, Scopes> classScopesBySignature = new HashMap<>(); + // All the annotation-derived rules for a given method name, enforced together when the + // invoked method's signature is not recorded (e.g. it implements a generic interface method) + private final Map<String, Set<Scopes>> annotationScopesByName = new HashMap<>(); private Set<String> confidentialClientMethods = new HashSet<>(); public void setSecuredObject(Object object) { Class<?> cls = ClassHelper.getRealClass(object); checkSecureClass(cls); - if (scopesMap.isEmpty()) { + if (annotationScopesByName.isEmpty()) { LOG.warning("The scopes map is empty"); } else if (LOG.isLoggable(Level.FINE)) { - for (Map.Entry<String, List<String>> entry : scopesMap.entrySet()) { - LOG.fine("Method: " + entry.getKey() + ", scopes: " + entry.getValue()); + for (Map.Entry<String, Set<Scopes>> entry : annotationScopesByName.entrySet()) { + for (Scopes scopes : entry.getValue()) { + LOG.fine("Method: " + entry.getKey() + ", scopes: " + Arrays.asList(scopes.value())); + } } } } @@ -78,11 +92,19 @@ public class OAuthScopesFilter implements ContainerRequestFilter { if (SKIP_METHODS.contains(m.getName())) { continue; } + // Subclasses are scanned before their superclasses and interfaces, so the most + // specific declaration of a given signature is recorded first and kept + String signature = ReflectionUtil.getMethodSignature(m); Scopes methodScopes = m.getAnnotation(Scopes.class); - Scopes theScopes = methodScopes == null ? classScopes : methodScopes; - if (theScopes != null) { - scopesMap.put(m.getName(), Arrays.asList(theScopes.value())); - scopesMatchAllMap.put(m.getName(), theScopes.matchAll()); + // Each overload has its own signature and so its own rule. The rules are also + // collected by name, but that is only used as a fallback when the invoked + // method's signature was not recorded, and then all of them have to be met + if (methodScopes != null) { + methodScopesBySignature.putIfAbsent(signature, methodScopes); + annotationScopesByName.computeIfAbsent(m.getName(), k -> new HashSet<>()).add(methodScopes); + } else if (classScopes != null) { + classScopesBySignature.putIfAbsent(signature, classScopes); + annotationScopesByName.computeIfAbsent(m.getName(), k -> new HashSet<>()).add(classScopes); } ConfidentialClient mConfClient = m.getAnnotation(ConfidentialClient.class); @@ -113,27 +135,60 @@ public class OAuthScopesFilter implements ContainerRequestFilter { } } protected void checkScopes(Method m) { - List<String> methodScopes = scopesMap.get(m.getName()); - if (methodScopes == null) { + List<Scopes> annotationScopes = getAnnotationScopes(m); + List<String> configuredScopes = scopesMap.get(m.getName()); + if (annotationScopes.isEmpty() && configuredScopes == null) { return; } - boolean matchAll = scopesMatchAllMap.get(m.getName()); OAuthContext context = OAuthContextUtils.getContext(mc); List<String> requestScopes = new LinkedList<>(); for (OAuthPermission perm : context.getPermissions()) { - if (matchAll) { - requestScopes.add(perm.getPermission()); - } else if (methodScopes.contains(perm.getPermission())) { - return; - } + requestScopes.add(perm.getPermission()); } - if (!requestScopes.containsAll(methodScopes)) { + // Every rule which applies to the method has to be satisfied + boolean allowed = true; + for (Scopes scopes : annotationScopes) { + allowed &= isAllowed(requestScopes, Arrays.asList(scopes.value()), scopes.matchAll()); + } + if (configuredScopes != null) { + allowed &= isAllowed(requestScopes, configuredScopes, + Boolean.TRUE.equals(scopesMatchAllMap.get(m.getName()))); + } + if (!allowed) { LOG.warning("Scopes do not match"); throw ExceptionUtils.toForbiddenException(null, null); } + } + + private List<Scopes> getAnnotationScopes(Method m) { + String signature = ReflectionUtil.getMethodSignature(m); + // A method-level annotation anywhere in the hierarchy takes precedence over a + // class-level one, otherwise an inherited class-level @Scopes would replace it + Scopes scopes = methodScopesBySignature.get(signature); + if (scopes == null) { + scopes = classScopesBySignature.get(signature); + } + if (scopes != null) { + return Collections.singletonList(scopes); + } + Set<Scopes> scopesForName = annotationScopesByName.get(m.getName()); + return scopesForName == null ? Collections.emptyList() : new ArrayList<>(scopesForName); + } + private static boolean isAllowed(List<String> requestScopes, List<String> methodScopes, + boolean matchAll) { + if (matchAll || methodScopes.isEmpty()) { + return requestScopes.containsAll(methodScopes); + } + for (String scope : requestScopes) { + if (methodScopes.contains(scope)) { + return true; + } + } + return false; } + protected Method getTargetMethod() { Method method = (Method)mc.get("org.apache.cxf.resource.method"); if (method != null) { diff --git a/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilterTest.java b/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilterTest.java new file mode 100644 index 00000000000..ca125bfef26 --- /dev/null +++ b/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilterTest.java @@ -0,0 +1,275 @@ +/** + * 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.cxf.rs.security.oauth2.filters; + +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import jakarta.ws.rs.ForbiddenException; +import org.apache.cxf.jaxrs.ext.MessageContext; +import org.apache.cxf.rs.security.oauth2.common.OAuthContext; +import org.apache.cxf.rs.security.oauth2.common.OAuthPermission; +import org.apache.cxf.rs.security.oauth2.common.UserSubject; + +import org.junit.Test; + +import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class OAuthScopesFilterTest { + + @Test + public void testOverloadedSubclassMethodKeepsItsOwnScopes() throws Exception { + Method adminMethod = OverloadingService.class.getMethod("operation", String.class); + Method readMethod = OverloadingService.class.getMethod("operation"); + + assertAllowed(new OverloadingService(), adminMethod, "admin"); + assertForbidden(new OverloadingService(), adminMethod, "read"); + + assertAllowed(new OverloadingService(), readMethod, "read"); + assertForbidden(new OverloadingService(), readMethod, "admin"); + } + + @Test + public void testOverloadedMethodsInSameClassKeepTheirOwnScopes() throws Exception { + Method readMethod = SameClassOverloadingService.class.getMethod("operation"); + Method adminMethod = SameClassOverloadingService.class.getMethod("operation", String.class); + + assertAllowed(new SameClassOverloadingService(), readMethod, "read"); + assertForbidden(new SameClassOverloadingService(), readMethod, "admin"); + + assertAllowed(new SameClassOverloadingService(), adminMethod, "admin"); + assertForbidden(new SameClassOverloadingService(), adminMethod, "read"); + } + + @Test + public void testOverridingMethodScopesTakePrecedenceOverSuperclass() throws Exception { + Method method = OverridingService.class.getMethod("operation"); + + assertAllowed(new OverridingService(), method, "admin"); + assertForbidden(new OverridingService(), method, "read"); + } + + @Test + public void testOverridingMethodScopesTakePrecedenceOverInterfaceClassScopes() throws Exception { + Method method = InterfaceOverridingService.class.getMethod("operation"); + + assertAllowed(new InterfaceOverridingService(), method, "admin"); + assertForbidden(new InterfaceOverridingService(), method, "read"); + } + + @Test + public void testMatchAllIsResolvedPerSignature() throws Exception { + Method matchAllMethod = MatchAllService.class.getMethod("operation", String.class); + Method matchAnyMethod = MatchAllService.class.getMethod("operation"); + + assertAllowed(new MatchAllService(), matchAllMethod, "read", "write"); + assertForbidden(new MatchAllService(), matchAllMethod, "read"); + + assertAllowed(new MatchAllService(), matchAnyMethod, "read"); + } + + @Test + public void testInheritedMethodScopesStillApply() throws Exception { + Method method = OverridingService.class.getMethod("inherited"); + + assertAllowed(new OverridingService(), method, "read"); + assertForbidden(new OverridingService(), method, "write"); + } + + @Test + public void testUnannotatedOverrideInheritsSuperclassScopes() throws Exception { + Method method = UnannotatedOverridingService.class.getMethod("operation"); + + assertAllowed(new UnannotatedOverridingService(), method, "read"); + assertForbidden(new UnannotatedOverridingService(), method, "write"); + } + + @Test + public void testScopesConfiguredByMethodName() throws Exception { + Method method = UnsecuredService.class.getMethod("operation", String.class); + + OAuthScopesFilter filter = new OAuthScopesFilter(); + filter.setScopesStringMap(Collections.singletonMap("operation", "admin")); + filter(filter, method, "admin"); + assertThrows(ForbiddenException.class, () -> filter(filter, method, "read")); + } + + @Test + public void testMethodScopesTakePrecedenceOverInheritedClassScopes() throws Exception { + Method method = InheritedClassScopesService.class.getMethod("operation"); + + assertAllowed(new InheritedClassScopesService(), method, "admin"); + assertForbidden(new InheritedClassScopesService(), method, "read"); + } + + @Test + public void testGenericInterfaceMethodScopesAreEnforced() throws Exception { + Method updateMethod = StringRepository.class.getMethod("update", String.class); + + // The erased interface signature does not match, so every rule for the name applies + assertAllowed(new StringRepository(), updateMethod, "admin", "read"); + assertForbidden(new StringRepository(), updateMethod, "read"); + assertForbidden(new StringRepository(), updateMethod, "admin"); + + Method readMethod = StringRepository.class.getMethod("update"); + assertAllowed(new StringRepository(), readMethod, "read"); + } + + @Test + public void testConfiguredScopesAreEnforcedTogetherWithAnnotations() throws Exception { + Method method = BaseService.class.getMethod("operation"); + + OAuthScopesFilter filter = createFilter(new BaseService()); + filter.setScopesStringMap(Collections.singletonMap("operation", "admin")); + filter(filter, method, "read", "admin"); + assertThrows(ForbiddenException.class, () -> filter(filter, method, "read")); + assertThrows(ForbiddenException.class, () -> filter(filter, method, "admin")); + } + + private static void assertAllowed(Object service, Method method, String... scopes) throws Exception { + filter(createFilter(service), method, scopes); + } + + private static void assertForbidden(Object service, Method method, String... scopes) throws Exception { + OAuthScopesFilter filter = createFilter(service); + assertThrows(ForbiddenException.class, () -> filter(filter, method, scopes)); + } + + private static OAuthScopesFilter createFilter(Object service) { + OAuthScopesFilter filter = new OAuthScopesFilter(); + filter.setSecuredObject(service); + return filter; + } + + private static void filter(OAuthScopesFilter filter, Method method, String... scopes) throws Exception { + List<OAuthPermission> permissions = new ArrayList<>(); + for (String scope : scopes) { + permissions.add(new OAuthPermission(scope)); + } + OAuthContext context = new OAuthContext(new UserSubject("alice"), null, permissions, "code"); + + MessageContext mc = mock(MessageContext.class); + when(mc.get("org.apache.cxf.resource.method")).thenReturn(method); + when(mc.getContent(OAuthContext.class)).thenReturn(context); + + Field field = OAuthScopesFilter.class.getDeclaredField("mc"); + field.setAccessible(true); + field.set(filter, mc); + + filter.filter(null); + } + + public static class BaseService { + @Scopes("read") + public void operation() { + } + + @Scopes("read") + public void inherited() { + } + } + + public static class OverloadingService extends BaseService { + @Scopes("admin") + public void operation(String value) { + } + } + + public static class SameClassOverloadingService { + @Scopes("read") + public void operation() { + } + + @Scopes("admin") + public void operation(String value) { + } + } + + public static class OverridingService extends BaseService { + @Override + @Scopes("admin") + public void operation() { + } + } + + public static class UnannotatedOverridingService extends BaseService { + @Override + public void operation() { + } + } + + @Scopes("read") + public interface ReadService { + void operation(); + } + + public static class InterfaceOverridingService implements ReadService { + @Override + @Scopes("admin") + public void operation() { + } + } + + public static class MatchAllService extends BaseService { + @Scopes(value = {"read", "write" }, matchAll = true) + public void operation(String value) { + } + } + + @Scopes("read") + public static class InheritedClassScopesBaseService { + @Scopes("admin") + public void operation() { + } + } + + public static class InheritedClassScopesService extends InheritedClassScopesBaseService { + @Override + public void operation() { + } + } + + public interface Repository<T> { + @Scopes("admin") + void update(T value); + + @Scopes("read") + void update(); + } + + public static class StringRepository implements Repository<String> { + @Override + public void update(String value) { + } + + @Override + public void update() { + } + } + + public static class UnsecuredService { + public void operation(String value) { + } + } +}
