An empty (but valid formed) groups list provided via the OIDC ID token
would be considered as a lacking detail, and therefore trigger a lookup
to the userinfo endpoint in an attempt to get that information.
This fixes this to properly distinguish between not-provided and empty
state, to avoid userinfo where provided as valid but empty.
Includes test to cover.
For #5101
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
- Disabled by default due to strict rejection by auth systems.
- Fixed issue when autoloading logout URL, but not provided in
autodiscovery response.
- Added proper handling for if the logout URL contains a query string
already.
- Added extra tests to cover.
- Forced config endpoint to be used, if set as a string, instead of
autodiscovery endpoint.
Now assume, based on OIDC discovery spec, that keys without 'use' are
'sig' keys. Should not affect existing use-cases since existance of such
keys would have throw exceptions in prev. versions of bookstack.
For #3869
Is generally aligned with out SAML2 group sync functionality, but for
OIDC based upon feedback in #3004.
Neeeded the tangental addition of being able to define custom scopes on
the initial auth request as some systems use this to provide additional
id token claims such as groups.
Includes tests to cover.
Tested live using Okta.
Fixes issue where certain errors would not show to the user
due to extra navigation jumps which lost the error message
in the process.
This simplifies and aligns exceptions with more directly
handled exception usage at the controller level.
Fixes#3264
As per #3047.
Also made some SAML specific fixes:
- IDP initiated login was broken due to forced default session value.
Double checked against OneLogin lib docs that this reverted logic was fine.
- Changed how the saml login flow works to use 'withoutMiddleware' on
the route instead of hacking out the session driver. This was due to
the array driver (previously used for the hack) no longer being
considered non-persistent.
