This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/annotation in repository https://gitbox.apache.org/repos/asf/cxf.git
commit b9419618dfaf0d2c3d3244a9c8d8d748b169354f Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Fri Nov 6 06:58:44 2026 +0000 Don't leave methods unprotected when a role annotation can't be read --- .../security/SecureAnnotationsInterceptor.java | 35 ++++++++++++--- .../security/SecureAnnotationsInterceptorTest.java | 50 ++++++++++++++++++++++ 2 files changed, 78 insertions(+), 7 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 0ec35183c1f..967e1b29857 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 @@ -69,16 +69,32 @@ public class SecureAnnotationsInterceptor extends SimpleAuthorizingInterceptor { } public void setAnnotationClassName(String name) { + final Class<?> annotationClass; try { - ClassLoaderUtils.loadClass(name, SecureAnnotationsInterceptor.class); - annotationClassName = name; + annotationClass = ClassLoaderUtils.loadClass(name, SecureAnnotationsInterceptor.class); } catch (Throwable ex) { ex.printStackTrace(); throw new IllegalArgumentException("Annotation class " + name + " is not available"); } + // The roles are read from the annotation's value(), so reject an annotation without one + // rather than leave the methods it is on unprotected + if (!annotationClass.isAnnotation() || !hasRolesValue(annotationClass)) { + throw new IllegalArgumentException("Annotation class " + name + + " is not an annotation with a String or String[] value()"); + } + annotationClassName = name; resolveSecuredObjectRoles(); } + private static boolean hasRolesValue(Class<?> annotationClass) { + try { + Class<?> valueType = annotationClass.getMethod("value").getReturnType(); + return valueType == String.class || valueType == String[].class; + } catch (NoSuchMethodException ex) { + return false; + } + } + public void setSecuredObject(Object object) { Class<?> cls = ClassHelper.getRealClass(object); securedObjectClasses.add(cls); @@ -280,13 +296,18 @@ public class SecureAnnotationsInterceptor extends SimpleAuthorizingInterceptor { for (Annotation ann : anns) { if (ann.annotationType().getName().equals(annotationClassName)) { try { - Method valueMethod = ann.annotationType().getMethod("value"); - String[] roles = (String[])valueMethod.invoke(ann); - return Arrays.asList(roles); + Object roles = ann.annotationType().getMethod("value").invoke(ann); + if (roles instanceof String[]) { + return Arrays.asList((String[])roles); + } else if (roles instanceof String) { + return Collections.singletonList((String)roles); + } + LOG.warning("The value of " + ann + " is not a String or String[]"); } catch (Exception ex) { - // ignore + LOG.log(Level.WARNING, "The roles of " + ann + " can not be read", ex); } - break; + // The method is protected by the annotation, so deny every user + return Collections.emptyList(); } } return null; 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 290c9ca1a4c..290a5dcf074 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 @@ -169,6 +169,33 @@ public class SecureAnnotationsInterceptorTest { } } + @Test + public void testSingleRoleAnnotation() throws Exception { + Method target = SingleRoleService.class.getMethod("operation"); + + SecureAnnotationsInterceptor in = createInterceptor(new SingleRoleService(), SingleRole.class); + assertAllowed(in, target, "admin"); + assertDenied(in, target, "user"); + assertDenied(in, target); + } + + @Test + public void testAnnotationClassWithoutRolesValueIsRejected() throws Exception { + SecureAnnotationsInterceptor in = new SecureAnnotationsInterceptor(); + assertThrows(IllegalArgumentException.class, + () -> in.setAnnotationClassName(IntValueRoles.class.getName())); + assertThrows(IllegalArgumentException.class, + () -> in.setAnnotationClassName(NoValueRoles.class.getName())); + assertThrows(IllegalArgumentException.class, + () -> in.setAnnotationClassName(String.class.getName())); + + // The annotation class is unchanged + in.setSecuredObject(new SameClassOverloadingService()); + Method target = SameClassOverloadingService.class.getMethod("operation"); + assertAllowed(in, target, "user"); + assertDenied(in, target, "admin"); + } + @Test public void testOverloadedSubclassMethodKeepsItsOwnRoles() throws Exception { Method adminMethod = OverloadingService.class.getMethod("operation", String.class); @@ -746,6 +773,29 @@ public class SecureAnnotationsInterceptorTest { String[] value(); } + @Retention (RetentionPolicy.RUNTIME) + @Target({ElementType.TYPE, ElementType.METHOD }) + public @interface SingleRole { + String value(); + } + + @Retention (RetentionPolicy.RUNTIME) + @Target({ElementType.TYPE, ElementType.METHOD }) + public @interface IntValueRoles { + int value(); + } + + @Retention (RetentionPolicy.RUNTIME) + @Target({ElementType.TYPE, ElementType.METHOD }) + public @interface NoValueRoles { + } + + public static class SingleRoleService { + @SingleRole("admin") + public void operation() { + } + } + private static final class TestService { @SecureRolesAllowed("testRole") public void echo() {
