smolnar82 commented on code in PR #876:
URL: https://github.com/apache/knox/pull/876#discussion_r1519517586
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java:
##########
@@ -238,10 +241,33 @@ public Pair<TokenType, String> getWireToken(final
ServletRequest request) {
}
}
+ /*
+ POST /{tenant}/oauth2/v2.0/token HTTP/1.1
+ Host: login.microsoftonline.com:443
+ Content-Type: application/x-www-form-urlencoded
+
+ client_id=535fb089-9ff3-47b6-9bfb-4f1264799865
+ &scope=https%3A%2F%2Fgraph.microsoft.com%2F.default
+ &client_secret=sampleCredentials
+ &grant_type=client_credentials
+ */
+
+ // Let's check whether this is a client credentials oauth request or
whether
+ // the token has been configured for another usecase specific header
if (parsed == null) {
- token = request.getParameter(this.paramName);
- if (token != null) {
- parsed = Pair.of(TokenType.JWT, token);
+ String grantType = request.getParameter(GRANT_TYPE);
+ if (CLIENT_CREDENTIALS.equals(grantType)) {
+ // this is indeed a client credentials flow client_id and
+ // client_secret are expected now the client_id will be in
+ // the token as the token_id so we will get that later
+ token = request.getParameter(CLIENT_SECRET);
+ parsed = Pair.of(TokenType.Passcode, token);
+ }
Review Comment:
I think this code - along with the above comment that explains the different
query parameters in a client credentials flow request - deserves to be in a
private method and can be added before the original `parsed == null` check.
This way it's easier to read, IMO.
For instance:
```
// Let's check whether this is a client credentials oauth request
parsed = parsed == null ? parseOauthRequestClientCredentials(request) :
parsed;
// Finally, whether the token has been configured for another usecase
specific header
if (parsed == null) {
...
```
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java:
##########
@@ -300,8 +302,23 @@ protected Subject createSubjectFromToken(final JWT token)
throws UnknownTokenExc
public Subject createSubjectFromTokenIdentifier(final String tokenId) throws
UnknownTokenException {
TokenMetadata metadata = tokenStateService.getTokenMetadata(tokenId);
+ String username = null;
if (metadata != null) {
- return createSubjectFromTokenData(metadata.getUserName(), null);
+ String type = metadata.getMetadata(TYPE);
Review Comment:
As you indicated in the PR's description,
> This change introduces only the acceptance of the Knox TokenID and
Passcode tokens as CLIENT_ID and CLIENT_SECRET in a standard OAuth 2.0 client
credentials flow request body
Based on this, I've to ask if there is going to be a separate PR where the
`type` metadata is created when OAuth clients get a new token (and replay the
`tokenId` and `passcode` fields as `CLIENT_ID` and `CLIENT_SECRET`)?
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java:
##########
@@ -300,8 +302,23 @@ protected Subject createSubjectFromToken(final JWT token)
throws UnknownTokenExc
public Subject createSubjectFromTokenIdentifier(final String tokenId) throws
UnknownTokenException {
TokenMetadata metadata = tokenStateService.getTokenMetadata(tokenId);
+ String username = null;
if (metadata != null) {
- return createSubjectFromTokenData(metadata.getUserName(), null);
+ String type = metadata.getMetadata(TYPE);
+ // using tokenID and passcode as CLIENT_ID and CLIENT_SECRET will
+ // result in a metadata item called "type". If the valid is set
Review Comment:
I'm not sure I understand this sentence. Did you mean "if the `type` is set
to CLIENT_ID"?
--
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]
