[
https://issues.apache.org/jira/browse/KNOX-2731?focusedWorklogId=755981&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-755981
]
ASF GitHub Bot logged work on KNOX-2731:
----------------------------------------
Author: ASF GitHub Bot
Created on: 12/Apr/22 19:22
Start Date: 12/Apr/22 19:22
Worklog Time Spent: 10m
Work Description: 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?
Issue Time Tracking
-------------------
Worklog Id: (was: 755981)
Time Spent: 20m (was: 10m)
> Allow group membership information to be included in issued JWTs
> ----------------------------------------------------------------
>
> Key: KNOX-2731
> URL: https://issues.apache.org/jira/browse/KNOX-2731
> Project: Apache Knox
> Issue Type: Improvement
> Reporter: Attila Magyar
> Assignee: Attila Magyar
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
