Add feature flags to control query sharing, KV exposure

UPDATING.md

@@ -22,6 +22,12 @@ This file documents any backwards-incompatible changes in Superset and
 assists people when migrating to a new version.
 
 ## Next
+* [9120](https://github.com/apache/incubator-superset/pull/9120): Changes the default behavior of ad-hoc sharing of
+queries in SQLLab to one that links to the saved query rather than one that copies the query data into the KVStore
+model and links to the record there. This is a security-related change that makes SQLLab query
+sharing respect the existing RBAC. Should you wish to retain the existing behavior, set two feature flags:
+`"KV_STORE": True` will re-enable the `/kv/` and `/kv/store/` endpoints, and `"SHARE_QUERIES_VIA_KV_STORE": True`
+will tell the front-end to utilize them for query sharing.
 * [9046](https://github.com/apache/incubator-superset/pull/9046): Replaces `can_only_access_owned_queries` by
 `all_query_access` favoring a white list approach. Since a new permission is introduced use `superset init` 
 to create and associate it by default to the `Admin` role. Note that, by default, all non `Admin` users will

superset-frontend/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx

@@ -21,6 +21,7 @@ import configureStore from 'redux-mock-store';
 import thunk from 'redux-thunk';
 import { OverlayTrigger } from 'react-bootstrap';
 import fetchMock from 'fetch-mock';
+import * as featureFlags from 'src/featureFlags';
 import { shallow } from 'enzyme';
 
 import * as utils from '../../../src/utils/common';
@@ -29,8 +30,9 @@ import ShareSqlLabQuery from '../../../src/SqlLab/components/ShareSqlLabQuery';
 
 const mockStore = configureStore([thunk]);
 const store = mockStore();
+let isFeatureEnabledMock;
 
-describe('ShareSqlLabQuery', () => {
+describe('ShareSqlLabQuery via /kv/store', () => {
   const storeQueryUrl = 'glob:*/kv/store/';
   const storeQueryMockId = '123';
 
@@ -50,9 +52,18 @@ describe('ShareSqlLabQuery', () => {
       schema: 'query_schema',
       autorun: false,
       sql: 'SELECT * FROM ...',
+      remoteId: 999,
     },
   };
 
+  const storedQueryAttributes = {
+    dbId: 0,
+    title: 'query title',
+    schema: 'query_schema',
+    autorun: false,
+    sql: 'SELECT * FROM ...',
+  };
+
   function setup(overrideProps) {
     const wrapper = shallow(
       <ShareSqlLabQuery {...defaultProps} {...overrideProps} />,
@@ -64,51 +75,113 @@ describe('ShareSqlLabQuery', () => {
     return wrapper;
   }
 
-  it('renders an OverlayTrigger with Button', () => {
-    const wrapper = setup();
-    const trigger = wrapper.find(OverlayTrigger);
-    const button = trigger.find(Button);
+  describe('via /kv/store', () => {
+    beforeAll(() => {
+      isFeatureEnabledMock = jest
+        .spyOn(featureFlags, 'isFeatureEnabled')
+        .mockImplementation(() => true);
+    });
 
-    expect(trigger).toHaveLength(1);
-    expect(button).toHaveLength(1);
-  });
+    afterAll(() => {
+      isFeatureEnabledMock.restore();
+    });
 
-  it('calls storeQuery() with the query when getCopyUrl() is called and saves the url', () => {
-    expect.assertions(4);
-    const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
+    it('renders an OverlayTrigger with Button', () => {
+      const wrapper = setup();
+      const trigger = wrapper.find(OverlayTrigger);
+      const button = trigger.find(Button);
 
-    const wrapper = setup();
-    const instance = wrapper.instance();
+      expect(trigger).toHaveLength(1);
+      expect(button).toHaveLength(1);
+    });
 
-    return instance.getCopyUrl().then(() => {
-      expect(storeQuerySpy.mock.calls).toHaveLength(1);
-      expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1);
-      expect(storeQuerySpy.mock.calls[0][0]).toMatchObject(
-        defaultProps.queryEditor,
-      );
-      expect(instance.state.shortUrl).toContain(storeQueryMockId);
+    it('calls storeQuery() with the query when getCopyUrl() is called and saves the url', () => {
+      expect.assertions(4);
+      const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
 
-      return Promise.resolve();
-    });
-  });
+      const wrapper = setup();
+      const instance = wrapper.instance();
 
-  it('dispatches an error toast upon fetching failure', () => {
-    expect.assertions(3);
-    const error = 'error';
-    const addDangerToastSpy = jest.fn();
-    fetchMock.post(storeQueryUrl, { throws: error }, { overwriteRoutes: true });
-    const wrapper = setup();
-    wrapper.setProps({ addDangerToast: addDangerToastSpy });
-
-    return wrapper
-      .instance()
-      .getCopyUrl()
-      .then(() => {
+      return instance.getCopyUrl().then(() => {
+        expect(storeQuerySpy.mock.calls).toHaveLength(1);
         expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1);
-        expect(addDangerToastSpy.mock.calls).toHaveLength(1);
-        expect(addDangerToastSpy.mock.calls[0][0]).toBe(error);
+        expect(storeQuerySpy.mock.calls[0][0]).toMatchObject(
+          storedQueryAttributes,
+        );
+        expect(instance.state.shortUrl).toContain(storeQueryMockId);
+
+        storeQuerySpy.mockRestore();
 
         return Promise.resolve();
       });
+    });
+
+    it('dispatches an error toast upon fetching failure', () => {
+      expect.assertions(3);
+      const error = 'error';
+      const addDangerToastSpy = jest.fn();
+      fetchMock.post(
+        storeQueryUrl,
+        { throws: error },
+        { overwriteRoutes: true },
+      );
+      const wrapper = setup();
+      wrapper.setProps({ addDangerToast: addDangerToastSpy });
+
+      return wrapper
+        .instance()
+        .getCopyUrl()
+        .then(() => {
+          expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1);
+          expect(addDangerToastSpy.mock.calls).toHaveLength(1);
+          expect(addDangerToastSpy.mock.calls[0][0]).toBe(error);
+
+          return Promise.resolve();
+        });
+    });
+  });
+  describe('via saved query', () => {
+    beforeAll(() => {
+      isFeatureEnabledMock = jest
+        .spyOn(featureFlags, 'isFeatureEnabled')
+        .mockImplementation(() => false);
+    });
+
+    afterAll(() => {
+      isFeatureEnabledMock.restore();
+    });
+
+    it('renders an OverlayTrigger with Button', () => {
+      const wrapper = setup();
+      const trigger = wrapper.find(OverlayTrigger);
+      const button = trigger.find(Button);
+
+      expect(trigger).toHaveLength(1);
+      expect(button).toHaveLength(1);
+    });
+
+    it('does not call storeQuery() with the query when getCopyUrl() is called', () => {
+      const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
+
+      const wrapper = setup();
+      const instance = wrapper.instance();
+
+      instance.getCopyUrl();
+
+      expect(storeQuerySpy.mock.calls).toHaveLength(0);
+      expect(fetchMock.calls(storeQueryUrl)).toHaveLength(0);
+      expect(instance.state.shortUrl).toContain(999);
+
+      storeQuerySpy.mockRestore();
+    });
+
+    it('shows a request to save the query when the query is not yet saved', () => {
+      const wrapper = setup({ remoteId: undefined });
+      const instance = wrapper.instance();
+
+      instance.getCopyUrl();
+
+      expect(instance.state.shortUrl).toContain('save');
+    });
   });
 });

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

@@ -20,6 +20,7 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import { Popover, OverlayTrigger } from 'react-bootstrap';
 import { t } from '@superset-ui/translation';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
 
 import Button from '../../components/Button';
 import CopyToClipboard from '../../components/CopyToClipboard';
@@ -34,6 +35,7 @@ const propTypes = {
     schema: PropTypes.string,
     autorun: PropTypes.bool,
     sql: PropTypes.string,
+    remoteId: PropTypes.number,
   }).isRequired,
   addDangerToast: PropTypes.func.isRequired,
 };
@@ -48,6 +50,14 @@ class ShareSqlLabQuery extends React.Component {
   }
 
   getCopyUrl() {
+    if (isFeatureEnabled(FeatureFlag.SHARE_QUERIES_VIA_KV_STORE)) {
+      return this.getCopyUrlForKvStore();
+    }
+
+    return this.getCopyUrlForSavedQuery();
+  }
+
+  getCopyUrlForKvStore() {
     const { dbId, title, schema, autorun, sql } = this.props.queryEditor;
     const sharedQuery = { dbId, title, schema, autorun, sql };
 
@@ -63,6 +73,21 @@ class ShareSqlLabQuery extends React.Component {
       });
   }
 
+  getCopyUrlForSavedQuery() {
+    let savedQueryToastContent;
+
+    if (this.props.queryEditor.remoteId) {
+      savedQueryToastContent =
+        window.location.origin +
+        window.location.pathname +
+        `?savedQueryId=${this.props.queryEditor.remoteId}`;
+      this.setState({ shortUrl: savedQueryToastContent });
+    } else {
+      savedQueryToastContent = t('Please save the query to enable sharing');
+      this.setState({ shortUrl: savedQueryToastContent });
+    }
+  }
+
   renderPopover() {
     return (
       <Popover id="sqllab-shareurl-popover">

superset-frontend/src/featureFlags.ts

@@ -24,6 +24,7 @@ export enum FeatureFlag {
   SCHEDULED_QUERIES = 'SCHEDULED_QUERIES',
   SQL_VALIDATORS_BY_ENGINE = 'SQL_VALIDATORS_BY_ENGINE',
   ESTIMATE_QUERY_COST = 'ESTIMATE_QUERY_COST',
+  SHARE_QUERIES_VIA_KV_STORE = 'SHARE_QUERIES_VIA_KV_STORE',
   SQLLAB_BACKEND_PERSISTENCE = 'SQLLAB_BACKEND_PERSISTENCE',
 }
 

superset/app.py

@@ -265,7 +265,10 @@ def init_views(self) -> None:
         appbuilder.add_view_no_menu(Dashboard)
         appbuilder.add_view_no_menu(DashboardModelViewAsync)
         appbuilder.add_view_no_menu(Datasource)
-        appbuilder.add_view_no_menu(KV)
+
+        if feature_flag_manager.is_feature_enabled("KV_STORE"):
+            appbuilder.add_view_no_menu(KV)
+
         appbuilder.add_view_no_menu(R)
         appbuilder.add_view_no_menu(SavedQueryView)
         appbuilder.add_view_no_menu(SavedQueryViewApi)

superset/config.py

@@ -278,7 +278,9 @@ def _try_json_readsha(filepath, length):  # pylint: disable=unused-argument
     # Experimental feature introducing a client (browser) cache
     "CLIENT_CACHE": False,
     "ENABLE_EXPLORE_JSON_CSRF_PROTECTION": False,
+    "KV_STORE": False,
     "PRESTO_EXPAND_DATA": False,
+    "SHARE_QUERIES_VIA_KV_STORE": False,
     "TAGGING_SYSTEM": False,
 }
 

tests/core_tests.py

@@ -30,13 +30,20 @@
 import string
 from typing import Any, Dict
 import unittest
-from unittest import mock
+from unittest import mock, skipUnless
 
 import pandas as pd
 import sqlalchemy as sqla
 
 from tests.test_app import app
-from superset import dataframe, db, jinja_context, security_manager, sql_lab
+from superset import (
+    dataframe,
+    db,
+    jinja_context,
+    security_manager,
+    sql_lab,
+    is_feature_enabled,
+)
 from superset.common.query_context import QueryContext
 from superset.connectors.connector_registry import ConnectorRegistry
 from superset.connectors.sqla.models import SqlaTable
@@ -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(
+        (is_feature_enabled("KV_STORE")), "skipping as /kv/ endpoints are not enabled"
+    )
     def test_kv(self):
         self.login(username="admin")