diff --git a/cloud_governance/policy/policy_operations/aws/tag_user/tag_iam_user.py b/cloud_governance/policy/policy_operations/aws/tag_user/tag_iam_user.py index 9372128fb..570d0d1d7 100644 --- a/cloud_governance/policy/policy_operations/aws/tag_user/tag_iam_user.py +++ b/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): """ diff --git a/tests/unittest/cloud_governance/aws/tag_user/test_tag_iam_user.py b/tests/unittest/cloud_governance/aws/tag_user/test_tag_iam_user.py index 23a7b296b..4aad26e72 100644 --- a/tests/unittest/cloud_governance/aws/tag_user/test_tag_iam_user.py +++ b/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'