pzampino commented on code in PR #554:
URL: https://github.com/apache/knox/pull/554#discussion_r848773721
##########
gateway-release/home/conf/topologies/homepage.xml:
##########
@@ -113,6 +113,10 @@
<name>knox.token.proxyuser.admin.hosts</name>
<value>*</value>
</param>
+ <param>
+ <name>knox.token.include.groups</name>
+ <value>false</value>
Review Comment:
Does the param need to be added here? It should simply default to false.
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/JWTokenAttributesBuilder.java:
##########
@@ -32,6 +34,9 @@ public class JWTokenAttributesBuilder {
private boolean managed;
private String jku;
private String type;
+ private Set<String> groups;
+ private String kid;
+ private String issuer = "KNOXSSO";
Review Comment:
I think this was like this before, but should the issuer really be KNOXSSO
in ALL cases?
##########
gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java:
##########
@@ -228,6 +235,9 @@ public void init() throws AliasServiceException,
ServiceLifecycleException, KeyL
}
}
+ String shouldIncludeGroupsParam =
context.getInitParameter(TOKEN_INCLUDE_GROUPS_IN_JWT);
+ shouldIncludeGroups = shouldIncludeGroupsParam == null ? false :
Boolean.parseBoolean(shouldIncludeGroupsParam);
Review Comment:
It looks like this is defaulting to false, and the explicit false in the
homepage topology should be unnecessary.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java:
##########
@@ -104,26 +103,15 @@ public void setAliasService(AliasService as) {
@Override
public JWT issueToken(JWTokenAttributes jwtAttributes) throws
TokenServiceException {
- String[] claimArray = new String[6];
- claimArray[0] = "KNOXSSO";
- claimArray[1] = jwtAttributes.getUserName();
- claimArray[2] = null;
- if (jwtAttributes.getExpires() == -1) {
- claimArray[3] = null;
- }
- else {
- claimArray[3] = String.valueOf(jwtAttributes.getExpires());
- }
final String algorithm = jwtAttributes.getAlgorithm();
if(SUPPORTED_HMAC_SIG_ALGS.contains(algorithm)) {
- claimArray[4] = null;
- claimArray[5] = null;
+ jwtAttributes.setKid(null);
Review Comment:
Do these need to be explicitly set to null, or could this code be simplified?
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java:
##########
@@ -104,26 +103,15 @@ public void setAliasService(AliasService as) {
@Override
public JWT issueToken(JWTokenAttributes jwtAttributes) throws
TokenServiceException {
- String[] claimArray = new String[6];
Review Comment:
JWTokenAttributes has replaced the claimsArray throughout?
##########
gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java:
##########
@@ -106,6 +111,7 @@ public class TokenResource {
private static final String TOKEN_TTL_PARAM = "knox.token.ttl";
private static final String TOKEN_TYPE_PARAM = "knox.token.type";
private static final String TOKEN_AUDIENCES_PARAM = "knox.token.audiences";
+ private static final String TOKEN_INCLUDE_GROUPS_IN_JWT =
"knox.token.include.groups";
Review Comment:
MINOR: It may be good to define a TOKEN_PARAM_PREFIX="knox.token.", and
reference this from the other config property name values to avoid accidental
variations across these properties. I realize it was this way before your
changes, but while you're here, it may be worth doing.
For example:
private static final String TOKEN_PARAM_PREFIX = "knox.token.";
private static final String TOKEN_TTLE_PARAM = TOKEN_PARAM_PREFIX + "ttl";
private static final String TOKEN_TYPE_PARAM = TOKEN_PARAM_PREFIX + "type";
private static final String TOKEN_AUDIENCES_PARAM = TOKEN_PARAM_PREFIX +
"audiences";
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java:
##########
@@ -369,16 +370,7 @@ protected TokenStateService createTokenStateService()
throws Exception {
/* create a test JWT token */
protected JWT getJWTToken(final long expiry) {
- String[] claims = new String[6];
- claims[0] = "KNOXSSO";
- claims[1] = "[email protected]";
- claims[2] = "https://login.example.com";;
- if(expiry > 0) {
- claims[3] = Long.toString(expiry);
- }
- claims[4] = "E0LDZulQ0XE_otJ5aoQtQu-RnXv8hU-M9U4dD7vDioA";
- claims[5] = null;
- JWT token = new JWTToken("RS256", claims);
+ JWT token = new JWTToken(new
JWTokenAttributesBuilder().setExpires(expiry).setAlgorithm("RS256").build());
Review Comment:
This test is slightly different in that the test token has differing
contents. Has the coverage of this test changed as a result? Or, does it only
reflect the change in handling of claims, whereas now, previously necessary
claims are no longer necessary?
##########
gateway-service-knoxtoken/src/test/java/org/apache/knox/gateway/service/knoxtoken/TokenServiceResourceTest.java:
##########
@@ -1396,8 +1444,8 @@ private Subject createTestSubject(final String username) {
return s;
}
- private static Map<String, String> parseJSONResponse(final String response)
throws IOException {
- return (new ObjectMapper()).readValue(response, new
TypeReference<Map<String, String>>(){});
+ private static Map<String, Object> parseJSONResponse(final String response)
throws IOException {
Review Comment:
Why was this changed to return a Map of Object values? What is the response
including that isn't a String?
--
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]
