Merge lp:~jamesbeedy/charm-helpers/add_uid_gid into lp:charm-helpers

[james beedy]

Added uid and gid specification functionality for adduser and addgroup.

Modified adduser and addgroup to include optional uid and gid
specification functionality.

Also added 3 new helper functions uid_exists, gid_exists, and group_exists.

Fixes: Bug #1586693

[Matt Bruzek (mbruzek)]

Thanks for the contribution James! Overall I think this kind of change is a good thing and I am interested to see it land. I see that the new arguments are optional so that will not break backwards compatibility with previous callers of this method.

I found a small problem with the cmd array manipulation. By using .append('--uid 1003') that creates a single array element with a space where we want 2 new elements. If you fix that, I can get behind this change.

Thanks for the work I look forward to the next iteration.

[Matt Bruzek (mbruzek)]

When I tried to 'make test' many of the existing tests fail because the positional arguments are setting the uid argument.

ERROR: tests.core.test_host.HelpersTest.test_doesnt_add_user_if_it_already_exists
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/tmp/add_uid_gid/.venv/local/lib/python2.7/site-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/tmp/add_uid_gid/tests/core/test_host.py", line 560, in test_doesnt_add_user_if_it_already_exists
    result = host.adduser(username, password)
  File "/tmp/add_uid_gid/charmhelpers/core/host.py", line 198, in adduser
    user_info = pwd.getpwuid(uid)
TypeError: an integer is required

