fix: make SAML callback session independent (#14467)

This PR makes SAML login independent of Rails session cookies

## Problem

The normal SAML login flow should be straightforward:

- User opens Chatwoot.
- Chatwoot creates `_chatwoot_session`.
- User starts SSO.
- Chatwoot redirects the browser to the SAML provider.
- The provider authenticates the user.
- The provider sends the browser back to Chatwoot's ACS URL.
- Chatwoot reads the SAML response, finds or creates the user, and logs
them in.

The fragile step is the ACS callback. Most SSO flows return to the app
through browser redirects where cookies usually pass through as
expected. **ADFS commonly returns the SAML response with a cross-site
POST**. With Chatwoot's session cookie using `SameSite=Lax`, browsers
may not send `_chatwoot_session` on that POST.

SAML validation itself does not need the old Rails session cookie. The
problem was our callback handoff after validation. DeviseTokenAuth
stores the verified OmniAuth payload in Rails session, then redirects to
a second callback route. If the browser does not preserve that session,
Chatwoot has already received a valid SAML response but can no longer
finish login.

## Solution

This PR removes the session-backed handoff for SAML only:

- The SAML callback completes login in the same request where OmniAuth
validates the SAML response.
- Chatwoot reads the verified auth payload directly from
`request.env['omniauth.auth']`.
- Account context and RelayState come from callback params or OmniAuth
env data, not Rails session.
- Other OmniAuth providers continue using the existing DeviseTokenAuth
flow.
- Mobile SAML still works when the IdP returns `RelayState=mobile`; the
callback redirects to the mobile deep link with the generated SSO token.

The previous SAML override used `303 See Other` to avoid replaying the
SAML POST into the second callback route. This change keeps that intent,
but removes the second callback route for SAML entirely.

## Screen recording

### SP Initiated

https://github.com/user-attachments/assets/b0735e93-3864-4cc3-b6fc-419fff4b549e

### IDP Initiated

https://github.com/user-attachments/assets/3ded0246-933c-4c85-9b7c-fa15fdc34883

## Testing

Manual validation:

- Complete a SAML login.
- In the browser network trace, find the IdP POST to
`/omniauth/saml/callback?account_id=<account-id>`.
- Confirm it redirects directly to `/app/login?...sso_auth_token=...`
for web login.
- For mobile, confirm `RelayState=mobile` redirects to the configured
mobile deep link.
- Confirm there is no intermediate `/auth/saml/callback` request.

Testing with mocksaml.com:

- Configure Chatwoot with a public `FRONTEND_URL`.
- Set the mocksaml ACS URL to:

```text
https://<chatwoot-host>/omniauth/saml/callback?account_id=<account-id>
```

- Set the mocksaml audience/SP entity ID to the value shown in Chatwoot
SAML settings, usually:

```text
https://<chatwoot-host>/saml/sp/<account-id>
```

- Use an email returned by mocksaml that exists in the SAML-enabled
account.
- Start login from Chatwoot's SSO login page.
- Confirm the callback redirects directly to the app login URL with an
SSO token.

---------

Co-authored-by: Sojan Jose <sojan@pepalo.com>
This commit is contained in:
Shivam Mishra 2026-05-18 12:52:45 +05:30 committed by GitHub
parent ef27e571f7
commit b8deb89613
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 22 additions and 39 deletions

View File

@ -1,26 +1,8 @@
module Enterprise::DeviseOverrides::OmniauthCallbacksController
def saml
# Call parent's omniauth_success which handles the auth
omniauth_success
end
def redirect_callbacks
# derive target redirect route from 'resource_class' param, which was set
# before authentication.
devise_mapping = get_devise_mapping
redirect_route = get_redirect_route(devise_mapping)
return omniauth_success if params[:provider] == 'saml'
# preserve omniauth info for success route. ignore 'extra' in twitter
# auth response to avoid CookieOverflow.
session['dta.omniauth.auth'] = request.env['omniauth.auth'].except('extra')
session['dta.omniauth.params'] = request.env['omniauth.params']
# For SAML, use 303 See Other to convert POST to GET and preserve session
if params[:provider] == 'saml'
redirect_to redirect_route, { status: 303 }.merge(redirect_options)
else
super
end
super
end
def omniauth_success
@ -33,7 +15,7 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController
end
def omniauth_failure
return super unless params[:provider] == 'saml'
return super unless params[:strategy] == 'saml'
relay_state = saml_relay_state
error = params[:message] || 'authentication-failed'
@ -63,11 +45,15 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController
end
def extract_saml_account_id
params[:account_id] || session[:saml_account_id] || request.env['omniauth.params']&.dig('account_id')
params[:account_id] || request.env['omniauth.params']&.dig('account_id')
end
def saml_relay_state
session[:saml_relay_state] || request.env['omniauth.params']&.dig('RelayState')
params[:RelayState] || request.env['omniauth.params']&.dig('RelayState')
end
def auth_hash
request.env['omniauth.auth'] || super
end
def for_mobile?(relay_state)

View File

@ -7,14 +7,12 @@ SAML_SETUP_PROC = proc do |env|
# Extract account_id from various sources
account_id = request.params['account_id'] ||
request.session[:saml_account_id] ||
env['omniauth.params']&.dig('account_id')
relay_state = request.params['RelayState'] || ''
if account_id
# Store in session and omniauth params for callback
request.session[:saml_account_id] = account_id
request.session[:saml_relay_state] = relay_state
# Keep SAML request context in OmniAuth env so the callback can be processed
# without depending on the Rails session cookie.
env['omniauth.params'] ||= {}
env['omniauth.params']['account_id'] = account_id
env['omniauth.params']['RelayState'] = relay_state

View File

@ -29,11 +29,6 @@ RSpec.describe 'Enterprise SAML OmniAuth Callbacks', type: :request do
get "/omniauth/saml/callback?account_id=#{account.id}"
# expect a 302 redirect to auth/saml/callback
expect(response).to redirect_to('http://www.example.com/auth/saml/callback')
follow_redirect!
# expect redirect to login with SSO token
expect(response).to redirect_to(%r{/app/login\?email=.+&sso_auth_token=.+$})
# verify user was created
@ -50,14 +45,21 @@ RSpec.describe 'Enterprise SAML OmniAuth Callbacks', type: :request do
get "/omniauth/saml/callback?account_id=#{account.id}"
# expect a 302 redirect to auth/saml/callback
expect(response).to redirect_to('http://www.example.com/auth/saml/callback')
follow_redirect!
expect(response).to redirect_to(%r{/app/login\?email=.+&sso_auth_token=.+$})
end
end
it 'redirects mobile SAML login to the mobile deep link' do
with_modified_env FRONTEND_URL: 'http://www.example.com' do
create(:user, email: 'mobile@example.com', account: account)
set_saml_config('mobile@example.com')
get "/omniauth/saml/callback?account_id=#{account.id}&RelayState=mobile"
expect(response).to redirect_to(%r{\Achatwootapp://auth/saml\?email=.+&sso_auth_token=.+\z})
end
end
it 'rejects an existing user from another account' do
with_modified_env FRONTEND_URL: 'http://www.example.com' do
other_account = create(:account)
@ -66,9 +68,6 @@ RSpec.describe 'Enterprise SAML OmniAuth Callbacks', type: :request do
get "/omniauth/saml/callback?account_id=#{account.id}"
expect(response).to redirect_to('http://www.example.com/auth/saml/callback')
follow_redirect!
expect(response).to redirect_to('http://www.example.com/app/login?error=saml-authentication-failed')
expect(existing_user.reload.provider).to eq('email')
expect(existing_user.accounts).not_to include(account)