feat(frontend): Parse JWT access token claims #5138
Conversation
|
|
||
| for (final Entry<String, Object> entry : jwt.getJWTClaimsSet().getClaims().entrySet()) { | ||
| final String claimName = entry.getKey(); | ||
| if (profile.getAttribute(claimName) == null) { |
There was a problem hiding this comment.
Only add claim to profile attribute if it doesn't exist to avoid overriding existing id token claims. Access token and id token will have duplicate standard claim names (see link below) with different values.
With this check in place, maybe we don't need an optional config for a set of claims to filter for? Since access token claims will not override id token claims.
There was a problem hiding this comment.
Yeah this makes sense! I like checking for existence first. I don't foresee cases where you'll want the same claim from 2 tokens, but I suppose in that case we could add some default prefix.
| @Override | ||
| public CommonProfile generate(WebContext context, CommonProfile profile) { | ||
| try { | ||
| final JWT jwt = JWTParser.parse(((OidcProfile) profile).getAccessToken().getValue()); |
There was a problem hiding this comment.
Is there any way to detect whether the access token is a JWT?
Otherwise, we may be logging tons of warnings.
There was a problem hiding this comment.
Only parsing JWT access token if OIDC_EXTRACT_JWT_ACCESS_TOKEN_CLAIMS = true so shouldn't log any warnings. I guess we can depend on users to only enable this feature if they know they want claims from their access token?
| import com.nimbusds.jwt.JWT; | ||
| import com.nimbusds.jwt.JWTParser; | ||
|
|
||
| public class OidcAuthorizationGenerator implements AuthorizationGenerator { |
There was a problem hiding this comment.
Can we name this class to reflect that its related to Parsing access tokens?
There was a problem hiding this comment.
We can also keep this name, but just pass in the "OidcConfigs" object in the constructor, and then decide whether to parse JWT access tokens inside of here!
There was a problem hiding this comment.
Passed in OidcConfigs in constructor
| @@ -25,5 +31,8 @@ protected void clientInit() { | |||
| defaultAuthenticator(new CustomOidcAuthenticator(getConfiguration(), this)); | |||
| defaultProfileCreator(new OidcProfileCreator<>(getConfiguration())); | |||
| defaultLogoutActionBuilder(new OidcLogoutActionBuilder<>(getConfiguration())); | |||
| if (parseJwtAccessTokenEnabled) { | |||
There was a problem hiding this comment.
Instead of doing this if here, I'd recommend always using the OidcAuthorizationGenerator but with the oidc configs passed in!
The OidcAuthorizationGenerator can internally decide what to do based on configs
There was a problem hiding this comment.
Another benefit is you will not need to change the constructor for this class!
| @@ -139,6 +139,7 @@ auth.oidc.responseMode = ${?AUTH_OIDC_RESPONSE_MODE} | |||
| auth.oidc.useNonce = ${?AUTH_OIDC_USE_NONCE} | |||
| auth.oidc.customParam.resource = ${?AUTH_OIDC_CUSTOM_PARAM_RESOURCE} | |||
| auth.oidc.readTimeout = ${?AUTH_OIDC_READ_TIMEOUT} | |||
| auth.oidc.parseJwtAccessTokenEnabled = ${?AUTH_OIDC_PARSE_JWT_ACCESS_TOKEN_ENABLED} # Whether to parse claims from JWT access token. Defaults to false. | |||
There was a problem hiding this comment.
minor: we can consider an alternate name here as well:
AUTH_OIDC_EXTRACT_JWT_ACCESS_TOKEN_CLAIMS
But i have no strong preference - up to you!
Checklist