Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement OGC API Features Part 4 transactions for Senosorthings provider #1911

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

webb-ben
Copy link
Member

Overview

  • Introduce simple OAFeat Part 4 transactions delegating entity validation to downstream SensorThings endpoint.
  • Add graceful failure to incorrectly configured STA collection

Related Issue / discussion

Additional information

Dependency policy (RFC2)

  • I have ensured that this PR meets RFC2 requirements

Updates to public demo

Contributions and licensing

(as per https://github.com/geopython/pygeoapi/blob/master/CONTRIBUTING.md#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to pygeoapi. I confirm that my contributions to pygeoapi will be compatible with the pygeoapi license guidelines at the time of contribution
  • I have already previously agreed to the pygeoapi Contributions and Licensing Guidelines

@webb-ben webb-ben requested a review from a team January 22, 2025 17:09
@francbartoli
Copy link
Contributor

Thanks @webb-ben for this nice feature.

I'd like to raise a discussion about the first point, in particular the validation and delegation part since I do believe it applies to other providers.
I'm a bit reluctant about the delegation:

  • pygeoapi offers an API contract to the consumers and in my opinion it's part of the contract interface to make sure it is respected and the payload received is valid. Otherwise, I might think we are delegating part of the OAFeat specification to the provider since you have to hit that to establish if the entity is valid or not
  • from a security perspective this is a risk and very highly attack-prone because an invalid payload might be used to perform for instance DoS attacks (by skipping pygeoapi) to the target provider, being an HTTP server or even worse a database

Hope these points are valuable for a broader discussion with the whole pygeoapi community

@webb-ben
Copy link
Member Author

Thanks for the comment @francbartoli.

  • from a security perspective this is a risk and very highly attack-prone because an invalid payload might be used to perform for instance DoS attacks (by skipping pygeoapi) to the target provider, being an HTTP server or even worse a database

I agree that any time we interact with a provided payload, there is an inherent risk. That being said, it is important to recognize that the docs try to make it clear for both OAFeat Part 4: Transactions and the Admin API that access control is not provided by pygeoapi—we explicitly state not to enable either componentswithout another service to provide authentication/authorization in line with the specification's security considerations. Is your comment expressing a desire for such software to be readily deployable with pygeoapi, or something different? It might be worth moving such a conversation to its own issue.

  • pygeoapi offers an API contract to the consumers and in my opinion it's part of the contract interface to make sure it is respected and the payload received is valid. Otherwise, I might think we are delegating part of the OAFeat specification to the provider since you have to hit that to establish if the entity is valid or not

SensorThings is nearly impossible to validate entirely within pygeoapi because of how broad the specification is. I have used pygeoapi as OGC API - Features server for the default STA entity model, STAPlus, and WQIE STA. If we decide to validate against the default STA entity model, we would still face the issue that SensorThings servers might still reject a payload due to the absence of a related dependent entity - something we can only discover by contacting the STA endpoint.

Another wrinkle is that, depending on the configuration of a FROST-Server, a payload with a client-assigned @iot.id might be rejected if the server is configured to exclusively provision the @iot.id for an entity. Conversely, a payload without an @iot.id might be rejected if the server requires it to be provided by the client. Only by using the response from the transaction with the SensorThings endpoint can we determine whether the payload is considered received and valid or invalid and discarded.

@tomkralidis
Copy link
Member

Thanks for the comments. I think the shortest path would be to introduce a validator plugin concept, which would have base methods such as is_valid, validate(payload) and so on.

Then, in pygeoapi configuration, one would define, in their provider definition:

    providers:
        - type: record
          editable: true
          validator: path.to.module.MyValidator
          ...

The idea would be that pygeoapi.api...., on POST/PUT, would trigger validation if configured, before sending to a given data plugin (noting that, at all times, anyone can write their own data plugin with full functionality as needed).

I have been working on this concept and will issue an RFC/PR in the coming weeks for review and comment.

In the meantime, in the context of how pygeoapi currently handles transactions, I think this PR has enough consistency with the rest of the codebase for merge.

@webb-ben
Copy link
Member Author

@tomkralidis would that be validation against geoJSON? As it is implemented this pull request is handling the creation of SensorThings JSON which isn't always valid geojson. That being said +1 to a generic validator and potentially injection inspector.

@tomkralidis
Copy link
Member

A given validator could be against anything really and would be responsible for data validation etc., depending on the data type.

@tomkralidis tomkralidis added this to the 0.20.0 milestone Jan 23, 2025
@tomkralidis tomkralidis merged commit 86e5b78 into geopython:master Jan 23, 2025
4 checks passed
@webb-ben webb-ben deleted the sta-transactions branch January 23, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants