fix(policy): align action-family validation and defaults (#2984)

* fix(policy): align action-family validation and defaults

* test(e2e): add accountinfo service-account roundtrip

* test(policy): add mixed action family cases

crates/e2e_test/src/replication_extension_test.rs

@@ -651,6 +651,22 @@ async fn list_service_accounts(
     Ok(response.json().await?)
 }
 
+async fn get_account_info(
+    env: &RustFSTestEnvironment,
+    signer_access_key: &str,
+    signer_secret_key: &str,
+) -> Result<serde_json::Value, Box<dyn Error + Send + Sync>> {
+    let url = format!("{}/rustfs/admin/v3/accountinfo", env.url);
+    let response = signed_request(http::Method::GET, &url, signer_access_key, signer_secret_key, None, None).await?;
+    if response.status() != StatusCode::OK {
+        let status = response.status();
+        let body = response.text().await.unwrap_or_default();
+        return Err(format!("account info failed: {status} {body}").into());
+    }
+
+    Ok(response.json().await?)
+}
+
 async fn wait_for_service_accounts(
     env: &RustFSTestEnvironment,
     signer_access_key: &str,
@@ -2379,6 +2395,57 @@ async fn test_site_replication_replicates_group_policy_backed_access_real_dual_n
     Ok(())
 }
 
+#[tokio::test]
+#[serial]
+async fn test_service_account_policy_from_accountinfo_round_trips_real_single_node() -> TestResult {
+    init_logging();
+
+    let mut env = RustFSTestEnvironment::new().await?;
+    env.start_rustfs_server(vec![]).await?;
+
+    let account_info = get_account_info(&env, &env.access_key, &env.secret_key).await?;
+    let policy_str = account_info
+        .get("policy")
+        .and_then(|value| value.as_str())
+        .ok_or("account info policy should be a JSON string")?;
+
+    let policy: serde_json::Value = serde_json::from_str(policy_str)?;
+    let statements = policy
+        .get("Statement")
+        .and_then(|value| value.as_array())
+        .ok_or("account info policy should include Statement array")?;
+
+    assert!(!statements.is_empty(), "account info policy Statement should not be empty: {policy}");
+
+    let req = AddServiceAccountReq {
+        policy: Some(policy),
+        target_user: None,
+        access_key: "svcacct-info-sample".to_string(),
+        secret_key: "svcacct-info-sample-secret-key-123456".to_string(),
+        name: Some("svcacct-info-sample".to_string()),
+        description: Some("service account created from accountinfo sample policy".to_string()),
+        expiration: None,
+        comment: None,
+    };
+
+    let created = add_service_account(&env, &env.access_key, &env.secret_key, &req).await?;
+    assert_eq!(created.0, "svcacct-info-sample");
+
+    let listed =
+        wait_for_service_accounts(&env, &env.access_key, &env.secret_key, Some(&env.access_key), &["svcacct-info-sample"])
+            .await?;
+    assert!(
+        listed
+            .accounts
+            .iter()
+            .any(|account| account.access_key == "svcacct-info-sample"),
+        "created service account should be listed for parent user: {:?}",
+        listed.accounts
+    );
+
+    Ok(())
+}
+
 #[tokio::test]
 #[serial]
 async fn test_site_replication_replicates_multiple_service_accounts_real_dual_node() -> Result<(), Box<dyn Error + Send + Sync>> {

crates/policy/src/policy.rs

@@ -67,6 +67,9 @@ pub enum Error {
     #[error("invalid action: '{0}'")]
     InvalidAction(String),
 
+    #[error("'Action' contains mixed action families in the same statement")]
+    MixedActionFamilies,
+
     #[error("invalid resource, type: '{0}', pattern: '{1}'")]
     InvalidResource(String, String),
 }

crates/policy/src/policy/policy.rs

@@ -364,31 +364,45 @@ pub mod default {
     use super::Policy;
 
     #[allow(clippy::incompatible_msrv)]
-    pub static DEFAULT_POLICIES: LazyLock<[(&'static str, Policy); 6]> = LazyLock::new(|| {
+    pub static DEFAULT_POLICIES: LazyLock<[(&'static str, Policy); 5]> = LazyLock::new(|| {
         [
             (
                 "readwrite",
                 Policy {
                     id: "".into(),
                     version: DEFAULT_VERSION.into(),
-                    statements: vec![Statement {
+                    statements: vec![
-                        sid: "".into(),
+                        Statement {
-                        effect: Effect::Allow,
+                            sid: "".into(),
-                        actions: ActionSet({
+                            effect: Effect::Allow,
-                            let mut hash_set = HashSet::new();
+                            actions: ActionSet({
-                            hash_set.insert(Action::S3Action(S3Action::AllActions));
+                                let mut hash_set = HashSet::new();
-                            hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                                hash_set.insert(Action::S3Action(S3Action::AllActions));
-                            hash_set
+                                hash_set
-                        }),
+                            }),
-                        not_actions: ActionSet(Default::default()),
+                            not_actions: ActionSet(Default::default()),
-                        resources: ResourceSet({
+                            resources: ResourceSet({
-                            let mut hash_set = HashSet::new();
+                                let mut hash_set = HashSet::new();
-                            hash_set.insert(Resource::S3("*".into()));
+                                hash_set.insert(Resource::S3("*".into()));
-                            hash_set
+                                hash_set
-                        }),
+                            }),
-                        conditions: Functions::default(),
+                            conditions: Functions::default(),
-                        ..Default::default()
+                            ..Default::default()
-                    }],
+                        },
+                        Statement {
+                            sid: "".into(),
+                            effect: Effect::Allow,
+                            actions: ActionSet({
+                                let mut hash_set = HashSet::new();
+                                hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                                hash_set
+                            }),
+                            not_actions: ActionSet(Default::default()),
+                            resources: ResourceSet(Default::default()),
+                            conditions: Functions::default(),
+                            ..Default::default()
+                        },
+                    ],
                 },
             ),
             (
@@ -396,26 +410,40 @@ pub mod default {
                 Policy {
                     id: "".into(),
                     version: DEFAULT_VERSION.into(),
-                    statements: vec![Statement {
+                    statements: vec![
-                        sid: "".into(),
+                        Statement {
-                        effect: Effect::Allow,
+                            sid: "".into(),
-                        actions: ActionSet({
+                            effect: Effect::Allow,
-                            let mut hash_set = HashSet::new();
+                            actions: ActionSet({
-                            hash_set.insert(Action::S3Action(S3Action::GetBucketLocationAction));
+                                let mut hash_set = HashSet::new();
-                            hash_set.insert(Action::S3Action(S3Action::GetObjectAction));
+                                hash_set.insert(Action::S3Action(S3Action::GetBucketLocationAction));
-                            hash_set.insert(Action::S3Action(S3Action::GetBucketQuotaAction));
+                                hash_set.insert(Action::S3Action(S3Action::GetObjectAction));
-                            hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                                hash_set.insert(Action::S3Action(S3Action::GetBucketQuotaAction));
-                            hash_set
+                                hash_set
-                        }),
+                            }),
-                        not_actions: ActionSet(Default::default()),
+                            not_actions: ActionSet(Default::default()),
-                        resources: ResourceSet({
+                            resources: ResourceSet({
-                            let mut hash_set = HashSet::new();
+                                let mut hash_set = HashSet::new();
-                            hash_set.insert(Resource::S3("*".into()));
+                                hash_set.insert(Resource::S3("*".into()));
-                            hash_set
+                                hash_set
-                        }),
+                            }),
-                        conditions: Functions::default(),
+                            conditions: Functions::default(),
-                        ..Default::default()
+                            ..Default::default()
-                    }],
+                        },
+                        Statement {
+                            sid: "".into(),
+                            effect: Effect::Allow,
+                            actions: ActionSet({
+                                let mut hash_set = HashSet::new();
+                                hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                                hash_set
+                            }),
+                            not_actions: ActionSet(Default::default()),
+                            resources: ResourceSet(Default::default()),
+                            conditions: Functions::default(),
+                            ..Default::default()
+                        },
+                    ],
                 },
             ),
             (
@@ -423,49 +451,38 @@ pub mod default {
                 Policy {
                     id: "".into(),
                     version: DEFAULT_VERSION.into(),
-                    statements: vec![Statement {
+                    statements: vec![
-                        sid: "".into(),
+                        Statement {
-                        effect: Effect::Allow,
+                            sid: "".into(),
-                        actions: ActionSet({
+                            effect: Effect::Allow,
-                            let mut hash_set = HashSet::new();
+                            actions: ActionSet({
-                            hash_set.insert(Action::S3Action(S3Action::PutObjectAction));
+                                let mut hash_set = HashSet::new();
-                            hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                                hash_set.insert(Action::S3Action(S3Action::PutObjectAction));
-                            hash_set
+                                hash_set
-                        }),
+                            }),
-                        not_actions: ActionSet(Default::default()),
+                            not_actions: ActionSet(Default::default()),
-                        resources: ResourceSet({
+                            resources: ResourceSet({
-                            let mut hash_set = HashSet::new();
+                                let mut hash_set = HashSet::new();
-                            hash_set.insert(Resource::S3("*".into()));
+                                hash_set.insert(Resource::S3("*".into()));
-                            hash_set
+                                hash_set
-                        }),
+                            }),
-                        conditions: Functions::default(),
+                            conditions: Functions::default(),
-                        ..Default::default()
+                            ..Default::default()
-                    }],
+                        },
-                },
+                        Statement {
-            ),
+                            sid: "".into(),
-            (
+                            effect: Effect::Allow,
-                "writeonly",
+                            actions: ActionSet({
-                Policy {
+                                let mut hash_set = HashSet::new();
-                    id: "".into(),
+                                hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
-                    version: DEFAULT_VERSION.into(),
+                                hash_set
-                    statements: vec![Statement {
+                            }),
-                        sid: "".into(),
+                            not_actions: ActionSet(Default::default()),
-                        effect: Effect::Allow,
+                            resources: ResourceSet(Default::default()),
-                        actions: ActionSet({
+                            conditions: Functions::default(),
-                            let mut hash_set = HashSet::new();
+                            ..Default::default()
-                            hash_set.insert(Action::S3Action(S3Action::PutObjectAction));
+                        },
-                            hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                    ],
-                            hash_set
-                        }),
-                        not_actions: ActionSet(Default::default()),
-                        resources: ResourceSet({
-                            let mut hash_set = HashSet::new();
-                            hash_set.insert(Resource::S3("*".into()));
-                            hash_set
-                        }),
-                        conditions: Functions::default(),
-                        ..Default::default()
-                    }],
                 },
             ),
             (
@@ -473,31 +490,45 @@ pub mod default {
                 Policy {
                     id: "".into(),
                     version: DEFAULT_VERSION.into(),
-                    statements: vec![Statement {
+                    statements: vec![
-                        sid: "".into(),
+                        Statement {
-                        effect: Effect::Allow,
+                            sid: "".into(),
-                        actions: ActionSet({
+                            effect: Effect::Allow,
-                            let mut hash_set = HashSet::new();
+                            actions: ActionSet({
-                            hash_set.insert(Action::AdminAction(AdminAction::ProfilingAdminAction));
+                                let mut hash_set = HashSet::new();
-                            hash_set.insert(Action::AdminAction(AdminAction::TraceAdminAction));
+                                hash_set.insert(Action::AdminAction(AdminAction::ProfilingAdminAction));
-                            hash_set.insert(Action::AdminAction(AdminAction::ConsoleLogAdminAction));
+                                hash_set.insert(Action::AdminAction(AdminAction::TraceAdminAction));
-                            hash_set.insert(Action::AdminAction(AdminAction::ServerInfoAdminAction));
+                                hash_set.insert(Action::AdminAction(AdminAction::ConsoleLogAdminAction));
-                            hash_set.insert(Action::AdminAction(AdminAction::TopLocksAdminAction));
+                                hash_set.insert(Action::AdminAction(AdminAction::ServerInfoAdminAction));
-                            hash_set.insert(Action::AdminAction(AdminAction::HealthInfoAdminAction));
+                                hash_set.insert(Action::AdminAction(AdminAction::TopLocksAdminAction));
-                            hash_set.insert(Action::AdminAction(AdminAction::PrometheusAdminAction));
+                                hash_set.insert(Action::AdminAction(AdminAction::HealthInfoAdminAction));
-                            hash_set.insert(Action::AdminAction(AdminAction::BandwidthMonitorAction));
+                                hash_set.insert(Action::AdminAction(AdminAction::PrometheusAdminAction));
-                            hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                                hash_set.insert(Action::AdminAction(AdminAction::BandwidthMonitorAction));
-                            hash_set
+                                hash_set
-                        }),
+                            }),
-                        not_actions: ActionSet(Default::default()),
+                            not_actions: ActionSet(Default::default()),
-                        resources: ResourceSet({
+                            resources: ResourceSet({
-                            let mut hash_set = HashSet::new();
+                                let mut hash_set = HashSet::new();
-                            hash_set.insert(Resource::S3("*".into()));
+                                hash_set.insert(Resource::S3("*".into()));
-                            hash_set
+                                hash_set
-                        }),
+                            }),
-                        conditions: Functions::default(),
+                            conditions: Functions::default(),
-                        ..Default::default()
+                            ..Default::default()
-                    }],
+                        },
+                        Statement {
+                            sid: "".into(),
+                            effect: Effect::Allow,
+                            actions: ActionSet({
+                                let mut hash_set = HashSet::new();
+                                hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                                hash_set
+                            }),
+                            not_actions: ActionSet(Default::default()),
+                            resources: ResourceSet(Default::default()),
+                            conditions: Functions::default(),
+                            ..Default::default()
+                        },
+                    ],
                 },
             ),
             (
@@ -512,7 +543,6 @@ pub mod default {
                             actions: ActionSet({
                                 let mut hash_set = HashSet::new();
                                 hash_set.insert(Action::AdminAction(AdminAction::AllAdminActions));
-                                hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
                                 hash_set
                             }),
                             not_actions: ActionSet(Default::default()),
@@ -526,7 +556,6 @@ pub mod default {
                             actions: ActionSet({
                                 let mut hash_set = HashSet::new();
                                 hash_set.insert(Action::KmsAction(KmsAction::AllActions));
-                                hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
                                 hash_set
                             }),
                             not_actions: ActionSet(Default::default()),
@@ -540,7 +569,6 @@ pub mod default {
                             actions: ActionSet({
                                 let mut hash_set = HashSet::new();
                                 hash_set.insert(Action::S3Action(S3Action::AllActions));
-                                hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
                                 hash_set
                             }),
                             not_actions: ActionSet(Default::default()),
@@ -552,6 +580,19 @@ pub mod default {
                             conditions: Functions::default(),
                             ..Default::default()
                         },
+                        Statement {
+                            sid: "".into(),
+                            effect: Effect::Allow,
+                            actions: ActionSet({
+                                let mut hash_set = HashSet::new();
+                                hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
+                                hash_set
+                            }),
+                            not_actions: ActionSet(Default::default()),
+                            resources: ResourceSet(HashSet::new()),
+                            conditions: Functions::default(),
+                            ..Default::default()
+                        },
                     ],
                 },
             ),
@@ -713,6 +754,14 @@ mod test {
         }
     }
 
+    #[test]
+    fn test_default_policy_names_are_unique() {
+        let mut names = HashSet::new();
+        for (name, _) in default::DEFAULT_POLICIES.iter() {
+            assert!(names.insert(*name), "duplicate default policy name: {name}");
+        }
+    }
+
     #[tokio::test]
     async fn test_deny_only_checks_only_deny_statements() -> Result<()> {
         let data = r#"
@@ -1448,6 +1497,166 @@ mod test {
         );
     }
 
+    #[test]
+    fn test_admin_statement_without_resource_is_valid() {
+        let data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": ["admin:ServerInfo"]
+    }
+  ]
+}
+"#;
+
+        let result = Policy::parse_config(data.as_bytes());
+        assert!(
+            result.is_ok(),
+            "Admin-only Action statement without Resource should be valid, got: {:?}",
+            result.err()
+        );
+    }
+
+    #[test]
+    fn test_sts_statement_without_resource_is_valid() {
+        let data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": ["sts:AssumeRole"]
+    }
+  ]
+}
+"#;
+
+        let result = Policy::parse_config(data.as_bytes());
+        assert!(
+            result.is_ok(),
+            "STS-only Action statement without Resource should be valid, got: {:?}",
+            result.err()
+        );
+    }
+
+    #[test]
+    fn test_kms_statement_without_resource_is_valid() {
+        let data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": ["kms:*"]
+    }
+  ]
+}
+"#;
+
+        let result = Policy::parse_config(data.as_bytes());
+        assert!(
+            result.is_ok(),
+            "KMS-only Action statement without Resource should be valid, got: {:?}",
+            result.err()
+        );
+    }
+
+    #[test]
+    fn test_mixed_action_families_are_invalid_even_with_resource() {
+        let data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": ["admin:*", "sts:AssumeRole"],
+      "Resource": ["arn:aws:s3:::*"]
+    }
+  ]
+}
+"#;
+
+        let result = Policy::parse_config(data.as_bytes());
+        assert!(result.is_err(), "Mixed action families should be rejected");
+        assert!(
+            matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::MixedActionFamilies)),
+            "Error should be MixedActionFamilies, got: {:?}",
+            result.unwrap_err()
+        );
+    }
+
+    #[test]
+    fn test_mixed_action_families_are_invalid_even_without_resource() {
+        let data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": ["admin:*", "s3:GetObject"]
+    }
+  ]
+}
+"#;
+
+        let result = Policy::parse_config(data.as_bytes());
+        assert!(result.is_err(), "Mixed action families should be rejected even when Resource is missing");
+        assert!(
+            matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::MixedActionFamilies)),
+            "Error should be MixedActionFamilies, got: {:?}",
+            result.unwrap_err()
+        );
+    }
+
+    #[test]
+    fn test_mixed_action_families_with_wildcard_variants_are_invalid() {
+        let data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": ["s3:*", "admin:*", "sts:AssumeRole"],
+      "Resource": ["arn:aws:s3:::*"]
+    }
+  ]
+}
+"#;
+
+        let result = Policy::parse_config(data.as_bytes());
+        assert!(result.is_err(), "Mixed action families with wildcard variants should be rejected");
+        assert!(
+            matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::MixedActionFamilies)),
+            "Error should be MixedActionFamilies, got: {:?}",
+            result.unwrap_err()
+        );
+    }
+
+    #[test]
+    fn test_notaction_without_resource_remains_invalid() {
+        let data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "NotAction": ["s3:DeleteObject"]
+    }
+  ]
+}
+"#;
+
+        let result = Policy::parse_config(data.as_bytes());
+        assert!(result.is_err(), "NotAction statement without Resource should remain invalid");
+        assert!(
+            matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::NonResource)),
+            "Error should be NonResource, got: {:?}",
+            result.unwrap_err()
+        );
+    }
+
     #[test]
     fn test_bucket_policy_serialize_omits_empty_fields() {
         use crate::policy::action::{Action, ActionSet, S3Action};

crates/policy/src/policy/statement.rs

@@ -78,6 +78,15 @@ fn build_resource(action: &Action, bucket: &str, object: &str, bucket_resource_o
     resource
 }
 
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+enum ActionFamily {
+    S3,
+    Admin,
+    Sts,
+    Kms,
+    Mixed,
+}
+
 impl Statement {
     fn is_kms(&self) -> bool {
         for act in self.actions.iter() {
@@ -109,6 +118,48 @@ impl Statement {
         false
     }
 
+    fn action_family(&self) -> Option<ActionFamily> {
+        if self.actions.is_empty() {
+            return None;
+        }
+
+        let mut saw_s3 = false;
+        let mut saw_admin = false;
+        let mut saw_sts = false;
+        let mut saw_kms = false;
+
+        for action in self.actions.iter() {
+            match action {
+                Action::S3Action(_) => saw_s3 = true,
+                Action::AdminAction(_) => saw_admin = true,
+                Action::StsAction(_) => saw_sts = true,
+                Action::KmsAction(_) => saw_kms = true,
+                Action::None => {}
+            }
+        }
+
+        let family_count = saw_s3 as u8 + saw_admin as u8 + saw_sts as u8 + saw_kms as u8;
+
+        if family_count != 1 {
+            return Some(ActionFamily::Mixed);
+        }
+
+        if saw_s3 {
+            return Some(ActionFamily::S3);
+        }
+        if saw_admin {
+            return Some(ActionFamily::Admin);
+        }
+        if saw_sts {
+            return Some(ActionFamily::Sts);
+        }
+        if saw_kms {
+            return Some(ActionFamily::Kms);
+        }
+
+        Some(ActionFamily::Mixed)
+    }
+
     /// Returns true when this statement would reach `conditions.evaluate_with_resolver` in
     /// [`Statement::is_allowed`] (including the KMS shortcut path). Does not evaluate conditions.
     pub(crate) async fn request_reaches_condition_eval(&self, args: &Args<'_>, resolver: &VariableResolver) -> bool {
@@ -186,9 +237,25 @@ impl Validator for Statement {
             return Err(IamError::BothActionAndNotAction.into());
         }
 
-        // policy must contain either Resource or NotResource (but not both), and cannot have both empty.
+        let action_family = if self.not_actions.is_empty() {
+            match self.action_family() {
+                Some(ActionFamily::Mixed) => return Err(IamError::MixedActionFamilies.into()),
+                family => family,
+            }
+        } else {
+            None
+        };
+
+        // Policy must contain either Resource or NotResource (but not both), unless
+        // the statement is Action-mode Admin/STS/KMS.
         if self.resources.is_empty() && self.not_resources.is_empty() {
-            return Err(IamError::NonResource.into());
+            let allow_empty_resource = matches!(
+                action_family,
+                Some(ActionFamily::Admin) | Some(ActionFamily::Sts) | Some(ActionFamily::Kms)
+            );
+            if !allow_empty_resource {
+                return Err(IamError::NonResource.into());
+            }
         }
 
         if !self.resources.is_empty() && !self.not_resources.is_empty() {