lmccay commented on code in PR #1219:
URL: https://github.com/apache/knox/pull/1219#discussion_r3189553637
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java:
##########
@@ -361,12 +361,15 @@ private Pair<TokenType, String>
parseFromHTTPBasicCredentials(final String heade
String passcode = values[1].isEmpty() ? null : values[1];
if (TOKEN.equalsIgnoreCase(username) ||
PASSCODE.equalsIgnoreCase(username)) {
parsed = Pair.of(TOKEN.equalsIgnoreCase(username) ? TokenType.JWT :
TokenType.Passcode, passcode);
- } else if (request != null &&
CLIENT_CREDENTIALS.equals(request.getParameter(GRANT_TYPE))) {
- // Allow client_credentials flow where client_id/client_secret are
provided via HTTP Basic
- if (passcode != null) {
- validateClientID(username, passcode);
- parsed = Pair.of(TokenType.Passcode, passcode);
- }
+ } else if (request != null) {
+ HttpServletRequest unwrappedRequest =
ServletRequestUtils.unwrapHttpServletRequest(request);
Review Comment:
This touches on something that I've been discussing with @pzampino offline
about. It is really not clear why we hide these params in the first place. I
know that once you read them, the input stream has been opened and it can't be
done again, so it breaks certain usecases. In cases where we fully know that we
aren't passing it on to someone else to use, this is okay. These OAuth flows
are an example of when its okay. So, I have 2 reactions to your question:
1. A convenience method to get the wrapped param would be fine but should
make it clear that it is unwrapping with the name - getWrappedRequestParam. The
method you have above would maybe return a query param or a hidden request body
param. That would potentially break usecases without knowing it. Cracking open
the request body should be an explicit action unless/until we remove the hiding
of the params.
2. The fact that you've asked AND that you had to do the same elsewhere may
be another smell of this implementation and maybe we shouldn't hide them at all.
Bottomline: if we keep them hidden then the interface should make it very
clear what you are opting into. Additionally, we should check whether we really
need to hide them. The unwrapping code will still work if we stop hiding them
anyway.
Thoughts?
--
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]
