From fd93964953e65eaf4e5996aa9aa0435540c6e374 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Fri, 3 Apr 2020 14:05:35 -0400 Subject: [PATCH] Changed status tweaks for API validation and encryption API validation topic: - do not set changed=True if the object did not actually change - deals with cases where API manipulates data before saving Warn if encrypted data prevent accurate changed status Handle false changed case of tower_user password password field not present in data Test changed=True warning with JT/WFJT survey spec defaults case for list data in JSON --- .../plugins/module_utils/tower_api.py | 57 +++++++++++++---- .../plugins/modules/tower_credential_type.py | 3 +- .../plugins/modules/tower_job_template.py | 2 + .../modules/tower_workflow_job_template.py | 2 + awx_collection/test/awx/conftest.py | 8 +-- awx_collection/test/awx/test_credential.py | 62 ++++++++++--------- .../test/awx/test_credential_type.py | 49 +++++++++++++++ awx_collection/test/awx/test_job_template.py | 37 ++++++++++- awx_collection/test/awx/test_notification.py | 1 - awx_collection/test/awx/test_user.py | 47 ++++++++++++++ 10 files changed, 220 insertions(+), 48 deletions(-) create mode 100644 awx_collection/test/awx/test_credential_type.py create mode 100644 awx_collection/test/awx/test_user.py diff --git a/awx_collection/plugins/module_utils/tower_api.py b/awx_collection/plugins/module_utils/tower_api.py index b7145e15f4..4bb6282859 100644 --- a/awx_collection/plugins/module_utils/tower_api.py +++ b/awx_collection/plugins/module_utils/tower_api.py @@ -44,6 +44,7 @@ class TowerModule(AnsibleModule): cookie_jar = CookieJar() authenticated = False config_name = 'tower_cli.cfg' + ENCRYPTED_STRING = "$encrypted$" def __init__(self, argument_spec, **kwargs): args = dict( @@ -574,6 +575,45 @@ class TowerModule(AnsibleModule): else: self.exit_json(**self.json_output) + def _encrypted_changed_warning(self, field, old, warning=False): + if not warning: + return + self.warn( + 'The field {0} of {1} {2} has encrypted data and may inaccurately report task is changed.'.format( + field, old.get('type', 'unknown'), old.get('id', 'unknown') + )) + + @staticmethod + def has_encrypted_values(obj): + """Returns True if JSON-like python content in obj has $encrypted$ + anywhere in the data as a value + """ + if isinstance(obj, dict): + for val in obj.values(): + if TowerModule.has_encrypted_values(val): + return True + elif isinstance(obj, list): + for val in obj: + if TowerModule.has_encrypted_values(val): + return True + elif obj == TowerModule.ENCRYPTED_STRING: + return True + return False + + def objects_could_be_different(self, old, new, field_set=None, warning=False): + if field_set is None: + field_set = set(fd for fd in new.keys() if fd not in ('modified', 'related', 'summary_fields')) + for field in field_set: + new_field = new.get(field, None) + old_field = old.get(field, None) + if old_field != new_field: + return True # Something doesn't match + elif self.has_encrypted_values(new_field) or field not in new: + # case of 'field not in new' - user password write-only field that API will not display + self._encrypted_changed_warning(field, old, warning=warning) + return True + return False + def update_if_needed(self, existing_item, new_item, on_update=None, associations=None): # This will exit from the module on its own # If the method successfully updates an item and on_update param is defined, @@ -601,22 +641,17 @@ class TowerModule(AnsibleModule): self.fail_json(msg="Unable to process update of item due to missing data {0}".format(ke)) # Check to see if anything within the item requires the item to be updated - needs_update = False - for field in new_item: - existing_field = existing_item.get(field, None) - new_field = new_item.get(field, None) - # If the two items don't match and we are not comparing '' to None - if existing_field != new_field and not (existing_field in (None, '') and new_field == ''): - # Something doesn't match so let's update it - needs_update = True - break + needs_patch = self.objects_could_be_different(existing_item, new_item) # If we decided the item needs to be updated, update it self.json_output['id'] = item_id - if needs_update: + if needs_patch: response = self.patch_endpoint(item_url, **{'data': new_item}) if response['status_code'] == 200: - self.json_output['changed'] = True + # compare apples-to-apples, old API data to new API data + # but do so considering the fields given in parameters + self.json_output['changed'] = self.objects_could_be_different( + existing_item, response['json'], field_set=new_item.keys(), warning=True) elif 'json' in response and '__all__' in response['json']: self.fail_json(msg=response['json']['__all__']) else: diff --git a/awx_collection/plugins/modules/tower_credential_type.py b/awx_collection/plugins/modules/tower_credential_type.py index 2c9217a1e3..5c3186bca0 100644 --- a/awx_collection/plugins/modules/tower_credential_type.py +++ b/awx_collection/plugins/modules/tower_credential_type.py @@ -123,9 +123,10 @@ def main(): # These will be passed into the create/updates credential_type_params = { 'name': new_name if new_name else name, - 'kind': kind, 'managed_by_tower': False, } + if kind: + credential_type_params['kind'] = kind if module.params.get('description'): credential_type_params['description'] = module.params.get('description') if module.params.get('inputs'): diff --git a/awx_collection/plugins/modules/tower_job_template.py b/awx_collection/plugins/modules/tower_job_template.py index 6266c80082..4447583ab2 100644 --- a/awx_collection/plugins/modules/tower_job_template.py +++ b/awx_collection/plugins/modules/tower_job_template.py @@ -450,6 +450,8 @@ def main(): existing_spec = module.get_endpoint(spec_endpoint)['json'] if new_spec != existing_spec: module.json_output['changed'] = True + if existing_item and module.has_encrypted_values(existing_spec): + module._encrypted_changed_warning('survey_spec', existing_item, warning=True) on_change = update_survey if state == 'absent': diff --git a/awx_collection/plugins/modules/tower_workflow_job_template.py b/awx_collection/plugins/modules/tower_workflow_job_template.py index c22086a939..198aa208fc 100644 --- a/awx_collection/plugins/modules/tower_workflow_job_template.py +++ b/awx_collection/plugins/modules/tower_workflow_job_template.py @@ -209,6 +209,8 @@ def main(): existing_spec = module.get_endpoint(spec_endpoint) if new_spec != existing_spec: module.json_output['changed'] = True + if existing_item and module.has_encrypted_values(existing_spec): + module._encrypted_changed_warning('survey_spec', existing_item, warning=True) on_change = update_survey if state == 'absent': diff --git a/awx_collection/test/awx/conftest.py b/awx_collection/test/awx/conftest.py index 5278f380cb..30e8866320 100644 --- a/awx_collection/test/awx/conftest.py +++ b/awx_collection/test/awx/conftest.py @@ -241,12 +241,12 @@ def silence_deprecation(): """The deprecation warnings are stored in a global variable they will create cross-test interference. Use this to turn them off. """ - with mock.patch('ansible.module_utils.basic.AnsibleModule.deprecate'): - yield + with mock.patch('ansible.module_utils.basic.AnsibleModule.deprecate') as this_mock: + yield this_mock @pytest.fixture def silence_warning(): """Warnings use global variable, same as deprecations.""" - with mock.patch('ansible.module_utils.basic.AnsibleModule.warn'): - yield + with mock.patch('ansible.module_utils.basic.AnsibleModule.warn') as this_mock: + yield this_mock diff --git a/awx_collection/test/awx/test_credential.py b/awx_collection/test/awx/test_credential.py index 93bdfaf837..43619576bb 100644 --- a/awx_collection/test/awx/test_credential.py +++ b/awx_collection/test/awx/test_credential.py @@ -81,30 +81,6 @@ def test_create_vault_credential(run_module, admin_user, organization, silence_d assert result['id'] == cred.pk -@pytest.mark.django_db -def test_create_custom_credential_type(run_module, admin_user, silence_deprecation): - # Example from docs - result = run_module('tower_credential_type', dict( - name='Nexus', - description='Credentials type for Nexus', - kind='cloud', - inputs={"fields": [{"id": "server", "type": "string", "default": "", "label": ""}], "required": []}, - injectors={'extra_vars': {'nexus_credential': 'test'}}, - state='present', - validate_certs='false' - ), admin_user) - assert not result.get('failed', False), result.get('msg', result) - assert result.get('changed'), result - - ct = CredentialType.objects.get(name='Nexus') - - assert result['name'] == 'Nexus' - assert result['id'] == ct.pk - - assert ct.inputs == {"fields": [{"id": "server", "type": "string", "default": "", "label": ""}], "required": []} - assert ct.injectors == {'extra_vars': {'nexus_credential': 'test'}} - - @pytest.mark.django_db def test_ct_precedence_over_kind(run_module, admin_user, organization, cred_type, silence_deprecation): result = run_module('tower_credential', dict( @@ -155,16 +131,15 @@ def test_missing_credential_type(run_module, admin_user, organization): @pytest.mark.django_db -def test_make_use_of_custom_credential_type(run_module, admin_user, cred_type): - Organization.objects.create(name='test-org') - +def test_make_use_of_custom_credential_type(run_module, organization, admin_user, cred_type): result = run_module('tower_credential', dict( name='Galaxy Token for Steve', - organization='test-org', + organization=organization.name, credential_type=cred_type.name, - inputs={'token': '7rEZK38DJl58A7RxA6EC7lLvUHbBQ1'}, - state='present' + inputs={'token': '7rEZK38DJl58A7RxA6EC7lLvUHbBQ1'} ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + assert result.get('changed', False), result cred = Credential.objects.get(name='Galaxy Token for Steve') assert cred.credential_type_id == cred_type.id @@ -174,3 +149,30 @@ def test_make_use_of_custom_credential_type(run_module, admin_user, cred_type): assert result['name'] == "Galaxy Token for Steve" assert result['id'] == cred.pk + + +@pytest.mark.django_db +def test_secret_field_write_twice(run_module, organization, admin_user, cred_type, silence_warning): + val1 = '7rEZK38DJl58A7RxA6EC7lLvUHbBQ1' + result = run_module('tower_credential', dict( + name='Galaxy Token for Steve', + organization=organization.name, + credential_type=cred_type.name, + inputs={'token': val1} + ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + + Credential.objects.get(id=result['id']).inputs['token'] == val1 + + val2 = '7rEZ238DJl5837rxA6xxxlLvUHbBQ1' + + result = run_module('tower_credential', dict( + name='Galaxy Token for Steve', + organization=organization.name, + credential_type=cred_type.name, + inputs={'token': val2} + ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + + Credential.objects.get(id=result['id']).inputs['token'] == val2 + assert result.get('changed'), result diff --git a/awx_collection/test/awx/test_credential_type.py b/awx_collection/test/awx/test_credential_type.py new file mode 100644 index 0000000000..29f4869ddf --- /dev/null +++ b/awx_collection/test/awx/test_credential_type.py @@ -0,0 +1,49 @@ +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +import pytest + +from awx.main.models import CredentialType + + +@pytest.mark.django_db +def test_create_custom_credential_type(run_module, admin_user, silence_deprecation): + # Example from docs + result = run_module('tower_credential_type', dict( + name='Nexus', + description='Credentials type for Nexus', + kind='cloud', + inputs={"fields": [{"id": "server", "type": "string", "default": "", "label": ""}], "required": []}, + injectors={'extra_vars': {'nexus_credential': 'test'}}, + state='present', + ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + assert result.get('changed'), result + + ct = CredentialType.objects.get(name='Nexus') + + assert result['name'] == 'Nexus' + assert result['id'] == ct.pk + + assert ct.inputs == {"fields": [{"id": "server", "type": "string", "default": "", "label": ""}], "required": []} + assert ct.injectors == {'extra_vars': {'nexus_credential': 'test'}} + + +@pytest.mark.django_db +def test_changed_false_with_api_changes(run_module, admin_user): + result = run_module('tower_credential_type', dict( + name='foo', + kind='cloud', + inputs={"fields": [{"id": "env_value", "label": "foo", "default": "foo"}]}, + injectors={'env': {'TEST_ENV_VAR': '{{ env_value }}'}}, + ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + assert result.get('changed'), result + + result = run_module('tower_credential_type', dict( + name='foo', + inputs={"fields": [{"id": "env_value", "label": "foo", "default": "foo"}]}, + injectors={'env': {'TEST_ENV_VAR': '{{ env_value }}'}}, + ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + assert not result.get('changed'), result diff --git a/awx_collection/test/awx/test_job_template.py b/awx_collection/test/awx/test_job_template.py index 57d85af2b4..3384d060d9 100644 --- a/awx_collection/test/awx/test_job_template.py +++ b/awx_collection/test/awx/test_job_template.py @@ -112,7 +112,7 @@ def test_job_template_with_survey_spec(run_module, admin_user, project, inventor assert result.get('changed', False), result jt = JobTemplate.objects.get(pk=result['id']) - # assert jt.survey_spec == survey_spec + assert jt.survey_spec == survey_spec prior_ct = ActivityStream.objects.count() result = run_module('tower_job_template', dict( @@ -130,3 +130,38 @@ def test_job_template_with_survey_spec(run_module, admin_user, project, inventor assert jt.survey_spec == survey_spec assert ActivityStream.objects.count() == prior_ct + + +@pytest.mark.django_db +def test_job_template_with_survey_encrypted_default(run_module, admin_user, project, inventory, silence_warning): + spec = { + "spec": [ + { + "index": 0, + "question_name": "my question?", + "default": "very_secret_value", + "variable": "myvar", + "type": "password", + "required": False + } + ], + "description": "test", + "name": "test" + } + for i in range(2): + result = run_module('tower_job_template', dict( + name='foo', + playbook='helloworld.yml', + project=project.name, + inventory=inventory.name, + survey_spec=spec, + survey_enabled=True + ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + + assert result.get('changed', False), result # not actually desired, but assert for sanity + + silence_warning.assert_called_once_with( + "The field survey_spec of job_template {0} has encrypted data and " + "may inaccurately report task is changed.".format(result['id']) + ) diff --git a/awx_collection/test/awx/test_notification.py b/awx_collection/test/awx/test_notification.py index f06b83b79e..9d916d1dc9 100644 --- a/awx_collection/test/awx/test_notification.py +++ b/awx_collection/test/awx/test_notification.py @@ -85,7 +85,6 @@ def test_invalid_notification_configuration(run_module, admin_user, organization @pytest.mark.django_db -@pytest.mark.xfail(reason='Handling API validation changes w.r.t. changed status is an open item') def test_deprecated_to_modern_no_op(run_module, admin_user, organization): nt_config = { 'url': 'http://www.example.com/hook', diff --git a/awx_collection/test/awx/test_user.py b/awx_collection/test/awx/test_user.py new file mode 100644 index 0000000000..bb0821ccd2 --- /dev/null +++ b/awx_collection/test/awx/test_user.py @@ -0,0 +1,47 @@ +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +import pytest + +from unittest import mock + +from awx.main.models import User + + +@pytest.fixture +def mock_auth_stuff(): + """Some really specific session-related stuff is done for changing or setting + passwords, so we will just avoid that here. + """ + with mock.patch('awx.api.serializers.update_session_auth_hash'): + yield + + +@pytest.mark.django_db +def test_create_user(run_module, admin_user, mock_auth_stuff): + result = run_module('tower_user', dict( + username='Bob', + password='pass4word' + ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + assert result.get('changed'), result + + user = User.objects.get(id=result['id']) + assert user.username == 'Bob' + + +@pytest.mark.django_db +def test_password_no_op_warning(run_module, admin_user, mock_auth_stuff, silence_warning): + for i in range(2): + result = run_module('tower_user', dict( + username='Bob', + password='pass4word' + ), admin_user) + assert not result.get('failed', False), result.get('msg', result) + + assert result.get('changed') # not actually desired, but assert for sanity + + silence_warning.assert_called_once_with( + "The field password of user {0} has encrypted data and " + "may inaccurately report task is changed.".format(result['id']) + )