From 6875a1a9e264c04f15736f42848825a25ad2c019 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 26 Apr 2021 08:50:50 -0700 Subject: [PATCH] Make g.user attribute access safe for public users (#14287) --- superset/charts/api.py | 2 +- superset/config.py | 2 +- superset/dashboards/api.py | 4 +++- superset/db_engine_specs/base.py | 2 +- superset/jinja_context.py | 6 +++--- superset/security/manager.py | 4 ++-- superset/sql_validators/presto_db.py | 2 +- superset/views/base_api.py | 5 ++++- superset/views/base_schemas.py | 4 ++-- superset/views/core.py | 27 ++++++++++++++++++--------- superset/views/database/views.py | 4 ++-- superset/views/sql_lab.py | 2 +- 12 files changed, 39 insertions(+), 25 deletions(-) diff --git a/superset/charts/api.py b/superset/charts/api.py index 237b24a2b3..aba386c835 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -954,7 +954,7 @@ class ChartRestApi(BaseSupersetModelRestApi): charts = ChartDAO.find_by_ids(requested_ids) if not charts: return self.response_404() - favorited_chart_ids = ChartDAO.favorited_ids(charts, g.user.id) + favorited_chart_ids = ChartDAO.favorited_ids(charts, g.user.get_id()) res = [ {"id": request_id, "value": request_id in favorited_chart_ids} for request_id in requested_ids diff --git a/superset/config.py b/superset/config.py index 3ddb7cd8e3..7a97b5260f 100644 --- a/superset/config.py +++ b/superset/config.py @@ -409,7 +409,7 @@ FEATURE_FLAGS: Dict[str, bool] = {} # from flask import g, request # def GET_FEATURE_FLAGS_FUNC(feature_flags_dict: Dict[str, bool]) -> Dict[str, bool]: # if hasattr(g, "user") and g.user.is_active: -# feature_flags_dict['some_feature'] = g.user and g.user.id == 5 +# feature_flags_dict['some_feature'] = g.user and g.user.get_id() == 5 # return feature_flags_dict GET_FEATURE_FLAGS_FUNC: Optional[Callable[[Dict[str, bool]], Dict[str, bool]]] = None diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index db807e8daa..64bfd82a86 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -826,7 +826,9 @@ class DashboardRestApi(BaseSupersetModelRestApi): dashboards = DashboardDAO.find_by_ids(requested_ids) if not dashboards: return self.response_404() - favorited_dashboard_ids = DashboardDAO.favorited_ids(dashboards, g.user.id) + favorited_dashboard_ids = DashboardDAO.favorited_ids( + dashboards, g.user.get_id() + ) res = [ {"id": request_id, "value": request_id in favorited_dashboard_ids} for request_id in requested_ids diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index cfc3060f06..7bbc4bd9bd 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -999,7 +999,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods if not cls.get_allow_cost_estimate(extra): raise Exception("Database does not support cost estimation") - user_name = g.user.username if g.user else None + user_name = g.user.username if g.user and hasattr(g.user, "username") else None parsed_query = sql_parse.ParsedQuery(sql) statements = parsed_query.get_statements() diff --git a/superset/jinja_context.py b/superset/jinja_context.py index b86b466e91..54fc09d161 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -142,8 +142,8 @@ class ExtraCache: if hasattr(g, "user") and g.user: if add_to_cache_keys: - self.cache_key_wrapper(g.user.id) - return g.user.id + self.cache_key_wrapper(g.user.get_id()) + return g.user.get_id() return None def current_username(self, add_to_cache_keys: bool = True) -> Optional[str]: @@ -154,7 +154,7 @@ class ExtraCache: :returns: The username """ - if g.user: + if g.user and hasattr(g.user, "username"): if add_to_cache_keys: self.cache_key_wrapper(g.user.username) return g.user.username diff --git a/superset/security/manager.py b/superset/security/manager.py index 73672bb964..de608da165 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -434,7 +434,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods view_menu_names = ( base_query.join(assoc_user_role) .join(self.user_model) - .filter(self.user_model.id == g.user.id) + .filter(self.user_model.id == g.user.get_id()) .filter(self.permission_model.name == permission_name) ).all() return {s.name for s in view_menu_names} @@ -1044,7 +1044,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods user_roles = ( self.get_session.query(assoc_user_role.c.role_id) - .filter(assoc_user_role.c.user_id == g.user.id) + .filter(assoc_user_role.c.user_id == g.user.get_id()) .subquery() ) regular_filter_roles = ( diff --git a/superset/sql_validators/presto_db.py b/superset/sql_validators/presto_db.py index a215ab8eb4..7f468c96fb 100644 --- a/superset/sql_validators/presto_db.py +++ b/superset/sql_validators/presto_db.py @@ -151,7 +151,7 @@ class PrestoDBSQLValidator(BaseSQLValidator): For example, "SELECT 1 FROM default.mytable" becomes "EXPLAIN (TYPE VALIDATE) SELECT 1 FROM default.mytable. """ - user_name = g.user.username if g.user else None + user_name = g.user.username if g.user and hasattr(g.user, "username") else None parsed_query = ParsedQuery(sql) statements = parsed_query.get_statements() diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 11632d67f2..b730ce1266 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -116,7 +116,10 @@ class BaseFavoriteFilter(BaseFilter): # pylint: disable=too-few-public-methods if security_manager.current_user is None: return query users_favorite_query = db.session.query(FavStar.obj_id).filter( - and_(FavStar.user_id == g.user.id, FavStar.class_name == self.class_name) + and_( + FavStar.user_id == g.user.get_id(), + FavStar.class_name == self.class_name, + ) ) if value: return query.filter(and_(self.model.id.in_(users_favorite_query))) diff --git a/superset/views/base_schemas.py b/superset/views/base_schemas.py index 659ec3fdfc..65c9a3567a 100644 --- a/superset/views/base_schemas.py +++ b/superset/views/base_schemas.py @@ -113,8 +113,8 @@ class BaseOwnedSchema(BaseSupersetSchema): @staticmethod def set_owners(instance: Model, owners: List[int]) -> None: owner_objs = list() - if g.user.id not in owners: - owners.append(g.user.id) + if g.user.get_id() not in owners: + owners.append(g.user.get_id()) for owner_id in owners: user = current_app.appbuilder.get_session.query( current_app.appbuilder.sm.user_model diff --git a/superset/views/core.py b/superset/views/core.py index 95ba394b34..cff9e482d7 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1067,8 +1067,10 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods views = [vn for vn in views if substr_parsed in get_datasource_label(vn)] if not schema_parsed and database.default_schemas: - user_schema = g.user.email.split("@")[0] - valid_schemas = set(database.default_schemas + [user_schema]) + user_schemas = ( + [g.user.email.split("@")[0]] if hasattr(g.user, "email") else [] + ) + valid_schemas = set(database.default_schemas + user_schemas) tables = [tn for tn in tables if tn.schema in valid_schemas] views = [vn for vn in views if vn.schema in valid_schemas] @@ -1261,7 +1263,9 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods database.set_sqlalchemy_uri(uri) database.db_engine_spec.mutate_db_for_connection_test(database) - username = g.user.username if g.user is not None else None + username = ( + g.user.username if g.user and hasattr(g.user, "username") else None + ) engine = database.get_sqla_engine(user_name=username) with closing(engine.raw_connection()) as conn: @@ -1515,7 +1519,7 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods ) -> FlaskResponse: """List of slices a user owns, created, modified or faved""" if not user_id: - user_id = g.user.id + user_id = g.user.get_id() owner_ids_query = ( db.session.query(Slice.id) @@ -1567,7 +1571,7 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods ) -> FlaskResponse: """List of slices created by this user""" if not user_id: - user_id = g.user.id + user_id = g.user.get_id() qry = ( db.session.query(Slice) .filter(or_(Slice.created_by_fk == user_id, Slice.changed_by_fk == user_id)) @@ -1595,7 +1599,7 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods ) -> FlaskResponse: """Favorite slices for a user""" if not user_id: - user_id = g.user.id + user_id = g.user.get_id() qry = ( db.session.query(Slice, FavStar.dttm) .join( @@ -1779,8 +1783,9 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods edit_perm = is_owner(dash, g.user) or admin_role in get_user_roles() if not edit_perm: + username = g.user.username if hasattr(g.user, "username") else "user" return json_error_response( - f'ERROR: "{g.user.username}" cannot alter ' + f'ERROR: "{username}" cannot alter ' f'dashboard "{dash.dashboard_title}"', status=403, ) @@ -2304,7 +2309,9 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods rendered_query, return_results=False, store_results=not query.select_as_cta, - user_name=g.user.username if g.user else None, + user_name=g.user.username + if g.user and hasattr(g.user, "username") + else None, start_time=now_as_float(), expand_data=expand_data, log_params=log_params, @@ -2376,7 +2383,9 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods rendered_query, return_results=True, store_results=store_results, - user_name=g.user.username if g.user else None, + user_name=g.user.username + if g.user and hasattr(g.user, "username") + else None, expand_data=expand_data, log_params=log_params, ) diff --git a/superset/views/database/views.py b/superset/views/database/views.py index e3c3f9283f..5236f31c49 100644 --- a/superset/views/database/views.py +++ b/superset/views/database/views.py @@ -219,7 +219,7 @@ class CsvToDatabaseView(SimpleFormView): sqla_table = SqlaTable(table_name=csv_table.table) sqla_table.database = expore_database sqla_table.database_id = database.id - sqla_table.user_id = g.user.id + sqla_table.user_id = g.user.get_id() sqla_table.schema = csv_table.schema sqla_table.fetch_metadata() db.session.add(sqla_table) @@ -360,7 +360,7 @@ class ExcelToDatabaseView(SimpleFormView): sqla_table = SqlaTable(table_name=excel_table.table) sqla_table.database = expore_database sqla_table.database_id = database.id - sqla_table.user_id = g.user.id + sqla_table.user_id = g.user.get_id() sqla_table.schema = excel_table.schema sqla_table.fetch_metadata() db.session.add(sqla_table) diff --git a/superset/views/sql_lab.py b/superset/views/sql_lab.py index 574e87e014..ef3d901d4d 100644 --- a/superset/views/sql_lab.py +++ b/superset/views/sql_lab.py @@ -297,4 +297,4 @@ class SqlLab(BaseSupersetView): @has_access def my_queries(self) -> FlaskResponse: # pylint: disable=no-self-use """Assigns a list of found users to the given role.""" - return redirect("/savedqueryview/list/?_flt_0_user={}".format(g.user.id)) + return redirect("/savedqueryview/list/?_flt_0_user={}".format(g.user.get_id()))