Event subscription template - alignment with OWASP and other linting rules#591
Event subscription template - alignment with OWASP and other linting rules#591rartych wants to merge 8 commits intocamaraproject:mainfrom
Conversation
PedroDiez
left a comment
There was a problem hiding this comment.
Minor editorial comments
LGTM
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
| schema: | ||
| type: array | ||
| minItems: 0 | ||
| maxItems: 1000 |
There was a problem hiding this comment.
Just wondering: What is happening when this maxItems limit is reached? Is it recommended, that one API invoker starts using multiple client Ids, when more than 1000 subscriptions are used?
There was a problem hiding this comment.
It is fair point. In fact it is the limitation in the response listing all subscriptions.
From security and efficiency perspective response pagination should be implemented.
There was a problem hiding this comment.
@rartych thanks for the clarification.
Pagination seems to be natively only supported in a later OpanAPI specification version.
Maybe the simplest is to add some text into the yaml file, that the API specification should provide support for pagination, when more subscriptions are expected than the max array limit.
There was a problem hiding this comment.
We have this section about pagination in CAMARA API design guide: https://github.com/camaraproject/Commonalities/blob/main/documentation/CAMARA-API-Design-Guide.md#41-pagination
We can add the pagination support in the template (page, perPage) and response headers and document that API initiatives can dismiss it based on their Use Cases. In that way, we ensure an homogeneous way for API initiatives that need/adopt it:
In GET /subscriptions:
REQUEST
...
parameters:
- $ref: "#/components/parameters/x-correlator"
- $ref: "#/components/parameters/Page"
- $ref: "#/components/parameters/PerPage"
...
parameters:
...
name: page
in: query
description: Requested index to indicate the start of the resources/subscriptions to be provided in the response
schema:
type: integer
default: 1
PerPage:
name: perPage
in: query
description: Requested number of resources/subscriptions to be provided in response
schema:
type: integer
default: 10
...RESPONSE:
responses:
"200":
description: OK
headers:
...
Content-Last-Key:
$ref: "#/components/headers/Content-Last-Key"
X-Total-Count:
$ref: "#/components/headers/X-Total-Count"
...
headers:
...
Content-Last-Key:
description: Indicates the index of the last result provided in the response
schema:
type: integer
X-Total-Count:
description: Total number of items matching criteria
schema:
type: integer
...
tlohmar
left a comment
There was a problem hiding this comment.
Looks generally good. Some questions.
|
|
||
| UpdateReason: | ||
| type: string | ||
| maxLength: 512 |
There was a problem hiding this comment.
This looks redundant, as this is an enum type.
|
|
||
| TerminationReason: | ||
| type: string | ||
| maxLength: 512 |
There was a problem hiding this comment.
Also here, redundant as an enum
What type of PR is this?
What this PR does / why we need it:
CAMARA_common.yaml includes schema definitions that raise linter errors when used in CAMARA API specifications.
New OWASP API linting rules will raise additional erros related to unrestricted string/integer properties.
This PR:
Which issue(s) this PR fixes:
Fixes #585
Does this PR introduce a breaking change?
Special notes for reviewers:
What is reasonable value for maximal number of subscriptions returned to API consumer for
GET /subscriptions?Should we think about pagination?
The values for not used transports (like MQTT, Kafka) can be refined when we decide to implement them.
For now some values are proposed, so the template file conforms to linting rules.
Changelog input
Additional documentation
https://github.com/camaraproject/Commonalities/blob/main/documentation/Linting-rules.md#5-owasp-api-security-top-10-2023-rules