Merge branch 'development'20181023

6 files changed, 77 insertions, 32 deletions

diff --git a/debian/changelog b/debian/changelog
index 4885c9c..d90876e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+google-compute-image-packages (2.8.8-1) stable; urgency=low
+
+  * Update sudoer group membership without overriding local groups.
+
+ -- Google Cloud Team <gc-team@google.com>  Tue, 23 Oct 2018 12:00:00 -0700
+
 google-compute-image-packages (2.8.7-1) stable; urgency=low
 
   * Remove users from sudoers group on removal (fixed).
diff --git a/google_compute_engine/accounts/accounts_utils.py b/google_compute_engine/accounts/accounts_utils.py
index ddb34c0..8c0741d 100644
--- a/google_compute_engine/accounts/accounts_utils.py
+++ b/google_compute_engine/accounts/accounts_utils.py
@@ -27,7 +27,7 @@ from google_compute_engine import constants
 from google_compute_engine import file_utils
 
 USER_REGEX = re.compile(r'\A[A-Za-z0-9._][A-Za-z0-9._-]{0,31}\Z')
-DEFAULT_GPASSWD_CMD = 'gpasswd -d {user} {group}'
+DEFAULT_GPASSWD_CMD = 'gpasswd {option} {user} {group}'
 DEFAULT_GROUPADD_CMD = 'groupadd {group}'
 DEFAULT_USERADD_CMD = 'useradd -m -s /bin/bash -p * {user}'
 DEFAULT_USERDEL_CMD = 'userdel -r {user}'
@@ -48,7 +48,7 @@ class AccountsUtils(object):
       logger: logger object, used to write to SysLog and serial port.
       groups: string, a comma separated list of groups.
       remove: bool, True if deprovisioning a user should be destructive.
-      gpasswd_cmd: string, command to remove a user from a group.
+      gpasswd_cmd: string, command to add or remove a user from a group.
       groupadd_cmd: string, command to add a new group.
       useradd_cmd: string, command to create a new user.
       userdel_cmd: string, command to delete a user.
@@ -68,7 +68,6 @@ class AccountsUtils(object):
 
     self._CreateSudoersGroup()
     self.groups = groups.split(',') if groups else []
-    self.groups.append(self.google_sudoers_group)
     self.groups = list(filter(self._GetGroup, self.groups))
     self.remove = remove
 
@@ -245,18 +244,25 @@ class AccountsUtils(object):
     file_utils.SetPermissions(
         authorized_keys_file, mode=0o600, uid=uid, gid=gid)
 
-  def _RemoveSudoer(self, user):
-    """Remove a Linux user account from the sudoers group.
+  def _UpdateSudoer(self, user, sudoer=False):
+    """Update sudoer group membership for a Linux user account.
 
     Args:
       user: string, the name of the Linux user account.
