fix: replace datamask with key from new key value api #17680

pkdotson · 2021-12-08T04:47:58Z

SUMMARY

This pr fixes the longurl problem in dashboard when a user has a lot of filters and shares the url with other users. The long copied url had the potential to break loading the dashboard. This pr uses the new keyvalue api for dashboard to replace the datamask content with secure hashed key.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

Screen.Recording.2021-12-08.at.5.59.57.PM.mov

after

Screen.Recording.2021-12-08.at.6.14.05.PM.mov

TESTING INSTRUCTIONS

Go to a dashboard and create filters. Ensure that the nativefilters param has a hashed key when applying new filters and saving the dashboard url.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

pkdotson · 2021-12-10T03:30:22Z

/testenv up

github-actions · 2021-12-10T03:32:25Z

@pkdotson Ephemeral environment spinning up at http://54.191.208.158:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

zhaoyongjie · 2021-12-10T15:18:34Z

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/state.ts

@@ -88,7 +88,6 @@ export const useFilterUpdates = (
 ) => {
  const filters = useFilters();
  const dataMaskApplied = useNativeFiltersDataMask();
-


Could we remove the unnecessary change?

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/keyValue.tsx

zhaoyongjie

Hey Philip. I have added some comments.If you have any questions, feel free ping me.

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/keyValue.tsx

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx

superset-frontend/src/dashboard/actions/hydrate.js

superset-frontend/src/constants.ts

superset-frontend/src/utils/urlUtils.ts

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/keyValue.tsx

zhaoyongjie · 2021-12-11T11:36:50Z

BTW:
I found a potential security issues when I review this PR. The user can create a cache store from the api/v1/dashboard/${dashId}/filter_state/${key}/ endpoint. This can constitute a memory attack.

We have to fix this issue in separate PR. @michael-s-molina @pkdotson

filter_put_method.mov

villebro

A few quick comments. Great work and seems to work nicely, but it's missing one key feature that we discussed with @michael-s-molina that's needed: when a user enters a dashboard with the native filter key, it should initialize a new key-value pair based on the contents of the original one. This is to ensure a user can have multiple tabs open based on the same key; when opening the new tab, the key will change, which will ensure that the state of the original tab is not overwritten. This is mostly an edge case for regular users, but an important one for public dashboards, where circulating the key to others will lead to the original state being overwritten if another anonymous user changes the filters.

superset-frontend/src/dashboard/components/Header/index.jsx

superset-frontend/src/constants.ts

…set-io/superset into frontend-dashboard-longurl-2

rusackas · 2021-12-17T21:53:27Z

/testenv up

github-actions · 2021-12-17T21:57:40Z

@rusackas Ephemeral environment spinning up at http://52.33.231.170:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx

rusackas · 2021-12-18T04:35:59Z

/testenv up

github-actions · 2021-12-18T04:40:24Z

@rusackas Ephemeral environment spinning up at http://35.86.151.100:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

superset-frontend/src/dashboard/containers/DashboardPage.tsx

zhaoyongjie

frontend code LGTM, but we have to polish the key-value backend codes in separate PR. When I input a random filter key, the server-side response 500.

villebro

LGTM, thanks for the hard work here!

github-actions · 2021-12-21T17:09:20Z

Ephemeral environment shutdown and build artifacts deleted.

michael-s-molina · 2021-12-27T16:49:47Z

frontend code LGTM, but we have to polish the key-value backend codes in separate PR. When I input a random filter key, the server-side response 500.

@zhaoyongjie This has been fixed in #17878

michael-s-molina · 2021-12-27T16:51:01Z

Thank you for the hard work @pkdotson!

* afirst stage to ccheck to get initial datamask * clean up code and update typescript * remove consoles * fix ts and update copy dashboard url * use key when one doesn't exists * lint clean up * fix errors * add suggested changes * remove line * add tests and add changes for copydashboard * fix lint * fix lint * fix lint * Update superset-frontend/src/dashboard/components/Header/index.jsx Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * add timeout * fix test * fix test, add qs to cypress and add suggestions * add suggestions * fix lint * more suggested changes for backwards comapat * fix lint * cleanup naming and add qs parse to tests * Update superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * Update superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * more changes and fix lint * remove nativefiler param * fix path * remove con * simplify logic Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

pkdotson added 3 commits December 7, 2021 12:18

afirst stage to ccheck to get initial datamask

5655338

clean up code and update typescript

94db318

remove consoles

38461bb

superset-github-bot bot added the preset-io label Dec 8, 2021

pull-request-size bot added the size/L label Dec 8, 2021

pkdotson added hold:testing! On hold for testing need:qa-review Requires QA review labels Dec 8, 2021

pkdotson added 3 commits December 7, 2021 22:24

fix ts and update copy dashboard url

b08a472

use key when one doesn't exists

6420292

lint clean up

3ce45d1

pkdotson marked this pull request as ready for review December 9, 2021 02:16

pkdotson requested review from villebro, michael-s-molina, geido, zhaoyongjie and rusackas December 9, 2021 02:16

fix errors

dfb050f

zhaoyongjie reviewed Dec 10, 2021

View reviewed changes

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts Show resolved Hide resolved

zhaoyongjie reviewed Dec 11, 2021

View reviewed changes

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/keyValue.tsx Outdated Show resolved Hide resolved

zhaoyongjie reviewed Dec 11, 2021

View reviewed changes

villebro reviewed Dec 12, 2021

View reviewed changes

superset-frontend/src/dashboard/components/Header/index.jsx Outdated Show resolved Hide resolved

superset-frontend/src/constants.ts Outdated Show resolved Hide resolved

pkdotson added 3 commits December 13, 2021 16:29

add suggested changes

e7ccfb7

remove line

e4b3830

add tests and add changes for copydashboard

548a5bb

pkdotson requested review from zhaoyongjie and villebro December 15, 2021 03:56

pkdotson added 2 commits December 17, 2021 12:14

more changes and fix lint

fc7ef7b

Merge branch 'frontend-dashboard-longurl-2' of https://github.com/pre…

c74b8d5

…set-io/superset into frontend-dashboard-longurl-2

pkdotson requested a review from villebro December 17, 2021 21:38

rusackas reviewed Dec 17, 2021

View reviewed changes

superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx Outdated Show resolved Hide resolved

pkdotson added 3 commits December 17, 2021 14:59

remove nativefiler param

a683b2d

fix path

0c9163a

remove con

8e19e18

zhaoyongjie reviewed Dec 20, 2021

View reviewed changes

superset-frontend/src/dashboard/containers/DashboardPage.tsx Outdated Show resolved Hide resolved

simplify logic

40d7343

pkdotson requested review from rusackas and zhaoyongjie and removed request for zhaoyongjie December 20, 2021 22:37

zhaoyongjie approved these changes Dec 21, 2021

View reviewed changes

villebro approved these changes Dec 21, 2021

View reviewed changes

pkdotson merged commit cfd851a into apache:master Dec 21, 2021

rusackas deleted the frontend-dashboard-longurl-2 branch January 4, 2022 04:10

geido mentioned this pull request Jan 7, 2022

Dashboard url slug won’t be really saved when edit in dashboard edit mode. #17938

Closed

3 tasks

villebro mentioned this pull request Jan 28, 2022

Lots of filters leads to too large url #17086

Closed

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: replace datamask with key from new key value api #17680

fix: replace datamask with key from new key value api #17680

pkdotson commented Dec 8, 2021 •

edited

pkdotson commented Dec 10, 2021

github-actions bot commented Dec 10, 2021

zhaoyongjie Dec 10, 2021

zhaoyongjie left a comment

zhaoyongjie commented Dec 11, 2021 •

edited

villebro left a comment

rusackas commented Dec 17, 2021

github-actions bot commented Dec 17, 2021

rusackas commented Dec 18, 2021

github-actions bot commented Dec 18, 2021

zhaoyongjie left a comment

villebro left a comment

github-actions bot commented Dec 21, 2021

michael-s-molina commented Dec 27, 2021

michael-s-molina commented Dec 27, 2021

fix: replace datamask with key from new key value api #17680

fix: replace datamask with key from new key value api #17680

Conversation

pkdotson commented Dec 8, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

pkdotson commented Dec 10, 2021

github-actions bot commented Dec 10, 2021

zhaoyongjie Dec 10, 2021

Choose a reason for hiding this comment

zhaoyongjie left a comment

Choose a reason for hiding this comment

zhaoyongjie commented Dec 11, 2021 • edited

villebro left a comment

Choose a reason for hiding this comment

rusackas commented Dec 17, 2021

github-actions bot commented Dec 17, 2021

rusackas commented Dec 18, 2021

github-actions bot commented Dec 18, 2021

zhaoyongjie left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2021

michael-s-molina commented Dec 27, 2021

michael-s-molina commented Dec 27, 2021

pkdotson commented Dec 8, 2021 •

edited

zhaoyongjie commented Dec 11, 2021 •

edited