From 678a7397108d3a7de3321cc8926afe48cf9083f8 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 22 Nov 2021 14:01:20 -0300 Subject: [PATCH 1/6] fix: Dashboard access when RBAC is disabled --- superset/security/manager.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 523f8d97767e..4c0c64b5228e 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1174,19 +1174,22 @@ def raise_for_dashboard_access(dashboard: "Dashboard") -> None: from superset.views.base import get_user_roles, is_user_admin from superset.views.utils import is_owner + has_rbac_access = True + if is_feature_enabled("DASHBOARD_RBAC"): has_rbac_access = any( dashboard_role.id in [user_role.id for user_role in get_user_roles()] for dashboard_role in dashboard.roles ) - can_access = ( - is_user_admin() - or is_owner(dashboard, g.user) - or (dashboard.published and has_rbac_access) - ) - if not can_access: - raise DashboardAccessDeniedError() + can_access = ( + is_user_admin() + or is_owner(dashboard, g.user) + or (dashboard.published and has_rbac_access) + ) + + if not can_access: + raise DashboardAccessDeniedError() @staticmethod def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: From 35f4a3620d2daeb518a4ea9eb0ec398887a8624b Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 22 Nov 2021 15:05:26 -0300 Subject: [PATCH 2/6] Sends 403 when forbidden --- superset/dashboards/api.py | 13 +++++++++++++ tests/integration_tests/dashboards/api_tests.py | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index d248afd514f8..e6a27101e556 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -46,6 +46,7 @@ DashboardInvalidError, DashboardNotFoundError, DashboardUpdateFailedError, + DashboardAccessDeniedError, ) from superset.dashboards.commands.export import ExportDashboardsCommand from superset.dashboards.commands.importers.dispatcher import ImportDashboardsCommand @@ -267,6 +268,8 @@ def get(self, id_or_slug: str) -> Response: $ref: '#/components/responses/400' 401: $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' 404: $ref: '#/components/responses/404' """ @@ -275,6 +278,8 @@ def get(self, id_or_slug: str) -> Response: dash = DashboardDAO.get_by_id_or_slug(id_or_slug) result = self.dashboard_get_response_schema.dump(dash) return self.response(200, result=result) + except DashboardAccessDeniedError: + return self.response_403() except DashboardNotFoundError: return self.response_404() @@ -327,6 +332,8 @@ def get_datasets(self, id_or_slug: str) -> Response: $ref: '#/components/responses/400' 401: $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' 404: $ref: '#/components/responses/404' """ @@ -336,6 +343,8 @@ def get_datasets(self, id_or_slug: str) -> Response: self.dashboard_dataset_schema.dump(dataset) for dataset in datasets ] return self.response(200, result=result) + except DashboardAccessDeniedError: + return self.response_403() except DashboardNotFoundError: return self.response_404() @@ -386,6 +395,8 @@ def get_charts(self, id_or_slug: str) -> Response: $ref: '#/components/responses/400' 401: $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' 404: $ref: '#/components/responses/404' """ @@ -401,6 +412,8 @@ def get_charts(self, id_or_slug: str) -> Response: form_data.pop("label_colors", None) return self.response(200, result=result) + except DashboardAccessDeniedError: + return self.response_403() except DashboardNotFoundError: return self.response_404() diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 743c4af65be0..a4f610e931c1 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -395,7 +395,7 @@ def test_get_dashboard_no_data_access(self): self.login(username="gamma") uri = f"api/v1/dashboard/{dashboard.id}" rv = self.client.get(uri) - self.assertEqual(rv.status_code, 200) + self.assertEqual(rv.status_code, 403) # rollback changes db.session.delete(dashboard) db.session.commit() From cb8bce129994ab6a0e7bfcc481d679609a093e2d Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 22 Nov 2021 15:13:15 -0300 Subject: [PATCH 3/6] Fixes issort --- superset/dashboards/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index e6a27101e556..6fc4e841d90e 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -39,6 +39,7 @@ from superset.dashboards.commands.create import CreateDashboardCommand from superset.dashboards.commands.delete import DeleteDashboardCommand from superset.dashboards.commands.exceptions import ( + DashboardAccessDeniedError, DashboardBulkDeleteFailedError, DashboardCreateFailedError, DashboardDeleteFailedError, @@ -46,7 +47,6 @@ DashboardInvalidError, DashboardNotFoundError, DashboardUpdateFailedError, - DashboardAccessDeniedError, ) from superset.dashboards.commands.export import ExportDashboardsCommand from superset.dashboards.commands.importers.dispatcher import ImportDashboardsCommand From 81e16304aa25b8c5e920b0b567f15a2d57cd6e26 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 22 Nov 2021 16:06:21 -0300 Subject: [PATCH 4/6] Changes assertion --- tests/integration_tests/dashboards/api_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index a4f610e931c1..41b187acb3db 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -395,7 +395,7 @@ def test_get_dashboard_no_data_access(self): self.login(username="gamma") uri = f"api/v1/dashboard/{dashboard.id}" rv = self.client.get(uri) - self.assertEqual(rv.status_code, 403) + assert rv.status_code == 403 # rollback changes db.session.delete(dashboard) db.session.commit() From 893595ec1a5e9d2d7b27ac7a7c7cf053905cf124 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 22 Nov 2021 16:31:26 -0300 Subject: [PATCH 5/6] Allow access to unpublished dashboards that don't have roles --- superset/security/manager.py | 1 + tests/integration_tests/dashboards/api_tests.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 4c0c64b5228e..7ecb2e4522e2 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1186,6 +1186,7 @@ def raise_for_dashboard_access(dashboard: "Dashboard") -> None: is_user_admin() or is_owner(dashboard, g.user) or (dashboard.published and has_rbac_access) + or (not dashboard.published and not dashboard.roles) ) if not can_access: diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 41b187acb3db..48843d63cb5a 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -395,7 +395,7 @@ def test_get_dashboard_no_data_access(self): self.login(username="gamma") uri = f"api/v1/dashboard/{dashboard.id}" rv = self.client.get(uri) - assert rv.status_code == 403 + assert rv.status_code == 200 # rollback changes db.session.delete(dashboard) db.session.commit() From e9932210621f0f5f11fac9579066c45f0096e40f Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 23 Nov 2021 08:21:03 -0300 Subject: [PATCH 6/6] Fixes the test_get_dashboard_changed_on test --- tests/integration_tests/dashboards/dao_tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration_tests/dashboards/dao_tests.py b/tests/integration_tests/dashboards/dao_tests.py index 6f6708a13d07..ba3516624845 100644 --- a/tests/integration_tests/dashboards/dao_tests.py +++ b/tests/integration_tests/dashboards/dao_tests.py @@ -85,6 +85,7 @@ def test_set_dash_metadata(self): @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") def test_get_dashboard_changed_on(self): + self.login(username="admin") session = db.session() dashboard = session.query(Dashboard).filter_by(slug="world_health").first()