search

belgif / rest-guide

REST Guidelines of Belgian government institutions
https://www.belgif.be/specification/rest/api-guide/
Apache License 2.0
24 stars 4 forks source link

How strict to validate readOnly request properties #167

Closed JDMKSZ closed 4 months ago

JDMKSZ commented 7 months ago

The REST guide has a rule on the use of the 'readOnly' property that states:

[...] Properties can be declared readOnly: true. This means that it MAY be sent as part of a response but MUST NOT be sent as part of the request. [...]

Examples are properties that are computed from other properties, or that represent a volatile state of a resource.

OpenAPI V3 states

[...] Declares the property as "read only". This means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request. [...]

How strict do we want to be about validation of a request containing a readOnly property? In the "MUST NOT" case, the client must never do it, in the "SHOULD NOT" case it may be accepted.

E.g. if a PUT operation does an update containing a readOnly resource ID in the payload, is this request acceptable if it doesn't change it from the original value? Could be useful if payload was adapted from a previous GET response.

original issue

The REST guide has a rule on the use of the 'readOnly' property that states:

Properties SHOULD be declared readOnly when appropriate.

Properties can be declared readOnly: true. This means that it MAY be sent as part of a response but MUST NOT be sent as part of the request. Properties marked as readOnly being true SHOULD NOT be in the required list of the defined schema.

Examples are properties that are computed from other properties, or that represent a volatile state of a resource.

This rule seems to be based on the OpenAPI V2 spec:

Relevant only for Schema "properties" definitions. Declares the property as "read only". This means that it MAY be sent as part of a response but MUST NOT be sent as part of the request. Properties marked as readOnly being true SHOULD NOT be in the required list of the defined schema. Default value is false.

However, in OpenAPI V3, this has changed to:

Relevant only for Schema "properties" definitions. Declares the property as "read only". This means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request. If the property is marked as readOnly being true and is in the required list, the required will take effect on the response only. A property MUST NOT be marked as both readOnly and writeOnly being true. Default value is false.

Therefore, I would like to change the Belgif guide to:

Properties SHOULD be declared readOnly when appropriate.

Properties can be declared readOnly: true. This means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request. If the property is marked as readOnly being true and is in the required list, the required will take effect on the response only. A property MUST NOT be marked as both readOnly and writeOnly being true.

Examples are properties that are computed from other properties, or that represent a volatile state of a resource.

This change also brings up the question: what about validation of a request containing a read-only property? In the "MUST NOT" case, the request should be refused (Bad Request). However, in the "SHOULD NOT" case, I guess it should be accepted unless it is different from the original value?

jpraet commented 7 months ago

Deviation for not allowing readOnly + required is intentional: see https://github.com/belgif/rest-guide/issues/143 and https://github.com/belgif/rest-guide/pull/156

pvdbosch commented 7 months ago

closing as duplicate

JDMKSZ commented 7 months ago

@pvdbosch : only the readOnly+required part is a duplicate, no? What about just changing the MUST NOT to SHOULD NOT?

pvdbosch commented 7 months ago

Reopening the issue with updated issue description, so it's just focused on how to validate requests that contain readOnly properties.

The "SHOULD NOT be sent as part of a request" wording from OpenAPI 3.0 is a ambiguous: clients shouldn't send them or else ...? It doesn't say if or when such a request is valid.

The newer version of the JSON Schema standard used by OpenAPI 3.1 is clearer:

If "readOnly" has a value of boolean true, it indicates that the value of the instance is managed exclusively by the owning authority, and attempts by an application to modify the value of this property are expected to be ignored or rejected by that owning authority.

An instance document that is marked as "readOnly" for the entire document MAY be ignored if sent to the owning authority, or MAY result in an error, at the authority's discretion.

[...]

For example, "readOnly" would be used to mark a database-generated serial number as read-only, while "writeOnly" would be used to mark a password input field.

A verification if a readOnly property was modified, can only be done by an API's specific backend logic, i.e. not by a generic validator comparing only the request against the OpenAPI. If we want to adopt this OpenAPI 3.1 spec as guideline, we should make sure the validators used in API Gateways (like the atlassian one), are not rejecting readOnly request properties.

JDMKSZ commented 7 months ago

Personally, I prefer the "SHOULD" in combination with the JSON Schema standard explanation "...are expected to be ignored or rejected by that owning authority". This leaves the freedom to either reject or ignore the property. As you point out, our tools and/or platforms might all react differently, and we don't have this behavior under control, so it would be hard to enforce this. Also, it allows a different reaction depending on the situation, which might be necessary in some cases?

clone1612 commented 7 months ago

If we want to adopt this OpenAPI 3.1 spec as guideline, we should make sure the validators used in API Gateways (like the atlassian one), are not rejecting readOnly request properties.

Even though the Atlassian features page indicate the properties are supported it doesn't actually validate them.

Looking at this issue it's unclear if that's intended, debugging I discovered it's a side-effect of the underlying json-schema-validator which only supports validation of JSON schemas up to draft-04, while readOnly was only added in draft-07.

pvdbosch commented 7 months ago

REST WG is okay to change the guideline to the OpenAPI 3.1 spec behavior (i.e. property is managed by the authority). I'll amend the existing PR #156

pvdbosch commented 7 months ago

Added to PR #156, ready for review next WG. Upon re-reading, we're now a bit stricter than OpenAPI 3.1: we're forbidding properties being readOnly and required, while OpenAPI 3.1 does allow it. If a property is readOnly and required under OpenAPI 3.1, it must still be present in requests, but risks being rejected or ignored if its value is different from the one managed by the server (i.e. owning authority).

We might also consider allowing it, and formulate it as:

This would allow a schema with a required "id" to be used for both PUT requests as for GET responses if clients fill in correct "id". However, there would still be a problem to reuse that schema when creating a new resource, bc the client would be required to fill in a dummy value that the server has to ignore.

pvdbosch commented 4 months ago

PR is merged. It's kept a bit stricter than OAS 3.1 (see previous comment) in order to be compatible with both OpenAPI 3.0 and 3.1.