broker: use /userinfo as fallback for claims not available in id token

[shiv-tyagi]

Fixes #1440.

This uses the information retrieved from /userinfo endpoint as a fallback for claims not available in id token.

This only affects the generic oidc broker, for msentraid, we simple drop the information retrieved from /userinfo.

I have verified both oidc and msentraid brokers with these changes, they both work fine.

[codecov]

Codecov Report

❌ Patch coverage is 77.50000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.22%. Comparing base (63aa65e) to head (06e3b78).

Files with missing lines	Patch %	Lines
...ernal/providers/genericprovider/genericprovider.go	40.00%	6 Missing ⚠️
authd-oidc-brokers/internal/broker/broker.go	88.88%	2 Missing ⚠️
authd-oidc-brokers/internal/providers/info/info.go	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1476      +/-   ##
==========================================
- Coverage   86.93%   80.22%   -6.72%     
==========================================
  Files          93       20      -73     
  Lines        6401     1082    -5319     
  Branches      111        0     -111     
==========================================
- Hits         5565      868    -4697     
+ Misses        780      214     -566     
+ Partials       56        0      -56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[adombeck]

The UserInfo request is one more network request which can go wrong, and it's not needed in most cases, so I think we should not do it unconditionally but only if the ID token is actually missing a required claim. I would implement it like this:

1. First, call b.provider.GetUserInfo(idToken)
2. If the ID token misses a required claim, GetUserInfo should return a MissingClaimError
3. If it does, we fetch the claims from the UserInfo endpoint and merge them with the ones from the ID token into a single claims struct and then call b.provider.GetUserInfo(claims)

[shiv-tyagi]

I don't think it is as simple as that. How do we differentiate if the email_verified claim is missing because it is a thin id token (first call) or if it is really missing (second call)? Currently, the email_verified claim check sits inside GetUserInfo.

If we really want to save a network call, the right way should be, define func IsRequiredClaimMissing(idToken info.Claimer) at provider end, if it returns true, we fetch claims from /userinfo and merge them and then do the GetUserInfo call (only one single time).

We need to decouple the ensuring required claims are present and is user's email verified steps.

[adombeck]

How do we differentiate if the email_verified claim is missing because it is a thin id token (first call) or if it is really missing (second call)?

In my proposal, GetUserInfo returns MissingClaimError in both cases, but the caller knows how to handle it. If GetUserInfo returns a MissingClaimError again after 3. from above then a required claim is still missing in the combined claims from the ID token and UserInfo, so we return an error.

[shiv-tyagi]

That would require us to move the actual "email is not verified" check out of generic provider and return an error while handling the MissingClaimError (form the second call) in broker.go. Does that sound okay?

I was assuming that we wanted to confine the email verified check within the generic broker.

[adombeck]

That would require us to move the actual "email is not verified" check out of generic provider

I don't understand why we would need to do that. I'm on my phone right now so I can't show you the diff of my proposal, but I don't see why we would have to do anything else in broker.go than return an error in the case that GetUserInfo returns a MissingClaimError again after 3. Maybe I'm missing something.

[shiv-tyagi]

Thanks for responding on a weekend @adombeck. Please feel free to check this and reply when you return to work.

if !userClaims.EmailVerified {
    return info.User{}, &providerErrors.ForDisplayError{Message: "Authentication failure: email not verified"}
}

Currently this line sits in in GetUserInfo in genericprovider.go, if I am not getting it wrong, now you are suggesting to just return a MissingClaimError from there.

The GetUserInfo is called from broker.go, so now we would need to handle MissingClaimError in broker.go for email_verified claim after 3 (why specifically for email_verified claim? - to display user friendly error like we do today). And this would apply for all providers (just that entra provider would never return MissingClaimError so that path would never get hit there).

[shiv-tyagi]

@adombeck I pushed some work. It saves a network call by only trying /userinfo if a mandatory claim required for a provider is missing in the id token.

Please check if this is what you were looking for.

I have included tests in my work and have tested this for both msentraid and oidc broker.

Thanks!

[adombeck]

I'm out of office this week, will take a look next week

[yetanotheralex]

Tested this on our side as well with okta and can confirm it's working; would be great to see this merged :)