From b8deb896136be24650b736700da814bafeaae9f0 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Mon, 18 May 2026 12:52:45 +0530 Subject: [PATCH] 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=`. - 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:///omniauth/saml/callback?account_id= ``` - Set the mocksaml audience/SP entity ID to the value shown in Chatwoot SAML settings, usually: ```text https:///saml/sp/ ``` - 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 --- .../omniauth_callbacks_controller.rb | 32 ++++++------------- .../config/initializers/omniauth_saml.rb | 6 ++-- .../omniauth_callbacks_controller_spec.rb | 23 +++++++------ 3 files changed, 22 insertions(+), 39 deletions(-) diff --git a/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb b/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb index 3dea12888d3..3e2713dd7ee 100644 --- a/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb +++ b/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb @@ -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) diff --git a/enterprise/config/initializers/omniauth_saml.rb b/enterprise/config/initializers/omniauth_saml.rb index f39e9d511d8..92d551e830e 100644 --- a/enterprise/config/initializers/omniauth_saml.rb +++ b/enterprise/config/initializers/omniauth_saml.rb @@ -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 diff --git a/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb b/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb index 8eb61fd8f25..b515cfb3b7c 100644 --- a/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb +++ b/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb @@ -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)