clean up error type: 'authentication failed' was a misnomer

trifectatechfoundation · Nov 26, 2024 · 72ef1b9 · 72ef1b9
1 parent a39d48b
commit 72ef1b9
Show file tree

Hide file tree

Showing 14 changed files with 58 additions and 66 deletions.
diff --git a/src/common/error.rs b/src/common/error.rs
@@ -22,7 +22,8 @@ pub enum Error {
     },
     UserNotFound(String),
     GroupNotFound(String),
-    Authentication(String),
+    Authorization(String),
+    InteractionRequired,
     Configuration(String),
     Options(String),
     Pam(PamError),
@@ -62,7 +63,8 @@ impl fmt::Display for Error {
             Error::InvalidCommand(p) => write!(f, "'{}': invalid command", p.display()),
             Error::UserNotFound(u) => write!(f, "user '{u}' not found"),
             Error::GroupNotFound(g) => write!(f, "group '{g}' not found"),
-            Error::Authentication(e) => write!(f, "authentication failed: {e}"),
+            Error::Authorization(u) => write!(f, "I'm sorry {u}. I'm afraid I can't do that"),
+            Error::InteractionRequired => write!(f, "interactive authentication is required"),
             Error::Configuration(e) => write!(f, "invalid configuration: {e}"),
             Error::Options(e) => write!(f, "{e}"),
             Error::Pam(e) => write!(f, "PAM error: {e}"),
@@ -105,10 +107,6 @@ impl From<std::io::Error> for Error {
 }
 
 impl Error {
-    pub fn auth(message: &str) -> Self {
-        Self::Authentication(message.to_string())
-    }
-
     /// Returns `true` if the error is [`Silent`].
     ///
     /// [`Silent`]: Error::Silent

diff --git a/src/sudo/pam.rs b/src/sudo/pam.rs
@@ -156,7 +156,7 @@ pub fn attempt_authenticate<C: Converser>(
                 if max_tries == 0 {
                     return Err(Error::MaxAuthAttempts(current_try));
                 } else if non_interactive {
-                    return Err(Error::Authentication("interaction required".to_string()));
+                    return Err(Error::InteractionRequired);
                 } else {
                     user_warn!("Authentication failed, try again.");
                 }

diff --git a/src/sudo/pipeline.rs b/src/sudo/pipeline.rs
@@ -62,10 +62,7 @@ impl<Policy: PolicyPlugin, Auth: AuthPlugin> Pipeline<Policy, Auth> {
 
         match authorization {
             Authorization::Forbidden => {
-                return Err(Error::auth(&format!(
-                    "I'm sorry {}. I'm afraid I can't do that",
-                    context.current_user.name
-                )));
+                return Err(Error::Authorization(context.current_user.name.to_string()));
             }
             Authorization::Allowed(auth) => {
                 self.apply_policy_to_context(&mut context, &policy)?;
@@ -118,10 +115,7 @@ impl<Policy: PolicyPlugin, Auth: AuthPlugin> Pipeline<Policy, Auth> {
 
         match pre.validate_authorization() {
             Authorization::Forbidden => {
-                return Err(Error::auth(&format!(
-                    "I'm sorry {}. I'm afraid I can't do that",
-                    context.current_user.name
-                )));
+                return Err(Error::Authorization(context.current_user.name.to_string()));
             }
             Authorization::Allowed(auth) => {
                 self.auth_and_update_record_file(&context, auth)?;

diff --git a/test-framework/sudo-compliance-tests/src/sudo/flag_group.rs b/test-framework/sudo-compliance-tests/src/sudo/flag_group.rs
@@ -170,7 +170,7 @@ fn group_does_not_add_groups_without_authorization() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         "a password is required"
     } else {
-        "authentication failed: I'm sorry ferris. I'm afraid I can't do that"
+        "I'm sorry ferris. I'm afraid I can't do that"
     };
 
     assert_contains!(output.stderr(), diagnostic);

diff --git a/test-framework/sudo-compliance-tests/src/sudo/flag_non_interactive.rs b/test-framework/sudo-compliance-tests/src/sudo/flag_non_interactive.rs
@@ -29,7 +29,7 @@ fn fails_if_password_needed() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         "sudo: a password is required"
     } else {
-        "interaction required"
+        "interactive authentication is required"
     };
     assert_contains!(stderr, diagnostic);
 
@@ -62,7 +62,7 @@ fn flag_remove_timestamp_plus_command_fails() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         "sudo: a password is required"
     } else {
-        "interaction required"
+        "interactive authentication is required"
     };
     assert_contains!(stderr, diagnostic);
 

diff --git a/test-framework/sudo-compliance-tests/src/sudo/nopasswd.rs b/test-framework/sudo-compliance-tests/src/sudo/nopasswd.rs
@@ -98,7 +98,7 @@ fn run_sudo_l_flag_without_pwd_if_one_nopasswd_is_set() -> Result<()> {
     } else {
         assert_contains!(
             actual,
-            format!("authentication failed: I'm sorry {USERNAME}. I'm afraid I can't do that")
+            format!("I'm sorry {USERNAME}. I'm afraid I can't do that")
         );
     }
 

diff --git a/test-framework/sudo-compliance-tests/src/sudo/sudoers/cmnd.rs b/test-framework/sudo-compliance-tests/src/sudo/sudoers/cmnd.rs
@@ -42,7 +42,7 @@ fn given_specific_command_then_other_command_is_not_allowed() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -77,7 +77,7 @@ fn command_specified_not_by_absolute_path_is_rejected() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -161,7 +161,7 @@ fn runas_override() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         format!("user root is not allowed to execute '{BIN_LS}' as ferris")
     } else {
-        "authentication failed: I'm sorry root. I'm afraid I can't do that".to_owned()
+        "I'm sorry root. I'm afraid I can't do that".to_owned()
     };
     assert_contains!(output.stderr(), diagnostic);
 
@@ -178,7 +178,7 @@ fn runas_override() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         format!("user root is not allowed to execute '{BIN_TRUE}' as root")
     } else {
-        "authentication failed: I'm sorry root. I'm afraid I can't do that".to_owned()
+        "I'm sorry root. I'm afraid I can't do that".to_owned()
     };
     assert_contains!(output.stderr(), diagnostic);
 
@@ -228,7 +228,7 @@ fn given_directory_then_commands_in_its_subdirectories_are_not_allowed() -> Resu
     let diagnostic = if sudo_test::is_original_sudo() {
         "user root is not allowed to execute '/usr/bin/true' as root"
     } else {
-        "authentication failed: I'm sorry root. I'm afraid I can't do that"
+        "I'm sorry root. I'm afraid I can't do that"
     };
     assert_contains!(output.stderr(), diagnostic);
 
@@ -314,7 +314,7 @@ fn arguments_can_be_forced() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         format!("user root is not allowed to execute '{BIN_TRUE} /root/ hello world' as root")
     } else {
-        "authentication failed: I'm sorry root. I'm afraid I can't do that".to_owned()
+        "I'm sorry root. I'm afraid I can't do that".to_owned()
     };
     assert_contains!(output.stderr(), diagnostic);
 
@@ -336,7 +336,7 @@ fn arguments_can_be_forbidded() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         format!("user root is not allowed to execute '{BIN_TRUE} /root/ hello world' as root")
     } else {
-        "authentication failed: I'm sorry root. I'm afraid I can't do that".to_owned()
+        "I'm sorry root. I'm afraid I can't do that".to_owned()
     };
     assert_contains!(output.stderr(), diagnostic);
 
@@ -358,7 +358,7 @@ fn wildcards_dont_cross_directory_boundaries() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         "user root is not allowed to execute '/usr/bin/sub/foo' as root"
     } else {
-        "authentication failed: I'm sorry root. I'm afraid I can't do that"
+        "I'm sorry root. I'm afraid I can't do that"
     };
     assert_contains!(output.stderr(), diagnostic);
 

diff --git a/test-framework/sudo-compliance-tests/src/sudo/sudoers/cmnd_alias.rs b/test-framework/sudo-compliance-tests/src/sudo/sudoers/cmnd_alias.rs
@@ -91,7 +91,7 @@ fn unlisted_cmnd_fails() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -117,7 +117,7 @@ fn command_specified_not_by_absolute_path_is_rejected() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -142,7 +142,7 @@ fn command_alias_negation() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -168,7 +168,7 @@ fn combined_cmnd_aliases() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -217,7 +217,7 @@ fn negation_not_order_sensitive() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -265,7 +265,7 @@ fn another_negation_combination() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -296,7 +296,7 @@ fn one_more_negation_combination() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -327,7 +327,7 @@ fn tripple_negation_combination() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -341,7 +341,7 @@ fn tripple_negation_combination() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -394,7 +394,7 @@ fn runas_override() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -414,7 +414,7 @@ fn runas_override() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 

diff --git a/test-framework/sudo-compliance-tests/src/sudo/sudoers/host_alias.rs b/test-framework/sudo-compliance-tests/src/sudo/sudoers/host_alias.rs
@@ -80,7 +80,7 @@ fn host_alias_negation() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -133,7 +133,7 @@ fn combined_host_aliases() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -161,7 +161,7 @@ fn unlisted_host_fails() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -189,7 +189,7 @@ fn negation_not_order_sensitive() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 

diff --git a/test-framework/sudo-compliance-tests/src/sudo/sudoers/host_list.rs b/test-framework/sudo-compliance-tests/src/sudo/sudoers/host_list.rs
@@ -43,7 +43,7 @@ fn given_specific_hostname_then_sudo_from_different_hostname_is_rejected() -> Re
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 
@@ -91,7 +91,7 @@ fn negation_rejects() -> Result<()> {
     } else {
         assert_contains!(
             stderr,
-            "authentication failed: I'm sorry root. I'm afraid I can't do that"
+            "I'm sorry root. I'm afraid I can't do that"
         );
     }
 

diff --git a/test-framework/sudo-compliance-tests/src/sudo/sudoers/includedir.rs b/test-framework/sudo-compliance-tests/src/sudo/sudoers/includedir.rs
@@ -41,7 +41,7 @@ fn ignores_files_with_names_ending_in_tilde() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         "root is not in the sudoers file"
     } else {
-        "authentication failed"
+        "I'm sorry root. I'm afraid I can't do that"
     };
     assert_contains!(output.stderr(), diagnostic);
 
