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

Allow Parameters on Public Dashboards #3659

Merged
merged 94 commits into from Jul 15, 2019

Conversation

Projects
None yet
8 participants
@rauchy
Copy link
Contributor

commented Mar 31, 2019

What type of PR is this? (check all applicable)

  • Feature

Description

This PR enables dashboards with safe parameters to be shared publicly.

Related Tickets & Documents

Towards #3284, #1081.

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@rauchy rauchy changed the base branch from allow-parameters-on-embeds to master Apr 2, 2019

rauchy added some commits Mar 19, 2019

change has_access and require_access signatures to work with the obje…
…cts that require access, instead of their groups
support executing queries using Query api_keys by instantiating an Ap…
…iUser that would be able to execute the specific query
show deprecation messages for ALLOW_PARAMETERS_IN_EMBEDS. Also, move
other message (email not verified) to use the same mechanism
change has_access and require_access signatures to work with the obje…
…cts that require access, instead of their groups

@rauchy rauchy force-pushed the allow-parameters-on-dashboards branch from a30c873 to 4098cf2 Jun 26, 2019

@rauchy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@arikfr

Aside from the code comments, based on our experience with visualization embeds it's worth adding the following tests:

  • Shared dashboard without parameters rendering
  • Shared dashboard with safe parameters rendering
  • Shared dashboard with un-safe parameters rendering

I've added your suggested tests in 4ad3787. The third test is a bit tricky, because you aren't supposed to be able to share a dashboard with unsafe parameters, so the test creates a dashboard without parameters, shares it, adds an unsafe parameter, logs out and visits the share link.

Currently - the result is that the dashboard can be viewed by an unauthenticated user with the link, and the widget with safe (or without any) parameter works, while the widget with an unsafe parameter errors. This raises a question - what should be the experience for this case? Halt the entire page or just the individual unsafe queries?

rauchy added some commits Jul 14, 2019

@rauchy rauchy requested a review from arikfr Jul 14, 2019

@arikfr

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

This raises a question - what should be the experience for this case? Halt the entire page or just the individual unsafe queries?

Current behavior sounds reasonable (as long as the error message is clear). We can consider hiding these widgets and showing only the safe ones. This also means that we can change the share dialog behavior to just warn about the fact that these widgets will not be shown. (no need to implement in this PR , though)

@rauchy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

as long as the error message is clear

Overridden the message in fd874ca, it is now:

This query contains potentially unsafe parameters and cannot be executed on a publicly shared dashboard.

@justinclift

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

As a generalisation, that wording sounds fine. 😄

Kind of wondering if it's feasible to be more specific with the potentially unsafe parameters wording fragment though. Something to convey "foo" is a text parameter, which we don't yet allow. But, not sure of better wording as a whole sentence. 😉

@rauchy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@justinclift I agree we should be more explanatory, but I’d focus the attention on the part which creates and shares the dashboard. Anyway this is an edge case where someone shares a safe dashboard and then makes it unsafe.

@rauchy rauchy merged commit 51d8131 into master Jul 15, 2019

15 of 18 checks passed

percy/redash 1 visual change needs review
Details
Header rules No header rules processed
Details
Pages changed 7 new files uploaded
Details
Datree Smart Policy Best Practices Verification
Details
Datree insights datreeio insights events
Details
Mixed content No mixed content detected
Details
Redirect rules 7 redirect rules processed
Details
WIP Ready for review
Details
build Workflow: build
Details
ci/circleci: backend-unit-tests Your tests passed on CircleCI!
Details
ci/circleci: frontend-e2e-tests Your tests passed on CircleCI!
Details
ci/circleci: frontend-lint Your tests passed on CircleCI!
Details
ci/circleci: frontend-unit-tests Your tests passed on CircleCI!
Details
ci/circleci: legacy-python-flake8-tests Your tests passed on CircleCI!
Details
ci/circleci: python-flake8-tests Your tests passed on CircleCI!
Details
codeclimate 1 fixed issue
Details
cypress: default-group 35 tests passed
Details
deploy/netlify Deploy preview ready!
Details

@rauchy rauchy deleted the allow-parameters-on-dashboards branch Jul 15, 2019

});
};

it.only('when there are no parameters', function () {

This comment has been minimized.

Copy link
@ranbena

ranbena Jul 17, 2019

Member

@rauchy a mistake?

This comment has been minimized.

Copy link
@gabrieldutra

gabrieldutra Jul 17, 2019

Member

Shouldn't CircleCI frontend-lint be warning those things? 🤔

This comment has been minimized.

Copy link
@gabrieldutra

This comment has been minimized.

Copy link
@ranbena

This comment has been minimized.

Copy link
@ranbena

ranbena Jul 17, 2019

Member

@rauchy do you wanna enable the other tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.