From f993bdc7efcbfe293a62140bf604ed888dddabbd Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Fri, 14 Feb 2020 14:30:49 +0000 Subject: [PATCH] [database] new, select star API migration (#9054) --- superset/security/manager.py | 7 +- superset/views/base_api.py | 11 +++ superset/views/core.py | 27 +++++++- superset/views/database/api.py | 98 +++++++++++++++++++++------ superset/views/database/decorators.py | 61 +++++++++++++++++ tests/core_tests.py | 9 +++ tests/database_api_tests.py | 78 ++++++++++++++++++++- 7 files changed, 266 insertions(+), 25 deletions(-) create mode 100644 superset/views/database/decorators.py diff --git a/superset/security/manager.py b/superset/security/manager.py index d9af4fcc7..59e7b7538 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -310,6 +310,11 @@ class SupersetSecurityManager(SecurityManager): return conf.get("PERMISSION_INSTRUCTIONS_LINK") + def can_access_datasource( + self, database: "Database", table_name: str, schema: str = None + ) -> bool: + return self._datasource_access_by_name(database, table_name, schema=schema) + def _datasource_access_by_name( self, database: "Database", table_name: str, schema: str = None ) -> bool: @@ -520,7 +525,7 @@ class SupersetSecurityManager(SecurityManager): return [d for d in datasource_names if d in names] else: full_names = {d.full_name for d in user_datasources} - return [d for d in datasource_names if d in full_names] + return [d for d in datasource_names if f"[{database}].[{d}]" in full_names] def merge_perm(self, permission_name: str, view_menu_name: str) -> None: """ diff --git a/superset/views/base_api.py b/superset/views/base_api.py index c3099a2ec..412b5ca9c 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -93,6 +93,14 @@ class BaseSupersetModelRestApi(ModelRestApi): } """ # pylint: disable=pointless-string-statement + def __init__(self): + super().__init__() + self.stats_logger = None + + def create_blueprint(self, appbuilder, *args, **kwargs): + self.stats_logger = self.appbuilder.get_app.config["STATS_LOGGER"] + return super().create_blueprint(appbuilder, *args, **kwargs) + def _init_properties(self): model_id = self.datamodel.get_pk_name() if self.list_columns is None and not self.list_model_schema: @@ -114,6 +122,9 @@ class BaseSupersetModelRestApi(ModelRestApi): ) return filters + def incr_stats(self, action: str, func_name: str) -> None: + self.stats_logger.incr(f"{self.__class__.__name__}.{func_name}.{action}") + @expose("/related/", methods=["GET"]) @protect() @safe diff --git a/superset/views/core.py b/superset/views/core.py index bbd50e3c7..c127ce722 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1939,11 +1939,34 @@ class Superset(BaseSupersetView): @expose("/select_star///") @event_logger.log_this def select_star(self, database_id, table_name, schema=None): - mydb = db.session.query(models.Database).get(database_id) + logging.warning( + f"{self.__class__.__name__}.select_star " + "This API endpoint is deprecated and will be removed in version 1.0.0" + ) + stats_logger.incr(f"{self.__class__.__name__}.select_star.init") + database = db.session.query(models.Database).get(database_id) + if not database: + stats_logger.incr( + f"deprecated.{self.__class__.__name__}.select_star.database_not_found" + ) + return json_error_response("Not found", 404) schema = utils.parse_js_uri_path_item(schema, eval_undefined=True) table_name = utils.parse_js_uri_path_item(table_name) + # Check that the user can access the datasource + if not self.appbuilder.sm.can_access_datasource(database, table_name, schema): + stats_logger.incr( + f"deprecated.{self.__class__.__name__}.select_star.permission_denied" + ) + logging.warning( + f"Permission denied for user {g.user} on table: {table_name} " + f"schema: {schema}" + ) + return json_error_response("Not found", 404) + stats_logger.incr(f"deprecated.{self.__class__.__name__}.select_star.success") return json_success( - mydb.select_star(table_name, schema, latest_partition=True, show_cols=True) + database.select_star( + table_name, schema, latest_partition=True, show_cols=True + ) ) @has_access_api diff --git a/superset/views/database/api.py b/superset/views/database/api.py index bbf60a2ea..9a846ceaf 100644 --- a/superset/views/database/api.py +++ b/superset/views/database/api.py @@ -18,13 +18,13 @@ from typing import Any, Dict, List, Optional from flask_appbuilder.api import expose, protect, safe from flask_appbuilder.models.sqla.interface import SQLAInterface -from flask_babel import lazy_gettext as _ -from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.exc import NoSuchTableError, SQLAlchemyError from superset import event_logger from superset.models.core import Database -from superset.utils.core import error_msg_from_exception, parse_js_uri_path_item +from superset.utils.core import error_msg_from_exception from superset.views.base_api import BaseSupersetModelRestApi +from superset.views.database.decorators import check_datasource_access from superset.views.database.filters import DatabaseFilter from superset.views.database.mixins import DatabaseMixin from superset.views.database.validators import sqlalchemy_uri_validator @@ -112,9 +112,13 @@ def get_table_metadata( class DatabaseRestApi(DatabaseMixin, BaseSupersetModelRestApi): datamodel = SQLAInterface(Database) - include_route_methods = {"get_list", "table_metadata"} + include_route_methods = {"get_list", "table_metadata", "select_star"} class_permission_name = "DatabaseView" - method_permission_name = {"get_list": "list", "table_metadata": "list"} + method_permission_name = { + "get_list": "list", + "table_metadata": "list", + "select_star": "list", + } resource_name = "database" allow_browser_login = True base_filters = [["id", DatabaseFilter, lambda: []]] @@ -143,11 +147,10 @@ class DatabaseRestApi(DatabaseMixin, BaseSupersetModelRestApi): "//table///", methods=["GET"] ) @protect() + @check_datasource_access @safe @event_logger.log_this - def table_metadata( - self, pk: int, table_name: str, schema_name: str - ): # pylint: disable=invalid-name + def table_metadata(self, database: Database, table_name: str, schema_name: str): """ Table schema info --- get: @@ -262,19 +265,74 @@ class DatabaseRestApi(DatabaseMixin, BaseSupersetModelRestApi): 500: $ref: '#/components/responses/500' """ - table_name_parsed = parse_js_uri_path_item(table_name) - schema_parsed = parse_js_uri_path_item(schema_name, eval_undefined=True) - # schemas can be None but not tables - if not table_name_parsed: - return self.response_422(message=_(f"Could not parse table name or schema")) - database: Database = self.datamodel.get(pk, self._base_filters) - if not database: - return self.response_404() - + self.incr_stats("init", self.table_metadata.__name__) try: - table_info: Dict = get_table_metadata( - database, table_name_parsed, schema_parsed - ) + table_info: Dict = get_table_metadata(database, table_name, schema_name) except SQLAlchemyError as e: + self.incr_stats("error", self.table_metadata.__name__) return self.response_422(error_msg_from_exception(e)) + self.incr_stats("success", self.table_metadata.__name__) return self.response(200, **table_info) + + @expose("//select_star//", methods=["GET"]) + @expose( + "//select_star///", + methods=["GET"], + ) + @protect() + @check_datasource_access + @safe + @event_logger.log_this + def select_star(self, database: Database, table_name: str, schema_name: str = None): + """ Table schema info + --- + get: + description: Get database select star for table + parameters: + - in: path + schema: + type: integer + name: pk + description: The database id + - in: path + schema: + type: string + name: table_name + description: Table name + - in: path + schema: + type: string + name: schema_name + description: Table schema + responses: + 200: + description: select star for table + content: + text/plain: + schema: + type: object + properties: + result: + type: string + description: SQL select star + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + self.incr_stats("init", self.select_star.__name__) + try: + result = database.select_star( + table_name, schema_name, latest_partition=True, show_cols=True + ) + except NoSuchTableError: + self.incr_stats("error", self.select_star.__name__) + return self.response(404, message="Table not found on the database") + self.incr_stats("success", self.select_star.__name__) + return self.response(200, result=result) diff --git a/superset/views/database/decorators.py b/superset/views/database/decorators.py new file mode 100644 index 000000000..ea72a3017 --- /dev/null +++ b/superset/views/database/decorators.py @@ -0,0 +1,61 @@ +# 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 functools +import logging + +from flask import g +from flask_babel import lazy_gettext as _ + +from superset.models.core import Database +from superset.utils.core import parse_js_uri_path_item + +logger = logging.getLogger(__name__) + + +def check_datasource_access(f): + """ + A Decorator that checks if a user has datasource access + """ + + def wraps( + self, pk: int, table_name: str, schema_name: str = None + ): # pylint: disable=invalid-name + schema_name_parsed = parse_js_uri_path_item(schema_name, eval_undefined=True) + table_name_parsed = parse_js_uri_path_item(table_name) + if not table_name_parsed: + return self.response_422(message=_("Table name undefined")) + database: Database = self.datamodel.get(pk) + if not database: + self.stats_logger.incr( + f"database_not_found_{self.__class__.__name__}.select_star" + ) + return self.response_404() + # Check that the user can access the datasource + if not self.appbuilder.sm.can_access_datasource( + database, table_name_parsed, schema_name_parsed + ): + self.stats_logger.incr( + f"permisssion_denied_{self.__class__.__name__}.select_star" + ) + logger.warning( + f"Permission denied for user {g.user} on table: {table_name_parsed} " + f"schema: {schema_name_parsed}" + ) + return self.response_404() + return f(self, database, table_name_parsed, schema_name_parsed) + + return functools.update_wrapper(wraps, f) diff --git a/tests/core_tests.py b/tests/core_tests.py index e51712bec..3e64e92f7 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -860,6 +860,15 @@ class CoreTests(SupersetTestCase): resp = self.get_resp(f"/superset/select_star/{examples_db.id}/birth_names") self.assertIn("gender", resp) + def test_get_select_star_not_allowed(self): + """ + Database API: Test get select star not allowed + """ + self.login(username="gamma") + example_db = utils.get_example_database() + resp = self.client.get(f"/superset/select_star/{example_db.id}/birth_names") + self.assertEqual(resp.status_code, 404) + @mock.patch("superset.views.core.results_backend_use_msgpack", False) @mock.patch("superset.views.core.results_backend") @mock.patch("superset.views.core.db") diff --git a/tests/database_api_tests.py b/tests/database_api_tests.py index adfc43f64..0360f6274 100644 --- a/tests/database_api_tests.py +++ b/tests/database_api_tests.py @@ -19,9 +19,10 @@ import json import prison +from sqlalchemy.sql import func -import tests.test_app -from superset import db +from superset import db, security_manager +from superset.connectors.sqla.models import SqlaTable from superset.models.core import Database from superset.utils.core import get_example_database @@ -142,3 +143,76 @@ class DatabaseApiTests(SupersetTestCase): uri = f"api/v1/database/{example_db.id}/birth_names/null/" rv = self.client.get(uri) self.assertEqual(rv.status_code, 404) + + def test_get_select_star(self): + """ + Database API: Test get select star + """ + self.login(username="admin") + example_db = get_example_database() + uri = f"api/v1/database/{example_db.id}/select_star/birth_names/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + response = json.loads(rv.data.decode("utf-8")) + self.assertIn("gender", response["result"]) + + def test_get_select_star_not_allowed(self): + """ + Database API: Test get select star not allowed + """ + self.login(username="gamma") + example_db = get_example_database() + uri = f"api/v1/database/{example_db.id}/select_star/birth_names/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 404) + + def test_get_select_star_datasource_access(self): + """ + Database API: Test get select star with datasource access + """ + session = db.session + table = SqlaTable( + schema="main", table_name="ab_permission", database=get_example_database() + ) + session.add(table) + session.commit() + + tmp_table_perm = security_manager.find_permission_view_menu( + "datasource_access", table.get_perm() + ) + gamma_role = security_manager.find_role("Gamma") + security_manager.add_permission_role(gamma_role, tmp_table_perm) + + self.login(username="gamma") + example_db = get_example_database() + uri = f"api/v1/database/{example_db.id}/select_star/ab_permission/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + + # rollback changes + security_manager.del_permission_role(gamma_role, tmp_table_perm) + db.session.delete(table) + db.session.commit() + + def test_get_select_star_not_found_database(self): + """ + Database API: Test get select star not found database + """ + self.login(username="admin") + max_id = db.session.query(func.max(Database.id)).scalar() + uri = f"api/v1/database/{max_id + 1}/select_star/birth_names/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 404) + + def test_get_select_star_not_found_table(self): + """ + Database API: Test get select star not found database + """ + self.login(username="admin") + example_db = get_example_database() + # sqllite will not raise a NoSuchTableError + if example_db.backend == "sqlite": + return + uri = f"api/v1/database/{example_db.id}/select_star/table_does_not_exist/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 404)