Fixes for the users service (#3026)

imobachgs · web-flow · commit 7378f2ee7aaa · 2026-01-14T23:15:04.000Z
Fixes for the users service

- Improve the detection of missing information for the root and the
first user.
- Crate the first user and/or set the root password/public key at the
end of the installation.
diff --git a/rust/agama-manager/src/service.rs b/rust/agama-manager/src/service.rs
@@ -685,6 +685,7 @@ impl MessageHandler<message::RunAction> for Service {
                     storage: self.storage.clone(),
                     files: self.files.clone(),
                     progress: self.progress.clone(),
+                    users: self.users.clone(),
                 };
                 action.run();
             }
@@ -755,6 +756,7 @@ struct InstallAction {
     storage: Handler<storage::Service>,
     files: Handler<files::Service>,
     progress: Handler<progress::Service>,
+    users: Handler<users::Service>,
 }
 
 impl InstallAction {
@@ -823,6 +825,7 @@ impl InstallAction {
         self.files.call(files::message::WriteFiles).await?;
         self.network.install().await?;
         self.hostname.call(hostname::message::Install).await?;
+        self.users.call(users::message::Install).await?;
 
         // call files before storage finish as it unmount /mnt/run which is important for chrooted scripts
         self.files
diff --git a/rust/agama-users/src/model.rs b/rust/agama-users/src/model.rs
@@ -25,6 +25,7 @@ use std::fs;
 use std::fs::{OpenOptions, Permissions};
 use std::io::Write;
 use std::os::unix::fs::PermissionsExt;
+use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
 
 /// Abstract the users-related configuration from the underlying system.
@@ -61,7 +62,17 @@ pub trait ModelAdapter: Send + 'static {
 }
 
 /// [ModelAdapter] implementation for systemd-based systems.
-pub struct Model {}
+pub struct Model {
+    install_dir: PathBuf,
+}
+
+impl Model {
+    pub fn new<P: AsRef<Path>>(install_dir: P) -> Self {
+        Self {
+            install_dir: PathBuf::from(install_dir.as_ref()),
+        }
+    }
+}
 
 impl ModelAdapter for Model {
     fn install(&self, config: &Config) -> Result<(), service::Error> {
@@ -84,7 +95,10 @@ impl ModelAdapter for Model {
             return Err(service::Error::MissingUserData);
         };
 
-        let useradd = Command::new("/usr/sbin/useradd").arg(user_name).output()?;
+        let useradd = Command::new("chroot")
+            .arg(&self.install_dir)
+            .args(["useradd", &user_name])
+            .output()?;
 
         if !useradd.status.success() {
             tracing::error!("User {} creation failed", user_name);
@@ -126,7 +140,9 @@ impl ModelAdapter for Model {
         user_name: &str,
         user_password: &UserPassword,
     ) -> Result<(), service::Error> {
-        let mut passwd_cmd = Command::new("/usr/sbin/chpasswd");
+        let mut passwd_cmd = Command::new("chroot");
+        passwd_cmd.arg(&self.install_dir);
+        passwd_cmd.arg("chpasswd");
 
         if user_password.hashed_password {
             passwd_cmd.arg("-e");
@@ -156,7 +172,7 @@ impl ModelAdapter for Model {
 
     /// Updates root's authorized_keys file with SSH key
     fn update_authorized_keys(&self, ssh_key: &str) -> Result<(), service::Error> {
-        let file_name = String::from("/root/.ssh/authorized_keys");
+        let file_name = self.install_dir.join("root/.ssh/authorized_keys");
         let mut authorized_keys_file = OpenOptions::new()
             .create(true)
             .append(true)
@@ -177,8 +193,9 @@ impl ModelAdapter for Model {
             return Ok(());
         };
 
-        let chfn = Command::new("/usr/bin/chfn")
-            .args(["-f", &full_name, &user_name])
+        let chfn = Command::new("chroot")
+            .arg(&self.install_dir)
+            .args(["chfn", "-f", &full_name, &user_name])
             .output()?;
 
         if !chfn.status.success() {
diff --git a/rust/agama-users/src/service.rs b/rust/agama-users/src/service.rs
@@ -81,7 +81,7 @@ impl Starter {
     pub async fn start(self) -> Result<Handler<Service>, Error> {
         let model = match self.model {
             Some(model) => model,
-            None => Box::new(Model {}),
+            None => Box::new(Model::new("/mnt")),
         };
         let service = Service {
             full_config: Config::new(),
@@ -117,28 +117,21 @@ impl Service {
     }
 
     fn get_proposal(&self) -> Option<api::users::Config> {
-        if self.find_issues().is_empty() {
+        if !self.full_config.is_empty() {
             return self.full_config.to_api();
         }
 
         None
     }
 
+    /// Updates the service issues.
+    ///
+    /// At least one user is mandatory
+    /// - typicaly root or
+    /// - first user which will operate throught sudo
     fn update_issues(&self) -> Result<(), Error> {
-        let issues = self.find_issues();
-        self.issues
-            .cast(issue::message::Set::new(Scope::Users, issues))?;
-        Ok(())
-    }
-
-    fn find_issues(&self) -> Vec<Issue> {
         let mut issues = vec![];
-
-        tracing::debug!("hello: {:?}", &self.full_config);
-        // At least one user is mandatory
-        // - typicaly root or
-        // - first user which will operate throught sudo
-        if self.full_config.root.is_none() && self.full_config.first_user.is_none() {
+        if self.full_config.is_empty() {
             issues.push(Issue::new(
                 "users.no_auth",
                 &gettext(
@@ -147,7 +140,21 @@ impl Service {
             ));
         }
 
-        issues
+        if self
+            .full_config
+            .first_user
+            .as_ref()
+            .is_some_and(|u| !u.is_valid())
+        {
+            issues.push(Issue::new(
+                "users.invalid_user",
+                &gettext("First user information is incomplete"),
+            ));
+        }
+
+        self.issues
+            .cast(issue::message::Set::new(Scope::Users, issues))?;
+        Ok(())
     }
 }
 
diff --git a/rust/agama-utils/src/api/users/config.rs b/rust/agama-utils/src/api/users/config.rs
@@ -51,6 +51,18 @@ impl Config {
 
         Some(self.clone())
     }
+
+    pub fn is_empty(&self) -> bool {
+        if self.root.as_ref().is_some_and(|r| !r.is_empty()) {
+            return false;
+        }
+
+        if self.first_user.as_ref().is_some_and(|u| !u.is_empty()) {
+            return false;
+        }
+
+        true
+    }
 }
 
 /// First user settings
@@ -73,9 +85,15 @@ pub struct FirstUserConfig {
 }
 
 impl FirstUserConfig {
-    /// Whether it is a valid user.
+    /// Whether it is an empty user.
+    pub fn is_empty(&self) -> bool {
+        self.user_name.is_none()
+    }
+
     pub fn is_valid(&self) -> bool {
-        self.user_name.is_some()
+        self.user_name.as_ref().is_some_and(|n| !n.is_empty())
+            && self.full_name.as_ref().is_some_and(|n| !n.is_empty())
+            && self.password.as_ref().is_some_and(|p| !p.is_empty())
     }
 }
 
@@ -94,6 +112,12 @@ pub struct UserPassword {
     pub hashed_password: bool,
 }
 
+impl UserPassword {
+    pub fn is_empty(&self) -> bool {
+        self.password.is_empty()
+    }
+}
+
 fn overwrite_if_not_empty(old: &mut String, new: String) {
     if !new.is_empty() {
         *old = new;
@@ -119,13 +143,25 @@ pub struct RootUserConfig {
 
 impl RootUserConfig {
     pub fn is_empty(&self) -> bool {
-        self.password.is_none() && self.ssh_public_key.is_none()
+        if self
+            .password
+            .as_ref()
+            .is_some_and(|p| !p.password.is_empty())
+        {
+            return false;
+        }
+
+        if self.ssh_public_key.as_ref().is_some_and(|p| !p.is_empty()) {
+            return false;
+        }
+
+        return true;
     }
 }
 
 #[cfg(test)]
 mod test {
-    use super::{FirstUserConfig, RootUserConfig, UserPassword};
+    use super::{Config, FirstUserConfig, RootUserConfig, UserPassword};
 
     #[test]
     fn test_parse_user_password() {
@@ -139,4 +175,116 @@ mod test {
         assert_eq!(&password.password, "$a$b123");
         assert_eq!(password.hashed_password, false);
     }
+
+    #[test]
+    fn test_is_empty() {
+        assert_eq!(Config::default().is_empty(), true);
+
+        let empty_user_config = Config {
+            first_user: Some(FirstUserConfig::default()),
+            ..Default::default()
+        };
+        assert_eq!(empty_user_config.is_empty(), true);
+
+        let empty_root_config = Config {
+            root: Some(RootUserConfig::default()),
+            ..Default::default()
+        };
+        assert_eq!(empty_root_config.is_empty(), true);
+
+        let password = UserPassword {
+            password: "secret".to_string(),
+            hashed_password: false,
+        };
+        let empty_password = UserPassword {
+            password: "".to_string(),
+            hashed_password: false,
+        };
+
+        let user_with_password = FirstUserConfig {
+            user_name: Some("jane".to_string()),
+            password: Some(password.clone()),
+            ..Default::default()
+        };
+        let user_with_password_config = Config {
+            first_user: Some(user_with_password),
+            ..Default::default()
+        };
+        assert_eq!(user_with_password_config.is_empty(), false);
+
+        let root_with_password = RootUserConfig {
+            password: Some(password.clone()),
+            ..Default::default()
+        };
+        let root_with_password_config = Config {
+            root: Some(root_with_password),
+            ..Default::default()
+        };
+        assert_eq!(root_with_password_config.is_empty(), false);
+
+        let root_with_empty_password = RootUserConfig {
+            password: Some(empty_password.clone()),
+            ..Default::default()
+        };
+        let root_with_empty_password_config = Config {
+            root: Some(root_with_empty_password),
+            ..Default::default()
+        };
+        assert_eq!(root_with_empty_password_config.is_empty(), true);
+
+        let root_with_ssh_key = RootUserConfig {
+            ssh_public_key: Some("12345678".to_string()),
+            ..Default::default()
+        };
+        let root_with_ssh_key_config = Config {
+            root: Some(root_with_ssh_key),
+            ..Default::default()
+        };
+        assert_eq!(root_with_ssh_key_config.is_empty(), false);
+    }
+
+    #[test]
+    fn test_user_is_valid() {
+        assert_eq!(FirstUserConfig::default().is_valid(), false);
+
+        let valid_user = FirstUserConfig {
+            user_name: Some("firstuser".to_string()),
+            full_name: Some("First User".to_string()),
+            password: Some(UserPassword {
+                password: "12345678".to_string(),
+                hashed_password: false,
+            }),
+        };
+        assert_eq!(valid_user.is_valid(), true);
+
+        let empty_user_name = FirstUserConfig {
+            user_name: Some("".to_string()),
+            full_name: Some("First User".to_string()),
+            password: Some(UserPassword {
+                password: "12345678".to_string(),
+                hashed_password: false,
+            }),
+        };
+        assert_eq!(empty_user_name.is_valid(), false);
+
+        let empty_full_name = FirstUserConfig {
+            user_name: Some("firstuser".to_string()),
+            full_name: Some("".to_string()),
+            password: Some(UserPassword {
+                password: "12345678".to_string(),
+                hashed_password: false,
+            }),
+        };
+        assert_eq!(empty_full_name.is_valid(), false);
+
+        let empty_password = FirstUserConfig {
+            user_name: Some("firstuser".to_string()),
+            full_name: Some("First User".to_string()),
+            password: Some(UserPassword {
+                password: "".to_string(),
+                hashed_password: false,
+            }),
+        };
+        assert_eq!(empty_password.is_valid(), false);
+    }
 }
