mike-jumper commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r484097208
##########
File path:
extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/user/AuthenticatedUser.java
##########
@@ -53,8 +56,9 @@
* @param credentials
* The credentials provided when this user was authenticated.
*/
- public void init(String username, Credentials credentials) {
+ public void init(String username, Credentials credentials, Set<String>
effectiveGroups) {
Review comment:
If adding a new parameter, that parameter needs to be documented within
the JavaDoc.
##########
File path:
extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +108,79 @@ public String processUsername(String token) throws
GuacamoleException {
logger.debug("Malformed claim within received JWT.", e);
}
+
+ return claims;
+ }
+
+ /**
+ * Validates and parses the given ID token, returning the username
contained
+ * therein, as defined by the username claim type given in
+ * guacamole.properties. If the username claim type is missing or the ID
+ * token is invalid, null is returned.
+ *
+ * @param token
+ * The ID token to validate and parse.
+ *
+ * @return
+ * The username contained within the given ID token, or null if the ID
+ * token is not valid or the username claim type is missing,
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public String processUsername(String token) throws GuacamoleException {
+ String usernameClaim = confService.getUsernameClaimType();
+
+ JwtClaims claims = validateToken(token);
+
+ if ( null != claims ) {
+ try {
+ // Pull username from claims
+ String username = claims.getStringClaimValue(usernameClaim);
+ if (username != null)
+ return username;
+ }
+ catch (MalformedClaimException e) {
+ logger.info("Rejected OpenID token with malformed claim: {}",
e.getMessage());
+ logger.debug("Malformed claim within received JWT.", e);
+ }
+
+ // Warn if username was not present in token, as it likely means
+ // the system is not set up correctly
+ logger.warn("Username claim \"{}\" missing from token. Perhaps the
"
+ + "OpenID scope and/or username claim type are "
+ + "misconfigured?", usernameClaim);
+ }
+
// Could not retrieve username from JWT
return null;
-
}
+ public Set<String> processGroups(String token) throws GuacamoleException {
Review comment:
Please document.
##########
File path:
extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -55,24 +59,7 @@
@Inject
private NonceService nonceService;
- /**
- * Validates and parses the given ID token, returning the username
contained
- * therein, as defined by the username claim type given in
- * guacamole.properties. If the username claim type is missing or the ID
- * token is invalid, null is returned.
- *
- * @param token
- * The ID token to validate and parse.
- *
- * @return
- * The username contained within the given ID token, or null if the ID
- * token is not valid or the username claim type is missing,
- *
- * @throws GuacamoleException
- * If guacamole.properties could not be parsed.
- */
- public String processUsername(String token) throws GuacamoleException {
-
+ private JwtClaims validateToken(String token) throws GuacamoleException {
Review comment:
Please document.
##########
File path:
extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/user/AuthenticatedUser.java
##########
@@ -43,6 +44,8 @@
*/
private Credentials credentials;
+ private Set<String> effectiveGroups;
Review comment:
Please document.
##########
File path:
extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +108,79 @@ public String processUsername(String token) throws
GuacamoleException {
logger.debug("Malformed claim within received JWT.", e);
}
+
+ return claims;
+ }
+
+ /**
+ * Validates and parses the given ID token, returning the username
contained
+ * therein, as defined by the username claim type given in
+ * guacamole.properties. If the username claim type is missing or the ID
+ * token is invalid, null is returned.
+ *
+ * @param token
+ * The ID token to validate and parse.
+ *
+ * @return
+ * The username contained within the given ID token, or null if the ID
+ * token is not valid or the username claim type is missing,
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public String processUsername(String token) throws GuacamoleException {
+ String usernameClaim = confService.getUsernameClaimType();
+
+ JwtClaims claims = validateToken(token);
+
+ if ( null != claims ) {
+ try {
+ // Pull username from claims
+ String username = claims.getStringClaimValue(usernameClaim);
+ if (username != null)
+ return username;
+ }
+ catch (MalformedClaimException e) {
+ logger.info("Rejected OpenID token with malformed claim: {}",
e.getMessage());
+ logger.debug("Malformed claim within received JWT.", e);
+ }
+
+ // Warn if username was not present in token, as it likely means
+ // the system is not set up correctly
+ logger.warn("Username claim \"{}\" missing from token. Perhaps the
"
+ + "OpenID scope and/or username claim type are "
+ + "misconfigured?", usernameClaim);
+ }
+
// Could not retrieve username from JWT
return null;
-
}
+ public Set<String> processGroups(String token) throws GuacamoleException {
+ String groupsClaim = confService.getGroupsClaimType();
+
+ JwtClaims claims = validateToken(token);
+
+ if ( null != claims ) {
+ try {
+ // Pull groups from claims
+ List<String> oidcGroups =
claims.getStringListClaimValue(groupsClaim);
+ if (oidcGroups != null && !oidcGroups.isEmpty())
+ return Collections.unmodifiableSet(new
HashSet<>(oidcGroups));
+ }
+ catch (MalformedClaimException e) {
+ logger.info("Rejected OpenID token with malformed claim: {}",
e.getMessage());
+ logger.debug("Malformed claim within received JWT.", e);
+ }
+
+ // Warn if groups was not present in token, as it likely means
+ // the system is not set up correctly
+ logger.warn("Groups claim \"{}\" missing from token. Perhaps the "
+ + "OpenID scope and/or groups claim type are "
+ + "misconfigured?", groupsClaim);
Review comment:
Is expected/standard that a groups claim of some kind will always be
present?
The warning for the username claim is always there because things are
seriously broken if there is no username, but if a groups claim may not be
present under normal, unbroken circumstances, then the conditions surrounding
this warning may need to be different.
##########
File path:
extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +108,79 @@ public String processUsername(String token) throws
GuacamoleException {
logger.debug("Malformed claim within received JWT.", e);
}
+
+ return claims;
+ }
+
+ /**
+ * Validates and parses the given ID token, returning the username
contained
+ * therein, as defined by the username claim type given in
+ * guacamole.properties. If the username claim type is missing or the ID
+ * token is invalid, null is returned.
+ *
+ * @param token
+ * The ID token to validate and parse.
+ *
+ * @return
+ * The username contained within the given ID token, or null if the ID
+ * token is not valid or the username claim type is missing,
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public String processUsername(String token) throws GuacamoleException {
+ String usernameClaim = confService.getUsernameClaimType();
+
+ JwtClaims claims = validateToken(token);
Review comment:
With `validateToken()` being called within both `processUsername()` and
`processGroups()`, I'm concerned about repeating that work. Perhaps things
should be restructured such that this is not called twice?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
