Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User is admin by default #3719

Conversation

VladimirSlavik
Copy link
Contributor

@VladimirSlavik VladimirSlavik commented Nov 24, 2021

Previously, when entering the user spoke, the new user was not set to be admin by default. This flips the default to make the new user an admin, unless it's unchecked.

More reasoning etc. in the change linked below.

TODOs:

  • link Fedora change
  • testing - kickstart
  • testing - manual
  • review change text by team
  • link FESCO bug here
  • submit change
  • review by Mairín

Pic or didn't happen:

Screenshot_vs_main_2021-11-24_15:09:35

Previously the first checkbox was empty when entering the screen.

@VladimirSlavik VladimirSlavik added manual testing required This issue can't be merged without manual testing blocked Don't merge this pull request! f36 labels Nov 24, 2021
@VladimirSlavik VladimirSlavik force-pushed the master-default-admin-user-in-gui branch 2 times, most recently from a0cc04b to 0ba476e Compare November 24, 2021 14:51
@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype users

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Thank you!

@jstodola
Copy link
Contributor

Would it make sense to have an anaconda configuration option for that? I can imagine that some future products might not want to create users as admins by default.

@VladimirSlavik
Copy link
Contributor Author

It isn't my first choice - growing the config api means more responsibility - but I agree that is a possibility.

@jstodola
Copy link
Contributor

Yes, it sounds reasonable. An anaconda config option can be added anytime in the future if necessary.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@VladimirSlavik
Copy link
Contributor Author

@mairin do you think you could help with this?

In the emails on fedora-devel, somebody said the checkbox Make this user administrator should have a better description. Suggestions voiced there:

  • "Grant user administrator privileges (allow sudo)"
  • The explanation can be even longer: maybe "(e.g. allow sudo as root, access to all logs, and other administrative actions)"

Do you have any opinion on the checkbox wording?

@mairin
Copy link
Contributor

mairin commented Dec 6, 2021

@VladimirSlavik Just as a quick "competitive analysis" here's what OS X and Windows do -

OS X - very similar to ours, a checkbox with the text "Allow user to administer this computer"
Windows - very verbose and detailed explanation on a radio button "Administrators have complete control over the PC. They can change any settings and access all of the files and programs stored on the PC."

I don't think install time is the ideal place for a long detailed explanation. I think the main concern I read in the comments on fedora-devel was confusion over whether or not the resulting user would be root or have sudo access. There was also concern users who were less experienced with UNIX-like systems would know what 'sudo' meant. Therefore I would propose this text change:

"[x] Add administrative privileges to this user account (wheel group membership)"

I think this, in describing the account as a user account that privileges are being added to, makes it more clear that it's not making the user root (which wouldnt make sense anyway since the username isn't root) and that it's an additive property instead of swapping out the account type. If the user wants to learn more about what this means, if you search for "wheel group" on google, ddg, etc some of the obvious top hits explain it.

This also mirrors the official documentation on this point, which says:

" Select the Make this user administrator check box if the user requires administrative rights (the installation program adds the user to the wheel group ). "

@VladimirSlavik
Copy link
Contributor Author

VladimirSlavik commented Dec 7, 2021

Thank you, changed.

Screenshot_vs_main_2021-12-07_15:14:23

@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype users

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@VladimirSlavik VladimirSlavik force-pushed the master-default-admin-user-in-gui branch from 411ea39 to 648c467 Compare January 5, 2022 09:34
@VladimirSlavik VladimirSlavik removed the blocked Don't merge this pull request! label Jan 5, 2022
@VladimirSlavik VladimirSlavik marked this pull request as ready for review January 5, 2022 09:35
@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

Suggested-by: Máirín Duffy <duffy@redhat.com>

Related: rhbz#2032952
@VladimirSlavik VladimirSlavik force-pushed the master-default-admin-user-in-gui branch from 648c467 to 6bb63bc Compare January 5, 2022 09:56
@VladimirSlavik VladimirSlavik removed the manual testing required This issue can't be merged without manual testing label Jan 5, 2022
@VladimirSlavik
Copy link
Contributor Author

VladimirSlavik commented Jan 5, 2022

/kickstart-test --testtype users

edit: Only commit messages changed, previous kickstart test results are still valid. Failures of this run are due to gnome-kiosk.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

LGTM!

@VladimirSlavik VladimirSlavik merged commit 63edc86 into rhinstaller:master Jan 10, 2022
@VladimirSlavik VladimirSlavik deleted the master-default-admin-user-in-gui branch January 10, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 participants