test(iam): freeze OIDC federation behavior (#5627)

crates/iam/src/federation/oidc/claims.rs

@@ -62,6 +62,7 @@ pub(super) fn authorization(oidc: &OidcSys, provider_id: String, claims: OidcCla
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::oidc::{make_test_sys, test_config};
     use serde_json::json;
     use std::collections::HashMap;
 
@@ -99,4 +100,43 @@ mod tests {
         };
         assert!(string_list_claim(&ambiguous, "roles").is_empty());
     }
+
+    #[test]
+    fn authorization_preserves_verified_claims_and_keeps_source_groups_distinct() {
+        let mut config = test_config("corp");
+        config.claim_prefix = "mapped-".to_string();
+        config.roles_claim = "roles".to_string();
+        let oidc = make_test_sys(vec![config]);
+        let raw = HashMap::from([
+            ("iss".to_string(), json!("https://corp.example.test")),
+            ("department".to_string(), json!("engineering")),
+            ("roles".to_string(), json!(["reader", "admin"])),
+        ]);
+        let authorization = authorization(
+            &oidc,
+            "corp".to_string(),
+            OidcClaims {
+                sub: " subject-123 ".to_string(),
+                email: " user@example.test ".to_string(),
+                username: " user ".to_string(),
+                groups: vec![
+                    "source-ops".to_string(),
+                    "source-developers".to_string(),
+                    "source-ops".to_string(),
+                ],
+                raw: raw.clone(),
+            },
+        );
+
+        assert_eq!(authorization.provider_id, "corp");
+        assert_eq!(authorization.claims.sub, " subject-123 ");
+        assert_eq!(authorization.claims.email, " user@example.test ");
+        assert_eq!(authorization.claims.username, " user ");
+        assert_eq!(authorization.claims.groups, ["source-ops", "source-developers", "source-ops"]);
+        assert_eq!(authorization.claims.raw, raw);
+        assert_eq!(authorization.policies, ["mapped-source-developers", "mapped-source-ops"]);
+        assert_eq!(authorization.groups, ["source-developers", "source-ops"]);
+        assert_eq!(authorization.roles_claim_key.as_deref(), Some("roles"));
+        assert_eq!(authorization.roles, ["reader", "admin"]);
+    }
 }

crates/iam/src/federation/transaction.rs

@@ -140,29 +140,51 @@ mod tests {
     };
     use crate::oidc::{OidcProviderConfig, OidcProviderSummary};
     use rustfs_credentials::Credentials;
