fix(object-lock): recover remaining s3 tests (#2294)

crates/ecstore/src/bucket/object_lock/objectlock_sys.rs

@@ -52,6 +52,7 @@ pub fn is_retention_active(mode: &str, retain_until_date: Option<&s3s::dto::Date
 /// Check if retention modification is blocked for the given object.
 pub fn check_retention_for_modification(
     user_defined: &std::collections::HashMap<String, String>,
+    new_mode: Option<&str>,
     new_retain_until: Option<OffsetDateTime>,
     bypass_governance: bool,
 ) -> Option<ObjectLockBlockReason> {
@@ -67,6 +68,7 @@ pub fn check_retention_for_modification(
     }
 
     let existing_retain_until = retention.retain_until_date.as_ref().map(|d| OffsetDateTime::from(d.clone()));
+    let mode_changed = new_mode != Some(mode_str);
 
     // Check if new retention period is shorter than existing
     let is_shortening = match (&existing_retain_until, &new_retain_until) {
@@ -78,7 +80,7 @@ pub fn check_retention_for_modification(
     // COMPLIANCE mode: cannot shorten retention at all (even with bypass)
     // Can only extend the retention period
     if mode_str == ObjectLockRetentionMode::COMPLIANCE {
-        if is_shortening {
+        if mode_changed || is_shortening {
             return Some(ObjectLockBlockReason::Retention {
                 mode: mode_str.to_string(),
                 retain_until: existing_retain_until,
@@ -93,7 +95,7 @@ pub fn check_retention_for_modification(
     // - Extending retention: allowed without bypass permission
     // - Shortening/removing retention: requires bypass permission
     if mode_str == ObjectLockRetentionMode::GOVERNANCE {
-        if is_shortening && !bypass_governance {
+        if (mode_changed || is_shortening) && !bypass_governance {
             return Some(ObjectLockBlockReason::Retention {
                 mode: mode_str.to_string(),
                 retain_until: existing_retain_until,
@@ -380,7 +382,7 @@ mod tests {
         // No existing retention - modification should be allowed
         let user_defined = std::collections::HashMap::new();
         let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(30));
-        assert!(check_retention_for_modification(&user_defined, new_retain, false).is_none());
+        assert!(check_retention_for_modification(&user_defined, None, new_retain, false).is_none());
     }
 
     #[test]
@@ -398,7 +400,10 @@ mod tests {
 
         // Extending by another 30 days should be allowed
         let new_retain = Some(existing_retain + time::Duration::days(30));
-        assert!(check_retention_for_modification(&user_defined, new_retain, false).is_none());
+        assert!(
+            check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::COMPLIANCE), new_retain, false)
+                .is_none()
+        );
     }
 
     #[test]
@@ -416,7 +421,8 @@ mod tests {
 
         // Shortening to 30 days should be blocked
         let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(30));
-        let result = check_retention_for_modification(&user_defined, new_retain, false);
+        let result =
+            check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::COMPLIANCE), new_retain, false);
         assert!(result.is_some());
         assert!(matches!(result, Some(ObjectLockBlockReason::Retention { .. })));
     }
@@ -435,7 +441,7 @@ mod tests {
         );
 
         // Clearing (None) should be blocked
-        let result = check_retention_for_modification(&user_defined, None, false);
+        let result = check_retention_for_modification(&user_defined, None, None, false);
         assert!(result.is_some());
     }
 
@@ -454,7 +460,8 @@ mod tests {
 
         // Shortening from 30 days to 15 days without bypass should be blocked
         let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(15));
-        let result = check_retention_for_modification(&user_defined, new_retain, false);
+        let result =
+            check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::GOVERNANCE), new_retain, false);
         assert!(result.is_some());
     }
 
@@ -474,7 +481,10 @@ mod tests {
 
         // Extending from 30 days to 60 days without bypass should be allowed
         let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(60));
-        assert!(check_retention_for_modification(&user_defined, new_retain, false).is_none());
+        assert!(
+            check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::GOVERNANCE), new_retain, false)
+                .is_none()
+        );
     }
 
     #[test]
@@ -492,7 +502,75 @@ mod tests {
 
         // Shortening from 30 days to 15 days with bypass should be allowed
         let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(15));
-        assert!(check_retention_for_modification(&user_defined, new_retain, true).is_none());
+        assert!(
+            check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::GOVERNANCE), new_retain, true)
+                .is_none()
+        );
+    }
+
+    #[test]
+    fn test_check_retention_for_modification_governance_mode_change_without_bypass() {
+        let mut user_defined = std::collections::HashMap::new();
+        let existing_retain = OffsetDateTime::now_utc() + time::Duration::days(30);
+        user_defined.insert("x-amz-object-lock-mode".to_string(), "GOVERNANCE".to_string());
+        user_defined.insert(
+            "x-amz-object-lock-retain-until-date".to_string(),
+            existing_retain
+                .format(&time::format_description::well_known::Rfc3339)
+                .unwrap(),
+        );
+
+        let result = check_retention_for_modification(
+            &user_defined,
+            Some(ObjectLockRetentionMode::COMPLIANCE),
+            Some(existing_retain),
+            false,
+        );
+        assert!(result.is_some());
+    }
+
+    #[test]
+    fn test_check_retention_for_modification_governance_mode_change_with_bypass() {
+        let mut user_defined = std::collections::HashMap::new();
+        let existing_retain = OffsetDateTime::now_utc() + time::Duration::days(30);
+        user_defined.insert("x-amz-object-lock-mode".to_string(), "GOVERNANCE".to_string());
+        user_defined.insert(
+            "x-amz-object-lock-retain-until-date".to_string(),
+            existing_retain
+                .format(&time::format_description::well_known::Rfc3339)
+                .unwrap(),
+        );
+
+        assert!(
+            check_retention_for_modification(
+                &user_defined,
+                Some(ObjectLockRetentionMode::COMPLIANCE),
+                Some(existing_retain),
+                true,
+            )
+            .is_none()
+        );
+    }
+
+    #[test]
+    fn test_check_retention_for_modification_compliance_mode_change() {
+        let mut user_defined = std::collections::HashMap::new();
+        let existing_retain = OffsetDateTime::now_utc() + time::Duration::days(30);
+        user_defined.insert("x-amz-object-lock-mode".to_string(), "COMPLIANCE".to_string());
+        user_defined.insert(
+            "x-amz-object-lock-retain-until-date".to_string(),
+            existing_retain
+                .format(&time::format_description::well_known::Rfc3339)
+                .unwrap(),
+        );
+
+        let result = check_retention_for_modification(
+            &user_defined,
+            Some(ObjectLockRetentionMode::GOVERNANCE),
+            Some(existing_retain),
+            true,
+        );
+        assert!(result.is_some());
     }
 
     #[test]