@@ -63,7 +63,7 @@ fn ignores_files_with_names_that_contain_a_dot() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         "root is not in the sudoers file"
     } else {
-        "authentication failed"
+        "I'm sorry root. I'm afraid I can't do that"
     };
     assert_contains!(output.stderr(), diagnostic);
 
@@ -325,7 +325,7 @@ fn no_hostname_expansion() -> Result<()> {
     let diagnostic = if sudo_test::is_original_sudo() {
         "root is not in the sudoers file"
     } else {
-        "authentication failed"
+        "I'm sorry root. I'm afraid I can't do that"
     };
     assert_contains!(output.stderr(), diagnostic);
 
@@ -351,7 +351,7 @@ fn ignores_directory_with_bad_perms() -> Result<()> {
     } else {
         [
             format!("sudo-rs: {ETC_DIR}/sudoers2.d cannot be world-writable"),
-            "authentication failed".to_owned(),
+            "I'm sorry root. I'm afraid I can't do that".to_owned(),
         ]
     };
     for diagnostic in diagnostics {
@@ -381,7 +381,7 @@ fn ignores_directory_with_bad_ownership() -> Result<()> {
     } else {
         [
             format!("sudo-rs: {ETC_DIR}/sudoers2.d must be owned by root"),
-            "authentication failed".to_owned(),
+            "I'm sorry root. I'm afraid I can't do that".to_owned(),
         ]
     };