fix(dashboard): draft dashboards should be viewable #14207

ktmud · 2021-04-16T20:59:28Z

SUMMARY

Temp fix for #14175 with some light refactoring:

Rename DashboardFilter to DashboardAccessFilter to be more specific---we should do the same for other filters too (ChartFilter, DatasetFilter, etc)
Replace get_by_id_or_slug in Dashboard DAO with the same logics used previously by the dashboard view.
Added a new inferred column status to replace published, but published is kept for backward compatibility.

TODO: we should replace the boolean column published with a enum column status to support the long-term solution mentioned in #14175 (comment)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Gamma user visiting a draft dashboard throws unexpected JS error

After

Users with Superset -> dashboard view access should be able to open a draft dashboard created by other users.

But they won't be able to view it in the dashboard list.

TEST PLAN

CI

For manual verification, see #14175 for details.

ADDITIONAL INFORMATION

Has associated issue: [Dashboard] Error message for draft/deleted dashboards is not actionable #14175 chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata #13306
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

ktmud · 2021-04-16T21:00:05Z

superset/dashboards/api.py

@@ -105,6 +108,7 @@ class DashboardRestApi(BaseSupersetModelRestApi):
    list_columns = [
        "id",
        "published",
+        "status",


superset/dashboards/api.py

superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx

pkdotson

I'll approve this. Frontend code looks good. Looks like the python unit test needs to be updated.

superset-frontend/src/components/ListView/types.ts

nytai · 2021-04-17T00:28:32Z

superset/dashboards/filters.py

@@ -57,16 +58,16 @@ class DashboardFavoriteFilter(BaseFavoriteFilter):
    model = Dashboard


-class DashboardFilter(BaseFilter):
+class DashboardAccessFilter(BaseFilter):


👍 more semantic, I was always a bit confused what DashboardFilter was really doing

superset/dashboards/api.py

amitmiran137 · 2021-04-18T19:21:01Z

superset/dashboards/dao.py

-            db.session.query(Dashboard)
-            .filter(id_or_slug_filter(id_or_slug))
-            .outerjoin(Slice, Dashboard.slices)
-            .outerjoin(Slice.table)
-            .outerjoin(Dashboard.owners)
-            .outerjoin(Dashboard.roles)
-        )
-        # Apply dashboard base filters
-        query = DashboardFilter("id", SQLAInterface(Dashboard, db.session)).apply(
-            query, None
-        )
-        dashboard = query.one_or_none()
+        dashboard = Dashboard.get(id_or_slug)
        if not dashboard:
            raise DashboardNotFoundError()


this exposes a new security risk: it would be easy to know if a dashboard exists or not even if user doesn't have access to begin with

But that's just the same as before right? The goal of this PR is to revert the behavior of non-owners having access to draft dashboards. Whether this is a risk that worth further actions is another topic.

superset/dashboards/dao.py

suddjian · 2021-04-19T17:56:45Z

I wonder if it would be more appropriate to change DashboardAccessFilter so that it respects draft dashboards. Seems like there could be other bugs lurking because of that.

Thank you for addressing this, by the way. There have been a couple of regressions due to this so I've made a ticket for myself to write some e2e tests around drafts.

codecov · 2021-04-19T21:44:28Z

Codecov Report

Merging #14207 (87d2801) into master (ff665fa) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 87d2801 differs from pull request most recent head 6496f89. Consider uploading reports for the commit 6496f89 to get more accurate results

@@            Coverage Diff             @@
##           master   #14207      +/-   ##
==========================================
- Coverage   76.74%   76.73%   -0.01%     
==========================================
  Files         952      954       +2     
  Lines       48043    48055      +12     
  Branches     5978     5972       -6     
==========================================
+ Hits        36870    36877       +7     
- Misses      10971    10976       +5     
  Partials      202      202

Flag	Coverage Δ
javascript	`72.08% <100.00%> (+0.03%)`	⬆️
mysql	`80.72% <100.00%> (-0.01%)`	⬇️
postgres	`80.75% <100.00%> (-0.01%)`	⬇️
python	`80.80% <100.00%> (-0.04%)`	⬇️
sqlite	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...perset-frontend/src/views/CRUD/alert/AlertList.tsx	`75.75% <ø> (ø)`
...perset-frontend/src/views/CRUD/chart/ChartList.tsx	`72.38% <ø> (ø)`
...t-frontend/src/views/CRUD/data/query/QueryList.tsx	`71.84% <ø> (ø)`
superset-frontend/src/components/ListView/types.ts	`100.00% <100.00%> (ø)`
...ews/CRUD/annotationlayers/AnnotationLayersList.tsx	`77.35% <100.00%> (ø)`
...d/src/views/CRUD/csstemplates/CssTemplatesList.tsx	`78.26% <100.00%> (ø)`
...rontend/src/views/CRUD/dashboard/DashboardList.tsx	`74.78% <100.00%> (+0.21%)`	⬆️
...uperset-frontend/src/views/CRUD/dashboard/types.ts	`100.00% <100.00%> (ø)`
...tend/src/views/CRUD/data/database/DatabaseList.tsx	`80.00% <100.00%> (ø)`
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx	`69.93% <100.00%> (ø)`
... and 18 more

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 ff665fa...6496f89. Read the comment docs.

ktmud · 2021-04-19T22:52:26Z

