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

Expose the tileset and the SQL queries in dev mode #772

Merged
merged 4 commits into from
Sep 6, 2023
Merged

Conversation

bchapuis
Copy link
Member

@bchapuis bchapuis commented Sep 4, 2023

Solves #766.


/**
* A resource that provides access to the tileset.
* A resource that provides access to the tileset file. Only suitable for development purposes, as
* it exposes SQL queries.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC it exposes the database JDBC URL with credentials as well. Basically anything consumed by baremaps for its internal ends up deserialized in Tileset.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Perdjesk Good point, I just hid the jdbc url. We need to find a good balance between security and usability for the dev mode

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a draft implementation that reverse the logic in order to allow fields instead of having to disallow them after the facts.
#773

Motivation is similar to what was done in https://github.com/apache/incubator-baremaps/pull/658/files#diff-94ecab9df3004fae4fd05c3c610d8ef629f81cb775f72a53a19070c0293bc27cL62 to remove hiding of field with conditions after de-serialization, which could be error prone for newly added field in tileset.json.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Perdjesk Thank you for the draft, I understand the motivation. However, I would prefer to keep things as simple and small as possible for now, i.e., to expose the tileset in development and the tileJSON in production.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my answer was vague, let's add a bit of context. First, our implementation of tileJSON is partial and I think that adding an extension to the model will slow us down. Second, dev modes are usually insecure and I think it is reasonable to assume that our users will not use it in production. Finally, an earlier version of the dev mode offered a maputnik integration and allowed the user to edit the style. I have not completely abandonned this idea (see #561) and we may introduce an API to edit local files when in dev mode (the security aspects associated with this API still need to be discussed).

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bchapuis bchapuis merged commit be499e3 into main Sep 6, 2023
@bchapuis bchapuis deleted the 766-tileset branch September 6, 2023 06:53
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.

2 participants