fix: SAML redirect uses SAMLRequest in URL (#3168)

* fix: SAML redirect uses SAMLRequest in URL

* revert samael fork and fix ce edition

* simplify enterprise ff

* logging to debug

* rename unused variable

backend/Cargo.lock

@@ -934,9 +934,9 @@ dependencies = [
 
 [[package]]
 name = "bytemuck"
-version = "1.14.1"
+version = "1.14.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "ed2490600f404f2b94c167e31d3ed1d5f3c225a0f3b80230053b3e0b7b962bd9"
+checksum = "ea31d69bda4949c1c1562c1e6f042a1caefac98cdc8a298260a2ff41c1e2d42b"
 dependencies = [
  "bytemuck_derive",
 ]
@@ -1505,9 +1505,9 @@ dependencies = [
 
 [[package]]
 name = "curve25519-dalek"
-version = "4.1.1"
+version = "4.1.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e89b8c6a2e4b1f45971ad09761aafb85514a84744b67a95e32c3cc1352d1f65c"
+checksum = "0a677b8922c94e01bdbb12126b0bc852f00447528dee1782229af9c720c3f348"
 dependencies = [
  "cfg-if",
  "cpufeatures",
@@ -2077,9 +2077,9 @@ dependencies = [
 
 [[package]]
 name = "ed25519-dalek"
-version = "2.1.0"
+version = "2.1.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1f628eaec48bfd21b865dc2950cfa014450c01d2fa2b69a86c2fd5844ec523c0"
+checksum = "4a3daa8e81a3963a60642bcc1f90a670680bd4a77535faa384e9d1c79d620871"
 dependencies = [
  "curve25519-dalek",
  "ed25519",
@@ -4162,9 +4162,9 @@ dependencies = [
 
 [[package]]
 name = "num-complex"
-version = "0.4.4"
+version = "0.4.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1ba157ca0885411de85d6ca030ba7e2a83a28636056c7c699b07c8b6f7383214"
+checksum = "23c6602fda94a57c990fe0df199a035d83576b496aa29f4e634a8ac6004e68a6"
 dependencies = [
  "bytemuck",
  "num-traits",
@@ -8669,9 +8669,9 @@ dependencies = [
 
 [[package]]
 name = "unicode-segmentation"
-version = "1.10.1"
+version = "1.11.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1dd624098567895118886609431a7c3b8f516e41d30e0643f03d94592a147e36"
+checksum = "d4c87d22b6e3f4a18d4d40ef354e97c90fcb14dd91d7dc0aa9d8a1172ebf7202"
 
 [[package]]
 name = "unicode-width"
@@ -9635,9 +9635,9 @@ checksum = "dff9641d1cd4be8d1a070daf9e3773c5f67e78b4d9d42263020c057706765c04"
 
 [[package]]
 name = "winnow"
-version = "0.5.37"
+version = "0.5.39"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a7cad8365489051ae9f054164e459304af2e7e9bb407c958076c8bf4aef52da5"
+checksum = "5389a154b01683d28c77f8f68f49dea75f0a4da32557a58f68ee51ebba472d29"
 dependencies = [
  "memchr",
 ]

backend/Cargo.toml

@@ -198,7 +198,7 @@ tokio-postgres = {version = "^0.7", features = ["array-impls", "with-serde_json-
 mysql_async = { version = "*", default-features = false, features = ["minimal", "default", "native-tls-tls"]}
 postgres-native-tls = "^0"
 native-tls = "^0"
-samael = { version = "0.0.14", features = ["xmlsec"] }
+samael = { version="0.0.14", features = ["xmlsec"] }
 gcp_auth = "0.9.0"
 rust_decimal = { version = "^1", features = ["db-postgres"]}
 jsonwebtoken = "8.3.0"

backend/windmill-api/src/lib.rs

@@ -9,7 +9,7 @@
 use crate::db::ApiAuthed;
 use crate::embeddings::load_embeddings_db;
 use crate::oauth2::AllClients;
-use crate::saml::{SamlSsoLogin, ServiceProviderExt};
+use crate::saml::ServiceProviderExt;
 use crate::scim::has_scim_token;
 use crate::tracing_init::MyOnFailure;
 use crate::{
@@ -164,13 +164,15 @@ pub async fn run_server(
         .allow_origin(Any);
 
     #[cfg(feature = "enterprise_saml")]
-    let sp_extension: (ServiceProviderExt, SamlSsoLogin) = match get_license_plan().await {
+    let sp_extension: ServiceProviderExt = match get_license_plan().await {
         LicensePlan::Enterprise => saml::build_sp_extension().await?,
-        LicensePlan::Pro => (ServiceProviderExt(None), SamlSsoLogin(None)),
+        LicensePlan::Pro => ServiceProviderExt(None),
     };
 
     #[cfg(not(feature = "enterprise_saml"))]
-    let sp_extension = (ServiceProviderExt(), SamlSsoLogin(None));
+    let sp_extension = ServiceProviderExt();
+
+    let sp_extension_arc = Arc::new(sp_extension);
 
     let embeddings_db = if server_mode {
         Some(load_embeddings_db(&db))
@@ -242,7 +244,7 @@ pub async fn run_server(
                 .nest("/oidc", oidc::global_service())
                 .nest(
                     "/saml",
-                    saml::global_service().layer(Extension(Arc::new(sp_extension.0))),
+                    saml::global_service().layer(Extension(Arc::clone(&sp_extension_arc))),
                 )
                 .nest(
                     "/scim",
@@ -274,7 +276,7 @@ pub async fn run_server(
                 )
                 .nest(
                     "/oauth",
-                    oauth2::global_service().layer(Extension(Arc::new(sp_extension.1))),
+                    oauth2::global_service().layer(Extension(Arc::clone(&sp_extension_arc))),
                 )
                 .route("/version", get(git_v))
                 .route("/uptodate", get(is_up_to_date))

backend/windmill-api/src/oauth2.rs

@@ -41,7 +41,7 @@ use windmill_common::utils::{not_found_if_none, now_from_db};
 use windmill_common::variables::build_crypt;
 
 use crate::db::ApiAuthed;
-use crate::saml::SamlSsoLogin;
+use crate::saml::{generate_redirect_url, ServiceProviderExt};
 use crate::users::{login_externally, LoginUserInfo};
 use crate::webhook_util::{InstanceEvent, WebhookShared};
 use crate::{db::DB, variables::encrypt, workspaces::WorkspaceSettings};
@@ -446,7 +446,12 @@ struct Logins {
     oauth: Vec<String>,
     saml: Option<String>,
 }
-async fn list_logins(Extension(sso): Extension<Arc<SamlSsoLogin>>) -> error::JsonResult<Logins> {
+async fn list_logins(
+    Extension(sso): Extension<Arc<ServiceProviderExt>>,
+) -> error::JsonResult<Logins> {
+    let saml_redirect_opt = generate_redirect_url(sso)
+        .await
+        .map_err(|e| Error::InternalErr(e.to_string()))?;
     Ok(Json(Logins {
         oauth: OAUTH_CLIENTS
             .read()
@@ -455,7 +460,7 @@ async fn list_logins(Extension(sso): Extension<Arc<SamlSsoLogin>>) -> error::Jso
             .keys()
             .map(|x| x.to_owned())
             .collect::<Vec<String>>(),
-        saml: sso.0.clone(),
+        saml: saml_redirect_opt,
     }))
 }
 

backend/windmill-api/src/saml.rs

@@ -12,7 +12,6 @@ use axum::response::Redirect;
 use axum::{routing::post, Router};
 #[cfg(feature = "enterprise_saml")]
 use axum::{Extension, Form};
-#[cfg(feature = "enterprise_saml")]
 use std::sync::Arc;
 
 #[cfg(feature = "enterprise_saml")]
@@ -43,10 +42,8 @@ pub struct ServiceProviderExt();
 #[cfg(feature = "enterprise_saml")]
 use windmill_common::ee::{get_license_plan, LicensePlan};
 
-pub struct SamlSsoLogin(pub Option<String>);
-
 #[cfg(feature = "enterprise_saml")]
-pub async fn build_sp_extension() -> anyhow::Result<(ServiceProviderExt, SamlSsoLogin)> {
+pub async fn build_sp_extension() -> anyhow::Result<ServiceProviderExt> {
     if let Some(url_metadata) = std::env::var("SAML_METADATA").ok() {
         //todo restrict for non ee
 
@@ -56,13 +53,7 @@ pub async fn build_sp_extension() -> anyhow::Result<(ServiceProviderExt, SamlSso
         // let pub_key = openssl::x509::X509::from_pem("")?;
         // let private_key = openssl::rsa::Rsa::private_key_from_pem("")?;
 
-        let url = idp_metadata
-            .idp_sso_descriptors
-            .clone()
-            .unwrap_or_default()
-            .get(0)
-            .and_then(|x| x.single_sign_on_services.get(0).map(|x| x.location.clone()));
-
+        let acs_url = format!("{}/api/saml/acs", BASE_URL.read().await.clone());
         let sp = ServiceProviderBuilder::default()
             .entity_id("windmill".to_string())
             // .key(private_key)
@@ -74,13 +65,51 @@ pub async fn build_sp_extension() -> anyhow::Result<(ServiceProviderExt, SamlSso
                 ..ContactPerson::default()
             })
             .idp_metadata(idp_metadata)
-            .acs_url(format!("{}/api/saml/acs", BASE_URL.read().await.clone()))
+            .acs_url(acs_url)
             .build()?;
 
-        tracing::info!("SAML Configured, sso login link at: {:?}", url);
-        Ok((ServiceProviderExt(Some(sp)), SamlSsoLogin(url)))
+        tracing::info!("SAML Configured - ACS url is {}", acs_url);
+        Ok(ServiceProviderExt(Some(sp)))
     } else {
-        Ok((ServiceProviderExt(None), SamlSsoLogin(None)))
+        Ok(ServiceProviderExt(None))
+    }
+}
+
+#[cfg(not(feature = "enterprise_saml"))]
+pub async fn generate_redirect_url(
+    _service_provider: Arc<ServiceProviderExt>,
+) -> anyhow::Result<Option<String>> {
+    return Ok(None);
+}
+
+#[cfg(feature = "enterprise_saml")]
+pub async fn generate_redirect_url(
+    service_provider: Arc<ServiceProviderExt>,
+) -> anyhow::Result<Option<String>> {
+    if let Some(sp) = &service_provider.0 {
+        let url = sp
+            .idp_metadata
+            .idp_sso_descriptors
+            .clone()
+            .unwrap_or_default()
+            .get(0)
+            .and_then(|x| x.single_sign_on_services.get(0).map(|x| x.location.clone()));
+
+        let authn_req = sp
+            .make_authentication_request(url.unwrap_or_default().as_str())
+            .map_err(|e| anyhow::anyhow!(e.to_string()))?;
+        let redirect_url = authn_req
+            .redirect(BASE_URL.read().await.clone().as_str())
+            .map_err(|e| anyhow::anyhow!(e.to_string()))?
+            .map(|u| u.to_string());
+
+        tracing::debug!(
+            "SAML Configured, sso login link at: {:?}",
+            redirect_url.clone()
+        );
+        Ok(redirect_url)
+    } else {
+        Ok(None)
     }
 }