diff --git a/Cargo.lock b/Cargo.lock index f46e7eb..8b97008 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -96,7 +96,7 @@ checksum = "cf8baf1c55e62ffcace7a9f06f4bd9cd3f0c4beb022d3b367256b91b87513d98" [[package]] name = "memona-plugin-ftp" -version = "0.1.1" +version = "0.1.2" dependencies = [ "memona-plugin-sdk", ] diff --git a/Cargo.toml b/Cargo.toml index 3265f3f..de78c21 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "memona-plugin-ftp" -version = "0.1.1" +version = "0.1.2" edition = "2024" license = "MIT" diff --git a/manifest.json b/manifest.json index 34ad259..1a792d9 100644 --- a/manifest.json +++ b/manifest.json @@ -2,7 +2,7 @@ "schemaVersion": 1, "id": "memona/ftp", "name": "FTP", - "version": "0.1.1", + "version": "0.1.2", "apiVersion": "0.1.0", "description": "Open an FTP server as a folder, and let agents work in it.", "license": "MIT", diff --git a/src/control.rs b/src/control.rs index 0fe3f5b..4cfbbba 100644 --- a/src/control.rs +++ b/src/control.rs @@ -21,7 +21,8 @@ pub fn outcome(code: u16) -> Result<(), ErrorCode> { match code { 200..=399 => Ok(()), // 530 is a refused login and 550 covers both a missing file and a denied - // one; the server does not distinguish them, so neither does this. + // one; the server does not distinguish them, so neither does this. Where + // the command itself narrows what a 550 can mean, `refusal` does better. 530 | 532 | 553 => Err(ErrorCode::Denied), 550 => Err(ErrorCode::NotFound), 450 | 452 | 552 => Err(ErrorCode::Conflict), @@ -32,6 +33,40 @@ pub fn outcome(code: u16) -> Result<(), ErrorCode> { } } +/// What a refusal of a create or a delete means. Those two commands are the +/// only ones whose 550 can be drawn by something already in the way, so they +/// are the only ones whose reply text is read. A listing, a stat or a read is +/// classified by `outcome` alone, because pure-ftpd answers a path inside a +/// folder it will not open with `550 Can't check for file existence`, which +/// truly cannot be told apart from absence and must stay `NotFound`. +/// +/// A server whose wording this does not recognise keeps the plain +/// classification, so an unfamiliar phrasing degrades to today's answer +/// rather than to a wrong `Conflict`. +pub fn refusal(code: u16, text: &str) -> Result<(), ErrorCode> { + match outcome(code) { + Err(ErrorCode::NotFound) if names_a_conflict(text) => { + Err(ErrorCode::Conflict) + } + verdict => verdict, + } +} + +/// Whether a refusal names something already there: a path that exists, or a +/// directory that still holds children. Matched on whole words, so the +/// `existence` of "Can't check for file existence" is never the `exists` of +/// "File exists". +fn names_a_conflict(text: &str) -> bool { + let lowered = text.to_lowercase(); + let words: Vec<&str> = lowered + .split(|letter: char| !letter.is_ascii_alphanumeric()) + .filter(|word| !word.is_empty()) + .collect(); + words.contains(&"exists") + || words.contains(&"nonempty") + || words.windows(2).any(|pair| matches!(pair, ["not" | "non", "empty"])) +} + /// What the first reply to a transfer command means. A transfer answers with /// a preliminary reply while the data connection is open; its completion /// comes only after that connection closes and is read separately. @@ -176,7 +211,7 @@ fn extended_port(text: &str) -> Option { #[cfg(test)] mod tests { - use super::{extended_port, outcome, preliminary}; + use super::{extended_port, outcome, preliminary, refusal}; use memona_plugin_sdk::ErrorCode; #[test] @@ -207,6 +242,81 @@ mod tests { assert!(matches!(outcome(150), Err(ErrorCode::Unavailable))); } + #[test] + fn a_refused_create_or_delete_names_what_is_in_the_way() { + // The two refusals a person can see through, in pure-ftpd's wording. + assert!(matches!( + refusal(550, "550 Can't create directory: File exists"), + Err(ErrorCode::Conflict) + )); + assert!(matches!( + refusal(550, "550 Can't remove directory: Directory not empty"), + Err(ErrorCode::Conflict) + )); + // The same two conditions as other servers phrase them. + assert!(matches!( + refusal(550, "550 Cannot create a file when that file already exists."), + Err(ErrorCode::Conflict) + )); + assert!(matches!( + refusal(550, "550 /work/dir: Directory not empty"), + Err(ErrorCode::Conflict) + )); + assert!(matches!( + refusal(550, "550 The directory is non-empty."), + Err(ErrorCode::Conflict) + )); + } + + #[test] + fn a_refusal_that_cannot_be_read_stays_a_missing_path() { + // The genuine ambiguity: a path inside a folder that will not open. The + // word is "existence", not "exists", and it must not be mistaken for one. + assert!(matches!( + refusal(550, "550 Can't check for file existence"), + Err(ErrorCode::NotFound) + )); + // A path that really is absent, and one the account may not touch. + assert!(matches!( + refusal(550, "550 Can't remove directory: No such file or directory"), + Err(ErrorCode::NotFound) + )); + assert!(matches!( + refusal(550, "550 Can't create directory: Permission denied"), + Err(ErrorCode::NotFound) + )); + // A server that says nothing useful keeps the plain classification. + assert!(matches!( + refusal(550, "550 Create directory operation failed."), + Err(ErrorCode::NotFound) + )); + assert!(matches!( + refusal(550, "550 Échec de la création du répertoire"), + Err(ErrorCode::NotFound) + )); + } + + #[test] + fn only_a_550_is_reread_for_a_conflict() { + assert!(refusal(257, "257 \"/work/dir\" created").is_ok()); + assert!(matches!( + refusal(530, "530 Login incorrect, the account exists"), + Err(ErrorCode::Denied) + )); + assert!(matches!( + refusal(553, "553 File name not allowed, it exists"), + Err(ErrorCode::Denied) + )); + assert!(matches!( + refusal(452, "452 Not enough space, and the file exists"), + Err(ErrorCode::Conflict) + )); + assert!(matches!( + refusal(421, "421 Service not available"), + Err(ErrorCode::Unavailable) + )); + } + #[test] fn a_transfer_starts_on_a_preliminary_reply_only() { assert!(preliminary(125).is_ok()); diff --git a/src/lib.rs b/src/lib.rs index be89908..0d3be11 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,7 +4,7 @@ mod control; mod listing; mod transfer; -use control::{Control, outcome, preliminary, transport}; +use control::{Control, outcome, preliminary, refusal, transport}; use listing::{machine_info, page}; use memona_plugin_sdk::memona::filesystem::host; use memona_plugin_sdk::{ @@ -339,9 +339,14 @@ impl Guest for Ftp { })) } + /// A refused create is a conflict when the path is already taken, which is + /// the one thing the server says in words rather than in its reply code. fn mkdir(target: Locator) -> Result { let path = route(&target)?; - with_control(|control| control.expect(&format!("MKD {path}")))?; + with_control(|control| { + let (code, text) = control.send(&format!("MKD {path}"))?; + refusal(code, &text) + })?; Ok(FileInfo { kind: EntryKind::Directory, size: 0, @@ -369,7 +374,8 @@ impl Guest for Ftp { } /// A file and an empty directory take different commands, and the server - /// only says which was wrong after the attempt. + /// only says which was wrong after the attempt. A directory that still + /// holds children is refused as a conflict, not as an absence. fn remove(target: Locator) -> Result<(), ErrorCode> { let path = route(&target)?; with_control(|control| { @@ -377,8 +383,8 @@ impl Guest for Ftp { if (200..300).contains(&code) { return Ok(()); } - let (code, _) = control.send(&format!("RMD {path}"))?; - outcome(code) + let (code, text) = control.send(&format!("RMD {path}"))?; + refusal(code, &text) }) }