fix: dashboard get by id or slug access filter #22358
Conversation
dpgaspar
changed the title
Codecov Report
@@ Coverage Diff @@
## master #22358 +/- ##
==========================================
- Coverage 66.85% 66.85% -0.01%
==========================================
Files 1847 1850 +3
Lines 70560 70658 +98
Branches 7737 7737
==========================================
+ Hits 47173 47238 +65
- Misses 21380 21413 +33
Partials 2007 2007
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
dpgaspar
changed the title
dpgaspar
requested review from
villebro,
ktmud,
john-bodley,
rusackas,
michael-s-molina and
geido
| @@ -40,11 +40,22 @@ class DashboardDAO(BaseDAO): | |||
| base_filter = DashboardAccessFilter | |||
|
|
|||
| @staticmethod | |||
| def get_by_id_or_slug(id_or_slug: str) -> Dashboard: | |||
| dashboard = Dashboard.get(id_or_slug) | |||
There was a problem hiding this comment.
Doesn't FAB fetch all the relationships as well, i.e., why do we need to explicitly compose a query which joins all the related models?
There was a problem hiding this comment.
it does but the get item endpoint for dashboards is custom on Superset because we need to fetch a dashboard by id or slug, so on this case we need to custom develop. FAB ModelRestApi on get item endpoints will get item by the table primary key
| """ | ||
| All users should have access to dashboards without roles |
There was a problem hiding this comment.
Is All users should have access to dashboards without roles not supported anymore? Does a draft dashboard have roles?
There was a problem hiding this comment.
yes this PR removes it. I'm setting /api/v1/dashboard/<1> to the same level of authorization has /api/v1/dashboard/, this mean the dashboard metadata has the same access level
|
/testenv up |
|
@michael-s-molina Ephemeral environment spinning up at http://34.220.124.185:8080. Credentials are |
dpgaspar
deleted the
danielgaspar/sc-53722/fix-dashboard-by-id-or-slug-unauthorised
branch
|
Ephemeral environment shutdown and build artifacts deleted. |
This reverts commit 3761694.
This reverts commit 3761694.
This reverts commit 3761694.
This reverts commit 3761694.
mistercrunch
added
🏷️ bot
SUMMARY
This PR re-introduces the defined dashboard access filter into Dashboard DAO get item method.
Currently viewing a list of dashboards and requesting a specific dashboard by its slug or id will allow for different access levels.
I believe that access filters at the DAO level need to be applied on all methods (that fetch or mutate a not yet fetched item) and offer a coherent centralised first layer of security checks
This PR also:
Changes HTTP response status codes for filter states, since a dashboard that a user does not have permission to access is automatically filtered at the DAO level, filter states for these dashboards will return 404 instead of 403 (Improved security)
Fixes a bug when using
DASHBOARD_RBAC, currently on master, a Gamma user (for example) that has access to a dashboard using data access permissions can list the dashboard but can't render the dashboard itself (/api/v1/dashboard/<id>and/api/v1/dashboard/<id>/charts/returns HTTP 403). This happens because onlyraise_for_dashboard_accessis called and not data access permission is previously checked.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION