coheigea commented on code in PR #3603:
URL: https://github.com/apache/cxf/pull/3603#discussion_r4322088431
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthRequestFilter.java:
##########
@@ -236,6 +238,17 @@ protected boolean checkRequestURI(HttpServletRequest
request, List<String> uris,
}
return foundValidScope;
}
+
+ private boolean hasAllRequiredScopes(List<OAuthPermission>
matchingPermissions) {
Review Comment:
Two further points please:
1. A subclass can’t see the required scopes (OAuthRequestFilter.java:83).
requiredScopes is private and there’s only a setter (setRequiredScopes, line
383), no getter. A subclass that overrides hasAllRequiredScopes to do pattern
matching has nothing to match against. Its only option is to also override
setRequiredScopes and keep its own copy of the list. Either of these would fix
it:
add protected List<String> getRequiredScopes(), or
give the method the list as a parameter: hasAllRequiredScopes(List<String>
requiredScopes, List<OAuthPermission> matchingPermissions).
Overriding checkScopeProperty has the same problem today, so a getter would
help both.
2. Javadoc (minor). The method is now an extension point, so the comment
inside it could become a short Javadoc. It should say what it receives (only
the permissions that passed the URI, HTTP-method and checkScopeProperty checks)
and that subclasses overriding checkScopeProperty to match loosely should
override this method as well. The current comment about duplicates can stay.
--
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]
