Link internal OIDC ID token and UserInfo when working with GitHub and other OAuth2 providers

[sberyozkin]

Description

Right now, when GitHub or other non-OIDC compliant provider is used which returns no ID token, Quarkus OIDC will generate an internal empty ID token and will enforce that UserInfo is fetched.

Dealing with UserInfo will be more optimal if it is cached, ex, by the default UserInfo cache. However, it may be reasonable instead to use this internal IdToken to retain UserInfo, something @FroMage is interested in - this IdToken is faked so we can put any claims there and then it will keep this UserInfo as a cookie value.

Implementation ideas

Store UserInfo as a complex userinfo claim in the internal IdToken.

We should probably also encrypt the internal token instead in this case - UserInfo may contain more sensitive information

[quarkus-bot]

/cc @pedroigor

[FroMage]

Well no, I only want to be allowed to use the UserInfo to add claims to the generated JWT. I only need to use the id from the UserInfo to add as a name claim on the JWT. I don't need the whole thing to be stored.

[sberyozkin]

The whole UserInfo would be 3 or so simple properties. I can imagine how we can put it as the whole, but I'm not sure it would be worth trying to introduce some extra interfaces allowing users to intercept the faked ID token creation :-), IMHO it would not be worth it in order to do some extra optimization - given the current cache approach works fine

[FroMage]

Well, I don't want the UserInfo to remain after the initial OIDC callback. I've no use for it. I don't want to fetch it on every subsequent request, and I don't want to cache it. I want it gone.

But I do need to be able to use it as a source for what's missing in the IdToken and in the JWT. If you're going to create a fake JWT IdToken (and I think that's a fine thing to have done) then at least let me derive what to add to it.

Ideally I'd at least be able to set the name claim, since that's what's required for the SecurityIdentity:

    @OidcUserInfoMapper
    public void mapIt(OidcTenantConfig tenantConfig, JwtClaimsBuilder builder, UserInfo info) {
        // only do this for github and facebook
        if("github".equals(tenantConfig.tenantId.get())
                || "facebook".equals(tenantConfig.tenantId.get())) {
            JsonValue id = (JsonValue) userInfo.get("id");
            String idString;
            switch(id.getValueType()) {
            case NUMBER:
                idString = Long.toString(((JsonNumber)id).longValue());
                break;
            case STRING:
                idString = ((JsonString)id).getString();
                break;
            default:
                // bail out
                throw new RuntimeException("Don't know how to handle userinfo id: "+id);
            }

            builder.claim(Claims.upn.name(), idString);
            
            if("github".equals(tenantConfig.tenantId.get())) {
                builder.claim(Claims.full_name, userInfo.getString("name"));
                builder.claim(Claims.preferred_username, userInfo.getString("login"));
            }
            if("facebook".equals(tenantConfig.tenantId.get())) {
                builder.claim(Claims.given_name, userInfo.getString("first_name"));
                builder.claim(Claims.family_name, userInfo.getString("last_name"));
                builder.claim(Claims.email, userInfo.getString("email"));
            }
        }
    }

This allows users to have a single endpoint for all oidc callbacks, and will only leave a single special-case for github because we need the access token to obtain the user's email via another endpoint. Well, I guess we could support it here too by making mapIt return Uni<Void> and doing the call, but I'm not sure it's worth complicating this further.

If I can't stick that name claim in the generated IdToken, then I need to reassociate it on every request with a SecurityIdentityAugmenter:

@ApplicationScoped
public class OidcSecurityAugmentor implements SecurityIdentityAugmentor {

    @Override
    public Uni<SecurityIdentity> augment(SecurityIdentity identity, AuthenticationRequestContext context) {
        if(identity.isAnonymous())
            return Uni.createFrom().item(identity);
        try {
            DefaultJWTCallerPrincipal oldPrincipal = (DefaultJWTCallerPrincipal) identity.getPrincipal();
            // only do this for github and facebook
            if("github".equals(oldPrincipal.getIssuer())
                    || "facebook".equals(oldPrincipal.getIssuer())) {
                String rawToken = oldPrincipal.getClaim(Claims.raw_token);
                JwtClaims claims;
                claims = new JwtClaims();
                for (String claim : oldPrincipal.getClaimNames()) {
                    claims.setClaim(claim, oldPrincipal.getClaim(claim));
                }
                UserInfo userInfo = identity.getAttribute(OidcUtils.USER_INFO_ATTRIBUTE);
                JsonValue id = (JsonValue) userInfo.get("id");
                String idString;
                switch(id.getValueType()) {
                case NUMBER:
                    idString = Long.toString(((JsonNumber)id).longValue());
                    break;
                case STRING:
                    idString = ((JsonString)id).getString();
                    break;
                default:
                    // bail out
                    throw new RuntimeException("Don't know how to handle userinfo id: "+id);
                }
                claims.setClaim(Claims.upn.name(), idString);
                QuarkusSecurityIdentity newIdentity = QuarkusSecurityIdentity.builder(identity).setPrincipal(new DefaultJWTCallerPrincipal(rawToken, "JWT", claims)).build();
                return Uni.createFrom().item(newIdentity);
            } else {
                return Uni.createFrom().item(identity);
            }
        }catch(Throwable t) {
            t.printStackTrace();
            throw t;
        }
    }

}

And this is crazy because it forces me to hang on to the UserInfo for every request, even after authenticated, when I only need to tell Quarkus where the damn name claim is.

[sberyozkin]

@FroMage I'm sorry that you want it gone but it will stay injected in the faked token, I've explained why here:

#22192 (comment).

Even injecting it will still be a hack on to of another hack - I do all I can to help you have a simpler integration code.
I don't think your code above is optimal, I've already told you, you can just get UserInfo as a SecurityIdentity attribute without parsing anything and just work against a typed UserInfo

[maxandersen]

@sberyozkin won't call to UserInfo require a call back to github ? that seems wasteful. Github isn't like a small irrelevant service - why not add some relevant user info into that fake generated id anyhow and avoid users to do custom stuff just for github ?

[sberyozkin]

@maxandersen I've missed your comment, sorry.
Here is how it works in general in OIDC Code flow: after the user has authenticated both ID token and access tokens are returned. ID Token usually contains the required user info claims, but sometimes not - when a call back to the Userinfo endpoint is required.
Github is not even an OIDC provider, so quarkus-oidc tries to deal with it as if it were the one.
The problem here is not about doing a callback - it has to be done anyway at least once, for GitHub, Keycloak , etc. It is about the proper way of caching UserInfo data to avoid calling back on every request when UserInfo is required.
quarkus-oidc offers a feature allowing to cache it. Steph does not want to use it, and instead get some of the the claims from UserInfo added to that fake ID token. I'm OK with this optimization, except that I'd indeed prefer to get all of UserInfo cached in that fake token as opposed to a non-generic solution. JSON-wise, the difference would be negligible if the whole UserInfo is cached