From a4873d97d8122df3646b2dfbdddc5524d4a1502f Mon Sep 17 00:00:00 2001
From: Jeff Bradberry <jeff.bradberry@gmail.com>
Date: Tue, 24 Sep 2019 17:27:19 -0400
Subject: [PATCH] Raise a validation error if a credential is set while the
 service is not

---
 awx/api/serializers.py                        | 12 ++--
 awx/main/models/mixins.py                     |  2 +-
 .../tests/functional/api/test_webhooks.py     | 56 +++++++++++++++++++
 3 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/awx/api/serializers.py b/awx/api/serializers.py
index 8355a6a3a6..4d60f9f660 100644
--- a/awx/api/serializers.py
+++ b/awx/api/serializers.py
@@ -2836,11 +2836,13 @@ class JobTemplateMixin(object):
                 'webhook_credential': _("Must be a Personal Access Token."),
             })
 
-        if webhook_service and webhook_credential:
-            if webhook_credential.kind != '{}_token'.format(webhook_service):
-                raise serializers.ValidationError({
-                    'webhook_credential': _("Must match the selected webhook service."),
-                })
+        if webhook_credential:
+            msg = {'webhook_credential': _("Must match the selected webhook service.")}
+            if webhook_service:
+                if webhook_credential.kind != '{}_token'.format(webhook_service):
+                    raise serializers.ValidationError(msg)
+            else:
+                raise serializers.ValidationError(msg)
 
         return super().validate(attrs)
 
diff --git a/awx/main/models/mixins.py b/awx/main/models/mixins.py
index 9e52d56d2b..ff499c4f66 100644
--- a/awx/main/models/mixins.py
+++ b/awx/main/models/mixins.py
@@ -530,7 +530,7 @@ class WebhookTemplateMixin(models.Model):
                 self.webhook_key = ''
 
             if update_fields and 'webhook_service' in update_fields:
-                update_fields.append('webhook_key')
+                update_fields.add('webhook_key')
 
         super().save(*args, **kwargs)
 
diff --git a/awx/main/tests/functional/api/test_webhooks.py b/awx/main/tests/functional/api/test_webhooks.py
index 20be437b89..971ea22b4f 100644
--- a/awx/main/tests/functional/api/test_webhooks.py
+++ b/awx/main/tests/functional/api/test_webhooks.py
@@ -202,3 +202,59 @@ def test_set_wrong_service_webhook_credential(organization_factory, job_template
     assert jt.webhook_key != ''
     assert jt.webhook_credential is None
     assert response.data == {'webhook_credential': ["Must match the selected webhook service."]}
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "service", [s for s, _ in WebhookTemplateMixin.SERVICES]
+)
+def test_set_webhook_credential_without_service(organization_factory, job_template_factory, patch, service):
+    objs = organization_factory("org", superusers=['admin'])
+    jt = job_template_factory("jt", organization=objs.organization,
+                              inventory='test_inv', project='test_proj').job_template
+    admin = objs.superusers.admin
+    assert jt.webhook_service == ''
+    assert jt.webhook_key == ''
+
+    cred_type = CredentialType.defaults['{}_token'.format(service)]()
+    cred_type.save()
+    cred = Credential.objects.create(credential_type=cred_type, name='test-cred',
+                                     inputs={'token': 'secret'})
+
+    url = reverse('api:job_template_detail', kwargs={'pk': jt.pk})
+    response = patch(url, {'webhook_credential': cred.pk}, user=admin, expect=400)
+    jt.refresh_from_db()
+
+    assert jt.webhook_service == ''
+    assert jt.webhook_key == ''
+    assert jt.webhook_credential is None
+    assert response.data == {'webhook_credential': ["Must match the selected webhook service."]}
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "service", [s for s, _ in WebhookTemplateMixin.SERVICES]
+)
+def test_unset_webhook_service_with_credential(organization_factory, job_template_factory, patch, service):
+    objs = organization_factory("org", superusers=['admin'])
+    jt = job_template_factory("jt", organization=objs.organization, webhook_service=service,
+                              inventory='test_inv', project='test_proj').job_template
+    admin = objs.superusers.admin
+    assert jt.webhook_service == service
+    assert jt.webhook_key != ''
+
+    cred_type = CredentialType.defaults['{}_token'.format(service)]()
+    cred_type.save()
+    cred = Credential.objects.create(credential_type=cred_type, name='test-cred',
+                                     inputs={'token': 'secret'})
+    jt.webhook_credential = cred
+    jt.save()
+
+    url = reverse('api:job_template_detail', kwargs={'pk': jt.pk})
+    response = patch(url, {'webhook_service': ''}, user=admin, expect=400)
+    jt.refresh_from_db()
+
+    assert jt.webhook_service == service
+    assert jt.webhook_key != ''
+    assert jt.webhook_credential == cred
+    assert response.data == {'webhook_credential': ["Must match the selected webhook service."]}