From ad461a3aab7769116bd3b0e3158b2633cafb6ed5 Mon Sep 17 00:00:00 2001 From: Peter Braun Date: Tue, 29 Jul 2025 13:21:56 +0200 Subject: [PATCH] fix: inconsistent return values in github migrator (#7035) * fix: inconsistent return values in github migrator * feat: check setting value before updating and report correct status * fix linter issues --- .../tests/unit/utils/test_github_migrator.py | 124 ++++++++++++++++++ awx/sso/utils/github_migrator.py | 4 +- awx/sso/utils/settings_migrator.py | 12 +- 3 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 awx/main/tests/unit/utils/test_github_migrator.py diff --git a/awx/main/tests/unit/utils/test_github_migrator.py b/awx/main/tests/unit/utils/test_github_migrator.py new file mode 100644 index 0000000000..8c585091fc --- /dev/null +++ b/awx/main/tests/unit/utils/test_github_migrator.py @@ -0,0 +1,124 @@ +""" +Unit tests for GitHub authenticator migrator functionality. +""" + +from unittest.mock import Mock, patch +from awx.sso.utils.github_migrator import GitHubMigrator + + +class TestGitHubMigrator: + """Tests for GitHubMigrator class.""" + + def setup_method(self): + """Set up test fixtures.""" + self.gateway_client = Mock() + self.command = Mock() + self.migrator = GitHubMigrator(self.gateway_client, self.command) + + def test_create_gateway_authenticator_returns_boolean_causes_crash(self): + """ + Test that verifies create_gateway_authenticator returns proper dictionary + structure instead of boolean when credentials are missing. + + This test verifies the fix for the bug. + """ + # Mock the get_controller_config to return a GitHub config with missing credentials + github_config_missing_creds = { + 'category': 'github', + 'settings': {'SOCIAL_AUTH_GITHUB_KEY': '', 'SOCIAL_AUTH_GITHUB_SECRET': 'test-secret'}, # Missing key + 'org_mappers': [], + 'team_mappers': [], + 'login_redirect_override': None, + } + + with patch.object(self.migrator, 'get_controller_config', return_value=[github_config_missing_creds]): + with patch.object(self.migrator, '_write_output'): # Mock output to avoid noise + # This should NOT crash now that the bug is fixed + result = self.migrator.migrate() + + # Verify the migration ran successfully without crashing + assert 'created' in result + assert 'failed' in result + # Should have failed=1 since the config has success=False (missing credentials) + assert result['failed'] == 1 + + def test_create_gateway_authenticator_returns_boolean_with_unknown_category(self): + """ + Test that verifies create_gateway_authenticator returns proper dictionary + structure instead of boolean when category is unknown. + + This test verifies the fix for the bug. + """ + # Mock the get_controller_config to return a GitHub config with unknown category + github_config_unknown_category = { + 'category': 'unknown-category', + 'settings': {'SOCIAL_AUTH_UNKNOWN_KEY': 'test-key', 'SOCIAL_AUTH_UNKNOWN_SECRET': 'test-secret'}, + 'org_mappers': [], + 'team_mappers': [], + 'login_redirect_override': None, + } + + with patch.object(self.migrator, 'get_controller_config', return_value=[github_config_unknown_category]): + with patch.object(self.migrator, '_write_output'): # Mock output to avoid noise + # This should NOT crash now that the bug is fixed + result = self.migrator.migrate() + + # Verify the migration ran successfully without crashing + assert 'created' in result + assert 'failed' in result + # Should have failed=1 since the config has success=False (unknown category) + assert result['failed'] == 1 + + def test_create_gateway_authenticator_direct_boolean_return_missing_creds(self): + """ + Test that directly calls create_gateway_authenticator and verifies it returns + proper dictionary structure instead of boolean for missing credentials. + """ + # Config with missing key (empty string) + config_missing_key = { + 'category': 'github', + 'settings': {'SOCIAL_AUTH_GITHUB_KEY': '', 'SOCIAL_AUTH_GITHUB_SECRET': 'test-secret'}, # Missing key + 'org_mappers': [], + 'team_mappers': [], + 'login_redirect_override': None, + } + + with patch.object(self.migrator, '_write_output'): # Mock output to avoid noise + result = self.migrator.create_gateway_authenticator(config_missing_key) + + # Now the method should return a proper dictionary structure + assert isinstance(result, dict), f"Expected dict, got {type(result)} with value: {result}" + assert 'success' in result, f"Expected 'success' key in result: {result}" + assert 'action' in result, f"Expected 'action' key in result: {result}" + assert 'error' in result, f"Expected 'error' key in result: {result}" + # Verify the expected values + assert result['success'] is False + assert result['action'] == 'skipped' + assert 'Missing OAuth2 credentials' in result['error'] + + def test_create_gateway_authenticator_direct_boolean_return_unknown_category(self): + """ + Test that directly calls create_gateway_authenticator and verifies it returns + proper dictionary structure instead of boolean for unknown category. + """ + # Config with unknown category + config_unknown_category = { + 'category': 'unknown-category', + 'settings': {'SOCIAL_AUTH_UNKNOWN_KEY': 'test-key', 'SOCIAL_AUTH_UNKNOWN_SECRET': 'test-secret'}, + 'org_mappers': [], + 'team_mappers': [], + 'login_redirect_override': None, + } + + with patch.object(self.migrator, '_write_output'): # Mock output to avoid noise + result = self.migrator.create_gateway_authenticator(config_unknown_category) + + # Now the method should return a proper dictionary structure + assert isinstance(result, dict), f"Expected dict, got {type(result)} with value: {result}" + assert 'success' in result, f"Expected 'success' key in result: {result}" + assert 'action' in result, f"Expected 'action' key in result: {result}" + assert 'error' in result, f"Expected 'error' key in result: {result}" + # Verify the expected values + assert result['success'] is False + assert result['action'] == 'skipped' + assert 'Unknown category unknown-category' in result['error'] diff --git a/awx/sso/utils/github_migrator.py b/awx/sso/utils/github_migrator.py index acbb3d74a0..2c2df8307f 100644 --- a/awx/sso/utils/github_migrator.py +++ b/awx/sso/utils/github_migrator.py @@ -124,7 +124,7 @@ class GitHubMigrator(BaseAuthenticatorMigrator): if not key_value or not secret_value: self._write_output(f'Skipping {category}: missing OAuth2 credentials', 'warning') - return False + return {'success': False, 'action': 'skipped', 'error': 'Missing OAuth2 credentials'} # Generate authenticator name and slug authenticator_name = category @@ -143,7 +143,7 @@ class GitHubMigrator(BaseAuthenticatorMigrator): authenticator_type = type_mapping.get(category) if not authenticator_type: self._write_output(f'Unknown category {category}, skipping', 'warning') - return False + return {'success': False, 'action': 'skipped', 'error': f'Unknown category {category}'} self._write_output(f'\n--- Processing {category} authenticator ---') self._write_output(f'Name: {authenticator_name}') diff --git a/awx/sso/utils/settings_migrator.py b/awx/sso/utils/settings_migrator.py index 9b963de900..a60e55fca2 100644 --- a/awx/sso/utils/settings_migrator.py +++ b/awx/sso/utils/settings_migrator.py @@ -96,11 +96,21 @@ class SettingsMigrator(BaseAuthenticatorMigrator): setting_value = config['setting_value'] self._write_output(f'\n--- Migrating setting: {setting_name} ---') - self._write_output(f'New value: {setting_value}') try: gateway_setting_name = self._convert_setting_name(setting_name) + # Get current gateway setting value to check if update is needed + current_gateway_value = self.gateway_client.get_gateway_setting(gateway_setting_name) + + # Compare current gateway value with controller value + if current_gateway_value == setting_value: + self._write_output(f'↷ Setting unchanged: {setting_name} (value already matches)', 'warning') + return {'success': True, 'action': 'skipped', 'error': None} + + self._write_output(f'Current value: {current_gateway_value}') + self._write_output(f'New value: {setting_value}') + # Use the new update_gateway_setting method self.gateway_client.update_gateway_setting(gateway_setting_name, setting_value)