-    use std::sync::{
-        Arc, Mutex,
-        atomic::{AtomicUsize, Ordering},
-    };
+    use std::sync::{Arc, Mutex};
+
+    #[derive(Clone, Copy, PartialEq, Eq)]
+    enum ProviderFailure {
+        None,
+        Exchange,
+        Verification,
+        Logout,
+    }
 
     struct TestProvider {
         with_policy: bool,
+        with_group: bool,
+        browser_provider_id: &'static str,
+        web_provider_id: &'static str,
+        failure: ProviderFailure,
         events: Arc<Mutex<Vec<&'static str>>>,
+        expected_logout: (&'static str, &'static str),
     }
 
     impl TestProvider {
+        fn new(events: Arc<Mutex<Vec<&'static str>>>) -> Self {
+            Self {
+                with_policy: true,
+                with_group: false,
+                browser_provider_id: "default",
+                web_provider_id: "default",
+                failure: ProviderFailure::None,
+                events,
+                expected_logout: ("default", "id-token"),
+            }
+        }
+
         fn record(&self, event: &'static str) {
             self.events.lock().expect("event log should not be poisoned").push(event);
         }
 
-        fn authorization(&self) -> FederatedAuthorization {
+        fn authorization(&self, provider_id: &str) -> FederatedAuthorization {
             FederatedAuthorization {
-                provider_id: "default".to_string(),
+                provider_id: provider_id.to_string(),
                 claims: FederatedClaims {
                     sub: "subject".to_string(),
                     email: String::new(),
                     username: "user".to_string(),
-                    groups: Vec::new(),
+                    groups: vec!["source-group".to_string()],
                     raw: Default::default(),
                 },
                 policies: if self.with_policy {
@@ -170,7 +192,11 @@ mod tests {
                 } else {
                     Vec::new()
                 },
-                groups: Vec::new(),
+                groups: if self.with_group {
+                    vec!["developers".to_string()]
+                } else {
+                    Vec::new()
+                },
                 roles_claim_key: None,
                 roles: Vec::new(),
             }
@@ -206,8 +232,11 @@ mod tests {
 
         async fn exchange_code(&self, _state: &str, _code: &str, _redirect_uri: &str) -> Result<FederatedCodeExchange> {
             self.record("exchange");
+            if self.failure == ProviderFailure::Exchange {
+                return Err(FederationError::CodeExchange("exchange failed".to_string()));
+            }
             Ok(FederatedCodeExchange {
-                authorization: self.authorization(),
+                authorization: self.authorization(self.browser_provider_id),
                 redirect_after: Some("/browser".to_string()),
                 id_token: "id-token".to_string(),
             })
@@ -215,11 +244,18 @@ mod tests {
 
         async fn verify_web_identity_token(&self, _jwt: &str) -> Result<FederatedAuthorization> {
             self.record("verify");
-            Ok(self.authorization())
+            if self.failure == ProviderFailure::Verification {
+                return Err(FederationError::TokenVerification("verification failed".to_string()));
+            }
+            Ok(self.authorization(self.web_provider_id))
         }
 
-        async fn create_logout_token(&self, _provider_id: &str, _id_token: &str) -> Result<String> {
+        async fn create_logout_token(&self, provider_id: &str, id_token: &str) -> Result<String> {
             self.record("logout");
+            assert_eq!((provider_id, id_token), self.expected_logout);
+            if self.failure == ProviderFailure::Logout {
+                return Err(FederationError::Logout("logout failed".to_string()));
+            }
             Ok("logout-token".to_string())
         }
 
@@ -228,19 +264,37 @@ mod tests {
         }
     }
 
-    struct CountingBinding {
-        calls: AtomicUsize,
+    struct RecordingBinding {
+        fail: bool,
         events: Arc<Mutex<Vec<&'static str>>>,
+        transactions: Mutex<Vec<(String, usize, Option<String>)>>,
+    }
+
+    impl RecordingBinding {
+        fn new(events: Arc<Mutex<Vec<&'static str>>>) -> Self {
+            Self {
+                fail: false,
+                events,
+                transactions: Mutex::new(Vec::new()),
+            }
+        }
     }
 
     #[async_trait::async_trait]
-    impl FederatedSessionBinding for CountingBinding {
+    impl FederatedSessionBinding for RecordingBinding {
         async fn bind(
             &self,
             transaction: &FederatedSessionTransaction,
         ) -> core::result::Result<Credentials, FederatedSessionBindingError> {
-            self.calls.fetch_add(1, Ordering::Relaxed);
             self.events.lock().expect("event log should not be poisoned").push("bind");
+            self.transactions.lock().expect("transactions should not be poisoned").push((
+                transaction.authorization.provider_id.clone(),
+                transaction.duration_seconds,
+                transaction.session_policy.clone(),
+            ));
+            if self.fail {
+                return Err(FederatedSessionBindingError::Internal("binding failed".to_string()));
+            }
             Ok(Credentials {
                 access_key: transaction.authorization.claims.session_identity(),
                 ..Default::default()
@@ -249,16 +303,14 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn callback_and_web_identity_share_session_binding() {
+    async fn callback_and_web_identity_preserve_provider_and_transaction_boundaries() {
         let events = Arc::new(Mutex::new(Vec::new()));
-        let provider = Arc::new(TestProvider {
-            with_policy: true,
-            events: events.clone(),
-        });
-        let binding = Arc::new(CountingBinding {
-            calls: AtomicUsize::new(0),
-            events: events.clone(),
-        });
+        let mut provider = TestProvider::new(events.clone());
+        provider.browser_provider_id = "corp";
+        provider.web_provider_id = "partner";
+        provider.expected_logout = ("corp", "id-token");
+        let provider = Arc::new(provider);
+        let binding = Arc::new(RecordingBinding::new(events.clone()));
         let service = FederatedIdentityService::new(FederatedIdentityRegistry::new(provider));
 
         let login = service
@@ -266,6 +318,7 @@ mod tests {
             .await
             .expect("callback flow should complete");
         assert_eq!(login.session.credentials.access_key, "user");
+        assert_eq!(login.session.authorization.provider_id, "corp");
         assert_eq!(login.redirect_after.as_deref(), Some("/browser"));
         assert_eq!(login.logout_token, "logout-token");
         assert_eq!(
@@ -275,25 +328,32 @@ mod tests {
         events.lock().expect("event log should not be poisoned").clear();
 
         let web_identity = service
-            .assume_role_with_web_identity("jwt", 3600, None, binding.as_ref())
+            .assume_role_with_web_identity("jwt", 7200, Some("session-policy".to_string()), binding.as_ref())
             .await
             .expect("web identity flow should complete");
         assert_eq!(web_identity.credentials.access_key, "user");
-        assert_eq!(binding.calls.load(Ordering::Relaxed), 2);
+        assert_eq!(web_identity.authorization.provider_id, "partner");
         assert_eq!(events.lock().expect("event log should not be poisoned").as_slice(), ["verify", "bind"]);
+        assert_eq!(
+            binding
+                .transactions
+                .lock()
+                .expect("transactions should not be poisoned")
+                .as_slice(),
+            [
+                ("corp".to_string(), 3600, None),
+                ("partner".to_string(), 7200, Some("session-policy".to_string())),
+            ]
+        );
     }
 
     #[tokio::test]
     async fn web_identity_without_policy_or_group_is_not_bound() {
         let events = Arc::new(Mutex::new(Vec::new()));
-        let provider = Arc::new(TestProvider {
-            with_policy: false,
-            events: events.clone(),
-        });
-        let binding = Arc::new(CountingBinding {
-            calls: AtomicUsize::new(0),
-            events,
-        });
+        let mut provider = TestProvider::new(events.clone());
+        provider.with_policy = false;
+        let provider = Arc::new(provider);
+        let binding = Arc::new(RecordingBinding::new(events.clone()));
         let service = FederatedIdentityService::new(FederatedIdentityRegistry::new(provider));
 
         let error = service
@@ -302,6 +362,102 @@ mod tests {
             .expect_err("authorization context is required");
 
         assert!(matches!(error, FederationError::NoAuthorizationContext));
-        assert_eq!(binding.calls.load(Ordering::Relaxed), 0);
+        assert_eq!(events.lock().expect("event log should not be poisoned").as_slice(), ["verify"]);
+    }
+
+    #[tokio::test]
+    async fn web_identity_group_only_authorization_is_bound_once() {
+        let events = Arc::new(Mutex::new(Vec::new()));
+        let mut provider = TestProvider::new(events.clone());
+        provider.with_policy = false;
+        provider.with_group = true;
+        let provider = Arc::new(provider);
+        let binding = Arc::new(RecordingBinding::new(events.clone()));
+        let service = FederatedIdentityService::new(FederatedIdentityRegistry::new(provider));
+
+        let session = service
+            .assume_role_with_web_identity("jwt", 3600, None, binding.as_ref())
+            .await
+            .expect("a mapped group is an authorization context");
+
+        assert!(session.authorization.policies.is_empty());
+        assert_eq!(session.authorization.groups, ["developers"]);
+        assert_eq!(events.lock().expect("event log should not be poisoned").as_slice(), ["verify", "bind"]);
+    }
+
+    #[tokio::test]
+    async fn callback_failures_preserve_existing_side_effect_order() {
+        for (provider_failure, binding_failure, expected_events) in [
+            (ProviderFailure::Exchange, false, vec!["exchange"]),
+            (ProviderFailure::None, true, vec!["exchange", "bind"]),
+            (ProviderFailure::Logout, false, vec!["exchange", "bind", "logout"]),
+        ] {
+            let events = Arc::new(Mutex::new(Vec::new()));
+            let mut provider = TestProvider::new(events.clone());
+            provider.failure = provider_failure;
+            let mut binding = RecordingBinding::new(events.clone());
+            binding.fail = binding_failure;
+            let binding = Arc::new(binding);
+            let service = FederatedIdentityService::new(FederatedIdentityRegistry::new(Arc::new(provider)));
+
+            let error = service
+                .complete_authorization_code("state", "code", "https://console.example/callback", 3600, binding.as_ref())
+                .await
+                .expect_err("the configured failure should be returned");
+
+            if provider_failure == ProviderFailure::Exchange {
+                assert!(matches!(error, FederationError::CodeExchange(ref message) if message == "exchange failed"));
+            } else if binding_failure {
+                assert!(matches!(
+                    error,
+                    FederationError::Binding(FederatedSessionBindingError::Internal(ref message))
+                        if message == "binding failed"
+                ));
+            } else {
+                assert!(matches!(error, FederationError::Logout(ref message) if message == "logout failed"));
+            }
+
+            assert_eq!(
+                events.lock().expect("event log should not be poisoned").as_slice(),
+                expected_events,
+                "later callback steps must not run after a failure"
+            );
+        }
+    }
+
+    #[tokio::test]
+    async fn web_identity_failures_preserve_existing_side_effect_order() {
+        for (provider_failure, binding_failure, expected_events) in [
+            (ProviderFailure::Verification, false, vec!["verify"]),
+            (ProviderFailure::None, true, vec!["verify", "bind"]),
+        ] {
+            let events = Arc::new(Mutex::new(Vec::new()));
+            let mut provider = TestProvider::new(events.clone());
+            provider.failure = provider_failure;
+            let mut binding = RecordingBinding::new(events.clone());
+            binding.fail = binding_failure;
+            let binding = Arc::new(binding);
+            let service = FederatedIdentityService::new(FederatedIdentityRegistry::new(Arc::new(provider)));
+
+            let error = service
+                .assume_role_with_web_identity("jwt", 3600, None, binding.as_ref())
+                .await
+                .expect_err("the configured failure should be returned");
+
+            if provider_failure == ProviderFailure::Verification {
+                assert!(matches!(error, FederationError::TokenVerification(_)));
+            } else {
+                assert!(matches!(
+                    error,
+                    FederationError::Binding(FederatedSessionBindingError::Internal(ref message))
+                        if message == "binding failed"
+                ));
+            }
+            assert_eq!(
+                events.lock().expect("event log should not be poisoned").as_slice(),
+                expected_events,
+                "later web identity steps must not run after a failure"
+            );
+        }
     }
 }

crates/iam/src/oidc.rs

@@ -2007,6 +2007,42 @@ fn claim_value_type_for_log(value: Option<&serde_json::Value>) -> &'static str {
     }
 }
 
+#[cfg(test)]
+pub(crate) fn make_test_sys(configs: Vec<OidcProviderConfig>) -> OidcSys {
+    let configs = configs.into_iter().map(|config| (config.id.clone(), config)).collect();
+    OidcSys {
+        configs,
+        provider_states: RwLock::new(HashMap::new()),
+        state_store: OidcStateStore::new(),
+        http_client: ReqwestHttpClient::new().expect("failed to initialize OIDC HTTP clients"),
+    }
+}
+
+#[cfg(test)]
+pub(crate) fn test_config(id: &str) -> OidcProviderConfig {
+    OidcProviderConfig {
+        id: id.to_string(),
+        enabled: true,
+        config_url: format!("https://example.com/{id}/.well-known/openid-configuration"),
+        issuer: None,
+        client_id: "client-id".to_string(),
+        client_secret: None,
+        scopes: vec!["openid".to_string()],
+        other_audiences: vec![],
+        redirect_uri: None,
+        redirect_uri_dynamic: true,
+        claim_name: "groups".to_string(),
+        claim_prefix: String::new(),
+        role_policy: String::new(),
+        display_name: id.to_string(),
+        groups_claim: "groups".to_string(),
+        roles_claim: String::new(),
+        email_claim: "email".to_string(),
+        username_claim: "preferred_username".to_string(),
+        hide_from_ui: false,
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -2958,44 +2994,6 @@ mod tests {
         handle.join().expect("mock body server thread should exit");
     }
 
-    /// Helper to create an OidcSys with configs only (no provider states needed).
-    fn make_test_sys(configs: Vec<OidcProviderConfig>) -> OidcSys {
-        let mut config_map = HashMap::new();
-        for c in configs {
-            config_map.insert(c.id.clone(), c);
-        }
-        OidcSys {
-            configs: config_map,
-            provider_states: RwLock::new(HashMap::new()),
-            state_store: OidcStateStore::new(),
-            http_client: ReqwestHttpClient::new().expect("failed to initialize OIDC HTTP clients"),
-        }
-    }
-
-    fn test_config(id: &str) -> OidcProviderConfig {
-        OidcProviderConfig {
-            id: id.to_string(),
-            enabled: true,
-            config_url: format!("https://example.com/{id}/.well-known/openid-configuration"),
-            issuer: None,
-            client_id: "client-id".to_string(),
-            client_secret: None,
-            scopes: vec!["openid".to_string()],
-            other_audiences: vec![],
-            redirect_uri: None,
-            redirect_uri_dynamic: true,
-            claim_name: "groups".to_string(),
-            claim_prefix: "".to_string(),
-            role_policy: "".to_string(),
-            display_name: id.to_string(),
-            groups_claim: "groups".to_string(),
-            roles_claim: String::new(),
-            email_claim: "email".to_string(),
-            username_claim: "preferred_username".to_string(),
-            hide_from_ui: false,
-        }
-    }
-
     #[test]
     fn test_oidc_provider_config_debug_redacts_client_secret() {
         let config = OidcProviderConfig {

crates/iam/src/oidc_state.rs

@@ -138,6 +138,8 @@ impl Default for OidcStateStore {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::sync::Arc;
+    use tokio::sync::Barrier;
 
     #[tokio::test]
     async fn test_state_store_insert_and_take() {
@@ -197,6 +199,41 @@ mod tests {
         }
     }
 
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn auth_state_is_consumed_once_under_concurrent_take() {
+        let store = OidcStateStore::new();
+        store
+            .insert(
+                "state_once".to_string(),
+                OidcAuthSession {
+                    provider_id: "corp".to_string(),
+                    pkce_verifier: "verifier".to_string(),
+                    nonce: "nonce".to_string(),
+                    redirect_after: None,
+                },
+            )
+            .await;
+
+        let first_store = store.clone();
+        let second_store = store.clone();
+        let barrier = Arc::new(Barrier::new(3));
+        let first_barrier = Arc::clone(&barrier);
+        let first = tokio::spawn(async move {
+            first_barrier.wait().await;
+            first_store.take("state_once").await
+        });
+        let second_barrier = Arc::clone(&barrier);
+        let second = tokio::spawn(async move {
+            second_barrier.wait().await;
+            second_store.take("state_once").await
+        });
+        barrier.wait().await;
+        let first = first.await.expect("first state consumer should finish");
+        let second = second.await.expect("second state consumer should finish");
+
+        assert_eq!(first.is_some() as usize + second.is_some() as usize, 1);
+    }
+
     #[tokio::test]
     async fn test_logout_state_store_insert_and_take() {
         let store = OidcStateStore::new();