Skip to content

OIDC RP-Initiated logout #4714

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

Merged
merged 8 commits into from
Dec 7, 2023
Merged

OIDC RP-Initiated logout #4714

merged 8 commits into from
Dec 7, 2023

Conversation

ssddanbrown
Copy link
Member

@ssddanbrown ssddanbrown commented Dec 6, 2023

Continuation of #4467.

https://openid.net/specs/openid-connect-rpinitiated-1_0.html

Todo

  • Add auto-discovery support.
  • Allow disabling via config option.
  • Testing
    • PHP Unit Coverage
    • Manual test on providers:
      • Okta
      • KeyCloak
      • Azure
      • Authentik
      • Auth0 (If supports)
    • Manual re-test of SAML2 logout with auto-initate on/off.
    • Manual re-test of OIDC logout with auto-initate on/off, with RP-initated logout on/off.
    • Manual re-test of custom social driver usage (specifically appearance/use in multiple parts of the app).
    • Test of auto-initate login and logout handling when lone custom social driver active.

Questionables

  • Can an OP error if a post_logout_redirect_uri is provided that doesn't meet spec (which we cannot check dynamically as config is OP side).
    • Answer: From testing, an OP will reject the request if an non-expected redirect URI is provided. Without a redirect URI you can be left at the OP system sign-in page, so don't think sending without a redirect URI by default is sensible.

Notes

  • Okta is really strict when it comes to redirect URIs, they had to match exactly, including query params (?prevent_auto_init=true) otherwise an error would be thrown pre-okta-signout. Might need to specify the adding of all URL possibilities. Also makes it more important that we never (or very rarely) change these.
  • Azure didn't require any logout redirect URLs to be provided, just returned as desired.
  • Looks like Auth0 do support RP-initiated logout but the endpoint via autodiscovery wasn't active for me, maybe have to talk to support or something.
    • Manually using a <ISSUER>/oidc/logout worked okay for me.
    • Auth0 also strict on return URL like okta.
  • Keycloak strict on logout return URLs, although allow wildcards which is nice.
  • Authentik never redirects back, and provides the option to user to logout of all, or go back to BookStack. Not an issue, but a little different to others.

Docs updates

  • Update advisory to advise RP-initiated logout will be active if supported via discovery.
    • Changed plans to make non-enabled by default.
  • Update docs with logout info.
    • Specifically mention the requirement to configure the "Sign-out redirect URI" (or similar) on the identity provider.
    • Might need to detail specifics, including return URLs and the http/https caveat from the spec.

joancyho and others added 4 commits August 29, 2023 13:07
Extracted logout to the login service so the logic can be shared instead
of re-implemented at each stage. For this, the SocialAuthService was
split so the driver management is in its own class, so it can be used
elsewhere without use (or circular dependencies) of the
SocialAuthService.

During review of #4467
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

1 participant