fix(policy): address review feedback from #2018 (#2021)

Signed-off-by: 安正超 <anzhengchao@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>

crates/policy/src/policy/function.rs

@@ -355,6 +355,34 @@ mod tests {
         }"# => true;
         "6"
     )]
+    #[test_case(
+        r#"{
+            "NumericLessThanEquals": {
+                "s3:max-keys": "10"
+            }
+        }"# => true; "numeric_less_than_equals"
+    )]
+    #[test_case(
+        r#"{
+            "DateLessThan": {
+                "aws:CurrentTime": "2026-01-01T00:00:00Z"
+            }
+        }"# => true; "date_less_than"
+    )]
+    #[test_case(
+        r#"{
+            "StringLikeIfExists": {
+                "aws:Referer": "http://www.example.com/*"
+            }
+        }"# => true; "string_like_if_exists"
+    )]
+    #[test_case(
+        r#"{
+            "ArnLike": {
+                "aws:SourceArn": "arn:aws:s3:::my-bucket"
+            }
+        }"# => true; "arn_like"
+    )]
     fn test_de(input: &str) -> bool {
         serde_json::from_str::<Functions>(input)
             .map_err(|e| eprintln!("{e:?}"))

crates/policy/src/policy/function/condition.rs

@@ -78,11 +78,13 @@ impl Condition {
             "NumericEquals" => Self::NumericEquals(d.next_value()?),
             "NumericNotEquals" => Self::NumericNotEquals(d.next_value()?),
             "NumericLessThan" => Self::NumericLessThan(d.next_value()?),
+            "NumericLessThanEquals" => Self::NumericLessThanEquals(d.next_value()?),
             "NumericGreaterThan" => Self::NumericGreaterThan(d.next_value()?),
             "NumericGreaterThanIfExists" => Self::NumericGreaterThanIfExists(d.next_value()?),
             "NumericGreaterThanEquals" => Self::NumericGreaterThanEquals(d.next_value()?),
             "DateEquals" => Self::DateEquals(d.next_value()?),
             "DateNotEquals" => Self::DateNotEquals(d.next_value()?),
+            "DateLessThan" => Self::DateLessThan(d.next_value()?),
             "DateLessThanEquals" => Self::DateLessThanEquals(d.next_value()?),
             "DateGreaterThan" => Self::DateGreaterThan(d.next_value()?),
             "DateGreaterThanEquals" => Self::DateGreaterThanEquals(d.next_value()?),
@@ -131,7 +133,7 @@ impl Condition {
 
     pub fn to_key_with_suffix(&self) -> String {
         match self {
-            Condition::IfExists(inner) => format!("{}IfExists", inner.to_key()),
+            Condition::IfExists(inner) => format!("{}IfExists", inner.to_key_with_suffix()),
             _ => self.to_key().to_owned(),
         }
     }
@@ -303,6 +305,7 @@ impl PartialEq for Condition {
 mod tests {
     use super::*;
     use crate::policy::function::{
+        Functions,
         func::{FuncKeyValue, InnerFunc},
         key::Key,
         string::StringFuncValue,
@@ -384,4 +387,90 @@ mod tests {
             "StringNotEquals should be true when key is absent"
         );
     }
+
+    #[test]
+    fn test_to_key_with_suffix_if_exists() {
+        let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms");
+        let cond = Condition::IfExists(Box::new(inner));
+        assert_eq!(cond.to_key_with_suffix(), "StringEqualsIfExists");
+    }
+
+    #[test]
+    fn test_to_key_with_suffix_nested_if_exists() {
+        // IfExists(IfExists(StringEquals)) must produce a stable, predictable key
+        let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms");
+        let once = Condition::IfExists(Box::new(inner));
+        let twice = Condition::IfExists(Box::new(once));
+        assert_eq!(twice.to_key_with_suffix(), "StringEqualsIfExistsIfExists");
+    }
+
+    #[test]
+    fn test_if_exists_serde_round_trip() {
+        let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms");
+        let cond = Condition::IfExists(Box::new(inner));
+
+        let functions = Functions {
+            for_normal: vec![cond],
+            ..Default::default()
+        };
+
+        let json = serde_json::to_string(&functions).unwrap();
+        assert_eq!(json, r#"{"StringEqualsIfExists":{"s3:x-amz-server-side-encryption":"aws:kms"}}"#);
+
+        let deserialized: Functions = serde_json::from_str(&json).unwrap();
+        assert_eq!(functions, deserialized);
+    }
+
+    #[test]
+    fn test_nested_if_exists_serde_round_trip() {
+        // Verifies that nested IfExists(IfExists(StringEquals)) serializes with the
+        // correct key and round-trips through serde without data loss.
+        let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms");
+        let once = Condition::IfExists(Box::new(inner));
+        let twice = Condition::IfExists(Box::new(once));
+
+        let functions = Functions {
+            for_normal: vec![twice],
+            ..Default::default()
+        };
+
+        let json = serde_json::to_string(&functions).unwrap();
+        assert_eq!(json, r#"{"StringEqualsIfExistsIfExists":{"s3:x-amz-server-side-encryption":"aws:kms"}}"#);
+
+        let deserialized: Functions = serde_json::from_str(&json).unwrap();
+        assert_eq!(functions, deserialized);
+    }
+
+    #[tokio::test]
+    async fn test_if_exists_key_absent_returns_true() {
+        let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms");
+        let cond = Condition::IfExists(Box::new(inner));
+
+        let values = HashMap::new();
+        assert!(
+            cond.evaluate_with_resolver(false, &values, None).await,
+            "IfExists should return true when the key is absent"
+        );
+    }
+
+    #[tokio::test]
+    async fn test_if_exists_key_present_delegates() {
+        let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms");
+        let cond = Condition::IfExists(Box::new(inner));
+
+        let mut values = HashMap::new();
+        values.insert("x-amz-server-side-encryption".to_string(), vec!["aws:kms".to_string()]);
+
+        assert!(
+            cond.evaluate_with_resolver(false, &values, None).await,
+            "IfExists should delegate to inner and return true when values match"
+        );
+
+        values.insert("x-amz-server-side-encryption".to_string(), vec!["AES256".to_string()]);
+
+        assert!(
+            !cond.evaluate_with_resolver(false, &values, None).await,
+            "IfExists should delegate to inner and return false when values differ"
+        );
+    }
 }

crates/policy/src/policy/principal.rs

@@ -130,9 +130,11 @@ impl Principal {
                 return true;
             }
         }
-        // Service principals (e.g., logging.s3.amazonaws.com) allow internal
-        // AWS services. Treat them as non-matching for user requests — they
-        // only apply to service-initiated actions.
+        for pattern in self.service.iter() {
+            if wildcard::is_simple_match(pattern, principal) {
+                return true;
+            }
+        }
         false
     }
 }
@@ -214,4 +216,28 @@ mod test {
         let reparsed: Principal = serde_json::from_str(&serialized).expect("Should re-parse");
         assert_eq!(principal, reparsed);
     }
+
+    #[test]
+    fn test_principal_is_match_service() {
+        let principal = Principal {
+            aws: HashSet::new(),
+            service: HashSet::from(["logging.s3.amazonaws.com".to_string()]),
+        };
+
+        assert!(principal.is_match("logging.s3.amazonaws.com"));
+        assert!(!principal.is_match("replication.s3.amazonaws.com"));
+        assert!(!principal.is_match("arn:aws:iam::123456789012:root"));
+    }
+
+    #[test]
+    fn test_principal_is_match_aws_and_service() {
+        let principal = Principal {
+            aws: HashSet::from(["arn:aws:iam::123456789012:root".to_string()]),
+            service: HashSet::from(["logging.s3.amazonaws.com".to_string()]),
+        };
+
+        assert!(principal.is_match("arn:aws:iam::123456789012:root"));
+        assert!(principal.is_match("logging.s3.amazonaws.com"));
+        assert!(!principal.is_match("other-principal"));
+    }
 }

rustfs/src/storage/access.rs

@@ -938,8 +938,11 @@ impl S3Access for FS {
     /// Checks whether the GetBucketLogging request has accesses to the resources.
     ///
     /// This method returns `Ok(())` by default.
-    async fn get_bucket_logging(&self, _req: &mut S3Request<GetBucketLoggingInput>) -> S3Result<()> {
-        Ok(())
+    async fn get_bucket_logging(&self, req: &mut S3Request<GetBucketLoggingInput>) -> S3Result<()> {
+        let req_info = ext_req_info_mut(&mut req.extensions)?;
+        req_info.bucket = Some(req.input.bucket.clone());
+
+        authorize_request(req, Action::S3Action(S3Action::GetBucketLoggingAction)).await
     }
 
     /// Checks whether the GetBucketMetricsConfiguration request has accesses to the resources.