Merge pull request #2079 from AlanCoding/creds_no_op

Allow no-op case when modifying deprecated credentials

awx/api/serializers.py

@@ -2092,14 +2092,14 @@ class InventorySourceSerializer(UnifiedJobTemplateSerializer, InventorySourceOpt
     def _update_deprecated_fields(self, fields, obj):
         if 'credential' in fields:
             new_cred = fields['credential']
-            existing_creds = obj.credentials.exclude(credential_type__kind='vault')
-            for cred in existing_creds:
-                # Remove all other cloud credentials
-                if cred != new_cred:
+            existing = obj.credentials.all()
+            if new_cred not in existing:
+                for cred in existing:
+                    # Remove all other cloud credentials
                     obj.credentials.remove(cred)
-            if new_cred:
-                # Add new credential
-                obj.credentials.add(new_cred)
+                if new_cred:
+                    # Add new credential
+                    obj.credentials.add(new_cred)
 
     def validate(self, attrs):
         deprecated_fields = {}
@@ -2889,11 +2889,12 @@ class JobOptionsSerializer(LabelsListMixin, BaseSerializer):
             ('network_credential', obj.network_credentials),
         ):
             if key in fields:
-                for cred in existing:
-                    obj.credentials.remove(cred)
-                if fields[key]:
-                    obj.credentials.add(fields[key])
-        obj.save()
+                new_cred = fields[key]
+                if new_cred not in existing:
+                    for cred in existing:
+                        obj.credentials.remove(cred)
+                    if new_cred:
+                        obj.credentials.add(new_cred)
 
     def validate(self, attrs):
         v1_credentials = {}
@@ -3772,10 +3773,13 @@ class WorkflowJobTemplateNodeSerializer(LaunchConfigurationBaseSerializer):
             deprecated_fields['credential'] = validated_data.pop('credential')
         obj = super(WorkflowJobTemplateNodeSerializer, self).update(obj, validated_data)
         if 'credential' in deprecated_fields:
-            for cred in obj.credentials.filter(credential_type__kind='ssh'):
-                obj.credentials.remove(cred)
-            if deprecated_fields['credential']:
-                obj.credentials.add(deprecated_fields['credential'])
+            existing = obj.credentials.filter(credential_type__kind='ssh')
+            new_cred = deprecated_fields['credential']
+            if new_cred not in existing:
+                for cred in existing:
+                    obj.credentials.remove(cred)
+                if new_cred:
+                    obj.credentials.add(new_cred)
         return obj
 
 

awx/main/tests/functional/api/test_deprecated_credential_assignment.py

@@ -394,3 +394,43 @@ def test_inventory_source_invalid_deprecated_credential(patch, admin, ec2_source
     url = reverse('api:inventory_source_detail', kwargs={'pk': ec2_source.pk})
     resp = patch(url, {'credential': 999999}, admin, expect=400)
     assert 'Credential 999999 does not exist' in resp.content
+
+
+@pytest.mark.django_db
+def test_deprecated_credential_activity_stream(patch, admin_user, machine_credential, job_template):
+    job_template.credentials.add(machine_credential)
+    starting_entries = job_template.activitystream_set.count()
+    # no-op patch
+    patch(
+        job_template.get_absolute_url(),
+        admin_user,
+        data={'credential': machine_credential.pk},
+        expect=200
+    )
+    # no-op should not produce activity stream entries
+    assert starting_entries == job_template.activitystream_set.count()
+
+
+@pytest.mark.django_db
+def test_multi_vault_preserved_on_put(get, put, admin_user, job_template, vault_credential):
+    '''
+    A PUT request will necessarily specify deprecated fields, but if the deprecated
+    field is a singleton while the `credentials` relation has many, that makes
+    it very easy to drop those credentials not specified in the PUT data
+    '''
+    vault2 = Credential.objects.create(
+        name='second-vault',
+        credential_type=vault_credential.credential_type,
+        inputs={'vault_password': 'foo', 'vault_id': 'foo'}
+    )
+    job_template.credentials.add(vault_credential, vault2)
+    assert job_template.credentials.count() == 2  # sanity check
+    r = get(job_template.get_absolute_url(), admin_user, expect=200)
+    # should be a no-op PUT request
+    put(
+        job_template.get_absolute_url(),
+        admin_user,
+        data=r.data,
+        expect=200
+    )
+    assert job_template.credentials.count() == 2

awx/main/tests/functional/conftest.py

@@ -563,7 +563,9 @@ def _request(verb):
                                 response.data[key] = str(value)
                     except Exception:
                         response.data = data_copy
-            assert response.status_code == expect
+            assert response.status_code == expect, 'Response data: {}'.format(
+                getattr(response, 'data', None)
+            )
         if hasattr(response, 'render'):
             response.render()
         __SWAGGER_REQUESTS__.setdefault(request.path, {})[

docs/multi_credential_assignment.md

@@ -93,9 +93,17 @@ as they did before:
 
 `PATCH /api/v2/job_templates/N/ {'credential': X, 'vault_credential': Y}`
 
-Under this model, when a JobTemplate with multiple vault Credentials is updated
-in this way, the new underlying list will _only_ contain the single Vault
-Credential specified in the deprecated request.
+If the job template (with pk=N) only has 1 vault credential,
+that will be replaced with the new `Y` vault credential.
+
+If the job template has multiple vault credentials, and these do not include
+`Y`, then the new list will _only_ contain the single vault credential `Y`
+specified in the deprecated request.
+
+If the JobTemplate already has the `Y` vault credential associated with it,
+then no change will take effect (the other vault credentials will not be
+removed in this case). This is so that clients making deprecated requests
+do not interfere with clients using the new `credentials` relation.
 
 `GET` requests to `/api/v2/job_templates/N/` and `/api/v2/jobs/N/`
 have traditionally included a variety of metadata in the response via