usnistgov / ACVP

Industry Working Group on Automated Cryptographic Algorithm Validation
https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
172 stars 66 forks source link

Updates to dependencies (and maybe other similar resources) do not allow for clearing fields #721

Closed AlexThurston closed 4 years ago

AlexThurston commented 5 years ago

environment Demo (but I suspect Prod as well)

testSessionId N/A

vsId N/A

Algorithm registration N/A

Endpoint in which the error is experienced PUT https://demo.acvts.nist.gov/acvp/v1/dependencies/{id}

Expected behaviour You cannot clear/remove fields when updating a dependency. Given you have a dependency with (for example) a SWID of "this is a swid", if I then try to update to remove the SWID put pushing a body as follows:

{
    "swid": ""
}

I get the response: Status Code: 400 Error: Incorrectly formatted JSON (1:33): Provided string did not match expected pattern.

However, if I push an update with the following body:

{
    "swid": " "
}

It is accepted. However, the field SWID is not removed. It (as you would expect) is set to a string with a single empty space.

What's additionally strange is that if after setting the SWID to a single space, I then make a subsequent update to another field, the SWID field is correctly then removed from the object returned when a GET is preformed.

I would have expected that I could clear a field by passing it an empty string in the body of the PUT and it would be correctly removed from the response of a GET.

smuellerDD commented 5 years ago

Am Freitag, 18. Oktober 2019, 17:51:14 CEST schrieb Alex Thurston:

Hi Alex,

environment Demo (but I suspect Prod as well)

testSessionId N/A

vsId N/A

Algorithm registration N/A

Endpoint in which the error is experienced PUT https://demo.acvts.nist.gov/acvp/v1/dependencies/{id}

Expected behaviour You cannot clear/remove fields when updating a dependency. Given you have a dependency with (for example) a SWID of "this is a swid", if I then try to update to remove the SWID put pushing a body as follows: ``` { "swid": "" }

Isn't PUT expected to have a complete JSON structure? So, just PUT of an individual keyword should not be sent, I guess.

I get the response: Status Code: 400 Error: Incorrectly formatted JSON (1:33): Provided string did not match expected pattern.

However, if I push an update with the following body:

{
    "swid": " "
}

It is accepted. However, the field SWID is not removed. It (as you would expect) is set to a string with a single empty space.

What's additionally strange is that if after setting the SWID to a single space, I then make a subsequent update to another field, the SWID field is correctly then removed from the object returned when a GET is preformed.

I would have expected that I could clear a field by passing it an empty string in the body of the PUT and it would be correctly removed from the response of a GET.

Ciao Stephan

AlexThurston commented 5 years ago

I didn't think there was any protocol rules for PUT and it having a complete structure. Generally, PUT are to be idempotent.

If the intention is that a PUT should have a complete structure then that is also not being enforced because my partial PUT bodies were being accepted and working correctly, ie, if I want to update only the name of a dependency, I am able to PUT with a body of:

{
    "name": "the new name"
}

It would also be strange that a PUT require the entire structure. There are - as I understand it - fields that are optional for a dependency (SWID, Family, Series, etc...). When you do a GET those fields aren't returned if they are unset or blank. I'd expect that GET to also return those fields if they were required in the PUT.

shaneshaffer commented 5 years ago

There are a number of issues in play here, some we can address in the short term, some that may be addressed in a future revision of the spec.

The current document is lacking in specificity when it comes to defining the request payload for update operations. The Vendor and Person updates are the most defined, describing how only changed properties should be included, omitted properties will not be modified, and a null value indicates a property is to be removed. This is not the classic RESTful PUT payload, which would require a complete object; it is more like a PATCH, specifically an RFC 7396 JSON Merge Patch. Whether the API should be altered to more accurately reflect the nature of the payloads is a subject of discussion for a future revision.

The remaining update request definitions don't really say anything about what they should look like. However they all have an example that clearly is not a complete object, and fits the description in the Vendor and Person request definitions. So while it is not explicitly stated, we must assume that the partial update style applies to all objects.

Under that assumption, the correct way to remove a swid from a Dependency, or any other property, would be to pass a null value.

At the moment however, our implementation doesn't handle removal correctly for any object type, whether a partial or full update is attempted. We are working on correcting this, starting with Dependencies.

Kritner commented 4 years ago

This should be corrected with the recent deploy to demo. to remove a property pass "propertyName": null along with your message body. Messages that don't include a property will assume that property is unchanged, save properties in the spec that specifically call out having to include the property each time.