Add feature flags to control query sharing, KV exposure #9120

willbarrett · 2020-02-11T19:30:18Z

SUMMARY

Preset would like query sharing to abide by the rules of query visibility used in the rest of the project. To accomplish this, we have updated the Share Query button to leverage the saved query functionality rather than the KV model, with a feature flag should other organizations want to maintain the current functionality. We have also added a feature flag to disable the /kv endpoints entirely. They appear to be used only for query sharing inside of the Superset project, but we do not know how these are leveraged in other systems.

Note that following the link to the saved query will open the saved query in SQLLab, rather than the current behavior, which is to open a copy of the saved query in SQLLab.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

"Save Query" toast after the change:
With an unsaved query:

With a saved query:

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

@robdiciuccio @etr2460 @suddjian @dpgaspar

suddjian · 2020-02-11T21:30:45Z

superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx

@@ -45,9 +47,19 @@ class ShareSqlLabQuery extends React.Component {
      shortUrl: t('Loading ...'),
    };
    this.getCopyUrl = this.getCopyUrl.bind(this);
+    this.getCopyUrlForSavedQuery = this.getCopyUrlForSavedQuery.bind(this);
+    this.getCopyUrlForKvStore = this.getCopyUrlForKvStore.bind(this);


.bind(this) is only necessary when you are going to be passing this.getCopyUrlForKvStore around as a separate variable. If it's only called via this.getCopyUrlForKvStore() then this will be bound correctly and these lines are unnecessary.

I'll try removing them - I had to add them at one point to get things functioning.

suddjian

Looks good - very minor clarity nit

etr2460 · 2020-02-12T22:58:28Z

