Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: OIDC: Call userinfo if no claims found in id token #5228

Merged

Conversation

cmintey
Copy link
Contributor

@cmintey cmintey commented Mar 17, 2025

What this PR does / why we need it:

Some IdPs don't put user claims into the ID token for enhanced security and instead will return the necessary claims from the UserInfo endpoint. There is usually some setting you can change in the IdP to allow sending the claims in the ID token, so this isn't really an issue. However, it makes sense for Mealie to follow this convention and to pull the user info from the UserInfo endpoint. In order to keep compatibility with existing installs (though it shouldn't be breaking), I've decided to add this as a fallback mechanism. If we can't find the claims we need in the ID token, then we will make the necessary call to the UserInfo endpoint of the IdP to obtain them.

Which issue(s) this PR fixes:

N/A

Testing

Tested manually with Authelia 4.38 and 4.39 as well as Authentik, both with and without the option to include claims in the ID token.
Unfortunately, it seems we cannot set up an E2E test for this as the mock OAuth server we use doesn't allow for returning mock data from the UserInfo endpoint

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@michael-genson
Copy link
Collaborator

Changes LGTM. How much lift would it be to make our own mock server or extend the existing one? If it's too much work I'd be okay with just accepting that we won't test this case, as long as the existing tests are passing (looks like you just pushed a commit to fix those)

@cmintey
Copy link
Contributor Author

cmintey commented Mar 17, 2025

How much lift would it be to make our own mock server or extend the existing one?

Probably would be quite a bit of work to roll our own or extend the existing one. Though we could potentially utilize Authelia (which is what I use in my homelab) as our "mock" server. It's completely configured via YAML and users can also be defined within a file.

Edit: Though with Authelia, we wouldn't be able to test both of these flows without restarting the server or changing the clientId env var in Mealie at runtime

@michael-genson michael-genson enabled auto-merge (squash) March 17, 2025 02:55
Copy link
Collaborator

@michael-genson michael-genson left a comment

Choose a reason for hiding this comment

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

That's alright, I think this is fine as-is. If it becomes a bigger problem later down the line maybe we can consider it

@michael-genson michael-genson merged commit d724f40 into mealie-recipes:mealie-next Mar 17, 2025
14 checks passed
@james-d-elliott
Copy link
Contributor

james-d-elliott commented Mar 17, 2025

For clarity the intention behind the change is not security but privacy since the information is not necessary to perform the link to a user account.

See Claim Stability and Uniqueness. This section forms the basis for this choice as the ID Token itself doesn't necessarily require user information as the account can be linked without it.

When taking into account the UserInfo Endpoint, the Claim Stability and Uniqueness previously mentioned, and the ID Token combined with Standard Claims; as well as Requesting Claims using the "claims" Request Parameter it seems very apparent the intentions of the spec are to not include extra claims by default but does allow for it.

I believe for these reasons we clearly made an error by including them by default in the base spec, and it also seems clear from the spec the intention is to link accounts in a user flow which links a user intending to log in or already logged in user to the pairwise of iss and / sub claims.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants