This is an automated email from the ASF dual-hosted git repository.
coheigea pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cxf.git
The following commit(s) were added to refs/heads/main by this push:
new 832021f8e62 Resolve OAuthScopesFilter annotation scopes by method
signature (#3627)
832021f8e62 is described below
commit 832021f8e623721ec9e17e522f173fc915a366ee
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
---
.../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) {
+ }
+ }
+}
