From 8b41810189e9067850b6de4a772316f72860995c Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Wed, 11 Oct 2017 14:22:17 -0400 Subject: [PATCH 1/2] 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.')) From 993fa9290dd4298f168438c5195e7033c73f81c3 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Fri, 13 Oct 2017 09:28:27 -0400 Subject: [PATCH 2/2] additional verbosity for vars parsing exceptions --- awx/api/serializers.py | 16 ++++---- .../functional/api/test_job_runtime_params.py | 2 +- awx/main/tests/unit/utils/test_common.py | 40 +++++++++++++++++++ awx/main/utils/common.py | 28 ++++++++++--- awx/main/validators.py | 5 ++- 5 files changed, 73 insertions(+), 18 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index f0015d3b79..6640aa6e43 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -25,7 +25,7 @@ from django.utils.timezone import now from django.utils.functional import cached_property # Django REST Framework -from rest_framework.exceptions import ValidationError, PermissionDenied +from rest_framework.exceptions import ValidationError, PermissionDenied, ParseError from rest_framework import fields from rest_framework import serializers from rest_framework import validators @@ -3278,10 +3278,9 @@ class JobLaunchSerializer(BaseSerializer): 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.') + except ParseError as e: + # Catch known user variable formatting errors + errors['extra_vars'] = str(e) if self.get_survey_enabled(obj): validation_errors = obj.survey_variable_validation(extra_vars) @@ -3357,10 +3356,9 @@ class WorkflowJobLaunchSerializer(BaseSerializer): 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.') + except ParseError as e: + # Catch known user variable formatting errors + errors['extra_vars'] = str(e) if self.get_survey_enabled(obj): validation_errors = obj.survey_variable_validation(extra_vars) diff --git a/awx/main/tests/functional/api/test_job_runtime_params.py b/awx/main/tests/functional/api/test_job_runtime_params.py index b197ac2520..be6f0b56fa 100644 --- a/awx/main/tests/functional/api/test_job_runtime_params.py +++ b/awx/main/tests/functional/api/test_job_runtime_params.py @@ -199,7 +199,7 @@ def test_job_reject_invalid_prompted_extra_vars(runtime_data, job_template_promp dict(extra_vars='{"unbalanced brackets":'), admin_user, expect=400) assert 'extra_vars' in response.data - assert 'valid JSON or YAML' in str(response.data['extra_vars'][0]) + assert 'Cannot parse as' in str(response.data['extra_vars'][0]) @pytest.mark.django_db diff --git a/awx/main/tests/unit/utils/test_common.py b/awx/main/tests/unit/utils/test_common.py index 41f9012040..d31f2b7472 100644 --- a/awx/main/tests/unit/utils/test_common.py +++ b/awx/main/tests/unit/utils/test_common.py @@ -5,9 +5,13 @@ import os import pytest from uuid import uuid4 +import json +import yaml from django.core.cache import cache +from rest_framework.exceptions import ParseError + from awx.main.utils import common from awx.main.models import ( @@ -38,6 +42,42 @@ def test_parse_yaml_or_json(input_, output): assert common.parse_yaml_or_json(input_) == output +class TestParserExceptions: + + @staticmethod + def json_error(data): + try: + json.loads(data) + return None + except Exception as e: + return str(e) + + @staticmethod + def yaml_error(data): + try: + yaml.load(data) + return None + except Exception as e: + return str(e) + + def test_invalid_JSON_and_YAML(self): + data = "{key:val" + with pytest.raises(ParseError) as exc: + common.parse_yaml_or_json(data, silent_failure=False) + message = str(exc.value) + assert "Cannot parse as" in message + assert self.json_error(data) in message + assert self.yaml_error(data) in message + + def test_invalid_vars_type(self): + data = "[1, 2, 3]" + with pytest.raises(ParseError) as exc: + common.parse_yaml_or_json(data, silent_failure=False) + message = str(exc.value) + assert "Cannot parse as" in message + assert "Input type `list` is not a dictionary" in message + + def test_set_environ(): key = str(uuid4()) old_environ = os.environ.copy() diff --git a/awx/main/utils/common.py b/awx/main/utils/common.py index 87779041d2..902621ca62 100644 --- a/awx/main/utils/common.py +++ b/awx/main/utils/common.py @@ -582,12 +582,25 @@ def cache_list_capabilities(page, prefetch_list, model, user): obj.capabilities_cache[display_method] = True +def validate_vars_type(vars_obj): + if not isinstance(vars_obj, dict): + vars_type = type(vars_obj) + if hasattr(vars_type, '__name__'): + data_type = vars_type.__name__ + else: + data_type = str(vars_type) + raise AssertionError( + _('Input type `{data_type}` is not a dictionary').format( + data_type=data_type) + ) + + def parse_yaml_or_json(vars_str, silent_failure=True): ''' 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. + is True, re-raise combination error if `silent_failure` if False. ''' if isinstance(vars_str, dict): return vars_str @@ -596,18 +609,21 @@ def parse_yaml_or_json(vars_str, silent_failure=True): try: vars_dict = json.loads(vars_str) - assert isinstance(vars_dict, dict) - except (ValueError, TypeError, AssertionError): + validate_vars_type(vars_dict) + except (ValueError, TypeError, AssertionError) as json_err: 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): + validate_vars_type(vars_dict) + except (yaml.YAMLError, TypeError, AttributeError, AssertionError) as yaml_err: if silent_failure: return {} - raise + raise ParseError(_( + 'Cannot parse as JSON (error: {json_error}) or ' + 'YAML (error: {yaml_error}).').format( + json_error=str(json_err), yaml_error=str(yaml_err))) return vars_dict diff --git a/awx/main/validators.py b/awx/main/validators.py index ce1c5b73b5..5af078a861 100644 --- a/awx/main/validators.py +++ b/awx/main/validators.py @@ -11,6 +11,7 @@ from django.core.exceptions import ValidationError # REST framework from rest_framework.serializers import ValidationError as RestValidationError +from rest_framework.exceptions import ParseError # AWX from awx.main.utils import parse_yaml_or_json @@ -183,6 +184,6 @@ def vars_validate_or_raise(vars_str): try: parse_yaml_or_json(vars_str, silent_failure=False) return vars_str - except: - raise RestValidationError(_('Must be valid JSON or YAML.')) + except ParseError as e: + raise RestValidationError(str(e))