Skip to content

Conversation

@imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Jun 10, 2025

Problem

There are a few problems with the user and root settings that "agama config show" generates.

  1. It is not possible to get the root password (it is not exposed). See bsc#1235602.
  2. The user section is exported even if a first user does not exist.
  3. The root section is exported even if no authentication mechanism has been defined.
  4. The hashed_password key is exported even if the root only has an SSH public key.

For instance, here's the output for a just started Agama instance:

{
  "root": {},
  "user": {
    "fullName": "",
    "userName": "",
    "password": "",
    "hashedPassword": false
  }
}

Solution

  • Move the password settings to a single struct (UserPassword). The password and the hashedPassword keys are always exported together.
  • Do not export the user or root sections unless it is needed.

I decided to not make the changes at HTTP level because it would imply adapting the web client too and we have more urgent things to work on.

Testing

  • Updated the unit test
  • Tested manually

@imobachgs imobachgs changed the title Export password feat(rust): improve user/root exported information Jun 10, 2025
@coveralls
Copy link

coveralls commented Jun 10, 2025

Coverage Status

coverage: 62.649%. remained the same
when pulling b3b71d0 on export-password
into c19a5b9 on master.


Some(FirstUserSettings {
user_name: Some(first_user.user_name),
full_name: Some(first_user.full_name),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means we still export even empty full_name. Is it correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the full_name is not mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so in exported profile will be full_name: "" which looks wrong to me.

.password
.as_ref()
.map(|p| p.hashed_password)
.unwrap_or_default(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when I see how tricky it is...maybe it belongs more to FirstUser or FirstUserSettings and having trait like Into?

Copy link
Contributor Author

@imobachgs imobachgs Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can change that. At first sight, I preferred to keep this logic just here because it is not needed anywhere else and, for consistency, I would have to implement other conversions too (From<&FirstUser> for FirstUserSettings, From<&RootUser> for RootUserSettings and From<&RootUserSettings> for RootUser).

Copy link
Contributor

@jreidinger jreidinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general approach looks good and it is better then before. Please after merge close mine #2238

@imobachgs imobachgs requested a review from jreidinger June 10, 2025 22:14
@imobachgs imobachgs merged commit d02c3bb into master Jun 11, 2025
21 checks passed
@imobachgs imobachgs deleted the export-password branch June 11, 2025 08:15
@imobachgs imobachgs mentioned this pull request Jun 30, 2025
imobachgs added a commit that referenced this pull request Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants