From fa27720a0c102d422f4b891e96802436d31a28bf Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Mon, 23 Mar 2026 15:59:21 +0100 Subject: [PATCH] quality: fix stale lock, add Rust idioms, efficiency improvements Security: - #28: is_stale_lock only treats ESRCH as stale (not EPERM) Correctness: - #29: atomic_write uses TmpGuard drop pattern (no leaked tmp files) - #30: parsers no longer trim() lines (preserves GECOS whitespace) Rust idioms: - #31: Username newtype with validation-on-construction - #32: ShadowEntry/PasswdEntry derive Default (cleaner tests) Efficiency: - #33: Cow<'static, str> in ShadowError (zero-alloc for static msgs) - #34: PAM read_from_tty opens /dev/tty once instead of 4 times - #35: atomic tmp_path_for uses AtomicU64 counter (thread-safe) 144 tests on Debian/Alpine/Fedora, zero clippy warnings. --- src/shadow-core/src/atomic.rs | 54 ++++++++-- src/shadow-core/src/error.rs | 13 +-- src/shadow-core/src/lock.rs | 30 +++--- src/shadow-core/src/pam.rs | 45 ++++---- src/shadow-core/src/passwd.rs | 11 +- src/shadow-core/src/shadow.rs | 186 ++++++++------------------------ src/shadow-core/src/validate.rs | 93 +++++++++++++--- 7 files changed, 220 insertions(+), 212 deletions(-) diff --git a/src/shadow-core/src/atomic.rs b/src/shadow-core/src/atomic.rs index a2e4a5d..ae0148a 100644 --- a/src/shadow-core/src/atomic.rs +++ b/src/shadow-core/src/atomic.rs @@ -16,9 +16,39 @@ use std::io::{self, Write}; use std::os::unix::fs::OpenOptionsExt; use std::os::unix::io::AsRawFd; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; use crate::error::ShadowError; +/// Drop guard that auto-deletes a temporary file unless explicitly committed. +/// +/// Ensures the tmp file is cleaned up on any error path, including panics. +struct TmpGuard { + path: PathBuf, + committed: bool, +} + +impl TmpGuard { + fn new(path: PathBuf) -> Self { + Self { + path, + committed: false, + } + } + + fn commit(&mut self) { + self.committed = true; + } +} + +impl Drop for TmpGuard { + fn drop(&mut self) { + if !self.committed { + let _ = fs::remove_file(&self.path); + } + } +} + /// Atomically replace a file's contents. /// /// Creates a temporary file in the same directory, writes content via the @@ -33,7 +63,7 @@ where F: FnOnce(&mut File) -> Result<(), ShadowError>, { let dir = target.parent().ok_or_else(|| { - ShadowError::Other(format!("no parent directory for {}", target.display())) + ShadowError::Other(format!("no parent directory for {}", target.display()).into()) })?; let tmp_path = tmp_path_for(target); @@ -44,6 +74,8 @@ where .map(|m| std::os::unix::fs::PermissionsExt::mode(&m.permissions())) .unwrap_or(0o600); + let mut guard = TmpGuard::new(tmp_path.clone()); + let mut tmp_file = std::fs::OpenOptions::new() .write(true) .create_new(true) @@ -51,12 +83,7 @@ where .open(&tmp_path) .map_err(|e| ShadowError::IoPath(e, tmp_path.clone()))?; - let result = f(&mut tmp_file); - - if result.is_err() { - let _ = fs::remove_file(&tmp_path); - return result; - } + f(&mut tmp_file)?; // Flush and fsync. tmp_file @@ -68,6 +95,9 @@ where // Atomic rename. fs::rename(&tmp_path, target).map_err(|e| ShadowError::IoPath(e, target.to_owned()))?; + // The rename succeeded — prevent the guard from deleting the (now-gone) tmp file. + guard.commit(); + // Fsync the parent directory to ensure the rename is durable. if let Ok(dir_fd) = File::open(dir) { let _ = nix::unistd::fsync(dir_fd.as_raw_fd()); @@ -76,14 +106,20 @@ where Ok(()) } -/// Generate a temporary file path in the same directory as the target. +/// Atomic counter for unique temp file names across threads. +static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); + +/// Generate a unique temporary file path in the same directory as the target. +/// +/// Uses PID + atomic counter to avoid collisions between threads. fn tmp_path_for(target: &Path) -> PathBuf { let file_name = target .file_name() .map(|n| n.to_string_lossy().to_string()) .unwrap_or_default(); let pid = std::process::id(); - target.with_file_name(format!(".{file_name}.shadow-rs.{pid}.tmp")) + let seq = TMP_COUNTER.fetch_add(1, Ordering::Relaxed); + target.with_file_name(format!(".{file_name}.shadow-rs.{pid}.{seq}.tmp")) } #[cfg(test)] diff --git a/src/shadow-core/src/error.rs b/src/shadow-core/src/error.rs index 6325b33..0cd2f92 100644 --- a/src/shadow-core/src/error.rs +++ b/src/shadow-core/src/error.rs @@ -5,6 +5,7 @@ //! Unified error types for shadow-rs utilities. +use std::borrow::Cow; use std::io; use std::path::PathBuf; @@ -24,22 +25,22 @@ pub enum ShadowError { IoPath(#[source] io::Error, PathBuf), /// File format parse error. #[error("parse error: {0}")] - Parse(String), + Parse(Cow<'static, str>), /// Lock acquisition failed. #[error("lock error: {0}")] - Lock(String), + Lock(Cow<'static, str>), /// Validation error (invalid username, UID range, etc.). #[error("{0}")] - Validation(String), + Validation(Cow<'static, str>), /// Authentication error (PAM failure, wrong password, etc.). #[error("authentication error: {0}")] - Auth(String), + Auth(Cow<'static, str>), /// Permission denied. #[error("permission denied: {0}")] - Permission(String), + Permission(Cow<'static, str>), /// Generic error with message. #[error("{0}")] - Other(String), + Other(Cow<'static, str>), } /// Print an error message prefixed with the utility name to stderr. diff --git a/src/shadow-core/src/lock.rs b/src/shadow-core/src/lock.rs index 2040eed..b651458 100644 --- a/src/shadow-core/src/lock.rs +++ b/src/shadow-core/src/lock.rs @@ -107,10 +107,9 @@ impl FileLock { } if Instant::now() >= deadline { - return Err(ShadowError::Lock(format!( - "cannot acquire lock {}: timed out", - lock_path.display(), - ))); + return Err(ShadowError::Lock( + format!("cannot acquire lock {}: timed out", lock_path.display()).into(), + )); } thread::sleep(RETRY_INTERVAL); @@ -125,10 +124,9 @@ impl FileLock { pub fn release(mut self) -> Result<(), ShadowError> { self.released = true; fs::remove_file(&self.lock_path).map_err(|e| { - ShadowError::Lock(format!( - "cannot release lock {}: {e}", - self.lock_path.display() - )) + ShadowError::Lock( + format!("cannot release lock {}: {e}", self.lock_path.display()).into(), + ) }) } } @@ -166,11 +164,14 @@ fn write_pid_file(tmp_path: &Path) -> Result<(), ShadowError> { .truncate(true) .mode(0o600) .open(tmp_path) - .map_err(|e| ShadowError::Lock(format!("cannot create {}: {e}", tmp_path.display())))?; + .map_err(|e| { + ShadowError::Lock(format!("cannot create {}: {e}", tmp_path.display()).into()) + })?; let pid = unistd::getpid(); - write!(file, "{pid}") - .map_err(|e| ShadowError::Lock(format!("cannot write {}: {e}", tmp_path.display())))?; + write!(file, "{pid}").map_err(|e| { + ShadowError::Lock(format!("cannot write {}: {e}", tmp_path.display()).into()) + })?; Ok(()) } @@ -191,8 +192,13 @@ fn is_stale_lock(lock_path: &Path) -> bool { } // Signal 0 checks if the process exists without actually sending a signal. + // Only ESRCH means "no such process". EPERM means the process exists but + // we lack permission to signal it — that is a valid lock holder. let pid = nix::unistd::Pid::from_raw(pid); - nix::sys::signal::kill(pid, None).is_err() + matches!( + nix::sys::signal::kill(pid, None), + Err(nix::errno::Errno::ESRCH) + ) } #[cfg(test)] diff --git a/src/shadow-core/src/pam.rs b/src/shadow-core/src/pam.rs index 56638a8..baa70fd 100644 --- a/src/shadow-core/src/pam.rs +++ b/src/shadow-core/src/pam.rs @@ -366,22 +366,19 @@ fn prompt_for_input( /// Read a line from `/dev/tty`, optionally disabling echo. /// -/// Opens `/dev/tty` directly (not stdin) to ensure we talk to the real -/// terminal even if stdin has been redirected. Uses `nix::sys::termios` to -/// disable `ECHO` for password prompts and restores the original settings -/// afterward (including on error, via a drop guard). +/// Opens `/dev/tty` once with read+write mode (not stdin) to ensure we talk +/// to the real terminal even if stdin has been redirected. Uses +/// `nix::sys::termios` to disable `ECHO` for password prompts and restores +/// the original settings afterward (including on error, via a drop guard). fn read_from_tty(message: &PamMessage, echo: bool) -> io::Result { - let tty = File::options().read(true).write(true).open("/dev/tty")?; + let mut tty = File::options().read(true).write(true).open("/dev/tty")?; // Show prompt. if !message.msg.is_null() { // SAFETY: `msg` is a null-terminated C string provided by PAM. let prompt = unsafe { CStr::from_ptr(message.msg) }; - let prompt_bytes = prompt.to_bytes(); - // Write prompt directly to the tty fd. - let mut tty_write = File::options().write(true).open("/dev/tty")?; - tty_write.write_all(prompt_bytes)?; - tty_write.flush()?; + tty.write_all(prompt.to_bytes())?; + tty.flush()?; } // Disable echo if needed, with a guard to restore on drop. @@ -392,15 +389,13 @@ fn read_from_tty(message: &PamMessage, echo: bool) -> io::Result { }; // Read one line from the tty. - let tty_read = File::open("/dev/tty")?; - let mut reader = io::BufReader::new(tty_read); + let mut reader = io::BufReader::new(tty.try_clone()?); let mut line = String::new(); reader.read_line(&mut line)?; // Print a newline after hidden input so the cursor moves down. if !echo { - let mut tty_newline = File::options().write(true).open("/dev/tty")?; - let _ = tty_newline.write_all(b"\n"); + tty.write_all(b"\n")?; } // Strip trailing newline. @@ -562,9 +557,9 @@ impl PamContext { /// Returns `ShadowError::Auth` if `pam_start` fails. pub fn new(service: &str, user: &str, mode: ConvMode) -> Result { let service_c = CString::new(service) - .map_err(|_| ShadowError::Auth("service name contains null byte".to_string()))?; + .map_err(|_| ShadowError::Auth("service name contains null byte".into()))?; let user_c = CString::new(user) - .map_err(|_| ShadowError::Auth("username contains null byte".to_string()))?; + .map_err(|_| ShadowError::Auth("username contains null byte".into()))?; let conv_data = Box::new(ConvData { mode }); let conv_data_ptr = (&raw const *conv_data).cast_mut(); @@ -591,14 +586,14 @@ impl PamContext { }; if rc != return_code::PAM_SUCCESS { - return Err(ShadowError::Auth(format!( - "pam_start failed with code {rc}" - ))); + return Err(ShadowError::Auth( + format!("pam_start failed with code {rc}").into(), + )); } if handle.is_null() { return Err(ShadowError::Auth( - "pam_start returned success but null handle".to_string(), + "pam_start returned success but null handle".into(), )); } @@ -627,7 +622,7 @@ impl PamContext { self.last_status = rc; if rc != return_code::PAM_SUCCESS { - return Err(ShadowError::Auth(self.strerror(rc))); + return Err(ShadowError::Auth(self.strerror(rc).into())); } Ok(()) @@ -646,7 +641,7 @@ impl PamContext { self.last_status = rc; if rc != return_code::PAM_SUCCESS { - return Err(ShadowError::Auth(self.strerror(rc))); + return Err(ShadowError::Auth(self.strerror(rc).into())); } Ok(()) @@ -666,7 +661,7 @@ impl PamContext { self.last_status = rc; if rc != return_code::PAM_SUCCESS { - return Err(ShadowError::Auth(self.strerror(rc))); + return Err(ShadowError::Auth(self.strerror(rc).into())); } Ok(()) @@ -681,7 +676,7 @@ impl PamContext { /// Returns `ShadowError::Auth` if `pam_set_item` fails. pub fn set_item_str(&mut self, item: i32, value: &str) -> Result<(), ShadowError> { let value_c = CString::new(value) - .map_err(|_| ShadowError::Auth("item value contains null byte".to_string()))?; + .map_err(|_| ShadowError::Auth("item value contains null byte".into()))?; // SAFETY: `self.handle` is valid. `value_c` is a valid null-terminated // C string. PAM copies the value internally, so `value_c` does not need @@ -691,7 +686,7 @@ impl PamContext { self.last_status = rc; if rc != return_code::PAM_SUCCESS { - return Err(ShadowError::Auth(self.strerror(rc))); + return Err(ShadowError::Auth(self.strerror(rc).into())); } Ok(()) diff --git a/src/shadow-core/src/passwd.rs b/src/shadow-core/src/passwd.rs index a908101..67c85f3 100644 --- a/src/shadow-core/src/passwd.rs +++ b/src/shadow-core/src/passwd.rs @@ -22,7 +22,7 @@ use std::str::FromStr; use crate::error::ShadowError; /// A single entry from `/etc/passwd`. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct PasswdEntry { /// Login name. pub name: String, @@ -85,10 +85,10 @@ impl FromStr for PasswdEntry { let uid = uid_str .parse::() - .map_err(|e| ShadowError::Parse(format!("invalid UID '{uid_str}': {e}")))?; + .map_err(|e| ShadowError::Parse(format!("invalid UID '{uid_str}': {e}").into()))?; let gid = gid_str .parse::() - .map_err(|e| ShadowError::Parse(format!("invalid GID '{gid_str}': {e}")))?; + .map_err(|e| ShadowError::Parse(format!("invalid GID '{gid_str}': {e}").into()))?; Ok(Self { name: name.to_string(), @@ -116,11 +116,10 @@ pub fn read_passwd_file(path: &Path) -> Result, ShadowError> { for line in reader.lines() { let line = line?; - let trimmed = line.trim(); - if trimmed.is_empty() || trimmed.starts_with('#') { + if line.is_empty() || line.starts_with('#') { continue; } - entries.push(trimmed.parse()?); + entries.push(line.parse()?); } Ok(entries) diff --git a/src/shadow-core/src/shadow.rs b/src/shadow-core/src/shadow.rs index 3cc0cdc..9bec1d8 100644 --- a/src/shadow-core/src/shadow.rs +++ b/src/shadow-core/src/shadow.rs @@ -21,7 +21,7 @@ use std::str::FromStr; use crate::error::ShadowError; /// A single entry from `/etc/shadow`. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct ShadowEntry { /// Login name (must match an `/etc/passwd` entry). pub name: String, @@ -114,7 +114,7 @@ fn parse_optional_field(field: &str) -> Result, ShadowError> { field .parse::() .map(Some) - .map_err(|e| ShadowError::Parse(format!("invalid numeric field '{field}': {e}"))) + .map_err(|e| ShadowError::Parse(format!("invalid numeric field '{field}': {e}").into())) } } @@ -209,11 +209,10 @@ pub fn read_shadow_file(path: &Path) -> Result, ShadowError> { for line in reader.lines() { let line = line?; - let trimmed = line.trim(); - if trimmed.is_empty() || trimmed.starts_with('#') { + if line.is_empty() || line.starts_with('#') { continue; } - entries.push(trimmed.parse()?); + entries.push(line.parse()?); } Ok(entries) @@ -281,15 +280,9 @@ mod tests { #[test] fn test_is_locked_with_bang() { let entry = ShadowEntry { - name: "u".to_string(), - passwd: "!$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "!$6$hash".into(), + ..Default::default() }; assert!(entry.is_locked()); } @@ -297,15 +290,9 @@ mod tests { #[test] fn test_is_locked_without_bang() { let entry = ShadowEntry { - name: "u".to_string(), - passwd: "$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "$6$hash".into(), + ..Default::default() }; assert!(!entry.is_locked()); } @@ -313,15 +300,8 @@ mod tests { #[test] fn test_has_no_password_empty() { let entry = ShadowEntry { - name: "u".to_string(), - passwd: String::new(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + ..Default::default() }; assert!(entry.has_no_password()); } @@ -329,15 +309,9 @@ mod tests { #[test] fn test_has_no_password_with_hash() { let entry = ShadowEntry { - name: "u".to_string(), - passwd: "$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "$6$hash".into(), + ..Default::default() }; assert!(!entry.has_no_password()); } @@ -345,15 +319,9 @@ mod tests { #[test] fn test_lock_adds_bang() { let mut entry = ShadowEntry { - name: "u".to_string(), - passwd: "$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "$6$hash".into(), + ..Default::default() }; entry.lock(); assert_eq!(entry.passwd, "!$6$hash"); @@ -362,15 +330,9 @@ mod tests { #[test] fn test_lock_already_locked_adds_another() { let mut entry = ShadowEntry { - name: "u".to_string(), - passwd: "!$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "!$6$hash".into(), + ..Default::default() }; entry.lock(); assert_eq!(entry.passwd, "!!$6$hash"); @@ -379,15 +341,9 @@ mod tests { #[test] fn test_unlock_removes_bang() { let mut entry = ShadowEntry { - name: "u".to_string(), - passwd: "!$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "!$6$hash".into(), + ..Default::default() }; assert!(entry.unlock()); assert_eq!(entry.passwd, "$6$hash"); @@ -396,15 +352,9 @@ mod tests { #[test] fn test_unlock_not_locked_returns_false() { let mut entry = ShadowEntry { - name: "u".to_string(), - passwd: "$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "$6$hash".into(), + ..Default::default() }; assert!(!entry.unlock()); assert_eq!(entry.passwd, "$6$hash", "should be unchanged"); @@ -414,15 +364,9 @@ mod tests { fn test_unlock_only_bang_returns_false() { // "!" alone cannot be unlocked — would result in empty password. let mut entry = ShadowEntry { - name: "u".to_string(), - passwd: "!".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "!".into(), + ..Default::default() }; assert!(!entry.unlock()); assert_eq!(entry.passwd, "!", "should be unchanged"); @@ -432,15 +376,9 @@ mod tests { fn test_unlock_double_bang_returns_false() { // "!!" — removing one '!' leaves "!" which is still invalid. let mut entry = ShadowEntry { - name: "u".to_string(), - passwd: "!!".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "!!".into(), + ..Default::default() }; assert!(!entry.unlock()); assert_eq!(entry.passwd, "!!", "should be unchanged"); @@ -449,15 +387,9 @@ mod tests { #[test] fn test_delete_password_clears() { let mut entry = ShadowEntry { - name: "u".to_string(), - passwd: "$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "$6$hash".into(), + ..Default::default() }; entry.delete_password(); assert_eq!(entry.passwd, ""); @@ -466,15 +398,10 @@ mod tests { #[test] fn test_expire_sets_zero() { let mut entry = ShadowEntry { - name: "u".to_string(), - passwd: "$6$hash".to_string(), + name: "u".into(), + passwd: "$6$hash".into(), last_change: Some(19500), - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + ..Default::default() }; entry.expire(); assert_eq!(entry.last_change, Some(0)); @@ -483,15 +410,9 @@ mod tests { #[test] fn test_status_char_locked() { let entry = ShadowEntry { - name: "u".to_string(), - passwd: "!$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "!$6$hash".into(), + ..Default::default() }; assert_eq!(entry.status_char(), "L"); } @@ -499,15 +420,8 @@ mod tests { #[test] fn test_status_char_no_password() { let entry = ShadowEntry { - name: "u".to_string(), - passwd: String::new(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + ..Default::default() }; assert_eq!(entry.status_char(), "NP"); } @@ -515,15 +429,9 @@ mod tests { #[test] fn test_status_char_usable() { let entry = ShadowEntry { - name: "u".to_string(), - passwd: "$6$hash".to_string(), - last_change: None, - min_age: None, - max_age: None, - warn_days: None, - inactive_days: None, - expire_date: None, - reserved: String::new(), + name: "u".into(), + passwd: "$6$hash".into(), + ..Default::default() }; assert_eq!(entry.status_char(), "P"); } diff --git a/src/shadow-core/src/validate.rs b/src/shadow-core/src/validate.rs index 4eaa636..f137098 100644 --- a/src/shadow-core/src/validate.rs +++ b/src/shadow-core/src/validate.rs @@ -32,9 +32,10 @@ pub fn validate_username(name: &str) -> Result<(), ShadowError> { } if name.len() > MAX_USERNAME_LEN { - return Err(ShadowError::Validation(format!( - "username '{name}' exceeds maximum length of {MAX_USERNAME_LEN} characters" - ))); + return Err(ShadowError::Validation( + format!("username '{name}' exceeds maximum length of {MAX_USERNAME_LEN} characters") + .into(), + )); } let mut chars = name.chars(); @@ -44,34 +45,78 @@ pub fn validate_username(name: &str) -> Result<(), ShadowError> { }; if !first.is_ascii_lowercase() && first != '_' { - return Err(ShadowError::Validation(format!( - "username '{name}' must start with a lowercase letter or underscore" - ))); + return Err(ShadowError::Validation( + format!("username '{name}' must start with a lowercase letter or underscore").into(), + )); } for ch in chars { if !ch.is_ascii_lowercase() && !ch.is_ascii_digit() && ch != '_' && ch != '-' && ch != '.' { - return Err(ShadowError::Validation(format!( - "username '{name}' contains invalid character '{ch}'" - ))); + return Err(ShadowError::Validation( + format!("username '{name}' contains invalid character '{ch}'").into(), + )); } } if name.ends_with('.') { - return Err(ShadowError::Validation(format!( - "username '{name}' must not end with a period" - ))); + return Err(ShadowError::Validation( + format!("username '{name}' must not end with a period").into(), + )); } if name.chars().all(|c| c == '.') { - return Err(ShadowError::Validation(format!( - "username '{name}' must not consist only of periods" - ))); + return Err(ShadowError::Validation( + format!("username '{name}' must not consist only of periods").into(), + )); } Ok(()) } +/// A validated Linux username. +/// +/// Guarantees that the contained string passes all `validate_username` rules. +/// Use `Username::new()` to validate and construct. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct Username(String); + +impl Username { + /// Validate and create a new `Username`. + /// + /// # Errors + /// + /// Returns `ShadowError::Validation` if the name violates any rule. + pub fn new(name: &str) -> Result { + validate_username(name)?; + Ok(Self(name.to_string())) + } + + /// Get the username as a string slice. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl std::fmt::Display for Username { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl AsRef for Username { + fn as_ref(&self) -> &str { + &self.0 + } +} + +impl std::ops::Deref for Username { + type Target = str; + fn deref(&self) -> &str { + &self.0 + } +} + #[cfg(test)] mod tests { use super::*; @@ -160,4 +205,22 @@ mod tests { fn test_uppercase_rejected() { assert!(validate_username("Root").is_err()); } + + // ------------------------------------------------------------------- + // Username newtype tests + // ------------------------------------------------------------------- + + #[test] + fn test_username_newtype_valid() { + let u = Username::new("testuser").unwrap(); + assert_eq!(u.as_str(), "testuser"); + assert_eq!(&*u, "testuser"); // Deref + assert_eq!(format!("{u}"), "testuser"); // Display + } + + #[test] + fn test_username_newtype_invalid() { + assert!(Username::new("").is_err()); + assert!(Username::new("Root").is_err()); + } }