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

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

Merged
merged 11 commits into from
Feb 19, 2020

Conversation

willbarrett
Copy link
Member

@willbarrett willbarrett commented Feb 11, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

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:
Screen Shot 2020-02-11 at 11 27 04 AM

With a saved query:
Screen Shot 2020-02-11 at 11 26 52 AM

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@robdiciuccio @etr2460 @suddjian @dpgaspar

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 11, 2020
@willbarrett willbarrett marked this pull request as ready for review February 11, 2020 21:08
@@ -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);
Copy link
Member

@suddjian suddjian Feb 11, 2020

Choose a reason for hiding this comment

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

.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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@suddjian suddjian left a comment

Choose a reason for hiding this comment

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

Looks good - very minor clarity nit

@etr2460
Copy link
Member

etr2460 commented Feb 12, 2020

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
Copy link
Member Author

@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
Copy link
Member

etr2460 commented Feb 12, 2020

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
Copy link
Member Author

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

@etr2460
Copy link
Member

etr2460 commented Feb 13, 2020

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
Copy link
Member Author

@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.

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

a couple requests, looks good otherwise

UPDATING.md Outdated Show resolved Hide resolved
@@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

LGTM, currently has conflicts with master

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the documentation!

@craig-rueda craig-rueda merged commit 38f3fd0 into apache:master Feb 19, 2020
@willbarrett willbarrett deleted the wbarrett/disable-kv-store-sharing branch February 19, 2020 18:09
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants