-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(security): Updating assert logic #10034
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ | |
# pylint: disable=C,R,W | ||
"""A set of constants and methods to manage permissions and security""" | ||
import logging | ||
from typing import Any, Callable, List, Optional, Set, Tuple, TYPE_CHECKING, Union | ||
from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKING, Union | ||
|
||
from flask import current_app, g | ||
from flask_appbuilder import Model | ||
|
@@ -38,7 +38,7 @@ | |
from sqlalchemy import or_ | ||
from sqlalchemy.engine.base import Connection | ||
from sqlalchemy.orm.mapper import Mapper | ||
from sqlalchemy.orm.query import Query | ||
from sqlalchemy.orm.query import Query as SqlaQuery | ||
|
||
from superset import sql_parse | ||
from superset.connectors.connector_registry import ConnectorRegistry | ||
|
@@ -52,6 +52,7 @@ | |
from superset.connectors.base.models import BaseDatasource | ||
from superset.connectors.druid.models import DruidCluster | ||
from superset.models.core import Database | ||
from superset.models.sql_lab import Query | ||
from superset.sql_parse import Table | ||
from superset.viz import BaseViz | ||
|
||
|
@@ -215,30 +216,32 @@ def can_access_all_queries(self) -> bool: | |
|
||
def can_access_all_datasources(self) -> bool: | ||
""" | ||
Return True if the user can access all Superset datasources, False otherwise. | ||
Return True if the user can fully access all the Superset datasources, False | ||
otherwise. | ||
|
||
:returns: Whether the user can access all Superset datasources | ||
:returns: Whether the user can fully access all Superset datasources | ||
""" | ||
|
||
return self.can_access("all_datasource_access", "all_datasource_access") | ||
|
||
def can_access_all_databases(self) -> bool: | ||
""" | ||
Return True if the user can access all Superset databases, False otherwise. | ||
Return True if the user can fully access all the Superset databases, False | ||
otherwise. | ||
|
||
:returns: Whether the user can access all Superset databases | ||
:returns: Whether the user can fully access all Superset databases | ||
""" | ||
|
||
return self.can_access("all_database_access", "all_database_access") | ||
|
||
def can_access_database(self, database: Union["Database", "DruidCluster"]) -> bool: | ||
""" | ||
Return True if the user can access the Superset database, False otherwise. | ||
Return True if the user can fully access the Superset database, False otherwise. | ||
|
||
Note for Druid the database is akin to the Druid cluster. | ||
|
||
:param database: The Superset database | ||
:returns: Whether the user can access the Superset database | ||
:returns: Whether the user can fully access the Superset database | ||
""" | ||
|
||
return ( | ||
|
@@ -249,14 +252,14 @@ def can_access_database(self, database: Union["Database", "DruidCluster"]) -> bo | |
|
||
def can_access_schema(self, datasource: "BaseDatasource") -> bool: | ||
""" | ||
Return True if the user can access the schema associated with the Superset | ||
Return True if the user can fully access the schema associated with the Superset | ||
datasource, False otherwise. | ||
|
||
Note for Druid datasources the database and schema are akin to the Druid cluster | ||
and datasource name prefix respectively, i.e., [schema.]datasource. | ||
|
||
:param datasource: The Superset datasource | ||
:returns: Whether the user can access the datasource's schema | ||
:returns: Whether the user can fully access the datasource's schema | ||
""" | ||
|
||
return ( | ||
|
@@ -267,15 +270,19 @@ def can_access_schema(self, datasource: "BaseDatasource") -> bool: | |
|
||
def can_access_datasource(self, datasource: "BaseDatasource") -> bool: | ||
""" | ||
Return True if the user can access the Superset datasource, False otherwise. | ||
Return True if the user can fully access of the Superset datasource, False | ||
otherwise. | ||
|
||
:param datasource: The Superset datasource | ||
:returns: Whether the user can access the Superset datasource | ||
:returns: Whether the user can fully access the Superset datasource | ||
""" | ||
|
||
return self.can_access_schema(datasource) or self.can_access( | ||
"datasource_access", datasource.perm or "" | ||
) | ||
try: | ||
self.raise_for_access(datasource=datasource) | ||
except SupersetSecurityException: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there's a different exception thrown, then this fails open. Is that secure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etr2460 in theory (by construction) the |
||
return False | ||
|
||
return True | ||
|
||
def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str: | ||
""" | ||
|
@@ -369,45 +376,12 @@ def can_access_table(self, database: "Database", table: "Table") -> bool: | |
:returns: Whether the user can access the SQL table | ||
""" | ||
|
||
from superset import db | ||
from superset.connectors.sqla.models import SqlaTable | ||
|
||
if self.can_access_database(database): | ||
return True | ||
|
||
schema_perm = self.get_schema_perm(database, schema=table.schema) | ||
if schema_perm and self.can_access("schema_access", schema_perm): | ||
return True | ||
|
||
datasources = SqlaTable.query_datasources_by_name( | ||
db.session, database, table.table, schema=table.schema | ||
) | ||
for datasource in datasources: | ||
if self.can_access("datasource_access", datasource.perm): | ||
return True | ||
return False | ||
|
||
def rejected_tables( | ||
self, sql: str, database: "Database", schema: str | ||
) -> Set["Table"]: | ||
""" | ||
Return the list of rejected SQL tables. | ||
|
||
:param sql: The SQL statement | ||
:param database: The SQL database | ||
:param schema: The SQL database schema | ||
:returns: The rejected tables | ||
""" | ||
|
||
from superset.sql_parse import Table | ||
try: | ||
self.raise_for_access(database=database, table=table) | ||
except SupersetSecurityException: | ||
return False | ||
|
||
return { | ||
table | ||
for table in sql_parse.ParsedQuery(sql).tables | ||
if not self.can_access_table( | ||
database, Table(table.table, table.schema or schema) | ||
) | ||
} | ||
return True | ||
|
||
def get_public_role(self) -> Optional[Any]: # Optional[self.role_model] | ||
from superset import conf | ||
|
@@ -860,45 +834,92 @@ def set_perm( | |
) | ||
) | ||
|
||
def assert_datasource_permission(self, datasource: "BaseDatasource") -> None: | ||
def raise_for_access( | ||
self, | ||
database: Optional["Database"] = None, | ||
datasource: Optional["BaseDatasource"] = None, | ||
query: Optional["Query"] = None, | ||
query_context: Optional["QueryContext"] = None, | ||
table: Optional["Table"] = None, | ||
viz: Optional["BaseViz"] = None, | ||
) -> None: | ||
""" | ||
Assert the the user has permission to access the Superset datasource. | ||
Raise an exception if the user cannot access the resource. | ||
|
||
:param database: The Superset database | ||
:param datasource: The Superset datasource | ||
:raises SupersetSecurityException: If the user does not have permission | ||
:param query: The SQL Lab query | ||
:param query_context: The query context | ||
:param table: The Superset table (requires database) | ||
:param viz: The visualization | ||
:raises SupersetSecurityException: If the user cannot access the resource | ||
""" | ||
|
||
if not self.can_access_datasource(datasource): | ||
raise SupersetSecurityException( | ||
self.get_datasource_access_error_object(datasource), | ||
) | ||
from superset.connectors.sqla.models import SqlaTable | ||
from superset.sql_parse import Table | ||
|
||
def assert_query_context_permission(self, query_context: "QueryContext") -> None: | ||
""" | ||
Assert the the user has permission to access the query context. | ||
if database and table or query: | ||
if query: | ||
database = query.database | ||
|
||
:param query_context: The query context | ||
:raises SupersetSecurityException: If the user does not have permission | ||
""" | ||
database = cast("Database", database) | ||
|
||
self.assert_datasource_permission(query_context.datasource) | ||
if self.can_access_database(database): | ||
return | ||
|
||
def assert_viz_permission(self, viz: "BaseViz") -> None: | ||
""" | ||
Assert the the user has permission to access the visualization. | ||
if query: | ||
tables = { | ||
Table(table_.table, table_.schema or query.schema) | ||
for table_ in sql_parse.ParsedQuery(query.sql).tables | ||
} | ||
elif table: | ||
tables = {table} | ||
|
||
:param viz: The visualization | ||
:raises SupersetSecurityException: If the user does not have permission | ||
""" | ||
denied = set() | ||
|
||
for table_ in tables: | ||
schema_perm = self.get_schema_perm(database, schema=table_.schema) | ||
|
||
if not (schema_perm and self.can_access("schema_access", schema_perm)): | ||
datasources = SqlaTable.query_datasources_by_name( | ||
self.get_session, database, table_.table, schema=table_.schema | ||
) | ||
|
||
# Access to any datasource is suffice. | ||
for datasource in datasources: | ||
if self.can_access("datasource_access", datasource.perm): | ||
break | ||
else: | ||
denied.add(table_) | ||
|
||
self.assert_datasource_permission(viz.datasource) | ||
if denied: | ||
raise SupersetSecurityException( | ||
self.get_table_access_error_object(denied), | ||
) | ||
|
||
if datasource or query_context or viz: | ||
if query_context: | ||
datasource = query_context.datasource | ||
elif viz: | ||
datasource = viz.datasource | ||
|
||
assert datasource | ||
|
||
if not ( | ||
self.can_access_schema(datasource) | ||
or self.can_access("datasource_access", datasource.perm or "") | ||
): | ||
raise SupersetSecurityException( | ||
self.get_datasource_access_error_object(datasource), | ||
) | ||
|
||
def get_rls_filters(self, table: "BaseDatasource") -> List[Query]: | ||
def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: | ||
""" | ||
Retrieves the appropriate row level security filters for the current user and the passed table. | ||
Retrieves the appropriate row level security filters for the current user and | ||
the passed table. | ||
|
||
:param table: The table to check against | ||
:returns: A list of filters. | ||
:returns: A list of filters | ||
""" | ||
if hasattr(g, "user") and hasattr(g.user, "id"): | ||
from superset import db | ||
|
@@ -929,10 +950,11 @@ def get_rls_filters(self, table: "BaseDatasource") -> List[Query]: | |
|
||
def get_rls_ids(self, table: "BaseDatasource") -> List[int]: | ||
""" | ||
Retrieves the appropriate row level security filters IDs for the current user and the passed table. | ||
Retrieves the appropriate row level security filters IDs for the current user | ||
and the passed table. | ||
|
||
:param table: The table to check against | ||
:returns: A list of IDs. | ||
:returns: A list of IDs | ||
""" | ||
ids = [f.id for f in self.get_rls_filters(table)] | ||
ids.sort() # Combinations rather than permutations | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not changed in this PR, but this try/catch seems kinda weird, I would've expected this api to be wrapped in the
handle_api_exception
decorator that i believe automatically generates the proper json error response from an exceptionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etr2460 I think this merely highlights the inconsistencies with how backend errors are handled. This is detailed in SIP-41.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etr2460 #9529 kind of addresses that, it's a POC for SIP-41, uses flask error handling