From 1f290ed940db5b1945cb0af4e5754fb01c183f3c Mon Sep 17 00:00:00 2001 From: Chris Church Date: Tue, 9 Feb 2016 00:13:09 -0500 Subject: [PATCH] Add custom serializer char/bool fields to accept null and coerce to appropriate type. Update validation for unique constraints so that error messages remain the same as before. Update key used in error response for non-field errors. Should address #791, #794, #809, #812 and #816. --- awx/api/fields.py | 80 +++++++++++++++++++++ awx/api/serializers.py | 106 ++++++++++++---------------- awx/main/tests/old/organizations.py | 4 +- awx/main/tests/old/projects.py | 28 ++++++-- awx/main/tests/old/users.py | 2 + awx/settings/defaults.py | 1 + 6 files changed, 153 insertions(+), 68 deletions(-) create mode 100644 awx/api/fields.py diff --git a/awx/api/fields.py b/awx/api/fields.py new file mode 100644 index 0000000000..6e4922fbcb --- /dev/null +++ b/awx/api/fields.py @@ -0,0 +1,80 @@ +# Copyright (c) 2016 Ansible, Inc. +# All Rights Reserved. + +# Django +from django.utils.encoding import force_text + +# Django REST Framework +from rest_framework import serializers + +__all__ = ['BooleanNullField', 'CharNullField', 'ChoiceNullField', 'EncryptedPasswordField'] + + +class NullFieldMixin(object): + ''' + Mixin to prevent shortcutting validation when we want to allow null input, + but coerce the resulting value to another type. + ''' + + def validate_empty_values(self, data): + (is_empty_value, data) = super(NullFieldMixin, self).validate_empty_values(data) + if is_empty_value and data is None: + return (False, data) + return (is_empty_value, data) + + +class BooleanNullField(NullFieldMixin, serializers.NullBooleanField): + ''' + Custom boolean field that allows null and empty string as False values. + ''' + + def to_internal_value(self, data): + return bool(super(BooleanNullField, self).to_internal_value(data)) + + +class CharNullField(NullFieldMixin, serializers.CharField): + ''' + Custom char field that allows null as input and coerces to an empty string. + ''' + + def __init__(self, **kwargs): + kwargs['allow_null'] = True + super(CharNullField, self).__init__(**kwargs) + + def to_internal_value(self, data): + return super(CharNullField, self).to_internal_value(data or u'') + + +class ChoiceNullField(NullFieldMixin, serializers.ChoiceField): + ''' + Custom choice field that allows null as input and coerces to an empty string. + ''' + + def __init__(self, **kwargs): + kwargs['allow_null'] = True + super(ChoiceNullField, self).__init__(**kwargs) + + def to_internal_value(self, data): + return super(ChoiceNullField, self).to_internal_value(data or u'') + + +class EncryptedPasswordField(CharNullField): + ''' + Custom field to handle encrypted password values (on credentials). + ''' + + def to_internal_value(self, data): + value = super(EncryptedPasswordField, self).to_internal_value(data or u'') + # If user submits a value starting with $encrypted$, ignore it. + if force_text(value).startswith('$encrypted$'): + raise serializers.SkipField + return value + + def to_representation(self, value): + # Replace the actual encrypted value with the string $encrypted$. + if force_text(value).startswith('$encrypted$'): + return '$encrypted$' + return value + + + \ No newline at end of file diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 49dd88c4f0..81c7ef5c8e 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -24,11 +24,13 @@ from django.core.exceptions import ObjectDoesNotExist, ValidationError as Django from django.db import models # from django.utils.translation import ugettext_lazy as _ from django.utils.encoding import force_text, smart_text +from django.utils.text import capfirst # Django REST Framework from rest_framework.exceptions import ValidationError from rest_framework import fields from rest_framework import serializers +from rest_framework import validators from rest_framework.utils.serializer_helpers import ReturnList # Django-Polymorphic @@ -42,6 +44,7 @@ from awx.main.redact import REPLACE_STR from awx.main.conf import tower_settings from awx.api.license import feature_enabled +from awx.api.fields import BooleanNullField, CharNullField, ChoiceNullField, EncryptedPasswordField from awx.fact.models import * # noqa @@ -202,11 +205,6 @@ class BaseSerializer(serializers.ModelSerializer): 'modified', 'name', 'description') summary_fields = () # FIXME: List of field names from this serializer that should be used when included as part of another's summary_fields. summarizable_fields = () # FIXME: List of field names on this serializer that should be included in summary_fields. - extra_kwargs = { - 'description': { - 'allow_null': True, - }, - } # add the URL and related resources type = serializers.SerializerMethodField() @@ -358,24 +356,50 @@ class BaseSerializer(serializers.ModelSerializer): # Pass model field default onto the serializer field if field is not read-only. if model_field.has_default() and not field_kwargs.get('read_only', False): - field_kwargs['default'] = model_field.get_default() + field_kwargs['default'] = field_kwargs['initial'] = model_field.get_default() + # Enforce minimum value of 0 for PositiveIntegerFields. if isinstance(model_field, (models.PositiveIntegerField, models.PositiveSmallIntegerField)) and 'choices' not in field_kwargs: field_kwargs['min_value'] = 0 + + # Use custom boolean field that allows null and empty string as False values. + if isinstance(model_field, models.BooleanField) and not field_kwargs.get('read_only', False): + field_class = BooleanNullField + + # Use custom char or choice field that coerces null to an empty string. + if isinstance(model_field, (models.CharField, models.TextField)) and not field_kwargs.get('read_only', False): + if 'choices' in field_kwargs: + field_class = ChoiceNullField + else: + field_class = CharNullField + # Update verbosity choices from settings (for job templates, jobs, ad hoc commands). if field_name == 'verbosity' and 'choices' in field_kwargs: field_kwargs['choices'] = getattr(settings, 'VERBOSITY_CHOICES', field_kwargs['choices']) + + # Update the message used for the unique validator to use capitalized + # verbose name; keeps unique message the same as with DRF 2.x. + for validator in field_kwargs.get('validators', []): + if isinstance(validator, validators.UniqueValidator): + unique_error_message = model_field.error_messages.get('unique', None) + if unique_error_message: + unique_error_message = unique_error_message % { + 'model_name': capfirst(opts.verbose_name), + 'field_label': capfirst(model_field.verbose_name), + } + validator.message = unique_error_message + return field_class, field_kwargs def build_relational_field(self, field_name, relation_info): field_class, field_kwargs = super(BaseSerializer, self).build_relational_field(field_name, relation_info) - # Don't include choicse for foreign key fields. + # Don't include choices for foreign key fields. field_kwargs.pop('choices', None) return field_class, field_kwargs - def validate_description(self, value): - # Description should always be empty string, never null. - return value or u'' + def get_unique_together_validators(self): + # Allow the model's full_clean method to handle the unique together validation. + return [] def run_validation(self, data=fields.empty): try: @@ -653,7 +677,6 @@ class UserSerializer(BaseSerializer): fields = ('*', '-name', '-description', '-modified', '-summary_fields', 'username', 'first_name', 'last_name', 'email', 'is_superuser', 'password', 'ldap_dn') - def to_representation(self, obj): ret = super(UserSerializer, self).to_representation(obj) @@ -772,17 +795,9 @@ class OrganizationSerializer(BaseSerializer): class ProjectOptionsSerializer(BaseSerializer): - scm_clean = serializers.NullBooleanField(default=False) - scm_delete_on_update = serializers.NullBooleanField(default=False) - class Meta: fields = ('*', 'local_path', 'scm_type', 'scm_url', 'scm_branch', 'scm_clean', 'scm_delete_on_update', 'credential') - extra_kwargs = { - 'scm_type': { - 'allow_null': True, - }, - } def get_related(self, obj): res = super(ProjectOptionsSerializer, self).get_related(obj) @@ -791,15 +806,6 @@ class ProjectOptionsSerializer(BaseSerializer): args=(obj.credential.pk,)) return res - def validate_scm_type(self, value): - return value or u'' - - def validate_scm_clean(self, value): - return bool(value) - - def validate_scm_delete_on_update(self, value): - return bool(value) - def validate(self, attrs): errors = {} @@ -831,9 +837,7 @@ class ProjectOptionsSerializer(BaseSerializer): class ProjectSerializer(UnifiedJobTemplateSerializer, ProjectOptionsSerializer): - scm_delete_on_next_update = serializers.BooleanField(read_only=True) - scm_update_on_launch = serializers.NullBooleanField(default=False) - status = serializers.ChoiceField(choices=Project.PROJECT_STATUS_CHOICES, read_only=True, required=False) + status = serializers.ChoiceField(choices=Project.PROJECT_STATUS_CHOICES, read_only=True) last_update_failed = serializers.BooleanField(read_only=True) last_updated = serializers.DateTimeField(read_only=True) @@ -842,9 +846,7 @@ class ProjectSerializer(UnifiedJobTemplateSerializer, ProjectOptionsSerializer): fields = ('*', 'scm_delete_on_next_update', 'scm_update_on_launch', 'scm_update_cache_timeout') + \ ('last_update_failed', 'last_updated') # Backwards compatibility - - def clean_scm_update_on_launch(self, value): - return bool(value) + read_only_fields = ('scm_delete_on_next_update',) def get_related(self, obj): res = super(ProjectSerializer, self).get_related(obj) @@ -1216,14 +1218,6 @@ class InventorySourceOptionsSerializer(BaseSerializer): class Meta: fields = ('*', 'source', 'source_path', 'source_script', 'source_vars', 'credential', 'source_regions', 'instance_filters', 'group_by', 'overwrite', 'overwrite_vars') - extra_kwargs = { - 'instance_filters': { - 'allow_null': True, - }, - 'group_by': { - 'allow_null': True, - }, - } def get_related(self, obj): res = super(InventorySourceOptionsSerializer, self).get_related(obj) @@ -1281,7 +1275,7 @@ class InventorySourceOptionsSerializer(BaseSerializer): class InventorySourceSerializer(UnifiedJobTemplateSerializer, InventorySourceOptionsSerializer): - status = serializers.ChoiceField(choices=InventorySource.INVENTORY_SOURCE_STATUS_CHOICES, read_only=True, required=False) + status = serializers.ChoiceField(choices=InventorySource.INVENTORY_SOURCE_STATUS_CHOICES, read_only=True) last_update_failed = serializers.BooleanField(read_only=True) last_updated = serializers.DateTimeField(read_only=True) @@ -1440,14 +1434,7 @@ class PermissionSerializer(BaseSerializer): class CredentialSerializer(BaseSerializer): - # FIXME: may want to make some of these filtered based on user accessing - - password = serializers.CharField(required=False, default='') - security_token = serializers.CharField(required=False, default='') - ssh_key_data = serializers.CharField(required=False, default='') - ssh_key_unlock = serializers.CharField(required=False, default='') - become_password = serializers.CharField(required=False, default='') - vault_password = serializers.CharField(required=False, default='') + # FIXME: may want to make some fields filtered based on user accessing class Meta: model = Credential @@ -1456,24 +1443,23 @@ class CredentialSerializer(BaseSerializer): 'become_method', 'become_username', 'become_password', 'vault_password') + def build_standard_field(self, field_name, model_field): + field_class, field_kwargs = super(CredentialSerializer, self).build_standard_field(field_name, model_field) + if field_name in Credential.PASSWORD_FIELDS: + field_class = EncryptedPasswordField + field_kwargs['required'] = False + field_kwargs['default'] = '' + return field_class, field_kwargs + def to_representation(self, obj): ret = super(CredentialSerializer, self).to_representation(obj) if obj is not None and 'user' in ret and (not obj.user or not obj.user.is_active): ret['user'] = None if obj is not None and 'team' in ret and (not obj.team or not obj.team.active): ret['team'] = None - # Replace the actual encrypted value with the string $encrypted$. - for field in Credential.PASSWORD_FIELDS: - if field in ret and force_text(ret[field]).startswith('$encrypted$'): - ret[field] = '$encrypted$' return ret def validate(self, attrs): - # If the value sent to the API startswith $encrypted$, ignore it. - for field in Credential.PASSWORD_FIELDS: - if force_text(attrs.get(field, '')).startswith('$encrypted$'): - attrs.pop(field, None) - # If creating a credential from a view that automatically sets the # parent_key (user or team), set the other value to None. view = self.context.get('view', None) diff --git a/awx/main/tests/old/organizations.py b/awx/main/tests/old/organizations.py index cf3c64ab87..b3d84a5da4 100644 --- a/awx/main/tests/old/organizations.py +++ b/awx/main/tests/old/organizations.py @@ -262,7 +262,9 @@ class OrganizationsTest(BaseTest): data1 = self.post(self.collection(), new_org, expect=201, auth=self.get_super_credentials()) # duplicate post results in 400 - self.post(self.collection(), new_org, expect=400, auth=self.get_super_credentials()) + response = self.post(self.collection(), new_org, expect=400, auth=self.get_super_credentials()) + self.assertTrue('name' in response, response) + self.assertTrue('Name' in response['name'][0], response) # look at what we got back from the post, make sure we added an org last_org = Organization.objects.order_by('-pk')[0] diff --git a/awx/main/tests/old/projects.py b/awx/main/tests/old/projects.py index b0a32ad6a5..45c96ca587 100644 --- a/awx/main/tests/old/projects.py +++ b/awx/main/tests/old/projects.py @@ -231,12 +231,19 @@ class ProjectsTest(BaseTransactionTest): 'description': 'Does amazing things', 'local_path': os.path.basename(project_dir), 'scm_type': None, + 'scm_update_on_launch': '', + 'scm_delete_on_update': None, + 'scm_clean': False, } # Adding a project with scm_type=None should work, but scm_type will be - # changed to an empty string. + # changed to an empty string. Other boolean fields should accept null + # or an empty string for False, but save the value as a boolean. response = self.post(projects, project_data, expect=201, auth=self.get_super_credentials()) self.assertEqual(response['scm_type'], u'') + self.assertEqual(response['scm_update_on_launch'], False) + self.assertEqual(response['scm_delete_on_update'], False) + self.assertEqual(response['scm_clean'], False) # can edit project using same local path. project_detail = reverse('api:project_detail', args=(response['id'],)) @@ -494,7 +501,9 @@ class ProjectsTest(BaseTransactionTest): ssh_key_data = TEST_SSH_KEY_DATA_LOCKED, ssh_key_unlock = TEST_SSH_KEY_DATA_UNLOCK, ssh_password = 'narf', - sudo_password = 'troz' + sudo_password = 'troz', + security_token = '', + vault_password = None, ) # can add credentials to a user (if user or org admin or super user) @@ -561,13 +570,17 @@ class ProjectsTest(BaseTransactionTest): # Repeating the same POST should violate a unique constraint. with self.current_user(self.super_django_user): data = dict(name='xyz', user=self.super_django_user.pk) - self.post(url, data, expect=400) + response = self.post(url, data, expect=400) + self.assertTrue('__all__' in response, response) + self.assertTrue('already exists' in response['__all__'][0], response) - # Test with null where we expect a string value. + # Test with null where we expect a string value. Value will be coerced + # to an empty string. with self.current_user(self.super_django_user): data = dict(name='zyx', user=self.super_django_user.pk, kind='ssh', become_username=None) - self.post(url, data, expect=400) + response = self.post(url, data, expect=201) + self.assertEqual(response['become_username'], '') # Test with encrypted ssh key and no unlock password. with self.current_user(self.super_django_user): @@ -698,14 +711,15 @@ class ProjectsTest(BaseTransactionTest): # user=user.pk, # no need to specify, this will be automatically filled in inventory=inventory.pk, project=project.pk, - permission_type=PERM_INVENTORY_DEPLOY + permission_type=PERM_INVENTORY_DEPLOY, + run_ad_hoc_commands=None, ) team_permission = dict( name='team can deploy a certain project to a certain inventory', # team=team.pk, # no need to specify, this will be automatically filled in inventory=inventory.pk, project=project.pk, - permission_type=PERM_INVENTORY_DEPLOY + permission_type=PERM_INVENTORY_DEPLOY, ) url = reverse('api:user_permissions_list', args=(user.pk,)) diff --git a/awx/main/tests/old/users.py b/awx/main/tests/old/users.py index c4ec3a87f1..42285ff588 100644 --- a/awx/main/tests/old/users.py +++ b/awx/main/tests/old/users.py @@ -207,6 +207,8 @@ class UsersTest(BaseTest): self.post(url, expect=403, data=new_super_user, auth=self.get_other_credentials()) self.post(url, expect=403, data=new_super_user, auth=self.get_normal_credentials()) self.post(url, expect=201, data=new_super_user, auth=self.get_super_credentials()) + new_super_user2 = dict(username='nommy2', password='cookie', is_superuser=None) + self.post(url, expect=201, data=new_super_user2, auth=self.get_super_credentials()) def test_auth_token_login(self): auth_token_url = reverse('api:auth_token_view') diff --git a/awx/settings/defaults.py b/awx/settings/defaults.py index d8d1fbc9fc..36f39ac3ec 100644 --- a/awx/settings/defaults.py +++ b/awx/settings/defaults.py @@ -226,6 +226,7 @@ REST_FRAMEWORK = { 'EXCEPTION_HANDLER': 'awx.api.views.api_exception_handler', 'VIEW_NAME_FUNCTION': 'awx.api.generics.get_view_name', 'VIEW_DESCRIPTION_FUNCTION': 'awx.api.generics.get_view_description', + 'NON_FIELD_ERRORS_KEY': '__all__', } AUTHENTICATION_BACKENDS = (