From 61bfc209d9f8f0c6865512512a6a8b4dc5b06d42 Mon Sep 17 00:00:00 2001 From: overtrue Date: Mon, 9 Mar 2026 17:39:12 +0800 Subject: [PATCH 1/2] feat: add anonymous bucket access commands --- crates/cli/src/commands/anonymous.rs | 1084 ++++++++++++++++++++++++++ crates/cli/src/commands/mod.rs | 12 + crates/cli/tests/help_contract.rs | 36 + crates/core/src/traits.rs | 12 + crates/s3/src/client.rs | 88 +++ 5 files changed, 1232 insertions(+) create mode 100644 crates/cli/src/commands/anonymous.rs diff --git a/crates/cli/src/commands/anonymous.rs b/crates/cli/src/commands/anonymous.rs new file mode 100644 index 0000000..e97e971 --- /dev/null +++ b/crates/cli/src/commands/anonymous.rs @@ -0,0 +1,1084 @@ +//! anonymous command - Manage anonymous access to buckets and objects +//! +//! Supports anonymous access operations compatible with `mc anonymous`: +//! - set: set access level to private/public/download/upload +//! - set-json: set access using a policy JSON file +//! - get: get permission level +//! - get-json: get raw policy JSON +//! - list: list access rules +//! - links: list public object links, optionally recursively + +use std::collections::{HashMap, HashSet}; +use std::fmt; + +use clap::{Args, Subcommand}; +use rc_core::{Alias, AliasManager, ListOptions, ObjectStore as _, RemotePath}; +use rc_s3::S3Client; +use serde::Serialize; + +use crate::exit_code::ExitCode; +use crate::output::{Formatter, OutputConfig}; + +#[derive(Args, Debug)] +pub struct AnonymousArgs { + #[command(subcommand)] + pub command: AnonymousCommands, +} + +#[derive(Subcommand, Debug)] +pub enum AnonymousCommands { + /// Set anonymous access permission + Set(SetArgs), + + /// Set anonymous access from a policy JSON file + SetJson(SetJsonArgs), + + /// Get anonymous access permission + Get(AnonymousPathArg), + + /// Get anonymous policy JSON + GetJson(AnonymousPathArg), + + /// List anonymous policies + List(AnonymousPathArg), + + /// List public links for anonymous readable prefixes + Links(LinksArgs), +} + +#[derive(Args, Debug)] +pub struct SetArgs { + /// Permission: private, public, download, upload + pub permission: String, + + /// Target path (alias/bucket or alias/bucket/prefix) + pub path: String, +} + +#[derive(Args, Debug)] +pub struct SetJsonArgs { + /// Path to policy JSON file + pub file: String, + + /// Target path (alias/bucket or alias/bucket/prefix) + pub path: String, +} + +#[derive(Args, Debug)] +pub struct AnonymousPathArg { + /// Target path (alias/bucket or alias/bucket/prefix) + pub path: String, +} + +#[derive(Args, Debug)] +pub struct LinksArgs { + /// Target path (alias/bucket or alias/bucket/prefix) + pub path: String, + + /// Recursively list links + #[arg(short, long)] + pub recursive: bool, +} + +#[derive(Debug, Serialize)] +struct PermissionOutput { + path: String, + permission: String, +} + +#[derive(Debug, Serialize)] +struct RuleOutput { + resource: String, + permission: String, +} + +#[derive(Debug, Serialize)] +struct LinkOutput { + url: String, + status: String, +} + +#[derive(Debug)] +struct AccessTarget { + alias: String, + bucket: String, + prefix: String, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum AccessLevel { + Private, + Download, + Upload, + Public, + Custom, +} + +impl AccessLevel { + fn parse(value: &str) -> Result { + match value.to_lowercase().as_str() { + "private" => Ok(Self::Private), + "public" => Ok(Self::Public), + "download" => Ok(Self::Download), + "upload" => Ok(Self::Upload), + _ => Err(format!( + "Invalid permission '{value}'. Allowed: private, public, download, upload" + )), + } + } + + fn is_read(self) -> bool { + matches!(self, Self::Download | Self::Public) + } + + fn is_write(self) -> bool { + matches!(self, Self::Upload | Self::Public) + } + + fn as_str(self) -> &'static str { + match self { + Self::Private => "private", + Self::Download => "download", + Self::Upload => "upload", + Self::Public => "public", + Self::Custom => "custom", + } + } +} + +impl fmt::Display for AccessLevel { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.as_str()) + } +} + +#[derive(Clone, Copy, Default, Debug)] +struct PermissionFlags { + read: bool, + write: bool, + custom: bool, +} + +#[derive(Debug)] +struct AccessRule { + resource: String, + flags: PermissionFlags, +} + +impl PermissionFlags { + fn add(&mut self, other: PermissionFlags) { + self.read |= other.read; + self.write |= other.write; + self.custom |= other.custom; + } + + fn level(self) -> AccessLevel { + if self.custom { + AccessLevel::Custom + } else if self.read && self.write { + AccessLevel::Public + } else if self.read { + AccessLevel::Download + } else if self.write { + AccessLevel::Upload + } else { + AccessLevel::Private + } + } +} + +pub async fn execute(args: AnonymousArgs, output_config: OutputConfig) -> ExitCode { + match args.command { + AnonymousCommands::Set(args) => execute_set(args, output_config).await, + AnonymousCommands::SetJson(args) => execute_set_json(args, output_config).await, + AnonymousCommands::Get(args) => execute_get(args, output_config).await, + AnonymousCommands::GetJson(args) => execute_get_json(args, output_config).await, + AnonymousCommands::List(args) => execute_list(args, output_config).await, + AnonymousCommands::Links(args) => execute_links(args, output_config).await, + } +} + +async fn execute_set(args: SetArgs, output_config: OutputConfig) -> ExitCode { + let formatter = Formatter::new(output_config); + + let permission = match AccessLevel::parse(&args.permission) { + Ok(permission) => permission, + Err(error) => { + formatter.error(&error); + return ExitCode::UsageError; + } + }; + + let target = match parse_anonymous_path(&args.path) { + Ok(target) => target, + Err(error) => { + formatter.error(&error); + return ExitCode::UsageError; + } + }; + + let (_alias, client) = match setup_anonymous_client(&target.alias, &formatter).await { + Ok(client) => client, + Err(code) => return code, + }; + + let result = if permission == AccessLevel::Private { + client.delete_bucket_policy(&target.bucket).await + } else { + let policy = build_policy(&permission, &target.bucket, &target.prefix); + client.set_bucket_policy(&target.bucket, &policy).await + }; + + match result { + Ok(()) => { + if formatter.is_json() { + formatter.json(&PermissionOutput { + path: target.path_with_bucket_prefix(), + permission: permission.as_str().to_string(), + }); + } else { + formatter.println(&format!( + "Set anonymous permission for '{}' to '{}'", + target.path_with_bucket_prefix(), + permission + )); + } + ExitCode::Success + } + Err(error) => { + formatter.error(&format!( + "Failed to set anonymous access for '{}': {error}", + target.path_with_bucket_prefix() + )); + exit_code_from_error(&error) + } + } +} + +async fn execute_set_json(args: SetJsonArgs, output_config: OutputConfig) -> ExitCode { + let formatter = Formatter::new(output_config); + + let target = match parse_anonymous_path(&args.path) { + Ok(target) => target, + Err(error) => { + formatter.error(&error); + return ExitCode::UsageError; + } + }; + + let policy = match tokio::fs::read_to_string(&args.file).await { + Ok(content) => content, + Err(error) => { + formatter.error(&format!("Failed to read policy file '{}': {error}", args.file)); + return ExitCode::GeneralError; + } + }; + + if serde_json::from_str::(&policy).is_err() { + formatter.error("Provided policy file is not valid JSON"); + return ExitCode::UsageError; + } + + let (_alias, client) = match setup_anonymous_client(&target.alias, &formatter).await { + Ok(client) => client, + Err(code) => return code, + }; + + match client.set_bucket_policy(&target.bucket, &policy).await { + Ok(()) => { + if formatter.is_json() { + formatter.json(&PermissionOutput { + path: target.path_with_bucket_prefix(), + permission: "custom".to_string(), + }); + } else { + formatter.println(&format!( + "Set anonymous policy from file for '{}' successfully", + target.path_with_bucket_prefix() + )); + } + ExitCode::Success + } + Err(error) => { + formatter.error(&format!( + "Failed to set anonymous policy for '{}': {error}", + target.path_with_bucket_prefix() + )); + exit_code_from_error(&error) + } + } +} + +async fn execute_get(args: AnonymousPathArg, output_config: OutputConfig) -> ExitCode { + let formatter = Formatter::new(output_config); + + let target = match parse_anonymous_path(&args.path) { + Ok(target) => target, + Err(error) => { + formatter.error(&error); + return ExitCode::UsageError; + } + }; + + let (_alias, client) = match setup_anonymous_client(&target.alias, &formatter).await { + Ok(client) => client, + Err(code) => return code, + }; + + let permission = match client.get_bucket_policy(&target.bucket).await { + Ok(Some(policy)) => match parse_permission_from_policy(&policy, &target.bucket, &target.prefix) { + Ok(permission) => permission, + Err(_) => AccessLevel::Custom, + }, + Ok(None) => AccessLevel::Private, + Err(error) => { + formatter.error(&format!( + "Failed to get anonymous policy for '{}': {error}", + target.path_with_bucket_prefix() + )); + return exit_code_from_error(&error); + } + }; + + if formatter.is_json() { + formatter.json(&PermissionOutput { + path: target.path_with_bucket_prefix(), + permission: permission.as_str().to_string(), + }); + } else { + formatter.println(&format!( + "Anonymous access permission for '{}' is '{}'", + target.path_with_bucket_prefix(), + permission + )); + } + + ExitCode::Success +} + +async fn execute_get_json(args: AnonymousPathArg, output_config: OutputConfig) -> ExitCode { + let formatter = Formatter::new(output_config); + let target = match parse_anonymous_path(&args.path) { + Ok(target) => target, + Err(error) => { + formatter.error(&error); + return ExitCode::UsageError; + } + }; + + let (_alias, client) = match setup_anonymous_client(&target.alias, &formatter).await { + Ok(client) => client, + Err(code) => return code, + }; + + let policy = match client.get_bucket_policy(&target.bucket).await { + Ok(Some(policy)) => policy, + Ok(None) => "{}".to_string(), + Err(error) => { + formatter.error(&format!( + "Failed to get anonymous policy for '{}': {error}", + target.path_with_bucket_prefix() + )); + return exit_code_from_error(&error); + } + }; + + match serde_json::from_str::(&policy) { + Ok(json) => formatter.json(&json), + Err(_) => formatter.json(&policy), + } + + ExitCode::Success +} + +async fn execute_list(args: AnonymousPathArg, output_config: OutputConfig) -> ExitCode { + let formatter = Formatter::new(output_config); + + let target = match parse_anonymous_path(&args.path) { + Ok(target) => target, + Err(error) => { + formatter.error(&error); + return ExitCode::UsageError; + } + }; + + let (_alias, client) = match setup_anonymous_client(&target.alias, &formatter).await { + Ok(client) => client, + Err(code) => return code, + }; + + let policy = match client.get_bucket_policy(&target.bucket).await { + Ok(Some(policy)) => policy, + Ok(None) => { + if formatter.is_json() { + formatter.json(&Vec::::new()); + } else { + formatter.println("No anonymous policies found."); + } + return ExitCode::Success; + } + Err(error) => { + formatter.error(&format!( + "Failed to list anonymous policies for '{}': {error}", + target.path_with_bucket_prefix() + )); + return exit_code_from_error(&error); + } + }; + + let rules = parse_access_rules(&policy, &target.bucket).unwrap_or_else(|_| { + vec![AccessRule { + resource: String::new(), + flags: PermissionFlags { + read: false, + write: false, + custom: true, + }, + }] + }); + + if rules.is_empty() { + if formatter.is_json() { + formatter.json(&Vec::::new()); + } else { + formatter.println("No anonymous policies found."); + } + return ExitCode::Success; + } + + // For compatibility with `mc anonymous list`, always show all known anonymous rules for + // the bucket. This avoids false negatives for bucket-level and inherited policies. + if rules.is_empty() { + if formatter.is_json() { + formatter.json(&Vec::::new()); + } else { + formatter.println("No anonymous policies found for target."); + } + return ExitCode::Success; + } + + let mut merged: HashMap = HashMap::new(); + for rule in rules { + merged + .entry(rule.resource) + .and_modify(|state| state.add(rule.flags)) + .or_insert(rule.flags); + } + + let mut rules: Vec = merged + .into_iter() + .map(|(resource, flags)| AccessRule { resource, flags }) + .collect(); + + // Sort for deterministic output. + rules.sort_by(|a, b| a.resource.cmp(&b.resource)); + + if formatter.is_json() { + let output = rules + .iter() + .map(|r| RuleOutput { + resource: r.resource.clone(), + permission: r.flags.level().as_str().to_string(), + }) + .collect::>(); + formatter.json(&output); + } else { + formatter.println("Anonymous policies:"); + for rule in rules { + formatter.println(&format!( + " {} => {}", + rule.resource, + rule.flags.level() + )); + } + } + + ExitCode::Success +} + +async fn execute_links(args: LinksArgs, output_config: OutputConfig) -> ExitCode { + let formatter = Formatter::new(output_config); + + let target = match parse_anonymous_path(&args.path) { + Ok(target) => target, + Err(error) => { + formatter.error(&error); + return ExitCode::UsageError; + } + }; + + let (alias, client) = match setup_anonymous_client(&target.alias, &formatter).await { + Ok(v) => v, + Err(code) => return code, + }; + + let policy = match client.get_bucket_policy(&target.bucket).await { + Ok(Some(policy)) => policy, + Ok(None) => { + formatter.println("No anonymous policies found for target."); + return ExitCode::Success; + } + Err(error) => { + formatter.error(&format!( + "Failed to resolve anonymous links for '{}': {error}", + target.path_with_bucket_prefix() + )); + return exit_code_from_error(&error); + } + }; + + let rules = match parse_access_rules(&policy, &target.bucket) { + Ok(rules) => rules, + Err(_) => { + formatter.println("Policy does not contain recognizable anonymous rules."); + return ExitCode::Success; + } + }; + + let mut urls = Vec::new(); + let mut seen = HashSet::new(); + let target_prefix = target.prefix.trim_end_matches('/').to_string(); + + for rule in rules { + if !rule.is_read() { + continue; + } + + if !prefixes_overlap(&rule.resource, &target_prefix) { + continue; + } + + let list_prefix = if target_prefix.is_empty() { + rule.resource.clone() + } else if rule.resource.is_empty() { + target_prefix.clone() + } else { + rule.resource.clone() + }; + + for key in list_keys_for_prefix( + &client, + &target.bucket, + &list_prefix, + args.recursive, + ) + .await + .unwrap_or_default() + { + if !target_prefix.is_empty() && !key_under_prefix(&key, &target_prefix) { + continue; + } + let url = build_public_url(&alias.endpoint, &target.bucket, &key); + if seen.insert(url.clone()) { + urls.push(url); + } + } + } + + urls.sort_unstable(); + if urls.is_empty() { + formatter.println("No public links found for target."); + return ExitCode::Success; + } + + if formatter.is_json() { + let output: Vec = urls + .iter() + .map(|u| LinkOutput { + url: u.clone(), + status: "success".to_string(), + }) + .collect(); + formatter.json(&output); + } else { + for url in urls { + formatter.println(&format!(" {url}")); + } + } + + ExitCode::Success +} + +fn parse_anonymous_path(path: &str) -> Result { + if path.trim().is_empty() { + return Err("Path cannot be empty".to_string()); + } + + let mut parts = path.splitn(3, '/'); + let alias = parts + .next() + .ok_or_else(|| "Path must include alias".to_string())?; + if alias.is_empty() { + return Err("Alias name is required (alias/bucket[/prefix])".to_string()); + } + + let bucket = parts + .next() + .ok_or_else(|| "Bucket is required (alias/bucket[/prefix])".to_string())?; + if bucket.is_empty() { + return Err("Bucket is required (alias/bucket[/prefix])".to_string()); + } + + let prefix = parts.next().map(|v| v.trim_end_matches('/')).unwrap_or(""); + if alias.is_empty() || bucket.is_empty() { + return Err("Path must be in format alias/bucket[/prefix]".to_string()); + } + + Ok(AccessTarget { + alias: alias.to_string(), + bucket: bucket.to_string(), + prefix: prefix.to_string(), + }) +} + +fn build_policy(level: &AccessLevel, bucket: &str, prefix: &str) -> String { + let object_resource = build_object_resource(bucket, prefix); + let bucket_resource = format!("arn:aws:s3:::{}", bucket); + let mut statements = Vec::new(); + let mut sid = 1u8; + + if level.is_read() { + statements.push(serde_json::json!({ + "Sid": format!("AnonymousRead{sid}"), + "Effect": "Allow", + "Principal": "*", + "Action": ["s3:GetObject"], + "Resource": object_resource, + })); + sid += 1; + + let mut condition = serde_json::json!({}); + if !prefix.is_empty() { + let sanitized = prefix.trim_end_matches('/'); + condition = serde_json::json!({ + "StringLike": { + "s3:prefix": [format!("{sanitized}"), format!("{sanitized}/*"), format!("{sanitized}*")] + } + }); + } + if prefix.is_empty() { + statements.push(serde_json::json!({ + "Sid": format!("AnonymousList{sid}"), + "Effect": "Allow", + "Principal": "*", + "Action": "s3:ListBucket", + "Resource": bucket_resource, + })); + } else { + statements.push(serde_json::json!({ + "Sid": format!("AnonymousList{sid}"), + "Effect": "Allow", + "Principal": "*", + "Action": "s3:ListBucket", + "Resource": bucket_resource, + "Condition": condition, + })); + } + sid += 1; + } + + if level.is_write() { + statements.push(serde_json::json!({ + "Sid": format!("AnonymousWrite{sid}"), + "Effect": "Allow", + "Principal": "*", + "Action": ["s3:PutObject"], + "Resource": build_object_resource(bucket, prefix), + })); + } + + serde_json::json!({ + "Version": "2012-10-17", + "Statement": statements, + }) + .to_string() +} + +fn build_object_resource(bucket: &str, prefix: &str) -> String { + if prefix.is_empty() { + format!("arn:aws:s3:::{}/*", bucket) + } else { + format!( + "arn:aws:s3:::{}/*", + format!("{}/{}", bucket, prefix.trim_end_matches('/')) + ) + } +} + +async fn list_keys_for_prefix( + client: &S3Client, + bucket: &str, + prefix: &str, + recursive: bool, +) -> Result, rc_core::Error> { + let mut options = ListOptions { + recursive, + max_keys: Some(1000), + prefix: if prefix.is_empty() { + None + } else { + Some(prefix.to_string()) + }, + ..Default::default() + }; + + let mut continuation: Option = None; + let mut keys = Vec::new(); + + loop { + options.continuation_token = continuation.clone(); + let path = RemotePath::new("anonymous", bucket, prefix); + let result = client.list_objects(&path, options.clone()).await?; + for item in result.items { + if !item.is_dir { + keys.push(item.key); + } + } + + if !result.truncated { + break; + } + continuation = result.continuation_token; + } + + Ok(keys) +} + +fn parse_access_rules(policy: &str, bucket: &str) -> Result, String> { + let policy: serde_json::Value = serde_json::from_str(policy).map_err(|e| e.to_string())?; + let statements = match policy.get("Statement") { + Some(statement) => parse_statement_list(statement)?, + None => return Err("Policy has no statements".to_string()), + }; + + let mut rules = Vec::new(); + + for statement in statements { + let effect = statement + .get("Effect") + .and_then(|value| value.as_str()) + .unwrap_or("Allow"); + if !effect.eq_ignore_ascii_case("allow") { + continue; + } + + if !is_public_principal(statement.get("Principal")) { + continue; + } + + let actions = parse_string_or_array(statement.get("Action")); + let resources = parse_string_or_array(statement.get("Resource")); + if actions.is_empty() || resources.is_empty() { + continue; + } + + let mut flags = PermissionFlags::default(); + for action in actions { + let parsed = parse_action_flags(&action); + flags.read |= parsed.read; + flags.write |= parsed.write; + flags.custom |= parsed.custom; + } + if !flags.read && !flags.write && !flags.custom { + continue; + } + + for resource in resources { + if let Some(resource_path) = normalize_bucket_resource(bucket, &resource) { + rules.push(AccessRule { + resource: resource_path, + flags, + }); + } + } + } + Ok(rules) +} + +fn parse_permission_from_policy( + policy: &str, + bucket: &str, + target_prefix: &str, +) -> Result { + let rules = parse_access_rules(policy, bucket)?; + Ok(permission_for_target(&rules, target_prefix)) +} + +fn permission_for_target(rules: &[AccessRule], target_prefix: &str) -> AccessLevel { + let target_prefix = target_prefix.trim_end_matches('/').to_string(); + let mut state = PermissionFlags::default(); + + for rule in rules { + if !target_covers_prefix(&rule.resource, &target_prefix) { + continue; + } + + state.add(rule.flags); + } + + state.level() +} + +fn target_covers_prefix(rule_prefix: &str, target_prefix: &str) -> bool { + if target_prefix.is_empty() { + return rule_prefix.is_empty(); + } + if rule_prefix.is_empty() { + return true; + } + + let rule_prefix = rule_prefix.trim_end_matches('/'); + let target_prefix = target_prefix.trim_end_matches('/'); + + target_prefix == rule_prefix + || target_prefix + .strip_prefix(rule_prefix) + .is_some_and(|rest| rest.is_empty() || rest.starts_with('/')) +} + +fn normalize_bucket_resource(bucket: &str, resource: &str) -> Option { + let resource = resource + .strip_prefix("arn:aws:s3:::") + .unwrap_or(resource); + + if resource == bucket { + return Some(String::new()); + } + if !resource.starts_with(bucket) { + return None; + } + + let mut rest = resource.strip_prefix(bucket)?; + if rest.is_empty() { + return Some(String::new()); + } + if !rest.starts_with('/') && rest != "*" && !rest.starts_with('*') { + return None; + } + + rest = rest.trim_start_matches('/'); + if rest.is_empty() { + return Some(String::new()); + } + + let trimmed = rest + .trim_end_matches('/') + .trim_end_matches('*') + .trim_start_matches('/'); + if trimmed.is_empty() { + Some(String::new()) + } else { + Some(trimmed.to_string()) + } +} + +fn parse_action_flags(action: &str) -> PermissionFlags { + let action = action.to_ascii_lowercase(); + if action == "*" || action == "s3:*" { + return PermissionFlags { + read: false, + write: false, + custom: true, + }; + } + + if action.contains(":get") { + PermissionFlags { + read: true, + write: false, + custom: false, + } + } else if action.contains(":put") || action.contains(":delete") || action.contains("upload") { + PermissionFlags { + read: false, + write: true, + custom: false, + } + } else { + PermissionFlags { + read: false, + write: false, + custom: false, + } + } +} + +fn is_public_principal(principal: Option<&serde_json::Value>) -> bool { + let Some(principal) = principal else { + return false; + }; + + match principal { + serde_json::Value::String(value) => value == "*", + serde_json::Value::Array(values) => values.iter().any(|value| is_public_principal(Some(value))), + serde_json::Value::Object(values) => { + values.values().any(|value| is_public_principal(Some(value))) + } + _ => false, + } +} + +fn prefixes_overlap(rule_prefix: &str, target_prefix: &str) -> bool { + if rule_prefix.is_empty() || target_prefix.is_empty() { + return true; + } + + let rule_prefix = rule_prefix.trim_end_matches('/'); + let target_prefix = target_prefix.trim_end_matches('/'); + if rule_prefix == target_prefix { + return true; + } + + rule_prefix.starts_with(&format!("{target_prefix}/")) || target_prefix.starts_with(&format!("{rule_prefix}/")) +} + +fn key_under_prefix(key: &str, prefix: &str) -> bool { + key == prefix + || key + .strip_prefix(prefix) + .is_some_and(|rest| rest.is_empty() || rest.starts_with('/')) +} + +fn parse_string_or_array(value: Option<&serde_json::Value>) -> Vec { + let Some(value) = value else { + return Vec::new(); + }; + + match value { + serde_json::Value::String(value) => vec![value.to_string()], + serde_json::Value::Array(values) => values + .iter() + .filter_map(|v| v.as_str().map(ToString::to_string)) + .collect(), + _ => Vec::new(), + } +} + +fn parse_statement_list(value: &serde_json::Value) -> Result, String> { + match value { + serde_json::Value::Array(values) => Ok(values.iter().collect()), + serde_json::Value::Object(_) => Ok(vec![value]), + _ => Err("Policy statement must be object or array".to_string()), + } +} + +fn build_public_url(endpoint: &str, bucket: &str, key: &str) -> String { + let endpoint = endpoint.trim_end_matches('/'); + if key.is_empty() { + format!("{endpoint}/{bucket}") + } else { + format!("{endpoint}/{bucket}/{key}") + } +} + +async fn setup_anonymous_client( + alias_name: &str, + formatter: &Formatter, +) -> Result<(Alias, S3Client), ExitCode> { + let alias_manager = match AliasManager::new() { + Ok(manager) => manager, + Err(error) => { + formatter.error(&format!("Failed to load aliases: {error}")); + return Err(ExitCode::GeneralError); + } + }; + + let alias = match alias_manager.get(alias_name) { + Ok(alias) => alias, + Err(_) => { + formatter.error(&format!("Alias '{alias_name}' not found")); + return Err(ExitCode::NotFound); + } + }; + + let client = match S3Client::new(alias.clone()).await { + Ok(client) => client, + Err(error) => { + formatter.error(&format!("Failed to create S3 client: {error}")); + return Err(ExitCode::NetworkError); + } + }; + + let capabilities = match client.capabilities().await { + Ok(caps) => caps, + Err(error) => { + formatter.error(&format!("Failed to detect capabilities: {error}")); + return Err(ExitCode::NetworkError); + } + }; + + if !capabilities.anonymous { + formatter.error("Backend does not support anonymous access operations"); + return Err(ExitCode::UnsupportedFeature); + } + + Ok((alias, client)) +} + +fn exit_code_from_error(error: &rc_core::Error) -> ExitCode { + ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError) +} + +impl AccessTarget { + fn path_with_bucket_prefix(&self) -> String { + if self.prefix.is_empty() { + format!("{}/{}", self.alias, self.bucket) + } else { + format!("{}/{}/{}", self.alias, self.bucket, self.prefix) + } + } +} + +impl AccessRule { + fn is_read(&self) -> bool { + self.flags.read || self.flags.custom + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_parse_anonymous_path() { + let target = parse_anonymous_path("local/mybucket").expect("valid target"); + assert_eq!(target.alias, "local"); + assert_eq!(target.bucket, "mybucket"); + assert!(target.prefix.is_empty()); + + let target = parse_anonymous_path("local/mybucket/path/prefix").expect("valid target"); + assert_eq!(target.alias, "local"); + assert_eq!(target.prefix, "path/prefix"); + } + + #[test] + fn test_parse_anonymous_path_errors() { + assert!(parse_anonymous_path("").is_err()); + assert!(parse_anonymous_path("local").is_err()); + assert!(parse_anonymous_path("/mybucket").is_err()); + assert!(parse_anonymous_path("local/").is_err()); + } + + #[test] + fn test_parse_permission() { + assert_eq!(AccessLevel::parse("private").ok(), Some(AccessLevel::Private)); + assert_eq!(AccessLevel::parse("public").ok(), Some(AccessLevel::Public)); + assert_eq!(AccessLevel::parse("download").ok(), Some(AccessLevel::Download)); + assert_eq!(AccessLevel::parse("upload").ok(), Some(AccessLevel::Upload)); + assert!(AccessLevel::parse("invalid").is_err()); + } + + #[test] + fn test_build_policy() { + let policy = build_policy(&AccessLevel::Download, "bucket", "photos"); + let value: serde_json::Value = serde_json::from_str(&policy).expect("valid json"); + assert_eq!(value["Version"], "2012-10-17"); + assert_eq!(value["Statement"].as_array().expect("array").len(), 2); + } + + #[test] + fn test_build_public_url() { + let url = build_public_url("http://localhost:9000/", "bucket", "path/to/object.txt"); + assert_eq!(url, "http://localhost:9000/bucket/path/to/object.txt"); + } +} diff --git a/crates/cli/src/commands/mod.rs b/crates/cli/src/commands/mod.rs index 48325a1..ff5751e 100644 --- a/crates/cli/src/commands/mod.rs +++ b/crates/cli/src/commands/mod.rs @@ -13,6 +13,7 @@ mod admin; mod alias; mod cat; mod completions; +mod anonymous; pub mod cp; pub mod diff; mod find; @@ -131,6 +132,10 @@ pub enum Commands { #[command(subcommand)] Tag(tag::TagCommands), + /// Manage anonymous access to buckets and objects + #[command(subcommand)] + Anonymous(anonymous::AnonymousCommands), + /// Manage bucket quota #[command(subcommand)] Quota(quota::QuotaCommands), @@ -177,6 +182,13 @@ pub async fn execute(cli: Cli) -> ExitCode { version::execute(version::VersionArgs { command: cmd }, output_config).await } Commands::Tag(cmd) => tag::execute(tag::TagArgs { command: cmd }, output_config).await, + Commands::Anonymous(cmd) => { + anonymous::execute( + anonymous::AnonymousArgs { command: cmd }, + output_config, + ) + .await + } Commands::Quota(cmd) => { quota::execute(quota::QuotaArgs { command: cmd }, output_config).await } diff --git a/crates/cli/tests/help_contract.rs b/crates/cli/tests/help_contract.rs index 8d4f83b..eb93866 100644 --- a/crates/cli/tests/help_contract.rs +++ b/crates/cli/tests/help_contract.rs @@ -121,6 +121,7 @@ fn top_level_command_help_contract() { "version", "tag", "quota", + "anonymous", "completions", ], }, @@ -260,6 +261,11 @@ fn top_level_command_help_contract() { usage: "Usage: rc quota [OPTIONS] ", expected_tokens: &["set", "info", "clear"], }, + HelpCase { + args: &["anonymous"], + usage: "Usage: rc anonymous [OPTIONS] ", + expected_tokens: &["set", "set-json", "get", "get-json", "list", "links"], + }, HelpCase { args: &["completions"], usage: "Usage: rc completions [OPTIONS] ", @@ -492,6 +498,36 @@ fn nested_subcommand_help_contract() { usage: "Usage: rc quota clear [OPTIONS] ", expected_tokens: &[], }, + HelpCase { + args: &["anonymous", "set"], + usage: "Usage: rc anonymous set [OPTIONS] ", + expected_tokens: &[], + }, + HelpCase { + args: &["anonymous", "set-json"], + usage: "Usage: rc anonymous set-json [OPTIONS] ", + expected_tokens: &[], + }, + HelpCase { + args: &["anonymous", "get"], + usage: "Usage: rc anonymous get [OPTIONS] ", + expected_tokens: &[], + }, + HelpCase { + args: &["anonymous", "get-json"], + usage: "Usage: rc anonymous get-json [OPTIONS] ", + expected_tokens: &[], + }, + HelpCase { + args: &["anonymous", "list"], + usage: "Usage: rc anonymous list [OPTIONS] ", + expected_tokens: &[], + }, + HelpCase { + args: &["anonymous", "links"], + usage: "Usage: rc anonymous links [OPTIONS] ", + expected_tokens: &["--recursive"], + }, ]; for case in cases { diff --git a/crates/core/src/traits.rs b/crates/core/src/traits.rs index 1a88957..90fff20 100644 --- a/crates/core/src/traits.rs +++ b/crates/core/src/traits.rs @@ -170,6 +170,9 @@ pub struct Capabilities { /// Supports object tagging pub tagging: bool, + /// Supports anonymous bucket access policies + pub anonymous: bool, + /// Supports S3 Select pub select: bool, @@ -280,6 +283,15 @@ pub trait ObjectStore: Send + Sync { /// Delete bucket tags async fn delete_bucket_tags(&self, bucket: &str) -> Result<()>; + + /// Get bucket policy as raw JSON string. Returns `None` when no policy exists. + async fn get_bucket_policy(&self, bucket: &str) -> Result>; + + /// Replace bucket policy using raw JSON string. + async fn set_bucket_policy(&self, bucket: &str, policy: &str) -> Result<()>; + + /// Remove bucket policy (set anonymous access to private). + async fn delete_bucket_policy(&self, bucket: &str) -> Result<()>; // async fn get_versioning(&self, bucket: &str) -> Result; // async fn set_versioning(&self, bucket: &str, enabled: bool) -> Result<()>; // async fn get_tags(&self, path: &RemotePath) -> Result>; diff --git a/crates/s3/src/client.rs b/crates/s3/src/client.rs index 6f0e6cd..dc8161f 100644 --- a/crates/s3/src/client.rs +++ b/crates/s3/src/client.rs @@ -725,6 +725,7 @@ impl ObjectStore for S3Client { versioning: true, object_lock: false, tagging: true, + anonymous: true, select: false, notifications: false, }) @@ -1147,6 +1148,93 @@ impl ObjectStore for S3Client { Ok(()) } + + async fn get_bucket_policy(&self, bucket: &str) -> Result> { + let response = match self.inner.get_bucket_policy().bucket(bucket).send().await { + Ok(policy) => policy, + Err(error) => { + if let aws_sdk_s3::error::SdkError::ServiceError(service_err) = &error { + if let Some(code) = service_err + .raw() + .headers() + .get("x-amz-error-code") + .and_then(|value| std::str::from_utf8(value.as_bytes()).ok()) + .map(|value| value.to_ascii_lowercase()) + { + if code == "nosuchbucketpolicy" || code == "nosuchpolicy" { + return Ok(None); + } + if code == "nosuchbucket" { + return Err(Error::NotFound(format!("Bucket not found: {bucket}"))); + } + } + + if service_err.raw().status().as_u16() == 404 { + return Ok(None); + } + } + return Err(Error::Network(format!("get_bucket_policy: {error}"))); + } + }; + + Ok(response.policy().map(|policy| policy.to_string())) + } + + async fn set_bucket_policy(&self, bucket: &str, policy: &str) -> Result<()> { + self.inner + .put_bucket_policy() + .bucket(bucket) + .policy(policy) + .send() + .await + .map_err(|e| Error::General(format!("set_bucket_policy: {e}")))?; + + Ok(()) + } + + async fn delete_bucket_policy(&self, bucket: &str) -> Result<()> { + match self + .inner + .delete_bucket_policy() + .bucket(bucket) + .send() + .await + { + Ok(_) => Ok(()), + Err(e) => { + if let aws_sdk_s3::error::SdkError::ServiceError(service_err) = &e { + if let Some(code) = service_err + .raw() + .headers() + .get("x-amz-error-code") + .and_then(|value| std::str::from_utf8(value.as_bytes()).ok()) + .map(|value| value.to_ascii_lowercase()) + { + if code == "nosuchbucketpolicy" || code == "nosuchpolicy" { + return Ok(()); + } + + if code == "nosuchbucket" { + return Err(Error::NotFound(format!("Bucket not found: {bucket}"))); + } + } + + if service_err.raw().status().as_u16() == 404 { + return Ok(()); + } + } + + let err_str = e.to_string(); + if err_str.contains("NoSuchBucket") { + Err(Error::NotFound(format!("Bucket not found: {bucket}"))) + } else if err_str.contains("NoSuchBucketPolicy") || err_str.contains("NoSuchPolicy") { + Ok(()) + } else { + Err(Error::General(format!("delete_bucket_policy: {e}"))) + } + } + } + } } #[cfg(test)] From 3bfbf4273015635f81f9b636a2900965c85b9d34 Mon Sep 17 00:00:00 2001 From: overtrue Date: Mon, 9 Mar 2026 21:51:08 +0800 Subject: [PATCH 2/2] feat(phase-2): address anonymous CI failures and review feedback --- Cargo.lock | 1 + crates/cli/Cargo.toml | 1 + crates/cli/src/commands/anonymous.rs | 169 +++++++++++++++++------- crates/cli/src/commands/mod.rs | 8 +- crates/s3/src/client.rs | 186 ++++++++++++++++++++------- 5 files changed, 270 insertions(+), 95 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8b71443..562eff8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2333,6 +2333,7 @@ dependencies = [ "tokio", "tracing", "tracing-subscriber", + "urlencoding", ] [[package]] diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index fa59300..4783e5f 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -50,6 +50,7 @@ humansize.workspace = true mime_guess.workspace = true glob.workspace = true shlex.workspace = true +urlencoding.workspace = true [features] default = [] diff --git a/crates/cli/src/commands/anonymous.rs b/crates/cli/src/commands/anonymous.rs index e97e971..a269333 100644 --- a/crates/cli/src/commands/anonymous.rs +++ b/crates/cli/src/commands/anonymous.rs @@ -217,6 +217,21 @@ async fn execute_set(args: SetArgs, output_config: OutputConfig) -> ExitCode { } }; + if permission == AccessLevel::Private && !target.prefix.is_empty() { + formatter.error( + "Cannot set a prefix target to private because that would delete the entire bucket policy", + ); + return ExitCode::UsageError; + } + + if !formatter.is_json() && !target.prefix.is_empty() { + formatter.println(&format!( + "Warning: setting '{}' will replace the entire bucket policy for '{}', which may remove unrelated statements", + target.path_with_bucket_prefix(), + target.bucket + )); + } + let (_alias, client) = match setup_anonymous_client(&target.alias, &formatter).await { Ok(client) => client, Err(code) => return code, @@ -269,7 +284,10 @@ async fn execute_set_json(args: SetJsonArgs, output_config: OutputConfig) -> Exi let policy = match tokio::fs::read_to_string(&args.file).await { Ok(content) => content, Err(error) => { - formatter.error(&format!("Failed to read policy file '{}': {error}", args.file)); + formatter.error(&format!( + "Failed to read policy file '{}': {error}", + args.file + )); return ExitCode::GeneralError; } }; @@ -284,6 +302,14 @@ async fn execute_set_json(args: SetJsonArgs, output_config: OutputConfig) -> Exi Err(code) => return code, }; + // This subcommand applies the provided JSON as-is and overwrites the whole bucket policy. + if !formatter.is_json() { + formatter.println(&format!( + "Warning: applying '{}' will replace the entire bucket policy for '{}'", + args.file, target.bucket + )); + } + match client.set_bucket_policy(&target.bucket, &policy).await { Ok(()) => { if formatter.is_json() { @@ -326,10 +352,12 @@ async fn execute_get(args: AnonymousPathArg, output_config: OutputConfig) -> Exi }; let permission = match client.get_bucket_policy(&target.bucket).await { - Ok(Some(policy)) => match parse_permission_from_policy(&policy, &target.bucket, &target.prefix) { - Ok(permission) => permission, - Err(_) => AccessLevel::Custom, - }, + Ok(Some(policy)) => { + match parse_permission_from_policy(&policy, &target.bucket, &target.prefix) { + Ok(permission) => permission, + Err(_) => AccessLevel::Custom, + } + } Ok(None) => AccessLevel::Private, Err(error) => { formatter.error(&format!( @@ -448,15 +476,6 @@ async fn execute_list(args: AnonymousPathArg, output_config: OutputConfig) -> Ex // For compatibility with `mc anonymous list`, always show all known anonymous rules for // the bucket. This avoids false negatives for bucket-level and inherited policies. - if rules.is_empty() { - if formatter.is_json() { - formatter.json(&Vec::::new()); - } else { - formatter.println("No anonymous policies found for target."); - } - return ExitCode::Success; - } - let mut merged: HashMap = HashMap::new(); for rule in rules { merged @@ -485,11 +504,8 @@ async fn execute_list(args: AnonymousPathArg, output_config: OutputConfig) -> Ex } else { formatter.println("Anonymous policies:"); for rule in rules { - formatter.println(&format!( - " {} => {}", - rule.resource, - rule.flags.level() - )); + let display_resource = display_rule_resource(&rule.resource); + formatter.println(&format!(" {display_resource} => {}", rule.flags.level())); } } @@ -548,23 +564,27 @@ async fn execute_links(args: LinksArgs, output_config: OutputConfig) -> ExitCode continue; } - let list_prefix = if target_prefix.is_empty() { - rule.resource.clone() - } else if rule.resource.is_empty() { - target_prefix.clone() - } else { - rule.resource.clone() - }; - - for key in list_keys_for_prefix( + let list_prefix = select_list_prefix(&rule.resource, &target_prefix); + let keys = match list_keys_for_prefix( &client, + &target.alias, &target.bucket, &list_prefix, args.recursive, ) .await - .unwrap_or_default() { + Ok(keys) => keys, + Err(error) => { + formatter.error(&format!( + "Failed to list objects for prefix '{}': {error}", + list_prefix + )); + return exit_code_from_error(&error); + } + }; + + for key in keys { if !target_prefix.is_empty() && !key_under_prefix(&key, &target_prefix) { continue; } @@ -623,6 +643,9 @@ fn parse_anonymous_path(path: &str) -> Result { if alias.is_empty() || bucket.is_empty() { return Err("Path must be in format alias/bucket[/prefix]".to_string()); } + if prefix.contains('*') || prefix.contains('?') { + return Err("Prefix cannot contain wildcard characters '*' or '?'".to_string()); + } Ok(AccessTarget { alias: alias.to_string(), @@ -696,17 +719,15 @@ fn build_policy(level: &AccessLevel, bucket: &str, prefix: &str) -> String { fn build_object_resource(bucket: &str, prefix: &str) -> String { if prefix.is_empty() { - format!("arn:aws:s3:::{}/*", bucket) + format!("arn:aws:s3:::{bucket}/*") } else { - format!( - "arn:aws:s3:::{}/*", - format!("{}/{}", bucket, prefix.trim_end_matches('/')) - ) + format!("arn:aws:s3:::{}/{}/*", bucket, prefix.trim_end_matches('/')) } } async fn list_keys_for_prefix( client: &S3Client, + alias_name: &str, bucket: &str, prefix: &str, recursive: bool, @@ -727,7 +748,7 @@ async fn list_keys_for_prefix( loop { options.continuation_token = continuation.clone(); - let path = RemotePath::new("anonymous", bucket, prefix); + let path = RemotePath::new(alias_name, bucket, prefix); let result = client.list_objects(&path, options.clone()).await?; for item in result.items { if !item.is_dir { @@ -837,9 +858,7 @@ fn target_covers_prefix(rule_prefix: &str, target_prefix: &str) -> bool { } fn normalize_bucket_resource(bucket: &str, resource: &str) -> Option { - let resource = resource - .strip_prefix("arn:aws:s3:::") - .unwrap_or(resource); + let resource = resource.strip_prefix("arn:aws:s3:::").unwrap_or(resource); if resource == bucket { return Some(String::new()); @@ -910,10 +929,12 @@ fn is_public_principal(principal: Option<&serde_json::Value>) -> bool { match principal { serde_json::Value::String(value) => value == "*", - serde_json::Value::Array(values) => values.iter().any(|value| is_public_principal(Some(value))), - serde_json::Value::Object(values) => { - values.values().any(|value| is_public_principal(Some(value))) + serde_json::Value::Array(values) => { + values.iter().any(|value| is_public_principal(Some(value))) } + serde_json::Value::Object(values) => values + .values() + .any(|value| is_public_principal(Some(value))), _ => false, } } @@ -929,7 +950,30 @@ fn prefixes_overlap(rule_prefix: &str, target_prefix: &str) -> bool { return true; } - rule_prefix.starts_with(&format!("{target_prefix}/")) || target_prefix.starts_with(&format!("{rule_prefix}/")) + rule_prefix.starts_with(&format!("{target_prefix}/")) + || target_prefix.starts_with(&format!("{rule_prefix}/")) +} + +fn select_list_prefix(rule_prefix: &str, target_prefix: &str) -> String { + if rule_prefix.is_empty() { + return target_prefix.to_string(); + } + if target_prefix.is_empty() { + return rule_prefix.to_string(); + } + + if target_prefix == rule_prefix || target_prefix.starts_with(&format!("{rule_prefix}/")) { + return target_prefix.to_string(); + } + if rule_prefix.starts_with(&format!("{target_prefix}/")) { + return rule_prefix.to_string(); + } + + target_prefix.to_string() +} + +fn display_rule_resource(resource: &str) -> &str { + if resource.is_empty() { "/" } else { resource } } fn key_under_prefix(key: &str, prefix: &str) -> bool { @@ -964,10 +1008,16 @@ fn parse_statement_list(value: &serde_json::Value) -> Result String { let endpoint = endpoint.trim_end_matches('/'); + let bucket = urlencoding::encode(bucket).into_owned(); if key.is_empty() { format!("{endpoint}/{bucket}") } else { - format!("{endpoint}/{bucket}/{key}") + let encoded_key = key + .split('/') + .map(|segment| urlencoding::encode(segment).into_owned()) + .collect::>() + .join("/"); + format!("{endpoint}/{bucket}/{encoded_key}") } } @@ -1057,13 +1107,21 @@ mod tests { assert!(parse_anonymous_path("local").is_err()); assert!(parse_anonymous_path("/mybucket").is_err()); assert!(parse_anonymous_path("local/").is_err()); + assert!(parse_anonymous_path("local/bucket/path/*").is_err()); + assert!(parse_anonymous_path("local/bucket/path/?").is_err()); } #[test] fn test_parse_permission() { - assert_eq!(AccessLevel::parse("private").ok(), Some(AccessLevel::Private)); + assert_eq!( + AccessLevel::parse("private").ok(), + Some(AccessLevel::Private) + ); assert_eq!(AccessLevel::parse("public").ok(), Some(AccessLevel::Public)); - assert_eq!(AccessLevel::parse("download").ok(), Some(AccessLevel::Download)); + assert_eq!( + AccessLevel::parse("download").ok(), + Some(AccessLevel::Download) + ); assert_eq!(AccessLevel::parse("upload").ok(), Some(AccessLevel::Upload)); assert!(AccessLevel::parse("invalid").is_err()); } @@ -1081,4 +1139,25 @@ mod tests { let url = build_public_url("http://localhost:9000/", "bucket", "path/to/object.txt"); assert_eq!(url, "http://localhost:9000/bucket/path/to/object.txt"); } + + #[test] + fn test_build_public_url_encodes_segments() { + let url = build_public_url( + "https://example.com/", + "my bucket", + "photos/with space/file#1?.jpg", + ); + assert_eq!( + url, + "https://example.com/my%20bucket/photos/with%20space/file%231%3F.jpg" + ); + } + + #[test] + fn test_select_list_prefix_uses_more_specific_prefix() { + assert_eq!(select_list_prefix("a", "a/b"), "a/b"); + assert_eq!(select_list_prefix("a/b", "a"), "a/b"); + assert_eq!(select_list_prefix("", "a"), "a"); + assert_eq!(select_list_prefix("a", ""), "a"); + } } diff --git a/crates/cli/src/commands/mod.rs b/crates/cli/src/commands/mod.rs index ff5751e..9924a7f 100644 --- a/crates/cli/src/commands/mod.rs +++ b/crates/cli/src/commands/mod.rs @@ -11,9 +11,9 @@ use crate::output::OutputConfig; mod admin; mod alias; +mod anonymous; mod cat; mod completions; -mod anonymous; pub mod cp; pub mod diff; mod find; @@ -183,11 +183,7 @@ pub async fn execute(cli: Cli) -> ExitCode { } Commands::Tag(cmd) => tag::execute(tag::TagArgs { command: cmd }, output_config).await, Commands::Anonymous(cmd) => { - anonymous::execute( - anonymous::AnonymousArgs { command: cmd }, - output_config, - ) - .await + anonymous::execute(anonymous::AnonymousArgs { command: cmd }, output_config).await } Commands::Quota(cmd) => { quota::execute(quota::QuotaArgs { command: cmd }, output_config).await diff --git a/crates/s3/src/client.rs b/crates/s3/src/client.rs index dc8161f..efa1ef7 100644 --- a/crates/s3/src/client.rs +++ b/crates/s3/src/client.rs @@ -23,6 +23,13 @@ use tokio::io::AsyncReadExt; /// streaming aws-chunked payloads. const SINGLE_PUT_OBJECT_MAX_SIZE: u64 = crate::multipart::DEFAULT_PART_SIZE; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum BucketPolicyErrorKind { + MissingPolicy, + MissingBucket, + Other, +} + /// Custom HTTP connector using reqwest, supporting insecure TLS (skip cert verification) /// and custom CA bundles. Used when `alias.insecure = true` or `alias.ca_bundle.is_some()`. #[derive(Debug, Clone)] @@ -254,6 +261,68 @@ impl S3Client { file_size > SINGLE_PUT_OBJECT_MAX_SIZE } + fn bucket_policy_error_kind( + error_code: Option<&str>, + status_code: Option, + error_text: &str, + ) -> BucketPolicyErrorKind { + let error_code = error_code.map(|code| code.to_ascii_lowercase()); + if matches!( + error_code.as_deref(), + Some("nosuchbucketpolicy") | Some("nosuchpolicy") + ) { + return BucketPolicyErrorKind::MissingPolicy; + } + if matches!(error_code.as_deref(), Some("nosuchbucket")) { + return BucketPolicyErrorKind::MissingBucket; + } + + let error_text = error_text.to_ascii_lowercase(); + if error_text.contains("nosuchbucketpolicy") || error_text.contains("nosuchpolicy") { + return BucketPolicyErrorKind::MissingPolicy; + } + if error_text.contains("nosuchbucket") { + return BucketPolicyErrorKind::MissingBucket; + } + if status_code == Some(404) { + return BucketPolicyErrorKind::MissingPolicy; + } + + BucketPolicyErrorKind::Other + } + + fn map_get_bucket_policy_error( + bucket: &str, + kind: BucketPolicyErrorKind, + error_text: &str, + ) -> Result> { + match kind { + BucketPolicyErrorKind::MissingPolicy => Ok(None), + BucketPolicyErrorKind::MissingBucket => { + Err(Error::NotFound(format!("Bucket not found: {bucket}"))) + } + BucketPolicyErrorKind::Other => { + Err(Error::Network(format!("get_bucket_policy: {error_text}"))) + } + } + } + + fn map_delete_bucket_policy_error( + bucket: &str, + kind: BucketPolicyErrorKind, + error_text: &str, + ) -> Result<()> { + match kind { + BucketPolicyErrorKind::MissingPolicy => Ok(()), + BucketPolicyErrorKind::MissingBucket => { + Err(Error::NotFound(format!("Bucket not found: {bucket}"))) + } + BucketPolicyErrorKind::Other => Err(Error::General(format!( + "delete_bucket_policy: {error_text}" + ))), + } + } + async fn read_next_part( file: &mut tokio::fs::File, file_path: &std::path::Path, @@ -1153,27 +1222,19 @@ impl ObjectStore for S3Client { let response = match self.inner.get_bucket_policy().bucket(bucket).send().await { Ok(policy) => policy, Err(error) => { - if let aws_sdk_s3::error::SdkError::ServiceError(service_err) = &error { - if let Some(code) = service_err + let error_text = error.to_string(); + let kind = if let aws_sdk_s3::error::SdkError::ServiceError(service_err) = &error { + let code = service_err .raw() .headers() .get("x-amz-error-code") - .and_then(|value| std::str::from_utf8(value.as_bytes()).ok()) - .map(|value| value.to_ascii_lowercase()) - { - if code == "nosuchbucketpolicy" || code == "nosuchpolicy" { - return Ok(None); - } - if code == "nosuchbucket" { - return Err(Error::NotFound(format!("Bucket not found: {bucket}"))); - } - } - - if service_err.raw().status().as_u16() == 404 { - return Ok(None); - } - } - return Err(Error::Network(format!("get_bucket_policy: {error}"))); + .and_then(|value| std::str::from_utf8(value.as_bytes()).ok()); + let status = Some(service_err.raw().status().as_u16()); + Self::bucket_policy_error_kind(code, status, &error_text) + } else { + Self::bucket_policy_error_kind(None, None, &error_text) + }; + return Self::map_get_bucket_policy_error(bucket, kind, &error_text); } }; @@ -1202,36 +1263,19 @@ impl ObjectStore for S3Client { { Ok(_) => Ok(()), Err(e) => { - if let aws_sdk_s3::error::SdkError::ServiceError(service_err) = &e { - if let Some(code) = service_err + let error_text = e.to_string(); + let kind = if let aws_sdk_s3::error::SdkError::ServiceError(service_err) = &e { + let code = service_err .raw() .headers() .get("x-amz-error-code") - .and_then(|value| std::str::from_utf8(value.as_bytes()).ok()) - .map(|value| value.to_ascii_lowercase()) - { - if code == "nosuchbucketpolicy" || code == "nosuchpolicy" { - return Ok(()); - } - - if code == "nosuchbucket" { - return Err(Error::NotFound(format!("Bucket not found: {bucket}"))); - } - } - - if service_err.raw().status().as_u16() == 404 { - return Ok(()); - } - } - - let err_str = e.to_string(); - if err_str.contains("NoSuchBucket") { - Err(Error::NotFound(format!("Bucket not found: {bucket}"))) - } else if err_str.contains("NoSuchBucketPolicy") || err_str.contains("NoSuchPolicy") { - Ok(()) + .and_then(|value| std::str::from_utf8(value.as_bytes()).ok()); + let status = Some(service_err.raw().status().as_u16()); + Self::bucket_policy_error_kind(code, status, &error_text) } else { - Err(Error::General(format!("delete_bucket_policy: {e}"))) - } + Self::bucket_policy_error_kind(None, None, &error_text) + }; + Self::map_delete_bucket_policy_error(bucket, kind, &error_text) } } } @@ -1248,6 +1292,60 @@ mod tests { assert_eq!(info.size_bytes, Some(1024)); } + #[test] + fn bucket_policy_error_kind_uses_error_code() { + assert_eq!( + S3Client::bucket_policy_error_kind(Some("NoSuchBucketPolicy"), Some(404), ""), + BucketPolicyErrorKind::MissingPolicy + ); + assert_eq!( + S3Client::bucket_policy_error_kind(Some("NoSuchBucket"), Some(404), ""), + BucketPolicyErrorKind::MissingBucket + ); + } + + #[test] + fn bucket_policy_error_kind_prefers_bucket_not_found_over_404_fallback() { + assert_eq!( + S3Client::bucket_policy_error_kind(None, Some(404), "NoSuchBucket"), + BucketPolicyErrorKind::MissingBucket + ); + assert_eq!( + S3Client::bucket_policy_error_kind(None, Some(404), "no details"), + BucketPolicyErrorKind::MissingPolicy + ); + } + + #[test] + fn bucket_policy_error_mapping_returns_expected_result() { + let get_missing_policy = S3Client::map_get_bucket_policy_error( + "bucket", + BucketPolicyErrorKind::MissingPolicy, + "NoSuchPolicy", + ) + .expect("missing policy should map to Ok(None)"); + assert!(get_missing_policy.is_none()); + + match S3Client::map_get_bucket_policy_error( + "bucket", + BucketPolicyErrorKind::MissingBucket, + "NoSuchBucket", + ) { + Err(Error::NotFound(message)) => assert!(message.contains("Bucket not found")), + other => panic!("Expected NotFound for missing bucket, got: {:?}", other), + } + + let delete_missing_policy = S3Client::map_delete_bucket_policy_error( + "bucket", + BucketPolicyErrorKind::MissingPolicy, + "NoSuchPolicy", + ); + assert!( + delete_missing_policy.is_ok(), + "Missing policy should be treated as successful delete" + ); + } + #[tokio::test] async fn reqwest_connector_insecure_without_ca_bundle_succeeds() { // When insecure is true and no CA bundle is provided, the connector should be created.