This seems like a pretty major product/functionality change as far as completely removing the concept of one-off query sharing (i'm pretty sure the kv endpoints are also used for one-off chart sharing too). What tradeoffs were considered when thinking about this feature removal? I personally make use of the query and chart sharing features all the time.

cc @sylvia-tomiyama for her take too

willbarrett · 2020-02-12T23:01:10Z

@etr2460 the unsecured/unvalidated/unowned /kv/ endpoints are unacceptable from a security standpoint to Preset. We've provided feature flags so that other organizations can continue using the system as-is. The Cartel designs have a different model entirely for sharing, so we view this fix as a stop-gap in advance of that work.

etr2460 · 2020-02-12T23:04:30Z

Could you expand a bit on the insecure nature of the /kv endpoints? It was my understanding that they'd let you view a query/chart form data, but actually running the query would still be dependent on the security manager granting you access to the base level datasources. They seem not unlike the google doc link sharing feature, and if the predictable nature of the urls is an issue then simply using a random key would resolve that issue.

willbarrett · 2020-02-12T23:09:25Z

@etr2460 I'll expand directly with you in Slack. I don't wish to have a detailed security-related conversation publicly.

etr2460 · 2020-02-13T16:54:17Z

The summary of the conversation was to add some documentation about how to reenable the feature if desired and update the UPDATING.md file

willbarrett · 2020-02-13T23:02:59Z

@etr2460 let me know if that entry in UPDATING.md fits your needs or if there's somewhere else you'd like me to drop a note as well.

etr2460

a couple requests, looks good otherwise

UPDATING.md

etr2460 · 2020-02-14T01:24:53Z

tests/core_tests.py

@@ -497,6 +504,9 @@ def test_shortner(self):
        resp = self.client.post("/r/shortner/", data=dict(data=data))
        assert re.search(r"\/r\/[0-9]+", resp.data.decode("utf-8"))

+    @skipUnless(


These should always be run in our test environment whether the flag is enabled or disabled. Can you set the feature flag to on before running them?

@etr2460 I spent an hour banging my head against it, and was unable to find an approach to modify feature flags after they were initialized without breaking the whole suite. I'm very open to recommendations.

Could enabling this feature on just for the tests be a viable approach? superset_test_config.py

In this case it would be, since the feature flag only removes functionality and doesn't add anything different. that said, I think we should be able to test both sides of the feature flag here, do you know how to do this @dpgaspar ?

Maybe mocking the feature flag checking function?

The issue in this case is that the application is booted once for the test suite and the feature flag changes the nature of the instantiation. This is a boot-time flag, not a run-time check. For this PR I'll go ahead and enable it for tests.

codecov-io · 2020-02-14T18:23:45Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e5c7adf). Click here to learn what that means.
The diff coverage is 50%.

@@            Coverage Diff            @@
##             master    #9120   +/-   ##
=========================================
  Coverage          ?   59.08%           
=========================================
  Files             ?      372           
  Lines             ?    11933           
  Branches          ?     2921           
=========================================
  Hits              ?     7051           
  Misses            ?     4700           
  Partials          ?      182

Impacted Files	Coverage Δ
...frontend/src/dashboard/components/MissingChart.jsx	`100% <ø> (ø)`
superset-frontend/src/chart/chartAction.js	`43.33% <50%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5c7adf...dbe24cb. Read the comment docs.

dpgaspar

LGTM, currently has conflicts with master

…est environment

etr2460

lgtm, thanks for the documentation!

Add feature flags to control query sharing, KV exposure

adbe239

superset-github-bot bot added the preset-io label Feb 11, 2020

pull-request-size bot added the size/M label Feb 11, 2020

Add tests, fix bug

2b4d1c9

willbarrett requested a review from etr2460 February 11, 2020 21:07

pull-request-size bot added size/L and removed size/M labels Feb 11, 2020

willbarrett requested a review from dpgaspar February 11, 2020 21:08

willbarrett marked this pull request as ready for review February 11, 2020 21:08

Skip test for kv endpoints when they are disabled

11bded6

suddjian reviewed Feb 11, 2020

View reviewed changes

suddjian approved these changes Feb 11, 2020

View reviewed changes

Will Barrett added 3 commits February 12, 2020 09:00

ESLint fixes

d097f7b

Remove unnecessary binds

e7b3c70

Fix eslint errors

f1e683b

Add note to UPDATING.md RE: new feature flag options

52585c2

etr2460 requested changes Feb 14, 2020

View reviewed changes

Use expanded version of RBAC

ea37902

dpgaspar approved these changes Feb 18, 2020

View reviewed changes

dpgaspar reviewed Feb 18, 2020

View reviewed changes

Will Barrett added 3 commits February 18, 2020 12:32

Enable KV_STORE and SHARE_QUERIES_VIA_KV_STORE feature flags in the t…

e5c7adf

…est environment

Merge branch 'master' into wbarrett/disable-kv-store-sharing

3163318

Fix black

dbe24cb

willbarrett requested a review from etr2460 February 18, 2020 23:32

robdiciuccio mentioned this pull request Feb 19, 2020

[SIP-30] Remove Tabs in SQL Lab #8655

Closed

etr2460 approved these changes Feb 19, 2020

View reviewed changes

craig-rueda approved these changes Feb 19, 2020

View reviewed changes

craig-rueda merged commit 38f3fd0 into apache:master Feb 19, 2020

willbarrett deleted the wbarrett/disable-kv-store-sharing branch February 19, 2020 18:09

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature flags to control query sharing, KV exposure #9120

Add feature flags to control query sharing, KV exposure #9120

willbarrett commented Feb 11, 2020 •

edited

Loading

suddjian Feb 11, 2020 •

edited

Loading

willbarrett Feb 11, 2020

suddjian left a comment

etr2460 commented Feb 12, 2020

willbarrett commented Feb 12, 2020

etr2460 commented Feb 12, 2020

willbarrett commented Feb 12, 2020

etr2460 commented Feb 13, 2020

willbarrett commented Feb 13, 2020

etr2460 left a comment

etr2460 Feb 14, 2020

willbarrett Feb 14, 2020

dpgaspar Feb 18, 2020

etr2460 Feb 18, 2020

willbarrett Feb 18, 2020

codecov-io commented Feb 14, 2020 •

edited

Loading

dpgaspar left a comment

etr2460 left a comment

Add feature flags to control query sharing, KV exposure #9120

Add feature flags to control query sharing, KV exposure #9120

Conversation

willbarrett commented Feb 11, 2020 • edited Loading

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

suddjian Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

willbarrett Feb 11, 2020

Choose a reason for hiding this comment

suddjian left a comment

Choose a reason for hiding this comment

etr2460 commented Feb 12, 2020

willbarrett commented Feb 12, 2020

etr2460 commented Feb 12, 2020

willbarrett commented Feb 12, 2020

etr2460 commented Feb 13, 2020

willbarrett commented Feb 13, 2020

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Feb 14, 2020

Choose a reason for hiding this comment

willbarrett Feb 14, 2020

Choose a reason for hiding this comment

dpgaspar Feb 18, 2020

Choose a reason for hiding this comment

etr2460 Feb 18, 2020

Choose a reason for hiding this comment

willbarrett Feb 18, 2020

Choose a reason for hiding this comment

codecov-io commented Feb 14, 2020 • edited Loading

Codecov Report

dpgaspar left a comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

willbarrett commented Feb 11, 2020 •

edited

Loading

suddjian Feb 11, 2020 •

edited

Loading

codecov-io commented Feb 14, 2020 •

edited

Loading