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.

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

feat(auth): implement SAML SLO #11599

Merged
merged 5 commits into from Mar 17, 2022

Conversation

laurentlp
Copy link
Contributor

@laurentlp laurentlp commented Mar 8, 2022

This PR adds the implementation of SAML Single Log Out (SLO) to the already existing authentication strategy.

Related PR: https://github.com/botpress/botpress-pro/pull/69

This PR will require extensive testing. Here are the scenarios I thought of:

  • Login when logged out of IdP
  • Login when already logged in within the IdP
  • Logout of Botpress and check if logged out of IdP
  • Logout of IdP and check if logged out of Botpress (does not seem to work with Okta. Will have to test with another provider)
  • Have multiple service providers (e.g. Botpress) logged in and check if logging out from one of them, log us out of the others. This is the SLO flow.

Note: passport-saml has some open issues that either bring constraints to using SLO or make it behave weirdly in some circustances:

@linear
Copy link

linear bot commented Mar 8, 2022

DEV-2313 [FEATURE] Implement SAML SLO (botpress/botpress botpress/v12#1602)

Is your feature request related to a problem? Please describe.
Currently, the SAML implementation for Botpress doesn't provide the SLO Logout URL, which is part of the SAML specification

Describe the solution you'd like
Add a route for SLO and call strategy.logout on the current logout route

Describe alternatives you've considered
It isn't possible to create a custom router for it because we can't log out using the SDK, we also can't hook the current logout call.

Additional context
https://github.com/node-saml/passport-saml
node-saml/passport-saml#221

botpress/borpress botpress/v12#1602 by @ davidvitora

@laurentlp laurentlp marked this pull request as ready for review March 9, 2022 22:59
samuelmasse
samuelmasse previously approved these changes Mar 16, 2022
samuelmasse
samuelmasse previously approved these changes Mar 16, 2022
@laurentlp laurentlp merged commit 368addd into master Mar 17, 2022
@laurentlp laurentlp deleted the laurentlp-dev-2313-feature-implement-saml-slo branch March 17, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants