You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/09/06 17:02:24 UTC
[GitHub] [guacamole-client] mildis opened a new pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
mildis opened a new pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-728798131
> Good with me. @mike-jumper, anything else you want to see fixed up?
I've found some problematic logic within `validateToken()` that should be addressed (above review), but everything else looks good to me.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r515491002
##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/AuthenticationProviderService.java
##########
@@ -91,13 +93,19 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throws GuacamoleException {
String username = null;
+ Set<String> groups = null;
// Validate OpenID token in request, if present, and derive username
HttpServletRequest request = credentials.getRequest();
if (request != null) {
String token = request.getParameter(TokenField.PARAMETER_NAME);
- if (token != null)
- username = tokenService.processUsername(token);
+ if (token != null) {
+ JwtClaims claims = tokenService.validateToken(token);
+ if ( null != claims ) {
Review comment:
A couple of issues with this one line:
* From a style perspective, there are extra spaces: `if (null != claims)`
* I'm not sure why the sudden switch to putting `null` as the first part of the condition - generally throughout the code we put the variable, first.
##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +122,85 @@ public String processUsername(String token) throws GuacamoleException {
logger.debug("Malformed claim within received JWT.", e);
}
+ return claims;
+ }
+
+ /**
+ * Parses the given JwtClaims, returning the username contained
+ * therein, as defined by the username claim type given in
+ * guacamole.properties. If the username claim type is missing or
+ * is invalid, null is returned.
+ *
+ * @param claims
+ * A valid JwtClaims to extract the username from.
+ *
+ * @return
+ * The username contained within the given JwtClaims, or null if the
+ * claim is not valid or the username claim type is missing,
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public String processUsername(JwtClaims claims) throws GuacamoleException {
+ String usernameClaim = confService.getUsernameClaimType();
+
+ 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;
-
}
+ /**
+ * Parses the given JwtClaims, returning the groups contained
+ * therein, as defined by the groups claim type given in
+ * guacamole.properties. If the groups claim type is missing or
+ * is invalid, an empty set is returned.
+ *
+ * @param claims
+ * A valid JwtClaims to extract groups from.
+ *
+ * @return
+ * A Set of String representing the groups the user is member of
+ * from the OpenID provider point of view, or an empty Set if
+ * claim is not valid or the groups claim type is missing,
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public Set<String> processGroups(JwtClaims claims) throws GuacamoleException {
+ String groupsClaim = confService.getGroupsClaimType();
+
+ if ( null != claims ) {
Review comment:
And here...
##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +122,85 @@ public String processUsername(String token) throws GuacamoleException {
logger.debug("Malformed claim within received JWT.", e);
}
+ return claims;
+ }
+
+ /**
+ * Parses the given JwtClaims, returning the username contained
+ * therein, as defined by the username claim type given in
+ * guacamole.properties. If the username claim type is missing or
+ * is invalid, null is returned.
+ *
+ * @param claims
+ * A valid JwtClaims to extract the username from.
+ *
+ * @return
+ * The username contained within the given JwtClaims, or null if the
+ * claim is not valid or the username claim type is missing,
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public String processUsername(JwtClaims claims) throws GuacamoleException {
+ String usernameClaim = confService.getUsernameClaimType();
+
+ if ( null != claims ) {
Review comment:
Similar to above comment:
* Remove extra spaces
* Re-order the comparison
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-731358236
Eek...I think we're going the opposite direction. Now the only commit I see is the Portuguese translation one - I don't see the group changes at all, anymore.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r484381427
##########
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:
At first I tough of maintaining a `synchronizedSet` of valid JWT (via `claims.getJwtId()`) in `validateToken(String)` and bypassing the whole validation if the JWT id was part of the Set.
However, as every JWT has its own lifespan, it would involved lots of housekeeping of a time-based cache.
And being able to call `getJwtId()` to search in the already validated Set means that most of the validation process has been done (create the `JwtConsumer` and calling `processToClaims()` the be able to get the `jit` claim).
My bet is that it's not worth it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r528175729
##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -55,24 +59,24 @@
@Inject
private NonceService nonceService;
+ private static final String MSG_REJECTED_TOKEN = "Rejected OpenID token with malformed claim: {}";
+ private static final String MSG_MALFORMED_CLAIM = "Malformed claim within received JWT.";
Review comment:
What's the reasoning behind moving these log messages into constants?
I'm wary of this because:
* The usage of `MSG_REJECTED_TOKEN` must be aware of the fact that it uses parameter substitution with `{}` (as well as the context of that `{}`). Likewise, the usage of `MSG_MALFORMED_CLAIM` must be aware that parameter substitution is _not_ used.
* The constants seem unlikely to be reused.
* The human-readable message can sometimes be useful when reading the source, as they provide context, but they are no longer readily readable in the vicinity of the relevant 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r485064975
##########
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:
@mike-jumper Should I mark this one solved ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-689198219
Please also check your commit messages - they currently look like:
All messages should start with the relevant JIRA issue and describe the high-level nature of the change being made. A message like "add JavaDoc" is missing the JIRA issue and a bit vague.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-731389802
@mildis No worries, commit looks good, now, and I've gone back and reviewed the changes. As long as @mike-jumper is good with them, I think we can likely move forward with merge.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r524993693
##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -88,12 +89,12 @@ public String processUsername(String token) throws GuacamoleException {
.setVerificationKeyResolver(resolver)
.build();
- try {
+ JwtClaims claims = null;
Review comment:
The logic:
```java
JwtClaims claims = null;
try {
claims = somethingElse;
doProcessing(claims);
doMoreProcessing(claims);
}
catch (VariousExceptions e) {
logger.debug("Yikes.", e);
}
return claims;
```
is dangerous because `claims` might become non-null yet still fail processing. This function would then effectively pretend to have succeeded, authenticating a user that is not actually properly authenticated.
Consider:
1. `claims` is initialized to `null`
2. `claims` is set to the result of `jwtConsumer.processToClaims(token)`
3. The call to `claims.getStringClaimValue()` fails with `MalformedClaimException`
4. Details of the exception are logged, but code just falls through to `return claims`, incorrectly returning a non-null value.
Even if the logic can be massaged to avoid this, or it can be shown that the above specific circumstance cannot occur, it's dangerous to maintain this form as it would be too easy for future changes in this area to introduce such a problem.
I recommend adopting the same form as used below in `processUsername()` - returning `claims` _only_ at the end of the `try`, with the final part of the function being the `return null` for what is then known to be a pure failure case.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper merged pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mike-jumper merged pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r519701967
##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +122,85 @@ public String processUsername(String token) throws GuacamoleException {
logger.debug("Malformed claim within received JWT.", e);
}
+ return claims;
+ }
+
+ /**
+ * Parses the given JwtClaims, returning the username contained
+ * therein, as defined by the username claim type given in
+ * guacamole.properties. If the username claim type is missing or
+ * is invalid, null is returned.
+ *
+ * @param claims
+ * A valid JwtClaims to extract the username from.
+ *
+ * @return
+ * The username contained within the given JwtClaims, or null if the
+ * claim is not valid or the username claim type is missing,
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties could not be parsed.
+ */
+ public String processUsername(JwtClaims claims) throws GuacamoleException {
+ String usernameClaim = confService.getUsernameClaimType();
+
+ if ( null != claims ) {
Review comment:
Done.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-730042683
@mildis, it looks like several other unrelated changes made their way into this as a result of a rebase. Did you perchance rebase against `master` instead of `staging/1.3.0`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-729763656
@mike-jumper is this ok for you ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-730351104
Maybe do a "git rebase -i <COMMIT BEFORE 71f4821>" and remove that commit, then force-push?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-731367437
OK.
Do you want the patch on top of master cbcac3a5d or staging/1.3.0 c4b9b0173 ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] knacktim commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
knacktim commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r484444952
##########
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:
This can be configured on the oidc implementation side. For example, in Keycloak you can configure it to just return the email and username in the token.
Edit: This means you are correct. We may just need to introduce a configuration options that lists out what the expected claims are, and put this method call behind that.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] knacktim commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
knacktim commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r484444952
##########
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:
This can be configured on the oidc implementation side. For example, in Keycloak you can configure it to just return the email and username in the token.
Edit: We may just need to introduce a configuration options that lists out what the expected claims are, and put this method call behind that.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r485255690
##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +124,89 @@ public String processUsername(String token) throws GuacamoleException {
logger.debug("Malformed claim within received JWT.", e);
}
+ return claims;
Review comment:
Please correct the indentation here. The Guacamole source uses 4 spaces for each level and no tabs.
##########
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:
Any reason to not just pass the `JwtClaims` in for `processUsername()` and `processGroups()` rather than repeatedly processing the token?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] knacktim commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
knacktim commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r484444952
##########
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:
This can be configured on the oidc implementation side. For example, in Keycloak you can configure it to just return the email and username in the token.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r484477298
##########
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:
Not sure that adding a new configuration option is the way to go.
As a sysadmin, I would either rely on basic scopes and add the groups claim to the profile scope or create a dedicated 'groups' scope which would add the groups claim to the ID token when requested.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-731384252
Here you are.
Sorry for the mess.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-731398545
The only minor change I’ve introduced is replacing 3x2 String by their final static equivalent to conform with SonarQube recommendations
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-731368096
@mildis `staging/1.3.0`, as this is slated to go into the 1.3.0 release.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#issuecomment-730232975
I did rebase on master previously.
Here it is rebased on staging/1.3.0 but GUACAMOLE-1207 has been included, don't know why.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r484381427
##########
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:
At first I tough of maintaining a `synchronizedSet` of valid JWT (via `claims.getJwtId()`) in `validateToken(String)` and bypassing the whole validation is the JWT id was part of the Set.
However, as every JWT has its own lifespan, it would involved lots of housekeeping of a time-based cache.
And being able to call `getJwtId()` to search in the already validated Set means that most of the validation process has been done (create the `JwtConsumer` and calling `processToClaims()` the be able to get the `jit` claim).
My bet is that it's not worth it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [guacamole-client] mildis commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token
Posted by GitBox <gi...@apache.org>.
mildis commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r528177013
##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -55,24 +59,24 @@
@Inject
private NonceService nonceService;
+ private static final String MSG_REJECTED_TOKEN = "Rejected OpenID token with malformed claim: {}";
+ private static final String MSG_MALFORMED_CLAIM = "Malformed claim within received JWT.";
Review comment:
The rational is that multiple occurences of the same String should be "factorized" so it is all consistent throughout its use.
I can revert the previous state if you prefer.
----------------------------------------------------------------
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:
users@infra.apache.org