This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/SecureAnnotationsInterceptor-generic in repository https://gitbox.apache.org/repos/asf/cxf.git
commit 03c178da380f16f10e71ff4cc75576c34711f261 Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Wed Nov 4 07:50:11 2026 +0000 Resolve generic overrides exactly in SecureAnnotationsInterceptor --- .../security/SecureAnnotationsInterceptor.java | 33 ++++-- .../security/SecureAnnotationsInterceptorTest.java | 112 ++++++++++++++++++++- 2 files changed, 133 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptor.java b/core/src/main/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptor.java index 284a8d5364f..0ec35183c1f 100644 --- a/core/src/main/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptor.java +++ b/core/src/main/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptor.java @@ -52,6 +52,9 @@ public class SecureAnnotationsInterceptor extends SimpleAuthorizingInterceptor { // the secured object's hierarchy. Keying by signature means that overloaded methods do not // share a rule private final Map<String, List<RolesDeclaration>> declarationsBySignature = new HashMap<>(); + // The erased signatures a method overrides with a different signature, e.g. update(Object) + // for update(String) implementing update(T) of Repository<String> + private final Map<String, Set<String>> overriddenSignatures = new HashMap<>(); private final Map<String, Set<String>> signaturesByName = new HashMap<>(); private final Set<Class<?>> securedClasses = new HashSet<>(); // The classes passed to setSecuredObject, so their roles can be read again if the @@ -93,6 +96,7 @@ public class SecureAnnotationsInterceptor extends SimpleAuthorizingInterceptor { return; } declarationsBySignature.clear(); + overriddenSignatures.clear(); signaturesByName.clear(); securedClasses.clear(); for (Class<?> cls : securedObjectClasses) { @@ -152,6 +156,12 @@ public class SecureAnnotationsInterceptor extends SimpleAuthorizingInterceptor { declarations.add(new RolesDeclaration(classRolesDeclarer, classRoles, false)); } signaturesByName.computeIfAbsent(m.getName(), k -> new HashSet<>()).add(signature); + if (!m.isBridge()) { + for (Method overridden : ReflectionUtil.getGenericallyOverriddenMethods(cls, m)) { + overriddenSignatures.computeIfAbsent(signature, k -> new HashSet<>()) + .add(ReflectionUtil.getMethodSignature(overridden)); + } + } } findRoles(cls.getSuperclass(), rolesMap); for (Class<?> interfaceCls : cls.getInterfaces()) { @@ -205,18 +215,21 @@ public class SecureAnnotationsInterceptor extends SimpleAuthorizingInterceptor { } private List<List<String>> getAnnotationRoles(Method method) { - List<List<String>> rules = getAnnotationRoles(ReflectionUtil.getMethodSignature(method)); - if (!rules.isEmpty()) { - return rules; + String signature = ReflectionUtil.getMethodSignature(method); + Set<String> signatures = new HashSet<>(); + if (declarationsBySignature.containsKey(signature)) { + signatures.add(signature); + signatures.addAll(overriddenSignatures.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(method.getName(), Collections.emptySet())); } - // No rule applies to the method's signature, e.g. because it implements a generic - // interface method whose erased signature differs, so every rule recorded for its - // name applies - Set<List<String>> nameRules = new LinkedHashSet<>(); - for (String s : signaturesByName.getOrDefault(method.getName(), Collections.emptySet())) { - nameRules.addAll(getAnnotationRoles(s)); + Set<List<String>> rules = new LinkedHashSet<>(); + for (String s : signatures) { + rules.addAll(getAnnotationRoles(s)); } - return new ArrayList<>(nameRules); + return new ArrayList<>(rules); } private List<List<String>> getAnnotationRoles(String signature) { diff --git a/core/src/test/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptorTest.java b/core/src/test/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptorTest.java index 4956b01fb5a..290c9ca1a4c 100644 --- a/core/src/test/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptorTest.java +++ b/core/src/test/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptorTest.java @@ -351,11 +351,61 @@ public class SecureAnnotationsInterceptorTest { } @Test - public void testGenericInterfaceMethodRolesStillApply() throws Exception { + public void testGenericInterfaceMethodRolesAreEnforced() throws Exception { + // update(String) overrides the erased update(Object), but not the update() overload Method target = StringRepository.class.getMethod("update", String.class); - + assertAllowed(new StringRepository(), target, "admin"); assertDenied(new StringRepository(), target, "user"); assertDenied(new StringRepository(), target); + + Method userMethod = StringRepository.class.getMethod("update"); + assertAllowed(new StringRepository(), userMethod, "user"); + assertDenied(new StringRepository(), userMethod, "admin"); + } + + @Test + public void testGenericInterfaceMethodRolesApplyTogetherWithClassRoles() throws Exception { + Method target = ClassRolesStringRepository.class.getMethod("update", String.class); + + assertAllowed(new ClassRolesStringRepository(), target, "user", "admin"); + assertDenied(new ClassRolesStringRepository(), target, "user"); + assertDenied(new ClassRolesStringRepository(), target, "admin"); + } + + @Test + public void testGenericInterfaceMethodRolesApplyToInheritedImplementation() throws Exception { + Method target = InheritingStringRepository.class.getMethod("update", String.class); + + assertAllowed(new InheritingStringRepository(), target, "admin"); + assertDenied(new InheritingStringRepository(), target, "user"); + } + + @Test + public void testGenericInterfaceMethodRolesApplyThroughTypeVariables() throws Exception { + Method target = TypedStringRepository.class.getMethod("update", String.class); + + assertAllowed(new TypedStringRepository(), target, "admin"); + assertDenied(new TypedStringRepository(), target, "user"); + } + + @Test + public void testUnrelatedOverloadIsNotLinkedToGenericInterfaceMethod() throws Exception { + Method integerMethod = OverloadingStringRepository.class.getMethod("update", Integer.class); + assertAllowed(new OverloadingStringRepository(), integerMethod, "user"); + assertDenied(new OverloadingStringRepository(), integerMethod, "admin"); + + Method stringMethod = OverloadingStringRepository.class.getMethod("update", String.class); + assertAllowed(new OverloadingStringRepository(), stringMethod, "admin"); + assertDenied(new OverloadingStringRepository(), stringMethod, "user"); + } + + @Test + public void testUnannotatedOverloadIsNotSecuredByOtherOverloads() throws Exception { + Method target = UnannotatedOverloadService.class.getMethod("delete"); + + assertAllowed(new UnannotatedOverloadService(), target, "user"); + assertDenied(new UnannotatedOverloadService(), + UnannotatedOverloadService.class.getMethod("delete", String.class), "user"); } @Test @@ -627,6 +677,64 @@ public class SecureAnnotationsInterceptorTest { } } + @RolesAllowed("user") + public static class ClassRolesStringRepository implements Repository<String> { + @Override + public void update(String value) { + } + + @Override + public void update() { + } + } + + public static class StringUpdater { + public void update(String value) { + } + + public void update() { + } + } + + public static class InheritingStringRepository extends StringUpdater implements Repository<String> { + } + + public abstract static class AbstractRepository<T> implements Repository<T> { // NOPMD + } + + public static class TypedStringRepository extends AbstractRepository<String> { + @Override + public void update(String value) { + } + + @Override + public void update() { + } + } + + public static class OverloadingStringRepository implements Repository<String> { + @Override + public void update(String value) { + } + + @Override + public void update() { + } + + @RolesAllowed("user") + public void update(Integer value) { + } + } + + public static class UnannotatedOverloadService { + @RolesAllowed("admin") + public void delete(String id) { + } + + public void delete() { + } + } + public static class UnsecuredService { public void delete() { }
