diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 2376956da5d6..3037daf69b39 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -2038,14 +2038,14 @@ def update(self, obj, validated_data): 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 = {} @@ -2834,11 +2834,12 @@ def _update_deprecated_fields(self, fields, obj): ('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 = {} @@ -3675,10 +3676,13 @@ def update(self, obj, validated_data): # TODO: remove when v2 API is deprecated 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 diff --git a/awx/main/tests/functional/api/test_deprecated_credential_assignment.py b/awx/main/tests/functional/api/test_deprecated_credential_assignment.py index f6466affd785..8dbb3a391cab 100644 --- a/awx/main/tests/functional/api/test_deprecated_credential_assignment.py +++ b/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 diff --git a/awx/main/tests/functional/conftest.py b/awx/main/tests/functional/conftest.py index 28d7b65564ca..cb12f1584314 100644 --- a/awx/main/tests/functional/conftest.py +++ b/awx/main/tests/functional/conftest.py @@ -553,7 +553,9 @@ def rf(url, data_or_user=None, user=None, middleware=None, expect=None, **kwargs 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, {})[ diff --git a/docs/multi_credential_assignment.md b/docs/multi_credential_assignment.md index f32203860ca3..3fbb4874287e 100644 --- a/docs/multi_credential_assignment.md +++ b/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