starred/BookStack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2026-02-13 11:19:37 +03:00
Code Issues 784 Packages Projects Releases 10 Wiki Activity

[PR #4714] [MERGED] OIDC RP-Initiated logout #6394

New Issue
Closed
opened 2026-02-05 10:31:09 +03:00 by OVERLORD · 0 comments
OVERLORD commented 2026-02-05 10:31:09 +03:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/BookStackApp/BookStack/pull/4714
Author: @ssddanbrown
Created: 12/6/2023
Status: ✅ Merged
Merged: 12/7/2023
Merged by: @ssddanbrown

Base: development ← Head: oidc_logout

📝 Commits (8)

  • 6b55104 Fixed OIDC Logout
  • a0942ef Fixed OIDC Logout
  • cc10d1d Merge branch 'fix/oidc-logout' into development
  • bba7dcc Auth: Refactored OIDC RP-logout PR code, Extracted logout
  • f32cfb4 OIDC RP Logout: Added autodiscovery support and test cases
  • a72e0fe Tests: Fixed debug test to work with social class changes
  • 81d256a OIDC RP Logout: Fixed issues during testing
  • 7312300 OIDC: Update example env option to reflect correct default

📊 Changes

20 files changed (+426 additions, -234 deletions)

View changed files

📝 .env.example.complete (+1 -0)
📝 app/Access/Controllers/LoginController.php (+11 -37)
📝 app/Access/Controllers/OidcController.php (+8 -3)
📝 app/Access/Controllers/RegisterController.php (+5 -5)
📝 app/Access/Controllers/SocialController.php (+2 -2)
📝 app/Access/LoginService.php (+34 -7)
📝 app/Access/Oidc/OidcProviderSettings.php (+5 -0)
📝 app/Access/Oidc/OidcService.php (+37 -0)
📝 app/Access/Saml2Service.php (+2 -12)
📝 app/Access/SocialAuthService.php (+19 -150)
➕ app/Access/SocialDriverManager.php (+147 -0)
📝 app/App/Providers/AppServiceProvider.php (+2 -2)
📝 app/Config/oidc.php (+7 -1)
📝 app/Theming/ThemeService.php (+4 -4)
📝 app/Users/Controllers/UserAccountController.php (+3 -3)
📝 app/Users/Controllers/UserController.php (+3 -3)
📝 resources/views/layouts/parts/header-user-menu.blade.php (+8 -2)
📝 routes/web.php (+1 -0)
📝 tests/Auth/OidcTest.php (+124 -0)
📝 tests/DebugViewTest.php (+3 -3)

📄 Description

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.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/BookStackApp/BookStack/pull/4714 **Author:** [@ssddanbrown](https://github.com/ssddanbrown) **Created:** 12/6/2023 **Status:** ✅ Merged **Merged:** 12/7/2023 **Merged by:** [@ssddanbrown](https://github.com/ssddanbrown) **Base:** `development` ← **Head:** `oidc_logout` --- ### 📝 Commits (8) - [`6b55104`](https://github.com/BookStackApp/BookStack/commit/6b55104ecb0dfe8da154161c689d2a937d0ae0e3) Fixed OIDC Logout - [`a0942ef`](https://github.com/BookStackApp/BookStack/commit/a0942ef441045f07646c3ba06f5f804827efd992) Fixed OIDC Logout - [`cc10d1d`](https://github.com/BookStackApp/BookStack/commit/cc10d1ddfc652f6bcf3bbf61d5ec2e2861394c03) Merge branch 'fix/oidc-logout' into development - [`bba7dcc`](https://github.com/BookStackApp/BookStack/commit/bba7dcce49394f478bd401486a2d8cba23ff79ac) Auth: Refactored OIDC RP-logout PR code, Extracted logout - [`f32cfb4`](https://github.com/BookStackApp/BookStack/commit/f32cfb4292f7e3b723b5f99aab17d0f3989c93b7) OIDC RP Logout: Added autodiscovery support and test cases - [`a72e0fe`](https://github.com/BookStackApp/BookStack/commit/a72e0fee707ff0c8d226fef263eba41a49689833) Tests: Fixed debug test to work with social class changes - [`81d256a`](https://github.com/BookStackApp/BookStack/commit/81d256aebd6e7bca7b86369abd5e79b3a6679f33) OIDC RP Logout: Fixed issues during testing - [`7312300`](https://github.com/BookStackApp/BookStack/commit/7312300d53af9d1328242121b332aaed19fbcaa0) OIDC: Update example env option to reflect correct default ### 📊 Changes **20 files changed** (+426 additions, -234 deletions) <details> <summary>View changed files</summary> 📝 `.env.example.complete` (+1 -0) 📝 `app/Access/Controllers/LoginController.php` (+11 -37) 📝 `app/Access/Controllers/OidcController.php` (+8 -3) 📝 `app/Access/Controllers/RegisterController.php` (+5 -5) 📝 `app/Access/Controllers/SocialController.php` (+2 -2) 📝 `app/Access/LoginService.php` (+34 -7) 📝 `app/Access/Oidc/OidcProviderSettings.php` (+5 -0) 📝 `app/Access/Oidc/OidcService.php` (+37 -0) 📝 `app/Access/Saml2Service.php` (+2 -12) 📝 `app/Access/SocialAuthService.php` (+19 -150) ➕ `app/Access/SocialDriverManager.php` (+147 -0) 📝 `app/App/Providers/AppServiceProvider.php` (+2 -2) 📝 `app/Config/oidc.php` (+7 -1) 📝 `app/Theming/ThemeService.php` (+4 -4) 📝 `app/Users/Controllers/UserAccountController.php` (+3 -3) 📝 `app/Users/Controllers/UserController.php` (+3 -3) 📝 `resources/views/layouts/parts/header-user-menu.blade.php` (+8 -2) 📝 `routes/web.php` (+1 -0) 📝 `tests/Auth/OidcTest.php` (+124 -0) 📝 `tests/DebugViewTest.php` (+3 -3) </details> ### 📄 Description Continuation of #4467. https://openid.net/specs/openid-connect-rpinitiated-1_0.html ### Todo - [x] Add auto-discovery support. - [x] Allow disabling via config option. - [x] Testing - [x] PHP Unit Coverage - [x] Manual test on providers: - [x] Okta - [x] KeyCloak - [x] Azure - [x] Authentik - [x] Auth0 (If supports) - [x] Manual re-test of SAML2 logout with auto-initate on/off. - [x] Manual re-test of OIDC logout with auto-initate on/off, with RP-initated logout on/off. - [x] Manual re-test of custom social driver usage (specifically appearance/use in multiple parts of the app). - [x] 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](https://auth0.com/docs/authenticate/login/logout/log-users-out-of-auth0) 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
OVERLORD added the pull-request label 2026-02-05 10:31:09 +03:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
🎨 Design
📖 Docs Update
🐛 Bug
🐛 Bug
:cat2:🐈 Possible duplicate
💿 Database
☕ Open to discussion
💻 Front-End
🐕 Support
🚪 Authentication
🌍 Translations
🔌 API Task
🏭 Back-End
⛲ Upstream
🔨 Feature Request
🛠️ Enhancement
🛠️ Enhancement
🛠️ Enhancement
❤️ Happy feedback
🔒 Security
🔍 Pending Validation
💆 UX
📝 WYSIWYG Editor
🌔 Out of scope
🔩 API Request
:octocat: Admin/Meta
🖌️ View Customization
❓ Question
🚀 Priority
🛡️ Blocked
🚚 Export System
♿ A11y
🔧 Maintenance
> Markdown Editor
pull-request
Mirrored from GitHub Pull Request
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
OVERLORD
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/BookStack#6394
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?