From 75f6bebc300ab7ec27bdf4ba25472d0708ce082f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 22 Oct 2025 11:31:27 +0200 Subject: [PATCH 1/2] Don't set issuer and idp_metadata in SAML settings The `issuer` setting was renamed to `sp_entity_id` in omniauth-saml [1], and it's been deprecated in ruby-saml since version 1.11.0, released on July 24, 2019 [2]. The ruby-saml code currently uses: ``` def sp_entity_id @sp_entity_id || @issuer end ``` So setting `issuer` to the same value as `sp_entity_id` if `sp_entity_id` is present, as we were doing, has no effect. On the other hand, neither omniauth-saml nor ruby-saml use the `idp_metadata_url` and `idp_metadata` settings. [1] https://github.com/omniauth/omniauth-saml/commit/74ed8dfb3aed [2] https://github.com/SAML-Toolkits/ruby-saml/releases/tag/v1.11.0 --- app/lib/omniauth_tenant_setup.rb | 9 --------- spec/lib/omniauth_tenant_setup_spec.rb | 3 --- 2 files changed, 12 deletions(-) diff --git a/app/lib/omniauth_tenant_setup.rb b/app/lib/omniauth_tenant_setup.rb index 8fe2b9a5b..4132b81a3 100644 --- a/app/lib/omniauth_tenant_setup.rb +++ b/app/lib/omniauth_tenant_setup.rb @@ -46,16 +46,7 @@ module OmniauthTenantSetup strategy = env["omniauth.strategy"] strategy.options[:sp_entity_id] = sp_entity_id if sp_entity_id.present? - strategy.options[:idp_metadata_url] = idp_metadata_url if idp_metadata_url.present? strategy.options[:idp_sso_service_url] = idp_sso_service_url if idp_sso_service_url.present? - - if strategy.options[:issuer].present? && sp_entity_id.present? - strategy.options[:issuer] = sp_entity_id - end - - if strategy.options[:idp_metadata].present? && idp_metadata_url.present? - strategy.options[:idp_metadata] = idp_metadata_url - end end end diff --git a/spec/lib/omniauth_tenant_setup_spec.rb b/spec/lib/omniauth_tenant_setup_spec.rb index c955f8311..9bf637a29 100644 --- a/spec/lib/omniauth_tenant_setup_spec.rb +++ b/spec/lib/omniauth_tenant_setup_spec.rb @@ -34,7 +34,6 @@ describe OmniauthTenantSetup do mars_strategy_options = mars_env["omniauth.strategy"].options expect(mars_strategy_options[:sp_entity_id]).to eq "https://mars.consul.dev/saml/metadata" - expect(mars_strategy_options[:idp_metadata_url]).to eq "https://mars-idp.example.com/metadata" expect(mars_strategy_options[:idp_sso_service_url]).to eq "https://mars-idp.example.com/sso" end @@ -48,7 +47,6 @@ describe OmniauthTenantSetup do venus_strategy_options = venus_env["omniauth.strategy"].options expect(venus_strategy_options[:sp_entity_id]).to eq "https://venus.consul.dev/saml/metadata" - expect(venus_strategy_options[:idp_metadata_url]).to eq "https://venus-idp.example.com/metadata" expect(venus_strategy_options[:idp_sso_service_url]).to eq "https://venus-idp.example.com/sso" end end @@ -79,7 +77,6 @@ describe OmniauthTenantSetup do earth_strategy_options = earth_env["omniauth.strategy"].options expect(earth_strategy_options[:sp_entity_id]).to eq "https://default.consul.dev/saml/metadata" - expect(earth_strategy_options[:idp_metadata_url]).to eq "https://default-idp.example.com/metadata" expect(earth_strategy_options[:idp_sso_service_url]).to eq "https://default-idp.example.com/sso" end end From 4332637c0fed687a5dfa1c350415e0174a486d67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 22 Oct 2025 11:58:40 +0200 Subject: [PATCH 2/2] Only access SAML single sign-on URL when necessary We were calling `parse_remote_to_hash` in the Devise initializer, which runs when the application starts. That meant that, if we got an exception when calling that method, the application wouldn't start. We got exceptions if the single sign-on (SSO) URL isn't available or we aren't providing the right credentials. So we're moving the call to `parse_remote_to_hash` to `OmniauthTenantSetup`, which is only called when actually trying to sign in with SAML. Since we're moving the code there, we're also unifying the code so SAML settings are configured the same way for the main tenant and other tenants, like we did for OpenID Connect in commit c3b523290. In order to keep the existing behavior, we're caching the result of `parse_remote_to_hash` in an instance variable. Not sure about the advantages and disadvantages of doing so over parsing the remote URL metadata on every SAML-related request. Note that the SAML tests in `OmniauthTenantSetup` use the `stub_secrets` method. But this method is called after the application has started, meaning it doesn't stub calls to `Rails.application.secrets` in `config/initializers/`. So, before this commit, the code that parsed the IDP metadata URL wasn't executed in the tests. Since now we've moved the code but we don't want to depend on external URLs when running the tests, we need to stub the call to the external URL. Since we're now stubbing the call, we're adding expectations in the tests to check that we correctly use the settings returned in that call. --- app/lib/omniauth_tenant_setup.rb | 24 ++++++++++++++++++++---- config/initializers/devise.rb | 11 +---------- spec/lib/omniauth_tenant_setup_spec.rb | 9 +++++++++ 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/app/lib/omniauth_tenant_setup.rb b/app/lib/omniauth_tenant_setup.rb index 4132b81a3..c4b24388d 100644 --- a/app/lib/omniauth_tenant_setup.rb +++ b/app/lib/omniauth_tenant_setup.rb @@ -42,14 +42,30 @@ module OmniauthTenantSetup end def saml_auth(env, sp_entity_id, idp_metadata_url, idp_sso_service_url) - unless Tenant.default? - strategy = env["omniauth.strategy"] + env["omniauth.strategy"].options.merge!( + saml_settings(sp_entity_id, idp_metadata_url, idp_sso_service_url) + ) + end - strategy.options[:sp_entity_id] = sp_entity_id if sp_entity_id.present? - strategy.options[:idp_sso_service_url] = idp_sso_service_url if idp_sso_service_url.present? + def saml_settings(sp_entity_id, idp_metadata_url, idp_sso_service_url) + remote_saml_settings(idp_metadata_url).tap do |settings| + settings[:sp_entity_id] = sp_entity_id if sp_entity_id.present? + settings[:idp_sso_service_url] = idp_sso_service_url if idp_sso_service_url.present? + settings[:allowed_clock_drift] = 1.minute end end + def remote_saml_settings(idp_metadata_url) + return {} if idp_metadata_url.blank? + + @remote_saml_settings ||= {} + @remote_saml_settings[idp_metadata_url] ||= parsed_saml_metadata(idp_metadata_url) + end + + def parsed_saml_metadata(idp_metadata_url) + OneLogin::RubySaml::IdpMetadataParser.new.parse_remote_to_hash(idp_metadata_url) + end + def oidc_auth(env, client_id, client_secret, issuer) strategy = env["omniauth.strategy"] diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 00de7c52c..610e54aee 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -286,16 +286,7 @@ Devise.setup do |config| Rails.application.secrets.wordpress_oauth2_secret, client_options: { site: Rails.application.secrets.wordpress_oauth2_site }, setup: ->(env) { OmniauthTenantSetup.wordpress_oauth2(env) } - saml_settings = {} - if Rails.application.secrets.saml_idp_metadata_url.present? - idp_metadata_parser = OneLogin::RubySaml::IdpMetadataParser.new - saml_settings = idp_metadata_parser.parse_remote_to_hash(Rails.application.secrets.saml_idp_metadata_url) - saml_settings[:idp_sso_service_url] = Rails.application.secrets.saml_idp_sso_service_url - saml_settings[:sp_entity_id] = Rails.application.secrets.saml_sp_entity_id - saml_settings[:allowed_clock_drift] = 1.minute - end - config.omniauth :saml, saml_settings.merge(setup: ->(env) { OmniauthTenantSetup.saml(env) }) - + config.omniauth :saml, setup: ->(env) { OmniauthTenantSetup.saml(env) } config.omniauth :openid_connect, name: :oidc, scope: [:openid, :email, :profile], diff --git a/spec/lib/omniauth_tenant_setup_spec.rb b/spec/lib/omniauth_tenant_setup_spec.rb index 9bf637a29..a71963eb4 100644 --- a/spec/lib/omniauth_tenant_setup_spec.rb +++ b/spec/lib/omniauth_tenant_setup_spec.rb @@ -2,6 +2,12 @@ require "rails_helper" describe OmniauthTenantSetup do describe "#saml" do + before do + allow(OmniauthTenantSetup).to receive(:parsed_saml_metadata) do |idp_metadata_url| + { idp_entity_id: idp_metadata_url.gsub("metadata", "entityid") } + end + end + it "uses different secrets for different tenants" do create(:tenant, schema: "mars") create(:tenant, schema: "venus") @@ -35,6 +41,7 @@ describe OmniauthTenantSetup do expect(mars_strategy_options[:sp_entity_id]).to eq "https://mars.consul.dev/saml/metadata" expect(mars_strategy_options[:idp_sso_service_url]).to eq "https://mars-idp.example.com/sso" + expect(mars_strategy_options[:idp_entity_id]).to eq "https://mars-idp.example.com/entityid" end Tenant.switch("venus") do @@ -48,6 +55,7 @@ describe OmniauthTenantSetup do expect(venus_strategy_options[:sp_entity_id]).to eq "https://venus.consul.dev/saml/metadata" expect(venus_strategy_options[:idp_sso_service_url]).to eq "https://venus-idp.example.com/sso" + expect(venus_strategy_options[:idp_entity_id]).to eq "https://venus-idp.example.com/entityid" end end @@ -78,6 +86,7 @@ describe OmniauthTenantSetup do expect(earth_strategy_options[:sp_entity_id]).to eq "https://default.consul.dev/saml/metadata" expect(earth_strategy_options[:idp_sso_service_url]).to eq "https://default-idp.example.com/sso" + expect(earth_strategy_options[:idp_entity_id]).to eq "https://default-idp.example.com/entityid" end end end