Resolve generic overrides exactly in SecureAnnotationsInterceptor

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 @@ private void resolveSecuredObjectRoles() {
             return;
         }
         declarationsBySignature.clear();
+        overriddenSignatures.clear();
         signaturesByName.clear();
         securedClasses.clear();
         for (Class<?> cls : securedObjectClasses) {
@@ -152,6 +156,12 @@ protected void findRoles(Class<?> cls, Map<String, String> rolesMap) {
                 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 @@ private List<List<String>> getRoleRules(Method method) {
     }
 
     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) {

core/src/test/java/org/apache/cxf/interceptor/security/SecureAnnotationsInterceptorTest.java

@@ -351,11 +351,61 @@ public void testMethodRolesFromUnrelatedSupertypesApplyTogether() throws Excepti
     }
 
     @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 void update() {
         }
     }
 
+    @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() {
         }