From 097c4505814a8a12d73ba5a45c9f86458ed05e5c Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Wed, 4 May 2016 10:39:12 -0400 Subject: [PATCH 1/4] new serializer to remove owner fields from credential detail views --- awx/api/serializers.py | 53 ++++++++++++++++++++++++++---------------- awx/api/views.py | 2 +- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index d679029057..c6d7b4f016 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -1583,18 +1583,6 @@ class ResourceAccessListElementSerializer(UserSerializer): class CredentialSerializer(BaseSerializer): # FIXME: may want to make some fields filtered based on user accessing - user = serializers.PrimaryKeyRelatedField( - queryset=User.objects.all(), required=False, default=None, write_only=True, - help_text='Write-only field used to add user to owner role. If provided, ' - 'do not give either team or organization. Only valid for creation.') - team = serializers.PrimaryKeyRelatedField( - queryset=Team.objects.all(), required=False, default=None, write_only=True, - help_text='Write-only field used to add team to owner role. If provided, ' - 'do not give either user or organization. Only valid for creation.') - organization = serializers.PrimaryKeyRelatedField( - queryset=Organization.objects.all(), required=False, default=None, write_only=True, - help_text='Write-only field used to add organization to owner role. If provided, ' - 'do not give either team or team. Only valid for creation.') class Meta: model = Credential @@ -1603,14 +1591,7 @@ class CredentialSerializer(BaseSerializer): 'ssh_key_data', 'ssh_key_unlock', 'become_method', 'become_username', 'become_password', 'vault_password', 'subscription', 'tenant', 'secret', 'client', - 'authorize', 'authorize_password', - 'user', 'team', 'organization') - - def create(self, validated_data): - # Remove the user, team, and organization processed in view - for field in ['user', 'team', 'organization']: - validated_data.pop(field, None) - return super(CredentialSerializer, self).create(validated_data) + 'authorize', 'authorize_password') def build_standard_field(self, field_name, model_field): field_class, field_kwargs = super(CredentialSerializer, self).build_standard_field(field_name, model_field) @@ -1647,6 +1628,38 @@ class CredentialSerializer(BaseSerializer): return summary_dict +class CredentialSerializerCreate(CredentialSerializer): + + user = serializers.PrimaryKeyRelatedField( + queryset=User.objects.all(), required=False, default=None, write_only=True, + help_text='Write-only field used to add user to owner role. If provided, ' + 'do not give either team or organization. Only valid for creation.') + team = serializers.PrimaryKeyRelatedField( + queryset=Team.objects.all(), required=False, default=None, write_only=True, + help_text='Write-only field used to add team to owner role. If provided, ' + 'do not give either user or organization. Only valid for creation.') + organization = serializers.PrimaryKeyRelatedField( + queryset=Organization.objects.all(), required=False, default=None, write_only=True, + help_text='Write-only field used to add organization to owner role. If provided, ' + 'do not give either team or team. Only valid for creation.') + + class Meta: + model = Credential + fields = ('*', 'user', 'team', 'organization') + + def create(self, validated_data): + ''' + Special cases are processed for creating a credential. Fields of + user, team, and organization are allowed, and if given, roles are + automatically created for these. This is only used for list-create + view. + ''' + # Remove the user, team, and organization processed in view + for field in ['user', 'team', 'organization']: + validated_data.pop(field, None) + return super(CredentialSerializer, self).create(validated_data) + + class JobOptionsSerializer(BaseSerializer): class Meta: diff --git a/awx/api/views.py b/awx/api/views.py index aa476f18c6..8ed1c30aed 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -1228,7 +1228,7 @@ class UserAccessList(ResourceAccessList): class CredentialList(ListCreateAPIView): model = Credential - serializer_class = CredentialSerializer + serializer_class = CredentialSerializerCreate def post(self, request, *args, **kwargs): for field in [x for x in ['user', 'team', 'organization'] if x in request.data and request.data[x] in ('', None)]: From b1dfa28459d76f36da8706ea1e01506269e9df01 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Thu, 5 May 2016 14:20:36 -0400 Subject: [PATCH 2/4] hit the is_valid method before stripping the special fields in credential view --- awx/api/views.py | 5 +++++ awx/main/tests/functional/api/test_credential.py | 10 ++++++++++ 2 files changed, 15 insertions(+) diff --git a/awx/api/views.py b/awx/api/views.py index 8ed1c30aed..e943063b8a 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -1231,6 +1231,11 @@ class CredentialList(ListCreateAPIView): serializer_class = CredentialSerializerCreate def post(self, request, *args, **kwargs): + + # Check the validity of POST data, including special fields + serializer = self.get_serializer(data=request.data) + serializer.is_valid(raise_exception=True) + for field in [x for x in ['user', 'team', 'organization'] if x in request.data and request.data[x] in ('', None)]: request.data.pop(field) kwargs.pop(field, None) diff --git a/awx/main/tests/functional/api/test_credential.py b/awx/main/tests/functional/api/test_credential.py index fffab6f1a0..165f3a1547 100644 --- a/awx/main/tests/functional/api/test_credential.py +++ b/awx/main/tests/functional/api/test_credential.py @@ -21,6 +21,16 @@ def test_create_user_credential_via_credentials_list(post, get, alice): assert response.status_code == 200 assert response.data['count'] == 1 +@pytest.mark.django_db +def test_credential_validation_error_with_bad_user(post, alice): + response = post(reverse('api:credential_list'), { + 'user': 'asdf', + 'name': 'Some name', + 'username': 'someusername' + }, alice) + assert response.status_code == 403 + assert response.data['detail'] == 'You do not have permission to perform this action.' + @pytest.mark.django_db def test_create_user_credential_via_user_credentials_list(post, get, alice): response = post(reverse('api:user_credentials_list', args=(alice.pk,)), { From 3d26506c8edbec3253f1fbc63e99bc48dabb1279 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Mon, 9 May 2016 11:34:49 -0400 Subject: [PATCH 3/4] allow blank credential owner fields --- awx/api/serializers.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index c6d7b4f016..5e5d1b6934 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -1631,15 +1631,18 @@ class CredentialSerializer(BaseSerializer): class CredentialSerializerCreate(CredentialSerializer): user = serializers.PrimaryKeyRelatedField( - queryset=User.objects.all(), required=False, default=None, write_only=True, + queryset=User.objects.all(), + required=False, default=None, write_only=True, allow_null=True, help_text='Write-only field used to add user to owner role. If provided, ' 'do not give either team or organization. Only valid for creation.') team = serializers.PrimaryKeyRelatedField( - queryset=Team.objects.all(), required=False, default=None, write_only=True, + queryset=Team.objects.all(), + required=False, default=None, write_only=True, allow_null=True, help_text='Write-only field used to add team to owner role. If provided, ' 'do not give either user or organization. Only valid for creation.') organization = serializers.PrimaryKeyRelatedField( - queryset=Organization.objects.all(), required=False, default=None, write_only=True, + queryset=Organization.objects.all(), + required=False, default=None, write_only=True, allow_null=True, help_text='Write-only field used to add organization to owner role. If provided, ' 'do not give either team or team. Only valid for creation.') From f9c177edd5d6ed58eb5fc6460a1f9adf4281dc51 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Tue, 10 May 2016 09:35:13 -0400 Subject: [PATCH 4/4] More accurate test to check validation of bad data --- awx/api/serializers.py | 6 ------ awx/main/tests/functional/api/test_credential.py | 8 ++++---- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 5e5d1b6934..7a81a06def 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -1651,12 +1651,6 @@ class CredentialSerializerCreate(CredentialSerializer): fields = ('*', 'user', 'team', 'organization') def create(self, validated_data): - ''' - Special cases are processed for creating a credential. Fields of - user, team, and organization are allowed, and if given, roles are - automatically created for these. This is only used for list-create - view. - ''' # Remove the user, team, and organization processed in view for field in ['user', 'team', 'organization']: validated_data.pop(field, None) diff --git a/awx/main/tests/functional/api/test_credential.py b/awx/main/tests/functional/api/test_credential.py index 165f3a1547..a272328b34 100644 --- a/awx/main/tests/functional/api/test_credential.py +++ b/awx/main/tests/functional/api/test_credential.py @@ -22,14 +22,14 @@ def test_create_user_credential_via_credentials_list(post, get, alice): assert response.data['count'] == 1 @pytest.mark.django_db -def test_credential_validation_error_with_bad_user(post, alice): +def test_credential_validation_error_with_bad_user(post, admin): response = post(reverse('api:credential_list'), { 'user': 'asdf', 'name': 'Some name', 'username': 'someusername' - }, alice) - assert response.status_code == 403 - assert response.data['detail'] == 'You do not have permission to perform this action.' + }, admin) + assert response.status_code == 400 + assert response.data['user'][0] == 'Incorrect type. Expected pk value, received unicode.' @pytest.mark.django_db def test_create_user_credential_via_user_credentials_list(post, get, alice):