Support specifying (extra) groups for user in identity section#2265
Support specifying (extra) groups for user in identity section#2265ogayot wants to merge 3 commits intocanonical:mainfrom
Conversation
ogayot
changed the title
ogayot
force-pushed
the
extra-groups
branch
3 times, most recently
from
6f8cfbb to
34fdebe
Compare
Chris-Peterson444
left a comment
Chris-Peterson444
left a comment
There was a problem hiding this comment.
Quick first pass
| @@ -72,13 +83,8 @@ def __init__(self, app): | |||
|
|
|||
| def load_autoinstall_data(self, data): | |||
There was a problem hiding this comment.
suggestion: the documentation states that append and override are mutually exclusive, I think we should assert that here with an AutoinstallValidationError here.
There was a problem hiding this comment.
Thanks! I couldn't find a clean way to do that with the JSON schema so I did it manually in Python.
Chris-Peterson444
left a comment
Chris-Peterson444
left a comment
There was a problem hiding this comment.
Full review this time. A couple of comments but otherwise looks pretty good to me, thanks Olivier.
| raise ValueError | ||
| return cls( | ||
| username=data["username"], | ||
| realname=data.get("realname", ""), |
There was a problem hiding this comment.
question: if we don't pass a realname in the identity flow then it will take the value of the user name. Here we set it to empty. Is the behavior difference intentional?
There was a problem hiding this comment.
Oh I see this is the original behavior, but maybe more clear now they are closer together. I'll leave the question up so we can consider it but not blocking imo.
There was a problem hiding this comment.
I think this realistically needs to be fixed in a follow on PR! Thanks for pointing it out!
| self.model.add_user(data) | ||
| self.model.user = User.from_identity_data(data) | ||
| self.model.hostname = data.hostname |
There was a problem hiding this comment.
question: why not use add_user here? I see we have a new implementation of add_user that does exactly this.
There was a problem hiding this comment.
TBH I'd like to deprecate add_user.
First, because it doesn't "add" a user but configures the user (there can only be one).
Second because we're passing it the hostname, which has nothing to do with the user.
dbungert
left a comment
dbungert
left a comment
There was a problem hiding this comment.
This is sensible, thanks for this. Please rebase before merge. Sorry for the delayed review!
|
Will rebase. We discussed with Chris and we think we should clarify that useradd would still create a group named after the user in any scenario (unless we pass the --no-user-group option but we don't) |
ogayot
force-pushed
the
extra-groups
branch
2 times, most recently
from
5444128 to
b77d022
Compare
|
FFE filed for resolute: https://bugs.launchpad.net/ubuntu/+source/subiquity/+bug/2143141 |
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
In the identity section, one can now specify the groups directive.
By default, the user is added to:
* the sudo group (hardcoded in Subiquity)
* the admin group (hardcoded in Subiquity)
* a group named after the user (automatically created by useradd(8))
The groups directive can be used to either:
* get the user added to a list of supplementary groups,
* get the user added to a list of groups, ignoring groups that are
hardcoded in Subiquity.
To ignore hardcoded groups, use:
# Two equivalent syntaxes for specifying the list of groups the user should
# belong to:
groups: [sudo, admin, lpadmin]
groups: {override: [sudo, admin, lpadmin]}
And for supplementary groups:
# Add the user to the wireshark group in addition to the default
groups: {append: [wireshark]}
Note that in any case, the user will be added to a group named after them.
This group is automatically created by useradd.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
3 participants
In the identity section, one can now specify the groups directive. The groups directive can be used to either:
To discard the defaults:
For supplementary groups: