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)