From 62b51b564928a6ccc2f337dfda4a276655292356 Mon Sep 17 00:00:00 2001 From: GatewayJ <835269233@qq.com> Date: Wed, 25 Feb 2026 11:58:30 +0800 Subject: [PATCH] feat: admin permission check (#1783) Signed-off-by: GatewayJ <835269233@qq.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: houseme --- crates/policy/src/error.rs | 20 ++--- crates/policy/src/policy.rs | 3 + crates/policy/src/policy/policy.rs | 124 +++++++++++++++++++++----- crates/policy/src/policy/statement.rs | 20 +++-- rustfs/src/admin/auth.rs | 26 +++--- rustfs/src/admin/handlers/policies.rs | 3 +- 6 files changed, 140 insertions(+), 56 deletions(-) diff --git a/crates/policy/src/error.rs b/crates/policy/src/error.rs index 5a0adce1a..3963234bd 100644 --- a/crates/policy/src/error.rs +++ b/crates/policy/src/error.rs @@ -192,7 +192,6 @@ mod tests { match policy_error { Error::Io(inner_io) => { assert_eq!(inner_io.kind(), ErrorKind::PermissionDenied); - assert!(inner_io.to_string().contains("permission denied")); } _ => panic!("Expected Io variant"), } @@ -205,7 +204,6 @@ mod tests { match policy_error { Error::Io(io_error) => { - assert!(io_error.to_string().contains(custom_error)); assert_eq!(io_error.kind(), ErrorKind::Other); } _ => panic!("Expected Io variant"), @@ -218,12 +216,9 @@ mod tests { let crypto_error = rustfs_crypto::Error::ErrUnexpectedHeader; let policy_error: Error = crypto_error.into(); - match policy_error { - Error::CryptoError(_) => { - // Verify the conversion worked - assert!(policy_error.to_string().contains("crypto")); - } - _ => panic!("Expected CryptoError variant"), + match &policy_error { + Error::CryptoError(_) => {} + _ => panic!("Expected CryptoError variant, got {:?}", policy_error), } } @@ -249,12 +244,9 @@ mod tests { let jwt_error = jwt_result.unwrap_err(); let policy_error: Error = jwt_error.into(); - match policy_error { - Error::JWTError(_) => { - // Verify the conversion worked - assert!(policy_error.to_string().contains("jwt err")); - } - _ => panic!("Expected JWTError variant"), + match &policy_error { + Error::JWTError(_) => {} + _ => panic!("Expected JWTError variant, got {:?}", policy_error), } } diff --git a/crates/policy/src/policy.rs b/crates/policy/src/policy.rs index f732595f0..92329983f 100644 --- a/crates/policy/src/policy.rs +++ b/crates/policy/src/policy.rs @@ -48,6 +48,9 @@ pub enum Error { #[error("both 'Action' and 'NotAction' are empty")] NonAction, + #[error("'Action' and 'NotAction' cannot both be specified in the same statement")] + BothActionAndNotAction, + #[error("'Resource' is empty")] NonResource, diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 259f44057..f757529f4 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -1015,17 +1015,105 @@ mod test { let result = Policy::parse_config(data.as_bytes()); assert!(result.is_err(), "Statement with both Resource and NotResource should be invalid"); - // Verify the specific error type - if let Err(e) = result { - let error_msg = format!("{}", e); - assert!( - error_msg.contains("Resource") - && error_msg.contains("NotResource") - && error_msg.contains("cannot both be specified"), - "Error should be BothResourceAndNotResource, got: {}", - error_msg - ); - } + assert!( + matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::BothResourceAndNotResource)), + "Error should be BothResourceAndNotResource, got: {:?}", + result.unwrap_err() + ); + } + + #[test] + fn test_statement_with_only_notaction_is_valid() { + // IAM allows a statement with only NotAction (no Action). Deserialization should accept + // missing "Action" (default to empty) and validate when exactly one of Action/NotAction is set. + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "NotAction": ["s3:DeleteBucket", "s3:DeleteObject"], + "Resource": ["arn:aws:s3:::mybucket/*"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_ok(), "Statement with only NotAction should be valid, got: {:?}", result.err()); + let policy = result.unwrap(); + assert_eq!(policy.statements.len(), 1); + assert!(policy.statements[0].actions.is_empty()); + assert!(!policy.statements[0].not_actions.is_empty()); + + // Round-trip: serialization must omit empty Action so re-parse does not violate both-Action-and-NotAction + let json = serde_json::to_string(&policy).expect("Should serialize"); + let round_tripped = Policy::parse_config(json.as_bytes()); + assert!( + round_tripped.is_ok(), + "NotAction-only statement must round-trip without gaining Action: {}", + round_tripped.unwrap_err() + ); + let parsed: serde_json::Value = serde_json::from_str(&json).expect("JSON valid"); + let stmt = &parsed["Statement"][0]; + assert!( + !stmt + .get("Action") + .is_some_and(|v| v.as_array().map(|a| a.is_empty()).unwrap_or(false)), + "Serialized JSON must not contain empty Action for NotAction-only statement" + ); + } + + #[test] + fn test_statement_with_both_action_and_notaction_is_invalid() { + // Test: A statement with both Action and NotAction returns BothActionAndNotAction error + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:GetObject"], + "NotAction": ["s3:DeleteObject"], + "Resource": ["arn:aws:s3:::mybucket/*"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_err(), "Statement with both Action and NotAction should be invalid"); + + assert!( + matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::BothActionAndNotAction)), + "Error should be BothActionAndNotAction, got: {:?}", + result.unwrap_err() + ); + } + + #[test] + fn test_statement_with_neither_action_nor_notaction_is_invalid() { + // Statement with both Action and NotAction omitted (both default to empty) fails validation. + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Resource": ["arn:aws:s3:::mybucket/*"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_err(), "Statement with neither Action nor NotAction should be invalid"); + + assert!( + matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::NonAction)), + "Error should be NonAction, got: {:?}", + result.unwrap_err() + ); } #[test] @@ -1046,15 +1134,11 @@ mod test { let result = Policy::parse_config(data.as_bytes()); assert!(result.is_err(), "Statement with neither Resource nor NotResource should be invalid"); - // Verify the specific error type - if let Err(e) = result { - let error_msg = format!("{}", e); - assert!( - error_msg.contains("Resource") && error_msg.contains("empty"), - "Error should be NonResource, got: {}", - error_msg - ); - } + assert!( + matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::NonResource)), + "Error should be NonResource, got: {:?}", + result.unwrap_err() + ); } #[test] diff --git a/crates/policy/src/policy/statement.rs b/crates/policy/src/policy/statement.rs index 7d69a99ec..3dd18ef9a 100644 --- a/crates/policy/src/policy/statement.rs +++ b/crates/policy/src/policy/statement.rs @@ -26,13 +26,13 @@ pub struct Statement { pub sid: ID, #[serde(rename = "Effect")] pub effect: Effect, - #[serde(rename = "Action")] + #[serde(rename = "Action", default, skip_serializing_if = "ActionSet::is_empty")] pub actions: ActionSet, - #[serde(rename = "NotAction", default)] + #[serde(rename = "NotAction", default, skip_serializing_if = "ActionSet::is_empty")] pub not_actions: ActionSet, - #[serde(rename = "Resource", default)] + #[serde(rename = "Resource", default, skip_serializing_if = "ResourceSet::is_empty")] pub resources: ResourceSet, - #[serde(rename = "NotResource", default)] + #[serde(rename = "NotResource", default, skip_serializing_if = "ResourceSet::is_empty")] pub not_resources: ResourceSet, #[serde(rename = "Condition", default)] pub conditions: Functions, @@ -151,6 +151,10 @@ impl Validator for Statement { return Err(IamError::NonAction.into()); } + if !self.actions.is_empty() && !self.not_actions.is_empty() { + return Err(IamError::BothActionAndNotAction.into()); + } + // policy must contain either Resource or NotResource (but not both), and cannot have both empty. if self.resources.is_empty() && self.not_resources.is_empty() { return Err(IamError::NonResource.into()); @@ -190,11 +194,11 @@ pub struct BPStatement { pub effect: Effect, #[serde(rename = "Principal")] pub principal: Principal, - #[serde(rename = "Action")] + #[serde(rename = "Action", default, skip_serializing_if = "ActionSet::is_empty")] pub actions: ActionSet, #[serde(rename = "NotAction", default, skip_serializing_if = "ActionSet::is_empty")] pub not_actions: ActionSet, - #[serde(rename = "Resource", default)] + #[serde(rename = "Resource", default, skip_serializing_if = "ResourceSet::is_empty")] pub resources: ResourceSet, #[serde(rename = "NotResource", default, skip_serializing_if = "ResourceSet::is_empty")] pub not_resources: ResourceSet, @@ -252,6 +256,10 @@ impl Validator for BPStatement { return Err(IamError::NonAction.into()); } + if !self.actions.is_empty() && !self.not_actions.is_empty() { + return Err(IamError::BothActionAndNotAction.into()); + } + if self.resources.is_empty() && self.not_resources.is_empty() { return Err(IamError::NonResource.into()); } diff --git a/rustfs/src/admin/auth.rs b/rustfs/src/admin/auth.rs index cf11e2ef5..e698b5d7a 100644 --- a/rustfs/src/admin/auth.rs +++ b/rustfs/src/admin/auth.rs @@ -52,15 +52,14 @@ pub async fn validate_admin_request( remote_addr, }; - for action in actions { - match check_admin_request_auth(iam_store.clone(), &ctx, action, "", "").await { - Ok(_) => return Ok(()), - Err(_) => { - continue; - } + for action in &actions { + if check_admin_request_auth(iam_store.clone(), &ctx, *action, "", "") + .await + .is_ok() + { + return Ok(()); } } - Err(s3_error!(AccessDenied, "Access Denied")) } @@ -113,14 +112,13 @@ pub async fn validate_admin_request_with_bucket( remote_addr, }; - for action in actions { - match check_admin_request_auth(iam_store.clone(), &ctx, action, bucket, "").await { - Ok(_) => return Ok(()), - Err(_) => { - continue; - } + for action in &actions { + if check_admin_request_auth(iam_store.clone(), &ctx, *action, bucket, "") + .await + .is_ok() + { + return Ok(()); } } - Err(s3_error!(AccessDenied, "Access Denied")) } diff --git a/rustfs/src/admin/handlers/policies.rs b/rustfs/src/admin/handlers/policies.rs index 0ab37c6e9..b96ee8d2c 100644 --- a/rustfs/src/admin/handlers/policies.rs +++ b/rustfs/src/admin/handlers/policies.rs @@ -196,9 +196,8 @@ impl Operation for AddCannedPolicy { })?; if policy.version.is_empty() { - return Err(s3_error!(InvalidRequest, "policy version is empty")); + return Err(s3_error!(InvalidArgument, "policy version is empty")); } - let Ok(iam_store) = rustfs_iam::get() else { return Err(s3_error!(InternalError, "iam not init")) }; iam_store.set_policy(&query.name, policy).await.map_err(|e| {