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

system user module, group append=yes #3062

Closed
trbs opened this issue May 30, 2013 · 9 comments
Closed

system user module, group append=yes #3062

trbs opened this issue May 30, 2013 · 9 comments

Comments

@trbs
Copy link
Contributor

trbs commented May 30, 2013

Ansible version: 0c03a5c

With the following action ansible will always make a change even if the user already belongs to the group:

- name: make sure foo is an admin
  action: user name=foo groups=admin append=yes

The relevant pieces of code in the user module are as follows:

    320         if self.groups is not None:
    321             current_groups = self.user_group_membership()
    322             groups_need_mod = False

and

    394     def user_group_membership(self):
    395         groups = []
    396         info = self.get_pwd_info()
    397         for group in grp.getgrall():
    398             if self.name in group.gr_mem and info[3] == group.gr_gid:
    399                 groups.append(group[0])
    400         return groups

The result of the user_group_membership function is always an empty list for me.

Because the check within the for loop will exclude all the groups with do not match the primary group. (info[3]==group.gr_gid)

I would think that the if statement should either read:

if self.name in group.gr_mem and not info[3] == group.gr_gid:

or

if self.name in group.gr_mem or info[3] == group.gr_gid:

To return all the groups the user belongs to including or excluding it's primary group.

With either of the change the task runs as expected.

@ScottSturdivant
Copy link
Contributor

Uh oh, looks like there's different behavior on different OSes here.

Michael merged in my change here, because on FreeBSD the previous user code was not indempotent: #3001

Putting in your proposed fix, I'm back to no longer being indempotent.

Out of curiousity, what's the result you get for list(pwd.getpwnam(username))? On my system, I get back this:

['server', '*', 1010, 1010, 'Server', '/usr/home/server', '/bin/tcsh']

@ScottSturdivant
Copy link
Contributor

Adding on here... the "and not" change didn't keep indempotency for me, but changing to "or" does. Which then makes me wonder, what's even the point of the second part of that clause? Isn't checking if the user name appears in the group membership list sufficient enough?

@mpdehaan
Copy link
Contributor

Can you please clarify this some more? Idempotency is a silly word and I'd like to talk about actual behavior you are seeing. Idempotency is about running commands when commands should not be run. This sounds like it's something more than that.

(A) I start in this condition with user belong to ____ groups. I am managing ___ OS.

(B) I run this task: ________

(C) I expect to have groups set to ______ but instead the result is they are set to _______

OR

It works for me and it is just reporting the wrong changed state about whether it changed anything.

etc

@trbs
Copy link
Contributor Author

trbs commented May 30, 2013

mpdehaan:

OS: Debian 7

I start out with user 'foo' having a primary group 'foo' and nothing else.
Then I execute the task:
action: user name=foo groups=admin append=yes

Ansible adds user foo to the admin group and says:

TASK: [make sure foo is an admin] ***************************************** 
 changed: [10.0.0.161]

When I run the play again, foo is now a member of admin and it's primairy group foo.

TASK: [make sure foo is an admin] ***************************************** 
 changed: [10.0.0.161]

It also still execute the command ['/usr/sbin/usermod', '-a', '-G', 'admin', 'foo'], however the tools is smart enough not to add it twice to /etc/group.

@mpdehaan
Copy link
Contributor

Thanks that helps clarify things, appreciate it.

@trbs
Copy link
Contributor Author

trbs commented May 30, 2013

Out of curiousity, what's the result you get for list(pwd.getpwnam(username))? On my system, I get back this:

['server', '*', 1010, 1010, 'Server', '/usr/home/server', '/bin/tcsh']

['foo', 'x', 1000, 1000, 'foo,,,', '/home/foo', '/bin/bash']

That looks pretty similar to me :)

I see that in your example you use 'groups' instead of 'group'.

  tasks:
    - name: Create server group
      group: name=server gid=1010
    - name: Create server user
      user: name=server uid=1010 comment="Server" home=/usr/home/server groups=server shell=/bin/tcsh

This does not work for me at all since useradd on debian will try to create a new group 'server' which has already been created in the previous step. Changing groups=server to group=server yield the desired outcome.

Could this be a problem in your playbook ?

As I see it your task is trying to add the primary group of the user to the additional groups. (At least this is how it works on Linux)

This should be prevented, currently Linux usermod utilities is ignore the command but it will still try on the next run.

I see two options here:

  1. We will raise an error when the users primary group is one of the items in the groups parameter. (My personal favorite)

  2. We will automatically filter out the users primary group out of the groups parameter. (This has some drawbacks as we now have 3 states to deal with: groups=[1,2,3], groups= and groups= because the only value was the users primary group)

@trbs
Copy link
Contributor Author

trbs commented May 30, 2013

See pull request #3072

@trbs
Copy link
Contributor Author

trbs commented May 30, 2013

The patch is rather large, sorry about that.

I was not convinced by the method name get_groups_set but could not think of something else at the moment.

@ScottSturdivant
Copy link
Contributor

Oh, I see. Originally I had a list of groups and in the spirit of shrinking it down to the simplest test case, wound up writing the bug with only a single item for the groups argument. That said, the documentation and the code aren't well documented in that regard. get_group_membership() apparently is supposed to return all of the groups a user belongs to...except for the primary group. IMO it should mention that in a docstring or comment.

Anyhow, sorry for screwing things up originally!

mpdehaan pushed a commit that referenced this issue May 31, 2013
Also consolidated duplicate groups code into one get_groups_set() method.
Removed unused call to user_group_membership.
Removed sorting operations on set functions cause sets are inherently unordered.
Minor style improvements to match the rest of the code.

The new function will make the order of group names passed to the system command less determistic.
Which was already the case for modify_user_usermod() but not for other methods.
It will also strip out duplicate group names automatically which was not always the case previously.
@ansible ansible locked and limited conversation to collaborators Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants