Fix IAM user spreadsheet column misalignment in tag sync

cloud_governance/policy/policy_operations/aws/tag_user/tag_iam_user.py

@@ -234,46 +234,62 @@ def __format_tags(self, username: str, headers: list = None):
 
     def delete_update_user_from_doc(self):
         """
-        This method removes IAM user if not in the IAM list
+        This method syncs the spreadsheet with the live IAM users: it removes rows for
+        users no longer present in IAM and appends rows for new IAM users, keeping every
+        value aligned to the sheet's actual header columns.
         @return:
         """
         self.__google_drive_operations.create_work_sheet(gsheet_id=self.__SPREADSHEET_ID, sheet_name=self.__sheet_name)
         iam_users = [user['UserName'] for user in
                      self.get_detail_resource_list(func_name=self.iam_client.list_users, input_tag='Users',
                                                    check_tag='Marker') if user['UserName'].count('-') <= 3]
-        csv_iam_users = []
-        iam_file = pd.DataFrame(columns=['User', "Project"])
-        if os.path.exists(self.file_name):
+        # The downloaded sheet CSV is the source of truth for both the header columns and
+        # the existing users. If it is missing (download failed / empty sheet) we must NOT
+        # sync: without the real header the appended rows would be misaligned, and with an
+        # empty baseline every IAM user would be re-appended as a duplicate.
+        if not os.path.exists(self.file_name):
+            logger.warning(f'Spreadsheet CSV not found: {self.file_name}. '
+                           f'Skipping IAM user sync to avoid duplicate/misaligned rows.')
+            return
+        try:
             iam_file = pd.read_csv(self.file_name)
-            if not iam_file.empty:
-                csv_iam_users = list(iam_file['User'])
-                for index, user in enumerate(csv_iam_users):
-                    if user not in iam_users:
-                        self.__google_drive_operations.delete_rows(spreadsheet_id=self.__SPREADSHEET_ID,
-                                                                   sheet_name=self.__sheet_name, row_number=index + 1)
-                        logger.info(f'removed user {user}')
-            else:
-                iam_file = pd.DataFrame(columns=['User'])
+        except pd.errors.EmptyDataError:
+            logger.warning(f'Spreadsheet CSV {self.file_name} is empty. '
+                           f'Skipping IAM user sync to avoid duplicate/misaligned rows.')
+            return
+        iam_file.columns = [str(column).strip() for column in iam_file.columns]
+        if 'User' not in iam_file.columns:
+            logger.warning(f'Spreadsheet CSV {self.file_name} is missing the "User" column. '
+                           f'Skipping IAM user sync to avoid misaligned rows.')
+            return
+        # Derive the columns from the actual sheet header so appended values land under the
+        # correct column, regardless of which/how many tags a user has.
+        sheet_columns = list(iam_file.columns)
+        csv_iam_users = [str(user).strip() for user in iam_file['User'].tolist()]
+        # Remove users no longer present in IAM. Delete in descending row order because
+        # each delete is applied immediately; deleting an earlier row would otherwise
+        # shift later rows up and cause the wrong row to be removed.
+        stale_row_indexes = [index for index, user in enumerate(csv_iam_users) if user not in iam_users]
+        for index in sorted(stale_row_indexes, reverse=True):
+            self.__google_drive_operations.delete_rows(spreadsheet_id=self.__SPREADSHEET_ID,
+                                                       sheet_name=self.__sheet_name, row_number=index + 1)
+            logger.info(f'removed user {csv_iam_users[index]}')
+        # Append new IAM users, building each row aligned to the sheet header order
         append_data = []
         for user in iam_users:
-            if user.count('-') <= 3:
-                if user not in csv_iam_users:
-                    if not iam_file.empty:
-                        tags = self.__format_tags(username=user, headers=list(iam_file.columns))
-                    else:
-                        append_data.append(['User'])
-                        tags = self.__format_tags(username=user)
-                    df2 = pd.DataFrame.from_dict([tags])
-                    iam_file = pd.concat([iam_file, df2], ignore_index=True)
-                    iam_file = iam_file.fillna('')
-                    append_data.append(list(iam_file.iloc[-1]))
-                    if len(tags) < len(list(iam_file.columns)):
-                        self.__trigger_mail(user=user)
+            if user.count('-') <= 3 and user not in csv_iam_users:
+                tags = self.__format_tags(username=user, headers=sheet_columns)
+                append_data.append([tags.get(column, '') for column in sheet_columns])
+                if len(tags) < len(sheet_columns):
+                    self.__trigger_mail(user=user)
         if append_data:
+            # Use RAW so tag values beginning with '=' (or '+', '-', '@') are stored as
+            # literal text instead of being interpreted as spreadsheet formulas.
             response = self.__google_drive_operations.append_values(spreadsheet_id=self.__SPREADSHEET_ID,
-                                                                    sheet_name=self.__sheet_name, values=append_data)
+                                                                    sheet_name=self.__sheet_name, values=append_data,
+                                                                    value_input_option='RAW')
             if response:
-                logger.info(f'Updated the users in the spreadsheet')
+                logger.info('Updated the users in the spreadsheet')
 
     def __trigger_mail(self, user: str):
         """

tests/unittest/cloud_governance/aws/tag_user/test_tag_iam_user.py

@@ -1,5 +1,6 @@
 import csv
 import os
+from unittest.mock import MagicMock
 
 import boto3
 from moto import mock_aws
@@ -10,6 +11,20 @@
 file_name = 'tag_user.csv'
 
 
+def __build_tag_user_with_mocked_gsheet(file_path: str):
+    """
+    Helper: build a TagUser and inject mocked Google-Sheet plumbing so
+    delete_update_user_from_doc can be exercised without hitting Google APIs.
+    """
+    tag_user = TagUser(file_name=file_path)
+    mock_gdo = MagicMock()
+    tag_user._TagUser__google_drive_operations = mock_gdo
+    tag_user._TagUser__SPREADSHEET_ID = 'dummy-spreadsheet-id'
+    tag_user._TagUser__sheet_name = 'test-account'
+    tag_user._TagUser__mail = MagicMock()
+    return tag_user, mock_gdo
+
+
 @mock_aws
 def test_generate_user_csv():
     """
@@ -79,3 +94,159 @@ def test_capa_cluster_user_excluded_from_csv():
         os.remove(file_name)
 
     assert row_count == 0
+
+
+@mock_aws
+def test_delete_update_aligns_new_user_columns(tmp_path):
+    """
+    A new IAM user must be appended with its tag values under the correct sheet
+    columns (e.g. Project under Project, not under Budget), regardless of which
+    subset of columns the user has.
+    """
+    iam_client = boto3.client('iam')
+    iam_client.create_user(UserName='existinguser')
+    iam_client.create_user(UserName='newuser', Tags=[{'Key': 'Budget', 'Value': 'dept-budget'},
+                                                     {'Key': 'Project', 'Value': 'PROJECT-A'},
+                                                     {'Key': 'Environment', 'Value': 'TEST'}])
+    csv_path = os.path.join(tmp_path, 'test-account.csv')
+    with open(csv_path, 'w', newline='') as f:
+        writer = csv.writer(f)
+        writer.writerow(['User', 'Budget', 'Project', 'Environment'])
+        writer.writerow(['existinguser', 'dept-budget', 'PROJECT-B', 'TEST'])
+
+    tag_user, mock_gdo = __build_tag_user_with_mocked_gsheet(csv_path)
+    tag_user.delete_update_user_from_doc()
+
+    mock_gdo.append_values.assert_called_once()
+    appended = mock_gdo.append_values.call_args.kwargs['values']
+    # only newuser is appended, aligned to [User, Budget, Project, Environment]
+    assert appended == [['newuser', 'dept-budget', 'PROJECT-A', 'TEST']]
+
+
+@mock_aws
+def test_delete_update_skips_when_csv_missing(tmp_path):
+    """
+    When the downloaded sheet CSV is absent, the sync must skip entirely rather than
+    re-appending every IAM user (which previously produced duplicate/misaligned rows).
+    """
+    iam_client = boto3.client('iam')
+    iam_client.create_user(UserName='someuser', Tags=[{'Key': 'Project', 'Value': 'PROJECT-A'}])
+    missing_csv = os.path.join(tmp_path, 'does-not-exist.csv')
+
+    tag_user, mock_gdo = __build_tag_user_with_mocked_gsheet(missing_csv)
+    tag_user.delete_update_user_from_doc()
+
+    mock_gdo.append_values.assert_not_called()
+    mock_gdo.delete_rows.assert_not_called()
+
+
+@mock_aws
+def test_delete_update_no_duplicate_for_existing_user(tmp_path):
+    """
+    A user already present in the sheet must not be appended again.
+    """
+    iam_client = boto3.client('iam')
+    iam_client.create_user(UserName='newuser', Tags=[{'Key': 'Project', 'Value': 'PROJECT-A'}])
+    csv_path = os.path.join(tmp_path, 'test-account.csv')
+    with open(csv_path, 'w', newline='') as f:
+        writer = csv.writer(f)
+        writer.writerow(['User', 'Budget', 'Project', 'Environment'])
+        writer.writerow(['newuser', 'dept-budget', 'PROJECT-A', 'TEST'])
+
+    tag_user, mock_gdo = __build_tag_user_with_mocked_gsheet(csv_path)
+    tag_user.delete_update_user_from_doc()
+
+    mock_gdo.append_values.assert_not_called()
+
+
+@mock_aws
+def test_delete_update_partial_tags_aligned_and_triggers_mail(tmp_path):
+    """
+    A new user missing some tag columns must still be aligned (blanks under the
+    missing columns) and must trigger the "add tags" reminder mail.
+    """
+    iam_client = boto3.client('iam')
+    iam_client.create_user(UserName='existinguser')
+    iam_client.create_user(UserName='partialuser', Tags=[{'Key': 'Project', 'Value': 'PROJECT-C'}])
+    csv_path = os.path.join(tmp_path, 'test-account.csv')
+    with open(csv_path, 'w', newline='') as f:
+        writer = csv.writer(f)
+        writer.writerow(['User', 'Budget', 'Project', 'Environment'])
+        writer.writerow(['existinguser', 'dept-budget', 'PROJECT-B', 'TEST'])
+
+    tag_user, mock_gdo = __build_tag_user_with_mocked_gsheet(csv_path)
+    tag_user._TagUser__trigger_mail = MagicMock()
+    tag_user.delete_update_user_from_doc()
+
+    appended = mock_gdo.append_values.call_args.kwargs['values']
+    # Project stays under Project; Budget/Environment are blank (not shifted)
+    assert appended == [['partialuser', '', 'PROJECT-C', '']]
+    tag_user._TagUser__trigger_mail.assert_called_once_with(user='partialuser')
+
+
+@mock_aws
+def test_delete_update_skips_when_csv_empty(tmp_path):
+    """
+    A zero-byte downloaded CSV raises pandas.errors.EmptyDataError; the sync must
+    skip through the warning path instead of crashing.
+    """
+    iam_client = boto3.client('iam')
+    iam_client.create_user(UserName='someuser', Tags=[{'Key': 'Project', 'Value': 'PROJECT-A'}])
+    empty_csv = os.path.join(tmp_path, 'empty.csv')
+    open(empty_csv, 'w').close()  # zero-byte file
+
+    tag_user, mock_gdo = __build_tag_user_with_mocked_gsheet(empty_csv)
+    tag_user.delete_update_user_from_doc()  # must not raise
+
+    mock_gdo.append_values.assert_not_called()
+    mock_gdo.delete_rows.assert_not_called()
+
+
+@mock_aws
+def test_delete_update_removes_stale_rows_in_descending_order(tmp_path):
+    """
+    Stale rows must be deleted in descending row order so that removing an earlier
+    row does not shift later rows and cause the wrong row to be deleted.
+    """
+    iam_client = boto3.client('iam')
+    iam_client.create_user(UserName='userB')  # only userB still exists in IAM
+    csv_path = os.path.join(tmp_path, 'test-account.csv')
+    with open(csv_path, 'w', newline='') as f:
+        writer = csv.writer(f)
+        writer.writerow(['User', 'Budget', 'Project', 'Environment'])
+        writer.writerow(['userA', 'dept-budget', 'PROJECT-A', 'TEST'])  # stale -> row 1
+        writer.writerow(['userB', 'dept-budget', 'PROJECT-A', 'TEST'])  # kept  -> row 2
+        writer.writerow(['userC', 'dept-budget', 'PROJECT-A', 'TEST'])  # stale -> row 3
+
+    tag_user, mock_gdo = __build_tag_user_with_mocked_gsheet(csv_path)
+    tag_user.delete_update_user_from_doc()
+
+    row_numbers = [call.kwargs['row_number'] for call in mock_gdo.delete_rows.call_args_list]
+    # userC (row 3) deleted before userA (row 1) -> descending order
+    assert row_numbers == [3, 1]
+
+
+@mock_aws
+def test_delete_update_stores_formula_like_values_as_raw(tmp_path):
+    """
+    Tag values beginning with '=' must be stored literally (value_input_option='RAW')
+    rather than being interpreted by Google Sheets as formulas.
+    """
+    iam_client = boto3.client('iam')
+    iam_client.create_user(UserName='existinguser')
+    iam_client.create_user(UserName='formulauser', Tags=[{'Key': 'Project', 'Value': '=SUM(A1:A2)'}])
+    csv_path = os.path.join(tmp_path, 'test-account.csv')
+    with open(csv_path, 'w', newline='') as f:
+        writer = csv.writer(f)
+        writer.writerow(['User', 'Budget', 'Project', 'Environment'])
+        writer.writerow(['existinguser', 'dept-budget', 'PROJECT-B', 'TEST'])
+
+    tag_user, mock_gdo = __build_tag_user_with_mocked_gsheet(csv_path)
+    tag_user._TagUser__trigger_mail = MagicMock()
+    tag_user.delete_update_user_from_doc()
+
+    call = mock_gdo.append_values.call_args
+    # the '=' value is preserved literally under Project ...
+    assert call.kwargs['values'] == [['formulauser', '', '=SUM(A1:A2)', '']]
+    # ... and RAW is used so Sheets does not evaluate it as a formula
+    assert call.kwargs['value_input_option'] == 'RAW'