feat: Support RelayState binding by default during SSO #215
+44
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Per [OASIS SAML 2.0 standard](https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf): > Some bindings define a "RelayState" mechanism for preserving and conveying state information. When > such a mechanism is used in conveying a request message as the initial step of a SAML protocol, it > places requirements on the selection and use of the binding subsequently used to convey the response. > Namely, if a SAML request message is accompanied by RelayState data, then the SAML responder > MUST return its SAML protocol response using a binding that also supports a RelayState mechanism, and > it MUST place the exact RelayState data it received with the request into the corresponding RelayState > parameter in the response. In order to make standards-compliant usage of `RelayState` easier for implementing developers, this PR makes two changes: 1. It adds a default `RelayState` param mapping to the gem's `:idp_sso_service_url_runtime_params` config. 2. It enables the use of `RelayState` when `OmniAuth.config.test_mode` is enabled. - It does this by extending `OmniAuth::Strategy#mock_request_call` to add any POST `RelayState` params to the query string that will be used in the callback URL. Tests have been added for both of these new behaviors.
smudge
force-pushed
the
relay-state-mocking
branch
from
6b3582a to
a508436
Compare
Contributor
gerardo-navarro
left a comment
gerardo-navarro
left a comment
There was a problem hiding this comment.
@smudge Thanks for this suggestion. The proposed looks sound to and improving the test utility is helpful.
I took a look at this PR. LGTM 👍
/cc @bufferoverflow
|
|
||
| option :name_identifier_format, nil | ||
| option :idp_sso_service_url_runtime_params, {} | ||
| option :idp_sso_service_url_runtime_params, { RelayState: 'RelayState' } |
Contributor
There was a problem hiding this comment.
IMO It is safe to include the RelayState param as default for the idp_sso_service_url_runtime_params option because:
RelayStateis a valid param for SAML request and part of the SAML standard specificationidp_sso_service_url_runtime_paramsoption is only used for constructing the params for the authn request and therefore the effect is limited to only building the redirect request to the IdP.
fh1ch
requested review from
bufferoverflow,
Copilot,
fh1ch and
gerardo-navarro
Copilot finished reviewing on behalf of
fh1ch
November 17, 2025 14:12
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for SAML 2.0 standard-compliant RelayState binding by making it easier for developers to use RelayState parameters during SSO flows.
- Adds default RelayState parameter mapping to the gem's configuration
- Enables RelayState support in OmniAuth test mode by extending the mock request handling
- Includes test coverage for both the normal SSO flow and test mode behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/omniauth/strategies/saml.rb | Adds default RelayState mapping to idp_sso_service_url_runtime_params and implements mock_request_call to handle RelayState in test mode |
| spec/omniauth/strategies/saml_spec.rb | Adds test cases for RelayState parameter handling in both normal and test mode scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # | ||
| # By default, the "mock" `OmniAuth::Strategy` implementation will forward along any URL params, | ||
| # so we can in turn take any POSTed RelayState params and put them in the GET query string: | ||
| query_hash = request.GET.merge!(additional_params_for_authn_request.slice('RelayState')) |
There was a problem hiding this comment.
The use of merge! mutates the original request.GET hash, which could have unintended side effects. Consider using non-mutating merge instead:
query_hash = request.GET.merge(additional_params_for_authn_request.slice('RelayState'))
Suggested change
| query_hash = request.GET.merge!(additional_params_for_authn_request.slice('RelayState')) | |
| query_hash = request.GET.merge(additional_params_for_authn_request.slice('RelayState')) |
Author
There was a problem hiding this comment.
I've long since lost the context of whether or not the mutation here was intentional.
Author
|
👋 Any further code changes y'all need from me here? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per OASIS SAML 2.0 standard:
In order to make standards-compliant usage of
RelayStateeasier for implementing developers, this PR makes two changes:RelayStateparam mapping to the gem's:idp_sso_service_url_runtime_paramsconfig.RelayStatewhenOmniAuth.config.test_modeis enabled.OmniAuth::Strategy#mock_request_callto add any POSTRelayStateparams to the query string that will be used in the callback URL.Tests have been added for both of these new behaviors.