-
Notifications
You must be signed in to change notification settings - Fork 209
Add option :slo_enabled to opt-out (diable) from SLO endpoints completely #243
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
Add option :slo_enabled to opt-out (diable) from SLO endpoints completely #243
Conversation
…for-slo-disabled-scenario Add Not Implemented handling when SLO disabled
|
@fh1ch @bufferoverflow Hi 👋 Can you please have a look at this PR when you have the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds the ability to disable Single Logout (SLO) functionality in the SAML OmniAuth strategy by introducing a new :slo_enabled configuration option that defaults to true.
- Adds
:slo_enabledoption with a default value oftrueto maintain backward compatibility - Returns HTTP 501 Not Implemented when SLO is disabled for both
/auth/saml/sloand/auth/saml/spsloendpoints - Includes comprehensive test coverage for the new disabled SLO behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/omniauth/strategies/saml.rb | Adds slo_enabled option, helper methods to check if SLO is enabled, and logic to return 501 response when disabled |
| spec/omniauth/strategies/saml_spec.rb | Adds test coverage for both SLO endpoints when SLO is disabled, verifying 501 status and response body |
| README.md | Documents the new :slo_enabled configuration option and explains how disabling SLO affects endpoint behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fh1ch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gerardo-navarro really nice work here, thanks a lot 🙇
All good from my end, over to @bufferoverflow for the final merge.
This pull request adds support for disabling Single Logout (SLO) in the SAML strategy by introducing a new configuration option. When SLO is disabled, the related endpoints will return an HTTP 501 Not Implemented response. The documentation and tests have been updated to reflect this new behavior.
Why this change is needed
What changed
slo_enabled.slo_enabled: true.How to validate locally
slo_enabled(or set false) and hit/users/auth/saml/spslo→ expect 501.slo_enabled: true, restart, hit same endpoint → previous SLO behavior (redirect/logout flow).?RelayState=//attacker.test) withslo_enabled: false→ no redirect (501).