Skip to content

OIDC: Add RP-initiated logout implementation - #4467

Merged
ssddanbrown merged 2 commits into
BookStackApp:developmentfrom
joancyho:fix/oidc-logout
Dec 7, 2023
Merged

OIDC: Add RP-initiated logout implementation#4467
ssddanbrown merged 2 commits into
BookStackApp:developmentfrom
joancyho:fix/oidc-logout

Conversation

@joancyho

@joancyho joancyho commented Aug 29, 2023

Copy link
Copy Markdown

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

ssddanbrown commented Sep 4, 2023

Copy link
Copy Markdown
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

joancyho commented Sep 5, 2023

Copy link
Copy Markdown
Author

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

ssddanbrown commented Dec 7, 2023

Copy link
Copy Markdown
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