Merge lp:~johnsca/charm-helpers/adduser-group into lp:charm-helpers

[Cory Johns]

Add primary and secondary groups params to host.adduser

This was a change that the big data charms use but was never proposed upstream.

[Stuart Bishop (stub)]

Can we have better parameter names than 'group' and 'groups'? eg. primary_group and secondary_groups? The current naming will certainly cause confusion, particularly when the docstring contains no hints for the new parameters.

Some tests would be nice but this is a simple enough update.

[lp:~johnsca/charm-helpers/adduser-group]

492. By Cory Johns on 2015-11-27

Changed param names and added test

493. By Cory Johns on 2015-11-27

Add doc string and explicit -g default to avoid "group exists" error

[Cory Johns (johnsca)]

Thanks for the feedback. I added a test and docstring, changed the param names as suggested, and made it always include the default primary group to avoid errors like this: http://pastebin.ubuntu.com/13506551/

[lp:~johnsca/charm-helpers/adduser-group]

494. By Cory Johns on 2015-11-27

Improved default primary_group handling to avoid errors

[Cory Johns (johnsca)]

"Always include" wasn't quite right, either. Now, creating a user without specifying a primary group will always succeed, regardless of whether the user group already exists or not.

[Stuart Bishop (stub)]

All good. Thanks!

review: Approve

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2015-11-18 09:26:07 +0000
+++ charmhelpers/core/host.py	2015-11-27 20:57:09 +0000
@@ -146,8 +146,22 @@
         return True
 
 
-def adduser(username, password=None, shell='/bin/bash', system_user=False):
-    """Add a user to the system"""
+def adduser(username, password=None, shell='/bin/bash', system_user=False,
+            primary_group=None, secondary_groups=None):
+    """
+    Add a user to the system.
+
+    Will log but otherwise succeed if the user already exists.
+
+    :param str username: Username to create
+    :param str password: Password for user; if ``None``, create a system user
+    :param str shell: The default shell for the user
+    :param bool system_user: Whether to create a login or system user
+    :param str primary_group: Primary group for user; defaults to their username
+    :param list secondary_groups: Optional list of additional groups
+
+    :returns: The password database entry struct, as returned by `pwd.getpwnam`
+    """
     try:
         user_info = pwd.getpwnam(username)
         log('user {0} already exists!'.format(username))
@@ -162,6 +176,16 @@
                 '--shell', shell,
                 '--password', password,
             ])
+        if not primary_group:
+            try:
+                grp.getgrnam(username)
+                primary_group = username  # avoid "group exists" error
+            except KeyError:
+                pass
+        if primary_group:
+            cmd.extend(['-g', primary_group])
+        if secondary_groups:
+            cmd.extend(['-G', ','.join(secondary_groups)])
         cmd.append(username)
         subprocess.check_call(cmd)
         user_info = pwd.getpwnam(username)

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2015-11-17 20:21:31 +0000
+++ tests/core/test_host.py	2015-11-27 20:57:09 +0000
@@ -395,10 +395,11 @@
         check_output.side_effect = exc
         self.assertFalse(host.service_running('foo'))
 
+    @patch('grp.getgrnam')
     @patch('pwd.getpwnam')
     @patch('subprocess.check_call')
     @patch.object(host, 'log')
-    def test_adds_a_user_if_it_doesnt_exist(self, log, check_call, getpwnam):
+    def test_adds_a_user_if_it_doesnt_exist(self, log, check_call, getpwnam, getgrnam):
         username = 'johndoe'
         password = 'eodnhoj'
         shell = '/bin/bash'
@@ -415,6 +416,7 @@
             '--create-home',
             '--shell', shell,
             '--password', password,
+            '-g', username,
             username
         ])
         getpwnam.assert_called_with(username)
@@ -436,10 +438,12 @@
         self.assertFalse(check_call.called)
         getpwnam.assert_called_with(username)
 
+    @patch('grp.getgrnam')
     @patch('pwd.getpwnam')
     @patch('subprocess.check_call')
     @patch.object(host, 'log')
-    def test_adds_a_user_with_different_shell(self, log, check_call, getpwnam):
+    def test_adds_a_user_with_different_shell(self, log, check_call, getpwnam,
+                                              getgrnam):
         username = 'johndoe'
         password = 'eodnhoj'
         shell = '/bin/zsh'
@@ -447,6 +451,7 @@
         new_user_pwnam = 'some user pwnam'
 
         getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam]
+        getgrnam.side_effect = KeyError('group not found')
 
         result = host.adduser(username, password, shell=shell)
 
@@ -460,6 +465,37 @@
         ])
         getpwnam.assert_called_with(username)
 
+    @patch('grp.getgrnam')
+    @patch('pwd.getpwnam')
+    @patch('subprocess.check_call')
+    @patch.object(host, 'log')
+    def test_adduser_with_groups(self, log, check_call, getpwnam, getgrnam):
+        username = 'johndoe'
+        password = 'eodnhoj'
+        shell = '/bin/bash'
+        existing_user_pwnam = KeyError('user not found')
+        new_user_pwnam = 'some user pwnam'
+
+        getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam]
+
+        result = host.adduser(username, password,
+                              primary_group='foo', secondary_groups=[
+                                  'bar', 'qux',
+                              ])
+
+        self.assertEqual(result, new_user_pwnam)
+        check_call.assert_called_with([
+            'useradd',
+            '--create-home',
+            '--shell', shell,
+            '--password', password,
+            '-g', 'foo',
+            '-G', 'bar,qux',
+            username
+        ])
+        getpwnam.assert_called_with(username)
+        assert not getgrnam.called
+
     @patch('pwd.getpwnam')
     @patch('subprocess.check_call')
     @patch.object(host, 'log')