From c8e10adc96e1219117bfb877ab1ddd902ceb3666 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Mon, 18 Dec 2017 09:40:04 -0500 Subject: [PATCH] fix bug saving extra_data and follow prompts rules display_extra_vars was not taking a copy of the data before acting on it - this causes a bug where the activity stream will modify the existing object on the model. That leads to new data not being accepted. Also moved the processing of extra_data to prior to the accept or ignore kwargs logic so that we pass the right (post-encryption) form of the variables. --- awx/api/serializers.py | 30 ++++++++++--------- awx/main/models/jobs.py | 2 +- awx/main/models/mixins.py | 7 +++-- .../serializers/test_workflow_serializers.py | 13 ++++---- awx/main/utils/encryption.py | 6 ++++ 5 files changed, 34 insertions(+), 24 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 30ea61de50..709ad1d769 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -3120,25 +3120,11 @@ class LaunchConfigurationBaseSerializer(BaseSerializer): def validate(self, attrs): attrs = super(LaunchConfigurationBaseSerializer, self).validate(attrs) - # Build unsaved version of this config, use it to detect prompts errors ujt = None if 'unified_job_template' in attrs: ujt = attrs['unified_job_template'] elif self.instance: ujt = self.instance.unified_job_template - mock_obj = self._build_mock_obj(attrs) - accepted, rejected, errors = ujt._accept_or_ignore_job_kwargs( - _exclude_errors=self.exclude_errors, **mock_obj.prompts_dict()) - - # Launch configs call extra_vars extra_data for historical reasons - if 'extra_vars' in errors: - errors['extra_data'] = errors.pop('extra_vars') - if errors: - raise serializers.ValidationError(errors) - - # Model `.save` needs the container dict, not the psuedo fields - if mock_obj.char_prompts: - attrs['char_prompts'] = mock_obj.char_prompts # Insert survey_passwords to track redacted variables if 'extra_data' in attrs: @@ -3164,6 +3150,22 @@ class LaunchConfigurationBaseSerializer(BaseSerializer): _('Provided variable {} has no database value to replace with.').format(key)) else: attrs['extra_data'][key] = db_extra_data[key] + + # Build unsaved version of this config, use it to detect prompts errors + mock_obj = self._build_mock_obj(attrs) + accepted, rejected, errors = ujt._accept_or_ignore_job_kwargs( + _exclude_errors=self.exclude_errors, **mock_obj.prompts_dict()) + + # Launch configs call extra_vars extra_data for historical reasons + if 'extra_vars' in errors: + errors['extra_data'] = errors.pop('extra_vars') + if errors: + raise serializers.ValidationError(errors) + + # Model `.save` needs the container dict, not the psuedo fields + if mock_obj.char_prompts: + attrs['char_prompts'] = mock_obj.char_prompts + return attrs diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index 4ddeda81a5..2bb2f53ca2 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -893,7 +893,7 @@ class LaunchTimeConfig(BaseModel): Hides fields marked as passwords in survey. ''' if self.survey_passwords: - extra_data = parse_yaml_or_json(self.extra_data) + extra_data = parse_yaml_or_json(self.extra_data).copy() for key, value in self.survey_passwords.items(): if key in extra_data: extra_data[key] = value diff --git a/awx/main/models/mixins.py b/awx/main/models/mixins.py index 5e30b29e05..9bf4d9292f 100644 --- a/awx/main/models/mixins.py +++ b/awx/main/models/mixins.py @@ -14,7 +14,7 @@ from awx.main.models.rbac import ( Role, RoleAncestorEntry, get_roles_on_resource ) from awx.main.utils import parse_yaml_or_json -from awx.main.utils.encryption import decrypt_value, get_encryption_key +from awx.main.utils.encryption import decrypt_value, get_encryption_key, is_encrypted from awx.main.fields import JSONField, AskForField @@ -266,9 +266,10 @@ class SurveyJobTemplateMixin(models.Model): survey_errors = [] for survey_element in self.survey_spec.get("spec", []): key = survey_element.get('variable', None) - if extra_passwords and key in extra_passwords and data.get(key, None): + value = data.get(key, None) + if extra_passwords and key in extra_passwords and is_encrypted(value): element_errors = self._survey_element_validation(survey_element, { - key: decrypt_value(get_encryption_key('value', pk=None), data[key]) + key: decrypt_value(get_encryption_key('value', pk=None), value) }) else: element_errors = self._survey_element_validation(survey_element, data) diff --git a/awx/main/tests/unit/api/serializers/test_workflow_serializers.py b/awx/main/tests/unit/api/serializers/test_workflow_serializers.py index 712345a1b6..49fb2da4e7 100644 --- a/awx/main/tests/unit/api/serializers/test_workflow_serializers.py +++ b/awx/main/tests/unit/api/serializers/test_workflow_serializers.py @@ -197,7 +197,7 @@ class TestWorkflowJobTemplateNodeSerializerSurveyPasswords(): assert attrs['extra_data']['var1'].startswith('$encrypted$') assert len(attrs['extra_data']['var1']) > len('$encrypted$') - def test_use_db_answer(self, jt): + def test_use_db_answer(self, jt, mocker): serializer = WorkflowJobTemplateNodeSerializer() wfjt = WorkflowJobTemplate(name='fake-wfjt') serializer.instance = WorkflowJobTemplateNode( @@ -205,11 +205,12 @@ class TestWorkflowJobTemplateNodeSerializerSurveyPasswords(): unified_job_template=jt, extra_data={'var1': '$encrypted$foooooo'} ) - attrs = serializer.validate({ - 'unified_job_template': jt, - 'workflow_job_template': wfjt, - 'extra_data': {'var1': '$encrypted$'} - }) + with mocker.patch('awx.main.models.mixins.decrypt_value', return_value='foo'): + attrs = serializer.validate({ + 'unified_job_template': jt, + 'workflow_job_template': wfjt, + 'extra_data': {'var1': '$encrypted$'} + }) assert 'survey_passwords' in attrs assert 'var1' in attrs['survey_passwords'] assert attrs['extra_data']['var1'] == '$encrypted$foooooo' diff --git a/awx/main/utils/encryption.py b/awx/main/utils/encryption.py index 29aed4721e..b8c0cc45a0 100644 --- a/awx/main/utils/encryption.py +++ b/awx/main/utils/encryption.py @@ -137,3 +137,9 @@ def encrypt_dict(data, fields): encrypt_fields = set(data.keys()).intersection(fields) for key in encrypt_fields: data[key] = encrypt_value(data[key]) + + +def is_encrypted(value): + if not isinstance(value, six.string_types): + return False + return value.startswith('$encrypted$') and len(value) > len('$encrypted$')