Merge pull request #2191 from AlanCoding/schedule_fixes

Fix bugs creating WFJT schedule with passwords

awx/api/serializers.py

@@ -3679,6 +3679,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
@@ -3687,39 +3691,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 password_dict:
-            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 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
+                # - ignore, if default present
-                if self.instance:
-                    db_extra_data = parse_yaml_or_json(self.instance.extra_data)
-                else:
-                    db_extra_data = {}
                 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']:
+                            # NOTE: validation _of_ the default values of password type
-                                attrs['survey_passwords'].pop(key, None)
+                            # questions not done here or on launch, but doing so could
-                                attrs['extra_data'].pop(key, None)
+                            # leak info about values, so it should not be added
-                            else:
+                            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:
@@ -3730,6 +3733,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')

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 = {}
+        exclude_errors = kwargs.pop('_exclude_errors', [])
-        rejected_fields = {}
+        prompted_data = {}
-        accepted_vars, rejected_vars, errors_dict = self.accept_or_ignore_variables(kwargs.get('extra_vars', {}))
+        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)

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"),