iop
/
superset
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: Security manager incorrect calls (#29884)

This commit is contained in:
Michael S. Molina 2024-08-23 11:39:45 -03:00 committed by GitHub
parent bc6d2dba37
commit d497dcad41
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 162 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

55
superset/connectors/sqla/models.py
View File

@ -21,10 +21,11 @@ import builtins
import dataclasses
import logging
import re
from collections import defaultdict
from collections.abc import Hashable
from dataclasses import dataclass, field
from datetime import datetime, timedelta
from typing import Any, Callable, cast
from typing import Any, Callable, cast, Optional, Union
import dateutil.parser
import numpy as np
@ -69,7 +70,7 @@ from sqlalchemy.sql.elements import ColumnClause, TextClause
from sqlalchemy.sql.expression import Label, TextAsFrom
from sqlalchemy.sql.selectable import Alias, TableClause
from superset import app, db, security_manager
from superset import app, db, is_feature_enabled, security_manager
from superset.commands.dataset.exceptions import DatasetNotFoundError
from superset.common.db_query_status import QueryStatus
from superset.connectors.sqla.utils import (
@ -712,6 +713,56 @@ class BaseDatasource(AuditMixinNullable, ImportExportMixin): # pylint: disable=
) -> BaseDatasource | None:
raise NotImplementedError()
def get_template_processor(self, **kwargs: Any) -> BaseTemplateProcessor:
raise NotImplementedError()
def text(self, clause: str) -> TextClause:
raise NotImplementedError()
def get_sqla_row_level_filters(
self,
template_processor: Optional[BaseTemplateProcessor] = None,
) -> list[TextClause]:
"""
Return the appropriate row level security filters for this table and the
current user. A custom username can be passed when the user is not present in the
Flask global namespace.
:param template_processor: The template processor to apply to the filters.
:returns: A list of SQL clauses to be ANDed together.
"""
template_processor = template_processor or self.get_template_processor()
all_filters: list[TextClause] = []
filter_groups: dict[Union[int, str], list[TextClause]] = defaultdict(list)
try:
for filter_ in security_manager.get_rls_filters(self):
clause = self.text(
f"({template_processor.process_template(filter_.clause)})"
)
if filter_.group_key:
filter_groups[filter_.group_key].append(clause)
else:
all_filters.append(clause)
if is_feature_enabled("EMBEDDED_SUPERSET"):
for rule in security_manager.get_guest_rls_filters(self):
clause = self.text(
f"({template_processor.process_template(rule['clause'])})"
)
all_filters.append(clause)
grouped_filters = [or_(*clauses) for clauses in filter_groups.values()]
all_filters.extend(grouped_filters)
return all_filters
except TemplateError as ex:
raise QueryObjectValidationError(
_(
"Error in jinja expression in RLS filters: %(msg)s",
msg=ex.message,
)
) from ex
class AnnotationDatasource(BaseDatasource):
"""Dummy object so we can query annotations using 'Viz' objects just like

2
superset/databases/api.py
View File

@ -2274,6 +2274,6 @@ class DatabaseRestApi(BaseSupersetModelRestApi):
# otherwise the database should have been filtered out
# in CsvToDatabaseForm
schemas_allowed_processed = security_manager.get_schemas_accessible_by_user(
database, schemas_allowed, True
database, database.get_default_catalog(), schemas_allowed, True
)
return self.response(200, schemas=schemas_allowed_processed)

9
superset/extensions/__init__.py
View File

@ -31,6 +31,7 @@ from superset.async_events.async_query_manager import AsyncQueryManager
from superset.async_events.async_query_manager_factory import AsyncQueryManagerFactory
from superset.extensions.ssh import SSHManagerFactory
from superset.extensions.stats_logger import BaseStatsLoggerManager
from superset.security.manager import SupersetSecurityManager
from superset.utils.cache_manager import CacheManager
from superset.utils.encrypt import EncryptedFieldFactory
from superset.utils.feature_flag_manager import FeatureFlagManager
@ -84,9 +85,9 @@ class UIManifestProcessor:
return {
"js_manifest": lambda bundle: get_files(bundle, "js"),
"css_manifest": lambda bundle: get_files(bundle, "css"),
"assets_prefix": self.app.config["STATIC_ASSETS_PREFIX"]
if self.app
else "",
"assets_prefix": (
self.app.config["STATIC_ASSETS_PREFIX"] if self.app else ""
),
}
def parse_manifest_json(self) -> None:
@ -132,7 +133,7 @@ manifest_processor = UIManifestProcessor(APP_DIR)
migrate = Migrate()
profiling = ProfilingExtension()
results_backend_manager = ResultsBackendManager()
security_manager = LocalProxy(lambda: appbuilder.sm)
security_manager: SupersetSecurityManager = LocalProxy(lambda: appbuilder.sm)
ssh_manager_factory = SSHManagerFactory()
stats_logger_manager = BaseStatsLoggerManager()
talisman = Talisman()

48
superset/models/helpers.py
View File

@ -22,7 +22,6 @@ import dataclasses
import logging
import re
import uuid
from collections import defaultdict
from collections.abc import Hashable
from datetime import datetime, timedelta
from typing import Any, cast, NamedTuple, Optional, TYPE_CHECKING, Union
@ -52,7 +51,7 @@ from sqlalchemy.sql.expression import Label, Select, TextAsFrom
from sqlalchemy.sql.selectable import Alias, TableClause
from sqlalchemy_utils import UUIDType
from superset import app, db, is_feature_enabled, security_manager
from superset import app, db, is_feature_enabled
from superset.advanced_data_type.types import AdvancedDataTypeResponse
from superset.common.db_query_status import QueryStatus
from superset.common.utils.time_range_utils import get_since_until_from_time_range
@ -806,47 +805,12 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
def get_sqla_row_level_filters(
self,
template_processor: Optional[BaseTemplateProcessor] = None,
template_processor: Optional[BaseTemplateProcessor] = None, # pylint: disable=unused-argument
) -> list[TextClause]:
"""
Return the appropriate row level security filters for this table and the
current user. A custom username can be passed when the user is not present in the
Flask global namespace.
:param template_processor: The template processor to apply to the filters.
:returns: A list of SQL clauses to be ANDed together.
"""
template_processor = template_processor or self.get_template_processor()
all_filters: list[TextClause] = []
filter_groups: dict[Union[int, str], list[TextClause]] = defaultdict(list)
try:
for filter_ in security_manager.get_rls_filters(self):
clause = self.text(
f"({template_processor.process_template(filter_.clause)})"
)
if filter_.group_key:
filter_groups[filter_.group_key].append(clause)
else:
all_filters.append(clause)
if is_feature_enabled("EMBEDDED_SUPERSET"):
for rule in security_manager.get_guest_rls_filters(self):
clause = self.text(
f"({template_processor.process_template(rule['clause'])})"
)
all_filters.append(clause)
grouped_filters = [or_(*clauses) for clauses in filter_groups.values()]
all_filters.extend(grouped_filters)
return all_filters
except TemplateError as ex:
raise QueryObjectValidationError(
_(
"Error in jinja expression in RLS filters: %(msg)s",
msg=ex.message,
)
) from ex
# TODO: We should refactor this mixin and remove this method
# as it exists in the BaseDatasource and is not applicable
# for datasources of type query
return []
def _process_sql_expression(
self,

4
superset/security/manager.py
View File

@ -2652,9 +2652,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return False
dashboards = [
r
for r in user.resources
if r["type"] == GuestTokenResourceType.DASHBOARD.value
r for r in user.resources if r["type"] == GuestTokenResourceType.DASHBOARD
]
# TODO (embedded): remove this check once uuids are rolled out

36
tests/integration_tests/security/guest_token_security_tests.py
View File

@ -113,15 +113,29 @@ class TestGuestUserDashboardAccess(SupersetTestCase):
self.authorized_guest = security_manager.get_guest_user_from_token(
{
"user": {},
"resources": [{"type": "dashboard", "id": str(self.embedded.uuid)}],
"resources": [
{
"type": GuestTokenResourceType.DASHBOARD,
"id": str(self.embedded.uuid),
}
],
"iat": 10,
"exp": 20,
"rls_rules": [],
}
)
self.unauthorized_guest = security_manager.get_guest_user_from_token(
{
"user": {},
"resources": [
{"type": "dashboard", "id": "06383667-3e02-4e5e-843f-44e9c5896b6c"}
{
"type": GuestTokenResourceType.DASHBOARD,
"id": "06383667-3e02-4e5e-843f-44e9c5896b6c",
}
],
"iat": 10,
"exp": 20,
"rls_rules": [],
}
)
@ -247,15 +261,29 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
self.authorized_guest = security_manager.get_guest_user_from_token(
{
"user": {},
"resources": [{"type": "dashboard", "id": str(self.embedded.uuid)}],
"resources": [
{
"type": GuestTokenResourceType.DASHBOARD,
"id": str(self.embedded.uuid),
}
],
"iat": 10,
"exp": 20,
"rls_rules": [],
}
)
self.unauthorized_guest = security_manager.get_guest_user_from_token(
{
"user": {},
"resources": [
{"type": "dashboard", "id": "06383667-3e02-4e5e-843f-44e9c5896b6c"}
{
"type": GuestTokenResourceType.DASHBOARD,
"id": "06383667-3e02-4e5e-843f-44e9c5896b6c",
}
],
"iat": 10,
"exp": 20,
"rls_rules": [],
}
)
self.chart = self.get_slice("Girls")

9
tests/integration_tests/security/row_level_security_tests.py
View File

@ -646,8 +646,15 @@ class GuestTokenRowLevelSecurityTests(SupersetTestCase):
return security_manager.get_guest_user_from_token(
{
"user": {},
"resources": [{"type": GuestTokenResourceType.DASHBOARD.value}],
"resources": [
{
"type": GuestTokenResourceType.DASHBOARD,
"id": "06383667-3e02-4e5e-843f-44e9c5896b6c",
}
],
"rls_rules": rules,
"iat": 10,
"exp": 20,
}
)

42
tests/unit_tests/charts/commands/importers/v1/import_test.py
View File

@ -76,7 +76,9 @@ def test_import_chart(mocker: MockerFixture, session_with_schema: Session) -> No
Test importing a chart.
"""
mocker.patch.object(security_manager, "can_access", return_value=True)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=True
)
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
@ -89,7 +91,7 @@ def test_import_chart(mocker: MockerFixture, session_with_schema: Session) -> No
assert chart.external_url is None
# Assert that the can write to chart was checked
security_manager.can_access.assert_called_once_with("can_write", "Chart")
mock_can_access.assert_called_once_with("can_write", "Chart")
def test_import_chart_managed_externally(
@ -98,7 +100,9 @@ def test_import_chart_managed_externally(
"""
Test importing a chart that is managed externally.
"""
mocker.patch.object(security_manager, "can_access", return_value=True)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=True
)
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
@ -111,7 +115,7 @@ def test_import_chart_managed_externally(
assert chart.external_url == "https://example.org/my_chart"
# Assert that the can write to chart was checked
security_manager.can_access.assert_called_once_with("can_write", "Chart")
mock_can_access.assert_called_once_with("can_write", "Chart")
def test_import_chart_without_permission(
@ -121,7 +125,9 @@ def test_import_chart_without_permission(
"""
Test importing a chart when a user doesn't have permissions to create.
"""
mocker.patch.object(security_manager, "can_access", return_value=False)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=False
)
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
@ -134,7 +140,7 @@ def test_import_chart_without_permission(
== "Chart doesn't exist and user doesn't have permission to create charts"
)
# Assert that the can write to chart was checked
security_manager.can_access.assert_called_once_with("can_write", "Chart")
mock_can_access.assert_called_once_with("can_write", "Chart")
def test_filter_chart_annotations(session: Session) -> None:
@ -162,8 +168,12 @@ def test_import_existing_chart_without_permission(
"""
Test importing a chart when a user doesn't have permissions to modify.
"""
mocker.patch.object(security_manager, "can_access", return_value=True)
mocker.patch.object(security_manager, "can_access_chart", return_value=False)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=True
)
mock_can_access_chart = mocker.patch.object(
security_manager, "can_access_chart", return_value=False
)
slice = (
session_with_data.query(Slice)
@ -180,8 +190,8 @@ def test_import_existing_chart_without_permission(
)
# Assert that the can write to chart was checked
security_manager.can_access.assert_called_once_with("can_write", "Chart")
security_manager.can_access_chart.assert_called_once_with(slice)
mock_can_access.assert_called_once_with("can_write", "Chart")
mock_can_access_chart.assert_called_once_with(slice)
def test_import_existing_chart_with_permission(
@ -191,8 +201,12 @@ def test_import_existing_chart_with_permission(
"""
Test importing a chart that exists when a user has access permission to that chart.
"""
mocker.patch.object(security_manager, "can_access", return_value=True)
mocker.patch.object(security_manager, "can_access_chart", return_value=True)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=True
)
mock_can_access_chart = mocker.patch.object(
security_manager, "can_access_chart", return_value=True
)
admin = User(
first_name="Alice",
@ -215,5 +229,5 @@ def test_import_existing_chart_with_permission(
with override_user(admin):
import_chart(config, overwrite=True)
# Assert that the can write to chart was checked
security_manager.can_access.assert_called_once_with("can_write", "Chart")
security_manager.can_access_chart.assert_called_once_with(slice)
mock_can_access.assert_called_once_with("can_write", "Chart")
mock_can_access_chart.assert_called_once_with(slice)

42
tests/unit_tests/dashboards/commands/importers/v1/import_test.py
View File

@ -65,7 +65,9 @@ def test_import_dashboard(mocker: MockerFixture, session_with_schema: Session) -
"""
Test importing a dashboard.
"""
mocker.patch.object(security_manager, "can_access", return_value=True)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=True
)
dashboard = import_dashboard(dashboard_config)
assert dashboard.dashboard_title == "Test dash"
@ -73,7 +75,7 @@ def test_import_dashboard(mocker: MockerFixture, session_with_schema: Session) -
assert dashboard.is_managed_externally is False
assert dashboard.external_url is None
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
mock_can_access.assert_called_once_with("can_write", "Dashboard")
def test_import_dashboard_managed_externally(
@ -83,7 +85,9 @@ def test_import_dashboard_managed_externally(
"""
Test importing a dashboard that is managed externally.
"""
mocker.patch.object(security_manager, "can_access", return_value=True)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=True
)
config = copy.deepcopy(dashboard_config)
config["is_managed_externally"] = True
@ -93,7 +97,7 @@ def test_import_dashboard_managed_externally(
assert dashboard.external_url == "https://example.org/my_dashboard"
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
mock_can_access.assert_called_once_with("can_write", "Dashboard")
def test_import_dashboard_without_permission(
@ -103,7 +107,9 @@ def test_import_dashboard_without_permission(
"""
Test importing a dashboard when a user doesn't have permissions to create.
"""
mocker.patch.object(security_manager, "can_access", return_value=False)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=False
)
with pytest.raises(ImportFailedError) as excinfo:
import_dashboard(dashboard_config)
@ -113,7 +119,7 @@ def test_import_dashboard_without_permission(
)
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
mock_can_access.assert_called_once_with("can_write", "Dashboard")
def test_import_existing_dashboard_without_permission(
@ -123,8 +129,12 @@ def test_import_existing_dashboard_without_permission(
"""
Test importing a dashboard when a user doesn't have permissions to create.
"""
mocker.patch.object(security_manager, "can_access", return_value=True)
mocker.patch.object(security_manager, "can_access_dashboard", return_value=False)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=True
)
mock_can_access_dashboard = mocker.patch.object(
security_manager, "can_access_dashboard", return_value=False
)
dashboard = (
session_with_data.query(Dashboard)
@ -141,8 +151,8 @@ def test_import_existing_dashboard_without_permission(
)
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
security_manager.can_access_dashboard.assert_called_once_with(dashboard)
mock_can_access.assert_called_once_with("can_write", "Dashboard")
mock_can_access_dashboard.assert_called_once_with(dashboard)
def test_import_existing_dashboard_with_permission(
@ -152,8 +162,12 @@ def test_import_existing_dashboard_with_permission(
"""
Test importing a dashboard that exists when a user has access permission to that dashboard.
"""
mocker.patch.object(security_manager, "can_access", return_value=True)
mocker.patch.object(security_manager, "can_access_dashboard", return_value=True)
mock_can_access = mocker.patch.object(
security_manager, "can_access", return_value=True
)
mock_can_access_dashboard = mocker.patch.object(
security_manager, "can_access_dashboard", return_value=True
)
admin = User(
first_name="Alice",
@ -173,5 +187,5 @@ def test_import_existing_dashboard_with_permission(
import_dashboard(dashboard_config, overwrite=True)
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
security_manager.can_access_dashboard.assert_called_once_with(dashboard)
mock_can_access.assert_called_once_with("can_write", "Dashboard")
mock_can_access_dashboard.assert_called_once_with(dashboard)

8
tests/unit_tests/jinja_context_test.py
View File

@ -418,10 +418,6 @@ def test_dataset_macro(mocker: MockerFixture) -> None:
"superset.connectors.sqla.models.security_manager.get_guest_rls_filters",
return_value=[],
)
mocker.patch(
"superset.models.helpers.security_manager.get_rls_filters",
return_value=[],
)
columns = [
TableColumn(column_name="ds", is_dttm=1, type="TIMESTAMP"),
@ -470,10 +466,6 @@ def test_dataset_macro(mocker: MockerFixture) -> None:
"superset.connectors.sqla.models.security_manager.get_guest_rls_filters",
return_value=[],
)
mocker.patch(
"superset.models.helpers.security_manager.get_guest_rls_filters",
return_value=[],
)
assert (
dataset_macro(1)