From d9a0887dc9af83f7fef7035b5cef978664dfc7d9 Mon Sep 17 00:00:00 2001 From: Anamika Aggarwal Date: Tue, 9 Sep 2025 16:45:31 +0200 Subject: [PATCH 1/3] Fix OIDC parameters for non-default tenants We were using the `client_options` hash for the default tenant, defined in the Devise initializer, but we forgot to include that key in the multitenant code. This means OIDC wasn't working when different tenants used different configurations. --- app/lib/omniauth_tenant_setup.rb | 7 ++++--- spec/lib/omniauth_tenant_setup_spec.rb | 21 ++++++++++++--------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/app/lib/omniauth_tenant_setup.rb b/app/lib/omniauth_tenant_setup.rb index c0909b0cd..4a37ae359 100644 --- a/app/lib/omniauth_tenant_setup.rb +++ b/app/lib/omniauth_tenant_setup.rb @@ -64,10 +64,11 @@ module OmniauthTenantSetup unless Tenant.default? strategy = env["omniauth.strategy"] - strategy.options[:client_id] = client_id if client_id.present? - strategy.options[:client_secret] = client_secret if client_secret.present? strategy.options[:issuer] = issuer if issuer.present? - strategy.options[:redirect_uri] = redirect_uri if redirect_uri.present? + strategy.options[:client_options] ||= {} + strategy.options[:client_options][:identifier] = client_id if client_id.present? + strategy.options[:client_options][:secret] = client_secret if client_secret.present? + strategy.options[:client_options][:redirect_uri] = redirect_uri if redirect_uri.present? end end diff --git a/spec/lib/omniauth_tenant_setup_spec.rb b/spec/lib/omniauth_tenant_setup_spec.rb index 9b8929dd7..f38eca3b4 100644 --- a/spec/lib/omniauth_tenant_setup_spec.rb +++ b/spec/lib/omniauth_tenant_setup_spec.rb @@ -119,11 +119,12 @@ describe OmniauthTenantSetup do OmniauthTenantSetup.oidc(mars_env) mars_strategy_options = mars_env["omniauth.strategy"].options + mars_client_options = mars_strategy_options[:client_options] - expect(mars_strategy_options[:client_id]).to eq "mars-client-id" - expect(mars_strategy_options[:client_secret]).to eq "mars-client-secret" expect(mars_strategy_options[:issuer]).to eq "https://mars-oidc.example.com" - expect(mars_strategy_options[:redirect_uri]).to eq "https://mars.consul.dev/auth/oidc/callback" + expect(mars_client_options[:secret]).to eq "mars-client-secret" + expect(mars_client_options[:identifier]).to eq "mars-client-id" + expect(mars_client_options[:redirect_uri]).to eq "https://mars.consul.dev/auth/oidc/callback" end Tenant.switch("venus") do @@ -134,11 +135,12 @@ describe OmniauthTenantSetup do OmniauthTenantSetup.oidc(venus_env) venus_strategy_options = venus_env["omniauth.strategy"].options + venus_client_options = venus_strategy_options[:client_options] - expect(venus_strategy_options[:client_id]).to eq "venus-client-id" - expect(venus_strategy_options[:client_secret]).to eq "venus-client-secret" expect(venus_strategy_options[:issuer]).to eq "https://venus-oidc.example.com" - expect(venus_strategy_options[:redirect_uri]).to eq "https://venus.consul.dev/auth/oidc/callback" + expect(venus_client_options[:identifier]).to eq "venus-client-id" + expect(venus_client_options[:secret]).to eq "venus-client-secret" + expect(venus_client_options[:redirect_uri]).to eq "https://venus.consul.dev/auth/oidc/callback" end end @@ -168,11 +170,12 @@ describe OmniauthTenantSetup do OmniauthTenantSetup.oidc(earth_env) earth_strategy_options = earth_env["omniauth.strategy"].options + earth_client_options = earth_strategy_options[:client_options] - expect(earth_strategy_options[:client_id]).to eq "default-client-id" - expect(earth_strategy_options[:client_secret]).to eq "default-client-secret" expect(earth_strategy_options[:issuer]).to eq "https://default-oidc.example.com" - expect(earth_strategy_options[:redirect_uri]).to eq "https://default.consul.dev/auth/oidc/callback" + expect(earth_client_options[:identifier]).to eq "default-client-id" + expect(earth_client_options[:secret]).to eq "default-client-secret" + expect(earth_client_options[:redirect_uri]).to eq "https://default.consul.dev/auth/oidc/callback" end end end From c3b5232907ea4592781b700462d4ea368e56c26a Mon Sep 17 00:00:00 2001 From: Anamika Aggarwal Date: Tue, 9 Sep 2025 16:50:23 +0200 Subject: [PATCH 2/3] Use the same code to configure OIDC for all tenants We were following the same pattern as we used for other providers like twitter or facebook, but for OIDC we aren't passing the key and the secret as separate attributes but only a hash of options. This means we don't need to duplicate the same logic in the devise initializer and the `OmniauthTenantSetup` class. Thanks to these changes, we'll be able to introduce dynamic redirect URLs for both the default tenant and the other tenants (see next commit). Note that we could probably apply similar changes for the SAML provider. We might do so in the future. For other providers, removing the references to `Rails.application.secrets` broke their configuration when we tested it back in 2022 as part of the multitenancy feature. We might check whether that's no longer the case (or whether we made a mistake during our tests in 2022) in the future. --- app/lib/omniauth_tenant_setup.rb | 14 ++++++-------- config/initializers/devise.rb | 6 ------ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/app/lib/omniauth_tenant_setup.rb b/app/lib/omniauth_tenant_setup.rb index 4a37ae359..cb5ff3ff7 100644 --- a/app/lib/omniauth_tenant_setup.rb +++ b/app/lib/omniauth_tenant_setup.rb @@ -61,15 +61,13 @@ module OmniauthTenantSetup end def oidc_auth(env, client_id, client_secret, issuer, redirect_uri) - unless Tenant.default? - strategy = env["omniauth.strategy"] + strategy = env["omniauth.strategy"] - strategy.options[:issuer] = issuer if issuer.present? - strategy.options[:client_options] ||= {} - strategy.options[:client_options][:identifier] = client_id if client_id.present? - strategy.options[:client_options][:secret] = client_secret if client_secret.present? - strategy.options[:client_options][:redirect_uri] = redirect_uri if redirect_uri.present? - end + strategy.options[:issuer] = issuer if issuer.present? + strategy.options[:client_options] ||= {} + strategy.options[:client_options][:identifier] = client_id if client_id.present? + strategy.options[:client_options][:secret] = client_secret if client_secret.present? + strategy.options[:client_options][:redirect_uri] = redirect_uri if redirect_uri.present? end def secrets diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 44ff33b03..00de7c52c 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -300,14 +300,8 @@ Devise.setup do |config| name: :oidc, scope: [:openid, :email, :profile], response_type: :code, - issuer: Rails.application.secrets.oidc_issuer, discovery: true, client_auth_method: :basic, - client_options: { - identifier: Rails.application.secrets.oidc_client_id, - secret: Rails.application.secrets.oidc_client_secret, - redirect_uri: Rails.application.secrets.oidc_redirect_uri - }, setup: ->(env) { OmniauthTenantSetup.oidc(env) } # ==> Warden configuration From 86bbfcaa0c5fc6f879e3152bfb310f1096b17b0a Mon Sep 17 00:00:00 2001 From: Anamika Aggarwal Date: Tue, 9 Sep 2025 16:51:26 +0200 Subject: [PATCH 3/3] Automatically set the redirect URI in OIDC When we first added OIDC support, we were configuring the redirect URI in the devise initializer, just like we did for other providers. Thanks to the changes in the previous commit, that code is no longer in the devise initializer, which means we can use `url_helpers` to get the redirect URI. This means we no longer need to define this URI in the secrets. This is particularly useful for multitenancy; previously, we had to define the redirect URI for every tenant because different tenants use different domains or different subdomains. --- app/lib/omniauth_tenant_setup.rb | 11 +++++++---- config/secrets.yml.example | 3 --- docs/en/features/oauth.md | 1 - docs/es/features/oauth.md | 1 - spec/lib/omniauth_tenant_setup_spec.rb | 21 ++++++++++----------- 5 files changed, 17 insertions(+), 20 deletions(-) diff --git a/app/lib/omniauth_tenant_setup.rb b/app/lib/omniauth_tenant_setup.rb index cb5ff3ff7..8fe2b9a5b 100644 --- a/app/lib/omniauth_tenant_setup.rb +++ b/app/lib/omniauth_tenant_setup.rb @@ -22,8 +22,7 @@ module OmniauthTenantSetup end def oidc(env) - oidc_auth(env, secrets.oidc_client_id, - secrets.oidc_client_secret, secrets.oidc_issuer, secrets.oidc_redirect_uri) + oidc_auth(env, secrets.oidc_client_id, secrets.oidc_client_secret, secrets.oidc_issuer) end private @@ -60,14 +59,18 @@ module OmniauthTenantSetup end end - def oidc_auth(env, client_id, client_secret, issuer, redirect_uri) + def oidc_auth(env, client_id, client_secret, issuer) strategy = env["omniauth.strategy"] strategy.options[:issuer] = issuer if issuer.present? strategy.options[:client_options] ||= {} strategy.options[:client_options][:identifier] = client_id if client_id.present? strategy.options[:client_options][:secret] = client_secret if client_secret.present? - strategy.options[:client_options][:redirect_uri] = redirect_uri if redirect_uri.present? + strategy.options[:client_options][:redirect_uri] = oidc_redirect_uri if oidc_redirect_uri.present? + end + + def oidc_redirect_uri + Rails.application.routes.url_helpers.user_oidc_omniauth_callback_url(Tenant.current_url_options) end def secrets diff --git a/config/secrets.yml.example b/config/secrets.yml.example index 0018d4986..c4968f194 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -97,7 +97,6 @@ staging: oidc_client_id: "" oidc_client_secret: "" oidc_issuer: "" - oidc_redirect_uri: "" <<: *maps <<: *apis @@ -160,7 +159,6 @@ preproduction: oidc_client_id: "" oidc_client_secret: "" oidc_issuer: "" - oidc_redirect_uri: "" <<: *maps <<: *apis @@ -222,6 +220,5 @@ production: oidc_client_id: "" oidc_client_secret: "" oidc_issuer: "" - oidc_redirect_uri: "" <<: *maps <<: *apis diff --git a/docs/en/features/oauth.md b/docs/en/features/oauth.md index 8c5b71a81..d9016dfbc 100644 --- a/docs/en/features/oauth.md +++ b/docs/en/features/oauth.md @@ -47,5 +47,4 @@ When you complete the application registration you'll get a *key* and *secret* v oidc_client_id: "your-oidc-client-id" oidc_client_secret: "your-oidc-client-secret" oidc_issuer: "https://your-oidc-provider.com" - oidc_redirect_uri: "https://yourapp.com/users/auth/oidc/callback" ``` diff --git a/docs/es/features/oauth.md b/docs/es/features/oauth.md index eb718637a..f40ef0f1d 100644 --- a/docs/es/features/oauth.md +++ b/docs/es/features/oauth.md @@ -47,5 +47,4 @@ Cuando completes el registro de la aplicación en su plataforma te darán un *ke oidc_client_id: "tu-id-de-cliente-oidc" oidc_client_secret: "tu-secreto-de-cliente-oidc" oidc_issuer: "https://tu-proveedor-oidc.com" - oidc_redirect_uri: "https://tuaplicacion.com/users/auth/oidc/callback" ``` diff --git a/spec/lib/omniauth_tenant_setup_spec.rb b/spec/lib/omniauth_tenant_setup_spec.rb index f38eca3b4..c955f8311 100644 --- a/spec/lib/omniauth_tenant_setup_spec.rb +++ b/spec/lib/omniauth_tenant_setup_spec.rb @@ -86,6 +86,10 @@ describe OmniauthTenantSetup do end describe "#oidc" do + before do + allow(Tenant).to receive(:default_url_options).and_return({ host: "consul.dev" }) + end + it "uses different secrets for different tenants" do create(:tenant, schema: "mars") create(:tenant, schema: "venus") @@ -94,19 +98,16 @@ describe OmniauthTenantSetup do oidc_client_id: "default-client-id", oidc_client_secret: "default-client-secret", oidc_issuer: "https://default-oidc.example.com", - oidc_redirect_uri: "https://default.consul.dev/auth/oidc/callback", tenants: { mars: { oidc_client_id: "mars-client-id", oidc_client_secret: "mars-client-secret", - oidc_issuer: "https://mars-oidc.example.com", - oidc_redirect_uri: "https://mars.consul.dev/auth/oidc/callback" + oidc_issuer: "https://mars-oidc.example.com" }, venus: { oidc_client_id: "venus-client-id", oidc_client_secret: "venus-client-secret", - oidc_issuer: "https://venus-oidc.example.com", - oidc_redirect_uri: "https://venus.consul.dev/auth/oidc/callback" + oidc_issuer: "https://venus-oidc.example.com" } } ) @@ -124,7 +125,7 @@ describe OmniauthTenantSetup do expect(mars_strategy_options[:issuer]).to eq "https://mars-oidc.example.com" expect(mars_client_options[:secret]).to eq "mars-client-secret" expect(mars_client_options[:identifier]).to eq "mars-client-id" - expect(mars_client_options[:redirect_uri]).to eq "https://mars.consul.dev/auth/oidc/callback" + expect(mars_client_options[:redirect_uri]).to eq "http://mars.consul.dev/users/auth/oidc/callback" end Tenant.switch("venus") do @@ -140,7 +141,7 @@ describe OmniauthTenantSetup do expect(venus_strategy_options[:issuer]).to eq "https://venus-oidc.example.com" expect(venus_client_options[:identifier]).to eq "venus-client-id" expect(venus_client_options[:secret]).to eq "venus-client-secret" - expect(venus_client_options[:redirect_uri]).to eq "https://venus.consul.dev/auth/oidc/callback" + expect(venus_client_options[:redirect_uri]).to eq "http://venus.consul.dev/users/auth/oidc/callback" end end @@ -151,13 +152,11 @@ describe OmniauthTenantSetup do oidc_client_id: "default-client-id", oidc_client_secret: "default-client-secret", oidc_issuer: "https://default-oidc.example.com", - oidc_redirect_uri: "https://default.consul.dev/auth/oidc/callback", tenants: { mars: { oidc_client_id: "mars-client-id", oidc_client_secret: "mars-client-secret", - oidc_issuer: "https://mars-oidc.example.com", - oidc_redirect_uri: "https://mars.consul.dev/auth/oidc/callback" + oidc_issuer: "https://mars-oidc.example.com" } } ) @@ -175,7 +174,7 @@ describe OmniauthTenantSetup do expect(earth_strategy_options[:issuer]).to eq "https://default-oidc.example.com" expect(earth_client_options[:identifier]).to eq "default-client-id" expect(earth_client_options[:secret]).to eq "default-client-secret" - expect(earth_client_options[:redirect_uri]).to eq "https://default.consul.dev/auth/oidc/callback" + expect(earth_client_options[:redirect_uri]).to eq "http://earth.consul.dev/users/auth/oidc/callback" end end end