This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/scopes in repository https://gitbox.apache.org/repos/asf/cxf.git
commit a87cc108d726feb25026244546e65305d89aeeda Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Thu Oct 29 09:49:46 2026 +0000 Enforce every applicable @Scopes rule in OAuthScopesFilter --- .../security/oauth2/filters/OAuthScopesFilter.java | 139 +++++++++++++++----- .../oauth2/filters/OAuthScopesFilterTest.java | 141 ++++++++++++++++++++- 2 files changed, 241 insertions(+), 39 deletions(-) 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 a43cb02d6c1..565a6ed737c 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 @@ -25,6 +25,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -58,53 +59,58 @@ public class OAuthScopesFilter implements ContainerRequestFilter { // 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<>(); + // The method-level and class-level @Scopes declarations which apply to each method signature + // in the secured object's hierarchy. Keying by signature means that overloaded methods do + // not share a rule + private final Map<String, List<ScopesDeclaration>> declarationsBySignature = new HashMap<>(); + // The erased signatures a method overrides with a different signature, e.g. update(Object) + // for update(String) implementing update(T) of a generic interface, found via bridge methods + private final Map<String, Set<String>> bridgedSignatures = new HashMap<>(); + private final Map<String, Set<String>> signaturesByName = new HashMap<>(); + private final Set<Class<?>> securedClasses = new HashSet<>(); + private boolean annotationScopesFound; private Set<String> confidentialClientMethods = new HashSet<>(); public void setSecuredObject(Object object) { Class<?> cls = ClassHelper.getRealClass(object); checkSecureClass(cls); - if (annotationScopesByName.isEmpty()) { + if (!annotationScopesFound) { LOG.warning("The scopes map is empty"); } else if (LOG.isLoggable(Level.FINE)) { - for (Map.Entry<String, Set<Scopes>> entry : annotationScopesByName.entrySet()) { - for (Scopes scopes : entry.getValue()) { - LOG.fine("Method: " + entry.getKey() + ", scopes: " + Arrays.asList(scopes.value())); + for (String signature : declarationsBySignature.keySet()) { + for (Scopes scopes : getAnnotationScopes(signature)) { + LOG.fine("Method: " + signature + ", scopes: " + Arrays.asList(scopes.value())); } } } } protected void checkSecureClass(Class<?> cls) { - if (cls == null || cls == Object.class) { + if (cls == null || cls == Object.class || !securedClasses.add(cls)) { return; } + // The class-level @Scopes may be inherited from a superclass (@Scopes is @Inherited) Scopes classScopes = cls.getAnnotation(Scopes.class); + Class<?> classScopesDeclarer = getScopesDeclarer(cls); ConfidentialClient classConfClient = cls.getAnnotation(ConfidentialClient.class); for (Method m : cls.getMethods()) { 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); - // 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 + List<ScopesDeclaration> declarations = + declarationsBySignature.computeIfAbsent(signature, k -> new ArrayList<>()); 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); + declarations.add(new ScopesDeclaration(m.getDeclaringClass(), methodScopes, true)); + } + if (classScopes != null) { + declarations.add(new ScopesDeclaration(classScopesDeclarer, classScopes, false)); + } + signaturesByName.computeIfAbsent(m.getName(), k -> new HashSet<>()).add(signature); + annotationScopesFound |= methodScopes != null || classScopes != null; + if (m.isBridge() && m.getDeclaringClass() == cls) { + recordBridgedSignature(cls, m, signature); } ConfidentialClient mConfClient = m.getAnnotation(ConfidentialClient.class); @@ -161,19 +167,78 @@ public class OAuthScopesFilter implements ContainerRequestFilter { } } + private static Class<?> getScopesDeclarer(Class<?> cls) { + Class<?> declarer = cls; + while (declarer != null && declarer.getDeclaredAnnotation(Scopes.class) == null) { + declarer = declarer.getSuperclass(); + } + return declarer == null ? cls : declarer; + } + + private void recordBridgedSignature(Class<?> cls, Method bridge, String bridgeSignature) { + // A bridge method has the erased signature of the method it overrides and delegates + // to the method with the same name and narrower types, which may be inherited + for (Method m : cls.getMethods()) { + if (!m.isBridge() && m.getName().equals(bridge.getName()) + && isAssignable(bridge.getParameterTypes(), m.getParameterTypes())) { + bridgedSignatures.computeIfAbsent(ReflectionUtil.getMethodSignature(m), k -> new HashSet<>()) + .add(bridgeSignature); + } + } + } + + private static boolean isAssignable(Class<?>[] types, Class<?>[] subTypes) { + if (types.length != subTypes.length) { + return false; + } + for (int i = 0; i < types.length; i++) { + if (!types[i].isAssignableFrom(subTypes[i])) { + return false; + } + } + return true; + } + 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); + Set<String> signatures = new HashSet<>(); + if (declarationsBySignature.containsKey(signature)) { + signatures.add(signature); + signatures.addAll(bridgedSignatures.getOrDefault(signature, Collections.emptySet())); + } else { + // The method is not one of the secured object's methods, so every rule + // recorded for its name applies + signatures.addAll(signaturesByName.getOrDefault(m.getName(), Collections.emptySet())); } - if (scopes != null) { - return Collections.singletonList(scopes); + Set<Scopes> scopes = new LinkedHashSet<>(); + for (String s : signatures) { + scopes.addAll(getAnnotationScopes(s)); } - Set<Scopes> scopesForName = annotationScopesByName.get(m.getName()); - return scopesForName == null ? Collections.emptyList() : new ArrayList<>(scopesForName); + return new ArrayList<>(scopes); + } + + private List<Scopes> getAnnotationScopes(String signature) { + // Every @Scopes which applies to the method has to be met, from any class or interface + // in the hierarchy. A method-level @Scopes only replaces the class-level @Scopes of the + // class or interface which declares it + List<ScopesDeclaration> declarations = + declarationsBySignature.getOrDefault(signature, Collections.emptyList()); + Set<Scopes> scopes = new LinkedHashSet<>(); + for (ScopesDeclaration declaration : declarations) { + if (declaration.methodLevel || !hasMethodScopes(declaration.declaringClass, declarations)) { + scopes.add(declaration.scopes); + } + } + return new ArrayList<>(scopes); + } + + private static boolean hasMethodScopes(Class<?> cls, List<ScopesDeclaration> declarations) { + for (ScopesDeclaration declaration : declarations) { + if (declaration.methodLevel && declaration.declaringClass == cls) { + return true; + } + } + return false; } private static boolean isAllowed(List<String> requestScopes, List<String> methodScopes, @@ -215,5 +280,15 @@ public class OAuthScopesFilter implements ContainerRequestFilter { this.confidentialClientMethods = confidentialClientMethods; } + private static final class ScopesDeclaration { + private final Class<?> declaringClass; + private final Scopes scopes; + private final boolean methodLevel; + ScopesDeclaration(Class<?> declaringClass, Scopes scopes, boolean methodLevel) { + this.declaringClass = declaringClass; + this.scopes = scopes; + this.methodLevel = methodLevel; + } + } } 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 index ca125bfef26..6d716906620 100644 --- 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 @@ -63,19 +63,29 @@ public class OAuthScopesFilterTest { } @Test - public void testOverridingMethodScopesTakePrecedenceOverSuperclass() throws Exception { + public void testOverridingMethodScopesApplyTogetherWithSuperclassScopes() throws Exception { Method method = OverridingService.class.getMethod("operation"); - assertAllowed(new OverridingService(), method, "admin"); + assertAllowed(new OverridingService(), method, "read", "admin"); assertForbidden(new OverridingService(), method, "read"); + assertForbidden(new OverridingService(), method, "admin"); } @Test - public void testOverridingMethodScopesTakePrecedenceOverInterfaceClassScopes() throws Exception { + public void testOverridingMethodCannotWeakenSuperclassScopes() throws Exception { + Method method = ReadOverridingService.class.getMethod("operation"); + + assertAllowed(new ReadOverridingService(), method, "read", "admin"); + assertForbidden(new ReadOverridingService(), method, "read"); + } + + @Test + public void testOverridingMethodScopesApplyTogetherWithInterfaceClassScopes() throws Exception { Method method = InterfaceOverridingService.class.getMethod("operation"); - assertAllowed(new InterfaceOverridingService(), method, "admin"); + assertAllowed(new InterfaceOverridingService(), method, "read", "admin"); assertForbidden(new InterfaceOverridingService(), method, "read"); + assertForbidden(new InterfaceOverridingService(), method, "admin"); } @Test @@ -127,13 +137,65 @@ public class OAuthScopesFilterTest { 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"); + // update(String) overrides the erased update(Object), but not the update() overload + assertAllowed(new StringRepository(), updateMethod, "admin"); assertForbidden(new StringRepository(), updateMethod, "read"); - assertForbidden(new StringRepository(), updateMethod, "admin"); Method readMethod = StringRepository.class.getMethod("update"); assertAllowed(new StringRepository(), readMethod, "read"); + assertForbidden(new StringRepository(), readMethod, "admin"); + } + + @Test + public void testGenericInterfaceMethodScopesApplyTogetherWithClassScopes() throws Exception { + Method method = ClassScopesStringRepository.class.getMethod("update", String.class); + + assertAllowed(new ClassScopesStringRepository(), method, "user", "admin"); + assertForbidden(new ClassScopesStringRepository(), method, "user"); + assertForbidden(new ClassScopesStringRepository(), method, "admin"); + } + + @Test + public void testSubclassScopesApplyTogetherWithSuperclassMethodScopes() throws Exception { + Method method = AdminSubService.class.getMethod("operation"); + + assertAllowed(new AdminSubService(), method, "read", "admin"); + assertForbidden(new AdminSubService(), method, "read"); + assertForbidden(new AdminSubService(), method, "admin"); + } + + @Test + public void testGenericInterfaceMethodScopesApplyToInheritedImplementation() throws Exception { + Method method = InheritingStringRepository.class.getMethod("update", String.class); + + assertAllowed(new InheritingStringRepository(), method, "admin"); + assertForbidden(new InheritingStringRepository(), method, "read"); + } + + @Test + public void testMethodScopesFromUnrelatedSupertypesApplyTogether() throws Exception { + Method method = AdminApiReadService.class.getMethod("operation"); + + assertAllowed(new AdminApiReadService(), method, "read", "admin"); + assertForbidden(new AdminApiReadService(), method, "read"); + assertForbidden(new AdminApiReadService(), method, "admin"); + } + + @Test + public void testUnannotatedOverloadIsNotSecuredByOtherOverloads() throws Exception { + Method method = UnannotatedOverloadService.class.getMethod("delete"); + + assertAllowed(new UnannotatedOverloadService(), method, "user"); + assertForbidden(new UnannotatedOverloadService(), + UnannotatedOverloadService.class.getMethod("delete", String.class), "user"); + } + + @Test + public void testInheritedClassScopesApplyToSubclassMethods() throws Exception { + Method method = InheritedClassScopesService.class.getMethod("subclassOperation"); + + assertAllowed(new InheritedClassScopesService(), method, "read"); + assertForbidden(new InheritedClassScopesService(), method, "write"); } @Test @@ -213,6 +275,19 @@ public class OAuthScopesFilterTest { } } + public static class AdminBaseService { + @Scopes("admin") + public void operation() { + } + } + + public static class ReadOverridingService extends AdminBaseService { + @Override + @Scopes("read") + public void operation() { + } + } + public static class UnannotatedOverridingService extends BaseService { @Override public void operation() { @@ -248,6 +323,9 @@ public class OAuthScopesFilterTest { @Override public void operation() { } + + public void subclassOperation() { + } } public interface Repository<T> { @@ -268,6 +346,55 @@ public class OAuthScopesFilterTest { } } + @Scopes("user") + public static class ClassScopesStringRepository implements Repository<String> { + @Override + public void update(String value) { + } + + @Override + public void update() { + } + } + + @Scopes("admin") + public static class AdminSubService extends BaseService { + @Override + public void operation() { + } + } + + public static class StringUpdater { + public void update(String value) { + } + + public void update() { + } + } + + public static class InheritingStringRepository extends StringUpdater implements Repository<String> { + } + + public interface AdminApi { + @Scopes("admin") + void operation(); + } + + public static class AdminApiReadService extends BaseService implements AdminApi { + @Override + public void operation() { + } + } + + public static class UnannotatedOverloadService { + @Scopes("admin") + public void delete(String id) { + } + + public void delete() { + } + } + public static class UnsecuredService { public void operation(String value) { }