FAIL: tests.core.test_host.HelpersTest.test_adds_a_user_if_it_doesnt_exist
AssertionError: Expected call: check_call(['useradd', '--create-home', '--shell', '/bin/bash', '--password', 'eodnho
j', '-g', 'johndoe', 'johndoe'])
Actual call: check_call(['useradd', '--uid eodnhoj', '--system', '-g', 'johndoe', 'johndoe'])

FAIL: tests.core.test_host.HelpersTest.test_adds_a_user_with_different_shell
AssertionError: Expected call: check_call(['useradd', '--create-home', '--shell', '/bin/zsh', '--password', 'eodnhoj
', 'johndoe'])
Actual call: check_call(['useradd', '--uid eodnhoj', '--system', 'johndoe'])

FAIL: tests.core.test_host.HelpersTest.test_adduser_with_groups
AssertionError: Expected call: check_call(['useradd', '--create-home', '--shell', '/bin/bash', '--password', 'eodnho
j', '-g', 'foo', '-G', 'bar,qux', 'johndoe'])
Actual call: check_call(['useradd', '--uid eodnhoj', '--system', '-g', 'foo', '-G', 'bar,qux', 'johndoe'])

The tests need to pass before this change can be accepted.

review: Needs Fixing

[lp:~jamesbeedy/charm-helpers/add_uid_gid]

581. By james beedy on 2016-05-31

cleanup

582. By James Beedy <jbeedy@virt-test0> on 2016-05-31

syntax

583. By James Beedy <jbeedy@virt-test0> on 2016-05-31

Add keyword argument for password in user tests

Modified calls to adduser to specify a keyword
arg for password.

[james beedy (jamesbeedy)]

@mbruzek Thanks for the review. The tests are now passing!
Looking at writing a few tests specific to the uid and gid functionality as well.

[Matt Bruzek (mbruzek)]

Hey James, I _just_ created a merge proposal against this branch of code that has fixes and tests passing. Please consider: https://code.launchpad.net/~mbruzek/charm-helpers/add_uid_gid/+merge/296130

Sorry it took so long, I had been interrupted several times with meetings before making this request.

[Matt Bruzek (mbruzek)]

> Hey James, I _just_ created a merge proposal against this branch of code that
> has fixes and tests passing. Please consider:
> https://code.launchpad.net/~mbruzek/charm-helpers/add_uid_gid/+merge/296132
>
> Sorry it took so long, I had been interrupted several times with meetings
> before making this request.

[lp:~jamesbeedy/charm-helpers/add_uid_gid]

584. By James Beedy <jbeedy@virt-test0> on 2016-05-31

Merge lp:~mbruzek/charm-helpers/add_uid_gid

Fix text conflict

[Matt Bruzek (mbruzek)]

There was still one lint error, but I fixed that and pushed this change to charm-helpers. Thank you for the contribution!

review: Approve

[Matt Bruzek (mbruzek)]

make tests pass and I am satisfied with the code changes.

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2016-05-17 17:41:24 +0000
+++ charmhelpers/core/host.py	2016-05-31 20:24:20 +0000
@@ -174,9 +174,8 @@
     """Return True if the host system uses systemd, False otherwise."""
     return os.path.isdir(SYSTEMD_SYSTEM)
 
-
 def adduser(username, password=None, shell='/bin/bash', system_user=False,
-            primary_group=None, secondary_groups=None):
+            primary_group=None, secondary_groups=None, uid=None):
     """Add a user to the system.
 
     Will log but otherwise succeed if the user already exists.
@@ -187,15 +186,21 @@
     :param bool system_user: Whether to create a login or system user
     :param str primary_group: Primary group for user; defaults to username
     :param list secondary_groups: Optional list of additional groups
+    :param int uid: UID for user being created
 
     :returns: The password database entry struct, as returned by `pwd.getpwnam`
     """
     try:
         user_info = pwd.getpwnam(username)
         log('user {0} already exists!'.format(username))
+        if uid:
+            user_info = pwd.getpwuid(int(uid))
+            log('user with uid {0} already exists!'.format(uid))
     except KeyError:
         log('creating user {0}'.format(username))
         cmd = ['useradd']
+        if uid:
+            cmd.extend(['--uid', str(uid)])
         if system_user or password is None:
             cmd.append('--system')
         else:
@@ -230,14 +235,58 @@
     return user_exists
 
 
-def add_group(group_name, system_group=False):
-    """Add a group to the system"""
+def uid_exists(uid):
+    """Check if a uid exists"""
+    try:
+        pwd.getpwuid(uid)
+        uid_exists = True
+    except KeyError:
+        uid_exists = False
+    return uid_exists
+
+
+def group_exists(groupname):
+    """Check if a group exists"""
+    try:
+        grp.getgrnam(groupname)
+        group_exists = True
+    except KeyError:
+        group_exists = False
+    return group_exists
+
+
+def gid_exists(gid):
+    """Check if a gid exists"""
+    try:
+        grp.getgrgid(gid)
+        gid_exists = True
+    except KeyError:
+        gid_exists = False
+    return gid_exists
+
+
+def add_group(group_name, system_group=False, gid=None):
+    """Add a group to the system
+
+    Will log but otherwise succeed if the group already exists.
+
+    :param str group_name: group to create
+    :param bool system_group: Create system group
+    :param int gid: GID for user being created
+
+    :returns: The password database entry struct, as returned by `grp.getgrnam`
+    """
     try:
         group_info = grp.getgrnam(group_name)
         log('group {0} already exists!'.format(group_name))
+        if gid:
+            group_info = grp.getgrgid(gid)
+            log('group with gid {0} already exists!'.format(gid))
     except KeyError:
         log('creating group {0}'.format(group_name))
         cmd = ['addgroup']
+        if gid:
+            cmd.extend(['--gid', str(gid)])
         if system_group:
             cmd.append('--system')
         else:

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2016-05-17 17:37:13 +0000
+++ tests/core/test_host.py	2016-05-31 20:24:20 +0000
@@ -533,7 +533,7 @@
 
         getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam]
 
-        result = host.adduser(username, password)
+        result = host.adduser(username, password=password)
 
         self.assertEqual(result, new_user_pwnam)
         check_call.assert_called_with([
@@ -557,7 +557,7 @@
 
         getpwnam.return_value = existing_user_pwnam
 
-        result = host.adduser(username, password)
+        result = host.adduser(username, password=password)
 
         self.assertEqual(result, existing_user_pwnam)
         self.assertFalse(check_call.called)
@@ -578,7 +578,7 @@
         getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam]
         getgrnam.side_effect = KeyError('group not found')
 
-        result = host.adduser(username, password, shell=shell)
+        result = host.adduser(username, password=password, shell=shell)
 
         self.assertEqual(result, new_user_pwnam)
         check_call.assert_called_with([
@@ -603,7 +603,7 @@
 
         getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam]
 
-        result = host.adduser(username, password,
+        result = host.adduser(username, password=password,
                               primary_group='foo', secondary_groups=[
                                   'bar', 'qux',
                               ])
@@ -642,6 +642,52 @@
         getpwnam.assert_called_with(username)
 
     @patch('pwd.getpwnam')
+    @patch('pwd.getpwuid')
+    @patch('grp.getgrnam')
+    @patch('subprocess.check_call')
+    @patch.object(host, 'log')
+    def test_add_user_uid(self, log, check_call, getgrnam, getpwuid, getpwnam):
+        user_name = 'james'
+        user_id = 1111
+        uid_key_error = KeyError('user not found')
+        getpwuid.side_effect = uid_key_error
+        host.adduser(user_name, uid=user_id)
+
+        check_call.assert_called_with([
+            'useradd',
+            '--uid',
+            str(user_id),
+            '--system',
+            '-g',
+            user_name,
+            user_name
+        ])
+        getpwnam.assert_called_with(user_name)
+        getpwuid.assert_called_with(user_id)
+
+    @patch('grp.getgrnam')
+    @patch('grp.getgrgid')
+    @patch('subprocess.check_call')
+    @patch.object(host, 'log')
+    def test_add_group_gid(self, log, check_call, getgrgid, getgrnam):
+        group_name = 'darkhorse'
+        group_id = 1005
+        existing_group_gid = KeyError('group not found')
+        new_group_gid = 1006
+        getgrgid.side_effect = [existing_group_gid, new_group_gid]
+
+        host.add_group(group_name, gid=group_id)
+        check_call.assert_called_with([
+            'addgroup',
+            '--gid',
+            str(group_id),
+            '--group',
+            group_name
+        ])
+        getgrgid.assert_called_with(group_id)
+        getgrnam.assert_called_with(group_name)
+
+    @patch('pwd.getpwnam')
     def test_user_exists_true(self, getpwnam):
         getpwnam.side_effect = 'pw info'
         self.assertTrue(host.user_exists('bob'))