rustfs/src/app/bucket_usecase.rs

@@ -35,8 +35,10 @@ use rustfs_ecstore::bucket::{
         BUCKET_VERSIONING_CONFIG,
     },
     metadata_sys,
+    object_lock::ObjectLockApi,
     policy_sys::PolicySys,
     utils::serialize,
+    versioning::VersioningApi,
     versioning_sys::BucketVersioningSys,
 };
 use rustfs_ecstore::client::object_api_utils::to_s3s_etag;
@@ -69,6 +71,32 @@ fn to_internal_error(err: impl Display) -> S3Error {
     S3Error::with_message(S3ErrorCode::InternalError, format!("{err}"))
 }
 
+fn versioning_configuration_has_object_lock_incompatible_settings(config: &VersioningConfiguration) -> bool {
+    config.suspended()
+        || config.exclude_folders.unwrap_or(false)
+        || config
+            .excluded_prefixes
+            .as_ref()
+            .is_some_and(|excluded_prefixes| !excluded_prefixes.is_empty())
+}
+
+async fn validate_bucket_versioning_update(bucket: &str, config: &VersioningConfiguration) -> S3Result<()> {
+    match metadata_sys::get_object_lock_config(bucket).await {
+        Ok((object_lock_config, _)) => {
+            if object_lock_config.enabled() && versioning_configuration_has_object_lock_incompatible_settings(config) {
+                return Err(S3Error::with_message(
+                    S3ErrorCode::InvalidBucketState,
+                    "An Object Lock configuration is present on this bucket, versioning cannot be suspended.".to_string(),
+                ));
+            }
+        }
+        Err(StorageError::ConfigNotFound) => {}
+        Err(err) => return Err(ApiError::from(err).into()),
+    }
+
+    Ok(())
+}
+
 fn create_bucket_exists_response(is_owner: bool) -> S3Result<S3Response<CreateBucketOutput>> {
     if is_owner {
         return Ok(S3Response::new(CreateBucketOutput::default()));
@@ -1357,6 +1385,8 @@ impl DefaultBucketUsecase {
             ..
         } = req.input;
 
+        validate_bucket_versioning_update(&bucket, &versioning_configuration).await?;
+
         let data = serialize_config(&versioning_configuration)?;
 
         metadata_sys::update(&bucket, BUCKET_VERSIONING_CONFIG, data)
@@ -1629,6 +1659,16 @@ mod tests {
         req
     }
 
+    #[test]
+    fn versioning_configuration_has_object_lock_incompatible_settings_rejects_suspended() {
+        let config = VersioningConfiguration {
+            status: Some(BucketVersioningStatus::from_static(BucketVersioningStatus::SUSPENDED)),
+            ..Default::default()
+        };
+
+        assert!(versioning_configuration_has_object_lock_incompatible_settings(&config));
+    }
+
     #[test]
     fn resolve_notification_region_prefers_global_region() {
         let binding = resolve_notification_region(Some("us-east-1".parse().unwrap()), Some("ap-southeast-1".parse().unwrap()));

rustfs/src/app/object_usecase.rs

@@ -554,6 +554,83 @@ pub(crate) async fn build_put_like_object_lock_metadata(
     Ok(Some(eval_metadata))
 }
 
+const MAXIMUM_RETENTION_DAYS: i32 = 36_500;
+const MAXIMUM_RETENTION_YEARS: i32 = 100;
+
+fn invalid_object_lock_configuration(message: impl Into<String>) -> S3Error {
+    S3Error::with_message(S3ErrorCode::MalformedXML, message.into())
+}
+
+fn invalid_retention_period(message: impl Into<String>) -> S3Error {
+    let mut err = S3Error::with_message(S3ErrorCode::Custom("InvalidRetentionPeriod".into()), message.into());
+    err.set_status_code(StatusCode::BAD_REQUEST);
+    err
+}
+
+fn validate_default_retention_configuration(default_retention: &DefaultRetention) -> S3Result<()> {
+    let Some(mode) = default_retention.mode.as_ref() else {
+        return Err(invalid_object_lock_configuration("retention mode must be specified"));
+    };
+
+    match mode.as_str() {
+        ObjectLockRetentionMode::COMPLIANCE | ObjectLockRetentionMode::GOVERNANCE => {}
+        _ => {
+            return Err(invalid_object_lock_configuration(format!("unknown retention mode {}", mode.as_str())));
+        }
+    }
+
+    match (default_retention.days, default_retention.years) {
+        (Some(days), None) => {
+            if days <= 0 {
+                return Err(invalid_retention_period(
+                    "Default retention period must be a positive integer value for 'Days'",
+                ));
+            }
+            if days > MAXIMUM_RETENTION_DAYS {
+                return Err(invalid_retention_period(format!("Default retention period too large for 'Days' {days}",)));
+            }
+        }
+        (None, Some(years)) => {
+            if years <= 0 {
+                return Err(invalid_retention_period(
+                    "Default retention period must be a positive integer value for 'Years'",
+                ));
+            }
+            if years > MAXIMUM_RETENTION_YEARS {
+                return Err(invalid_retention_period(format!(
+                    "Default retention period too large for 'Years' {years}",
+                )));
+            }
+        }
+        (Some(_), Some(_)) => {
+            return Err(invalid_object_lock_configuration("either Days or Years must be specified, not both"));
+        }
+        (None, None) => {
+            return Err(invalid_object_lock_configuration("either Days or Years must be specified"));
+        }
+    }
+
+    Ok(())
+}
+
+fn validate_object_lock_configuration_input(input_cfg: &ObjectLockConfiguration) -> S3Result<()> {
+    let enabled = input_cfg.object_lock_enabled.as_ref().map(ObjectLockEnabled::as_str);
+    if enabled != Some(ObjectLockEnabled::ENABLED) {
+        return Err(invalid_object_lock_configuration(
+            "only 'Enabled' value is allowed to ObjectLockEnabled element",
+        ));
+    }
+
+    if let Some(rule) = input_cfg.rule.as_ref() {
+        let Some(default_retention) = rule.default_retention.as_ref() else {
+            return Err(invalid_object_lock_configuration("Rule must include DefaultRetention"));
+        };
+        validate_default_retention_configuration(default_retention)?;
+    }
+
+    Ok(())
+}
+
 pub(crate) fn validate_existing_object_lock_for_write(existing_obj_info: &ObjectInfo) -> S3Result<()> {
     let legal_hold = get_object_legalhold_meta(&existing_obj_info.user_defined);
     if legal_hold
@@ -1164,6 +1241,8 @@ impl DefaultObjectUsecase {
             .await
             .map_err(ApiError::from)?;
 
+        validate_object_lock_configuration_input(&input_cfg)?;
+
         match metadata_sys::get_object_lock_config(&bucket).await {
             Ok(_) => {}
             Err(err) => {
@@ -1237,6 +1316,7 @@ impl DefaultObjectUsecase {
             .as_ref()
             .and_then(|r| r.retain_until_date.as_ref())
             .map(|d| OffsetDateTime::from(d.clone()));
+        let new_mode = retention.as_ref().and_then(|r| r.mode.as_ref()).map(|mode| mode.as_str());
 
         // TODO(security): Known TOCTOU race condition (fix in future PR).
         //
@@ -1270,7 +1350,7 @@ impl DefaultObjectUsecase {
         if let Ok(existing_obj_info) = store.get_object_info(&bucket, &key, &check_opts).await {
             let bypass_governance = has_bypass_governance_header(&req.headers);
             if let Some(block_reason) =
-                check_retention_for_modification(&existing_obj_info.user_defined, new_retain_until, bypass_governance)
+                check_retention_for_modification(&existing_obj_info.user_defined, new_mode, new_retain_until, bypass_governance)
             {
                 return Err(S3Error::with_message(S3ErrorCode::AccessDenied, block_reason.error_message()));
             }
@@ -5010,6 +5090,51 @@ mod tests {
         assert_eq!(err.code(), &S3ErrorCode::InternalError);
     }
 
+    #[test]
+    fn validate_object_lock_configuration_rejects_disabled_status() {
+        let cfg = ObjectLockConfiguration {
+            object_lock_enabled: Some(ObjectLockEnabled::from("Disabled".to_string())),
+            rule: None,
+        };
+
+        let err = validate_object_lock_configuration_input(&cfg).unwrap_err();
+        assert_eq!(err.code(), &S3ErrorCode::MalformedXML);
+    }
+
+    #[test]
+    fn validate_object_lock_configuration_rejects_invalid_default_retention_mode() {
+        let cfg = ObjectLockConfiguration {
+            object_lock_enabled: Some(ObjectLockEnabled::from_static(ObjectLockEnabled::ENABLED)),
+            rule: Some(ObjectLockRule {
+                default_retention: Some(DefaultRetention {
+                    mode: Some(ObjectLockRetentionMode::from("abc".to_string())),
+                    days: Some(1),
+                    years: None,
+                }),
+            }),
+        };
+
+        let err = validate_object_lock_configuration_input(&cfg).unwrap_err();
+        assert_eq!(err.code(), &S3ErrorCode::MalformedXML);
+    }
+
+    #[test]
+    fn validate_object_lock_configuration_rejects_days_and_years_together() {
+        let cfg = ObjectLockConfiguration {
+            object_lock_enabled: Some(ObjectLockEnabled::from_static(ObjectLockEnabled::ENABLED)),
+            rule: Some(ObjectLockRule {
+                default_retention: Some(DefaultRetention {
+                    mode: Some(ObjectLockRetentionMode::from_static(ObjectLockRetentionMode::GOVERNANCE)),
+                    days: Some(1),
+                    years: Some(1),
+                }),
+            }),
+        };
+
+        let err = validate_object_lock_configuration_input(&cfg).unwrap_err();
+        assert_eq!(err.code(), &S3ErrorCode::MalformedXML);
+    }
+
     #[tokio::test]
     async fn execute_put_object_retention_returns_internal_error_when_store_uninitialized() {
         let input = PutObjectRetentionInput::builder()

scripts/s3-tests/excluded_tests.txt

@@ -237,26 +237,6 @@ test_non_multipart_get_part
 test_non_multipart_sse_c_get_part
 test_object_copy_canned_acl
 test_object_header_acl_grants
-test_object_lock_changing_mode_from_compliance
-test_object_lock_changing_mode_from_governance_with_bypass
-test_object_lock_changing_mode_from_governance_without_bypass
-test_object_lock_get_obj_lock
-test_object_lock_get_obj_metadata
-test_object_lock_put_obj_lock
-test_object_lock_put_obj_lock_invalid_days
-test_object_lock_put_obj_lock_invalid_mode
-test_object_lock_put_obj_lock_invalid_status
-test_object_lock_put_obj_lock_invalid_years
-test_object_lock_put_obj_lock_with_days_and_years
-test_object_lock_put_obj_retention
-test_object_lock_put_obj_retention_increase_period
-test_object_lock_put_obj_retention_invalid_mode
-test_object_lock_put_obj_retention_override_default_retention
-test_object_lock_put_obj_retention_shorten_period
-test_object_lock_put_obj_retention_shorten_period_bypass
-test_object_lock_put_obj_retention_versionid
-test_object_lock_suspend_versioning
-test_object_lock_uploading_obj
 test_object_raw_get_x_amz_expires_not_expired
 test_object_raw_get_x_amz_expires_not_expired_tenant
 test_object_raw_get_x_amz_expires_out_max_range

scripts/s3-tests/implemented_tests.txt

@@ -403,6 +403,26 @@ test_object_lock_delete_object_with_retention_and_marker
 test_object_lock_delete_multipart_object_with_legal_hold_on
 test_object_lock_delete_multipart_object_with_retention
 test_object_lock_multi_delete_object_with_retention
+test_object_lock_changing_mode_from_compliance
+test_object_lock_changing_mode_from_governance_with_bypass
+test_object_lock_changing_mode_from_governance_without_bypass
+test_object_lock_get_obj_lock
+test_object_lock_get_obj_metadata
+test_object_lock_put_obj_lock
+test_object_lock_put_obj_lock_invalid_days
+test_object_lock_put_obj_lock_invalid_mode
+test_object_lock_put_obj_lock_invalid_status
+test_object_lock_put_obj_lock_invalid_years
+test_object_lock_put_obj_lock_with_days_and_years
+test_object_lock_put_obj_retention
+test_object_lock_put_obj_retention_increase_period
+test_object_lock_put_obj_retention_invalid_mode
+test_object_lock_put_obj_retention_override_default_retention
+test_object_lock_put_obj_retention_shorten_period
+test_object_lock_put_obj_retention_shorten_period_bypass
+test_object_lock_put_obj_retention_versionid
+test_object_lock_suspend_versioning
+test_object_lock_uploading_obj
 
 # Checksum validation tests
 test_object_checksum_sha256