coheigea commented on code in PR #3627:
URL: https://github.com/apache/cxf/pull/3627#discussion_r4372240446
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilter.java:
##########
@@ -78,11 +91,16 @@ protected void checkSecureClass(Class<?> cls) {
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 = 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());
+ if (methodScopes != null) {
+ methodScopesBySignature.putIfAbsent(signature, methodScopes);
+ annotationScopesByName.computeIfAbsent(m.getName(), k -> new
HashSet<>()).add(methodScopes);
Review Comment:
Each overload gets its own rule: annotation scopes are stored per signature
in methodScopesBySignature and looked up by the invoked method’s signature
first, so foo() and foo(arg1) each enforce only their own @Scopes.
annotationScopesByName (this line) is only a fallback for when the invoked
method’s signature wasn’t recorded — e.g. an implementation of a generic
interface method, whose erased signature differs — and then all rules for that
name must be satisfied. I’ll add a same-class overload test and a comment here
to make that clearer.
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilter.java:
##########
@@ -113,27 +131,72 @@ protected void checkClient(Method m) {
}
}
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 = 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;
+ }
+ private static String getMethodSignature(Method m) {
Review Comment:
Yep good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
