dpgaspar commented on a change in pull request #12680:
URL: https://github.com/apache/superset/pull/12680#discussion_r566861565
##########
File path: superset/dashboards/filters.py
##########
@@ -107,8 +121,9 @@ def apply(self, query: Query, value: Any) -> Query:
query = query.filter(
or_(
Dashboard.id.in_(owner_ids_query),
- Dashboard.id.in_(published_dash_query),
+ Dashboard.id.in_(datesource_perm_query),
Review comment:
nit: typo `datasource_perm_query`
##########
File path: tests/dashboards/security/security_dataset_tests.py
##########
@@ -0,0 +1,241 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for Superset"""
+import json
+
+import prison
+import pytest
+from flask import escape
+
+from superset import app
+from superset.models import core as models
+from tests.dashboards.base_case import DashboardTestCase
+from tests.dashboards.consts import *
+from tests.dashboards.dashboard_test_utils import *
+from tests.dashboards.superset_factory_util import *
+from tests.fixtures.energy_dashboard import load_energy_table_with_slice
+
+
+class TestDashboardDatasetSecurity(DashboardTestCase):
+ @pytest.fixture
+ def load_dashboard(self):
+ with app.app_context():
+ table = (
+
db.session.query(SqlaTable).filter_by(table_name="energy_usage").one()
+ )
+ # get a slice from the allowed table
+ slice = db.session.query(Slice).filter_by(slice_name="Energy
Sankey").one()
+
+ self.grant_public_access_to_table(table)
+
+ pytest.hidden_dash_slug = f"hidden_dash_{random_slug()}"
+ pytest.published_dash_slug = f"published_dash_{random_slug()}"
+
+ # Create a published and hidden dashboard and add them to the
database
+ published_dash = Dashboard()
+ published_dash.dashboard_title = "Published Dashboard"
+ published_dash.slug = pytest.published_dash_slug
+ published_dash.slices = [slice]
+ published_dash.published = True
+
+ hidden_dash = Dashboard()
+ hidden_dash.dashboard_title = "Hidden Dashboard"
+ hidden_dash.slug = pytest.hidden_dash_slug
+ hidden_dash.slices = [slice]
+ hidden_dash.published = False
+
+ db.session.merge(published_dash)
+ db.session.merge(hidden_dash)
+ yield db.session.commit()
+
+ self.revoke_public_access_to_table(table)
+ db.session.delete(published_dash)
+ db.session.delete(hidden_dash)
+ db.session.commit()
+
+ def test_dashboard_access__admin_can_access_all(self):
+ # arrange
+ self.login(username=ADMIN_USERNAME)
+ dashboard_title_by_url = {
+ dash.url: dash.dashboard_title for dash in get_all_dashboards()
+ }
+
+ # act
+ responses_by_url = {
+ url: self.client.get(url).data.decode("utf-8")
+ for url in dashboard_title_by_url.keys()
+ }
+
+ # assert
+ for dashboard_url, get_dashboard_response in responses_by_url.items():
+ assert (
+ escape(dashboard_title_by_url[dashboard_url]) in
get_dashboard_response
+ )
+
+ def test_get_dashboards__users_are_dashboards_owners(self):
+ # arrange
+ username = "gamma"
+ user = security_manager.find_user(username)
+ my_owned_dashboard = create_dashboard_to_db(
+ dashboard_title="My Dashboard", published=False, owners=[user],
+ )
+
+ not_my_owned_dashboard = create_dashboard_to_db(
+ dashboard_title="Not My Dashboard", published=False,
+ )
+
+ self.login(user.username)
+
+ # act
+ get_dashboards_response = self.get_resp(DASHBOARDS_API_URL)
+
+ # assert
+ self.assertIn(my_owned_dashboard.url, get_dashboards_response)
Review comment:
nit: we are currently preferring `assert in get_dashboards_response`
##########
File path: tests/dashboards/superset_factory_util.py
##########
@@ -0,0 +1,303 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+from typing import Any, Callable, List, Optional
+
+from flask_appbuilder import Model
+from flask_appbuilder.security.sqla.models import User
+
+from superset import appbuilder
+from superset.connectors.sqla.models import SqlaTable, sqlatable_user
+from superset.models.core import Database
+from superset.models.dashboard import (
+ Dashboard,
+ dashboard_slices,
+ dashboard_user,
+ DashboardRoles,
+)
+from superset.models.slice import Slice, slice_user
+from tests.dashboards.dashboard_test_utils import random_slug, random_str,
random_title
+
+logger = logging.getLogger(__name__)
+
+session = appbuilder.get_session
+
+inserted_dashboards_ids = []
+inserted_databases_ids = []
+inserted_sqltables_ids = []
+inserted_slices_ids = []
Review comment:
Currently there is a mix of patterns on fixture creation on our test
codebase. I think that the pytest fixture and test parameters bring a lot of
value, and it's always preferable to have an expected pattern. Different
creation methods keep things DRY, but that's also possible using pytest
fixtures, heres a simple example:
https://github.com/apache/superset/blob/master/tests/reports/commands_tests.py#L132
##########
File path: tests/dashboards/security/security_dataset_tests.py
##########
@@ -0,0 +1,260 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for Superset"""
+import json
+
+import prison
+import pytest
+from flask import escape
+
+from superset import app, security_manager
+from superset.models import core as models
+from tests.dashboards.base_case import DashboardTestCase
+from tests.dashboards.consts import *
+from tests.dashboards.dashboard_test_utils import *
+from tests.dashboards.superset_factory_util import *
+from tests.fixtures.energy_dashboard import load_energy_table_with_slice
+
+
+# @mark.amit
+class TestDashboardDatasetSecurity(DashboardTestCase):
+ @pytest.fixture
+ def load_dashboard(self):
+ with app.app_context():
+ table = (
+
db.session.query(SqlaTable).filter_by(table_name="energy_usage").one()
+ )
+ # get a slice from the allowed table
+ slice = db.session.query(Slice).filter_by(slice_name="Energy
Sankey").one()
+
+ self.grant_public_access_to_table(table)
+
+ pytest.hidden_dash_slug = f"hidden_dash_{random_slug()}"
+ pytest.published_dash_slug = f"published_dash_{random_slug()}"
+
+ # Create a published and hidden dashboard and add them to the
database
+ published_dash = Dashboard()
+ published_dash.dashboard_title = "Published Dashboard"
+ published_dash.slug = pytest.published_dash_slug
+ published_dash.slices = [slice]
+ published_dash.published = True
+
+ hidden_dash = Dashboard()
+ hidden_dash.dashboard_title = "Hidden Dashboard"
+ hidden_dash.slug = pytest.hidden_dash_slug
+ hidden_dash.slices = [slice]
+ hidden_dash.published = False
+
+ db.session.merge(published_dash)
+ db.session.merge(hidden_dash)
+ yield db.session.commit()
+
+ self.revoke_public_access_to_table(table)
+ db.session.delete(published_dash)
+ db.session.delete(hidden_dash)
+ db.session.commit()
+
+ def test_dashboard_access__admin_can_access_all(self):
+ # arrange
+ self.login(username=ADMIN_USERNAME)
+ dashboard_title_by_url = {
+ dash.url: dash.dashboard_title for dash in get_all_dashboards()
+ }
+
+ # act
+ responses_by_url = {
+ url: self.client.get(url).data.decode("utf-8")
+ for url in dashboard_title_by_url.keys()
+ }
+
+ # assert
+ for dashboard_url, get_dashboard_response in responses_by_url.items():
+ assert (
+ escape(dashboard_title_by_url[dashboard_url]) in
get_dashboard_response
+ )
+
+ def test_get_dashboards__users_are_dashboards_owners(self):
+ # arrange
+ username = "gamma"
+ user = security_manager.find_user(username)
+ my_owned_dashboard = create_dashboard_to_db(
+ dashboard_title="My Dashboard",
+ slug=f"my_dash_{random_slug()}",
+ published=False,
+ owners=[user],
+ )
+
+ not_my_owned_dashboard = create_dashboard_to_db(
+ dashboard_title="Not My Dashboard",
+ slug=f"not_my_dash_{random_slug()}",
+ published=False,
+ )
+
+ self.login(user.username)
+
+ # act
+ get_dashboards_response = self.get_resp(DASHBOARDS_API_URL)
+
+ # assert
+ self.assertIn(my_owned_dashboard.url, get_dashboards_response)
+ self.assertNotIn(not_my_owned_dashboard.url, get_dashboards_response)
+
+ def test_get_dashboards__owners_can_view_empty_dashboard(self):
+ # arrange
+ dash = create_dashboard_to_db("Empty Dashboard",
slug="empty_dashboard")
+ dashboard_url = dash.url
+ gamma_user = security_manager.find_user("gamma")
+ self.login(gamma_user.username)
+
+ # act
+ get_dashboards_response = self.get_resp(DASHBOARDS_API_URL)
+
+ # assert
+ self.assertNotIn(dashboard_url, get_dashboards_response)
+
+ def test_get_dashboards__users_can_view_favorites_dashboards(self):
+ # arrange
+ user = security_manager.find_user("gamma")
+ fav_dash_slug = f"my_favorite_dash_{random_slug()}"
+ regular_dash_slug = f"regular_dash_{random_slug()}"
+
+ favorite_dash = Dashboard()
+ favorite_dash.dashboard_title = "My Favorite Dashboard"
+ favorite_dash.slug = fav_dash_slug
+
+ regular_dash = Dashboard()
+ regular_dash.dashboard_title = "A Plain Ol Dashboard"
+ regular_dash.slug = regular_dash_slug
+
+ db.session.merge(favorite_dash)
+ db.session.merge(regular_dash)
+ db.session.commit()
+
+ dash =
db.session.query(Dashboard).filter_by(slug=fav_dash_slug).first()
+
+ favorites = models.FavStar()
+ favorites.obj_id = dash.id
+ favorites.class_name = "Dashboard"
+ favorites.user_id = user.id
+
+ db.session.merge(favorites)
+ db.session.commit()
+
+ self.login(user.username)
+
+ # act
+ get_dashboards_response = self.get_resp(DASHBOARDS_API_URL)
+
+ # assert
+ self.assertIn(f"/superset/dashboard/{fav_dash_slug}/",
get_dashboards_response)
Review comment:
This is a good point, might be some legacy logic, removing it is a
breaking so it get perpetuated. Maybe @betodealmeida can help with more
context?
##########
File path: tests/dashboards/superset_factory_util.py
##########
@@ -0,0 +1,303 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+from typing import List, Optional
+
+from flask_appbuilder import Model
+from flask_appbuilder.security.sqla.models import User
+
+from superset import appbuilder
+from superset.connectors.sqla.models import SqlaTable, sqlatable_user
+from superset.models.core import Database
+from superset.models.dashboard import (
+ Dashboard,
+ dashboard_slices,
+ dashboard_user,
+ DashboardRoles,
+)
+from superset.models.slice import Slice, slice_user
+from tests.dashboards.dashboard_test_utils import random_slug, random_str,
random_title
+
+logger = logging.getLogger(__name__)
+
+session = appbuilder.get_session
+
+inserted_dashboards_ids = []
+inserted_databases_ids = []
+inserted_sqltables_ids = []
+inserted_slices_ids = []
+
+
+def create_dashboard_to_db(
+ dashboard_title: Optional[str] = None,
+ slug: Optional[str] = None,
+ published: bool = False,
+ owners: Optional[List[User]] = None,
+ slices: Optional[List[Slice]] = None,
+ css: str = "",
+ json_metadata: str = "",
+ position_json: str = "",
+) -> Dashboard:
+ dashboard = create_dashboard(
+ dashboard_title,
+ slug,
+ published,
+ owners,
+ slices,
+ css,
+ json_metadata,
+ position_json,
+ )
+
+ insert_model(dashboard)
+ inserted_dashboards_ids.append(dashboard.id)
+ return dashboard
+
+
+def create_dashboard(
+ dashboard_title: Optional[str] = None,
+ slug: Optional[str] = None,
+ published: bool = False,
+ owners: Optional[List[User]] = None,
+ slices: Optional[List[Slice]] = None,
+ css: str = "",
+ json_metadata: str = "",
+ position_json: str = "",
+) -> Dashboard:
+ dashboard_title = dashboard_title or random_title()
+ slug = slug or random_slug()
+ owners = owners or []
+ slices = slices or []
+ return Dashboard(
+ dashboard_title=dashboard_title,
+ slug=slug,
+ published=published,
+ owners=owners,
+ css=css,
+ position_json=position_json,
+ json_metadata=json_metadata,
+ slices=slices,
+ )
+
+
+def insert_model(dashboard: Model) -> None:
+ session.add(dashboard)
+ session.commit()
+ session.refresh(dashboard)
+
+
+def create_slice_to_db(
+ name: Optional[str] = None,
+ datasource_id: Optional[int] = None,
+ owners: Optional[List[User]] = None,
+) -> Slice:
+ slice_ = create_slice(datasource_id, name, owners)
+ insert_model(slice_)
+ inserted_slices_ids.append(slice_.id)
+ return slice_
+
+
+def create_slice(datasource_id, name, owners):
Review comment:
nit: missing type annotations
##########
File path: tests/dashboards/security/security_dataset_tests.py
##########
@@ -0,0 +1,241 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for Superset"""
+import json
+
+import prison
+import pytest
+from flask import escape
+
+from superset import app
+from superset.models import core as models
+from tests.dashboards.base_case import DashboardTestCase
+from tests.dashboards.consts import *
+from tests.dashboards.dashboard_test_utils import *
+from tests.dashboards.superset_factory_util import *
+from tests.fixtures.energy_dashboard import load_energy_table_with_slice
+
+
+class TestDashboardDatasetSecurity(DashboardTestCase):
+ @pytest.fixture
+ def load_dashboard(self):
+ with app.app_context():
+ table = (
+
db.session.query(SqlaTable).filter_by(table_name="energy_usage").one()
+ )
+ # get a slice from the allowed table
+ slice = db.session.query(Slice).filter_by(slice_name="Energy
Sankey").one()
+
+ self.grant_public_access_to_table(table)
+
+ pytest.hidden_dash_slug = f"hidden_dash_{random_slug()}"
+ pytest.published_dash_slug = f"published_dash_{random_slug()}"
+
+ # Create a published and hidden dashboard and add them to the
database
+ published_dash = Dashboard()
+ published_dash.dashboard_title = "Published Dashboard"
+ published_dash.slug = pytest.published_dash_slug
+ published_dash.slices = [slice]
+ published_dash.published = True
+
+ hidden_dash = Dashboard()
+ hidden_dash.dashboard_title = "Hidden Dashboard"
+ hidden_dash.slug = pytest.hidden_dash_slug
+ hidden_dash.slices = [slice]
+ hidden_dash.published = False
+
+ db.session.merge(published_dash)
+ db.session.merge(hidden_dash)
+ yield db.session.commit()
+
+ self.revoke_public_access_to_table(table)
+ db.session.delete(published_dash)
+ db.session.delete(hidden_dash)
+ db.session.commit()
+
+ def test_dashboard_access__admin_can_access_all(self):
+ # arrange
+ self.login(username=ADMIN_USERNAME)
+ dashboard_title_by_url = {
+ dash.url: dash.dashboard_title for dash in get_all_dashboards()
+ }
+
+ # act
+ responses_by_url = {
+ url: self.client.get(url).data.decode("utf-8")
+ for url in dashboard_title_by_url.keys()
+ }
+
+ # assert
+ for dashboard_url, get_dashboard_response in responses_by_url.items():
+ assert (
+ escape(dashboard_title_by_url[dashboard_url]) in
get_dashboard_response
+ )
+
+ def test_get_dashboards__users_are_dashboards_owners(self):
+ # arrange
+ username = "gamma"
+ user = security_manager.find_user(username)
+ my_owned_dashboard = create_dashboard_to_db(
+ dashboard_title="My Dashboard", published=False, owners=[user],
+ )
+
+ not_my_owned_dashboard = create_dashboard_to_db(
+ dashboard_title="Not My Dashboard", published=False,
+ )
+
+ self.login(user.username)
+
+ # act
+ get_dashboards_response = self.get_resp(DASHBOARDS_API_URL)
+
+ # assert
+ self.assertIn(my_owned_dashboard.url, get_dashboards_response)
+ self.assertNotIn(not_my_owned_dashboard.url, get_dashboards_response)
Review comment:
This test creates 2 dashboards, we should delete them after the test
runs. Can we use a pytest fixture here?
##########
File path: superset/dashboards/filters.py
##########
@@ -74,12 +75,14 @@ def apply(self, query: Query, value: Any) -> Query:
datasource_perms =
security_manager.user_view_menu_names("datasource_access")
schema_perms = security_manager.user_view_menu_names("schema_access")
- published_dash_query = (
+ dashboard_has_roles = Dashboard.roles.any()
+ datesource_perm_query = (
Review comment:
nit: typo `datasource_perm_query`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
