do not export empty defaults for user #2238
Conversation
rust/agama-lib/src/users/settings.rs
Outdated
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub password: Option<String>, | ||
| /// Whether the password is hashed or is plain text | ||
| #[serde(skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
here is one tricky point as it does not work together with https://github.com/agama-project/agama/pull/2238/files#diff-7680a69ef487f58ca65b770c64fb4205ea33f19d9081eb44d5b0bed8d8e8e14cR58
If we want never return false value here we can have own method for skipping. Just please tell me if you agree to never export hash_password: false
imobachgs
left a comment
imobachgs
left a comment
There was a problem hiding this comment.
Is the user key still exported?
rust/agama-lib/src/users/store.rs
Outdated
| } | ||
|
|
||
| // small helper to convert dbus empty string to None | ||
| fn optional_string(value: String) -> Option<String> { |
There was a problem hiding this comment.
I would put this function in https://github.com/agama-project/agama/blob/master/rust/agama-lib/src/dbus.rs
rust/agama-lib/src/users/settings.rs
Outdated
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub password: Option<String>, | ||
| /// Whether the password is hashed or is plain text | ||
| #[serde(skip_serializing_if = "Option::is_none")] |
|
|
||
| impl RootUserSettings { | ||
| pub fn skip_export(&self) -> bool { | ||
| self.password.is_none() && self.hashed_password.is_none() && self.ssh_public_key.is_none() |
There was a problem hiding this comment.
@imobachgs sadly here we have identical problem to first user with hashed password...but here it is even more tricky as password and hashed_password are always skipped during deserializing, so unless ssh_public_key is set, then it is always empty....but on other hand we want somehow indicate user that they need to set it in root user.
So I am not sure how to proceed here. Of course I can change it to something like self.hashed_password == Some(false)
There was a problem hiding this comment.
btw if hashed_password is set to true, should we really omit serialization of password to profile?
| // sadly for boolean we do not have on dbus way to set it to not_set | ||
| hashed_password: Some(first_user.hashed_password), | ||
| }; | ||
| let first_user = if first_user.skip_export() { |
There was a problem hiding this comment.
Perhaps we could extend the D-Bus API with information about whether the user or the root was set instead of checking many conditions here.
|
Please, excuse me for the noise since this comment is not strictly related to the changes proposed here, but they somehow reminded me a concern about optional fields I found when reworking authentication forms in the web interface: an empty string has to be sent for unset a value, see agama/web/src/components/users/RootUserForm.tsx Lines 120 to 122 in a4e4169 Just in case this rings a bell now that you're touching the users code. It might be worth creating a card with the details to address this behavior in the future. Sending empty values to represent 'no value' feels a bit awkward. |
|
@dgdavid yeap, it basically exposing dbus API which use empty string as unset. So it can be also addressed, I am just not sure if it is better to send patch, delete or something else ( I think json allows even sending null ). |
|
Replaced with #2462. |
Problem
When user do not configure first user and try to export profile or do
agama config editand do not touch user section it failed to load with invalid user that cannot be empty.Solution
Skip exporting user if it is not defined.
Testing