Merge ~smoser/cloud-init:bug/1354694-whitespace-in-group-string into cloud-init:master

[Scott Moser]

[Scott Moser (smoser)]

this needs a test added.

review: Needs Fixing

[Scott Moser (smoser)]

Would like to test that this config works:
#cloud-config
users:
  - default
  - name: foobar
    gecos: "My User"
    groups: sudo, adm
  - name: wark
    groups: [sudo, adm]
  - name: wark2
    groups: [sudo, "adm "]
  - name: allnew
    groups: group1, group2, group3

It is probably sufficient to test the distros add_user specifically.

[Scott Moser (smoser)]

has tests now.

review: Resubmit

[Scott Moser (smoser)]

In addition to the unit tests, I've tested the user-data below and verified expected groups and users are added and now WARN in cloud-init.log.

#cloud-config
users:
  - default
  - name: foobar
    gecos: "My User"
    groups: sudo, adm
  - name: wark
    groups: [sudo, adm]
  - name: wark2
    groups: [sudo, "adm "]
  - name: allnew
    groups: group1, group2, group3

diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index 1f73195..94f31f8 100755
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -398,12 +398,16 @@ class Distro(object):
         # support kwargs having groups=[list] or groups="g1,g2"
         groups = kwargs.get('groups')
         if groups:
-            if isinstance(groups, (list, tuple)):
-                # kwargs.items loop below wants a comma delimeted string
-                # that can go right through to the command.
-                kwargs['groups'] = ",".join(groups)
-            else:
-                groups = [group.strip() for group in groups.split(",")]
+            if isinstance(groups, six.string_types):
+                groups = groups.split(",")
+
+            # remove any white spaces in group names, most likely
+            # that came in as a string like: groups: group1, group2
+            groups = [g.strip() for g in groups]
+
+            # kwargs.items loop below wants a comma delimeted string
+            # that can go right through to the command.
+            kwargs['groups'] = ",".join(groups)
 
             primary_group = kwargs.get('primary_group')
             if primary_group:
@@ -413,10 +417,10 @@ class Distro(object):
             for group in groups:
                 if not util.is_group(group):
                     self.create_group(group)
-                    LOG.debug("created group %s for user %s", name, group)
+                    LOG.debug("created group '%s' for user '%s'", group, name)
 
         # Check the values and create the command
-        for key, val in kwargs.items():
+        for key, val in sorted(kwargs.items()):
 
             if key in adduser_opts and val and isinstance(val, str):
                 adduser_cmd.extend([adduser_opts[key], val])
@@ -433,7 +437,7 @@ class Distro(object):
 
         # Don't create the home directory if directed so or if the user is a
         # system user
-        if 'no_create_home' in kwargs or 'system' in kwargs:
+        if kwargs.get('no_create_home') or kwargs.get('system'):
             adduser_cmd.append('-M')
             log_adduser_cmd.append('-M')
         else:
