From f3591b81a72f9e683dce609f1272de7c687943dc Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Thu, 4 May 2017 10:46:04 -0400 Subject: [PATCH 1/2] more multicredential JobTemplate changes * allow for filtering Jobs and JobTemplates by v1 `cloud_credential` and `network_credential` fields * properly validate uniqueness of `extra_credentials` types see: #5807 --- awx/api/filters.py | 5 ++ awx/api/views.py | 13 ++-- awx/main/models/jobs.py | 2 +- awx/main/tests/functional/api/test_job.py | 72 +------------------ .../tests/functional/api/test_job_template.py | 60 +++++++++++++++- 5 files changed, 73 insertions(+), 79 deletions(-) diff --git a/awx/api/filters.py b/awx/api/filters.py index aa617910c4..0a243f9d33 100644 --- a/awx/api/filters.py +++ b/awx/api/filters.py @@ -265,6 +265,11 @@ class FieldLookupBackend(BaseFilterBackend): key = key[5:] q_not = True + # Make legacy v1 Job/Template fields work for backwards compatability + # TODO: remove after API v1 deprecation period + if queryset.model._meta.object_name in ('JobTemplate', 'Job') and key in ('cloud_credential', 'network_credential'): + key = 'extra_credentials' + # Make legacy v1 Credential fields work for backwards compatability # TODO: remove after API v1 deprecation period # diff --git a/awx/api/views.py b/awx/api/views.py index debfea42ae..a2e902c8b9 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2704,6 +2704,12 @@ class JobTemplateExtraCredentialsList(SubListCreateAttachDetachAPIView): new_in_api_v2 = True def is_valid_relation(self, parent, sub, created=False): + current_extra_types = [ + cred.credential_type.pk for cred in parent.extra_credentials.all() + ] + if sub.credential_type.pk in current_extra_types: + return {'error': _('Cannot assign multiple %s credentials.' % sub.credential_type.name)} + if sub.credential_type.kind not in ('net', 'cloud'): return {'error': _('Extra credentials must be network or cloud.')} return super(JobTemplateExtraCredentialsList, self).is_valid_relation(parent, sub, created) @@ -3470,7 +3476,7 @@ class JobDetail(RetrieveUpdateDestroyAPIView): return super(JobDetail, self).destroy(request, *args, **kwargs) -class JobExtraCredentialsList(SubListCreateAttachDetachAPIView): +class JobExtraCredentialsList(SubListAPIView): model = Credential serializer_class = CredentialSerializer @@ -3479,11 +3485,6 @@ class JobExtraCredentialsList(SubListCreateAttachDetachAPIView): new_in_320 = True new_in_api_v2 = True - def is_valid_relation(self, parent, sub, created=False): - if sub.credential_type.kind not in ('net', 'cloud'): - return {'error': _('Extra credentials must be network or cloud.')} - return super(JobExtraCredentialsList, self).is_valid_relation(parent, sub, created) - class JobLabelList(SubListAPIView): diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index 40f4c22e9a..d99b3f4a10 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -181,7 +181,7 @@ class JobOptions(BaseModel): @property def all_credentials(self): - credentials = self.extra_credentials.all() + credentials = list(self.extra_credentials.all()) if self.vault_credential: credentials.insert(0, self.vault_credential) if self.credential: diff --git a/awx/main/tests/functional/api/test_job.py b/awx/main/tests/functional/api/test_job.py index 0ca4fcf13b..828ac89c54 100644 --- a/awx/main/tests/functional/api/test_job.py +++ b/awx/main/tests/functional/api/test_job.py @@ -5,52 +5,7 @@ from awx.api.versioning import reverse # TODO: test this with RBAC and lower-priveleged users @pytest.mark.django_db -def test_extra_credential_creation(get, post, organization_factory, job_template_factory, credentialtype_aws): - objs = organization_factory("org", superusers=['admin']) - jt = job_template_factory("jt", organization=objs.organization, - inventory='test_inv', project='test_proj').job_template - job = jt.create_unified_job() - - url = reverse('api:job_extra_credentials_list', kwargs={'version': 'v2', 'pk': job.pk}) - response = post(url, { - 'name': 'My Cred', - 'credential_type': credentialtype_aws.pk, - 'inputs': { - 'username': 'bob', - 'password': 'secret', - } - }, objs.superusers.admin) - assert response.status_code == 201 - - response = get(url, user=objs.superusers.admin) - assert response.data.get('count') == 1 - - -# TODO: test this with RBAC and lower-priveleged users -@pytest.mark.django_db -def test_attach_extra_credential(get, post, organization_factory, job_template_factory, credential): - objs = organization_factory("org", superusers=['admin']) - jt = job_template_factory("jt", organization=objs.organization, - inventory='test_inv', project='test_proj').job_template - job = jt.create_unified_job() - - url = reverse('api:job_extra_credentials_list', kwargs={'version': 'v2', 'pk': job.pk}) - response = get(url, user=objs.superusers.admin) - assert response.data.get('count') == 0 - - response = post(url, { - 'associate': True, - 'id': credential.id, - }, objs.superusers.admin) - assert response.status_code == 204 - - response = get(url, user=objs.superusers.admin) - assert response.data.get('count') == 1 - - -# TODO: test this with RBAC and lower-priveleged users -@pytest.mark.django_db -def test_detach_extra_credential(get, post, organization_factory, job_template_factory, credential): +def test_extra_credentials(get, organization_factory, job_template_factory, credential): objs = organization_factory("org", superusers=['admin']) jt = job_template_factory("jt", organization=objs.organization, inventory='test_inv', project='test_proj').job_template @@ -61,28 +16,3 @@ def test_detach_extra_credential(get, post, organization_factory, job_template_f url = reverse('api:job_extra_credentials_list', kwargs={'version': 'v2', 'pk': job.pk}) response = get(url, user=objs.superusers.admin) assert response.data.get('count') == 1 - - response = post(url, { - 'disassociate': True, - 'id': credential.id, - }, objs.superusers.admin) - assert response.status_code == 204 - - response = get(url, user=objs.superusers.admin) - assert response.data.get('count') == 0 - - -@pytest.mark.django_db -def test_attach_extra_credential_wrong_kind_xfail(get, post, organization_factory, job_template_factory, machine_credential): - """Extra credentials only allow net + cloud credentials""" - objs = organization_factory("org", superusers=['admin']) - jt = job_template_factory("jt", organization=objs.organization, - inventory='test_inv', project='test_proj').job_template - job = jt.create_unified_job() - - url = reverse('api:job_extra_credentials_list', kwargs={'version': 'v2', 'pk': job.pk}) - response = post(url, { - 'associate': True, - 'id': machine_credential.id, - }, objs.superusers.admin) - assert response.status_code == 400 diff --git a/awx/main/tests/functional/api/test_job_template.py b/awx/main/tests/functional/api/test_job_template.py index 5cd43bb4b0..1289e0f8ee 100644 --- a/awx/main/tests/functional/api/test_job_template.py +++ b/awx/main/tests/functional/api/test_job_template.py @@ -58,6 +58,42 @@ def test_extra_credential_creation(get, post, organization_factory, job_template assert response.data.get('count') == 1 +@pytest.mark.django_db +def test_extra_credential_unique_type_xfail(get, post, organization_factory, job_template_factory, credentialtype_aws): + objs = organization_factory("org", superusers=['admin']) + jt = job_template_factory("jt", organization=objs.organization, + inventory='test_inv', project='test_proj').job_template + + url = reverse('api:job_template_extra_credentials_list', kwargs={'version': 'v2', 'pk': jt.pk}) + response = post(url, { + 'name': 'My Cred', + 'credential_type': credentialtype_aws.pk, + 'inputs': { + 'username': 'bob', + 'password': 'secret', + } + }, objs.superusers.admin) + assert response.status_code == 201 + + response = get(url, user=objs.superusers.admin) + assert response.data.get('count') == 1 + + # this request should fail because you can't assign the same type (aws) + # twice + response = post(url, { + 'name': 'My Cred', + 'credential_type': credentialtype_aws.pk, + 'inputs': { + 'username': 'joe', + 'password': 'another-secret', + } + }, objs.superusers.admin) + assert response.status_code == 400 + + response = get(url, user=objs.superusers.admin) + assert response.data.get('count') == 1 + + # TODO: test this with RBAC and lower-priveleged users @pytest.mark.django_db def test_attach_extra_credential(get, post, organization_factory, job_template_factory, credential): @@ -130,7 +166,7 @@ def test_v1_extra_credentials_detail(get, organization_factory, job_template_fac @pytest.mark.django_db -def test_v1_set_extra_credentials(get, patch, organization_factory, job_template_factory, credential, net_credential): +def test_v1_set_extra_credentials_assignment(get, patch, organization_factory, job_template_factory, credential, net_credential): objs = organization_factory("org", superusers=['admin']) jt = job_template_factory("jt", organization=objs.organization, inventory='test_inv', project='test_proj').job_template @@ -163,6 +199,28 @@ def test_v1_set_extra_credentials(get, patch, organization_factory, job_template assert response.data.get('network_credential') is None +@pytest.mark.django_db +def test_filter_by_v1(get, organization_factory, job_template_factory, credential, net_credential): + objs = organization_factory("org", superusers=['admin']) + jt = job_template_factory("jt", organization=objs.organization, + inventory='test_inv', project='test_proj').job_template + jt.extra_credentials.add(credential) + jt.extra_credentials.add(net_credential) + jt.save() + + for query in ( + ('cloud_credential', str(credential.pk)), + ('network_credential', str(net_credential.pk)) + ): + url = reverse('api:job_template_list', kwargs={'version': 'v1'}) + response = get( + url, + user=objs.superusers.admin, + QUERY_STRING='='.join(query) + ) + assert response.data.get('count') == 1 + + @pytest.mark.django_db @pytest.mark.parametrize( "grant_project, grant_credential, grant_inventory, expect", [ From ef09744b75048ff8d1eff198ec3c9b69212faf19 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Thu, 4 May 2017 11:51:37 -0400 Subject: [PATCH 2/2] enforce variable name syntax and uniqueness for Credential Types see: #6158 --- awx/main/fields.py | 20 ++++++++++++++++++-- awx/main/tests/functional/test_credential.py | 2 ++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/awx/main/fields.py b/awx/main/fields.py index a492932b43..ca4fbc64c8 100644 --- a/awx/main/fields.py +++ b/awx/main/fields.py @@ -363,6 +363,8 @@ class JSONSchemaField(JSONBField): super(JSONSchemaField, self).validate(value, model_instance) errors = [] for error in Draft4Validator(self.schema(model_instance)).iter_errors(value): + if error.validator == 'pattern' and 'error' in error.schema: + error.message = error.schema['error'] % error.instance errors.append(error) if errors: @@ -471,7 +473,11 @@ class CredentialTypeInputField(JSONSchemaField): 'items': {'type': 'string'}, 'uniqueItems': True }, - 'id': {'type': 'string'}, + 'id': { + 'type': 'string', + 'pattern': '^[a-zA-Z_]+[a-zA-Z0-9_]*$', + 'error': '%s is an invalid variable name', + }, 'label': {'type': 'string'}, 'help_text': {'type': 'string'}, 'multiline': {'type': 'boolean'}, @@ -490,14 +496,24 @@ class CredentialTypeInputField(JSONSchemaField): value, model_instance ) + ids = {} for field in value.get('fields', []): - if field.get('id') == 'tower': + id_ = field.get('id') + if id_ == 'tower': raise django_exceptions.ValidationError( _('"tower" is a reserved field name'), code='invalid', params={'value': value}, ) + if id_ in ids: + raise django_exceptions.ValidationError( + _('field IDs must be unique (%s)' % id_), + code='invalid', + params={'value': value}, + ) + ids[id_] = True + class CredentialTypeInjectorField(JSONSchemaField): diff --git a/awx/main/tests/functional/test_credential.py b/awx/main/tests/functional/test_credential.py index 62e6c49bcc..b6ea847038 100644 --- a/awx/main/tests/functional/test_credential.py +++ b/awx/main/tests/functional/test_credential.py @@ -58,6 +58,8 @@ def test_cloud_kind_uniqueness(): ({'fields': [{'id': 'username', 'label': 'Username', 'type': 'string'}]}, True), ({'fields': [{'id': 'username', 'label': 'Username', 'help_text': 1}]}, False), ({'fields': [{'id': 'username', 'label': 'Username', 'help_text': 'Help Text'}]}, True), # noqa + ({'fields': [{'id': 'username', 'label': 'Username'}, {'id': 'username', 'label': 'Username 2'}]}, False), # noqa + ({'fields': [{'id': '$invalid$', 'label': 'Invalid'}]}, False), # noqa ({'fields': [{'id': 'password', 'label': 'Password', 'type': 'number'}]}, True), ({'fields': [{'id': 'ssh_key', 'label': 'SSH Key', 'type': 'ssh_private_key'}]}, True), # noqa ({'fields': [{'id': 'other', 'label': 'Other', 'type': 'boolean'}]}, False),