pt
/
superset
mirror of https://github.com/apache/superset.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: check if guest user modified query (#27484)

This commit is contained in:
Beto Dealmeida 2024-03-12 21:28:06 -04:00 committed by GitHub
parent 85d0d88fc2
commit 735b895dd5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 156 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

85
superset/security/manager.py
View File

@ -143,6 +143,56 @@ RoleModelView.edit_columns = ["name", "permissions", "user"]
RoleModelView.related_views = []
def query_context_modified(query_context: "QueryContext") -> bool:
"""
Check if a query context has been modified.
This is used to ensure guest users don't modify the payload and fetch data
different from what was shared with them in dashboards.
"""
form_data = query_context.form_data
stored_chart = query_context.slice_
# sanity checks
if form_data is None or stored_chart is None:
return True
# cannot request a different chart
if form_data.get("slice_id") != stored_chart.id:
return True
# compare form_data
requested_metrics = {
frozenset(metric.items()) if isinstance(metric, dict) else metric
for metric in form_data.get("metrics") or []
}
stored_metrics = {
frozenset(metric.items()) if isinstance(metric, dict) else metric
for metric in stored_chart.params_dict.get("metrics") or []
}
if not requested_metrics.issubset(stored_metrics):
return True
# compare queries in query_context
queries_metrics = {
frozenset(metric.items()) if isinstance(metric, dict) else metric
for query in query_context.queries
for metric in query.metrics or []
}
if stored_chart.query_context:
stored_query_context = json.loads(cast(str, stored_chart.query_context))
for query in stored_query_context.get("queries") or []:
stored_metrics.update(
{
frozenset(metric.items()) if isinstance(metric, dict) else metric
for metric in query.get("metrics") or []
}
)
return not queries_metrics.issubset(stored_metrics)
class SupersetSecurityManager( # pylint: disable=too-many-public-methods
SecurityManager
):
@ -1941,29 +1991,20 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
self.get_table_access_error_object(denied)
)
if self.is_guest_user() and query_context:
# Guest users MUST not modify the payload so it's requesting a different
# chart or different ad-hoc metrics from what's saved.
form_data = query_context.form_data
stored_chart = query_context.slice_
if (
form_data is None
or stored_chart is None
or form_data.get("slice_id") != stored_chart.id
or form_data.get("metrics", []) != stored_chart.params_dict["metrics"]
or any(
query.metrics != stored_chart.params_dict["metrics"]
for query in query_context.queries
)
):
raise SupersetSecurityException(
SupersetError(
error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
message=_("Guest user cannot modify chart payload"),
level=ErrorLevel.ERROR,
)
# Guest users MUST not modify the payload so it's requesting a
# different chart or different ad-hoc metrics from what's saved.
if (
query_context
and self.is_guest_user()
and query_context_modified(query_context)
):
raise SupersetSecurityException(
SupersetError(
error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
message=_("Guest user cannot modify chart payload"),
level=ErrorLevel.ERROR,
)
)
if datasource or query_context or viz:
form_data = None

112
tests/unit_tests/security/manager_test.py
View File

@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.
# pylint: disable=invalid-name, unused-argument, redefined-outer-name
import pytest
from flask_appbuilder.security.sqla.models import Role, User
from pytest_mock import MockFixture
@ -25,6 +27,7 @@ from superset.exceptions import SupersetSecurityException
from superset.extensions import appbuilder
from superset.models.slice import Slice
from superset.security.manager import SupersetSecurityManager
from superset.superset_typing import AdhocMetric
from superset.utils.core import override_user
@ -36,12 +39,29 @@ def test_security_manager(app_context: None) -> None:
assert sm
def test_raise_for_access_guest_user(
@pytest.fixture
def stored_metrics() -> list[AdhocMetric]:
"""
Return a list of metrics.
"""
return [
{
"column": None,
"expressionType": "SQL",
"hasCustomLabel": False,
"label": "COUNT(*) + 1",
"sqlExpression": "COUNT(*) + 1",
},
]
def test_raise_for_access_guest_user_ok(
mocker: MockFixture,
app_context: None,
stored_metrics: list[AdhocMetric],
) -> None:
"""
Test that guest user can't modify chart payload.
Test that guest user can submit an unmodified chart payload.
"""
sm = SupersetSecurityManager(appbuilder)
mocker.patch.object(sm, "is_guest_user", return_value=True)
@ -49,23 +69,11 @@ def test_raise_for_access_guest_user(
query_context = mocker.MagicMock()
query_context.slice_.id = 42
stored_metrics = [
{
"aggregate": None,
"column": None,
"datasourceWarning": False,
"expressionType": "SQL",
"hasCustomLabel": False,
"label": "COUNT(*) + 1",
"optionName": "metric_ssa1gwimio_cxpyjc7vj3s",
"sqlExpression": "COUNT(*) + 1",
}
]
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"metrics": stored_metrics,
}
# normal request
query_context.form_data = {
"slice_id": 42,
"metrics": stored_metrics,
@ -73,7 +81,26 @@ def test_raise_for_access_guest_user(
query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore
sm.raise_for_access(query_context=query_context)
# tampered requests
def test_raise_for_access_guest_user_tampered_id(
mocker: MockFixture,
app_context: None,
stored_metrics: list[AdhocMetric],
) -> None:
"""
Test that guest user cannot modify the chart ID.
"""
sm = SupersetSecurityManager(appbuilder)
mocker.patch.object(sm, "is_guest_user", return_value=True)
mocker.patch.object(sm, "can_access", return_value=True)
query_context = mocker.MagicMock()
query_context.slice_.id = 42
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"metrics": stored_metrics,
}
query_context.form_data = {
"slice_id": 43,
"metrics": stored_metrics,
@ -82,15 +109,32 @@ def test_raise_for_access_guest_user(
with pytest.raises(SupersetSecurityException):
sm.raise_for_access(query_context=query_context)
def test_raise_for_access_guest_user_tampered_form_data(
mocker: MockFixture,
app_context: None,
stored_metrics: list[AdhocMetric],
) -> None:
"""
Test that guest user cannot modify metrics in the form data.
"""
sm = SupersetSecurityManager(appbuilder)
mocker.patch.object(sm, "is_guest_user", return_value=True)
mocker.patch.object(sm, "can_access", return_value=True)
query_context = mocker.MagicMock()
query_context.slice_.id = 42
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"metrics": stored_metrics,
}
tampered_metrics = [
{
"aggregate": None,
"column": None,
"datasourceWarning": False,
"expressionType": "SQL",
"hasCustomLabel": False,
"label": "COUNT(*) + 2",
"optionName": "metric_ssa1gwimio_cxpyjc7vj3s",
"sqlExpression": "COUNT(*) + 2",
}
]
@ -102,6 +146,36 @@ def test_raise_for_access_guest_user(
with pytest.raises(SupersetSecurityException):
sm.raise_for_access(query_context=query_context)
def test_raise_for_access_guest_user_tampered_queries(
mocker: MockFixture,
app_context: None,
stored_metrics: list[AdhocMetric],
) -> None:
"""
Test that guest user cannot modify metrics in the queries.
"""
sm = SupersetSecurityManager(appbuilder)
mocker.patch.object(sm, "is_guest_user", return_value=True)
mocker.patch.object(sm, "can_access", return_value=True)
query_context = mocker.MagicMock()
query_context.slice_.id = 42
query_context.slice_.query_context = None
query_context.slice_.params_dict = {
"metrics": stored_metrics,
}
tampered_metrics = [
{
"column": None,
"expressionType": "SQL",
"hasCustomLabel": False,
"label": "COUNT(*) + 2",
"sqlExpression": "COUNT(*) + 2",
}
]
query_context.form_data = {
"slice_id": 42,
"metrics": stored_metrics,