diff --git a/tests/unittests/test_distros/test_create_users.py b/tests/unittests/test_distros/test_create_users.py
new file mode 100644
index 0000000..01a49f4
--- /dev/null
+++ b/tests/unittests/test_distros/test_create_users.py
@@ -0,0 +1,147 @@
+from cloudinit import distros
+from ..helpers import (TestCase, mock)
+
+
+class MyBaseDistro(distros.Distro):
+    # MyBaseDistro is here to test base Distro class implementations
+
+    def __init__(self, name="basedistro", cfg={}, paths={}):
+        super(MyBaseDistro, self).__init__(name, cfg, paths)
+
+    def install_packages(self, pkglist):
+        raise NotImplementedError()
+
+    def _write_network(self, settings):
+        raise NotImplementedError()
+
+    def package_command(self, cmd, args=None, pkgs=None):
+        raise NotImplementedError()
+
+    def update_package_sources(self):
+        raise NotImplementedError()
+
+    def apply_locale(self, locale, out_fn=None):
+        raise NotImplementedError()
+
+    def set_timezone(self, tz):
+        raise NotImplementedError()
+
+    def _read_hostname(self, filename, default=None):
+        raise NotImplementedError()
+
+    def _write_hostname(self, hostname, filename):
+        raise NotImplementedError()
+
+    def _read_system_hostname(self):
+        raise NotImplementedError()
+
+
+class TestCreateUser(TestCase):
+    def setUp(self):
+        super(TestCase, self).setUp()
+        self.dist = MyBaseDistro()
+
+    def _useradd2call(self, args):
+        # return a mock call for the useradd command in args
+        # with expected 'logstring'.
+        args = ['useradd'] + args
+        logcmd = [a for a in args]
+        for i in range(len(args)):
+            if args[i] in ('--password',):
+                logcmd[i + 1] = 'REDACTED'
+        return mock.call(args, logstring=logcmd)
+
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_basic(self, m_subp):
+        user = 'foouser'
+        self.dist.create_user(user)
+        self.assertEqual(
+            m_subp.call_args_list,
+            [self._useradd2call([user, '-m']),
+             mock.call(['passwd', '-l', user])])
+
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_no_home(self, m_subp):
+        user = 'foouser'
+        self.dist.create_user(user, no_create_home=True)
+        self.assertEqual(
+            m_subp.call_args_list,
+            [self._useradd2call([user, '-M']),
+             mock.call(['passwd', '-l', user])])
+
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_system_user(self, m_subp):
+        # system user should have no home and get --system
+        user = 'foouser'
+        self.dist.create_user(user, system=True)
+        self.assertEqual(
+            m_subp.call_args_list,
+            [self._useradd2call([user, '--system', '-M']),
+             mock.call(['passwd', '-l', user])])
+
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_explicit_no_home_false(self, m_subp):
+        user = 'foouser'
+        self.dist.create_user(user, no_create_home=False)
+        self.assertEqual(
+            m_subp.call_args_list,
+            [self._useradd2call([user, '-m']),
+             mock.call(['passwd', '-l', user])])
+
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_unlocked(self, m_subp):
+        user = 'foouser'
+        self.dist.create_user(user, lock_passwd=False)
+        self.assertEqual(
+            m_subp.call_args_list,
+            [self._useradd2call([user, '-m'])])
+
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_set_password(self, m_subp):
+        user = 'foouser'
+        password = 'passfoo'
+        self.dist.create_user(user, passwd=password)
+        self.assertEqual(
+            m_subp.call_args_list,
+            [self._useradd2call([user, '--password', password, '-m']),
+             mock.call(['passwd', '-l', user])])
+
+    @mock.patch("cloudinit.distros.util.is_group")
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_group_added(self, m_subp, m_is_group):
+        m_is_group.return_value = False
+        user = 'foouser'
+        self.dist.create_user(user, groups=['group1'])
+        expected = [
+            mock.call(['groupadd', 'group1']),
+            self._useradd2call([user, '--groups', 'group1', '-m']),
+            mock.call(['passwd', '-l', user])]
+        self.assertEqual(m_subp.call_args_list, expected)
+
+    @mock.patch("cloudinit.distros.util.is_group")
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_only_new_group_added(self, m_subp, m_is_group):
+        ex_groups = ['existing_group']
+        groups = ['group1', ex_groups[0]]
+        m_is_group.side_effect = lambda m: m in ex_groups
+        user = 'foouser'
+        self.dist.create_user(user, groups=groups)
+        expected = [
+            mock.call(['groupadd', 'group1']),
+            self._useradd2call([user, '--groups', ','.join(groups), '-m']),
+            mock.call(['passwd', '-l', user])]
+        self.assertEqual(m_subp.call_args_list, expected)
+
+    @mock.patch("cloudinit.distros.util.is_group")
+    @mock.patch("cloudinit.distros.util.subp")
+    def test_create_groups_with_whitespace_string(self, m_subp, m_is_group):
+        # groups supported as a comma delimeted string even with white space
+        m_is_group.return_value = False
+        user = 'foouser'
+        self.dist.create_user(user, groups='group1, group2')
+        expected = [
+            mock.call(['groupadd', 'group1']),
+            mock.call(['groupadd', 'group2']),
+            self._useradd2call([user, '--groups', 'group1,group2', '-m']),
+            mock.call(['passwd', '-l', user])]
+        self.assertEqual(m_subp.call_args_list, expected)