Add usergroup support to user.present

[boltronics]

What does this PR do?

Replaces the seemingly redundant gid_from_name argument from user.present and replaces it with a usergroup argument that has the effect of adding the --user-group argument to the useradd command on GNU/Linux hosts.

I whitelisted it to GNU/Linux for now since I don't know which other operating systems support the -U/--user-group argument.

I tested it on an OpenSUSE 42.3 and Debian Stretch by creating a basic state sls:

test:
  user.present:
    - usergroup: True

test2:
  user.present:
    - usergroup: False

On both hosts the intended behaviour was observed.

What issues does this PR fix or reference?

#48640

Previous Behavior

gid_from_name has been removed. AFAICT this command no longer has any practical benefit - please correct me if I'm overlooking something. The behaviour it intended to provide can be simulated just as easily by:

test:
  user.present:
    - gid: test

(where we want a test group to match a test user). This is much easier to read and understand. It's also clearer that the group will not be automatically created in this case.

Regardless of the gid_from_name option, the group will not be created since 2017.7.5 due to the regression referenced. This PR does not revert that regression (which is actually a pain to deal with on a stable release and IMO really should be fixed), but I believe this is a better approach for the long-term.

New Behavior

We now mimic the existing --user-group option behaviour of the useradd command which I believe is closer to user expectations - even for users of distributions such as SuSE that don't typically use this.

If usergroup: True is set in the user state sls file, the -U argument will be passed to the useradd command causing the group with the same name as the user name to be created. If usergroup: False is set, the -N/--no-user-group argument is added instead. If left undefined, the defaults that the distribution or user typically defines in /etc/default/useradd and /etc/login.defs are used as usual.

Tests written?

No. There are no existing tests for gid_from_name either. I'm not sure what the best way to do this would be, but I suspect it would involve having the test inspect the constructed cmd list in the useradd.add function to check for the presence of -U instead, as opposed to just inspecting a return value of a mocked command.

Commits signed with GPG?

Yes

[rares-pop]

the default for 'usergroup' is None. Should you compare this to None too? I'm thinking to change this to: if not usergroup:

[boltronics]

There are three possibilities - we explicitly set usergroup to False to trigger -N to require no user group be created, we set it to True to trigger -U to require a user group to be created, or don't touch it at all which will go with the OS defaults - which differs depending on the distribution or user preferences.

If you instead go with if not usergroup, you'll be force overriding the OS defaults (or overriding whatever the user configured the OS to do), so I imagine most people won't be too happy about that.

[rares-pop]

Ah, okay. So it's intended to use the OS defaults unless specified.

[cachedout]

Shouldn't we then document this so that a user knows when to set this to False and what behavior that will produce?

[boltronics]

@rares-pop Correct. I don't see any reason to change things the user didn't request. @cachedout Yep, fair call. Will do.

[boltronics]

That has now been updated. I also rebased onto the latest develop branch.

[rallytime]

@boltronics Can you fix these lint errors?

https://jenkinsci.saltstack.com/job/pr-lint/job/PR-48704/4/warnings52Result/

[boltronics]

@rallytime Hopefully the lint error is sorted now. Thanks for pointing that out.

[rallytime]

@boltronics Sure thing. The lint check is happy now, but now there are some related test failures with the user states and the git pillar tests: https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py3/job/PR-48704/5/#showFailuresLink

Can you take a look?

[boltronics]

No worries, can do.

[boltronics]

Sorry for the hold-up. I've been busy, but will try to get to this this week.

[cachedout]

Hi @boltronics. Just checking in with you. :)

[boltronics]

Sorry for the delay. I haven't forgotten, just taking care of higher priorities first. I hope to get to this soon.

[cachedout]

@boltronics Many thanks!

[cachedout]

Hi @boltronics. Just checking back in with you. :) Any updates here?

[boltronics]

Thanks @cachedout. I'm actually trying to look into this now (sorry again for the wait), but can't even push to my repo due to some GitHub issues it seems. My commits just keep disappearing...

[boltronics]

Thanks @cachedout. I'm actually trying to look into this now (sorry again for the wait), but can't even push to my repo due to some GitHub issues it seems. My commits just keep disappearing...

[boltronics]

Hi @cachedout. Hopefully GitHub is working this time... I think I'm all good for fixing the tests (or I will be when GH is fixed so my pushed code doesn't go to /dev/null). The only one I'm concerned about is https://github.com/saltstack/salt/blob/develop/tests/integration/states/test_user.py#L131 test_user_present_gid_from_name_default as that doesn't make sense to me. Say I rename it to test_user_present_usergroup_default... it would only exist to test undefined behaviour. ie.

        if grains['os_family'] in ('Suse',):                                                                                                                                                       
            self.assertEqual(group_name, 'users')                                                                                                                                                    
        elif grains['os_family'] == 'MacOS':                                                                                                                                                         
            self.assertEqual(group_name, 'staff') 

Of course, Apple, SuSE, etc. could change their mind about their default behaviour at any point which would break the test.

If we set usergroup to True or False we can immediately see what we should expect, but if we set it to None (or just leave it unset) it'll do whatever the OS will do by default. We can try to guess what might happen, or we can try to get a better idea by parsing /etc/login.defs, etc. eg.

login_defs_path = '/etc/login.defs'                                   
# It looks like FreeBSD creates a usergroup by default. I'm           
# working on the assumption that this is usually true outside         
# of MacOS and Linux.                                                 
usergroup = True                                                      
# MacOS users' primary group defaults to staff (20), not the          
# name of user                                                        
if grains['os_family'] == 'MacOS':                                    
    usergroup = False                                                 
elif grains['kernel'] == 'Linux':                                     
    # It looks like login usually defaults to "no".                   
    try:                                                              
        with salt.utils.files.fopen(login_defs_path, 'r') as lfh:     
            for line in lfh:                                          
                if line.startswith('USERGROUPS_ENAB'):                
                    usergroup = line.strip().endswith('yes')          
                break                                                 
    except IOError:                                                   
        usergroup = False                                             

but I'm not sure I see a useful test here. We're only validating that we guessed correctly what the OS might do when the user didn't request anything specific, as opposed to testing the usergroup functionality worked as intended. We already have what will be a test_user_present_usergroup function, so I propose replacing test_user_present_gid_from_name_default with test_user_present_usergroup_disabled instead.

Please let me know your thoughts. In the meantime, I'll proceed with this proposal for this PR.

[boltronics]

Hi @cachedout, I'm trying to update my PR, but can't because GitHub is completely broken right now. Unfortunately posting messages directly to the PR is also broken, so I'm attempting one last time, this time via e-mail. This is my repo: https://github.com/boltronics/salt/commits/develop-user_present-usergroup Please let the tests run on it when you can. I think it fixes the issues raised. Thanks.

[boltronics]

I have just rebased against the current develop HEAD, and looks like Jenkins triggered successfully now.

...and it also looks like all the comments came through from yesterday that reportedly failed to submit. 😄

[cachedout]

@boltronics Sorry for the delay here. I think we're about ready to go with this. If you can resolve the merge conflict that snuck in then we'll get this merged.

[boltronics]

@cachedout Done.

[boltronics]

I also just added the docstring commit since that's dealing with the same function.

[thatch45]

Thanks @boltronics ! This looks great to me. And thanks for following the PR for so long!