Skip to content

Conversation

sberyozkin
Copy link
Member

Fixes #5829

@sberyozkin
Copy link
Member Author

@pedroigor, @stuartwdouglas please note that setting an issuer property disables the default Vertx validation as per comment at #5829, as the Vertx validation is only centered around the site URL match which can not be sufficient and indeed, going forward it would be good if the users were setting the expected claim values (issuer, audience, etc) themselves.
Ideally we'd get this in for 1.1.0, as it is a basic and isolated new code

@sberyozkin sberyozkin added this to the 1.1.0 milestone Dec 4, 2019
* Configuration how to validate the token claims.
*/
@ConfigItem
Claims claims;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about quarkus.oidc.claims config property. It may clash with the claims request parameter as defined by https://openid.net/specs/openid-connect-core-1_0.html#ClaimsParameter.

Maybe, we could use quarkus.oidc.token.issuer|audience ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor Good point, please believe me a token qualifier was something I thought for a sec or two about as well :-), because we may want to actually have an option to have both id token and access token validated separately, right now this validation will affect either the id token or access token depending on the application type. But in time we may qualify further and introduce an idtoken group if needed.
Recall though we already have an Authentication group which has scopes but will also host more properties related to the redirect, but in any case, it is good to avoid the confusion, so I'll rename shortly
Thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor done

@sberyozkin
Copy link
Member Author

@gsmet Just downloaded the logs, KC tests are good, I can't even see what is failing :-)

@sberyozkin
Copy link
Member Author

@stuartwdouglas @pedroigor please check it again

try {
OidcUtils.validateClaims(config.token, token.accessToken());
} catch (OIDCException e) {
result.completeExceptionally(new AuthenticationFailedException());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should log at debug level or include the cause.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartwdouglas sure, I forgot to update it to include the cause

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Object claimValue = json.getValue(Claims.aud.name());
List<String> audience = Collections.emptyList();
if (claimValue instanceof JsonArray) {
audience = ((JsonArray) claimValue).stream().map(v -> v.toString()).collect(Collectors.toList());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't allow lambda in runtime code, they use up a significant amount of additional memory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@gsmet gsmet dismissed stuartwdouglas’s stale review December 6, 2019 17:18

Comments addressed.

@gsmet gsmet merged commit b362baa into quarkusio:master Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Quarkus OIDC does not work with Auth0

4 participants