OIDC userinfo endpoint support #4955
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BooksStack's OIDC Client requests the 'profile' and 'email' scope values in order to have access to the 'name', 'email', and other claims. It looks for these claims in the ID Token that is returned along with the Access Token. However, the OIDC-core specification section 5.4 [1] only requires that the Provider include those claims in the ID Token *if* an Access Token is not also issued. If an Access Token is issued, the Provider can leave out those claims from the ID Token, and the Client is supposed to obtain them by submitting the Access Token to the UserInfo Endpoint. So I suppose it's just good luck that the OIDC Providers that BookStack has been tested with just so happen to also stick those claims in the ID Token even though they don't have to. But others (in particular: https://login.infomaniak.com) don't do so, and require fetching the UserInfo Endpoint.) A workaround is currently possible by having the user write a theme with a ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE hook that fetches the UserInfo Endpoint. This workaround isn't great, for a few reasons: 1. Asking the user to implement core parts of the OIDC protocol is silly. 2. The user either needs to re-fetch the .well-known/openid-configuration file to discover the endpoint (adding yet another round-trip to each login) or hard-code the endpoint, which is fragile. 3. The hook doesn't receive the HTTP client configuration. So, have BookStack's OidcService fetch the UserInfo Endpoint and inject those claims into the ID Token, if a UserInfo Endpoint is defined. Two points about this: - Injecting them into the ID Token's claims is the most obvious approach given the current code structure; though I'm not sure it is the best approach, perhaps it should instead fetch the user info in processAuthorizationResponse() and pass that as an argument to processAccessTokenCallback() which would then need a bit of restructuring. But this made sense because it's also how the ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE hook works. - OIDC *requires* that a UserInfo Endpoint exists, so why bother with that "if a UserInfo Endpoint is defined" bit? Simply out of an abundance of caution that there's an existing BookStack user that is relying on it not fetching the UserInfo Endpoint in order to work with a non-compliant OIDC Provider. [1]: https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
- Added endpoint validation to ensure HTTPS as per spec - Added some missing types - Removed redirectUri from OidcProviderSettings since it's not a provider-based setting, but a setting for the oauth client, so extracted that back to service.
Allows a proper defined object instead of an array an extracts related logic out of OidcService. Updated userinfo to only be called if we're missing details.
ssddanbrown
force-pushed
the
oidc_userinfo
branch
from
7fd795e
to
a71c8c6
Compare
Wrapped userinfo response in its own class for additional handling and validation. Updated userdetails to take abstract claim data, to be populated by either userinfo data or id token data.
Not yet tested, nor checked all response validations.
Also updated test to suit validation changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Review and continuation of #4726.
Main functional change is to only call endpoint if missing data from ID token.
Reviewed original PR against spec, and app logic, in 9183e7f.
Other OIDC code clean-up/improvements done while in this area.
Todo
application/jwt(signed/encrypted) based responses.Doc Updates
OIDC_USERINFO_ENDPOINToption.