From e39f4aa41acc48310096caf4e54e05a444e0882e Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Tue, 25 Jul 2023 14:28:16 -0300 Subject: [PATCH 1/8] adding logic to check dataset used by filters --- superset/security/manager.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/superset/security/manager.py b/superset/security/manager.py index 28354ac18dafe..31029a745e59f 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2063,6 +2063,16 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: ) exists = db.session.query(query.exists()).scalar() + + # check for datasets that are only used by filters + if not exists: + dashboards_json = db.session.query(Dashboard.json_metadata).filter(Dashboard.id.in_(dashboard_ids)).all() + for json_ in dashboards_json: + json_metadata = json.loads(json_.json_metadata) + filter_dataset_ids = [target['datasetId'] for filter in json_metadata['native_filter_configuration'] for target in filter['targets']] + if datasource.id in filter_dataset_ids: + exists = True + return exists @staticmethod From 27afd2cca320a71227efdb4833a4f371a0b3b84d Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Tue, 25 Jul 2023 15:55:47 -0300 Subject: [PATCH 2/8] handling exceptions --- superset/security/manager.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 31029a745e59f..6d49d563df75b 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -16,6 +16,7 @@ # under the License. # pylint: disable=too-many-lines """A set of constants and methods to manage permissions and security""" +import json import logging import re import time @@ -2068,10 +2069,14 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: if not exists: dashboards_json = db.session.query(Dashboard.json_metadata).filter(Dashboard.id.in_(dashboard_ids)).all() for json_ in dashboards_json: - json_metadata = json.loads(json_.json_metadata) - filter_dataset_ids = [target['datasetId'] for filter in json_metadata['native_filter_configuration'] for target in filter['targets']] - if datasource.id in filter_dataset_ids: - exists = True + try: + json_metadata = json.loads(json_.json_metadata) + for filter in json_metadata.get('native_filter_configuration', []): + filter_dataset_ids = [target.get('datasetId') for target in filter.get('targets', [])] + if datasource.id in filter_dataset_ids: + exists = True + except ValueError: + pass return exists From e5ae33b4e5e654a532b6fb176ccdd237dad3ee5d Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Tue, 25 Jul 2023 21:37:10 -0300 Subject: [PATCH 3/8] Adding integration test --- superset/security/manager.py | 15 ++++-- .../security/guest_token_security_tests.py | 49 +++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 6d49d563df75b..74382edc67be0 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2064,15 +2064,22 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: ) exists = db.session.query(query.exists()).scalar() - + # check for datasets that are only used by filters if not exists: - dashboards_json = db.session.query(Dashboard.json_metadata).filter(Dashboard.id.in_(dashboard_ids)).all() + dashboards_json = ( + db.session.query(Dashboard.json_metadata) + .filter(Dashboard.id.in_(dashboard_ids)) + .all() + ) for json_ in dashboards_json: try: json_metadata = json.loads(json_.json_metadata) - for filter in json_metadata.get('native_filter_configuration', []): - filter_dataset_ids = [target.get('datasetId') for target in filter.get('targets', [])] + for filter in json_metadata.get("native_filter_configuration", []): + filter_dataset_ids = [ + target.get("datasetId") + for target in filter.get("targets", []) + ] if datasource.id in filter_dataset_ids: exists = True except ValueError: diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index e0517c9b28ae3..6c4fe4c37bc29 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -15,23 +15,27 @@ # specific language governing permissions and limitations # under the License. """Unit tests for Superset""" +import json from unittest import mock import pytest from flask import g from superset import db, security_manager +from superset.connectors.sqla.models import SqlaTable from superset.daos.dashboard import EmbeddedDashboardDAO from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard from superset.security.guest_token import GuestTokenResourceType from superset.sql_parse import Table +from superset.utils.database import get_example_database from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, load_birth_names_data, ) +from tests.integration_tests.fixtures.public_role import public_role_like_gamma @mock.patch.dict( @@ -233,3 +237,48 @@ def test_raise_for_dashboard_access_as_guest_no_rbac(self): db.session.delete(dash) db.session.commit() + + @pytest.mark.usefixtures("public_role_like_gamma") + def test_can_access_based_on_dashboard_with_filter(self): + """ + Test that a user can access a datasource used only by a filter in a dashboard. + """ + # Create a test dataset + test_dataset = SqlaTable( + database_id=get_example_database().id, + schema="main", + table_name="test_table", + ) + db.session.add(test_dataset) + db.session.commit() + + # Create an embedabble dashboard with a filter powered by the test dataset + test_dashboard = Dashboard() + test_dashboard.dashboard_title = "Test Embedded Dashboard" + test_dashboard.json_metadata = json.dumps( + { + "native_filter_configuration": [ + {"targets": [{"datasetId": test_dataset.id}]} + ] + } + ) + test_dashboard.owners = [] + test_dashboard.slices = [] + test_dashboard.published = False + db.session.add(test_dashboard) + db.session.commit() + self.embedded = EmbeddedDashboardDAO.upsert(test_dashboard, []) + + # grant access to the dashboad + self.authorized_guest.resources = [ + {"type": "dashboard", "id": str(self.embedded.uuid)} + ] + self.authorized_guest.resources + self.authorized_guest.roles = [security_manager.get_public_role()] + g.user = self.authorized_guest + + # The user should have access to the datasource via the dashboard + assert security_manager.can_access_based_on_dashboard(test_dataset) == True + + db.session.delete(test_dashboard) + db.session.delete(test_dataset) + db.session.commit() From 36fa0c3b303dd0b58f6f115627015033b57de616 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Tue, 25 Jul 2023 21:41:55 -0300 Subject: [PATCH 4/8] simplifying role --- tests/integration_tests/security/guest_token_security_tests.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 6c4fe4c37bc29..b8cfb3a2a893a 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -35,7 +35,6 @@ load_birth_names_dashboard_with_slices, load_birth_names_data, ) -from tests.integration_tests.fixtures.public_role import public_role_like_gamma @mock.patch.dict( @@ -238,7 +237,6 @@ def test_raise_for_dashboard_access_as_guest_no_rbac(self): db.session.delete(dash) db.session.commit() - @pytest.mark.usefixtures("public_role_like_gamma") def test_can_access_based_on_dashboard_with_filter(self): """ Test that a user can access a datasource used only by a filter in a dashboard. From 6dffe8a063ab9523a8914cdbe6ba989ffbfd8bc9 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Tue, 25 Jul 2023 22:23:04 -0300 Subject: [PATCH 5/8] Improving test logic --- .../security/guest_token_security_tests.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index b8cfb3a2a893a..7404961dd8d52 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -268,14 +268,12 @@ def test_can_access_based_on_dashboard_with_filter(self): self.embedded = EmbeddedDashboardDAO.upsert(test_dashboard, []) # grant access to the dashboad - self.authorized_guest.resources = [ - {"type": "dashboard", "id": str(self.embedded.uuid)} - ] + self.authorized_guest.resources - self.authorized_guest.roles = [security_manager.get_public_role()] g.user = self.authorized_guest + g.user.resources = [{"type": "dashboard", "id": str(self.embedded.uuid)}] + g.user.roles = [security_manager.get_public_role()] # The user should have access to the datasource via the dashboard - assert security_manager.can_access_based_on_dashboard(test_dataset) == True + security_manager.raise_for_access(datasource=test_dataset) db.session.delete(test_dashboard) db.session.delete(test_dataset) From 5579a93bbb828e5d548ad036ccb2b0ef690009ce Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Wed, 26 Jul 2023 00:49:02 -0300 Subject: [PATCH 6/8] fixing tests --- superset/security/manager.py | 4 ++-- .../integration_tests/security/guest_token_security_tests.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 74382edc67be0..af33b5206d438 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2075,10 +2075,10 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: for json_ in dashboards_json: try: json_metadata = json.loads(json_.json_metadata) - for filter in json_metadata.get("native_filter_configuration", []): + for filter_ in json_metadata.get("native_filter_configuration", []): filter_dataset_ids = [ target.get("datasetId") - for target in filter.get("targets", []) + for target in filter_.get("targets", []) ] if datasource.id in filter_dataset_ids: exists = True diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 7404961dd8d52..b5f4e239bac5e 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -245,7 +245,7 @@ def test_can_access_based_on_dashboard_with_filter(self): test_dataset = SqlaTable( database_id=get_example_database().id, schema="main", - table_name="test_table", + table_name="test_table_embedded_filter", ) db.session.add(test_dataset) db.session.commit() From bcf1fc5369c8c14f48d32c75495b969f7a6d8599 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Fri, 28 Jul 2023 02:39:46 -0300 Subject: [PATCH 7/8] Addressing PR feedback --- superset/security/manager.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index af33b5206d438..218db65952de6 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2063,10 +2063,11 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: .filter(Dashboard.id.in_(dashboard_ids)) ) - exists = db.session.query(query.exists()).scalar() + if db.session.query(query.exists()).scalar(): + return True # check for datasets that are only used by filters - if not exists: + else: dashboards_json = ( db.session.query(Dashboard.json_metadata) .filter(Dashboard.id.in_(dashboard_ids)) @@ -2081,11 +2082,11 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: for target in filter_.get("targets", []) ] if datasource.id in filter_dataset_ids: - exists = True + return True except ValueError: pass - return exists + return False @staticmethod def _get_current_epoch_time() -> float: From 7668aec67d59874f57df730fa175af17cc9ddf08 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Fri, 28 Jul 2023 10:26:22 -0300 Subject: [PATCH 8/8] Small changes --- superset/security/manager.py | 34 +++++++++---------- .../security/guest_token_security_tests.py | 5 +-- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 218db65952de6..b4b66439760c4 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2067,24 +2067,22 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: return True # check for datasets that are only used by filters - else: - dashboards_json = ( - db.session.query(Dashboard.json_metadata) - .filter(Dashboard.id.in_(dashboard_ids)) - .all() - ) - for json_ in dashboards_json: - try: - json_metadata = json.loads(json_.json_metadata) - for filter_ in json_metadata.get("native_filter_configuration", []): - filter_dataset_ids = [ - target.get("datasetId") - for target in filter_.get("targets", []) - ] - if datasource.id in filter_dataset_ids: - return True - except ValueError: - pass + dashboards_json = ( + db.session.query(Dashboard.json_metadata) + .filter(Dashboard.id.in_(dashboard_ids)) + .all() + ) + for json_ in dashboards_json: + try: + json_metadata = json.loads(json_.json_metadata) + for filter_ in json_metadata.get("native_filter_configuration", []): + filter_dataset_ids = [ + target.get("datasetId") for target in filter_.get("targets", []) + ] + if datasource.id in filter_dataset_ids: + return True + except ValueError: + pass return False diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index b5f4e239bac5e..e5bf589184396 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -237,9 +237,10 @@ def test_raise_for_dashboard_access_as_guest_no_rbac(self): db.session.delete(dash) db.session.commit() - def test_can_access_based_on_dashboard_with_filter(self): + def test_can_access_datasource_used_in_dashboard_filter(self): """ - Test that a user can access a datasource used only by a filter in a dashboard. + Test that a user can access a datasource used only by a filter in a dashboard + they have access to. """ # Create a test dataset test_dataset = SqlaTable(