You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "mildis (via GitHub)" <gi...@apache.org> on 2023/08/10 11:33:49 UTC
[GitHub] [guacamole-client] mildis opened a new pull request, #905: GUACAMOLE-1844 : OIDC JWT claims as user token
mildis opened a new pull request, #905:
URL: https://github.com/apache/guacamole-client/pull/905
This patch allows IDP to send JWT claims that can be mapped to user tokens, prefixed with OIDC_.
Same case transormation apply than LDAP_ and CAS_.
Define openid-attributes-claim-type with a comma-separated list of claims that should be mapped.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on a diff in pull request #905: GUACAMOLE-1844 : OIDC JWT claims as user token
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #905:
URL: https://github.com/apache/guacamole-client/pull/905#discussion_r1313522024
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -125,6 +133,15 @@ public class ConfigurationService {
public String getName() { return "openid-groups-claim-type"; }
};
+ /**
+ * The claims within any valid JWT that should be mapped to
+ * the authenticated user's tokens, as configured with guacamole.properties.
+ */
+ private static final StringListProperty OPENID_ATTRIBUTES_CLAIM_TYPE =
+ new StringListProperty() {
+ @Override
+ public String getName() { return "openid-attributes-claim-type"; }
+ };
Review Comment:
This is a very minor thing, but please follow the established style throughout the prior property declarations, particularly as it relates to indentation.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java:
##########
@@ -201,4 +209,53 @@ public Set<String> processGroups(JwtClaims claims) throws GuacamoleException {
// Could not retrieve groups from JWT
return Collections.emptySet();
}
+
+ /**
+ * Parses the given JwtClaims, returning the attributes contained
+ * therein, as defined by the attributes claim type given in
+ * guacamole.properties. If the attributes claim type is missing or
+ * is invalid, an empty set is returned.
+ *
+ * @param claims
+ * A valid JwtClaims to extract attributes from.
+ *
+ * @return
+ * A Map of String,String representing the attributes and values
+ * from the OpenID provider point of view, or an empty Map if
+ * claim is not valid or the attributes claim type is missing.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public Map<String, String> processAttributes(JwtClaims claims) throws GuacamoleException {
+ List<String> attributesClaim = confService.getAttributesClaimType();
+
+ if (claims != null && !attributesClaim.isEmpty()) {
+ try {
+ logger.debug("Iterating over attributes claim list : {}", attributesClaim);
+ Map<String, String> tokens = new HashMap<String, String>(attributesClaim.size());
+ for (String key: attributesClaim) {
+ String oidcAttr = claims.getStringClaimValue(key);
+ if (oidcAttr != null && !oidcAttr.isEmpty()) {
+ String tokenName = TokenName.canonicalize(key, OIDC_ATTRIBUTE_TOKEN_PREFIX);
+ tokens.put(tokenName, oidcAttr);
+ logger.debug("Claim {} found and set to {} as {}", key, tokenName, oidcAttr);
+ }
+ else {
+ logger.debug("Claim {} not found in JWT.", key);
+ }
+ }
+ logger.debug("Processed attributes map : {}", tokens);
+ return Collections.unmodifiableMap(tokens);
+ }
+ catch (MalformedClaimException e) {
+ logger.info("Rejected OpenID token with malformed claim: {}", e.getMessage());
+ logger.debug("Malformed claim within received JWT.", e);
+ }
+ }
Review Comment:
It might be good to have a few inline comments in this section to document what you're doing. The `logger.debug()` statements kind of accomplish that, but if you look at the methods above you'll see there are at least some one-line comments to give hints for someone reading through the code.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #905: GUACAMOLE-1844 : OIDC JWT claims as user token
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on code in PR #905:
URL: https://github.com/apache/guacamole-client/pull/905#discussion_r1290301491
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -126,6 +128,12 @@ public class ConfigurationService {
};
+ private static final StringGuacamoleProperty OPENID_ATTRIBUTES_CLAIM_TYPE =
+ new StringGuacamoleProperty() {
+ @Override
+ public String getName() { return "openid-attributes-claim-type"; }
+ };
Review Comment:
It would be better to use `StringListProperty` for this, rather than manually parse the string into an array each time a claim is being handled.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java:
##########
@@ -201,4 +214,53 @@ public Set<String> processGroups(JwtClaims claims) throws GuacamoleException {
// Could not retrieve groups from JWT
return Collections.emptySet();
}
+
+ /**
+ * Parses the given JwtClaims, returning the attributes contained
+ * therein, as defined by the attributes claim type given in
+ * guacamole.properties. If the attributes claim type is missing or
+ * is invalid, an empty set is returned.
+ *
+ * @param claims
+ * A valid JwtClaims to extract groups from.
+ *
+ * @return
+ * A Map of String,String representing the attributes and values
+ * from the OpenID provider point of view, or an empty Map if
+ * claim is not valid or the attributes claim type is missing.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public Map<String, String> processAttributes(JwtClaims claims) throws GuacamoleException {
+ String attributesClaim = confService.getAttributesClaimType();
+
+ if (claims != null && !attributesClaim.isEmpty()) {
+ try {
+ String[] attrs = DELIMITER_PATTERN.split(attributesClaim);
+ logger.debug("Iterating over attributes claim list : {}", attrs.toString());
+ Map<String, String> tokens = new HashMap<String, String>(attrs.length);
+ for (String key: attrs) {
+ String oidcAttr = claims.getStringClaimValue(key);
+ if (oidcAttr != null && !oidcAttr.isEmpty()) {
+ String tokenName = TokenName.canonicalize(key, OIDC_ATTRIBUTE_TOKEN_PREFIX);
+ tokens.put(tokenName, oidcAttr.toString());
+ logger.debug("Claim {} found and set to {} as {}", key, tokenName, oidcAttr.toString());
+ } else {
Review Comment:
Please do not cuddle the `else`. See established code and: https://guacamole.apache.org/guac-style/#braces
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -326,6 +334,10 @@ public String getGroupsClaimType() throws GuacamoleException {
return environment.getProperty(OPENID_GROUPS_CLAIM_TYPE, DEFAULT_GROUPS_CLAIM_TYPE);
}
+ public String getAttributesClaimType() throws GuacamoleException {
Review Comment:
Same here - please document.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -45,6 +45,8 @@ public class ConfigurationService {
*/
private static final String DEFAULT_GROUPS_CLAIM_TYPE = "groups";
+ private static final String DEFAULT_ATTRIBUTES_CLAIM_TYPE = "";
Review Comment:
And here (JavaDoc).
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -126,6 +128,12 @@ public class ConfigurationService {
};
+ private static final StringGuacamoleProperty OPENID_ATTRIBUTES_CLAIM_TYPE =
Review Comment:
All newly-added functions, member variables, etc. need to be documented with JavaDoc comments (see surrounding code).
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1844 : OIDC JWT claims as user token [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on PR #905:
URL: https://github.com/apache/guacamole-client/pull/905#issuecomment-1846505103
Sorry - GitHub auto-closed this by virtue of the `staging/1.5.4` branch being deleted in preparation for the 1.5.4 release. This was not an intentional closure.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1844 : OIDC JWT claims as user token [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #905:
URL: https://github.com/apache/guacamole-client/pull/905#issuecomment-1888338966
@mildis Updated this to the master branch, since I don't think this is really a bug fix, but should be included in the next feature release (1.6.0).
Please also note there are now merge conflicts that need to be resolved.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1844 : OIDC JWT claims as user token [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper closed pull request #905: GUACAMOLE-1844 : OIDC JWT claims as user token
URL: https://github.com/apache/guacamole-client/pull/905
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1844 : OIDC JWT claims as user token [guacamole-client]
Posted by "mildis (via GitHub)" <gi...@apache.org>.
mildis closed pull request #905: GUACAMOLE-1844 : OIDC JWT claims as user token
URL: https://github.com/apache/guacamole-client/pull/905
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org