@suddjian previously the list view already filters out draft dashboards for non-owners/admins, so as long as we use DashboardAccessFilter only for the list view, it's OK to let DashboardAccessFilter filer out drafts. There are a couple of unit tests on the API with drafts but they don't seem to align with the original behavior which this PR tries to revert back to.

ktmud · 2021-04-20T22:45:29Z

tests/dashboards/security/security_dataset_tests.py

-        uri = DASHBOARD_API_URL_FORMAT.format(dashboard.id)
-        rv = self.client.get(uri)
-        self.assert404(rv)
-


This is a duplicate test to https://github.com/apache/superset/blob/7a9036e35d557fccd0ccaeb7277c10b9e1039a70/tests/dashboards/api_tests.py#L221

ktmud · 2021-04-21T00:51:20Z

@suddjian @pkdotson @amitmiran137 I finally fixed all the test cases but had to change some expected values because the behavior of the API endpoints has changed (what returns 404 before would now return 200---like it does before for the Dashboard page). Would appreciate an extra pair of eyes if you have time.

etr2460

lgtm, thanks for the fix!

etr2460 · 2021-04-21T16:40:26Z

superset-frontend/src/views/CRUD/dashboard/types.ts

@@ -24,3 +24,8 @@ export type DashboardObject = {
  position?: string;
  metadata?: string;
 };
+
+export enum DashboardStatus {


yay, new types!

* fix(dashboard): draft dashboards should have open access * Remove a duplicate test

pull-request-size bot added the size/L label Apr 16, 2021

ktmud force-pushed the dashboard-draft-open-access branch from 10d7c27 to 695e151 Compare April 16, 2021 21:18

ktmud commented Apr 16, 2021

View reviewed changes

pkdotson approved these changes Apr 16, 2021

View reviewed changes

ktmud force-pushed the dashboard-draft-open-access branch 2 times, most recently from 2c5b8fb to e8520d4 Compare April 16, 2021 22:58

nytai reviewed Apr 17, 2021

View reviewed changes

superset-frontend/src/components/ListView/types.ts Outdated Show resolved Hide resolved

nytai reviewed Apr 17, 2021

View reviewed changes

ktmud force-pushed the dashboard-draft-open-access branch from e8520d4 to aaf88e2 Compare April 17, 2021 00:59

amitmiran137 reviewed Apr 18, 2021

View reviewed changes

superset/dashboards/api.py Show resolved Hide resolved

amitmiran137 reviewed Apr 18, 2021

View reviewed changes

superset/dashboards/dao.py Outdated Show resolved Hide resolved

ktmud force-pushed the dashboard-draft-open-access branch 2 times, most recently from d577a74 to 0386f47 Compare April 19, 2021 21:23

ktmud force-pushed the dashboard-draft-open-access branch 3 times, most recently from bfb4a73 to 7a9036e Compare April 20, 2021 04:21

ktmud added 2 commits April 20, 2021 15:44

fix(dashboard): draft dashboards should have open access

d22f62c

Remove a duplicate test

6496f89

ktmud force-pushed the dashboard-draft-open-access branch from 7a9036e to 6496f89 Compare April 20, 2021 22:44

ktmud commented Apr 20, 2021

View reviewed changes

etr2460 approved these changes Apr 21, 2021

View reviewed changes

ktmud merged commit 2dd20df into apache:master Apr 21, 2021

ktmud deleted the dashboard-draft-open-access branch April 21, 2021 16:54

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021

fix(dashboard): draft dashboards should be viewable (apache#14207)

5f7fff1

* fix(dashboard): draft dashboards should have open access * Remove a duplicate test

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

mistercrunch added the 🚢 1.2.0 label Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dashboard): draft dashboards should be viewable #14207

fix(dashboard): draft dashboards should be viewable #14207

ktmud commented Apr 16, 2021 •

edited

Loading

ktmud Apr 16, 2021

pkdotson left a comment

nytai Apr 17, 2021

amitmiran137 Apr 18, 2021

ktmud Apr 19, 2021 •

edited

Loading

suddjian commented Apr 19, 2021 •

edited

Loading

codecov bot commented Apr 19, 2021 •

edited

Loading

ktmud commented Apr 19, 2021

ktmud Apr 20, 2021

ktmud commented Apr 21, 2021

etr2460 left a comment

etr2460 Apr 21, 2021

fix(dashboard): draft dashboards should be viewable #14207

fix(dashboard): draft dashboards should be viewable #14207

Conversation

ktmud commented Apr 16, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

ktmud Apr 16, 2021

Choose a reason for hiding this comment

pkdotson left a comment

Choose a reason for hiding this comment

nytai Apr 17, 2021

Choose a reason for hiding this comment

amitmiran137 Apr 18, 2021

Choose a reason for hiding this comment

ktmud Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

suddjian commented Apr 19, 2021 • edited Loading

codecov bot commented Apr 19, 2021 • edited Loading

Codecov Report

ktmud commented Apr 19, 2021

ktmud Apr 20, 2021

Choose a reason for hiding this comment

ktmud commented Apr 21, 2021

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Apr 21, 2021

Choose a reason for hiding this comment

ktmud commented Apr 16, 2021 •

edited

Loading

ktmud Apr 19, 2021 •

edited

Loading

suddjian commented Apr 19, 2021 •

edited

Loading

codecov bot commented Apr 19, 2021 •

edited

Loading