From 8b41810189e9067850b6de4a772316f72860995c Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Wed, 11 Oct 2017 14:22:17 -0400 Subject: [PATCH] Consolidation of variables parsing throughout codebase * Remove attempted support of key=value pattern, because it is not actually allowed in practice * Have variables validator defer to the utils variables parser * Prune serializers of a handful of cases that previous attempts at cleanup have missed --- awx/api/serializers.py | 58 +++++++++++++--------------------------- awx/main/models/base.py | 30 ++------------------- awx/main/utils/common.py | 21 +++++++++++---- awx/main/validators.py | 25 +++++------------ 4 files changed, 43 insertions(+), 91 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 873f822171..f0015d3b79 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -11,9 +11,6 @@ import urllib from collections import OrderedDict from dateutil import rrule -# PyYAML -import yaml - # Django from django.conf import settings from django.contrib.auth import authenticate @@ -1312,6 +1309,9 @@ class HostSerializer(BaseSerializerWithVariables): raise serializers.ValidationError({"detail": _("Cannot create Host for Smart Inventory")}) return value + def validate_variables(self, value): + return vars_validate_or_raise(value) + def validate(self, attrs): name = force_text(attrs.get('name', self.instance and self.instance.name or '')) host, port = self._get_host_port_from_name(name) @@ -1319,19 +1319,9 @@ class HostSerializer(BaseSerializerWithVariables): if port: attrs['name'] = host variables = force_text(attrs.get('variables', self.instance and self.instance.variables or '')) - try: - vars_dict = json.loads(variables.strip() or '{}') - vars_dict['ansible_ssh_port'] = port - attrs['variables'] = json.dumps(vars_dict) - except (ValueError, TypeError): - try: - vars_dict = yaml.safe_load(variables) - if vars_dict is None: - vars_dict = {} - vars_dict['ansible_ssh_port'] = port - attrs['variables'] = yaml.dump(vars_dict) - except (yaml.YAMLError, TypeError): - raise serializers.ValidationError({'variables': _('Must be valid JSON or YAML.')}) + vars_dict = parse_yaml_or_json(variables) + vars_dict['ansible_ssh_port'] = port + attrs['variables'] = json.dumps(vars_dict) return super(HostSerializer, self).validate(attrs) @@ -3286,18 +3276,12 @@ class JobLaunchSerializer(BaseSerializer): extra_vars = attrs.get('extra_vars', {}) - if isinstance(extra_vars, basestring): - try: - extra_vars = json.loads(extra_vars) - except (ValueError, TypeError): - try: - extra_vars = yaml.safe_load(extra_vars) - assert isinstance(extra_vars, dict) - except (yaml.YAMLError, TypeError, AttributeError, AssertionError): - errors['extra_vars'] = _('Must be valid JSON or YAML.') - - if not isinstance(extra_vars, dict): - extra_vars = {} + try: + extra_vars = parse_yaml_or_json(extra_vars, silent_failure=False) + except: + # Log exception, in case of yet-unknown parsing failures + logger.exception('extra_vars parsing error on Job Template launch.') + errors['extra_vars'] = _('Must be valid JSON or YAML.') if self.get_survey_enabled(obj): validation_errors = obj.survey_variable_validation(extra_vars) @@ -3371,18 +3355,12 @@ class WorkflowJobLaunchSerializer(BaseSerializer): extra_vars = attrs.get('extra_vars', {}) - if isinstance(extra_vars, basestring): - try: - extra_vars = json.loads(extra_vars) - except (ValueError, TypeError): - try: - extra_vars = yaml.safe_load(extra_vars) - assert isinstance(extra_vars, dict) - except (yaml.YAMLError, TypeError, AttributeError, AssertionError): - errors['extra_vars'] = _('Must be valid JSON or YAML.') - - if not isinstance(extra_vars, dict): - extra_vars = {} + try: + extra_vars = parse_yaml_or_json(extra_vars, silent_failure=False) + except: + # Log exception, in case of yet-unknown parsing failures + logger.exception('extra_vars parsing error on Workflow JT launch.') + errors['extra_vars'] = _('Must be valid JSON or YAML.') if self.get_survey_enabled(obj): validation_errors = obj.survey_variable_validation(extra_vars) diff --git a/awx/main/models/base.py b/awx/main/models/base.py index 6c038dad74..f41f7a94e4 100644 --- a/awx/main/models/base.py +++ b/awx/main/models/base.py @@ -1,13 +1,6 @@ # Copyright (c) 2015 Ansible, Inc. # All Rights Reserved. -# Python -import json -import shlex - -# PyYAML -import yaml - # Django from django.db import models from django.core.exceptions import ValidationError, ObjectDoesNotExist @@ -21,7 +14,7 @@ from taggit.managers import TaggableManager from crum import get_current_user # AWX -from awx.main.utils import encrypt_field +from awx.main.utils import encrypt_field, parse_yaml_or_json __all__ = ['prevent_search', 'VarsDictProperty', 'BaseModel', 'CreatedModifiedModel', 'PasswordFieldsModel', 'PrimordialModel', 'CommonModel', @@ -80,26 +73,7 @@ class VarsDictProperty(object): if hasattr(v, 'items'): return v v = v.encode('utf-8') - d = None - try: - d = json.loads(v.strip() or '{}') - except ValueError: - pass - if d is None: - try: - d = yaml.safe_load(v) - # This can happen if the whole file is commented out - if d is None: - d = {} - except yaml.YAMLError: - pass - if d is None and self.key_value: - d = {} - for kv in [x.decode('utf-8') for x in shlex.split(v, posix=True)]: - if '=' in kv: - k, v = kv.split('=', 1) - d[k] = v - return d if hasattr(d, 'items') else {} + return parse_yaml_or_json(v) def __set__(self, obj, value): raise AttributeError('readonly property') diff --git a/awx/main/utils/common.py b/awx/main/utils/common.py index c11e576e15..87779041d2 100644 --- a/awx/main/utils/common.py +++ b/awx/main/utils/common.py @@ -582,21 +582,32 @@ def cache_list_capabilities(page, prefetch_list, model, user): obj.capabilities_cache[display_method] = True -def parse_yaml_or_json(vars_str): +def parse_yaml_or_json(vars_str, silent_failure=True): ''' - Attempt to parse a string with variables, and if attempt fails, - return an empty dictionary. + Attempt to parse a string of variables. + First, with JSON parser, if that fails, then with PyYAML. + If both attempts fail, return an empty dictionary if `silent_failure` + is True, re-raise the last error if `silent_failure` if False. ''' if isinstance(vars_str, dict): return vars_str + elif isinstance(vars_str, basestring) and vars_str == '""': + return {} + try: vars_dict = json.loads(vars_str) - except (ValueError, TypeError): + assert isinstance(vars_dict, dict) + except (ValueError, TypeError, AssertionError): try: vars_dict = yaml.safe_load(vars_str) + # Can be None if '---' + if vars_dict is None: + return {} assert isinstance(vars_dict, dict) except (yaml.YAMLError, TypeError, AttributeError, AssertionError): - vars_dict = {} + if silent_failure: + return {} + raise return vars_dict diff --git a/awx/main/validators.py b/awx/main/validators.py index e5b1568464..ce1c5b73b5 100644 --- a/awx/main/validators.py +++ b/awx/main/validators.py @@ -4,8 +4,6 @@ # Python import base64 import re -import yaml -import json # Django from django.utils.translation import ugettext_lazy as _ @@ -14,6 +12,9 @@ from django.core.exceptions import ValidationError # REST framework from rest_framework.serializers import ValidationError as RestValidationError +# AWX +from awx.main.utils import parse_yaml_or_json + def validate_pem(data, min_keys=0, max_keys=None, min_certs=0, max_certs=None): """ @@ -179,21 +180,9 @@ def vars_validate_or_raise(vars_str): job templates, inventories, or hosts are either an acceptable blank string, or are valid JSON or YAML dict """ - if isinstance(vars_str, dict) or (isinstance(vars_str, basestring) and vars_str == '""'): + try: + parse_yaml_or_json(vars_str, silent_failure=False) return vars_str - - try: - r = json.loads((vars_str or '').strip() or '{}') - if isinstance(r, dict): - return vars_str - except ValueError: - pass - try: - r = yaml.safe_load(vars_str) - # Can be None if '---' - if isinstance(r, dict) or r is None: - return vars_str - except yaml.YAMLError: - pass - raise RestValidationError(_('Must be valid JSON or YAML.')) + except: + raise RestValidationError(_('Must be valid JSON or YAML.'))