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.

Sign up for GitHub

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

Jump to bottom

OIDC: Add RP-initiated logout implementation #4467

Merged
merged 2 commits into from
Dec 7, 2023
Merged

OIDC: Add RP-initiated logout implementation #4467

merged 2 commits into from
Dec 7, 2023

Conversation

joancyho
Copy link

Reference Issues/PRS: 3715

#3715

What does this implement/fix? Explain your changes.

If the authentication is through an OIDC provider, this change also logs the user out of the provider as well when the user clicks on "Logout" on Bookstack.

Please add the parameter OIDC_END_SESSION_ENDPOINT in .env file based on end_session_endpoint from /.well-known/openid-configuration
e.g.
OIDC_END_SESSION_ENDPOINT=

Any other comments?

No

@ssddanbrown
Copy link
Member

Thanks for offering this @joancyho.
Before I dig into the implementation details of this PR, could you confirm:

  • Is this a complete implementation of the RP-initiated logout?
  • Has this logic and code been checked fully against the spec?
    • By this, I mean adhering to all "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT" parts of the spec.
  • What OIDC identity platforms/software has this been tested with so far?

From my view, I still need to dig into support of RP-initated logout to just validate it's worth maintaining for BookStack, which your responses will help with.

@joancyho
Copy link
Author

joancyho commented Sep 5, 2023

Thanks for offering this @joancyho. Before I dig into the implementation details of this PR, could you confirm:

* Is this a complete implementation of the [RP-initiated logout](https://openid.net/specs/openid-connect-rpinitiated-1_0.html)?

* Has this logic and code been checked fully against the spec?
  
  * By this, I mean adhering to all  "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT" parts of the spec.

* What OIDC identity platforms/software has this been tested with so far?

From my view, I still need to dig into support of RP-initated logout to just validate it's worth maintaining for BookStack, which your responses will help with.

You are welcome.

  1. It is a complete implementation of the RP-initiated logout.
  2. The logic and code have been checked fully against the spec.
  3. Keycloak has been tested with this code so far.

Thank you.

@ssddanbrown ssddanbrown changed the title Fix/OIDC logout OIDC: Add RP-initiated logout implementation Oct 30, 2023
@ssddanbrown ssddanbrown added this to the Next Feature Release milestone Oct 30, 2023
ssddanbrown added a commit that referenced this pull request Dec 6, 2023
@ssddanbrown
Auth: Refactored OIDC RP-logout PR code, Extracted logout
bba7dcc 
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
@ssddanbrown ssddanbrown mentioned this pull request Dec 6, 2023
Merged
14 tasks
@ssddanbrown ssddanbrown merged commit cc10d1d into BookStackApp:development Dec 7, 2023
@ssddanbrown
Copy link
Member

Thanks again for this @joancyho.
This has now been merged to be part of the next feature release.
I followed this up to add testing, autodiscovery support and some other bits via PR #4714.

@ssddanbrown ssddanbrown mentioned this pull request Dec 7, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
🚪 Authentication 🏭 Back-End
Milestone
v23.12
Development

Successfully merging this pull request may close these issues.
2 participants
@joancyho @ssddanbrown