Allow saved dashboards to be exported by users with execute_sql permission #144
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @simonw, any chance you could take a look at this soon? No worries if not, I know you're super busy, but this addresses a major use case/pain point for our use of the tool. If you can't get to it anytime soon that's okay, I think we might just maintain a fork with this patch applied. Also apologies the PR description is so long! I thought it'd be helpful to document my thought process and everything I'd tried, but maybe it ended up being kind of intimidating as a result... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #133, but in a funky way.
Originally, I thought this would be fairly straightforward: I started by simply removing the conditions for hiding the export buttons if the user was on a saved dashboard page, and tried to move on from there, poking and prodding at the codebase until it did what I wanted.
However, I quickly noticed that saved dashboard pages use a GET form, while dashboard index pages (the ones with custom SQL and fully exportable queries) use a POST form. It seemed like it might be a security hazard to allow for a GET request to potentially hold up the server for really long queries--I was imagining a CSRF that initiated a denial of service attack on a dashboard site, and though I'm not 100% sure it's possible, I wanted to play it safe--so I decided not to go the GET route.
I then learned that submit buttons actually support a
formmethodattribute which would allow the export buttons to convert the request to a POST if they were clicked, but then I faced the problem of embedding the CSRF token in the form only on POST requests (I didn't want it to be included in GET requests because that would make the URLs look gross). All of this led me down a bit of a rabbit hole and I started worrying about how much new logic this whole endeavor would introduce to the back-end, some of which might potentially accidentally introduce new security holes.Then I thought of a weird solution: what if I didn't add any new logic to the backend, and instead made the export buttons trigger some JS on the client-side that simply simulated a user entering an SQL query in the dashboard index page and exporting it? It turns out that this can be simulated via a properly constructed form, so that's what this PR does right now. As a result, my (possibly flawed) belief is that it can't introduce any new security holes in the back-end, because it only makes changes to the front-end.
Anyways, it's also possibly too clever for its own good, and could be brittle. And one major limitation is that it doesn't currently support dashboards with named parameters, though I don't think adding support for that would be hard. But I figured I'd submit what I've got so far and see what you think before moving forward, @simonw.