From f753bea24f3a897909c6102d68dfce34eaca938c Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Fri, 15 Jun 2018 14:34:49 -0400 Subject: [PATCH] Test-based fixes to launch config password handling Fix bug creating WFJT schedule with passwords: discard survey_passwords field if given in WFJT prompts processing method. Fixed by porting prior JT fix to WFJT method of same name. Fix bug where API browser will show encrypted form of variables in the POST submission box after failed attempt: copy extra_data so encrypted data values are not added in still-linked request.data Fix a bug where submitted extra_data $encrypted$ string literal was saved because survey_passwords was empty when there was no diff from prior. Allow not answering required password questions with a non-empty default value when saving a launch config. The literal $encrypted$ string now gets passed into the prompts / survey validator. --- awx/api/serializers.py | 53 ++++++++++++------- awx/main/models/workflow.py | 19 ++++--- .../tests/functional/api/test_schedules.py | 47 +++++++++++++++- 3 files changed, 92 insertions(+), 27 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index cdba8a4dfd..a6bba2aff4 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -3632,6 +3632,10 @@ class LaunchConfigurationBaseSerializer(BaseSerializer): return summary_fields def validate(self, attrs): + db_extra_data = {} + if self.instance: + db_extra_data = parse_yaml_or_json(self.instance.extra_data) + attrs = super(LaunchConfigurationBaseSerializer, self).validate(attrs) ujt = None @@ -3640,39 +3644,38 @@ class LaunchConfigurationBaseSerializer(BaseSerializer): elif self.instance: ujt = self.instance.unified_job_template - # Replace $encrypted$ submissions with db value if exists # build additional field survey_passwords to track redacted variables + password_dict = {} + extra_data = parse_yaml_or_json(attrs.get('extra_data', {})) + if hasattr(ujt, 'survey_password_variables'): + # Prepare additional field survey_passwords for save + for key in ujt.survey_password_variables(): + if key in extra_data: + password_dict[key] = REPLACE_STR + + # Replace $encrypted$ submissions with db value if exists if 'extra_data' in attrs: - extra_data = parse_yaml_or_json(attrs.get('extra_data', {})) - if hasattr(ujt, 'survey_password_variables'): - # Prepare additional field survey_passwords for save - password_dict = {} - for key in ujt.survey_password_variables(): - if key in extra_data: - password_dict[key] = REPLACE_STR + if password_dict: if not self.instance or password_dict != self.instance.survey_passwords: attrs['survey_passwords'] = password_dict.copy() # Force dict type (cannot preserve YAML formatting if passwords are involved) - if not isinstance(attrs['extra_data'], dict): - attrs['extra_data'] = parse_yaml_or_json(attrs['extra_data']) # Encrypt the extra_data for save, only current password vars in JT survey + # but first, make a copy or else this is referenced by request.data, and + # user could get encrypted string in form data in API browser + attrs['extra_data'] = extra_data.copy() encrypt_dict(attrs['extra_data'], password_dict.keys()) # For any raw $encrypted$ string, either # - replace with existing DB value # - raise a validation error - # - remove key from extra_data if survey default is present - if self.instance: - db_extra_data = parse_yaml_or_json(self.instance.extra_data) - else: - db_extra_data = {} + # - ignore, if default present for key in password_dict.keys(): if attrs['extra_data'].get(key, None) == REPLACE_STR: if key not in db_extra_data: element = ujt.pivot_spec(ujt.survey_spec)[key] - if 'default' in element and element['default']: - attrs['survey_passwords'].pop(key, None) - attrs['extra_data'].pop(key, None) - else: + # NOTE: validation _of_ the default values of password type + # questions not done here or on launch, but doing so could + # leak info about values, so it should not be added + if not ('default' in element and element['default']): raise serializers.ValidationError( {"extra_data": _('Provided variable {} has no database value to replace with.').format(key)}) else: @@ -3683,6 +3686,18 @@ class LaunchConfigurationBaseSerializer(BaseSerializer): accepted, rejected, errors = ujt._accept_or_ignore_job_kwargs( _exclude_errors=self.exclude_errors, **mock_obj.prompts_dict()) + # Remove all unprocessed $encrypted$ strings, indicating default usage + if 'extra_data' in attrs and password_dict: + for key, value in attrs['extra_data'].copy().items(): + if value == REPLACE_STR: + if key in password_dict: + attrs['extra_data'].pop(key) + attrs.get('survey_passwords', {}).pop(key, None) + else: + errors.setdefault('extra_vars', []).append( + _('"$encrypted$ is a reserved keyword, may not be used for {var_name}."'.format(key)) + ) + # Launch configs call extra_vars extra_data for historical reasons if 'extra_vars' in errors: errors['extra_data'] = errors.pop('extra_vars') diff --git a/awx/main/models/workflow.py b/awx/main/models/workflow.py index f553fd7254..198595424f 100644 --- a/awx/main/models/workflow.py +++ b/awx/main/models/workflow.py @@ -371,23 +371,28 @@ class WorkflowJobTemplate(UnifiedJobTemplate, WorkflowJobOptions, SurveyJobTempl return workflow_job def _accept_or_ignore_job_kwargs(self, _exclude_errors=(), **kwargs): - prompted_fields = {} - rejected_fields = {} - accepted_vars, rejected_vars, errors_dict = self.accept_or_ignore_variables(kwargs.get('extra_vars', {})) + exclude_errors = kwargs.pop('_exclude_errors', []) + prompted_data = {} + rejected_data = {} + accepted_vars, rejected_vars, errors_dict = self.accept_or_ignore_variables( + kwargs.get('extra_vars', {}), + _exclude_errors=exclude_errors, + extra_passwords=kwargs.get('survey_passwords', {})) if accepted_vars: - prompted_fields['extra_vars'] = accepted_vars + prompted_data['extra_vars'] = accepted_vars if rejected_vars: - rejected_fields['extra_vars'] = rejected_vars + rejected_data['extra_vars'] = rejected_vars # WFJTs do not behave like JTs, it can not accept inventory, credential, etc. bad_kwargs = kwargs.copy() bad_kwargs.pop('extra_vars', None) + bad_kwargs.pop('survey_passwords', None) if bad_kwargs: - rejected_fields.update(bad_kwargs) + rejected_data.update(bad_kwargs) for field in bad_kwargs: errors_dict[field] = _('Field is not allowed for use in workflows.') - return prompted_fields, rejected_fields, errors_dict + return prompted_data, rejected_data, errors_dict def can_start_without_user_input(self): return not bool(self.variables_needed_to_start) diff --git a/awx/main/tests/functional/api/test_schedules.py b/awx/main/tests/functional/api/test_schedules.py index c03ecead1a..21e3b91065 100644 --- a/awx/main/tests/functional/api/test_schedules.py +++ b/awx/main/tests/functional/api/test_schedules.py @@ -2,7 +2,8 @@ import pytest from awx.api.versioning import reverse -from awx.main.models import JobTemplate +from awx.main.models import JobTemplate, Schedule +from awx.main.utils.encryption import decrypt_value, get_encryption_key RRULE_EXAMPLE = 'DTSTART:20151117T050000Z RRULE:FREQ=DAILY;INTERVAL=1;COUNT=1' @@ -51,6 +52,50 @@ def test_valid_survey_answer(post, admin_user, project, inventory, survey_spec_f admin_user, expect=201) +@pytest.mark.django_db +def test_encrypted_survey_answer(post, patch, admin_user, project, inventory, survey_spec_factory): + job_template = JobTemplate.objects.create( + name='test-jt', + project=project, + playbook='helloworld.yml', + inventory=inventory, + ask_variables_on_launch=False, + survey_enabled=True, + survey_spec=survey_spec_factory([{'variable': 'var1', 'type': 'password'}]) + ) + + # test encrypted-on-create + url = reverse('api:job_template_schedules_list', kwargs={'pk': job_template.id}) + r = post(url, {'name': 'test sch', 'rrule': RRULE_EXAMPLE, 'extra_data': '{"var1": "foo"}'}, + admin_user, expect=201) + assert r.data['extra_data']['var1'] == "$encrypted$" + schedule = Schedule.objects.get(pk=r.data['id']) + assert schedule.extra_data['var1'].startswith('$encrypted$') + assert decrypt_value(get_encryption_key('value', pk=None), schedule.extra_data['var1']) == 'foo' + + # test a no-op change + r = patch( + schedule.get_absolute_url(), + data={'extra_data': {'var1': '$encrypted$'}}, + user=admin_user, + expect=200 + ) + assert r.data['extra_data']['var1'] == '$encrypted$' + schedule.refresh_from_db() + assert decrypt_value(get_encryption_key('value', pk=None), schedule.extra_data['var1']) == 'foo' + + # change to a different value + r = patch( + schedule.get_absolute_url(), + data={'extra_data': {'var1': 'bar'}}, + user=admin_user, + expect=200 + ) + assert r.data['extra_data']['var1'] == '$encrypted$' + schedule.refresh_from_db() + assert decrypt_value(get_encryption_key('value', pk=None), schedule.extra_data['var1']) == 'bar' + + @pytest.mark.django_db @pytest.mark.parametrize('rrule, error', [ ("", "This field may not be blank"),