+      sudoer: bool, True if the user should be a sudoer.
 
     Returns:
       bool, True if user update succeeded.
     """
-    self.logger.debug('Removing user %s from the Google sudoers group.', user)
-    command = self.gpasswd_cmd.format(
-        user=user, group=self.google_sudoers_group)
+    if sudoer:
+      self.logger.info('Adding user %s to the Google sudoers group.', user)
+      command = self.gpasswd_cmd.format(
+          option='-a', user=user, group=self.google_sudoers_group)
+    else:
+      self.logger.info('Removing user %s from the Google sudoers group.', user)
+      command = self.gpasswd_cmd.format(
+          option='-d', user=user, group=self.google_sudoers_group)
+
     try:
       subprocess.check_call(command.split(' '))
     except subprocess.CalledProcessError as e:
@@ -330,11 +336,13 @@ class AccountsUtils(object):
       self.logger.warning('Invalid user account name %s.', user)
       return False
     if not self._GetUser(user):
-      # User does not exist. Attempt to create the user.
-      if not self._AddUser(user):
+      # User does not exist. Attempt to create the user and add them to the
+      # appropriate user groups.
+      if not (self._AddUser(user) and
+              self._UpdateUserGroups(user, self.groups)):
         return False
-    # Add the user to the appropriate user groups.
-    if not self._UpdateUserGroups(user, self.groups):
+    # Add the user to the google sudoers group.
+    if not self._UpdateSudoer(user, sudoer=True):
       return False
 
     # Don't try to manage account SSH keys with a shell set to disable
@@ -371,4 +379,4 @@ class AccountsUtils(object):
       else:
         self.logger.info('Removed user account %s.', user)
     self._RemoveAuthorizedKeys(user)
-    self._RemoveSudoer(user)
+    self._UpdateSudoer(user, sudoer=False)
diff --git a/google_compute_engine/accounts/tests/accounts_utils_test.py b/google_compute_engine/accounts/tests/accounts_utils_test.py
index 0f0e56c..89fbde7 100644
--- a/google_compute_engine/accounts/tests/accounts_utils_test.py
+++ b/google_compute_engine/accounts/tests/accounts_utils_test.py
@@ -31,7 +31,7 @@ class AccountsUtilsTest(unittest.TestCase):
     self.sudoers_file = '/sudoers/file'
     self.users_dir = '/users'
     self.users_file = '/users/file'
-    self.gpasswd_cmd = 'useradd -m -s /bin/bash -p * {user}'
+    self.gpasswd_cmd = 'gpasswd {option} {user} {group}'
     self.groupadd_cmd = 'groupadd {group}'
     self.useradd_cmd = 'useradd -m -s /bin/bash -p * {user}'
     self.userdel_cmd = 'userdel -r {user}'
@@ -60,7 +60,7 @@ class AccountsUtilsTest(unittest.TestCase):
         logger=mock_logger, groups='foo,google,bar', remove=True)
     mock_create.assert_called_once_with()
     self.assertEqual(utils.logger, mock_logger)
-    self.assertEqual(sorted(utils.groups), ['google', 'google-sudoers'])
+    self.assertEqual(sorted(utils.groups), ['google'])
     self.assertTrue(utils.remove)
 
   @mock.patch('google_compute_engine.accounts.accounts_utils.grp')
@@ -431,30 +431,47 @@ class AccountsUtilsTest(unittest.TestCase):
     mock_permissions.assert_not_called()
 
   @mock.patch('google_compute_engine.accounts.accounts_utils.subprocess.check_call')
-  def testRemoveSudoer(self, mock_call):
+  def testUpdateSudoer(self, mock_call):
     user = 'user'
-    command = self.usermod_cmd.format(user=user, groups=self.sudoers_group)
+    command = self.gpasswd_cmd.format(
+        option='-d', user=user, group=self.sudoers_group)
 
     self.assertTrue(
-        accounts_utils.AccountsUtils._RemoveSudoer(self.mock_utils, user))
-    mock.call.assert_called_once_with(command.split(' ')),
+        accounts_utils.AccountsUtils._UpdateSudoer(self.mock_utils, user))
+    mock.call.assert_called_once_with(command.split(' '))
     expected_calls = [
+        mock.call.info(mock.ANY, user),
         mock.call.debug(mock.ANY, user),
+    ]
+    self.assertEqual(self.mock_logger.mock_calls, expected_calls)
+
+  @mock.patch('google_compute_engine.accounts.accounts_utils.subprocess.check_call')
+  def testUpdateSudoerAddSudoer(self, mock_call):
+    user = 'user'
+    command = self.gpasswd_cmd.format(
+        option='-a', user=user, group=self.sudoers_group)
+
+    self.assertTrue(
+        accounts_utils.AccountsUtils._UpdateSudoer(
+            self.mock_utils, user, sudoer=True))
+    mock.call.assert_called_once_with(command.split(' '))
+    expected_calls = [
+        mock.call.info(mock.ANY, user),
         mock.call.debug(mock.ANY, user),
     ]
     self.assertEqual(self.mock_logger.mock_calls, expected_calls)
 
   @mock.patch('google_compute_engine.accounts.accounts_utils.subprocess.check_call')
-  def testRemoveSudoerError(self, mock_call):
+  def testUpdateSudoerError(self, mock_call):
     user = 'user'
     command = self.usermod_cmd.format(user=user, groups=self.sudoers_group)
     mock_call.side_effect = subprocess.CalledProcessError(1, 'Test')
 
     self.assertFalse(
-        accounts_utils.AccountsUtils._RemoveSudoer(self.mock_utils, user))
-    mock.call.assert_called_once_with(command.split(' ')),
+        accounts_utils.AccountsUtils._UpdateSudoer(self.mock_utils, user))
+    mock.call.assert_called_once_with(command.split(' '))
     expected_calls = [
-        mock.call.debug(mock.ANY, user),
+        mock.call.info(mock.ANY, user),
         mock.call.warning(mock.ANY, user, mock.ANY),
     ]
     self.assertEqual(self.mock_logger.mock_calls, expected_calls)
@@ -587,8 +604,8 @@ class AccountsUtilsTest(unittest.TestCase):
     for user in valid_users:
       self.assertTrue(
           accounts_utils.AccountsUtils.UpdateUser(self.mock_utils, user, keys))
-      self.mock_utils._UpdateUserGroups.assert_called_once_with(user, groups)
-      self.mock_utils._UpdateUserGroups.reset_mock()
+      self.mock_utils._UpdateSudoer.assert_called_once_with(user, sudoer=True)
+      self.mock_utils._UpdateSudoer.reset_mock()
       self.mock_utils._UpdateAuthorizedKeys.assert_called_once_with(user, keys)
       self.mock_utils._UpdateAuthorizedKeys.reset_mock()
     self.mock_logger.warning.assert_not_called()
@@ -624,7 +641,6 @@ class AccountsUtilsTest(unittest.TestCase):
         accounts_utils.AccountsUtils.UpdateUser(self.mock_utils, user, []))
     self.mock_utils._GetUser.assert_called_once_with(user)
     self.mock_utils._AddUser.assert_called_once_with(user)
-    self.mock_utils._UpdateUserGroups.assert_not_called()
 
   def testUpdateUserFailedUpdateGroups(self):
     user = 'user'
@@ -653,8 +669,23 @@ class AccountsUtilsTest(unittest.TestCase):
 
     self.assertTrue(
         accounts_utils.AccountsUtils.UpdateUser(self.mock_utils, user, []))
+    self.mock_utils._UpdateSudoer.assert_called_once_with(user, sudoer=True)
     self.mock_utils._UpdateAuthorizedKeys.assert_not_called()
 
+  def testUpdateUserSudoersError(self):
+    user = 'user'
+    groups = ['a', 'b', 'c']
+    keys = ['Key 1', 'Key 2']
+    pw_entry = accounts_utils.pwd.struct_passwd(tuple(['']*7))
+    self.mock_utils.groups = groups
+    self.mock_utils._GetUser.return_value = pw_entry
+    self.mock_utils._UpdateSudoer.return_value = False
+
+    self.assertFalse(
+        accounts_utils.AccountsUtils.UpdateUser(self.mock_utils, user, keys))
+    self.mock_utils._GetUser.assert_called_once_with(user)
+    self.mock_utils._UpdateSudoer.assert_called_once_with(user, sudoer=True)
+
   def testUpdateUserError(self):
     user = 'user'
     groups = ['a', 'b', 'c']
@@ -676,7 +707,7 @@ class AccountsUtilsTest(unittest.TestCase):
 
     accounts_utils.AccountsUtils.RemoveUser(self.mock_utils, user)
     self.mock_utils._RemoveAuthorizedKeys.assert_called_once_with(user)
-    self.mock_utils._RemoveSudoer.assert_called_once_with(user)
+    self.mock_utils._UpdateSudoer.assert_called_once_with(user, sudoer=False)
     mock_call.assert_not_called()
 
   @mock.patch('google_compute_engine.accounts.accounts_utils.subprocess.check_call')
@@ -690,7 +721,7 @@ class AccountsUtilsTest(unittest.TestCase):
     expected_calls = [mock.call.info(mock.ANY, user)] * 2
     self.assertEqual(self.mock_logger.mock_calls, expected_calls)
     self.mock_utils._RemoveAuthorizedKeys.assert_called_once_with(user)
-    self.mock_utils._RemoveSudoer.assert_called_once_with(user)
+    self.mock_utils._UpdateSudoer.assert_called_once_with(user, sudoer=False)
 
   @mock.patch('google_compute_engine.accounts.accounts_utils.subprocess.check_call')
   def testRemoveUserError(self, mock_call):
@@ -707,7 +738,7 @@ class AccountsUtilsTest(unittest.TestCase):
     ]
     self.assertEqual(self.mock_logger.mock_calls, expected_calls)
     self.mock_utils._RemoveAuthorizedKeys.assert_called_once_with(user)
-    self.mock_utils._RemoveSudoer.assert_called_once_with(user)
+    self.mock_utils._UpdateSudoer.assert_called_once_with(user, sudoer=False)
 
 
 if __name__ == '__main__':
diff --git a/setup.py b/setup.py
index a43f1b1..91d7695 100755
--- a/setup.py
+++ b/setup.py
@@ -36,7 +36,7 @@ setuptools.setup(
     packages=setuptools.find_packages(),
     scripts=glob.glob('scripts/*'),
     url='https://github.com/GoogleCloudPlatform/compute-image-packages',
-    version='2.8.7',
+    version='2.8.8',
     # Entry points create scripts in /usr/bin that call a function.
     entry_points={
         'console_scripts': [
diff --git a/specs/google-compute-engine.spec b/specs/google-compute-engine.spec
index 16975e3..0f4e1be 100644
--- a/specs/google-compute-engine.spec
+++ b/specs/google-compute-engine.spec
@@ -18,7 +18,7 @@
 %endif
 
 Name: google-compute-engine
-Version: 2.8.7
+Version: 2.8.8
 Release: 1%{?dist}
 Summary: Google Compute Engine guest environment.
 License: ASL 2.0
diff --git a/specs/python-google-compute-engine.spec b/specs/python-google-compute-engine.spec
index f62541d..9b0c5c4 100644
--- a/specs/python-google-compute-engine.spec
+++ b/specs/python-google-compute-engine.spec
@@ -18,7 +18,7 @@
 %endif
 
 Name: python-google-compute-engine
-Version: 2.8.7
+Version: 2.8.8
 Release: 1%{?dist}
 Summary: Google Compute Engine python library
 License: ASL 2.0